( )⚙ D11037 amend: make `hg amend -r` rebase temporary commit onto target commit

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11037

amend: make `hg amend -r` rebase temporary commit onto target commit
AbandonedPublic

Authored by martinvonz on Jul 9 2021, 4:28 PM.

Details

Reviewers
Alphare
baymax
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Jul 9 2021, 4:28 PM
Alphare accepted this revision as: Alphare.Jul 12 2021, 5:16 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
hgext/amend.py
203

I may not understand the version argument (set to 1), but shouldn't it differ from the one at the start of the main function?

martinvonz marked an inline comment as done.Jul 12 2021, 11:49 AM
martinvonz added inline comments.
hgext/amend.py
203

I think that version number is supposed to be bumped when you change format of the file in a way that breaks existing readers (i.e. by changing to a non-cbor format?). The current user (hg graft) always passes version=1 and doesn't care what the version is when it reads the file.

martinvonz marked an inline comment as done.Jul 12 2021, 2:43 PM
martinvonz updated this revision to Diff 29211.
marmoute added a subscriber: marmoute.Jul 15 2021, 7:55 PM

I mostly have various questions, some being related to the question in the previous changeset:

  1. why do we even need a temporary commit instead simply using update logic directly
  2. Can you clarify the business about graft, version etc (see my comment on`_continue_amend_rev`)
  3. If we need to rebase a changesets, why do we need to directly call the rebase extensions instead of using some common factored out code ? (or at least maybe hg graft ?)
  4. Can we get more docstring about the role and semantic of the various new functions ?
hgext/amend.py
186–192

Can't we use the rewriteutil do to this instead of relying on a third party extension ?

203

Are we not using rebase here instead of graft ?

And in all case: why does the rebase/graft behavior matters here ? If we are falling back to the logic of another extension/command¹ why isn't that command dealing with its state file itself instead of use having to mimic it?

Or maybe I am missing something here.

[1] which as find hacky

207

What's the difference between continue_amend_rev and do_continue_amend_rev which part of the continuation is it actually doing ? I think having some docstring for these functions would help.

215

Same feedback about doc-string applies.

260

Should we filter out None value explicitly here instead of relying on the next loop to get rid of it ?

mercurial/state.py
381–384

small nits: I would advocate for intermediate variable to be used instead of a statement spawning over four line.

386–388

same minor nits here.

martinvonz marked an inline comment as done.Jul 15 2021, 8:35 PM
martinvonz updated this revision to Diff 29294.
martinvonz marked 3 inline comments as done.Jul 15 2021, 8:40 PM
martinvonz added inline comments.
hgext/amend.py
186–192

Can't we use the rewriteutil do to this instead of relying on a third party extension ?

Maybe. I've never seen rewriteutil used for rebasing commits. Can you explain?

(rebase has been a first-party extension for a long time, as far as I know)

203

Are we not using rebase here instead of graft ?

Correct.

And in all case: why does the rebase/graft behavior matters here ? If we are falling back to the logic of another extension/command¹ why isn't that command dealing with its state file itself instead of use having to mimic it?

This is about the state saved by the amend extension, not state saved by either rebase or graft. The only reason I mentioned hg graft was to answer Alphare's question about version.

207

Good point. Hopefully the added docstrings help. Let me know otherwise, of course.

260

I agree that it would seem like it and that's what I did initially, but it becomes annoying to do that after the next patch.

mercurial/state.py
381–384

The current style matches the surrounding functions. I'll send a separate patch (either before or after this one) to extract the message string to separate variables

baymax requested changes to this revision.Feb 2 2022, 8:06 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Feb 2 2022, 8:06 PM
martinvonz marked 2 inline comments as done.Feb 11 2022, 6:36 PM
martinvonz abandoned this revision.

It looks like this is being worked on in the evolve extension, so I'll drop this patch.

Revision Contents
Changeset List

PathPackages
Mhgext/amend.py (87 lines)
Mmercurial/state.py (16 lines)
Mtests/test-amend-rev.t (100 lines)

Diff 29050

hgext/amend.py

