This is an archive of the discontinued Mercurial Phabricator instance.

copies: consider nullrev a common ancestor
ClosedPublic

Authored by martinvonz on Jan 15 2019, 7:27 PM.

Details

Summary

I've seen many bugs in the git codebase that were caused by it not
having a null revision and being forced to treat root commits
differently. Mercurial has a null revision and I think it's generally
a bug to treat it differently from other commits in graph algorithms.

This effectively undoes 83cfa1baf8ad (copies: don't report copies with
unrelated branch, 2010-01-01). The test cases that that commit added
still passes. I suspect some other fix after that commit made it
unnecessary.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Jan 15 2019, 7:27 PM
yuja added a subscriber: yuja.Jan 18 2019, 7:14 AM
I've seen many bugs in the git codebase that were caused by it not
having a null revision and being forced to treat root commits
differently. Mercurial has a null revision and I think it's generally
a bug to treat it differently from other commits in graph algorithms.

I agree with that, but I don't think every non-merge revision (p2 = null)
should be considered a direct child of null revision.

@@ -65,8 +63,6 @@

else:
    parents = cl.parentrevs(r)

Here parents[1] may be nullrev. Is that expected?

for p in parents:
  • if p < 0:
  • continue
In D5594#83078, @yuja wrote:
I've seen many bugs in the git codebase that were caused by it not
having a null revision and being forced to treat root commits
differently. Mercurial has a null revision and I think it's generally
a bug to treat it differently from other commits in graph algorithms.

I agree with that, but I don't think every non-merge revision (p2 = null)
should be considered a direct child of null revision.

@@ -65,8 +63,6 @@

else:
    parents = cl.parentrevs(r)

Here parents[1] may be nullrev. Is that expected?

for p in parents:
  • if p < 0:
  • continue

There are definitely repositories in the wild where p1 is nullrev (and p2 is not). It's unusual but expressable so, of course, it happened.

For that matters, there is also case with nullrev != p1 && p1 == p2.

yuja added a comment.Jan 18 2019, 9:01 AM
There are definitely repositories in the wild where p1 is nullrev (and p2 is not). It's unusual but expressable so, of course, it happened.
For that matters, there is also case with nullrev != p1 && p1 == p2.

Well, I think it should be considered a sort of corruption. context.py for
example would go wrong.

Maybe we can add some check to "hg verify".

In D5594#83078, @yuja wrote:
I've seen many bugs in the git codebase that were caused by it not
having a null revision and being forced to treat root commits
differently. Mercurial has a null revision and I think it's generally
a bug to treat it differently from other commits in graph algorithms.

I agree with that, but I don't think every non-merge revision (p2 = null)
should be considered a direct child of null revision.

@@ -65,8 +63,6 @@

else:
    parents = cl.parentrevs(r)

Here parents[1] may be nullrev. Is that expected?

Oh, good catch! I'll fix that.

It seems like a more natural API to have parents() methods return only valid parents, but I suppose that may just be too large a change to make.

for p in parents:
  • if p < 0:
  • continue
martinvonz updated this revision to Diff 13434.Jan 24 2019, 6:57 PM
This revision was automatically updated to reflect the committed changes.