martinvonz added inline comments.
INLINE COMMENTS
> rebase.py:392
> _('changesets'), total)
> - p1, p2, base = defineparents(repo, rev, self.dest,
> - self.state,
> - self.destancestors,
> - self.obsoletenotrebased)
> + p1, p2, base = defineparents(repo, rev, self.dest,
> self.state)
> self.storestatus(tr=tr)
IIUC, the reason you no longer need to pass destancestors and
obsoletenotrebased is because they are now calculated on the fly instead of
upfront. It seems like that might be slower, but I haven't spent time thinking
about under what circumstances it might be. Have you? Could it be noticeable
even in the worst case you can think of, or would it be completely drowned by
rebase's other costs (such as writing revlogs and working copy files)?
> rebase.py:1004
> + cl = repo.changelog
> + def ancestor(rev1, rev2):
> + # take revision numbers instead of nodes
Should this be a wrapper of cl.isancestor() instead? Looks like that's how you
use it.
> rebase.py:1009
> + oldps = repo.changelog.parentrevs(rev) # old parents
> + newps = list(oldps) # new parents
> + bases = set() # merge base candidates
nit: Is this for converting to a list or for making a copy (of something that's
already a list)? If the latter, I feel like oldps[:} is clearer.
> rebase.py:1010
> + newps = list(oldps) # new parents
> + bases = set() # merge base candidates
> + dests = adjustdest(repo, rev, dest, state)
I think each step of the rebase effectively applies the diff in (rev - base)
onto p1 (and p2 is only artificially added later to produce the graph we want),
so I think we need to keep better track of the bases than using a set.
Here's a test that will fail after this patch (the manifest will contain A as
well):
$ hg debugdrawdag <<'EOF'
> D
> /|
> B C
> \|
> A E
> |/
> Q
> EOF
$ hg rebase -d E -r 'B + C + D'
$ hg manifest --rev tip
B
C
E
Q
> rebase.py:1019
> + # For non-merge changeset, just move p to adjusted dest as requested.
> + if oldps[1] == nullrev:
> + newps[i] = dests[i]
This looks a bit weird. Would it work the same if you move it outside?
Something like:
if oldps[1] == nullrev:
assert dests[1] == nullrev
newps = dests
if newps != oldps: # maybe always true?
bases.add(oldps[0])
else:
newps = oldps[:]
for i, p in enumerate(oldps):
....
Having written that and looked at adjustdest(), I see that the assertion would
fail because adjustdest() would return [dest,dest] for a non-merge source. That
seems surprising to me. Should adjustdest() be changed not to do that?
> rebase.py:1025
> + # parent", which is a surprise. This is a limit because "--dest" only
> + # accepts one dest per src.
> + #
I don't quite understand the responsibility of adjustdest() vs this method. Why
would or why would we not let adjustdest() handle the merge cases too instead
of doing that here?
> rebase.py:1057
> + # right thing.
> + newps.reverse()
>
Here's another case where I believe bases should also be updated (they should
be reversed).
> test-rebase-obsolete.t:1109
> +
> +Rebased F should have one parent, just like in the test case above
> +
nit: seems unnecessary to even mention this now (the other tests don't)
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D21
To: quark, durin42, #hg-reviewers
Cc: martinvonz, durin42, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