from __future__ import absolute_import from __future__ import absolute_import
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
cmdutil, cmdutil,
commands, commands,
error, error,
extensions,
merge, merge,
obsolete, obsolete,
registrar, registrar,
repair, repair,
revsetlang,
rewriteutil, rewriteutil,
scmutil, scmutil,
state as statemod, state as statemod,
util, util,
) )
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
], ],
) )
if not obsolete.isenabled(repo, obsolete.createmarkersopt): if not obsolete.isenabled(repo, obsolete.createmarkersopt):
raise error.StateError( raise error.StateError(
_(b'--rev requires evolution.createmarkers to be enabled') _(b'--rev requires evolution.createmarkers to be enabled')
) )
rebase = _find_rebase()
state = {} state = {}
state_store = statemod.cmdstate(repo, b'amend-state') state_store = statemod.cmdstate(repo, b'amend-state')
with repo.wlock(), repo.lock(), util.acceptintervention( with repo.wlock(), repo.lock(), util.acceptintervention(
repo.transaction(b'amend') repo.transaction(b'amend')
), state_store.save_on_conflicts(1, state): ), state_store.save_on_conflicts(1, state):
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
target_ctx = scmutil.revsingle(repo, opts['rev']) target_ctx = scmutil.revsingle(repo, opts['rev'])
to_rewrite = repo.revs(b'%d::.', target_ctx.rev()) to_rewrite = repo.revs(b'%d::.', target_ctx.rev())
rewriteutil.precheck(repo, to_rewrite, b'amend') rewriteutil.precheck(repo, to_rewrite, b'amend')
try: try:
# Create a temporary commit of the working copy. # Create a temporary commit of the working copy.
ret = commands._docommit( ret = commands._docommit(
ui, repo, message=b'temporary commit for "amend --rev"' ui, repo, message=b'temporary commit for "amend --rev"'
) )
if ret: if ret:
return ret return ret
temp_ctx = repo[b'tip'] temp_ctx = repo[b'tip']
state[b'target_node'] = target_ctx.node() state[b'target_node'] = target_ctx.node()
state[b'temp_node'] = temp_ctx.node() state[b'temp_node'] = temp_ctx.node()
_do_continue_amend_rev(ui, repo, state, rebase)
except error.InterventionRequired: except error.InterventionRequired:
raise raise
except Exception: except Exception:
_do_abort_amend_rev(ui, repo, state) _do_abort_amend_rev(ui, repo, state)
raise raise
def _find_rebase():
try:
return extensions.find(b'rebase')
except KeyError:
raise error.ConfigError(
b'amend --rev requires the rebase extension to be enabled'
)
marmouteUnsubmitted
Not Done

Can't we use the rewriteutil do to this instead of relying on a third party extension ?

marmoute: Can't we use the rewriteutil do to this instead of relying on a third party extension ?
martinvonzAuthorUnsubmitted
Done

Can't we use the rewriteutil do to this instead of relying on a third party extension ?

Maybe. I've never seen rewriteutil used for rebasing commits. Can you explain?

(rebase has been a first-party extension for a long time, as far as I know)

martinvonz: > Can't we use the rewriteutil do to this instead of relying on a third party extension ?
def _continue_amend_rev(ui, repo): def _continue_amend_rev(ui, repo):
raise error.Abort(_(b'--continue is not yet implemented')) rebase = _find_rebase()
state_store = statemod.cmdstate(repo, b'amend-state')
state = state_store.read()
with repo.wlock(), repo.lock(), util.acceptintervention(
repo.transaction(b'amend')
), state_store.save_on_conflicts(1, state):
AlphareUnsubmitted
Done

I may not understand the version argument (set to 1), but shouldn't it differ from the one at the start of the main function?

Alphare: I may not understand the `version` argument (set to 1), but shouldn't it differ from the one at…
martinvonzAuthorUnsubmitted
Done

I think that version number is supposed to be bumped when you change format of the file in a way that breaks existing readers (i.e. by changing to a non-cbor format?). The current user (hg graft) always passes version=1 and doesn't care what the version is when it reads the file.

