This is an archive of the discontinued Mercurial Phabricator instance.

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
208

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
208

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.

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
191–197

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

208

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

212

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.

220

Same feedback about doc-string applies.

269

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
191–197

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)

208

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.

212

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

269

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.