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

From: Ramkumar Ramachandra <artagnon@gmail.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Git List <git@vger.kernel.org>,
	Christian Couder <chriscool@tuxfamily.org>,
	Daniel Barkalow <barkalow@iabervon.org>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
Date: Fri, 19 Aug 2011 01:20:47 +0530	[thread overview]
Message-ID: <CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com> (raw)
In-Reply-To: <20110818191812.GG30436@elie.gateway.2wire.net>

Hi Jonathan,

Jonathan Nieder writes:
> Could be briefer. :)

Sorry about the braindump :P

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

In the future, we might want a 'merge' instruction in the sequencer --
I want to make it clear that we're going for a significant UI change
so that everyone (including tests, scripts) become comfortable with
the new UI.

>> 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).
>
> Sounds like a sensible thing to do.  I assume the "one" in the
> parenthesis is supposed to be "zero", making the "if" not even part of
> the user-visible description of what it does --- it's just the
> termination condition of a loop.

Right, sorry about the convoluted thought.

> 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.  In the case of one-commit operations, *every*
new command that calls into the sequencer will will need to persist
information in its own way using hacks like CHERRY_PICK_HEAD and
MERGE_HEAD.  And we have to make "git commit" unlink yet another file
:)  I'm not talking about some hypothetical case: I'm already planning
to make 'git am' call into the sequencer, so we'll need an AM_HEAD.

One final resort: Move some code back into cherry-pick, and call into
a later-function in the sequencer only if it's a many-commit
operation.  The new commands can enjoy the comfort of calling into an
earlier-function in the sequencer that'll do all the revision walk
setup and call the later-function.  I think this is reasonable.

> 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.  Either behavior seems fine for such commands.

I don't think I follow.  This will be determined as a single-commit
operation after setting up the revisions.  I don't think it should be
treated as a multi-commit operation because the literal tree'ish
contains "..".

-- Ram