martinvonz: I think that version number is supposed to be bumped when you change format of the file in a…
marmouteUnsubmitted
Done

Are we not using rebase here instead of graft ?

And in all case: why does the rebase/graft behavior matters here ? If we are falling back to the logic of another extension/command¹ why isn't that command dealing with its state file itself instead of use having to mimic it?

Or maybe I am missing something here.

[1] which as find hacky

marmoute: Are we not using `rebase` here instead of `graft` ? And in all case: why does the…
martinvonzAuthorUnsubmitted
Done

Are we not using rebase here instead of graft ?

Correct.

And in all case: why does the rebase/graft behavior matters here ? If we are falling back to the logic of another extension/command¹ why isn't that command dealing with its state file itself instead of use having to mimic it?

This is about the state saved by the amend extension, not state saved by either rebase or graft. The only reason I mentioned hg graft was to answer Alphare's question about version.

martinvonz: > Are we not using rebase here instead of graft ? Correct. >And in all case: why does the…
_do_continue_amend_rev(ui, repo, state, rebase)
def _do_continue_amend_rev(ui, repo, state, rebase):
marmouteUnsubmitted
Done

What's the difference between continue_amend_rev and do_continue_amend_rev which part of the continuation is it actually doing ? I think having some docstring for these functions would help.

marmoute: What's the difference between `continue_amend_rev` and `do_continue_amend_rev` which part of…
martinvonzAuthorUnsubmitted
Done

Good point. Hopefully the added docstrings help. Let me know otherwise, of course.

martinvonz: Good point. Hopefully the added docstrings help. Let me know otherwise, of course.
unfi = repo.unfiltered()
target_ctx = unfi[state[b'target_node']]
temp_ctx = unfi[state[b'temp_node']]
_rebase_temp_node(ui, repo, state, rebase, temp_ctx, target_ctx)
def _rebase_temp_node(ui, repo, state, rebase, temp_ctx, target_ctx):
marmouteUnsubmitted
Done

Same feedback about doc-string applies.

marmoute: Same feedback about doc-string applies.
if b'rebased_temp_node' in state:
return repo.unfiltered()[state[b'rebased_temp_node']]
elif statemod.ischildunfinished(repo, b'amend', b'rebase'):
with ui.silent(), statemod.delegating(repo, b'amend', b'rebase'):
ret = statemod.continuechild(ui, repo, b'amend', b'rebase')
elif temp_ctx.p1() != target_ctx:
with ui.silent(), statemod.delegating(repo, b'amend', b'rebase'):
ret = rebase.rebase(
ui,
repo,
rev=[revsetlang.formatspec(b'%d', temp_ctx.rev())],
dest=revsetlang.formatspec(b'%d', target_ctx.rev()),
)
if ret:
raise error.Abort(_(b'failed to rebase temporary commit'))
rebased_temp_ctx = repo[b'tip']
state[b'rebased_temp_node'] = rebased_temp_ctx.node()
return rebased_temp_ctx
def _abort_amend_rev(ui, repo): def _abort_amend_rev(ui, repo):
with repo.wlock(), repo.lock(), repo.transaction(b'amend'): with repo.wlock(), repo.lock(), repo.transaction(b'amend'):
state_store = statemod.cmdstate(repo, b'amend-state') state_store = statemod.cmdstate(repo, b'amend-state')
state = state_store.read() state = state_store.read()
_do_abort_amend_rev(ui, repo, state) _do_abort_amend_rev(ui, repo, state)
state_store.delete() state_store.delete()
def _do_abort_amend_rev(ui, repo, state): def _do_abort_amend_rev(ui, repo, state):
unfi = repo.unfiltered() unfi = repo.unfiltered()
if statemod.ischildunfinished(repo, b'amend', b'rebase'):
with ui.silent():
statemod.abortchild(ui, repo, b'amend', b'rebase')
to_strip = []
temp_node = state.get(b'temp_node') temp_node = state.get(b'temp_node')
if temp_node and temp_node in unfi: if temp_node and temp_node in unfi:
to_strip.append(temp_node)
temp_ctx = unfi[temp_node] temp_ctx = unfi[temp_node]
merge.clean_update(temp_ctx) merge.clean_update(temp_ctx)
with repo.dirstate.parentchange(): with repo.dirstate.parentchange():
scmutil.movedirstate(repo, temp_ctx.p1()) scmutil.movedirstate(unfi, temp_ctx.p1())
repair.delayedstrip(ui, repo, [temp_node]) rebased_temp_node = state.get(b'rebased_temp_node')
to_strip.append(rebased_temp_node)
marmouteUnsubmitted
Done

