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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Aug 18, 2011, 18:51 UTC
Message-ID: <CALkWK0noHBnW-7zZLw=jJdDVFxXmsm2vHHYnUJc9miLLuDRnAg@mail.gmail.com>
In-Reply-To: <7v7h6eld2c.fsf@alter.siamese.dyndns.org>

Hi Junio,

Junio C Hamano writes:

> [...]

Here are some comments from my end after extensive thought. Instead of responding to your comments directly, I've sprinkled bits of it here and there, reordering it as I see fit. Thanks for detailing out your thoughts so well :)

0. Before getting into historical mistakes and evaluating existing
workflows, let's first try to detail the problem the sequencer is
trying to solve: pick/ revert aren't the only operations we want to
support.  Let's imagine some hypothetical "foo" operation that does
something incredibly complex in the instruction sheet:

pick b38bcc foo pick 49ab7c

Now, let's imagine that the "foo" command failed.  What is the
strategy we will employ to handle his?  I think the question boils
down to: should we teach '--continue' the prerequisite for executing a
"pick" command (clean worktree etc), or how to finish a "foo" command?
 What will this answer depend upon?

1. Now, let's discuss the motivation for d3f4628e.  I didn't want to
create a separate workflow for pick-one-commit versus
pick-many-commits.  It would have to go something like:

  parse_arguments(whatever tree'ishes specified on argc)
  setup_revision_walker
  n = count_revisions
  if (n == 1)
    do foo; // Don't create sequencer state
  else
    create sequencer state; do foo; cleanup sequencer state;

Ugly. Wouldn't you agree? So, what choice did I have? I can't conditionally create sequencer state. So, an idea struck me: if I clean it up before someone notices, I'm safe. So, I decided to remove it before the last commit -- when there's only one commit, it'll be removed immediately. So far so good: no regressions introduced.

2.  Okay, now let's try to touch upon the issue of marking something as "done":

Show 5 quoted lines

>  - The case to resume an unexpected stop (i.e. "pick" that causes conflict
>   or "rebase" without "-i") also feels right. The user fixes up and marks
>   things as "done" with "add/rm", and tell the stopped command that it can
>   now continue with "rebase --continue". The same comment applies for
>   "am".

I could argue that "add/rm" need not be the only way to specify "done" in the general case. We needn't be consistent about that- we can print hints about what "done" would mean, and not invent a new "--mark-as-done" command. What varies here is the definition of "done": does it mean that the user has "done" what the instruction sheet operation was supposed to do (including the committing), or has simply done a part of the job and passed on the baton?

3. How tight do we want the sequencer to be?  Should we allow
executing other commands during the operation of the sequencer, or
should we simply expect the user to "--continue" the operation first
and take care of everything else later?

Show 5 quoted lines

>  - The sequencing flow the current "rebase -i" implements when resuming a
>   controlled stop (i.e. "edit" or "reword"), even as the last step of the
>   insn sheet, feels like the right thing. The actual tweaking of the
>   commit done by "commit --amend" is oblivious to the sequencing state,
>   and resuming is controlled by "rebase --continue".

I often "git commit -m temp" in the middle of a "rebase -i", and I'm strongly in favor of allowing this somehow.

> There is a (complicated) workflow to split an commit during
> "rebase -i" that does _not_ want an auto-resume by a commit.

Exactly! :) Auto-resuming after a "commit" is fundamentally wrong.

> Also not
> all conflicted/stopped state can be concluded with "commit" (think:
> "rebase/am --skip").

A good point on consistency.

4.  Let's correct the historical mistakes and get our act together.
The final pending question is: how many historical mistakes are we
willing to stomp now?

Show 8 quoted lines

> I personally am leaning towards teaching "--continue" to "merge" and
> "cherry-pick", while keeping only existing awareness of these two commands
> in "commit" as historical mistakes (look for 'unlink(git_path("[A-Z_]*"))'
> in builtin/commit.c). That would mean that in the long run, new users need
> to learn only one thing: when "git Foo" stops because it needs help from
> you resolving conflicts, after you help it by editing the files in your
> working tree and making that with add/rm, you say "git Foo --continue" to
> signal that you are done helping it.

Okay, stomp nothing.  Under this constraint, the best we can do is:
1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD
helps 'git commit' finish.  Modify tests to use '--continue' instead
of the earlier commit-to-finish workflow, and advertise this feature
everywhere.
2. Make 'cherry-pick --continue' invoke 'git commit' as well.
CHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes
successfully: (if there is one commit left, remove the sequencer
state; otherwise, drop the first insn on top of the list and execute
the next insn).  Modify tests to use '--continue' instead of the
earlier commit-to-finish workflow, and advertise this feature
everywhere.  Unfortunately, if the user executes 'git commit' instead
of the newer '--continue', we're screwed: a stray sequencer state will
be left behind.

Does the above seem sensible, or should we do something about the historical mistakes?

Thanks.

-- Ram