martinvonz added inline comments.


>                              _('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)?

> +    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.

> +    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.

> +    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 

  $ hg debugdrawdag <<'EOF'
  >   D
  >  /|
  > B C
  >  \|
  >   A E
  >   |/
  >   Q
  > EOF
  $ hg rebase -d E -r 'B + C + D'
  $ hg manifest --rev tip

> +        # 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?
    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?

> +        # 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?

> +            # 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)

  rHG Mercurial


To: quark, durin42, #hg-reviewers
Cc: martinvonz, durin42, mercurial-devel
Mercurial-devel mailing list

Reply via email to