Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:50

Jonathan Nieder [off-list ref] writes:
One part I'm handwaving is what to do about commands like "git
cherry-pick foo^..foo" which use a commit range that only happens to
contain one commit.
Historically cherry-pick took a single commit from the command line. That
can easily be distinguished from a "set that ended up with one positive
commit after walking the history". I think you can even check before
triggering the revision walking machinery in the first place.

Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:51:50

Hi Jonathan and Junio,

Jonathan Nieder writes:
Why can't it become the sequencer's responsibility, FWIW?  That's an
implementation detail.
It can be the sequencer's responsibility if we want that!  I'm just
dabbling with the different implementation strategies and trying to
see the advantages/ disadvantages of each one.  The "right" thing to
do wasn't obvious to me immediately.

Junio C Hamano writes:
Ramkumar Ramachandra [off-list ref] writes:
quoted
quoted
Why modify tests?  I think "git merge --continue" is a nice idea,
and I don't see how it's inconsistent in any way with continuing to
allow old practice.
I agree. Updating the test will hide a regression if Ram's update breaks
the existing workflow to conclude a conflicted merge with "git commit".
If we are to add "git merge --continue" sugarcoat to make it easier to
teach new people saying that "To any Git operation that stops and asks you
to help, you can tell it that you are done helping by re-running the same
command with --continue flag", then _new_ tests should be added to make
sure that "git merge --continue" does act just like "git commit" in such a
case.
Right.  We have to keep the old tests around -- my bad.
quoted
quoted
As Junio hinted, it could make a lot of sense for "git cherry-pick
<single commit>" to not create sequencer state in the first place.
"git cherry-pick --continue" does not need it --- it is enough to
commit with the conflict resolved.  "git cherry-pick --abort" does not
need it, either --- it is enough to "git reset --merge HEAD".
Okay, here's my problem with the idea: it'll essentially require the
sequencer to differentiate between one-commit operations and
many-commit operations.
I think you are looking at it in a wrong way. It is just about giving
backward compatibility to historical hacks. cherry-pick and revert may be
the only ones needed, and a new command Foo that implements its workflow
in terms of the sequencer can choose to (and should choose to unless there
is a compelling reason not to, because of the exact reason you stated
here) do a single-command insn sheet with "git Foo --continue" to conclude
if that one and only step needed help from the end user.
No, no.  I don't _want_ to create an AM_HEAD or FOO_HEAD for every new
command that we write -- I was merely pointing out what would happen
if the sequencer were only built to handle multi-commit-operations,
and every new command would have to handle single-commit-operations on
their own.  I was emphasizing that the sequencer will need to handle
single-commit-operations as well; Jonathan has suggested that we have
special hacks to handle cherry-pick and merge commands in the
sequencer.

To conclude, let me list out the various implementation strategies
we've thought about, and the one we seem to have settled on:
0. Remove the sequencer state from "git commit".  This is wrong, as
Junio pointed out in the first email.
1. Let commands handle single-commit-operations themselves, and call
into the sequencer only for multi-commit-operations.  If they don't
call into the sequencer at all, there's no state to persist or
cleanup.  This approach is clearly wrong because each new command
would have to come up with AM_HEAD and FOO_HEAD to persist the
single-commit-operation state.
2. Expose two functions from the sequencer: "earlier-function" and
"later-function".  Let cherry-pick and merge handle their own
CHERRY_PICK_HEAD and MERGE_HEAD hacks first (by setting up the
revision walker, counting commits etc), and call into later-function
in the sequencer to do less work from the sequencer's end.  All new
commands can directly call into earlier-function directly.  I know
it's a little hand-wavy, but I hope it's atleast parse'able this time.
 As I pointed out in another email, this is also broken because
cherry-pick/ merge would have to implement their own versions of every
subcommand like "--continue".
3. Let all commands call into the sequencer for everything.  Let's
teach the sequencer about the CHERRY_PICK_HEAD and MERGE_HEAD hacks so
that it can deal with them when a "pick", "revert" or "merge"
operation is encountered.  We'll handle it by treating
CHERRY_PICK_HEAD and MERGE_HEAD as part of the "sequencer state" (so
that subcommands work just fine on them).  For all future sequencer
commands, all data will be persisted inside '.git/sequencer'.  This
seems most reasonable now.

In a nutshell, instead of side-stepping historical hacks, we want to
teach the sequencer about them as specific cases to handle carefully.
We want to this without affecting the operation of the sequencer in
the general case.  Sounds great!  An uncompromising UI at the cost of
little ugliness in the sequencer.

Thanks.

-- Ram
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help