Git development
 help / color / mirror / Atom feed
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 20:28 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108202216.GL1942@burratino>

Hi again,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
> [...]
>> Junio explained this to me in [1].  It's very unnatural for a user to
>> want to execute "git cherry-pick --continue" when the previous command
>> was a "git revert": it probably means that she forgot about the
>> in-progress "git revert".
> [...]
>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
>
> I don't think that's what Junio said.
>
> Did this actually happen, or is it a theoretical worry?  I think I would
> be more likely to run "git cherry-pick <foo>..<bar>" than "git
> cherry-pick --continue" if I had forgotten about an in-progress
> revert.  The former already errors out with a sensible message.
>
> Or is the problem that I might run:
>
>        git revert foo..bar
>        git reset --merge; # conflict --- let's clean this up
>
>        # ah, I remember reverting the patch that conflicted before;
>        # let's reuse the resolution.
>        git cherry-pick baz
>        edit file.c; # another conflict, sigh
>        git add file.c
>        git cherry-pick --continue; # oops!
>
> ?  That seems like a real worry, but the same problem could happen
> with cherry-pick used both for the multipick and single-pick, so I
> don't think your patch fundamentally addresses it.

Good catch.  I didn't replay this scenario in my head earlier.

> In other words, this is a problem caused by the overloading of the
> same cherry-pick command for single-pick and multi-pick.  I think it
> should be preventable by remembering which action failed when stopping
> a sequence and doing only a single-pick resume if
> CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.

I was attempting to fix this to simplify the life of the user, not
complicate it further- the user might have no idea what the next
command in the sequence is, and I don't see the point in
inconveniencing her.  In retrospect, I think we should simply drop
this patch.

> What
> happens when there is a mixture of picks and reverts?

Permitted.

-- Ram

^ permalink raw reply

* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Ramkumar Ramachandra @ 2012-01-08 20:17 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108194014.GI1942@burratino>

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Parse the instruction sheet in '.git/sequencer/todo' as a list of
>> (action, operand) pairs, instead of assuming that all lines have the
>> same action.
>
> Yes, I've always liked this one.
>
> Haven't re-read the patch to avoid wasted effort if there are changes
> when the previous patches in the series change.  Maybe it would be
> possible to send as a standalone?

If I don't get manage to get the series right in a couple of re-rolls,
I'll do that.

-- Ram

^ permalink raw reply

* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 20:22 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=-AWy7HnVASB1rt8njavTYOhV7Zxsdq4TE+VShVZmEzQ@mail.gmail.com>

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>> I don't know --- it's not confusing to me.  Could you explain further
>> what harm the current behavior does?  E.g., could it cause me to
>> misunderstand some basic concepts, or could it lead me to run commands
>> that cause me to scratch my head or lose data?
>
> Junio explained this to me in [1].  It's very unnatural for a user to
> want to execute "git cherry-pick --continue" when the previous command
> was a "git revert": it probably means that she forgot about the
> in-progress "git revert".
[...]
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355

I don't think that's what Junio said.

Did this actually happen, or is it a theoretical worry?  I think I would
be more likely to run "git cherry-pick <foo>..<bar>" than "git
cherry-pick --continue" if I had forgotten about an in-progress
revert.  The former already errors out with a sensible message.

Or is the problem that I might run:

	git revert foo..bar
	git reset --merge; # conflict --- let's clean this up

	# ah, I remember reverting the patch that conflicted before;
	# let's reuse the resolution.
	git cherry-pick baz
	edit file.c; # another conflict, sigh
	git add file.c
	git cherry-pick --continue; # oops!

?  That seems like a real worry, but the same problem could happen
with cherry-pick used both for the multipick and single-pick, so I
don't think your patch fundamentally addresses it.

In other words, this is a problem caused by the overloading of the
same cherry-pick command for single-pick and multi-pick.  I think it
should be preventable by remembering which action failed when stopping
a sequence and doing only a single-pick resume if
CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.

The "oops" is bad since the operator might have been intending to run
some more tests and amend as necessary before continuing the
multi-pick.  It is not _that_ bad, since more typically one would have
already run some tests before running cherry-pick --continue to commit
the resolution.  Still probably worth fixing.

> The problem becomes more serious when the
> sequencer grows more capabilities: a "git merge --continue" to
> continue a "git am" sounds much more absurd.  Ofcourse, we will
> provide a way to continue any sequencer operation in the future: "git
> continue" seems to be a good candidate.

I don't understand why "cherry-pick --continue" resuming a revert
sequence implies that "merge --continue" would have to as well.

All that said, forbidding cherry-pick --continue from resuming a
revert sequence would be fine with me, _as long as the semantics are
clearly spelled out in the commit message and documentation_.  What
happens when there is a mixture of picks and reverts?

Thanks.
Jonathan

^ permalink raw reply

* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Ramkumar Ramachandra @ 2012-01-08 20:16 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108200748.GJ1942@burratino>

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Three kinds of errors can arise from parsing '.git/sequencer/todo':
>> 1. Unrecognized action
>> 2. Malformed object name
>> 3. Object name does not refer to a valid commit
>>
>> Since we would like to make the instruction sheet user-editable in the
>> future (much like the 'rebase -i' sheet), report more fine-grained
>> parse errors prefixed with the filename and line number.
>
> Seems like a sensible idea.  In other words,
> [...]

Thanks for the new wording.

> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>>       return 0;
>>  }
>>
>> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>> +static int parse_insn_line(char *bol, char *eol,
>> +                     struct replay_insn_list *item, int lineno)
>>  {
>> +     const char *todo_file = git_path(SEQ_TODO_FILE);
>
> This idiom is _still_ scary.  I don't want to have to shout about
> this, but why didn't the commit message at least acknowledge it to put
> the reader's mind at ease when this has come up again and again?

Carried over from the previous re-roll; sorry I didn't pay enough attention.

> [...]
>> +             bol += strlen("revert ");
>> +     } else {
>> +             error_len = eol - bol > 255 ? 255 : eol - bol;
>> +             return error(_("%s:%d: Unrecognized action: %.*s"),
>> +                     todo_file, lineno, (int)error_len, bol);
>
> Ah, so my example above was wrong: the actual message is
>
>        error: .git/sequencer/todo:5: Unrecognized action: the quick bro
>        wn fox jumps over the lazy dog
>
> I guess that's fine.  Is it intended?
>
>> +     }
>>
>>       /* Eat up extra spaces/ tabs before object name */
>> -     padding = strspn(bol, " \t");
>> -     if (!padding)
>> -             return -1;
>> -     bol += padding;
>> +     bol += strspn(bol, " \t");
>
> What does this have to do with the stated goal of the patch?  It seems
> to me that an unrelated and unadvertised bugfix snuck in.

Not a bugfix: since I have to report sensible error messages now, I
changed the "pick" and "revert" checks to "pick " || "pick\t" and
"revert " || "revert\t" -- then, I can report "invalid action" if it
doesn't match instead of a useless "missing space after action".

> [...]
> By the way, this is gross.  Probably get_sha1 should grow a variant
> that takes a buffer and a length.

Yes; will do.

> [...]
>>       item->operand = lookup_commit_reference(commit_sha1);
>> -     if (!item->operand)
>> -             return -1;
>> +     if (!item->operand) {
>> +             error_len = eol - bol > 255 ? 255 : eol - bol;
>> +             return error(_("%s:%d: Not a valid commit: %.*s"),
>> +                     todo_file, lineno, (int)error_len, bol);
>> +     }
>
> Hmm, this one can be emitted even when there was no corruption or
> internal error, because the user removed a commit she was
> cherry-picking and the gc-ed before a "git cherry-pick --continue".
> Alternatively, it can happen because the repository has grown very
> crowded and what used to be an unambiguous commit name no longer is
> one (not enough digits).  Will the error message be intuitive in these
> situations?

Something like "Unable to look up commit: %s" perhaps?

> [...]
>> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>>
>>       for (i = 1; *p; i++) {
>>               char *eol = strchrnul(p, '\n');
>> -             if (parse_insn_line(p, eol, &item) < 0)
>> -                     return error(_("Could not parse line %d."), i);
>> +             if (parse_insn_line(p, eol, &item, i) < 0)
>> +                     return -1;
>
> Not related to this patch, but why the "< 0" test?  It makes this
> reader worry that there is some unusual return value convention that
> he should be taking into account.

Right.  Will fix.

Thanks.

-- Ram

^ permalink raw reply

* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Ramkumar Ramachandra @ 2012-01-08 20:07 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108200910.GK1942@burratino>

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
>
>>> So what change does the patch actually make?  Is this a renaming?
>>
>> Yes, it renames "action" to "command" where appropriate.
>
> Wouldn't a simple renaming have a diffstat with the same number of added
> and removed lines?

Yes, almost.  A few extra lines added because I needed a new data enum
for the "command"; also added a convenience function: action_name().

-- Ram

^ permalink raw reply

* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 20:03 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108193749.GH1942@burratino>

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> When we allow mixing "revert" and "pick" instructions in the same
>> sheet in the next patch, the following workflow would be perfectly
>> valid:
>>
>>   $ git cherry-pick base..latercommit
>>   [conflict occurs]
>>   $ edit problematicfile
>>   $ git add problematicfile
>>   $ git revert --continue
>>   [finishes successfully]
>
> Does "workflow" mean "sequence of commands"?

Yes.  Clarified wording.

>> This is confusing to the operator, because the sequencer is an
>> implementation detail hidden behind the 'git cherry-pick' and 'git
>> revert' builtins.
>
> I don't know --- it's not confusing to me.  Could you explain further
> what harm the current behavior does?  E.g., could it cause me to
> misunderstand some basic concepts, or could it lead me to run commands
> that cause me to scratch my head or lose data?

Junio explained this to me in [1].  It's very unnatural for a user to
want to execute "git cherry-pick --continue" when the previous command
was a "git revert": it probably means that she forgot about the
in-progress "git revert".  The problem becomes more serious when the
sequencer grows more capabilities: a "git merge --continue" to
continue a "git am" sounds much more absurd.  Ofcourse, we will
provide a way to continue any sequencer operation in the future: "git
continue" seems to be a good candidate.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/185355

Thanks.

-- Ram

^ permalink raw reply

* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 20:09 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0kHn+SDaeRHa8bUHWvOEVkr01sVDzvnw9E+-Nne2cii1Q@mail.gmail.com>

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>> So what change does the patch actually make?  Is this a renaming?
>
> Yes, it renames "action" to "command" where appropriate.

Wouldn't a simple renaming have a diffstat with the same number of added
and removed lines?

^ permalink raw reply

* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2012-01-08 20:07 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> Three kinds of errors can arise from parsing '.git/sequencer/todo':
> 1. Unrecognized action
> 2. Malformed object name
> 3. Object name does not refer to a valid commit
>
> Since we would like to make the instruction sheet user-editable in the
> future (much like the 'rebase -i' sheet), report more fine-grained
> parse errors prefixed with the filename and line number.

Seems like a sensible idea.  In other words, the infrastructure that
parses .git/sequencer/todo is meant to handle arbitrary user input
some day, so it can be used as the implementation of "git rebase
--interactive" and "git sequence --edit", and currently it is
suboptimal for that purpose because the parse error messages just say

	error: Could not parse line 5.

This patch shifts responsibility to parse_insn_line(), which can come
up with a more detailed message like

	error: .git/sequencer/todo:5: unrecognized action "frobnicate"

Once the operator is allowed to edit the sequence, the message might
be adjusted to something meaning

	error: <sequence you just gave me>:5: unrecognized action "frobnicate"

instead of exposing an implementation detail, or maybe some day "git
sequence --edit" could even re-launch the editor with an error message
in a comment before the problematic line and the cursor pointing
there.  And for now, pointing to the explicit filename is useful since
this should only happen if there was filesystem corruption, tampering,
or a git bug.

By the way, an example of the output before and after the patch would
have been useful in saving some trouble of figuring this out.

Ok, onward to the patch.

[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>  	return 0;
>  }
>  
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> +			struct replay_insn_list *item, int lineno)
>  {
> +	const char *todo_file = git_path(SEQ_TODO_FILE);

This idiom is _still_ scary.  I don't want to have to shout about
this, but why didn't the commit message at least acknowledge it to put
the reader's mind at ease when this has come up again and again?

[...]
> +		bol += strlen("revert ");
> +	} else {
> +		error_len = eol - bol > 255 ? 255 : eol - bol;
> +		return error(_("%s:%d: Unrecognized action: %.*s"),
> +			todo_file, lineno, (int)error_len, bol);

Ah, so my example above was wrong: the actual message is

	error: .git/sequencer/todo:5: Unrecognized action: the quick bro
	wn fox jumps over the lazy dog

I guess that's fine.  Is it intended?

> +	}
>  
>  	/* Eat up extra spaces/ tabs before object name */
> -	padding = strspn(bol, " \t");
> -	if (!padding)
> -		return -1;
> -	bol += padding;
> +	bol += strspn(bol, " \t");

What does this have to do with the stated goal of the patch?  It seems
to me that an unrelated and unadvertised bugfix snuck in.

[...]
> @@ -741,12 +744,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>  	status = get_sha1(bol, commit_sha1);
>  	*end_of_object_name = saved;
>  
> -	if (status < 0)
> -		return -1;
> +	if (status < 0) {
> +		error_len = eol - bol > 255 ? 255 : eol - bol;
> +		return error(_("%s:%d: Malformed object name: %.*s"),
> +			todo_file, lineno, (int)error_len, bol);
> +	}