Should we filter out None value explicitly here instead of relying on the next loop to get rid of it ?

marmoute: Should we filter out `None` value explicitly here instead of relying on the next loop to get…
martinvonzAuthorUnsubmitted
Done

I agree that it would seem like it and that's what I did initially, but it becomes annoying to do that after the next patch.

martinvonz: I agree that it would seem like it and that's what I did initially, but it becomes annoying to…
to_strip = [node for node in to_strip if node and node in unfi]
if to_strip:
repair.delayedstrip(ui, unfi, to_strip)
def extsetup(ui): def extsetup(ui):
def _after_rebase_loaded(loaded):
if loaded:
statemod.addunfinished( statemod.addunfinished(
b'amend', b'amend',
fname=b'amend-state', fname=b'amend-state',
allowcommit=False, allowcommit=False,
childopnames=[b'rebase'],
abortfunc=_abort_amend_rev, abortfunc=_abort_amend_rev,
continueflag=True,
continuefunc=_continue_amend_rev, continuefunc=_continue_amend_rev,
) )
extensions.afterloaded(b'rebase', _after_rebase_loaded)

mercurial/state.py

) )
if not c.continuefunc: if not c.continuefunc:
raise error.ProgrammingError( raise error.ProgrammingError(
_(b'op %s has no continue function') % childopname _(b'op %s has no continue function') % childopname
) )
return c.continuefunc(ui, repo) return c.continuefunc(ui, repo)
def abortchild(ui, repo, opname, childopname):
"""Checks that childopname is in progress, and aborts it."""
p, c = _getparentandchild(opname, childopname)
if not ischildunfinished(repo, opname, childopname):
raise error.ProgrammingError(
_(b'child op %s of parent %s is not unfinished')
% (childopname, opname)
)
marmouteUnsubmitted
Not Done

small nits: I would advocate for intermediate variable to be used instead of a statement spawning over four line.

marmoute: small nits: I would advocate for intermediate variable to be used instead of a statement…
martinvonzAuthorUnsubmitted
Done

The current style matches the surrounding functions. I'll send a separate patch (either before or after this one) to extract the message string to separate variables

martinvonz: The current style matches the surrounding functions. I'll send a separate patch (either before…
if not c.abortfunc:
raise error.ProgrammingError(
_(b'op %s has no abort function') % childopname
)
marmouteUnsubmitted
Not Done

same minor nits here.

marmoute: same minor nits here.
return c.abortfunc(ui, repo)
addunfinished( addunfinished(
b'update', b'update',
fname=b'updatestate', fname=b'updatestate',
clearable=True, clearable=True,
cmdmsg=_(b'last update was interrupted'), cmdmsg=_(b'last update was interrupted'),
cmdhint=_(b"use 'hg update' to get a consistent checkout"), cmdhint=_(b"use 'hg update' to get a consistent checkout"),
statushint=_(b"To continue: hg update ."), statushint=_(b"To continue: hg update ."),
) )

tests/test-amend-rev.t

