[PATCH] histedit: make --keep work more like graft and use default phase for copies
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Sun Nov 27 19:38:17 EST 2016
On 11/27/2016 04:02 PM, Augie Fackler wrote:
>
>> On Nov 25, 2016, at 8:52 AM, Pierre-Yves David
>> <pierre-yves.david at ens-lyon.org
>> <mailto:pierre-yves.david at ens-lyon.org>> wrote:
>>
>>
>>
>> On 11/19/2016 02:44 AM, Augie Fackler wrote:
>>> On Fri, Nov 18, 2016 at 05:16:06PM +0100, Mads Kiilerich wrote:
>>>> # HG changeset patch
>>>> # User Mads Kiilerich <madski at unity3d.com <mailto:madski at unity3d.com>>
>>>> # Date 1479485469 -3600
>>>> # Fri Nov 18 17:11:09 2016 +0100
>>>> # Node ID 19ad769b648182e92a49015d85389fe2a8303c18
>>>> # Parent 1156ec81f70907ff843ca30bb81b4ef59b9b7068
>>>> histedit: make --keep work more like graft and use default phase for
>>>> copies
>>>
>>> queued, thanks
>>
>> Can we get a bit more explanation from Mads about why we would like to
>> change the behavior? I'm open to a change but the current behavior
>> make sense to me so I wonder what is the usercase to change it. In
>> addition, `hg rebase --keep` is to preserve the source phases too¹ so
>> this changes introduce and inconsistency here.
>>
>> [1] except when source is public, in that case draft is used.
>
> IIRC Mads was reporting that it was preserving public phase, which
> struck me as an obvious bug.
here is my current understanding of the situation:
Before this patch:
------------------
1) histedit was refusing to run on public changeset in all cases
(regardless of --keep usage)
2) histedit was preserving the phrase of changeset it touch in all
cases: (draft → draft, secret → secret, public not allowed)
(note, this phase preserving behavior in all cases match rebase).
This patch is doing two things:
-------------------------------
a) allow histedit on public changeset if --keep is present,
b) change histedit --keep to use 'new-commit' phase,
As Augie pointed, the first change (allow public with --keep) make
perfect sense.
On the other hand, I'm not sure about what's motivate (b). I'm not sure
if there is an actual use case behind it or if it is just a reaction to
histedit preserving the public phases.
In all cases, this behavior change regarding phase preservation is
problematic for two reasons:
1) consistency: rebase still preserve phase when --keep is used. I think
it is important to have a consistent behavior between the two commands
on this point.
2) backward compatibility: it is likely that we currently have existing
users of histedit at rely on the fact secret changesets stay secret with
'histedit --keep', changing that behavior would lead them to turn
'secret' content draft and risk to push/publish it by mistake after
upgrading.
Given these serious concerns and the fact they was 90 other patches
ready to be accepted on top of it, I've dropped this one from
hg-committed for now.
>> More comment below.
>>
>>>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>>>> --- a/hgext/histedit.py
>>>> +++ b/hgext/histedit.py
>>>> @@ -91,7 +91,8 @@ ones) until after it has completed all t
>>>> probably perform several strip operations when it's done. For the
>>>> above example,
>>>> it had to run strip twice. Strip can be slow depending on a variety
>>>> of factors,
>>>> so you might need to be a little patient. You can choose to keep the
>>>> original
>>>> -revisions by passing the ``--keep`` flag.
>>>> +revisions by passing the ``--keep`` flag. This will work more like
>>>> graft and
>>>> +not preserve/copy the original phase.
>>>>
>>>> The ``edit`` operation will drop you back to a command prompt,
>>>> allowing you to edit files freely, or even use ``hg record`` to commit
>>>> @@ -466,7 +467,7 @@ class histeditaction(object):
>>>> rulectx = repo[self.node]
>>>>
>>>> editor = self.commiteditor()
>>>> - commit = commitfuncfor(repo, rulectx)
>>>> + commit = commitfuncfor(repo, rulectx, self.state.keep)
>>>>
>>>> commit(text=rulectx.description(), user=rulectx.user(),
>>>> date=rulectx.date(), extra=rulectx.extra(),
>>>> editor=editor)
>>>> @@ -489,7 +490,7 @@ class histeditaction(object):
>>>> return ctx, []
>>>> return ctx, [(self.node, (ctx.node(),))]
>>>>
>>>> -def commitfuncfor(repo, src):
>>>> +def commitfuncfor(repo, src, keep):
>>>> """Build a commit function for the replacement of <src>
>>>>
>>>> This function ensure we apply the same treatment to all changesets.
>>>> @@ -503,8 +504,9 @@ def commitfuncfor(repo, src):
>>>> def commitfunc(**kwargs):
>>>> phasebackup = repo.ui.backupconfig('phases', 'new-commit')
>>>> try:
>>>> - repo.ui.setconfig('phases', 'new-commit', phasemin,
>>>> - 'histedit')
>>>> + if not keep:
>>>> + repo.ui.setconfig('phases', 'new-commit', phasemin,
>>>> + 'histedit')
>>>> extra = kwargs.get('extra', {}).copy()
>>>> extra['histedit_source'] = src.hex()
>>>> kwargs['extra'] = extra
>>>> @@ -531,7 +533,7 @@ def applychanges(ui, repo, ctx, opts):
>>>> repo.ui.setconfig('ui', 'forcemerge', '', 'histedit')
>>>> return stats
>>>>
>>>> -def collapse(repo, first, last, commitopts, skipprompt=False):
>>>> +def collapse(repo, first, last, commitopts, skipprompt=False,
>>>> keep=False):
>>>> """collapse the set of revisions from first to last as new one.
>>>>
>>>> Expected commit options are:
>>>> @@ -545,7 +547,7 @@ def collapse(repo, first, last, commitop
>>>> if not ctxs:
>>>> return None
>>>> for c in ctxs:
>>>> - if not c.mutable():
>>>> + if not c.mutable() and not keep:
>>
>> It seems like this changeset is introducing a secondary changes, it
>> makes it possible to `hg histedit --keep` public changeset. It sounds
>> like an independent changes that does not alter any existing behavior.
>> It would be better to have it as a separate changes.
>
> That honestly seems like it should always have been allowed? In a
> perfect world yes, this should have been a separate change, but I don’t
> think it’s worth churning the stack for something so minor.
I agree this is minor. The main drawback is that grouping this change
with the behavior one means it get dropped too.
>>>> raise error.ParseError(
>>>> _("cannot fold into public change %s") %
>>>> node.short(c.node()))
>>>> base = first.parents()[0]
>>>> @@ -669,16 +671,15 @@ class fold(histeditaction):
>>>> return
>>>> else:
>>>> c = repo[prev.node]
>>>> - if not c.mutable():
>>>> + if not c.mutable() and not self.state.keep:
>>>> raise error.ParseError(
>>>> _("cannot fold into public change %s") %
>>>> node.short(c.node()))
>>>>
>>>> -
>>>> def continuedirty(self):
>>>> repo = self.repo
>>>> rulectx = repo[self.node]
>>>>
>>>> - commit = commitfuncfor(repo, rulectx)
>>>> + commit = commitfuncfor(repo, rulectx, self.state.keep)
>>>> commit(text='fold-temp-revision %s' % node.short(self.node),
>>>> user=rulectx.user(), date=rulectx.date(),
>>>> extra=rulectx.extra())
>>>> @@ -752,9 +753,10 @@ class fold(histeditaction):
>>>> phasebackup = repo.ui.backupconfig('phases', 'new-commit')
>>>> try:
>>>> phasemin = max(ctx.phase(), oldctx.phase())
>>>> - repo.ui.setconfig('phases', 'new-commit', phasemin,
>>>> 'histedit')
>>>> + if not self.state.keep:
>>>> + repo.ui.setconfig('phases', 'new-commit', phasemin,
>>>> 'histedit')
>>>> n = collapse(repo, ctx, repo[newnode], commitopts,
>>>> - skipprompt=self.skipprompt())
>>>> + skipprompt=self.skipprompt(),
>>>> keep=self.state.keep)
>>>> finally:
>>>> repo.ui.restoreconfig(phasebackup)
>>>> if n is None:
>>>> diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
>>>> --- a/tests/test-histedit-edit.t
>>>> +++ b/tests/test-histedit-edit.t
>>>> @@ -453,6 +453,7 @@ rollback should not work after a histedi
>>>> $ echo foo >> a
>>>> $ hg ci -m 'extend a'
>>>> $ hg phase --public 1
>>>> +
>>>> Attempting to fold a change into a public change should not work:
>>>> $ cat > ../edit.sh <<EOF
>>>> > cat "\$1" | sed s/pick/fold/ > tmp
>>>> @@ -480,3 +481,19 @@ Attempting to fold a change into a publi
>>>> # f, fold = use commit, but combine it with the one above
>>>> # r, roll = like fold, but discard this commit's description
>>>> #
>>>> +
>>>> +but it should work more like graft when using --keep
>>>> +
>>>> + $ HGEDITOR="sh ../edit.sh" hg histedit 2 --keep
>>>> + saved backup bundle to
>>>> $TESTTMP/r0/.hg/strip-backup/17b1aa9a4a0b-8d4a5be3-backup.hg (glob)
>>>> +
>>>> + $ hg log -G -T '{rev} {node|short} {phase} {desc|firstline}'
>>>> + @ 3 311eafe309d8 draft add b
>>>> + |
>>>> + | o 2 0012be4a27ea draft extend a
>>>> + | |
>>>> + | o 1 18aa70c8ad22 public add b
>>>> + |/
>>>> + o 0 0efcea34f18a public a
>>>> +
>>>> +
>>
>> --
>> Pierre-Yves David
>
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list