This seems a little repetitive --- maybe it would make sense to
factor out a function to emit errors of the form

	error: file:lineno: message: line

By the way, this is gross.  Probably get_sha1 should grow a variant
that takes a buffer and a length.

[...]
>  	item->operand = lookup_commit_reference(commit_sha1);
> -	if (!item->operand)
> -		return -1;
> +	if (!item->operand) {
> +		error_len = eol - bol > 255 ? 255 : eol - bol;
> +		return error(_("%s:%d: Not a valid commit: %.*s"),
> +			todo_file, lineno, (int)error_len, bol);
> +	}

Hmm, this one can be emitted even when there was no corruption or
internal error, because the user removed a commit she was
cherry-picking and the gc-ed before a "git cherry-pick --continue".
Alternatively, it can happen because the repository has grown very
crowded and what used to be an unambiguous commit name no longer is
one (not enough digits).  Will the error message be intuitive in these
situations?

[...]
> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>  
>  	for (i = 1; *p; i++) {
>  		char *eol = strchrnul(p, '\n');
> -		if (parse_insn_line(p, eol, &item) < 0)
> -			return error(_("Could not parse line %d."), i);
> +		if (parse_insn_line(p, eol, &item, i) < 0)
> +			return -1;

Not related to this patch, but why the "< 0" test?  It makes this
reader worry that there is some unusual return value convention that
he should be taking into account.

To sum up: the idea looks promising, but this patch doesn't seem to be
ready yet.

Hope that helps,
Jonathan

^ permalink raw reply

* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Ramkumar Ramachandra @ 2012-01-08 19:53 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108193454.GG1942@burratino>

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>>                                                  So, while a future
>> instruction sheet would look like:
>>
>>   pick next~4
>>   action3 b74fea
>>   revert rr/moo^2~34
>>
>> the actions "pick", "action3" and "revert" need not necessarily
>> correspond to the specific builtins.
>
> So what change does the patch actually make?  Is this a renaming?

Yes, it renames "action" to "command" where appropriate.

> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -64,16 +64,21 @@ struct replay_opts {
>>
>>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>>
>> -static const char *action_name(const struct replay_opts *opts)
>> +static const char *command_name(struct replay_opts *opts)
>
> Why is the const being dropped?  I'm lost, so not reading further.

Minor error.  The rest of the patch should be fine.

-- Ram

^ permalink raw reply

* Re: [PATCH 0/6] The move to sequencer.c
From: Ramkumar Ramachandra @ 2012-01-08 19:51 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Git List
In-Reply-To: <20120108192853.GE1942@burratino>

Jonathan Nieder wrote:
> For the sake of new and forgetful readers: what is that objective?

The objective is to build a generalized sequencer, of which "git
revert" and "git cherry-pick" are two special cases.  We can later
extend it to encompass more builtins and other commands.  Why?  In
general, we want uniform semantics to continue, skip, and abort any
operation that requires user-intervention (example: a conflict that
needs to be resolved before proceeding).  A lot of this work is
inspired by the way "rebase -i" works: it presents the user an
instruction sheet with a sequence of actions to perform.  Example
future use cases:

  $ git cherry-pick moo..foo
  [conflict]
  $ edit problematicfile
  $ git add problematicfile
  $ git continue
  [finishes successfully]

  $ git revert moo..foo
  [conflict]
  $ edit problematicfile
  $ git add problematicfile
  $ git continue
  [finishes successfully]

In these examples, the instruction sheet is uniformly filled with
"pick" or "revert" actions, which is not very interesting.  When we
get an interface to allow easy editing of the instruction sheet and
encompass more builtins, more interesting sequences of operations will
be possible like:

  pick rr/revert-cherry-pick^2~34
  revert master@{12}
  merge next
  am /tmp/jrn.patch

Thanks.

-- Ram

^ permalink raw reply

* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Jonathan Nieder @ 2012-01-08 19:40 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-5-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all lines have the
> same action.

Yes, I've always liked this one.

Haven't re-read the patch to avoid wasted effort if there are changes
when the previous patches in the series change.  Maybe it would be
possible to send as a standalone?

^ permalink raw reply

* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 19:37 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-4-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> When we allow mixing "revert" and "pick" instructions in the same
> sheet in the next patch, the following workflow would be perfectly
> valid:
>
>   $ git cherry-pick base..latercommit
>   [conflict occurs]
>   $ edit problematicfile
>   $ git add problematicfile
>   $ git revert --continue
>   [finishes successfully]

Does "workflow" mean "sequence of commands"?

> This is confusing to the operator, because the sequencer is an
> implementation detail hidden behind the 'git cherry-pick' and 'git
> revert' builtins.

I don't know --- it's not confusing to me.  Could you explain further
what harm the current behavior does?  E.g., could it cause me to
misunderstand some basic concepts, or could it lead me to run commands
that cause me to scratch my head or lose data?

>  builtin/revert.c                |   57 +++++++++++++++++++++++++++++++++++++++
>  sequencer.h                     |    1 +
>  t/t3510-cherry-pick-sequence.sh |   11 +++++++
>  3 files changed, 69 insertions(+), 0 deletions(-)

I haven't read the patch, but based on the above rationale and this
diffstat, it doesn't look good.

^ permalink raw reply

* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 19:34 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-3-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

>                                                  So, while a future
> instruction sheet would look like:
>
>   pick next~4
>   action3 b74fea
>   revert rr/moo^2~34
>
> the actions "pick", "action3" and "revert" need not necessarily
> correspond to the specific builtins.

So what change does the patch actually make?  Is this a renaming?

[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -64,16 +64,21 @@ struct replay_opts {
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> -static const char *action_name(const struct replay_opts *opts)
> +static const char *command_name(struct replay_opts *opts)

Why is the const being dropped?  I'm lost, so not reading further.

^ permalink raw reply

* Re: [PATCH 1/6] revert: move replay_action, replay_subcommand to header
From: Jonathan Nieder @ 2012-01-08 19:31 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-2-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> Our plan to build a sequencer involves leaving the revert builtin with
> just argument parsing work.  Since the enums replay_action and
> replay_subcommand have nothing to do with this argument parsing, move
> them to sequencer.h in advance.
>
> "REVERT" and "CHERRY_PICK" are unsuitable variable names for exposing
> publicly, as their purpose is unclear in the global context: rename
> them to "REPLAY_REVERT" and "REPLAY_PICK" respectively.

My first reaction: this probably would be more self-explanatory if
squashed with the patch the moves the rest of the code to sequencer.[ch].

Second reaction: ah, but the s/REVERT/REPLAY_REVERT/ and
s/CHERRY_PICK/REPLAY_PICK/ changes would be lost in the noise of the move.
Maybe it would be possible to make those changes but keep the enum in
builtin/revert.c, explaining that this is a preparatory step and they
will be moving in a few moments?

Hope that helps,
Jonathan

^ permalink raw reply

* Re: [PATCH 0/6] The move to sequencer.c
From: Jonathan Nieder @ 2012-01-08 19:28 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

(-cc: Junio)
Ramkumar Ramachandra wrote:

> I've tried a slightly different approach: the objective of the patches
> seem to be much clearer this time.

For the sake of new and forgetful readers: what is that objective?

^ permalink raw reply

* Re: SVN -> Git *but* with special changes
From: Thomas Hochstein @ 2012-01-08 11:24 UTC (permalink / raw)
  To: git
In-Reply-To: <1326000327637-7163752.post@n2.nabble.com>

Abscissa wrote:

> Well that's strange, it finished "upgrading", but now git is still just
> reporting 1.7.0.4, which is *exactly* the same version it said before. 

What kind of distribution do you use?

"apt-get" sounds like Debian or Ubuntu, but those all have at least
git 1.7.1:

| Debian versions:
| stable      1:1.7.2.5-3 
| stable-bpo  1:1.7.7.3-1~bpo60+2 
| testing     1:1.7.7.3-1 
| unstable    1:1.7.8.2-1 
| exp         1:1.7.8~rc3-1 

| Ubuntu versions:
| Precise Pangolin  1:1.7.7.3-1
| Oneiric Ocelot    1:1.7.5.4-1
| Natty Narwhal     1:1.7.4.1-3
| Maverick Meerkat  1:1.7.1-1.1ubuntu0.1

-thh

^ permalink raw reply

* Re: SVN -> Git *but* with special changes
From: Thomas Hochstein @ 2012-01-08 11:20 UTC (permalink / raw)
  To: git
In-Reply-To: <1325999865995-7163737.post@n2.nabble.com>

Abscissa schrieb:

> Ok, I see. It's reporting 1.7.0, so that would explain it. One "sudo apt-get
> upgrade git" and...erm...well, it seems to be upgrading my whole damn
> computer, but I'll see how it all works out. Thanks!

"apt-get upgrade" will upgrade _all_ packages and doesn't take a
parameter:
| upgrade
|    upgrade is used to install the newest versions of all packages
|    currently installed on the system from the sources enumerated in
|    /etc/apt/sources.list. 

-thh

^ permalink raw reply

* [PATCH 6/6] sequencer: factor code out of revert builtin
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

Expose the cherry-picking machinery through a public
sequencer_pick_revisions() (renamed from pick_revisions() in
builtin/revert.c), so that cherry-picking and reverting are special
cases of a general sequencer operation.  The cherry-pick builtin is
now a thin wrapper that does command-line argument parsing before
calling into sequencer_pick_revisions().  In the future, we can write
a new "foo" builtin that calls into the sequencer like:

  memset(&opts, 0, sizeof(opts));
  opts.command = REPLAY_CMD_FOO;
  opts.revisions = xmalloc(sizeof(*opts.revs));
  parse_args_populate_opts(argc, argv, &opts);
  init_revisions(opts.revs);
  sequencer_pick_revisions(&opts);

This patch does not intend to make any functional changes.  Check
with:

  $ git blame -s -C HEAD^..HEAD -- sequencer.c | grep -C3 '^[^^]'

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c | 1006 +-----------------------------------------------------
 sequencer.c      |  987 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 sequencer.h      |   25 ++
 3 files changed, 1013 insertions(+), 1005 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 35553e7..47f71f3 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,19 +1,9 @@
 #include "cache.h"
 #include "builtin.h"
-#include "object.h"
-#include "commit.h"
-#include "tag.h"
-#include "run-command.h"
-#include "exec_cmd.h"
-#include "utf8.h"
 #include "parse-options.h"
-#include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
 #include "rerere.h"
-#include "merge-recursive.h"
-#include "refs.h"
-#include "dir.h"
 #include "sequencer.h"
 
 /*
@@ -39,43 +29,11 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-struct replay_opts {
-	enum replay_command command;
-	enum replay_subcommand subcommand;
-
-	/* Boolean options */
-	int edit;
-	int record_origin;
-	int no_commit;
-	int signoff;
-	int allow_ff;
-	int allow_rerere_auto;
-
-	int mainline;
-
-	/* Merge strategy */
-	const char *strategy;
-	const char **xopts;
-	size_t xopts_nr, xopts_alloc;
-
-	/* Only used by REPLAY_NONE */
-	struct rev_info *revs;
-};
-
-#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-
 static const char *command_name(struct replay_opts *opts)
 {
 	return opts->command == REPLAY_CMD_REVERT ? "revert" : "cherry-pick";
 }
 
-static const char *action_name(enum replay_action action)
-{
-	return action == REPLAY_REVERT ? "revert" : "pick";
-}
-
-static char *get_encoding(const char *message);
-
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
 	return opts->command == REPLAY_CMD_REVERT ? revert_usage : cherry_pick_usage;
@@ -234,966 +192,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		usage_with_options(usage_str, options);
 }
 
-struct commit_message {
-	char *parent_label;
-	const char *label;
-	const char *subject;
-	char *reencoded_message;
-	const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
-	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
-	char *q;
-
-	if (!commit->buffer)
-		return -1;
-	encoding = get_encoding(commit->buffer);
-	if (!encoding)
-		encoding = "UTF-8";
-	if (!git_commit_encoding)
-		git_commit_encoding = "UTF-8";
-
-	out->reencoded_message = NULL;
-	out->message = commit->buffer;
-	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(commit->buffer,
-					git_commit_encoding, encoding);
-	if (out->reencoded_message)
-		out->message = out->reencoded_message;
-
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
-	subject_len = find_commit_subject(out->message, &subject);
-
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
-			      strlen("... ") + subject_len + 1);
-	q = out->parent_label;
-	q = mempcpy(q, "parent of ", strlen("parent of "));
-	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
-	q = mempcpy(q, "... ", strlen("... "));
-	out->subject = q;
-	q = mempcpy(q, subject, subject_len);
-	*q = '\0';
-	return 0;
-}
-
-static void free_message(struct commit_message *msg)
-{
-	free(msg->parent_label);
-	free(msg->reencoded_message);
-}
-
-static char *get_encoding(const char *message)
-{
-	const char *p = message, *eol;
-
-	while (*p && *p != '\n') {
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-		if (!prefixcmp(p, "encoding ")) {
-			char *result = xmalloc(eol - 8 - p);
-			strlcpy(result, p + 9, eol - 8 - p);
-			return result;
-		}
-		p = eol;
-		if (*p == '\n')
-			p++;
-	}
-	return NULL;
-}
-
-static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
-{
-	const char *filename;
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
-	filename = git_path("%s", pseudoref);
-	fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"), filename);
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), filename);
-	strbuf_release(&buf);
-}
-
-static void print_advice(int show_hint)
-{
-	char *msg = getenv("GIT_CHERRY_PICK_HELP");
-
-	if (msg) {
-		fprintf(stderr, "%s\n", msg);
-		/*
-		 * A conflict has occured but the porcelain
-		 * (typically rebase --interactive) wants to take care
-		 * of the commit itself so remove CHERRY_PICK_HEAD
-		 */
-		unlink(git_path("CHERRY_PICK_HEAD"));
-		return;
-	}
-
-	if (show_hint) {
-		advise("after resolving the conflicts, mark the corrected paths");
-		advise("with 'git add <paths>' or 'git rm <paths>'");
-		advise("and commit the result with 'git commit'");
-	}
-}
-
-static void write_message(struct strbuf *msgbuf, const char *filename)
-{
-	static struct lock_file msg_file;
-
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
-	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s"), filename);
-	strbuf_release(msgbuf);
-	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s"), filename);
-}
-
-static struct tree *empty_tree(void)
-{
-	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
-}
-
-static int error_dirty_index(struct replay_opts *opts)
-{
-	if (read_cache_unmerged())
-		return error_resolve_conflict(command_name(opts));
-
-	/* Different translation strings for cherry-pick and revert */
-	if (opts->command == REPLAY_CMD_CHERRY_PICK)
-		error(_("Your local changes would be overwritten by cherry-pick."));
-	else
-		error(_("Your local changes would be overwritten by revert."));
-
-	if (advice_commit_before_merge)
-		advise(_("Commit your changes or stash them to proceed."));
-	return -1;
-}
-
-static int fast_forward_to(const unsigned char *to, const unsigned char *from)
-{
-	struct ref_lock *ref_lock;
-
-	read_cache();
-	if (checkout_fast_forward(from, to))
-		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
-	return write_ref_sha1(ref_lock, to, "cherry-pick");
-}
-
-static int do_recursive_merge(struct commit *base, struct commit *next,
-			      const char *base_label, const char *next_label,
-			      unsigned char *head, struct strbuf *msgbuf,
-			      struct replay_opts *opts)
-{
-	struct merge_options o;
-	struct tree *result, *next_tree, *base_tree, *head_tree;
-	int clean, index_fd;
-	const char **xopt;
-	static struct lock_file index_lock;
-
-	index_fd = hold_locked_index(&index_lock, 1);
-
-	read_cache();
-
-	init_merge_options(&o);
-	o.ancestor = base ? base_label : "(empty tree)";
-	o.branch1 = "HEAD";
-	o.branch2 = next ? next_label : "(empty tree)";
-
-	head_tree = parse_tree_indirect(head);
-	next_tree = next ? next->tree : empty_tree();
-	base_tree = base ? base->tree : empty_tree();
-
-	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
-		parse_merge_opt(&o, *xopt);
-
-	clean = merge_trees(&o,
-			    head_tree,
-			    next_tree, base_tree, &result);
-
-	if (active_cache_changed &&
-	    (write_cache(index_fd, active_cache, active_nr) ||
-	     commit_locked_index(&index_lock)))
-		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), command_name(opts));
-	rollback_lock_file(&index_lock);
-
-	if (!clean) {
-		int i;
-		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
-		for (i = 0; i < active_nr;) {
-			struct cache_entry *ce = active_cache[i++];
-			if (ce_stage(ce)) {
-				strbuf_addch(msgbuf, '\t');
-				strbuf_addstr(msgbuf, ce->name);
-				strbuf_addch(msgbuf, '\n');
-				while (i < active_nr && !strcmp(ce->name,
-						active_cache[i]->name))
-					i++;
-			}
-		}
-	}
-
-	return !clean;
-}
-
-/*
- * If we are cherry-pick, and if the merge did not result in
- * hand-editing, we will hit this commit and inherit the original
- * author date and name.
- * If we are revert, or if our cherry-pick results in a hand merge,
- * we had better say that the current user is responsible for that.
- */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
-{
-	/* 6 is max possible length of our args array including NULL */
-	const char *args[6];
-	int i = 0;
-
-	args[i++] = "commit";
-	args[i++] = "-n";
-	if (opts->signoff)
-		args[i++] = "-s";
-	if (!opts->edit) {
-		args[i++] = "-F";
-		args[i++] = defmsg;
-	}
-	args[i] = NULL;
-
-	return run_command_v_opt(args, RUN_GIT_CMD);
-}
-
-static int do_pick_commit(struct commit *commit, enum replay_action action,
-			struct replay_opts *opts)
-{
-	unsigned char head[20];
-	struct commit *base, *next, *parent;
-	const char *base_label, *next_label;
-	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
-	char *defmsg = NULL;
-	struct strbuf msgbuf = STRBUF_INIT;
-	int res;
-
-	if (opts->no_commit) {
-		/*
-		 * We do not intend to commit immediately.  We just want to
-		 * merge the differences in, so let's compute the tree
-		 * that represents the "current" state for merge-recursive
-		 * to work on.
-		 */
-		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
-	} else {
-		if (get_sha1("HEAD", head))
-			return error(_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(opts);
-	}
-	discard_cache();
-
-	if (!commit->parents) {
-		parent = NULL;
-	}
-	else if (commit->parents->next) {
-		/* Reverting or cherry-picking a merge commit */
-		int cnt;
-		struct commit_list *p;
-
-		if (!opts->mainline)
-			return error(_("Commit %s is a merge but no -m option was given."),
-				sha1_to_hex(commit->object.sha1));
-
-		for (cnt = 1, p = commit->parents;
-		     cnt != opts->mainline && p;
-		     cnt++)
-			p = p->next;
-		if (cnt != opts->mainline || !p)
-			return error(_("Commit %s does not have parent %d"),
-				sha1_to_hex(commit->object.sha1), opts->mainline);
-		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("Mainline was specified but commit %s is not a merge."),
-			sha1_to_hex(commit->object.sha1));
-	else
-		parent = commit->parents->item;
-
-	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
-		return fast_forward_to(commit->object.sha1, head);
-
-	if (parent && parse_commit(parent) < 0)
-		/* TRANSLATORS: The first %s will be "revert" or
-		   "cherry-pick", the second %s a SHA1 */
-		return error(_("%s: cannot parse parent commit %s"),
-			command_name(opts), sha1_to_hex(parent->object.sha1));
-
-	if (get_message(commit, &msg) != 0)
-		return error(_("Cannot get commit message for %s"),
-			sha1_to_hex(commit->object.sha1));
-
-	/*
-	 * "commit" is an existing commit.  We would want to apply
-	 * the difference it introduces since its first parent "prev"
-	 * on top of the current HEAD if we are cherry-pick.  Or the
-	 * reverse of it if we are revert.
-	 */
-
-	defmsg = git_pathdup("MERGE_MSG");
-
-	if (action == REPLAY_REVERT) {
-		base = commit;
-		base_label = msg.label;
-		next = parent;
-		next_label = msg.parent_label;
-		strbuf_addstr(&msgbuf, "Revert \"");
-		strbuf_addstr(&msgbuf, msg.subject);
-		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
-		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-
-		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
-		}
-		strbuf_addstr(&msgbuf, ".\n");
-	} else {
-		const char *p;
-
-		base = parent;
-		base_label = msg.parent_label;
-		next = commit;
-		next_label = msg.label;
-
-		/*
-		 * Append the commit log message to msgbuf; it starts
-		 * after the tree, parent, author, committer
-		 * information followed by "\n\n".
-		 */
-		p = strstr(msg.message, "\n\n");
-		if (p) {
-			p += 2;
-			strbuf_addstr(&msgbuf, p);
-		}
-
-		if (opts->record_origin) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-			strbuf_addstr(&msgbuf, ")\n");
-		}
-	}
-
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
-		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf, opts);
-		write_message(&msgbuf, defmsg);
-	} else {
-		struct commit_list *common = NULL;
-		struct commit_list *remotes = NULL;
-
-		write_message(&msgbuf, defmsg);
-
-		commit_list_insert(base, &common);
-		commit_list_insert(next, &remotes);
-		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
-					common, sha1_to_hex(head), remotes);
-		free_commit_list(common);
-		free_commit_list(remotes);
-	}
-
-	/*
-	 * If the merge was clean or if it failed due to conflict, we write
-	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
-	 * However, if the merge did not even start, then we don't want to
-	 * write it at all.
-	 */
-	if (action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
-		write_cherry_pick_head(commit, "REVERT_HEAD");
-
-	if (res) {
-		error(action == REPLAY_REVERT
-		      ? _("could not revert %s... %s")
-		      : _("could not apply %s... %s"),
-		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
-		      msg.subject);
-		print_advice(res == 1);
-		rerere(opts->allow_rerere_auto);
-	} else {
-		if (!opts->no_commit)
-			res = run_git_commit(defmsg, opts);
-	}
-
-	free_message(&msg);
-	free(defmsg);
-
-	return res;
-}
-
-static void prepare_revs(struct replay_opts *opts)
-{
-	if (opts->command != REPLAY_CMD_REVERT)
-		opts->revs->reverse ^= 1;
-
-	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
-
-	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
-}
-
-static void read_and_refresh_cache(struct replay_opts *opts)
-{
-	static struct lock_file index_lock;
-	int index_fd = hold_locked_index(&index_lock, 0);
-	if (read_index_preload(&the_index, NULL) < 0)
-		die(_("git %s: failed to read the index"), command_name(opts));
-	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-	if (the_index.cache_changed) {
-		if (write_index(&the_index, index_fd) ||
-		    commit_locked_index(&index_lock))
-			die(_("git %s: failed to refresh the index"),
-				command_name(opts));
-	}
-	rollback_lock_file(&index_lock);
-}
-
-/*
- * Append a (commit, action) to the end of the replay_insn_list.
- *
- * next starts by pointing to the variable that holds the head of an
- * empty replay_insn_list, and is updated to point to the "next" field of
- * the last item on the list as new (commit, action) pairs are appended.
- *
- * Usage example:
- *
- *     struct replay_insn_list *list;
- *     struct replay_insn_list **next = &list;
- *
- *     next = replay_insn_list_append(c1, a1, next);
- *     next = replay_insn_list_append(c2, a2, next);
- *     assert(len(list) == 2);
- *     return list;
- */
-static struct replay_insn_list **replay_insn_list_append(struct commit *operand,
-							enum replay_action action,
-							struct replay_insn_list **next)
-{
-	struct replay_insn_list *new = xmalloc(sizeof(*new));
-	new->action = action;
-	new->operand = operand;
-	*next = new;
-	new->next = NULL;
-	return &new->next;
-}
-
-static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
-{
-	struct replay_insn_list *cur;
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		const char *sha1_abbrev, *action_str, *subject;
-		int subject_len;
-
-		action_str = action_name(cur->action);
-		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->operand->buffer, &subject);
-		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
-			subject_len, subject);
-	}
-	return 0;
-}
-
-static int parse_insn_line(char *bol, char *eol,
-			struct replay_insn_list *item, int lineno)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	unsigned char commit_sha1[20];
-	char *end_of_object_name;
-	int saved, status;
-	size_t error_len;
-
-	if (!prefixcmp(bol, "pick ") || !prefixcmp(bol, "pick\t")) {
-		item->action = REPLAY_PICK;
-		bol += strlen("pick ");
-	} else if (!prefixcmp(bol, "revert ") || !prefixcmp(bol, "revert\t")) {
-		item->action = REPLAY_REVERT;
-		bol += strlen("revert ");
-	} else {
-		error_len = eol - bol > 255 ? 255 : eol - bol;
-		return error(_("%s:%d: Unrecognized action: %.*s"),
-			todo_file, lineno, (int)error_len, bol);
-	}
-
-	/* Eat up extra spaces/ tabs before object name */
-	bol += strspn(bol, " \t");
-
-	end_of_object_name = bol + strcspn(bol, " \t\n");
-	saved = *end_of_object_name;
-	*end_of_object_name = '\0';
-	status = get_sha1(bol, commit_sha1);
-	*end_of_object_name = saved;
-
-	if (status < 0) {
-		error_len = eol - bol > 255 ? 255 : eol - bol;
-		return error(_("%s:%d: Malformed object name: %.*s"),
-			todo_file, lineno, (int)error_len, bol);
-	}
-
-	item->operand = lookup_commit_reference(commit_sha1);
-	if (!item->operand) {
-		error_len = eol - bol > 255 ? 255 : eol - bol;
-		return error(_("%s:%d: Not a valid commit: %.*s"),
-			todo_file, lineno, (int)error_len, bol);
-	}
-
-	item->next = NULL;
-	return 0;
-}
-
-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
-{
-	struct replay_insn_list **next = todo_list;
-	struct replay_insn_list item = { NULL, 0, NULL };
-	char *p = buf;
-	int i;
-
-	for (i = 1; *p; i++) {
-		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item, i) < 0)
-			return -1;
-		next = replay_insn_list_append(item.operand, item.action, next);
-		p = *eol ? eol + 1 : eol;
-	}
-	if (!*todo_list)
-		return error(_("No commits parsed."));
-	return 0;
-}
-
-static void read_populate_todo(struct replay_insn_list **todo_list)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	struct strbuf buf = STRBUF_INIT;
-	int fd, res;
-
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		die_errno(_("Could not open %s"), todo_file);
-	if (strbuf_read(&buf, fd, 0) < 0) {
-		close(fd);
-		strbuf_release(&buf);
-		die(_("Could not read %s."), todo_file);
-	}
-	close(fd);
-
-	res = parse_insn_buffer(buf.buf, todo_list);
-	strbuf_release(&buf);
-	if (res)
-		die(_("Unusable instruction sheet: %s"), todo_file);
-}
-
-static int populate_opts_cb(const char *key, const char *value, void *data)
-{
-	struct replay_opts *opts = data;
-	int error_flag = 1;
-
-	if (!value)
-		error_flag = 0;
-	else if (!strcmp(key, "options.no-commit"))
-		opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.edit"))
-		opts->edit = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.signoff"))
-		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.record-origin"))
-		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.allow-ff"))
-		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.mainline"))
-		opts->mainline = git_config_int(key, value);
-	else if (!strcmp(key, "options.strategy"))
-		git_config_string(&opts->strategy, key, value);
-	else if (!strcmp(key, "options.strategy-option")) {
-		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-		opts->xopts[opts->xopts_nr++] = xstrdup(value);
-	} else
-		return error(_("Invalid key: %s"), key);
-
-	if (!error_flag)
-		return error(_("Invalid value for %s: %s"), key, value);
-
-	return 0;
-}
-
-static void read_populate_opts(struct replay_opts **opts_ptr)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (!file_exists(opts_file))
-		return;
-	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), opts_file);
-}
-
-static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
-				struct replay_opts *opts)
-{
-	struct commit *commit;
-	struct replay_insn_list **next;
-	enum replay_action action;
-
-	prepare_revs(opts);
-
-	next = todo_list;
-	action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
-	while ((commit = get_revision(opts->revs)))
-		next = replay_insn_list_append(commit, action, next);
-}
-
-static int create_seq_dir(void)
-{
-	const char *seq_dir = git_path(SEQ_DIR);
-
-	if (file_exists(seq_dir)) {
-		error(_("a cherry-pick or revert is already in progress"));
-		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
-		return -1;
-	}
-	else if (mkdir(seq_dir, 0777) < 0)
-		die_errno(_("Could not create sequencer directory %s"), seq_dir);
-	return 0;
-}
-
-static enum replay_command read_cmd(void)
-{
-	const char *cmd_file = git_path(SEQ_CMD_FILE);
-	struct strbuf buf = STRBUF_INIT;
-	enum replay_command res;
-	int fd;
-
-	fd = open(cmd_file, O_RDONLY);
-	if (fd < 0)
-		die_errno(_("Could not open %s"), cmd_file);
-	if (strbuf_read(&buf, fd, 0) < 0) {
-		close(fd);
-		strbuf_release(&buf);
-		die(_("Could not read %s."), cmd_file);
-	}
-	close(fd);
-
-	if (!strcmp(buf.buf, "revert\n"))
-		res = REPLAY_CMD_REVERT;
-	else if (!strcmp(buf.buf, "cherry-pick\n"))
-		res = REPLAY_CMD_CHERRY_PICK;
-	else {
-		strbuf_release(&buf);
-		die(_("Malformed command file: %s"), cmd_file);
-	}
-	strbuf_release(&buf);
-	return res;
-}
-
-static void save_cmd(struct replay_opts *opts)
-{
-	const char *cmd_file = git_path(SEQ_CMD_FILE);
-	static struct lock_file cmd_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&cmd_lock, cmd_file, LOCK_DIE_ON_ERROR);
-	strbuf_addf(&buf, "%s\n", command_name(opts));
-	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s"), cmd_file);
-	if (commit_lock_file(&cmd_lock) < 0)
-		die(_("Error wrapping up %s."), cmd_file);
-}
-
-static void save_head(const char *head)
-{
-	const char *head_file = git_path(SEQ_HEAD_FILE);
-	static struct lock_file head_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
-	strbuf_addf(&buf, "%s\n", head);
-	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s"), head_file);
-	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), head_file);
-}
-
-static int reset_for_rollback(const unsigned char *sha1)
-{
-	const char *argv[4];	/* reset --merge <arg> + NULL */
-	argv[0] = "reset";
-	argv[1] = "--merge";
-	argv[2] = sha1_to_hex(sha1);
-	argv[3] = NULL;
-	return run_command_v_opt(argv, RUN_GIT_CMD);
-}
-
-static int rollback_single_pick(void)
-{
-	unsigned char head_sha1[20];
-
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
-		return error(_("no cherry-pick or revert in progress"));
-	if (read_ref_full("HEAD", head_sha1, 0, NULL))
-		return error(_("cannot resolve HEAD"));
-	if (is_null_sha1(head_sha1))
-		return error(_("cannot abort from a branch yet to be born"));
-	return reset_for_rollback(head_sha1);
-}
-
-static int sequencer_rollback(struct replay_opts *opts)
-{
-	const char *filename;
-	FILE *f;
-	unsigned char sha1[20];
-	struct strbuf buf = STRBUF_INIT;
-
-	filename = git_path(SEQ_HEAD_FILE);
-	f = fopen(filename, "r");
-	if (!f && errno == ENOENT) {
-		/*
-		 * There is no multiple-cherry-pick in progress.
-		 * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
-		 * a single-cherry-pick in progress, abort that.
-		 */
-		return rollback_single_pick();
-	}
-	if (!f)
-		return error(_("cannot open %s: %s"), filename,
-						strerror(errno));
-	if (strbuf_getline(&buf, f, '\n')) {
-		error(_("cannot read %s: %s"), filename, ferror(f) ?
-			strerror(errno) : _("unexpected end of file"));
-		fclose(f);
-		goto fail;
-	}
-	fclose(f);
-	if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
-		error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
-			filename);
-		goto fail;
-	}
-	if (reset_for_rollback(sha1))
-		goto fail;
-	remove_sequencer_state();
-	strbuf_release(&buf);
-	return 0;
-fail:
-	strbuf_release(&buf);
-	return -1;
-}
-
-static void save_todo(struct replay_insn_list *todo_list)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	static struct lock_file todo_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list) < 0)
-		die(_("Could not format %s."), todo_file);
-	if (write_in_full(fd, buf.buf, buf.len) < 0) {
-		strbuf_release(&buf);
-		die_errno(_("Could not write to %s"), todo_file);
-	}
-	if (commit_lock_file(&todo_lock) < 0) {
-		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), todo_file);
-	}
-	strbuf_release(&buf);
-}
-
-static void save_opts(struct replay_opts *opts)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
-	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
-	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
-	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
-	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
-	if (opts->mainline) {
-		struct strbuf buf = STRBUF_INIT;
-		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
-		strbuf_release(&buf);
-	}
-	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
-							"options.strategy-option",
-							opts->xopts[i], "^$", 0);
-	}
-}
-
-static int pick_commits(struct replay_insn_list *todo_list, struct replay_opts *opts)
-{
-	struct replay_insn_list *cur;
-	int res;
-
-	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
-	if (opts->allow_ff)
-		assert(!(opts->signoff || opts->no_commit ||
-				opts->record_origin || opts->edit));
-	read_and_refresh_cache(opts);
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur);
-		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res)
-			return res;
-	}
-
-	/*
-	 * Sequence of picks finished successfully; cleanup by
-	 * removing the .git/sequencer directory
-	 */
-	remove_sequencer_state();
-	return 0;
-}
-
-static int continue_single_pick(void)
-{
-	const char *argv[] = { "commit", NULL };
-
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
-		return error(_("no cherry-pick or revert in progress"));
-	return run_command_v_opt(argv, RUN_GIT_CMD);
-}
-
-static int sequencer_continue(struct replay_opts *opts)
-{
-	struct replay_insn_list *todo_list = NULL;
-	enum replay_command cmd;
-
-	if (!file_exists(git_path(SEQ_TODO_FILE)))
-		return continue_single_pick();
-
-	/*
-	 * Disallow continuing a cherry-pick with 'git revert
-	 * --continue' and viceversa
-	 */
-	cmd = read_cmd();
-	if (cmd != opts->command)
-		return error(_("cannot %s: a %s is in progress."),
-			command_name(opts),
-			cmd == REPLAY_CMD_REVERT ? "revert" : "cherry-pick");
-
-	read_populate_opts(&opts);
-	read_populate_todo(&todo_list);
-
-	/* Verify that the conflict has been resolved */
-	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
-	    file_exists(git_path("REVERT_HEAD"))) {
-		int ret = continue_single_pick();
-		if (ret)
-			return ret;
-	}
-	if (index_differs_from("HEAD", 0))
-		return error_dirty_index(opts);
-	todo_list = todo_list->next;
-	return pick_commits(todo_list, opts);
-}
-
-static int single_pick(struct commit *cmit, struct replay_opts *opts)
-{
-	enum replay_action action;
-	action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
-
-	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
-	return do_pick_commit(cmit, action, opts);
-}
-
-static int pick_revisions(struct replay_opts *opts)
-{
-	struct replay_insn_list *todo_list = NULL;
-	unsigned char sha1[20];
-
-	if (opts->subcommand == REPLAY_NONE)
-		assert(opts->revs);
-
-	read_and_refresh_cache(opts);
-
-	/*
-	 * Decide what to do depending on the arguments; a fresh
-	 * cherry-pick should be handled differently from an existing
-	 * one that is being continued
-	 */
-	if (opts->subcommand == REPLAY_REMOVE_STATE) {
-		remove_sequencer_state();
-		return 0;
-	}
-	if (opts->subcommand == REPLAY_ROLLBACK)
-		return sequencer_rollback(opts);
-	if (opts->subcommand == REPLAY_CONTINUE)
-		return sequencer_continue(opts);
-
-	/*
-	 * If we were called as "git cherry-pick <commit>", just
-	 * cherry-pick/revert it, set CHERRY_PICK_HEAD /
-	 * REVERT_HEAD, and don't touch the sequencer state.
-	 * This means it is possible to cherry-pick in the middle
-	 * of a cherry-pick sequence.
-	 */
-	if (opts->revs->cmdline.nr == 1 &&
-	    opts->revs->cmdline.rev->whence == REV_CMD_REV &&
-	    opts->revs->no_walk &&
-	    !opts->revs->cmdline.rev->flags) {
-		struct commit *cmit;
-		if (prepare_revision_walk(opts->revs))
-			die(_("revision walk setup failed"));
-		cmit = get_revision(opts->revs);
-		if (!cmit || get_revision(opts->revs))
-			die("BUG: expected exactly one commit from walk");
-		return single_pick(cmit, opts);
-	}
-
-	/*
-	 * Start a new cherry-pick/ revert sequence; but
-	 * first, make sure that an existing one isn't in
-	 * progress
-	 */
-
-	walk_revs_populate_todo(&todo_list, opts);
-	if (create_seq_dir() < 0)
-		return -1;
-	if (get_sha1("HEAD", sha1)) {
-		if (opts->command == REPLAY_CMD_REVERT)
-			return error(_("Can't revert as initial commit"));
-		return error(_("Can't cherry-pick into empty head"));
-	}
-	save_cmd(opts);
-	save_head(sha1_to_hex(sha1));
-	save_opts(opts);
-	return pick_commits(todo_list, opts);
-}
-
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -1205,7 +203,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	opts.command = REPLAY_CMD_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -1220,7 +218,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.command = REPLAY_CMD_CHERRY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
diff --git a/sequencer.c b/sequencer.c
index d1f28a6..a7e494c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,7 +1,20 @@
 #include "cache.h"
 #include "sequencer.h"