$ cat << EOF >> $HGRCPATH $ cat << EOF >> $HGRCPATH
> [extensions] > [extensions]
> amend= > amend=
> rebase=
> debugdrawdag=$TESTDIR/drawdag.py > debugdrawdag=$TESTDIR/drawdag.py
> [experimental] > [experimental]
> evolution.createmarkers=True > evolution.createmarkers=True
> EOF > EOF
Some simple setup Some simple setup
$ hg init amend-into-grandparent $ hg init amend-into-grandparent
Fails if the working copy is clean Fails if the working copy is clean
$ hg amend -r 'desc("modify a")' $ hg amend -r 'desc("modify a")'
nothing changed nothing changed
[1] [1]
Fails if evolution is not enabled Fails if rebase is not enabled
$ echo a3 > a $ echo a3 > a
$ hg amend -r '.^' --config extensions.rebase=!
config error: amend --rev requires the rebase extension to be enabled
[30]
Fails if evolution is not enabled
$ hg amend -r 'desc("modify a")' --config experimental.evolution.createmarkers=False $ hg amend -r 'desc("modify a")' --config experimental.evolution.createmarkers=False
abort: --rev requires evolution.createmarkers to be enabled abort: --rev requires evolution.createmarkers to be enabled
[20] [20]
Can amend into grandparent Can amend into grandparent
$ hg amend -r 'desc("modify a")' $ hg amend -r 'desc("modify a")'
$ hg log -G -T '{rev} {desc}' $ hg log -G -T '{rev} {desc}'
@ 3 temporary commit for "amend --rev" (known-bad-output !) @ 4 temporary commit for "amend --rev" (known-bad-output !)
| (known-bad-output !) | (known-bad-output !)
o 2 add b | o 2 add b (known-bad-output !)
| |/ (known-bad-output !)
o 1 modify a o 1 modify a
| |
o 0 add a o 0 add a
Target commit has new content Target commit has new content
$ hg cat -r 'desc("modify a")' a $ hg cat -r 'desc("modify a")' a
a2 (known-bad-output !) a2 (known-bad-output !)
a3 (missing-correct-output !) a3 (missing-correct-output !)
The working copy is clean and there is no unfinished operation The working copy is clean and there is no unfinished operation
$ hg st -v $ hg st -v
Can abort or continue after conflict while rebasing temporary commit
--------------------------------------------------------------------------------
Common setup for abort and continue
$ cd "$TESTTMP"
$ hg init conflict-rebasing-temp-commit
$ cd conflict-rebasing-temp-commit
$ echo a > a
$ hg ci -Aqm 'add a'
$ echo a2 > a
$ hg ci -m 'modify a'
$ echo a3 > a
$ hg log -G -T '{rev} {desc}'
@ 1 modify a
|
o 0 add a
$ hg amend -r 'desc("add a")'
warning: conflicts while merging a! (edit, then use 'hg resolve --mark')
unresolved conflicts (see 'hg resolve', then 'hg amend --continue')
[240]
$ hg st -v
M a
? a.orig
# The repository is in an unfinished *amend* state.
# Unresolved merge conflicts:
#
# a
#
# To mark files as resolved: hg resolve --mark FILE
# To continue: hg amend --continue
# To abort: hg amend --abort
Make a copy of the repo and working copy to test continuing
$ cp -R . ../conflict-rebasing-temp-commit-continue
Can abort
$ hg abort
rebase aborted
saved backup bundle to $TESTTMP/conflict-rebasing-temp-commit/.hg/strip-backup/5d5724f8921e-27ec44bb-backup.hg
The log output looks like it did before we started
$ hg log -G -T '{rev} {desc}'
@ 1 modify a
|
o 0 add a
The working copy has the change it had before we started
$ hg diff
diff -r 41c4ea50d4cf a
--- a/a Thu Jan 01 00:00:00 1970 +0000
+++ b/a Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-a2
+a3
There is no unfinished operation
$ hg st -v
M a
? a.orig
Can continue
$ cd ../conflict-rebasing-temp-commit-continue
$ echo resolved > a
$ hg resolve -m
(no more unresolved files)
continue: hg amend --continue
$ hg continue
$ hg log -G -T '{rev} {desc}'
@ 3 temporary commit for "amend --rev" (known-bad-output !)
| (known-bad-output !)
| o 1 modify a (known-bad-output !)
|/ (known-bad-output !)
o 0 add a
Target commit has new content
$ hg cat -r 'desc("add a")' a
a (known-bad-output !)
resolved (missing-correct-output !)
The working copy is clean and there is no unfinished operation
$ hg st -v
? a.orig