[PATCH 4 of 4] rebase: exclude obsoletes without a successor in destination (issue5300)
Martin von Zweigbergk
martinvonz at google.com
Tue Nov 14 12:00:35 EST 2017
On Tue, Nov 14, 2017 at 3:24 AM, Denis Laxalde <denis at laxalde.org> wrote:
> Martin von Zweigbergk a écrit :
>
>> On Mon, Nov 13, 2017 at 3:07 PM, Augie Fackler <raf at durin42.com> wrote:
>>
>> On Sun, Nov 12, 2017 at 06:06:37PM +0100, Denis Laxalde wrote:
>>>
>>>> # HG changeset patch
>>>> # User Denis Laxalde <denis at laxalde.org>
>>>> # Date 1510489041 -3600
>>>> # Sun Nov 12 13:17:21 2017 +0100
>>>> # Node ID 25d1891bd300bc527cb7e878532c8512ff5489de
>>>> # Parent 1d16e28225e01d2fac9f31ed058ae1f4c00bb9da
>>>> # EXP-Topic rebase-obsolete
>>>> rebase: exclude obsoletes without a successor in destination (issue5300)
>>>>
>>>
>>>
>> If I read it right, this line should be something like "rebase: exclude
>> *descendants of* obsoletes without..."
>>
>
> That's correct here and in the release note below.
>
>
>
>>
>>> I took patches 1-3. I think this is right. I'm 90% sure this is
>>> right. But I'm a little tired, and so I want Martin's more careful eye
>>> on it if he has time.
>>>
>>>
>> Thanks for asking. I think this makes sense. Thanks for working on this,
>> Denis!
>>
>> I think a general thing to note is that if a commit in the rebase set has
>> become obsolete, then there's a conflict between the rebase invocation and
>> the obsmarkers. The rebase command tells us to move the descendants onto
>> the rebase destination, while the obsmarkers tell us to move them onto the
>> obsolete commit's successor. Since we don't keep track of some default
>> rebase destination (like git's upstream concept), any "hg rebase"
>> invocation is a one-off as far as hg is concerned, so I think it makes
>> sense to resolve the conflict in favor of the obsmarkers. That probably
>> didn't make much sense, so let me give an example:
>>
>> o B'
>> |
>> o O
>> |
>> | o U
>> | |
>> | | o C
>> | | |
>> | | x B
>> | |/
>> | .
>> |/
>> o A
>>
>> Let's say C's upstream was somehow configured to be U here, and that B had
>> been rebased onto O instead. If the user then runs "hg evolve -r C", you
>> could argue that there would be a similar conflict between rebase/upstream
>> that want to move C onto U and obsmarkers that want to move C onto B'.
>> However, since we don't have that feature, B' is the only natural
>> destination.
>>
>
> I think I get this right, but I'm not sure if you actually expect me to
> change something (apart for other explicit comments, which I'll address)?
>
Not asking for any changes. I just thought I'd mention an aspect I had not
thought about before.
> Also, the change proposed here handles cases where we cannot rebase C
> onto B' because both B and B' are part of the rebase set (and B' is not
> in destination).
>
>
>
>>
>>>> In the following example, when trying to rebase 3:: onto 2, the rebase
>>>> will abort with "this rebase will cause divergence from: 4":
>>>>
>>>> o 7 f
>>>> |
>>>> | o 6 e
>>>> | |
>>>> | o 5 d'
>>>> | |
>>>> x | 4 d (rewritten as 5)
>>>> |/
>>>> o 3 c
>>>> |
>>>> | o 2 x
>>>> | |
>>>> o | 1 b
>>>> |/
>>>> o 0 a
>>>>
>>>> By excluding obsolete changesets without a successor in destination (4
>>>> in the example above) and their descendants, we make rebase work in this
>>>> case, thus giving:
>>>>
>>>> o 11 e
>>>> |
>>>> o 10 d'
>>>> |
>>>> o 9 c
>>>> |
>>>> o 8 b
>>>> |
>>>> | o 7 f
>>>> | |
>>>> | | x 6 e (rewritten using rebase as 11)
>>>> | | |
>>>> | | x 5 d' (rewritten using rebase as 10)
>>>> | | |
>>>> | x | 4 d
>>>> | |/
>>>> | x 3 c (rewritten using rebase as 9)
>>>> | |
>>>> o | 2 x
>>>> | |
>>>> | x 1 b (rewritten using rebase as 8)
>>>> |/
>>>> o 0 a
>>>>
>>>> where branch 4:: is left behind while branch 5:: is rebased as expected.
>>>>
>>>> The rationale is that users may not be interested in rebasing orphan
>>>> changesets when specifying a rebase set that include them but would
>>>> still want "stable" ones to be rebased. Currently, the user is suggested
>>>> to allow divergence (but probably does not want it) or they must specify
>>>> a rebase set excluding problematic changesets (which might be a bit
>>>> cumbersome). The approach proposed here corresponds to "Option 2" in
>>>> https://www.mercurial-scm.org/wiki/CEDRebase.
>>>>
>>>
>>>
>> I think I'd still want "Option 3", but I don't think this patch prevents
>> that from happening later.
>>
>>
>>
>>>> .. feature::
>>>>
>>>> Let 'hg rebase' avoid content-divergence when obsolete changesets
>>>> are
>>>> present in the rebase set by skipping possible sources of
>>>> divergence.
>>>>
>>>
>>>
>> Similar to what I said above, I think it would help to mention descendants
>> here. While "possible sources of divergence" is vague enough to be
>> correct,
>> it would be helpful if we could clarify it.
>>
>>
>>>
>>>>
>>>
>> Just to make sure, the stuff below is not part of the release notes, is
>> it?
>> I'm not familiar with the syntax, so I can't tell (and I'm too lazy to
>> test
>> it).
>>
>
> The directive block only includes indented lines, so by de-denting the
> text below, it will not be in the release note.
>
>
> We extend _computeobsoletenotrebased() so that it also return a set of
>>>
>>>> obsolete changesets in rebaseset without a successor in destination.
>>>> This 'obsoletewithoutsuccessorindestination' is then stored as an
>>>> attribute of rebaseruntime and used in _performrebasesubset() to:
>>>>
>>>> * filter out descendants of these changesets from the revisions to
>>>> rebase;
>>>> * issue a message about these revisions being skipped.
>>>>
>>>> This only occurs if 'evolution.allowdivergence' option is off and
>>>> 'rebaseskipobsolete' is on.
>>>>
>>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>>> --- a/hgext/rebase.py
>>>> +++ b/hgext/rebase.py
>>>> @@ -179,6 +179,7 @@ class rebaseruntime(object):
>>>> # other extensions
>>>> self.keepopen = opts.get('keepopen', False)
>>>> self.obsoletenotrebased = {}
>>>> + self.obsoletewithoutsuccessorindestination = set()
>>>>
>>>> @property
>>>> def repo(self):
>>>> @@ -311,9 +312,10 @@ class rebaseruntime(object):
>>>> if not self.ui.configbool('experimental',
>>>>
>>> 'rebaseskipobsolete'):
>>>
>>>> return
>>>> obsoleteset = set(obsoleterevs)
>>>> - self.obsoletenotrebased = _computeobsoletenotrebased(sel
>>>> f.repo,
>>>> - obsoleteset, destmap)
>>>> + self.obsoletenotrebased, self.obsoletewithoutsuccessori
>>>> ndestination
>>>>
>>> = \
>>>
>>>> + _computeobsoletenotrebased(self.repo, obsoleteset,
>>>> destmap)
>>>> skippedset = set(self.obsoletenotrebased)
>>>> + skippedset.update(self.obsoletewithoutsuccessorindestination)
>>>> _checkobsrebase(self.repo, self.ui, obsoleteset, skippedset)
>>>>
>>>> def _prepareabortorcontinue(self, isabort):
>>>> @@ -419,12 +421,26 @@ class rebaseruntime(object):
>>>> def _performrebasesubset(self, tr, subset, pos, total):
>>>> repo, ui, opts = self.repo, self.ui, self.opts
>>>> sortedrevs = repo.revs('sort(%ld, -topo)', subset)
>>>> + allowdivergence = self.ui.configbool(
>>>> + 'experimental', 'evolution.allowdivergence')
>>>> + if not allowdivergence:
>>>> + sortedrevs -= repo.revs(
>>>> + 'descendants(%ld) and not %ld',
>>>> + self.obsoletewithoutsuccessorindestination,
>>>> + self.obsoletewithoutsuccessorindestination,
>>>> + )
>>>> for rev in sortedrevs:
>>>> dest = self.destmap[rev]
>>>> ctx = repo[rev]
>>>> desc = _ctxdesc(ctx)
>>>> if self.state[rev] == rev:
>>>> ui.status(_('already rebased %s\n') % desc)
>>>> + elif (not allowdivergence
>>>> + and rev in self.obsoletewithoutsuccessori
>>>>
>>> ndestination):
>>>
>>>> + msg = _('note: not rebasing %s and its descendants as '
>>>> + 'this would cause divergences\n') % desc
>>>> + repo.ui.status(msg)
>>>> + self.skipped.add(rev)
>>>> elif rev in self.obsoletenotrebased:
>>>> succ = self.obsoletenotrebased[rev]
>>>> if succ is None:
>>>> @@ -1616,11 +1632,16 @@ def _filterobsoleterevs(repo, revs):
>>>> return set(r for r in revs if repo[r].obsolete())
>>>>
>>>> def _computeobsoletenotrebased(repo, rebaseobsrevs, destmap):
>>>> - """return a mapping obsolete => successor for all obsolete nodes to
>>>>
>>> be
>>>
>>>> - rebased that have a successors in the destination
>>>> + """Return (obsoletenotrebased, obsoletewithoutsuccessorindest
>>>>
>>> ination).
>>>
>>>> +
>>>> + `obsoletenotrebased` is a mapping mapping obsolete => successor for
>>>>
>>> all
>>>
>>>> + obsolete nodes to be rebased given in `rebaseobsrevs`.
>>>>
>>>> - obsolete => None entries in the mapping indicate nodes with no
>>>>
>>> successor"""
>>>
>>>> + `obsoletewithoutsuccessorindestination` is a set with obsolete
>>>>
>>> revisions
>>>
>>>> + without a successor in destination.
>>>> + """
>>>> obsoletenotrebased = {}
>>>> + obsoletewithoutsuccessorindestination = set([])
>>>>
>>>> assert repo.filtername is None
>>>> cl = repo.changelog
>>>> @@ -1641,8 +1662,17 @@ def _computeobsoletenotrebased(repo, reb
>>>> if cl.isancestor(succnode, destnode):
>>>> obsoletenotrebased[srcrev] = nodemap[succnode]
>>>> break
>>>> + else:
>>>> + # Unless 'srcrev' only has one successor and it's not
>>>> in
>>>> + # rebaseset, we might skip it and its descendants as it
>>>>
>>> has no
>>>
>>>> + # successor in destination. Otherwise, we'll issue the
>>>> + # divergence warning and abort.
>>>>
>>>
Hmm, that actually doesn't sound like "successor in destination", it sounds
like "no successor in rebase set". It sounds like it'll miss this case:
$ hg hg rebase -b C -d E
o E
|
o B''
|
| o D
| |
| x B'
| |
| | o C
| | |
| | x B
| |/
| o A
|/
o X
I think it would be nice to *not* skip C or D in this case (IIUC, C would
be skipped by this patch). Maybe it's enough to check only the immediate
visible successors? I.e. using the "closest=True" thing that I think Boris
added recently.
> + if (len(successors) > 1
>>>>
>>>
>>>
>> This line doesn't seem necessary, but feel free to keep if you think it
>> helps readability (I find it slightly distracting).
>>
>
> You're right, it's not needed.
>
>
>
>>
>>> + and any(nodemap[s] in destmap
>>>
>>>> + for s in successors if s != srcnode)):
>>>> + obsoletewithoutsuccessorindestination.add(srcrev)
>>>>
>>>
This does not seem to check the "only has one successor" that you
described. I'm not sure what the reason for that condition is, so I don't
know if the code or the description should be fixed (or if I just
misunderstood). I saw some comment suggesting that we don't care much about
splits yet, so I suppose the "only has one successor" is not about that?
>>>> - return obsoletenotrebased
>>>> + return obsoletenotrebased, obsoletewithoutsuccessorindestination
>>>>
>>>> def summaryhook(ui, repo):
>>>> if not repo.vfs.exists('rebasestate'):
>>>> diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
>>>> --- a/tests/test-rebase-obsolete.t
>>>> +++ b/tests/test-rebase-obsolete.t
>>>> @@ -987,6 +987,208 @@ Create the changes that we will rebase
>>>> rebasing 21:7bdc8a87673d "dummy change" (tip)
>>>> $ cd ..
>>>>
>>>> +Divergence cases due to obsolete changesets
>>>> +-------------------------------------------
>>>> +
>>>> +We should ignore branches with unstable changesets when they are based
>>>>
>>> on an
>>>
>>>> +obsolete changeset which successor is in rebase set.
>>>> +
>>>> + $ hg init divergence
>>>> + $ cd divergence
>>>> + $ cat >> .hg/hgrc << EOF
>>>> + > [extensions]
>>>> + > strip =
>>>> + > [alias]
>>>> + > strip = strip --no-backup --quiet
>>>> + > [templates]
>>>> + > instabilities = '{rev}:{node|short} {desc|firstline}{if(instabilit
>>>> ies,"
>>>>
>>> ({instabilities})")}\n'
>>>
>>>> + > EOF
>>>> +
>>>> + $ hg debugdrawdag <<EOF
>>>> + > e f
>>>> + > | |
>>>> + > d' d # replace: d -> d'
>>>> + > \ /
>>>> + > c
>>>> + > |
>>>> + > x b
>>>> + > \|
>>>> + > a
>>>> + > EOF
>>>> + $ hg log -G -r 'a'::
>>>> + o 7:1143e9adc121 f
>>>> + |
>>>> + | o 6:d60ebfa0f1cb e
>>>> + | |
>>>> + | o 5:027ad6c5830d d'
>>>> + | |
>>>> + x | 4:76be324c128b d (rewritten using replace as 5:027ad6c5830d)
>>>> + |/
>>>> + o 3:a82ac2b38757 c
>>>> + |
>>>> + | o 2:630d7c95eff7 x
>>>> + | |
>>>> + o | 1:488e1b7e7341 b
>>>> + |/
>>>> + o 0:b173517d0057 a
>>>> +
>>>> +
>>>> +Changeset d and its descendants are excluded to avoid divergence of d,
>>>>
>>> which
>>>
>>>> +would occur because the successor of d (d') is also in rebaseset. As a
>>>> +consequence f (descendant of d) is left behind.
>>>> +
>>>> + $ hg rebase -b 'e' -d 'x'
>>>> + rebasing 1:488e1b7e7341 "b" (b)
>>>> + rebasing 3:a82ac2b38757 "c" (c)
>>>> + rebasing 5:027ad6c5830d "d'" (d')
>>>> + rebasing 6:d60ebfa0f1cb "e" (e)
>>>> + note: not rebasing 4:76be324c128b "d" (d) and its descendants as this
>>>>
>>> would cause divergences
>>>
>>>
>> The plural "divergences" sounds wrong to me, but I'm not sure. Augie (or
>> others), since you're a native English speaker, do you think it should be
>> singular or plural?
>>
>
> It feels strange to me as well. I took it from the existing message
> "this rebase will cause divergences from: <obsolete>" (which, maybe, was
> also written by a non-native).
>
>
>
>> + $ hg log -G -r 'a'::
>>>> + o 11:eb6d63fc4ed5 e
>>>> + |
>>>> + o 10:44d8c724a70c d'
>>>> + |
>>>> + o 9:d008e6b4d3fd c
>>>> + |
>>>> + o 8:67e8f4a16c49 b
>>>> + |
>>>> + | o 7:1143e9adc121 f
>>>> + | |
>>>> + | | x 6:d60ebfa0f1cb e (rewritten using rebase as 11:eb6d63fc4ed5)
>>>> + | | |
>>>> + | | x 5:027ad6c5830d d' (rewritten using rebase as 10:44d8c724a70c)
>>>> + | | |
>>>> + | x | 4:76be324c128b d (rewritten using replace as 5:027ad6c5830d)
>>>> + | |/
>>>> + | x 3:a82ac2b38757 c (rewritten using rebase as 9:d008e6b4d3fd)
>>>> + | |
>>>> + o | 2:630d7c95eff7 x
>>>> + | |
>>>> + | x 1:488e1b7e7341 b (rewritten using rebase as 8:67e8f4a16c49)
>>>> + |/
>>>> + o 0:b173517d0057 a
>>>> +
>>>> + $ hg strip -r 8:
>>>> +
>>>> +If the rebase set has an obsolete (d) with a unique successor (d')
>>>>
>>> outside the
>>>
>>>> +rebase set and not in destination, we still get the divergence warning.
>>>>
>>> By
>>>
>>>> +allowing divergence, we can perform the rebase.
>>>> +
>>>> + $ hg rebase -r 'c'::'f' -d 'x'
>>>> + abort: this rebase will cause divergences from: 76be324c128b
>>>> + (to force the rebase please set experimental.evolution.allowdi
>>>>
>>> vergence=True)
>>>
>>>> + [255]
>>>> + $ hg rebase --config experimental.evolution.allowdivergence=true -r
>>>>
>>> 'c'::'f' -d 'x'
>>>
>>>> + rebasing 3:a82ac2b38757 "c" (c)
>>>> + rebasing 4:76be324c128b "d" (d)
>>>> + rebasing 7:1143e9adc121 "f" (f tip)
>>>> + $ hg log -G -r 'a':: -T instabilities
>>>> + o 10:e1744ea07510 f
>>>> + |
>>>> + o 9:e2b36ea9a0a0 d (content-divergent)
>>>> + |
>>>> + o 8:6a0376de376e c
>>>> + |
>>>> + | x 7:1143e9adc121 f
>>>> + | |
>>>> + | | o 6:d60ebfa0f1cb e (orphan)
>>>> + | | |
>>>> + | | o 5:027ad6c5830d d' (orphan content-divergent)
>>>> + | | |
>>>> + | x | 4:76be324c128b d
>>>> + | |/
>>>> + | x 3:a82ac2b38757 c
>>>> + | |
>>>> + o | 2:630d7c95eff7 x
>>>> + | |
>>>> + | o 1:488e1b7e7341 b
>>>> + |/
>>>> + o 0:b173517d0057 a
>>>> +
>>>> + $ hg strip -r 8:
>>>> +
>>>> +(Not skipping obsoletes means that divergence is allowed.)
>>>> +
>>>> + $ hg rebase --config experimental.rebaseskipobsolete=false -r
>>>>
>>> 'c'::'f' -d 'x'
>>>
>>>> + rebasing 3:a82ac2b38757 "c" (c)
>>>> + rebasing 4:76be324c128b "d" (d)
>>>> + rebasing 7:1143e9adc121 "f" (f tip)
>>>> +
>>>> + $ hg strip -r 0:
>>>> +
>>>> +Similar test on a more complex graph
>>>> +
>>>> + $ hg debugdrawdag <<EOF
>>>> + > g
>>>> + > |
>>>> + > f e
>>>> + > | |
>>>> + > e' d # replace: e -> e'
>>>> + > \ /
>>>> + > c
>>>> + > |
>>>> + > x b
>>>> + > \|
>>>> + > a
>>>> + > EOF
>>>> + $ hg log -G -r 'a':
>>>> + o 8:2876ce66c6eb g
>>>> + |
>>>> + | o 7:3ffec603ab53 f
>>>> + | |
>>>> + x | 6:e36fae928aec e (rewritten using replace as 5:63324dc512ea)
>>>> + | |
>>>> + | o 5:63324dc512ea e'
>>>> + | |
>>>> + o | 4:76be324c128b d
>>>> + |/
>>>> + o 3:a82ac2b38757 c
>>>> + |
>>>> + | o 2:630d7c95eff7 x
>>>> + | |
>>>> + o | 1:488e1b7e7341 b
>>>> + |/
>>>> + o 0:b173517d0057 a
>>>> +
>>>> + $ hg rebase -b 'f' -d 'x'
>>>> + rebasing 1:488e1b7e7341 "b" (b)
>>>> + rebasing 3:a82ac2b38757 "c" (c)
>>>> + rebasing 5:63324dc512ea "e'" (e')
>>>> + rebasing 7:3ffec603ab53 "f" (f)
>>>> + rebasing 4:76be324c128b "d" (d)
>>>> + note: not rebasing 6:e36fae928aec "e" (e) and its descendants as this
>>>>
>>> would cause divergences
>>>
>>>> + $ hg log -G -r 'a':
>>>> + o 13:a1707a5b7c2c d
>>>> + |
>>>> + | o 12:ef6251596616 f
>>>> + | |
>>>> + | o 11:b6f172e64af9 e'
>>>> + |/
>>>> + o 10:d008e6b4d3fd c
>>>> + |
>>>> + o 9:67e8f4a16c49 b
>>>> + |
>>>> + | o 8:2876ce66c6eb g
>>>> + | |
>>>> + | | x 7:3ffec603ab53 f (rewritten using rebase as 12:ef6251596616)
>>>> + | | |
>>>> + | x | 6:e36fae928aec e (rewritten using replace as 5:63324dc512ea)
>>>> + | | |
>>>> + | | x 5:63324dc512ea e' (rewritten using rebase as 11:b6f172e64af9)
>>>> + | | |
>>>> + | x | 4:76be324c128b d (rewritten using rebase as 13:a1707a5b7c2c)
>>>> + | |/
>>>> + | x 3:a82ac2b38757 c (rewritten using rebase as 10:d008e6b4d3fd)
>>>> + | |
>>>> + o | 2:630d7c95eff7 x
>>>> + | |
>>>> + | x 1:488e1b7e7341 b (rewritten using rebase as 9:67e8f4a16c49)
>>>> + |/
>>>> + o 0:b173517d0057 a
>>>> +
>>>> +
>>>> + $ cd ..
>>>> +
>>>> Rebase merge where successor of one parent is equal to destination
>>>>
>>> (issue5198)
>>>
>>>>
>>>> $ hg init p1-succ-is-dest
>>>> _______________________________________________
>>>> Mercurial-devel mailing list
>>>> Mercurial-devel at mercurial-scm.org
>>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20171114/31813bbc/attachment.html>
More information about the Mercurial-devel
mailing list