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

[Ramkumar Ramachandra <artagnon@gmail.com>]

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 <artagnon@gmail.com> writes:
>>> 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.

>>> 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