-#include "strbuf.h"
 #include "dir.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
+
+#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 void remove_sequencer_state(void)
 {
@@ -11,3 +24,975 @@ void remove_sequencer_state(void)
 	remove_dir_recursively(&seq_dir, 0);
 	strbuf_release(&seq_dir);
 }
+
+static const char *command_name(struct replay_opts *opts)
+{
+	return opts->command == REPLAY_CMD_REVERT ? "revert" : "cherry-pick";
+}
+
+static const char *action_name(enum replay_action action)
+{
+	return action == REPLAY_REVERT ? "revert" : "pick";
+}
+
+struct commit_message {
+	char *parent_label;
+	const char *label;
+	const char *subject;
+	char *reencoded_message;
+	const char *message;
+};
+
+static char *get_encoding(const char *message);
+
+static int get_message(struct commit *commit, struct commit_message *out)
+{
+	const char *encoding;
+	const char *abbrev, *subject;
+	int abbrev_len, subject_len;
+	char *q;
+
+	if (!commit->buffer)
+		return -1;
+	encoding = get_encoding(commit->buffer);
+	if (!encoding)
+		encoding = "UTF-8";
+	if (!git_commit_encoding)
+		git_commit_encoding = "UTF-8";
+
+	out->reencoded_message = NULL;
+	out->message = commit->buffer;
+	if (strcmp(encoding, git_commit_encoding))
+		out->reencoded_message = reencode_string(commit->buffer,
+					git_commit_encoding, encoding);
+	if (out->reencoded_message)
+		out->message = out->reencoded_message;
+
+	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+	abbrev_len = strlen(abbrev);
+
+	subject_len = find_commit_subject(out->message, &subject);
+
+	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+			      strlen("... ") + subject_len + 1);
+	q = out->parent_label;
+	q = mempcpy(q, "parent of ", strlen("parent of "));
+	out->label = q;
+	q = mempcpy(q, abbrev, abbrev_len);
+	q = mempcpy(q, "... ", strlen("... "));
+	out->subject = q;
+	q = mempcpy(q, subject, subject_len);
+	*q = '\0';
+	return 0;
+}
+
+static void free_message(struct commit_message *msg)
+{
+	free(msg->parent_label);
+	free(msg->reencoded_message);
+}
+
+static char *get_encoding(const char *message)
+{
+	const char *p = message, *eol;
+
+	while (*p && *p != '\n') {
+		for (eol = p + 1; *eol && *eol != '\n'; eol++)
+			; /* do nothing */
+		if (!prefixcmp(p, "encoding ")) {
+			char *result = xmalloc(eol - 8 - p);
+			strlcpy(result, p + 9, eol - 8 - p);
+			return result;
+		}
+		p = eol;
+		if (*p == '\n')
+			p++;
+	}
+	return NULL;
+}
+
+static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
+{
+	const char *filename;
+	int fd;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+
+	filename = git_path("%s", pseudoref);
+	fd = open(filename, O_WRONLY | O_CREAT, 0666);
+	if (fd < 0)
+		die_errno(_("Could not open '%s' for writing"), filename);
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
+		die_errno(_("Could not write to '%s'"), filename);
+	strbuf_release(&buf);
+}
+
+static void print_advice(int show_hint)
+{
+	char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
+	if (msg) {
+		fprintf(stderr, "%s\n", msg);
+		/*
+		 * A conflict has occured but the porcelain
+		 * (typically rebase --interactive) wants to take care
+		 * of the commit itself so remove CHERRY_PICK_HEAD
+		 */
+		unlink(git_path("CHERRY_PICK_HEAD"));
+		return;
+	}
+
+	if (show_hint) {
+		advise("after resolving the conflicts, mark the corrected paths");
+		advise("with 'git add <paths>' or 'git rm <paths>'");
+		advise("and commit the result with 'git commit'");
+	}
+}
+
+static void write_message(struct strbuf *msgbuf, const char *filename)
+{
+	static struct lock_file msg_file;
+
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
+					       LOCK_DIE_ON_ERROR);
+	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
+		die_errno(_("Could not write to %s"), filename);
+	strbuf_release(msgbuf);
+	if (commit_lock_file(&msg_file) < 0)
+		die(_("Error wrapping up %s"), filename);
+}
+
+static struct tree *empty_tree(void)
+{
+	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
+}
+
+static int error_dirty_index(struct replay_opts *opts)
+{
+	if (read_cache_unmerged())
+		return error_resolve_conflict(command_name(opts));
+
+	/* Different translation strings for cherry-pick and revert */
+	if (opts->command == REPLAY_CMD_CHERRY_PICK)
+		error(_("Your local changes would be overwritten by cherry-pick."));
+	else
+		error(_("Your local changes would be overwritten by revert."));
+
+	if (advice_commit_before_merge)
+		advise(_("Commit your changes or stash them to proceed."));
+	return -1;
+}
+
+static int fast_forward_to(const unsigned char *to, const unsigned char *from)
+{
+	struct ref_lock *ref_lock;
+
+	read_cache();
+	if (checkout_fast_forward(from, to))
+		exit(1); /* the callee should have complained already */
+	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
+	return write_ref_sha1(ref_lock, to, "cherry-pick");
+}
+
+static int do_recursive_merge(struct commit *base, struct commit *next,
+			      const char *base_label, const char *next_label,
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
+{
+	struct merge_options o;
+	struct tree *result, *next_tree, *base_tree, *head_tree;
+	int clean, index_fd;
+	const char **xopt;
+	static struct lock_file index_lock;
+
+	index_fd = hold_locked_index(&index_lock, 1);
+
+	read_cache();
+
+	init_merge_options(&o);
+	o.ancestor = base ? base_label : "(empty tree)";
+	o.branch1 = "HEAD";
+	o.branch2 = next ? next_label : "(empty tree)";
+
+	head_tree = parse_tree_indirect(head);
+	next_tree = next ? next->tree : empty_tree();
+	base_tree = base ? base->tree : empty_tree();
+
+	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
+		parse_merge_opt(&o, *xopt);
+
+	clean = merge_trees(&o,
+			    head_tree,
+			    next_tree, base_tree, &result);
+
+	if (active_cache_changed &&
+	    (write_cache(index_fd, active_cache, active_nr) ||
+	     commit_locked_index(&index_lock)))
+		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
+		die(_("%s: Unable to write new index file"), command_name(opts));
+	rollback_lock_file(&index_lock);
+
+	if (!clean) {
+		int i;
+		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
+		for (i = 0; i < active_nr;) {
+			struct cache_entry *ce = active_cache[i++];
+			if (ce_stage(ce)) {
+				strbuf_addch(msgbuf, '\t');
+				strbuf_addstr(msgbuf, ce->name);
+				strbuf_addch(msgbuf, '\n');
+				while (i < active_nr && !strcmp(ce->name,
+						active_cache[i]->name))
+					i++;
+			}
+		}
+	}
+
+	return !clean;
+}
+
+/*
+ * If we are cherry-pick, and if the merge did not result in
+ * hand-editing, we will hit this commit and inherit the original
+ * author date and name.
+ * If we are revert, or if our cherry-pick results in a hand merge,
+ * we had better say that the current user is responsible for that.
+ */
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+{
+	/* 6 is max possible length of our args array including NULL */
+	const char *args[6];
+	int i = 0;
+
+	args[i++] = "commit";
+	args[i++] = "-n";
+	if (opts->signoff)
+		args[i++] = "-s";
+	if (!opts->edit) {
+		args[i++] = "-F";
+		args[i++] = defmsg;
+	}
+	args[i] = NULL;
+
+	return run_command_v_opt(args, RUN_GIT_CMD);
+}
+
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
+{
+	unsigned char head[20];
+	struct commit *base, *next, *parent;
+	const char *base_label, *next_label;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	char *defmsg = NULL;
+	struct strbuf msgbuf = STRBUF_INIT;
+	int res;
+
+	if (opts->no_commit) {
+		/*
+		 * We do not intend to commit immediately.  We just want to
+		 * merge the differences in, so let's compute the tree
+		 * that represents the "current" state for merge-recursive
+		 * to work on.
+		 */
+		if (write_cache_as_tree(head, 0, NULL))
+			die (_("Your index file is unmerged."));
+	} else {
+		if (get_sha1("HEAD", head))
+			return error(_("You do not have a valid HEAD"));
+		if (index_differs_from("HEAD", 0))
+			return error_dirty_index(opts);
+	}
+	discard_cache();
+
+	if (!commit->parents) {
+		parent = NULL;
+	}
+	else if (commit->parents->next) {
+		/* Reverting or cherry-picking a merge commit */
+		int cnt;
+		struct commit_list *p;
+
+		if (!opts->mainline)
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
+
+		for (cnt = 1, p = commit->parents;
+		     cnt != opts->mainline && p;
+		     cnt++)
+			p = p->next;
+		if (cnt != opts->mainline || !p)
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), opts->mainline);
+		parent = p->item;
+	} else if (0 < opts->mainline)
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
+	else
+		parent = commit->parents->item;
+
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
+		return fast_forward_to(commit->object.sha1, head);
+
+	if (parent && parse_commit(parent) < 0)
+		/* TRANSLATORS: The first %s will be "revert" or
+		   "cherry-pick", the second %s a SHA1 */
+		return error(_("%s: cannot parse parent commit %s"),
+			command_name(opts), sha1_to_hex(parent->object.sha1));
+
+	if (get_message(commit, &msg) != 0)
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
+
+	/*
+	 * "commit" is an existing commit.  We would want to apply
+	 * the difference it introduces since its first parent "prev"
+	 * on top of the current HEAD if we are cherry-pick.  Or the
+	 * reverse of it if we are revert.
+	 */
+
+	defmsg = git_pathdup("MERGE_MSG");
+
+	if (action == REPLAY_REVERT) {
+		base = commit;
+		base_label = msg.label;
+		next = parent;
+		next_label = msg.parent_label;
+		strbuf_addstr(&msgbuf, "Revert \"");
+		strbuf_addstr(&msgbuf, msg.subject);
+		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
+		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents && commit->parents->next) {
+			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
+		}
+		strbuf_addstr(&msgbuf, ".\n");
+	} else {
+		const char *p;
+
+		base = parent;
+		base_label = msg.parent_label;
+		next = commit;
+		next_label = msg.label;
+
+		/*
+		 * Append the commit log message to msgbuf; it starts
+		 * after the tree, parent, author, committer
+		 * information followed by "\n\n".
+		 */
+		p = strstr(msg.message, "\n\n");
+		if (p) {
+			p += 2;
+			strbuf_addstr(&msgbuf, p);
+		}
+
+		if (opts->record_origin) {
+			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+			strbuf_addstr(&msgbuf, ")\n");
+		}
+	}
+
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
+		res = do_recursive_merge(base, next, base_label, next_label,
+					 head, &msgbuf, opts);
+		write_message(&msgbuf, defmsg);
+	} else {
+		struct commit_list *common = NULL;
+		struct commit_list *remotes = NULL;
+
+		write_message(&msgbuf, defmsg);
+
+		commit_list_insert(base, &common);
+		commit_list_insert(next, &remotes);
+		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+					common, sha1_to_hex(head), remotes);
+		free_commit_list(common);
+		free_commit_list(remotes);
+	}
+
+	/*
+	 * If the merge was clean or if it failed due to conflict, we write
+	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
+	 * However, if the merge did not even start, then we don't want to
+	 * write it at all.
+	 */
+	if (action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
+		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
+	if (action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
+		write_cherry_pick_head(commit, "REVERT_HEAD");
+
+	if (res) {
+		error(action == REPLAY_REVERT
+		      ? _("could not revert %s... %s")
+		      : _("could not apply %s... %s"),
+		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
+		      msg.subject);
+		print_advice(res == 1);
+		rerere(opts->allow_rerere_auto);
+	} else {
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
+	}
+
+	free_message(&msg);
+	free(defmsg);
+
+	return res;
+}
+
+static void prepare_revs(struct replay_opts *opts)
+{
+	if (opts->command != REPLAY_CMD_REVERT)
+		opts->revs->reverse ^= 1;
+
+	if (prepare_revision_walk(opts->revs))
+		die(_("revision walk setup failed"));
+
+	if (!opts->revs->commits)
+		die(_("empty commit set passed"));
+}
+
+static void read_and_refresh_cache(struct replay_opts *opts)
+{
+	static struct lock_file index_lock;
+	int index_fd = hold_locked_index(&index_lock, 0);
+	if (read_index_preload(&the_index, NULL) < 0)
+		die(_("git %s: failed to read the index"), command_name(opts));
+	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+	if (the_index.cache_changed) {
+		if (write_index(&the_index, index_fd) ||
+		    commit_locked_index(&index_lock))
+			die(_("git %s: failed to refresh the index"),
+				command_name(opts));
+	}
+	rollback_lock_file(&index_lock);
+}
+
+/*
+ * Append a (commit, action) to the end of the replay_insn_list.
+ *
+ * next starts by pointing to the variable that holds the head of an
+ * empty replay_insn_list, and is updated to point to the "next" field of
+ * the last item on the list as new (commit, action) pairs are appended.
+ *
+ * Usage example:
+ *
+ *     struct replay_insn_list *list;
+ *     struct replay_insn_list **next = &list;
+ *
+ *     next = replay_insn_list_append(c1, a1, next);
+ *     next = replay_insn_list_append(c2, a2, next);
+ *     assert(len(list) == 2);
+ *     return list;
+ */
+static struct replay_insn_list **replay_insn_list_append(struct commit *operand,
+							enum replay_action action,
+							struct replay_insn_list **next)
+{
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
+	*next = new;
+	new->next = NULL;
+	return &new->next;
+}
+
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
+{
+	struct replay_insn_list *cur;
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = action_name(cur->action);
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
+		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+			subject_len, subject);
+	}
+	return 0;
+}
+
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
+{
+	const char *todo_file = git_path(SEQ_TODO_FILE);
+	unsigned char commit_sha1[20];
+	char *end_of_object_name;
+	int saved, status;
+	size_t error_len;
+
+	if (!prefixcmp(bol, "pick ") || !prefixcmp(bol, "pick\t")) {
+		item->action = REPLAY_PICK;
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ") || !prefixcmp(bol, "revert\t")) {
+		item->action = REPLAY_REVERT;
+		bol += strlen("revert ");
+	} else {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Unrecognized action: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
+
+	/* Eat up extra spaces/ tabs before object name */
+	bol += strspn(bol, " \t");
+
+	end_of_object_name = bol + strcspn(bol, " \t\n");
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	status = get_sha1(bol, commit_sha1);
+	*end_of_object_name = saved;
+
+	if (status < 0) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Malformed object name: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Not a valid commit: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
+
+	item->next = NULL;
+	return 0;
+}
+
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
+{
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = { NULL, 0, NULL };
+	char *p = buf;
+	int i;
+
+	for (i = 1; *p; i++) {
+		char *eol = strchrnul(p, '\n');
+		if (parse_insn_line(p, eol, &item, i) < 0)
+			return -1;
+		next = replay_insn_list_append(item.operand, item.action, next);
+		p = *eol ? eol + 1 : eol;
+	}
+	if (!*todo_list)
+		return error(_("No commits parsed."));
+	return 0;
+}
+
+static void read_populate_todo(struct replay_insn_list **todo_list)
+{
+	const char *todo_file = git_path(SEQ_TODO_FILE);
+	struct strbuf buf = STRBUF_INIT;
+	int fd, res;
+
+	fd = open(todo_file, O_RDONLY);
+	if (fd < 0)
+		die_errno(_("Could not open %s"), todo_file);
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), todo_file);
+	}
+	close(fd);
+
+	res = parse_insn_buffer(buf.buf, todo_list);
+	strbuf_release(&buf);
+	if (res)
+		die(_("Unusable instruction sheet: %s"), todo_file);
+}
+
+static int populate_opts_cb(const char *key, const char *value, void *data)
+{
+	struct replay_opts *opts = data;
+	int error_flag = 1;
+
+	if (!value)
+		error_flag = 0;
+	else if (!strcmp(key, "options.no-commit"))
+		opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.edit"))
+		opts->edit = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.signoff"))
+		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.record-origin"))
+		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.allow-ff"))
+		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.mainline"))
+		opts->mainline = git_config_int(key, value);
+	else if (!strcmp(key, "options.strategy"))
+		git_config_string(&opts->strategy, key, value);
+	else if (!strcmp(key, "options.strategy-option")) {
+		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+		opts->xopts[opts->xopts_nr++] = xstrdup(value);
+	} else
+		return error(_("Invalid key: %s"), key);
+
+	if (!error_flag)
+		return error(_("Invalid value for %s: %s"), key, value);
+
+	return 0;
+}
+
+static void read_populate_opts(struct replay_opts **opts_ptr)
+{
+	const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+	if (!file_exists(opts_file))
+		return;
+	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+		die(_("Malformed options sheet: %s"), opts_file);
+}
+
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
+				struct replay_opts *opts)
+{
+	struct commit *commit;
+	struct replay_insn_list **next;
+	enum replay_action action;
+
+	prepare_revs(opts);
+
+	next = todo_list;
+	action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
+	while ((commit = get_revision(opts->revs)))
+		next = replay_insn_list_append(commit, action, next);
+}
+
+static int create_seq_dir(void)
+{
+	const char *seq_dir = git_path(SEQ_DIR);
+
+	if (file_exists(seq_dir)) {
+		error(_("a cherry-pick or revert is already in progress"));
+		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
+		return -1;
+	}
+	else if (mkdir(seq_dir, 0777) < 0)
+		die_errno(_("Could not create sequencer directory %s"), seq_dir);
+	return 0;
+}
+
+static enum replay_command read_cmd(void)
+{
+	const char *cmd_file = git_path(SEQ_CMD_FILE);
+	struct strbuf buf = STRBUF_INIT;
+	enum replay_command res;
+	int fd;
+
+	fd = open(cmd_file, O_RDONLY);
+	if (fd < 0)
+		die_errno(_("Could not open %s"), cmd_file);
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), cmd_file);
+	}
+	close(fd);
+
+	if (!strcmp(buf.buf, "revert\n"))
+		res = REPLAY_CMD_REVERT;
+	else if (!strcmp(buf.buf, "cherry-pick\n"))
+		res = REPLAY_CMD_CHERRY_PICK;
+	else {
+		strbuf_release(&buf);
+		die(_("Malformed command file: %s"), cmd_file);
+	}
+	strbuf_release(&buf);
+	return res;
+}
+
+static void save_cmd(struct replay_opts *opts)
+{
+	const char *cmd_file = git_path(SEQ_CMD_FILE);
+	static struct lock_file cmd_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&cmd_lock, cmd_file, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", command_name(opts));
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s"), cmd_file);
+	if (commit_lock_file(&cmd_lock) < 0)
+		die(_("Error wrapping up %s."), cmd_file);
+}
+
+static void save_head(const char *head)
+{
+	const char *head_file = git_path(SEQ_HEAD_FILE);
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s"), head_file);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s."), head_file);
+}
+
+static int reset_for_rollback(const unsigned char *sha1)
+{
+	const char *argv[4];	/* reset --merge <arg> + NULL */
+	argv[0] = "reset";
+	argv[1] = "--merge";
+	argv[2] = sha1_to_hex(sha1);
+	argv[3] = NULL;
+	return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int rollback_single_pick(void)
+{
+	unsigned char head_sha1[20];
+
+	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
+	    !file_exists(git_path("REVERT_HEAD")))
+		return error(_("no cherry-pick or revert in progress"));
+	if (read_ref_full("HEAD", head_sha1, 0, NULL))
+		return error(_("cannot resolve HEAD"));
+	if (is_null_sha1(head_sha1))
+		return error(_("cannot abort from a branch yet to be born"));
+	return reset_for_rollback(head_sha1);
+}
+
+static int sequencer_rollback(struct replay_opts *opts)
+{
+	const char *filename;
+	FILE *f;
+	unsigned char sha1[20];
+	struct strbuf buf = STRBUF_INIT;
+
+	filename = git_path(SEQ_HEAD_FILE);
+	f = fopen(filename, "r");
+	if (!f && errno == ENOENT) {
+		/*
+		 * There is no multiple-cherry-pick in progress.
+		 * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
+		 * a single-cherry-pick in progress, abort that.
+		 */
+		return rollback_single_pick();
+	}
+	if (!f)
+		return error(_("cannot open %s: %s"), filename,
+						strerror(errno));
+	if (strbuf_getline(&buf, f, '\n')) {
+		error(_("cannot read %s: %s"), filename, ferror(f) ?
+			strerror(errno) : _("unexpected end of file"));
+		fclose(f);
+		goto fail;
+	}
+	fclose(f);
+	if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
+		error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
+			filename);
+		goto fail;
+	}
+	if (reset_for_rollback(sha1))
+		goto fail;
+	remove_sequencer_state();
+	strbuf_release(&buf);
+	return 0;
+fail:
+	strbuf_release(&buf);
+	return -1;
+}
+
+static void save_todo(struct replay_insn_list *todo_list)
+{
+	const char *todo_file = git_path(SEQ_TODO_FILE);
+	static struct lock_file todo_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+	if (format_todo(&buf, todo_list) < 0)
+		die(_("Could not format %s."), todo_file);
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s"), todo_file);
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s."), todo_file);
+	}
+	strbuf_release(&buf);
+}
+
+static void save_opts(struct replay_opts *opts)
+{
+	const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+	if (opts->no_commit)
+		git_config_set_in_file(opts_file, "options.no-commit", "true");
+	if (opts->edit)
+		git_config_set_in_file(opts_file, "options.edit", "true");
+	if (opts->signoff)
+		git_config_set_in_file(opts_file, "options.signoff", "true");
+	if (opts->record_origin)
+		git_config_set_in_file(opts_file, "options.record-origin", "true");
+	if (opts->allow_ff)
+		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+	if (opts->mainline) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "%d", opts->mainline);
+		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		strbuf_release(&buf);
+	}
+	if (opts->strategy)
+		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+	if (opts->xopts) {
+		int i;
+		for (i = 0; i < opts->xopts_nr; i++)
+			git_config_set_multivar_in_file(opts_file,
+							"options.strategy-option",
+							opts->xopts[i], "^$", 0);
+	}
+}
+
+static int pick_commits(struct replay_insn_list *todo_list, struct replay_opts *opts)
+{
+	struct replay_insn_list *cur;
+	int res;
+
+	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
+	read_and_refresh_cache(opts);
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
+		if (res)
+			return res;
+	}
+
+	/*
+	 * Sequence of picks finished successfully; cleanup by
+	 * removing the .git/sequencer directory
+	 */
+	remove_sequencer_state();
+	return 0;
+}
+
+static int continue_single_pick(void)
+{
+	const char *argv[] = { "commit", NULL };
+
+	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
+	    !file_exists(git_path("REVERT_HEAD")))
+		return error(_("no cherry-pick or revert in progress"));
+	return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int sequencer_continue(struct replay_opts *opts)
+{
+	struct replay_insn_list *todo_list = NULL;
+	enum replay_command cmd;
+
+	if (!file_exists(git_path(SEQ_TODO_FILE)))
+		return continue_single_pick();
+
+	/*
+	 * Disallow continuing a cherry-pick with 'git revert
+	 * --continue' and viceversa
+	 */
+	cmd = read_cmd();
+	if (cmd != opts->command)
+		return error(_("cannot %s: a %s is in progress."),
+			command_name(opts),
+			cmd == REPLAY_CMD_REVERT ? "revert" : "cherry-pick");
+
+	read_populate_opts(&opts);
+	read_populate_todo(&todo_list);
+
+	/* Verify that the conflict has been resolved */
+	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
+	    file_exists(git_path("REVERT_HEAD"))) {
+		int ret = continue_single_pick();
+		if (ret)
+			return ret;
+	}
+	if (index_differs_from("HEAD", 0))
+		return error_dirty_index(opts);
+	todo_list = todo_list->next;
+	return pick_commits(todo_list, opts);
+}
+
+static int single_pick(struct commit *cmit, struct replay_opts *opts)
+{
+	enum replay_action action;
+	action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
+
+	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
+	return do_pick_commit(cmit, action, opts);
+}
+
+int sequencer_pick_revisions(struct replay_opts *opts)
+{
+	struct replay_insn_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
+	read_and_refresh_cache(opts);
+
+	/*
+	 * Decide what to do depending on the arguments; a fresh
+	 * cherry-pick should be handled differently from an existing
+	 * one that is being continued
+	 */
+	if (opts->subcommand == REPLAY_REMOVE_STATE) {
+		remove_sequencer_state();
+		return 0;
+	}
+	if (opts->subcommand == REPLAY_ROLLBACK)
+		return sequencer_rollback(opts);
+	if (opts->subcommand == REPLAY_CONTINUE)
+		return sequencer_continue(opts);
+
+	/*
+	 * If we were called as "git cherry-pick <commit>", just
+	 * cherry-pick/revert it, set CHERRY_PICK_HEAD /
+	 * REVERT_HEAD, and don't touch the sequencer state.
+	 * This means it is possible to cherry-pick in the middle
+	 * of a cherry-pick sequence.
+	 */
+	if (opts->revs->cmdline.nr == 1 &&
+	    opts->revs->cmdline.rev->whence == REV_CMD_REV &&
+	    opts->revs->no_walk &&
+	    !opts->revs->cmdline.rev->flags) {
+		struct commit *cmit;
+		if (prepare_revision_walk(opts->revs))
+			die(_("revision walk setup failed"));
+		cmit = get_revision(opts->revs);
+		if (!cmit || get_revision(opts->revs))
+			die("BUG: expected exactly one commit from walk");
+		return single_pick(cmit, opts);
+	}
+
+	/*
+	 * Start a new cherry-pick/ revert sequence; but
+	 * first, make sure that an existing one isn't in
+	 * progress
+	 */
+
+	walk_revs_populate_todo(&todo_list, opts);
+	if (create_seq_dir() < 0)
+		return -1;
+	if (get_sha1("HEAD", sha1)) {
+		if (opts->command == REPLAY_CMD_REVERT)
+			return error(_("Can't revert as initial commit"));
+		return error(_("Can't cherry-pick into empty head"));
+	}
+	save_cmd(opts);
+	save_head(sha1_to_hex(sha1));
+	save_opts(opts);
+	return pick_commits(todo_list, opts);
+}
diff --git a/sequencer.h b/sequencer.h
index d1cb5c2..58482f1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -24,6 +24,29 @@ enum replay_subcommand {
 	REPLAY_ROLLBACK
 };
 
+struct replay_opts {
+	enum replay_command command;
+	enum replay_subcommand subcommand;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
+};
+
 struct replay_insn_list {
 	struct commit *operand;
 	enum replay_action action;
@@ -33,4 +56,6 @@ struct replay_insn_list {
 /* Removes SEQ_DIR. */
 extern void remove_sequencer_state(void);
 
+int sequencer_pick_revisions(struct replay_opts *opts);
+
 #endif
-- 
1.7.8.2

^ permalink raw reply related

* [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

Three kinds of errors can arise from parsing '.git/sequencer/todo':
1. Unrecognized action
2. Malformed object name
3. Object name does not refer to a valid commit

Since we would like to make the instruction sheet user-editable in the
future (much like the 'rebase -i' sheet), report more fine-grained
parse errors prefixed with the filename and line number.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   45 +++++++++++++++++++++++++++------------------
 1 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 54ea394..35553e7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 	return 0;
 }
 
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
 {
+	const char *todo_file = git_path(SEQ_TODO_FILE);
 	unsigned char commit_sha1[20];
 	char *end_of_object_name;
-	int saved, status, padding;
+	int saved, status;
+	size_t error_len;
 
-	if (!prefixcmp(bol, "pick")) {
+	if (!prefixcmp(bol, "pick ") || !prefixcmp(bol, "pick\t")) {
 		item->action = REPLAY_PICK;
-		bol += strlen("pick");
-	} else if (!prefixcmp(bol, "revert")) {
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ") || !prefixcmp(bol, "revert\t")) {
 		item->action = REPLAY_REVERT;
-		bol += strlen("revert");
-	} else
-		return -1;
+		bol += strlen("revert ");
+	} else {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Unrecognized action: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 
 	/* Eat up extra spaces/ tabs before object name */
-	padding = strspn(bol, " \t");
-	if (!padding)
-		return -1;
-	bol += padding;
+	bol += strspn(bol, " \t");
 
 	end_of_object_name = bol + strcspn(bol, " \t\n");
 	saved = *end_of_object_name;
@@ -741,12 +744,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;
 
-	if (status < 0)
-		return -1;
+	if (status < 0) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Malformed object name: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 
 	item->operand = lookup_commit_reference(commit_sha1);
-	if (!item->operand)
-		return -1;
+	if (!item->operand) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Not a valid commit: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 
 	item->next = NULL;
 	return 0;
@@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item) < 0)
-			return error(_("Could not parse line %d."), i);
+		if (parse_insn_line(p, eol, &item, i) < 0)
+			return -1;
 		next = replay_insn_list_append(item.operand, item.action, next);
 		p = *eol ? eol + 1 : eol;
 	}
-- 
1.7.8.2

^ permalink raw reply related

* [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all lines have the
same action.  Now, an instruction sheet like the following is
perfectly valid:

  pick fdc0b12 picked
  revert 965fed4 anotherpick

The operator can use this feature by hand-editing the instruction
sheet and using '--continue' as appropriate:

  $ git cherry-pick foo..bar
  [conflict occurs]
  $ edit problematicfile
  $ git add problematicfile
  $ edit .git/sequencer/todo
  $ git cherry-pick --continue
  [finishes successfully]

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |  135 +++++++++++++++++++--------------------
 sequencer.h                     |    6 ++
 t/t3510-cherry-pick-sequence.sh |   46 ++++++++++----
 3 files changed, 105 insertions(+), 82 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 52fa115..54ea394 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -475,7 +475,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -550,7 +551,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->command == REPLAY_CMD_REVERT) {
+	if (action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -591,7 +592,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->command == REPLAY_CMD_REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -615,13 +616,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->command == REPLAY_CMD_CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+	if (action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
 		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (opts->command == REPLAY_CMD_REVERT && ((opts->no_commit && res == 0) || res == 1))
+	if (action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->command == REPLAY_CMD_REVERT
+		error(action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -668,70 +669,70 @@ static void read_and_refresh_cache(struct replay_opts *opts)
 }
 
 /*
- * Append a commit to the end of the commit_list.
+ * Append a (commit, action) to the end of the replay_insn_list.
  *
  * next starts by pointing to the variable that holds the head of an
- * empty commit_list, and is updated to point to the "next" field of
- * the last item on the list as new commits are appended.
+ * empty replay_insn_list, and is updated to point to the "next" field of
+ * the last item on the list as new (commit, action) pairs are appended.
  *
  * Usage example:
  *
- *     struct commit_list *list;
- *     struct commit_list **next = &list;
+ *     struct replay_insn_list *list;
+ *     struct replay_insn_list **next = &list;
  *
- *     next = commit_list_append(c1, next);
- *     next = commit_list_append(c2, next);
- *     assert(commit_list_count(list) == 2);
+ *     next = replay_insn_list_append(c1, a1, next);
+ *     next = replay_insn_list_append(c2, a2, next);
+ *     assert(len(list) == 2);
  *     return list;
  */
-static struct commit_list **commit_list_append(struct commit *commit,
-					       struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(struct commit *operand,
+							enum replay_action action,
+							struct replay_insn_list **next)
 {
-	struct commit_list *new = xmalloc(sizeof(struct commit_list));
-	new->item = commit;
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
 	*next = new;
 	new->next = NULL;
 	return &new->next;
 }
 
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
-		struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 {
-	struct commit_list *cur = NULL;
-	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->command == REPLAY_CMD_REVERT ? "revert" : "pick";
-	const char *subject;
-	int subject_len;
+	struct replay_insn_list *cur;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = action_name(cur->action);
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
 		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
 			subject_len, subject);
 	}
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 {
 	unsigned char commit_sha1[20];
-	enum replay_action action;
 	char *end_of_object_name;
 	int saved, status, padding;
 
 	if (!prefixcmp(bol, "pick")) {
-		action = REPLAY_PICK;
+		item->action = REPLAY_PICK;
 		bol += strlen("pick");
 	} else if (!prefixcmp(bol, "revert")) {
-		action = REPLAY_REVERT;
+		item->action = REPLAY_REVERT;
 		bol += strlen("revert");
 	} else
-		return NULL;
+		return -1;
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
 	if (!padding)
-		return NULL;
+		return -1;
 	bol += padding;
 
 	end_of_object_name = bol + strcspn(bol, " \t\n");
@@ -740,37 +741,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if ((action == REPLAY_PICK && opts->command == REPLAY_CMD_REVERT) ||
-		(action == REPLAY_REVERT && opts->command == REPLAY_CMD_CHERRY_PICK)) {
-		error(_("Cannot %s during a %s"), action_name(action),
-			command_name(opts));
-		return NULL;
-	}
-
 	if (status < 0)
-		return NULL;
+		return -1;
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return -1;
 
-	return lookup_commit_reference(commit_sha1);
+	item->next = NULL;
+	return 0;
 }
 
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
-			struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 {
-	struct commit_list **next = todo_list;
-	struct commit *commit;
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = { NULL, 0, NULL };
 	char *p = buf;
 	int i;
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		commit = parse_insn_line(p, eol, opts);
-		if (!commit)
+		if (parse_insn_line(p, eol, &item) < 0)
 			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(item.operand, item.action, next);
 		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
@@ -778,8 +771,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
-			struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
@@ -795,7 +787,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	}
 	close(fd);
 
-	res = parse_insn_buffer(buf.buf, todo_list, opts);
+	res = parse_insn_buffer(buf.buf, todo_list);
 	strbuf_release(&buf);
 	if (res)
 		die(_("Unusable instruction sheet: %s"), todo_file);
@@ -844,17 +836,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct commit *commit;
-	struct commit_list **next;
+	struct replay_insn_list **next;
+	enum replay_action action;
 
 	prepare_revs(opts);
 
 	next = todo_list;
+	action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
 	while ((commit = get_revision(opts->revs)))
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(commit, action, next);
 }
 
 static int create_seq_dir(void)
@@ -996,7 +990,7 @@ fail:
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
@@ -1004,7 +998,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	int fd;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
+	if (format_todo(&buf, todo_list) < 0)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
@@ -1048,9 +1042,9 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list, struct replay_opts *opts)
 {
-	struct commit_list *cur;
+	struct replay_insn_list *cur;
 	int res;
 
 	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
@@ -1060,8 +1054,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
-		res = do_pick_commit(cur->item, opts);
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
 		if (res)
 			return res;
 	}
@@ -1086,7 +1080,7 @@ static int continue_single_pick(void)
 
 static int sequencer_continue(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	enum replay_command cmd;
 
 	if (!file_exists(git_path(SEQ_TODO_FILE)))
@@ -1103,7 +1097,7 @@ static int sequencer_continue(struct replay_opts *opts)
 			cmd == REPLAY_CMD_REVERT ? "revert" : "cherry-pick");
 
 	read_populate_opts(&opts);
-	read_populate_todo(&todo_list, opts);
+	read_populate_todo(&todo_list);
 
 	/* Verify that the conflict has been resolved */
 	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
@@ -1120,13 +1114,16 @@ static int sequencer_continue(struct replay_opts *opts)
 
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
+	enum replay_action action;
+	action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
+
 	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
-	return do_pick_commit(cmit, opts);
+	return do_pick_commit(cmit, action, opts);
 }
 
 static int pick_revisions(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
 	if (opts->subcommand == REPLAY_NONE)
diff --git a/sequencer.h b/sequencer.h
index 00ab685..d1cb5c2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -24,6 +24,12 @@ enum replay_subcommand {
 	REPLAY_ROLLBACK
 };
 
+struct replay_insn_list {
+	struct commit *operand;
+	enum replay_action action;
+	struct replay_insn_list *next;
+};
+
 /* Removes SEQ_DIR. */
 extern void remove_sequencer_state(void);
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 73298cf..f97ab79 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -456,7 +456,7 @@ test_expect_success 'sign-off needs to be reaffirmed after conflict resolution,
 	! grep Signed-off-by: msg
 '
 
-test_expect_success 'malformed instruction sheet 1' '
+test_expect_success 'malformed instruction sheet, action' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
@@ -467,23 +467,12 @@ test_expect_success 'malformed instruction sheet 1' '
 	test_expect_code 128 git cherry-pick --continue
 '
 
-test_expect_success 'malformed instruction sheet 2' '
-	pristine_detach initial &&
-	test_expect_code 1 git cherry-pick base..anotherpick &&
-	echo "resolved" >foo &&
-	git add foo &&
-	git commit &&
-	sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
-	cp new_sheet .git/sequencer/todo &&
-	test_expect_code 128 git cherry-pick --continue
-'
-
 test_expect_success 'empty commit set' '
 	pristine_detach initial &&
 	test_expect_code 128 git cherry-pick base..base
 '
 
-test_expect_success 'malformed instruction sheet 3' '
+test_expect_success 'malformed instruction sheet, object name' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
@@ -528,4 +517,35 @@ test_expect_success 'revert --continue refuses to follow cherry-pick' '
        test_expect_code 128 git revert --continue
 '
 
+test_expect_success 'mixed pick and revert instructions' '
+	pristine_detach initial &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	oldsha=`git rev-parse --short HEAD~1` &&
+	echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:000000 100644 OBJID OBJID A	foo
+	:000000 100644 OBJID OBJID A	unrelated
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.8.2

^ permalink raw reply related

* [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

When we allow mixing "revert" and "pick" instructions in the same
sheet in the next patch, the following workflow would be perfectly
valid:

  $ git cherry-pick base..latercommit
  [conflict occurs]
  $ edit problematicfile
  $ git add problematicfile
  $ git revert --continue
  [finishes successfully]

This is confusing to the operator, because the sequencer is an
implementation detail hidden behind the 'git cherry-pick' and 'git
revert' builtins.  So, disallow this workflow by keeping track of the
builtin command executed (either "revert" or "cherry-pick") in
'.git/sequencer/cmd'.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |   57 +++++++++++++++++++++++++++++++++++++++
 sequencer.h                     |    1 +
 t/t3510-cherry-pick-sequence.sh |   11 +++++++
 3 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3ac6da0..52fa115 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -871,6 +871,50 @@ static int create_seq_dir(void)
 	return 0;
 }
 
+static enum replay_command read_cmd(void)
+{
+	const char *cmd_file = git_path(SEQ_CMD_FILE);
+	struct strbuf buf = STRBUF_INIT;
+	enum replay_command res;
+	int fd;
+
+	fd = open(cmd_file, O_RDONLY);
+	if (fd < 0)
+		die_errno(_("Could not open %s"), cmd_file);
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), cmd_file);
+	}
+	close(fd);
+
+	if (!strcmp(buf.buf, "revert\n"))
+		res = REPLAY_CMD_REVERT;
+	else if (!strcmp(buf.buf, "cherry-pick\n"))
+		res = REPLAY_CMD_CHERRY_PICK;
+	else {
+		strbuf_release(&buf);
+		die(_("Malformed command file: %s"), cmd_file);
+	}
+	strbuf_release(&buf);
+	return res;
+}
+
+static void save_cmd(struct replay_opts *opts)
+{
+	const char *cmd_file = git_path(SEQ_CMD_FILE);
+	static struct lock_file cmd_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&cmd_lock, cmd_file, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", command_name(opts));
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s"), cmd_file);
+	if (commit_lock_file(&cmd_lock) < 0)
+		die(_("Error wrapping up %s."), cmd_file);
+}
+
 static void save_head(const char *head)
 {
 	const char *head_file = git_path(SEQ_HEAD_FILE);
@@ -1043,9 +1087,21 @@ static int continue_single_pick(void)
 static int sequencer_continue(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
+	enum replay_command cmd;
 
 	if (!file_exists(git_path(SEQ_TODO_FILE)))
 		return continue_single_pick();
+
+	/*
+	 * Disallow continuing a cherry-pick with 'git revert
+	 * --continue' and viceversa
+	 */
+	cmd = read_cmd();
+	if (cmd != opts->command)
+		return error(_("cannot %s: a %s is in progress."),
+			command_name(opts),
+			cmd == REPLAY_CMD_REVERT ? "revert" : "cherry-pick");
+
 	read_populate_opts(&opts);
 	read_populate_todo(&todo_list, opts);
 
@@ -1126,6 +1182,7 @@ static int pick_revisions(struct replay_opts *opts)
 			return error(_("Can't revert as initial commit"));
 		return error(_("Can't cherry-pick into empty head"));
 	}
+	save_cmd(opts);
 	save_head(sha1_to_hex(sha1));
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
diff --git a/sequencer.h b/sequencer.h
index 07e0639..00ab685 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -2,6 +2,7 @@
 #define SEQUENCER_H
 
 #define SEQ_DIR		"sequencer"
+#define SEQ_CMD_FILE	"sequencer/cmd"
 #define SEQ_HEAD_FILE	"sequencer/head"
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 97f3710..73298cf 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -48,6 +48,7 @@ test_expect_success 'cherry-pick persists data on failure' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick -s base..anotherpick &&
 	test_path_is_dir .git/sequencer &&
+	test_path_is_file .git/sequencer/cmd &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
 	test_path_is_file .git/sequencer/opts
@@ -69,6 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
 	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
+	test_path_is_file .git/sequencer/cmd &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
 	test_path_is_file .git/sequencer/opts &&
@@ -517,4 +519,13 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+test_expect_success 'revert --continue refuses to follow cherry-pick' '
+       pristine_detach initial &&
+       test_expect_code 1 git cherry-pick base..anotherpick &&
+       echo "c" >foo &&
+       git add foo &&
+       git commit &&
+       test_expect_code 128 git revert --continue
+'
+
 test_done
-- 
1.7.8.2

^ permalink raw reply related

* [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

Currently, 'git cherry-pick' fills up the '.git/sequencer/todo'
instruction sheet with "pick" actions, while 'git revert' fills it up
with "revert" actions.  Inspired by the way 'rebase -i' works, we
would like to permit mixing arbitrary actions in the same instruction
sheet.  To do this, we first have to decouple the notion of an action
in the instruction sheet from builtin commands.  So, while a future
instruction sheet would look like:

  pick next~4
  action3 b74fea
  revert rr/moo^2~34

the actions "pick", "action3" and "revert" need not necessarily
correspond to the specific builtins.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   62 +++++++++++++++++++++++++++++------------------------
 sequencer.h      |    5 ++++
 2 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index c804045..3ac6da0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -40,7 +40,7 @@ static const char * const cherry_pick_usage[] = {
 };
 
 struct replay_opts {
-	enum replay_action action;
+	enum replay_command command;
 	enum replay_subcommand subcommand;
 
 	/* Boolean options */
@@ -64,16 +64,21 @@ struct replay_opts {
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-static const char *action_name(const struct replay_opts *opts)
+static const char *command_name(struct replay_opts *opts)
+{
+	return opts->command == REPLAY_CMD_REVERT ? "revert" : "cherry-pick";
+}
+
+static const char *action_name(enum replay_action action)
 {
-	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+	return action == REPLAY_REVERT ? "revert" : "pick";
 }
 
 static char *get_encoding(const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
+	return opts->command == REPLAY_CMD_REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -130,7 +135,7 @@ static void verify_opt_mutually_compatible(const char *me, ...)
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
-	const char *me = action_name(opts);
+	const char *me = command_name(opts);
 	int remove_state = 0;
 	int contin = 0;
 	int rollback = 0;
@@ -152,7 +157,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 	};
 
-	if (opts->action == REPLAY_PICK) {
+	if (opts->command == REPLAY_CMD_CHERRY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -363,10 +368,10 @@ static struct tree *empty_tree(void)
 static int error_dirty_index(struct replay_opts *opts)
 {
 	if (read_cache_unmerged())
-		return error_resolve_conflict(action_name(opts));
+		return error_resolve_conflict(command_name(opts));
 
 	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == REPLAY_PICK)
+	if (opts->command == REPLAY_CMD_CHERRY_PICK)
 		error(_("Your local changes would be overwritten by cherry-pick."));
 	else
 		error(_("Your local changes would be overwritten by revert."));
@@ -422,7 +427,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&index_lock)))
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), action_name(opts));
+		die(_("%s: Unable to write new index file"), command_name(opts));
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -530,7 +535,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		return error(_("%s: cannot parse parent commit %s"),
-			action_name(opts), sha1_to_hex(parent->object.sha1));
+			command_name(opts), sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
@@ -545,7 +550,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REPLAY_REVERT) {
+	if (opts->command == REPLAY_CMD_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -586,7 +591,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->command == REPLAY_CMD_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -610,13 +615,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
+	if (opts->command == REPLAY_CMD_CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
 		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
+	if (opts->command == REPLAY_CMD_REVERT && ((opts->no_commit && res == 0) || res == 1))
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->action == REPLAY_REVERT
+		error(opts->command == REPLAY_CMD_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -636,7 +641,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 static void prepare_revs(struct replay_opts *opts)
 {
-	if (opts->action != REPLAY_REVERT)
+	if (opts->command != REPLAY_CMD_REVERT)
 		opts->revs->reverse ^= 1;
 
 	if (prepare_revision_walk(opts->revs))
@@ -651,12 +656,13 @@ static void read_and_refresh_cache(struct replay_opts *opts)
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
 	if (read_index_preload(&the_index, NULL) < 0)
-		die(_("git %s: failed to read the index"), action_name(opts));
+		die(_("git %s: failed to read the index"), command_name(opts));
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
 	if (the_index.cache_changed) {
 		if (write_index(&the_index, index_fd) ||
 		    commit_locked_index(&index_lock))
-			die(_("git %s: failed to refresh the index"), action_name(opts));
+			die(_("git %s: failed to refresh the index"),
+				command_name(opts));
 	}
 	rollback_lock_file(&index_lock);
 }
@@ -693,7 +699,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 {
 	struct commit_list *cur = NULL;
 	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
+	const char *action_str = opts->command == REPLAY_CMD_REVERT ? "revert" : "pick";
 	const char *subject;
 	int subject_len;
 
@@ -738,10 +744,10 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	 * Verify that the action matches up with the one in
 	 * opts; we don't support arbitrary instructions
 	 */
-	if (action != opts->action) {
-		const char *action_str;
-		action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
-		error(_("Cannot %s during a %s"), action_str, action_name(opts));
+	if ((action == REPLAY_PICK && opts->command == REPLAY_CMD_REVERT) ||
+		(action == REPLAY_REVERT && opts->command == REPLAY_CMD_CHERRY_PICK)) {
+		error(_("Cannot %s during a %s"), action_name(action),
+			command_name(opts));
 		return NULL;
 	}
 
@@ -1003,7 +1009,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	struct commit_list *cur;
 	int res;
 
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
 	if (opts->allow_ff)
 		assert(!(opts->signoff || opts->no_commit ||
 				opts->record_origin || opts->edit));
@@ -1058,7 +1064,7 @@ static int sequencer_continue(struct replay_opts *opts)
 
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
 	return do_pick_commit(cmit, opts);
 }
 
@@ -1116,7 +1122,7 @@ static int pick_revisions(struct replay_opts *opts)
 	if (create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1)) {
-		if (opts->action == REPLAY_REVERT)
+		if (opts->command == REPLAY_CMD_REVERT)
 			return error(_("Can't revert as initial commit"));
 		return error(_("Can't cherry-pick into empty head"));
 	}
@@ -1133,7 +1139,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
 		opts.edit = 1;
-	opts.action = REPLAY_REVERT;
+	opts.command = REPLAY_CMD_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
@@ -1148,7 +1154,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	int res;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.action = REPLAY_PICK;
+	opts.command = REPLAY_CMD_CHERRY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index 1d9fcec..07e0639 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -11,6 +11,11 @@ enum replay_action {
 	REPLAY_PICK
 };
 
+enum replay_command {
+	REPLAY_CMD_REVERT,
+	REPLAY_CMD_CHERRY_PICK
+};
+
 enum replay_subcommand {
 	REPLAY_NONE,
 	REPLAY_REMOVE_STATE,
-- 
1.7.8.2

^ permalink raw reply related

* [PATCH 1/6] revert: move replay_action, replay_subcommand to header
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>

Our plan to build a sequencer involves leaving the revert builtin with
just argument parsing work.  Since the enums replay_action and
replay_subcommand have nothing to do with this argument parsing, move
them to sequencer.h in advance.

"REVERT" and "CHERRY_PICK" are unsuitable variable names for exposing
publicly, as their purpose is unclear in the global context: rename
them to "REPLAY_REVERT" and "REPLAY_PICK" respectively.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   42 +++++++++++++++++-------------------------
 sequencer.h      |   12 ++++++++++++
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0d8020c..c804045 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,14 +39,6 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand {
-	REPLAY_NONE,
-	REPLAY_REMOVE_STATE,
-	REPLAY_CONTINUE,
-	REPLAY_ROLLBACK
-};
-
 struct replay_opts {
 	enum replay_action action;
 	enum replay_subcommand subcommand;
@@ -74,14 +66,14 @@ struct replay_opts {
 
 static const char *action_name(const struct replay_opts *opts)
 {
-	return opts->action == REVERT ? "revert" : "cherry-pick";
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
 static char *get_encoding(const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -160,7 +152,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 	};
 
-	if (opts->action == CHERRY_PICK) {
+	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -374,7 +366,7 @@ static int error_dirty_index(struct replay_opts *opts)
 		return error_resolve_conflict(action_name(opts));
 
 	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == CHERRY_PICK)
+	if (opts->action == REPLAY_PICK)
 		error(_("Your local changes would be overwritten by cherry-pick."));
 	else
 		error(_("Your local changes would be overwritten by revert."));
@@ -553,7 +545,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REVERT) {
+	if (opts->action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -594,7 +586,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -618,13 +610,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
 		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->action == REVERT
+		error(opts->action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -644,7 +636,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 static void prepare_revs(struct replay_opts *opts)
 {
-	if (opts->action != REVERT)
+	if (opts->action != REPLAY_REVERT)
 		opts->revs->reverse ^= 1;
 
 	if (prepare_revision_walk(opts->revs))
@@ -701,7 +693,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 {
 	struct commit_list *cur = NULL;
 	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REVERT ? "revert" : "pick";
+	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
 	const char *subject;
 	int subject_len;
 
@@ -722,10 +714,10 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	int saved, status, padding;
 
 	if (!prefixcmp(bol, "pick")) {
-		action = CHERRY_PICK;
+		action = REPLAY_PICK;
 		bol += strlen("pick");
 	} else if (!prefixcmp(bol, "revert")) {
-		action = REVERT;
+		action = REPLAY_REVERT;
 		bol += strlen("revert");
 	} else
 		return NULL;
@@ -748,7 +740,7 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	 */
 	if (action != opts->action) {
 		const char *action_str;
-		action_str = action == REVERT ? "revert" : "cherry-pick";
+		action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
 		error(_("Cannot %s during a %s"), action_str, action_name(opts));
 		return NULL;
 	}
@@ -1124,7 +1116,7 @@ static int pick_revisions(struct replay_opts *opts)
 	if (create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1)) {
-		if (opts->action == REVERT)
+		if (opts->action == REPLAY_REVERT)
 			return error(_("Can't revert as initial commit"));
 		return error(_("Can't cherry-pick into empty head"));
 	}
@@ -1141,7 +1133,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
 		opts.edit = 1;
-	opts.action = REVERT;
+	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
@@ -1156,7 +1148,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	int res;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.action = CHERRY_PICK;
+	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index 2d4528f..1d9fcec 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -6,6 +6,18 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+enum replay_action {
+	REPLAY_REVERT,
+	REPLAY_PICK
+};
+
+enum replay_subcommand {
+	REPLAY_NONE,
+	REPLAY_REMOVE_STATE,
+	REPLAY_CONTINUE,
+	REPLAY_ROLLBACK
+};
+
 /* Removes SEQ_DIR. */
 extern void remove_sequencer_state(void);
 
-- 
1.7.8.2

^ permalink raw reply related

* [PATCH 0/6] The move to sequencer.c
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Jonathan Nieder

Hi,

I've tried a slightly different approach: the objective of the patches
seem to be much clearer this time.

[1/6] revert: move replay_action, replay_subcommand to header
[2/6] revert: decouple sequencer actions from builtin commands
[3/6] revert: don't let revert continue a cherry-pick
[4/6] revert: allow mixing "pick" and "revert" actions
[5/6] revert: report fine-grained error messages from insn parser
[6/6] sequencer: factor code out of revert builtin


[1/6] first moves out a couple of data structures to the header

[2/6] decouples "actions" from a "commands" completely.  Although this
sort of separation might not be necessary at this stage (because we
just have a couple of actions that directly correspond to builtin
commands), I think it makes [4/6] much easier to read.

[3/6] mainly exists so that [4/6] doesn't allow 'git revert
--continue' to continue a 'git cherry-pick' and viceversa.  Note that
a 'git revert --continue' can execute an instruction sheet with "pick"
instructions and viceversa after [4/6].

[4/6] should be very clear this time: do_pick_commit() takes an extra
argument "action", and checks that instead of the "opts->command"
everywhere.  The parser is also updated to parse into (commit, action)
pairs.

[5/6] is fairly straightforward.

[6/6] makes the final move.  This is something I've been pushing for
quite some time: exciting things like 'git continue' will follow this.

Cheers!

 builtin/revert.c                |  959 +-------------------------------------
 sequencer.c                     |  987 ++++++++++++++++++++++++++++++++++++++-
 sequencer.h                     |   49 ++
 t/t3510-cherry-pick-sequence.sh |   57 ++-
 4 files changed, 1088 insertions(+), 964 deletions(-)

-- 
1.7.8.2

^ permalink raw reply

* Re: SVN -> Git *but* with special changes
From: Adam Borowski @ 2012-01-08 12:08 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Carlos Martín Nieto, Abscissa, git
In-Reply-To: <m2hb06mpwn.fsf@linux-m68k.org>

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Sun, Jan 08, 2012 at 11:47:52AM +0100, Andreas Schwab wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> > On Sat, Jan 07, 2012 at 09:25:27PM -0800, Abscissa wrote:
> >> Well that's strange, it finished "upgrading", but now git is still just
> >> reporting 1.7.0.4, which is *exactly* the same version it said before. The
> >> git-svn package should already be up-to-date because I just installed it 
> >> today. So I don't know what's up with that.
> >
> > Nothing odd about that. apt-get upgrade means "upgrade my system". If
> > you want to get a newer version of package X, you do apt-get install X
> > and it will install the latest version of that package.
> 
> If apt-get upgrade doesn't get you a newer version then apt-get install
> won't help you either.

No, this is true only if none of packages involved uses a new library
(including new sonames).  "apt-get upgrade" is forbidden to add or remove
packages, and thus will skip upgrades that need a new dependency.
The message at the end will mention "## not upgraded" though.

"apt-get install X" or "apt-get dist-upgrade" have no such restrictions.

-- 
1KB		// Yo momma uses IPv4!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox