[GSoC update] Sequencer for inclusion

[GSoC update] Sequencer for inclusion

[Ramkumar Ramachandra]

Hi,

I'm excited to announce the first iteration of a fresh series -- this
one is intended for inclusion.  There are five new patches since last
time, the most significant of which is "revert: Save command-line
options for continuing operation", which introduces a parser for
command-line options.  Some patches might be slightly controversial,
but I think most of the new ideas have already been discussed
previously.  Another significant introduction is "sequencer: Announce
sequencer state location" -- this is where most of the post mid-term
work will go.

I would have liked to reuse the gitconfig parser as-is for the opts
parsing, but it's too tangled up in config.c.  I think it's safe to
say that the opts file format deviates only slightly from the
gitconfig format, and I'm quite happy with the end result.

What this requires now:
1. Review all existing code.  Many things might have moved around due
to heavy rebasing.
2. Repetition of unimplemented suggestions.  I'm sorry if I've missed
out some -- I was focusing on new features this time, not fixing
existing problems.
3. Rigorous testing of the new options parser.  We don't want
segfaults because of a silly parser.
4. New tests and documentation.  There's really no end to this, so
please suggest only things that are absolutely necessary before
inclusion.

In general, please point out things that need to be done immediately
before the inclusion.  The series is becoming large and unmanagable --
we can fix minor issues after the merge.

Thanks for reading.

-- Ram

Ramkumar Ramachandra (17):
  advice: Introduce error_resolve_conflict
  revert: Inline add_message_to_msg function
  revert: Don't check lone argument in get_encoding
  revert: Rename no_replay to record_origin
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Don't create invalid replay_opts in parse_args
  sequencer: Announce sequencer state location
  revert: Save data for continuing after conflict resolution
  revert: Save command-line options for continuing operation
  revert: Introduce a layer of indirection over pick_commits
  reset: Make hard reset remove the sequencer state
  revert: Remove sequencer state when no commits are pending
  revert: Introduce --reset to remove sequencer state
  revert: Introduce --continue to continue the operation

 Documentation/git-cherry-pick.txt |    6 +
 Documentation/git-revert.txt      |    6 +
 Documentation/sequencer.txt       |    9 +
 Makefile                          |    2 +
 advice.c                          |   31 +-
 advice.h                          |    3 +-
 builtin/reset.c                   |    2 +
 builtin/revert.c                  |  863 ++++++++++++++++++++++++++++++-------
 git-rebase--interactive.sh        |   25 +-
 sequencer.c                       |   19 +
 sequencer.h                       |   19 +
 t/7106-reset-sequence.sh          |   40 ++
 t/t3510-cherry-pick-sequence.sh   |  197 +++++++++
 13 files changed, 1058 insertions(+), 164 deletions(-)
 create mode 100644 Documentation/sequencer.txt
 create mode 100644 sequencer.c
 create mode 100644 sequencer.h
 create mode 100755 t/7106-reset-sequence.sh
 create mode 100755 t/t3510-cherry-pick-sequence.sh

-- 
1.7.5.GIT

[PATCH 01/17] advice: Introduce error_resolve_conflict

[Ramkumar Ramachandra]

Enable future callers to report a conflict and not die immediately by
introducing a new function called error_resolve_conflict.
Re-implement die_resolve_conflict as a call to error_resolve_conflict
followed by a call to die.  Consequently, the message printed by
die_resolve_conflict changes from

fatal: 'commit' is not possible because you have unmerged files.
       Please, fix them up in the work tree ...
       ...

to

error: 'commit' is not possible because you have unmerged files.
hint: Please, fix them up in the work tree ...
hint: ...
fatal: Exiting because of an unresolved conflict.

Hints are printed using the same advise function introduced in
v1.7.3-rc0~26^2~3 (Introduce advise() to print hints, 2010-08-11).

Inspired-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c         |   31 ++++++++++++++++++++++++-------
 advice.h         |    3 ++-
 builtin/revert.c |    9 ---------
 3 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..a6b3fc2 100644
--- a/advice.c
+++ b/advice.c
@@ -19,6 +19,15 @@ static struct {
 	{ "detachedhead", &advice_detached_head },
 };
 
+void advise(const char *advice, ...)
+{
+	va_list params;
+
+	va_start(params, advice);
+	vreportf("hint: ", advice, params);
+	va_end(params);
+}
+
 int git_default_advice_config(const char *var, const char *value)
 {
 	const char *k = skip_prefix(var, "advice.");
@@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value)
 	return 0;
 }
 
-void NORETURN die_resolve_conflict(const char *me)
+int error_resolve_conflict(const char *me)
 {
-	if (advice_resolve_conflict)
+	error("'%s' is not possible because you have unmerged files.", me);
+	if (advice_resolve_conflict) {
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
 		 */
-		die("'%s' is not possible because you have unmerged files.\n"
-		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
-		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
-	else
-		die("'%s' is not possible because you have unmerged files.", me);
+		advise("Please, fix them up in the work tree,");
+		advise("and then use 'git add/rm <file>' as");
+		advise("appropriate to mark resolution and make a commit,");
+		advise("or use 'git commit -a'.");
+	}
+	return -1;
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	die("Exiting because of an unresolved conflict.");
 }
diff --git a/advice.h b/advice.h
index 3244ebb..e5d0af7 100644
--- a/advice.h
+++ b/advice.h
@@ -11,7 +11,8 @@ extern int advice_implicit_identity;
 extern int advice_detached_head;
 
 int git_default_advice_config(const char *var, const char *value);
-
+void advise(const char *advice, ...);
+int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
 
 #endif /* ADVICE_H */
diff --git a/builtin/revert.c b/builtin/revert.c
index 1f27c63..2df3f3b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -214,15 +214,6 @@ static void write_cherry_pick_head(void)
 	strbuf_release(&buf);
 }
 
-static void advise(const char *advice, ...)
-{
-	va_list params;
-
-	va_start(params, advice);
-	vreportf("hint: ", advice, params);
-	va_end(params);
-}
-
 static void print_advice(void)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
-- 
1.7.5.GIT

[PATCH 02/17] revert: Inline add_message_to_msg function

[Ramkumar Ramachandra]

The add_message_to_msg function is poorly implemented, has an unclear
API, and only one callsite.  Replace the callsite with a cleaner
implementation.  Additionally, fix a bug introduced in 9509af6 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
was being incremented when "\n\n" was not found in "message".

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2df3f3b..f88e007 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -185,19 +185,6 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
-{
-	const char *p = message;
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-
-	if (!*p)
-		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
-
-	p += 2;
-	strbuf_addstr(msgbuf, p);
-}
-
 static void write_cherry_pick_head(void)
 {
 	int fd;
@@ -462,11 +449,16 @@ static int do_pick_commit(void)
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {
+		const char *p = strstr(msg.message, "\n\n");
+
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+
+		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
+		strbuf_addstr(&msgbuf, p);
+
 		if (no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-- 
1.7.5.GIT

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> The add_message_to_msg function is poorly implemented, has an unclear
> API, and only one callsite.  Replace the callsite with a cleaner
> implementation.  Additionally, fix a bug introduced in 9509af6 (Make
> git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
> was being incremented when "\n\n" was not found in "message".

Rather than being an optimization, the main impact of this change is
to avoid a NULL pointer dereference in some cases, right?

If so, the subject line should say so.  Is it possible to reproduce
this?  Could we add a test to avoid regressing in the future?

Less importantly:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -462,11 +449,16 @@ static int do_pick_commit(void)
>  		}
>  		strbuf_addstr(&msgbuf, ".\n");
>  	} else {
> +		const char *p = strstr(msg.message, "\n\n");
> +
>  		base = parent;
>  		base_label = msg.parent_label;
>  		next = commit;
>  		next_label = msg.label;
> -		add_message_to_msg(&msgbuf, msg.message);
> +
> +		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
> +		strbuf_addstr(&msgbuf, p);

I think this would be clearer like so:

		const char *p;
		...
		p = strstr(...);
		if (p)
			p += 2;
		else
			p = sha1_to_hex...
		strbuf_addstr(&msgbuf, p);

i.e., putting all the code that manipulates p together.  Besides,
pre-C99 compilers don't like "p" to be initialized to a non-constant.
:)

At [1], I also see a suggestion of a comment that could clarify the
code a little.

Hope that helps,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/176139/focus=176183

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>
>> The add_message_to_msg function is poorly implemented, has an unclear
>> API, and only one callsite.  Replace the callsite with a cleaner
>> implementation.  Additionally, fix a bug introduced in 9509af6 (Make
>> git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
>> was being incremented when "\n\n" was not found in "message".
>
> Rather than being an optimization, the main impact of this change is
> to avoid a NULL pointer dereference in some cases, right?
>
> If so, the subject line should say so.  Is it possible to reproduce
> this?  Could we add a test to avoid regressing in the future?
>
> Less importantly:
>
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -462,11 +449,16 @@ static int do_pick_commit(void)
>>               }
>>               strbuf_addstr(&msgbuf, ".\n");
>>       } else {
>> +             const char *p = strstr(msg.message, "\n\n");
>> +
>>               base = parent;
>>               base_label = msg.parent_label;
>>               next = commit;
>>               next_label = msg.label;
>> -             add_message_to_msg(&msgbuf, msg.message);
>> +
>> +             p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
>> +             strbuf_addstr(&msgbuf, p);
>
> I think this would be clearer like so:
>
>                const char *p;
>                ...
>                p = strstr(...);
>                if (p)
>                        p += 2;
>                else
>                        p = sha1_to_hex...
>                strbuf_addstr(&msgbuf, p);
>
> i.e., putting all the code that manipulates p together.  Besides,
> pre-C99 compilers don't like "p" to be initialized to a non-constant.
> :)
>
> At [1], I also see a suggestion of a comment that could clarify the
> code a little.

Fixed all issues.  The commit message now reads

revert: Inline add_message_to_msg function

The add_message_to_msg function is poorly implemented, has an unclear
API, and only one callsite.  Replace the callsite with a cleaner
implementation.  Additionally, fix a bug introduced in 9509af6 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
was being dereferenced when "\n\n" was not found in "message".

Thanks.

-- Ram

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> Fixed all issues.  The commit message now reads
>
> revert: Inline add_message_to_msg function
>
> The add_message_to_msg function is poorly implemented, has an unclear
> API, and only one callsite.  Replace the callsite with a cleaner
> implementation.  Additionally, fix a bug introduced in 9509af6 (Make
> git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
> was being dereferenced when "\n\n" was not found in "message".

That's basically the same as before, with "dereferenced" in place of
"incremented".  An improvement, sure, but it still doesn't answer the
basic questions like "how can I reproduce the bug?".

So no, in the commit message at least I don't think all issues are
fixed.  The subject line says the idea is to inline this function, but
it's actually a behavior change.  The description of the behavior
change makes reference to a variable "message" without saying what the
content of that variable is --- e.g., will trying to cherry-pick a
commit with a one-line commit message trigger this bug?  (No, it
won't.)

And honestly, making that small change and calling it fixed just feels
half-hearted and rushed.  After changing it, did you look back over
the message, read it as though unknowledgeable about the patch, and
decide whether it conveyed the right impression, which is the
equivalent of testing when writing documentation?

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Fixed all issues.  The commit message now reads
>>
>> revert: Inline add_message_to_msg function
>>
>> The add_message_to_msg function is poorly implemented, has an unclear
>> API, and only one callsite.  Replace the callsite with a cleaner
>> implementation.  Additionally, fix a bug introduced in 9509af6 (Make
>> git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
>> was being dereferenced when "\n\n" was not found in "message".
>
> That's basically the same as before, with "dereferenced" in place of
> "incremented".  An improvement, sure, but it still doesn't answer the
> basic questions like "how can I reproduce the bug?".

Thanks for poking -- I figured out what's going on.  The msg.message
doesn't just contain the commit message; it contains information like
tree, parent etc (all the information shown by cat-file).  A typical
msg.message looks like:

>>
tree cb35cb5a8736245fd669729ce40d790062f1d74b
parent dcab267d57895e9e8710f5a954e8b9bdfa459c71
author Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530
committer Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530

This is the subject of the message

This is the description of the message
<<

The strstr(msg.message, "\n\n") actually stops after the 0 in the
committer line.  It's trying to find out where the commit message
actually begins.  I even created a commit using commit-tree (not using
porcelain), but it's impossible to find a msg.message that doesn't
have a "\n\n" -- even one with an empty commit message looks like:

>>
tree cb35cb5a8736245fd669729ce40d790062f1d74b
parent dcab267d57895e9e8710f5a954e8b9bdfa459c71
author Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530
committer Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530

<<

The earlier logic was flawed:

	while (*p && (*p != '\n' || p[1] != '\n'))
		p++;

	if (!*p)
		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));

Here, the !*p will never be satisfied even in the case of the empty
commit message because p reads "\n\n" at this point: the picked commit
will be filled up with an empty commit message as well.  This is also
shown by the tests in t3505-cherry-pick-emtpy.sh.  Perhaps this really
is the desired behavior: why should cherry-pick try to add something
to the commit message when we're picking a commit with an empty commit
message? Now I realize that the new logic is flawed too.  Since it's
totally impossible to reproduce this bug, it's not really a bug --
there's just some dead code in the older implementation, and we want
to remove it and make it clear.

New commit message and commit.

-- 8< --
Subject: [PATCH] revert: Simplify and inline add_message_to_msg

The add_message_to_msg function has some dead code, an unclear API,
only one callsite.  While it originally intended fill up an empty
commit message with the commit object name while picking, it really
doesn't do this -- a bug introduced in 9509af6 (Make git-revert &
git-cherry-pick a builtin, 2007-03-01).  Today, tests in
t3505-cherry-pick-empty.sh indicate that not filling up an empty
commit message is the desired behavior.  Re-implement and inline the
function accordingly.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 2df3f3b..2b5f261 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -185,19 +185,6 @@ static char *get_encoding(const char *message)
        return NULL;
 }

-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
-{
-       const char *p = message;
-       while (*p && (*p != '\n' || p[1] != '\n'))
-               p++;
-
-       if (!*p)
-               strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
-
-       p += 2;
-       strbuf_addstr(msgbuf, p);
-}
-
 static void write_cherry_pick_head(void)
 {
        int fd;
@@ -462,11 +449,22 @@ static int do_pick_commit(void)
                }
                strbuf_addstr(&msgbuf, ".\n");
        } else {
+               const char *p;
+
                base = parent;
                base_label = msg.parent_label;
                next = commit;
                next_label = msg.label;
-               add_message_to_msg(&msgbuf, msg.message);
+
+               /*
+                * 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");
+               p += 2;
+               strbuf_addstr(&msgbuf, p);
+
                if (no_replay) {
                        strbuf_addstr(&msgbuf, "(cherry picked from commit ");
                        strbuf_addstr(&msgbuf,
sha1_to_hex(commit->object.sha1));
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The strstr(msg.message, "\n\n") actually stops after the 0 in the
> committer line.  It's trying to find out where the commit message
> actually begins.  I even created a commit using commit-tree (not using
> porcelain), but it's impossible to find a msg.message that doesn't
> have a "\n\n" -- even one with an empty commit message looks like:
> ...

... with current git, sure, but I think the code to check for "just in
case we didn't find \n\n" is there because prehistoric versions of git
could write such a commit object.

Hence...

> @@ -462,11 +449,22 @@ static int do_pick_commit(void)
>                 }
>                 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");

I would prefer to have if (p) { ... } around the two lines below.

> +               p += 2;
> +               strbuf_addstr(&msgbuf, p);
> +
>                 if (no_replay) {
>                         strbuf_addstr(&msgbuf, "(cherry picked from commit ");
>                         strbuf_addstr(&msgbuf,
> sha1_to_hex(commit->object.sha1));

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> The strstr(msg.message, "\n\n") actually stops after the 0 in the
>> committer line.  It's trying to find out where the commit message
>> actually begins.  I even created a commit using commit-tree (not using
>> porcelain), but it's impossible to find a msg.message that doesn't
>> have a "\n\n" -- even one with an empty commit message looks like:
>> ...
>
> ... with current git, sure, but I think the code to check for "just in
> case we didn't find \n\n" is there because prehistoric versions of git
> could write such a commit object.

Ah, I see :)

> I would prefer to have if (p) { ... } around the two lines below.
>
>> +               p += 2;
>> +               strbuf_addstr(&msgbuf, p);

Done.  Thanks.

-- Ram

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Since it's
> totally impossible to reproduce this bug, it's not really a bug --

Huh?

	git cat-file commit HEAD | sed -e '/^$/ q' >newcommit &&
	cmit=$(git hash-object -t commit -w newcommit) &&
	git cherry-pick $cmit

Fun bonus:

	git log $cmit

Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Since it's
>> totally impossible to reproduce this bug, it's not really a bug --
>
> Huh?
>
>        git cat-file commit HEAD | sed -e '/^$/ q' >newcommit &&
>        cmit=$(git hash-object -t commit -w newcommit) &&
>        git cherry-pick $cmit

Oh.  I should've been clearer -- it's totally impossible to reproduce
this bug, except by hand-crafting a commit object.
Thanks for the interesting exercise.

-- Ram

[PATCH 03/17] revert: Don't check lone argument in get_encoding

[Ramkumar Ramachandra]

The get_encoding function has only one callsite, and its caller makes
sure that a NULL argument isn't passed.  Don't unnecessarily double
check the same argument in get_encoding.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index f88e007..6c826e1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -167,9 +167,6 @@ static char *get_encoding(const char *message)
 {
 	const char *p = message, *eol;
 
-	if (!p)
-		die (_("Could not read commit message of %s"),
-				sha1_to_hex(commit->object.sha1));
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
-- 
1.7.5.GIT

Re: [PATCH 03/17] revert: Don't check lone argument in get_encoding

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> The get_encoding function has only one callsite, and its caller makes
> sure that a NULL argument isn't passed.  Don't unnecessarily double
> check the same argument in get_encoding.

Such a double-check is not a huge maintenance burden, is it?  As I
mentioned at [1], I am guessing the actual motivation is (1) to avoid
having to pass "commit" around and (2) to avoid burdening translators
with a message that will never be shown.

Would it be possible to clarify the commit message so it is no longer
necessary to guess?

[1] http://thread.gmane.org/gmane.comp.version-control.git/176139/focus=176166

Re: [PATCH 03/17] revert: Don't check lone argument in get_encoding

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> The get_encoding function has only one callsite, and its caller makes
>> sure that a NULL argument isn't passed.  Don't unnecessarily double
>> check the same argument in get_encoding.
>
> Such a double-check is not a huge maintenance burden, is it?  As I
> mentioned at [1], I am guessing the actual motivation is (1) to avoid
> having to pass "commit" around and (2) to avoid burdening translators
> with a message that will never be shown.
>
> Would it be possible to clarify the commit message so it is no longer
> necessary to guess?

I should have done this earlier.  Thanks for being so patient :)

revert: Don't check lone argument in get_encoding

The get_encoding function has only one callsite, and its caller makes
sure that a NULL argument isn't passed.  Don't unnecessarily double
check the same argument in get_encoding.  As a result, avoid passing
"commit" around, and remove a string marked for translation that will
never be shown.

-- Ram

Re: [PATCH 03/17] revert: Don't check lone argument in get_encoding

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> I should have done this earlier.  Thanks for being so patient :)
>
> revert: Don't check lone argument in get_encoding
>
> The get_encoding function has only one callsite, and its caller makes
> sure that a NULL argument isn't passed.  Don't unnecessarily double
> check the same argument in get_encoding.  As a result, avoid passing
> "commit" around, and remove a string marked for translation that will
> never be shown.

Getting closer.  This still leaves the motivation unclear --- when I
read it it sounds to me like you're saying that avoiding unnecessary
code is the motivation, and that not passing "commit" around and
removing a string marked for translation are means to that end.

[PATCH 04/17] revert: Rename no_replay to record_origin

[Ramkumar Ramachandra]

Rename the variable corresponding to the "-x" command-line option from
"no_replay" to a more apt "record_origin".

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 6c826e1..3d8b3a9 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,7 +35,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static struct commit *commit;
 static int commit_argc;
@@ -91,7 +91,7 @@ static void parse_args(int argc, const char **argv)
 
 	if (action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
+			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
@@ -456,7 +456,7 @@ static int do_pick_commit(void)
 		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
 		strbuf_addstr(&msgbuf, p);
 
-		if (no_replay) {
+		if (record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
@@ -551,7 +551,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
 		if (no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
 		if (edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
-- 
1.7.5.GIT

Re: [PATCH 04/17] revert: Rename no_replay to record_origin

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Rename the variable corresponding to the "-x" command-line option from
> "no_replay" to a more apt "record_origin".

Why is it more apt?  What does "-x" do?  Why was it called "no_replay"
before?  Is there some other motivation to this change (e.g., does this
pave the way to using the name "replay" for something else)?

Re: [PATCH 04/17] revert: Rename no_replay to record_origin

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Rename the variable corresponding to the "-x" command-line option from
>> "no_replay" to a more apt "record_origin".
>
> Why is it more apt?  What does "-x" do?  Why was it called "no_replay"
> before?  Is there some other motivation to this change (e.g., does this
> pave the way to using the name "replay" for something else)?

Thanks for probing.  Found out the reason after some digging.

revert: Rename no_replay to record_origin

The "-x" command-line option is used to record the name of the
original commits being picked in the commit message.  The variable
corresponding to this option is named "no_replay" for historical
reasons.  This name was introduced in f810379 (Make builtin-revert.c
use parse_options, 2007-10-07) to replace "replay", the opposite of
"no_replay".  The name "replay" was introduced in 9509af6 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01).  Back then, "-x"
was the only command-line option that modified the commit message of
the commit being picked, and was in effect replaying a slightly
different commit; hence the names "replay" and "no_replay".

Today, there are many other options like "-s" which append various
things to the commit message, and the term "replay" doesn't mean what
it used to.  So, give the variable corresponding to the "-x"
command-line option a better name: "record_origin".

-- Ram

Re: [PATCH 04/17] revert: Rename no_replay to record_origin

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Thanks for probing.  Found out the reason after some digging.
>
> revert: Rename no_replay to record_origin
>
> The "-x" command-line option is used to record the name of the
> original commits being picked in the commit message.  The variable
> corresponding to this option is named "no_replay" for historical
> reasons.

Heh.  I guess "for crazy historical reasons no one remembers, and it's
especially confusing because the term 'replay' is used to describe
from time to time to describe what cherry-pick does (e.g., in the
documentation for the --mainline option)" would be enough of a summary
to justify the change.

> This name was introduced in f810379 (Make builtin-revert.c
> use parse_options, 2007-10-07) to replace "replay", the opposite of
> "no_replay".  The name "replay" was introduced in 9509af6 (Make
> git-revert & git-cherry-pick a builtin, 2007-03-01).

It's actually older than that: it comes from the scripted version:

	-x|--i-really-want-to-expose-my-private-commit-object-name)
		replay=
		;;

>From v1.4.3-rc2~12 (cherry-pick: make -r the default, 2006-10-05;
it's of course older than that), we learn that this used to be the
default, disabled by a --replay option that meant

	Usually the command appends which commit was
	cherry-picked after the original commit message when
	making a commit.  This option, '--replay', causes it to
	use the original commit message intact.  This is useful
	when you are reordering the patches in your private tree
	before publishing.

or in other words, "simulate making the commit anew again, instead
of explicitly recording that this is a new commit based on that
old one".

In other words, "replay" originally essentially meant what
"cherry-pick" does now.

Thanks, that was interesting.

Cheers,
Jonathan

[PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Currently, the return value from revert_or_cherry_pick is a
non-negative number representing the intended exit status from `git
revert` or `git cherry-pick`.  Change this by replacing some of the
calls to "die" with calls to "error", so that it can return negative
values too.  Postive return values indicate conflicts, while negative
ones indicate other errors.  This return status is propogated updwards
from do_pick_commit, to be finally handled in cmd_cherry_pick and
cmd_revert.

In the same spirit, also introduce a new function error_dirty_index,
based on die_dirty_index, which prints some hints and returns an error
to its caller do_pick_commit.

While the full benefits of this patch will only be seen once all the
"die" calls are replaced with calls to "error", its immediate impact
is to change some of the "die:" messages to "error:" messages and
print a new "fatal: cherry-pick failed" message when the operation
fails.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   69 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3d8b3a9..e2fd7b0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -241,25 +241,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(const char *me)
+static int error_dirty_index(const char *me)
 {
-	if (read_cache_unmerged()) {
-		die_resolve_conflict(me);
-	} else {
-		if (advice_commit_before_merge) {
-			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"
-					  "Please, commit your changes or stash them to proceed."));
-		} else {
-			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
-		}
-	}
+	if (read_cache_unmerged())
+		return error_resolve_conflict(me);
+
+	/* Different translation strings for cherry-pick and revert */
+	if (action == 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(_("Please, commit your changes or stash them to proceed."));
+	return -1;
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -373,12 +368,12 @@ static int do_pick_commit(void)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+			return error(_("Your index file is unmerged."));
 	} else {
 		if (get_sha1("HEAD", head))
-			die (_("You do not have a valid HEAD"));
+			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me);
+			return error_dirty_index(me);
 	}
 	discard_cache();
 
@@ -391,20 +386,20 @@ static int do_pick_commit(void)
 		struct commit_list *p;
 
 		if (!mainline)
-			die(_("Commit %s is a merge but no -m option was given."),
-			    sha1_to_hex(commit->object.sha1));
+			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 != mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != mainline || !p)
-			die(_("Commit %s does not have parent %d"),
-			    sha1_to_hex(commit->object.sha1), mainline);
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), mainline);
 		parent = p->item;
 	} else if (0 < mainline)
-		die(_("Mainline was specified but commit %s is not a merge."),
-		    sha1_to_hex(commit->object.sha1));
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
@@ -414,12 +409,12 @@ static int do_pick_commit(void)
 	if (parent && parse_commit(parent) < 0)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
-		die(_("%s: cannot parse parent commit %s"),
-		    me, sha1_to_hex(parent->object.sha1));
+		return error(_("%s: cannot parse parent commit %s"),
+			me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, &msg) != 0)
-		die(_("Cannot get commit message for %s"),
-				sha1_to_hex(commit->object.sha1));
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
@@ -572,14 +567,22 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res < 0)
+		die(_("revert failed"));
+	return res;
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res < 0)
+		die(_("cherry-pick failed"));
+	return res;
 }
-- 
1.7.5.GIT

Re: [PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Currently, the return value from revert_or_cherry_pick is a
> non-negative number representing the intended exit status from `git
> revert` or `git cherry-pick`.  Change this by replacing some of the
> calls to "die" with calls to "error", so that it can return negative
> values too.  Postive return values indicate conflicts, while negative
> ones indicate other errors.  This return status is propogated updwards
> from do_pick_commit, to be finally handled in cmd_cherry_pick and
> cmd_revert.

As mentioned at [1], this accurately describes the effect, but not the
motivation.  (In tricky cases like this, knowing the motivation would
help with review immensely.)

[...]
> While the full benefits of this patch will only be seen once all the
> "die" calls are replaced with calls to "error", its immediate impact
> is to change some of the "die:" messages to "error:" messages and
> print a new "fatal: cherry-pick failed" message when the operation
> fails.

I think by "die:" you mean "fatal:".

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -373,12 +368,12 @@ static int do_pick_commit(void)
>  		 * to work on.
>  		 */
>  		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +			return error(_("Your index file is unmerged."));

write_cache_as_tree() locks the index and does not always commit or
roll it back except on success.  Current callers aren't likely to try
to lock the index again (since they just die()), but presumably the
goal of returning error() here is to allow for callers that want to
stay alive and do something more.  How should they recover (i.e., what
is the intended API)?

Similar questions probably apply to calls to other APIs that return -1
to mean "I failed; please print an appropriate message about that and
exit".  I haven't checked carefully, since the answer to this example
could help in knowing what to look for in the others.

[1] http://thread.gmane.org/gmane.comp.version-control.git/176647/focus=176664

Re: [PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Currently, the return value from revert_or_cherry_pick is a
>> non-negative number representing the intended exit status from `git
>> revert` or `git cherry-pick`.  Change this by replacing some of the
>> calls to "die" with calls to "error", so that it can return negative
>> values too.  Postive return values indicate conflicts, while negative
>> ones indicate other errors.  This return status is propogated updwards
>> from do_pick_commit, to be finally handled in cmd_cherry_pick and
>> cmd_revert.
>
> As mentioned at [1], this accurately describes the effect, but not the
> motivation.  (In tricky cases like this, knowing the motivation would
> help with review immensely.)

Sorry, I should have done this earlier.  Does this look alright?

revert: Propogate errors upwards from do_pick_commit

Currently, revert_or_cherry_pick can fail in two ways.  If it
encounters a conflict, it returns a positive number indicating the
intended exit status for the git wrapper to pass on; for all other
errors, it calls die().  The latter behavior is inconsiderate towards
callers, as it denies them the opportunity to do some post-processing
or cleanup before exiting.  For instance, later in the series, callers
will want to save some data about the current operation before
exiting.

Change this by replacing some of the calls to "die" with calls to
"error", so that revert_or_cherry_pick can return negative values too.
While postive return values indicate conflicts as before, negative
ones indicate other errors.  This return status is propogated updwards
from do_pick_commit, to be finally handled in cmd_cherry_pick and
cmd_revert.

In the same spirit, also introduce a new function error_dirty_index,
based on die_dirty_index, which prints some hints and returns an error
to its caller do_pick_commit.

While the full benefits of this patch will only be seen once all the
"die" calls are replaced with calls to "error", its immediate impact
is to change some of the "fatal:" messages to "error:" messages and
print a new "fatal: cherry-pick failed" message when the operation
fails.


>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -373,12 +368,12 @@ static int do_pick_commit(void)
>>                * to work on.
>>                */
>>               if (write_cache_as_tree(head, 0, NULL))
>> -                     die (_("Your index file is unmerged."));
>> +                     return error(_("Your index file is unmerged."));
>
> write_cache_as_tree() locks the index and does not always commit or
> roll it back except on success.  Current callers aren't likely to try
> to lock the index again (since they just die()), but presumably the
> goal of returning error() here is to allow for callers that want to
> stay alive and do something more.  How should they recover (i.e., what
> is the intended API)?

Hm, there was supposed to be a discard_cache() before this error as I
recall -- fixed now.  Thanks for catching.

> Similar questions probably apply to calls to other APIs that return -1
> to mean "I failed; please print an appropriate message about that and
> exit".  I haven't checked carefully, since the answer to this example
> could help in knowing what to look for in the others.

I think the others are fairly clear actually.  Let me know if you find
something.

-- Ram

Re: [PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> revert: Propogate errors upwards from do_pick_commit
>
> Currently, revert_or_cherry_pick can fail in two ways.  If it
> encounters a conflict, it returns a positive number indicating the
> intended exit status for the git wrapper to pass on; for all other
> errors, it calls die().  The latter behavior is inconsiderate towards
> callers, as it denies them the opportunity to do some post-processing
> or cleanup before exiting.  For instance, later in the series, callers
> will want to save some data about the current operation before
> exiting.

Thanks for explaining.  Why can't callers use set_die_routine() or
atexit()?  Is the last sentence true?

> Change this by replacing some of the calls to "die" with calls to
> "error", so that revert_or_cherry_pick can return negative values too.
> While postive return values indicate conflicts as before, negative
> ones indicate other errors.  This return status is propogated updwards
> from do_pick_commit, to be finally handled in cmd_cherry_pick and
> cmd_revert.
>
> In the same spirit, also introduce a new function error_dirty_index,
> based on die_dirty_index, which prints some hints and returns an error
> to its caller do_pick_commit.
>
> While the full benefits of this patch will only be seen once all the
> "die" calls are replaced with calls to "error", its immediate impact
> is to change some of the "fatal:" messages to "error:" messages and
> print a new "fatal: cherry-pick failed" message when the operation
> fails.

Starting to get long.  It's not immediately obvious to me why
error_dirty_index is a detail that needs to be singled out.  Maybe
these three paragraphs could be summarized by saying:

 After this patch, revert_or_cherry_pick will still return a positive
 return value to indicate an exit status for conflicts as before,
 while for other (fatal) errors it will print an error message and
 return -1 instead of die()-ing.  The cmd_revert and cmd_cherry_pick
 are adjusted to handle the fatal errors by die()-ing themselves.

 Thus for now, the only user-visible impact would be to change some
 "fatal:" messages to say "error:" and to add a new "fatal:
 cherry-pick failed" message at the end when the operation fails.

 Since no callers take advantage of the ability to recover from
 errors yet, it is possible and even likely that these functions do
 not completely clean up after themselves on (fatal) error but leave
 some state to be cleared away by exit().  Callers beware!

That last paragraph summarizes my worry.  Since this API change does
not seem to be used by the other patches, I would prefer not to
leave such a trap for unwary new callers, at least until the intended
legitimate use is a little clearer.

[...]
>> write_cache_as_tree() locks the index and does not always commit or
>> roll it back except on success.  Current callers aren't likely to try
>> to lock the index again (since they just die()), but presumably the
>> goal of returning error() here is to allow for callers that want to
>> stay alive and do something more.  How should they recover (i.e., what
>> is the intended API)?
>
> Hm, there was supposed to be a discard_cache() before this error as I
> recall -- fixed now.  Thanks for catching.

discard_index() does not unlock the index.

I had wondered what the discard_cache() stuff was about before; thanks
for explaining.

>> Similar questions probably apply to calls to other APIs that return -1
>> to mean "I failed; please print an appropriate message about that and
>> exit".  I haven't checked carefully, since the answer to this example
>> could help in knowing what to look for in the others.
>
> I think the others are fairly clear actually.

Roughly speaking, there are two kinds of functions that return error()
instead of die()-ing in the git codebase:

 1. Those that recover from their errors, leaving the process in a
    sane state that allows the caller (e.g., a long-lived interactive
    porcelain) to go on to do other things

 2. Those that do not have enough information to give a useful error
    message themselves, so delegate to the caller the responsibility
    to die() with an error message about where in the program the
    failure occured.

The commit message suggests that you are introducing a new category:

 3. Those that could die() without trouble, _except_ that some callers
    want to do cleanup of private state on exit and atexit() is not
    working for them for some reason

But from this description of it you can see that the category seems
questionable and unlikely to be necessary to me.

How do callers know which of the three categories a function being
called falls into?  That question is relevant because the caller must
respond accordingly:

 a. In case (1), the caller can go on with its work.
 b. In case (2), the caller writes a good error message and die()s ASAP.
 c. In the new case (3), the caller needs to exit() or die() ASAP,
    but there is no message that especially needs to be written.

I had thought the idea was to put pick_commits() in case (1) (usable
by long-lived porcelain like cgit that don't want to exit).  To
accomplish that, when you call a function of type (2), you would need
to audit its implementation to see what changes it needs to move to
type (1), if any.  (FWIW, I am not convinced you've tried to do this,
hence the comment above about it.)

It clearly is not part of case (2).  All the useful context has
already been written to stderr when pick_commits() returns.

If it is case (3), the reader is going to wonder why you didn't just
use atexit() or set_die_handler() and why it is worth the
complication.

Does that make sense?
Jonathan

Re: [PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> revert: Propogate errors upwards from do_pick_commit
>>
>> Currently, revert_or_cherry_pick can fail in two ways.  If it
>> encounters a conflict, it returns a positive number indicating the
>> intended exit status for the git wrapper to pass on; for all other
>> errors, it calls die().  The latter behavior is inconsiderate towards
>> callers, as it denies them the opportunity to do some post-processing
>> or cleanup before exiting.  For instance, later in the series, callers
>> will want to save some data about the current operation before
>> exiting.
>
> Thanks for explaining.  Why can't callers use set_die_routine() or
> atexit()?  Is the last sentence true?

Definitely a misguided commit message.
New commit message.

revert: Propogate errors upwards from do_pick_commit

Currently, revert_or_cherry_pick can fail in two ways.  If it
encounters a conflict, it returns a positive number indicating the
intended exit status for the git wrapper to pass on; for all other
errors, it calls die().  The latter behavior is inconsiderate towards
callers, as it denies them the opportunity to recover from errors and
do other things.

After this patch, revert_or_cherry_pick will still return a positive
return value to indicate an exit status for conflicts as before, while
for some other errors, it will print an error message and return -1
instead of die()-ing.  The cmd_revert and cmd_cherry_pick are adjusted
to handle the fatal errors by die()-ing themselves.

While the full benefits of this patch will only be seen once all the
"die" calls are replaced with calls to "error", its immediate impact
is to change some "fatal:" messages to say "error:" and to add a new
"fatal: cherry-pick failed" message at the end when the operation
fails.

>  Since no callers take advantage of the ability to recover from
>  errors yet, it is possible and even likely that these functions do
>  not completely clean up after themselves on (fatal) error but leave
>  some state to be cleared away by exit().  Callers beware!
>
> That last paragraph summarizes my worry.  Since this API change does
> not seem to be used by the other patches, I would prefer not to
> leave such a trap for unwary new callers, at least until the intended
> legitimate use is a little clearer.

Very legitimate.  Thanks for explaining.  However, I don't think I've
introduced any new traps for callers except for the
write_cache_as_tree() thing which is fixed now -- so, I've omitted
your warning in the commit message because I'm not entirely sure it's
true.  Yes, we don't have callers that take advantage of this yet, but
the idea of the patch is to work in the right direction -- we have to
convert all the "die" calls to "error" calls and make a nice public
API out of it sometime in the future.

Also, should we worry about it so much at this stage? We haven't set
up a public API

>>> write_cache_as_tree() locks the index and does not always commit or
>>> roll it back except on success.  Current callers aren't likely to try
>>> to lock the index again (since they just die()), but presumably the
>>> goal of returning error() here is to allow for callers that want to
>>> stay alive and do something more.  How should they recover (i.e., what
>>> is the intended API)?
>>
>> Hm, there was supposed to be a discard_cache() before this error as I
>> recall -- fixed now.  Thanks for catching.
>
> discard_index() does not unlock the index.

Ugh, sorry.  I realize now that you were talking about the lockfile,
which will only be cleaned up by exit().  I thought about this for a
while and decided to classify this as an unrecoverable error for the
moment -- we have no way to clean up the lockfile at this stage, so
might as well die() instead of setting a trap for callers.

>>> Similar questions probably apply to calls to other APIs that return -1
>>> to mean "I failed; please print an appropriate message about that and
>>> exit".  I haven't checked carefully, since the answer to this example
>>> could help in knowing what to look for in the others.
>>
>> I think the others are fairly clear actually.
>
> Roughly speaking, there are two kinds of functions that return error()
> instead of die()-ing in the git codebase:
>
>  1. Those that recover from their errors, leaving the process in a
>    sane state that allows the caller (e.g., a long-lived interactive
>    porcelain) to go on to do other things
>
>  2. Those that do not have enough information to give a useful error
>    message themselves, so delegate to the caller the responsibility
>    to die() with an error message about where in the program the
>    failure occured.
>
> The commit message suggests that you are introducing a new category:
>
>  3. Those that could die() without trouble, _except_ that some callers
>    want to do cleanup of private state on exit and atexit() is not
>    working for them for some reason

Thanks for the excellent categorization -- this change falls mostly in
category (1), and a little bit in (2) too.  When we work towards a
general sequencer, do_pick_commit has no idea about the name of git
wrapper that invoked it ("git cherry-pick" or "git revert" in this
case).  Thinking about these things helped me write a better commit
message.

Thanks.

-- Ram

[PATCH 06/17] revert: Eliminate global "commit" variable

[Ramkumar Ramachandra]

Since we want to develop the functionality to either pick or revert
individual commits atomically later in the series, make "commit" a
variable to be passed around explicitly as an argument for clarity.
This involves changing several functions to take an additional
argument, but no functional changes.  Additionaly, this will permit
more than one commit to be cherry-picked at once, should we choose to
develop this functionality in future.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e2fd7b0..840e2a3 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
-static struct commit *commit;
 static int commit_argc;
 static const char **commit_argv;
 static int allow_rerere_auto;
@@ -116,25 +115,25 @@ struct commit_message {
 	const char *message;
 };
 
-static int get_message(const char *raw_message, struct commit_message *out)
+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 (!raw_message)
+	if (!commit->buffer)
 		return -1;
-	encoding = get_encoding(raw_message);
+	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 = raw_message;
+	out->message = commit->buffer;
 	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(raw_message,
+		out->reencoded_message = reencode_string(commit->buffer,
 					git_commit_encoding, encoding);
 	if (out->reencoded_message)
 		out->message = out->reencoded_message;
@@ -182,7 +181,7 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -350,7 +349,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(void)
+static int do_pick_commit(struct commit *commit)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -412,7 +411,7 @@ static int do_pick_commit(void)
 		return error(_("%s: cannot parse parent commit %s"),
 			me, sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(commit, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 			sha1_to_hex(commit->object.sha1));
 
@@ -457,7 +456,7 @@ static int do_pick_commit(void)
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			write_cherry_pick_head(commit);
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -535,6 +534,7 @@ static void read_and_refresh_cache(const char *me)
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	struct commit *commit;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -557,7 +557,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	prepare_revs(&revs);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
+		int res = do_pick_commit(commit);
 		if (res)
 			return res;
 	}
-- 
1.7.5.GIT

Re: [PATCH 06/17] revert: Eliminate global "commit" variable

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Since we want to develop the functionality to either pick or revert
> individual commits atomically later in the series, make "commit" a
> variable to be passed around explicitly as an argument for clarity.

The above explanation is not so clear to me, but the patch looks good.
Isn't the idea something like

	commit = grab_a_nice_commit();
	res = do_pick_commit();

being just an unpleasant API relative to

	res = do_pick_commit(grab_a_nice_commit());

because in the latter it is more obvious which commit is being
cherry-picked?  Likewise with the functions it calls.

Or perhaps the idea is that eventually we will want to expose something
like do_pick_commit to other translation units, but a static variable
like "commit" would not be appropriate for exposing.  Or that we save
a word of global memory.  Or that this way if do_pick_commit or a
function it calls ever ends up recursing by mistake it won't get
broken.  Or that we can use multiple threads some day.  Or...

Oh, the uncertainty! :)  It is not clear to me what any of the above
have to do with wanting the functionality to replay an individual
commit atomically.  By the way, what does pickiing or reverting a
commit atomically mean, and how is it different from ordinary
cherry-picks?

Re: [PATCH 06/17] revert: Eliminate global "commit" variable

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Since we want to develop the functionality to either pick or revert
>> individual commits atomically later in the series, make "commit" a
>> variable to be passed around explicitly as an argument for clarity.
>
> The above explanation is not so clear to me, but the patch looks good.
> Isn't the idea something like
>
>        commit = grab_a_nice_commit();
>        res = do_pick_commit();
>
> being just an unpleasant API relative to
>
>        res = do_pick_commit(grab_a_nice_commit());
>
> because in the latter it is more obvious which commit is being
> cherry-picked?  Likewise with the functions it calls.
>
> Or perhaps the idea is that eventually we will want to expose something
> like do_pick_commit to other translation units, but a static variable
> like "commit" would not be appropriate for exposing.  Or that we save
> a word of global memory.  Or that this way if do_pick_commit or a
> function it calls ever ends up recursing by mistake it won't get
> broken.  Or that we can use multiple threads some day.  Or...
>
> Oh, the uncertainty! :)  It is not clear to me what any of the above
> have to do with wanting the functionality to replay an individual
> commit atomically.  By the way, what does pickiing or reverting a
> commit atomically mean, and how is it different from ordinary
> cherry-picks?

Right.  How about this?

revert: Eliminate global "commit" variable

Functions which act on commits currently rely on a file-scope static
variable to be set before they're called.  Consequently, the API and
corresponding callsites are ugly and unclear.  Remove this variable
and change their API to accept the commit to act on as additional
argument so that the callsites change from looking like

commit = prepare_a_commit();
act_on_commit();

to looking like

commit = prepare_a_commit();
act_on_commit(commit);

This change is also in line with our long-term goal of exposing some
of these functions through a public API.

-- Ram

Re: [PATCH 06/17] revert: Eliminate global "commit" variable

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Right.  How about this?
>
> revert: Eliminate global "commit" variable
>
> Functions which act on commits currently rely on a file-scope static
> variable to be set before they're called.  Consequently, the API and
> corresponding callsites are ugly and unclear.  Remove this variable
> and change their API to accept the commit to act on as additional
> argument so that the callsites change from looking like
>
> commit = prepare_a_commit();
> act_on_commit();
>
> to looking like
>
> commit = prepare_a_commit();
> act_on_commit(commit);
>
> This change is also in line with our long-term goal of exposing some
> of these functions through a public API.

Sounds fine to me.  I could nitpick by saying that "Functions which
act on commits" is a little vague (I think the actual story is
something like "The global 'commit' variable used to represent one of
the arguments passed to 'git cherry-pick' or 'git revert' and thus was
static along with the options.  When cherry-pick learned to replay
multiple commits in sequence, that wasn't changed, resulting in a
somewhat unnatural calling sequence that uses a global to pass in a
parameters:

	for each commit c {
		commit = c;
		res = do_pick_commit();
		if (res)
			return res;
	}

Teach do_pick_commit and the functions it calls to accept the commit
to act on as an argument, like we always should have.  Part of a
campaign to clean up cherry-pick/revert's internal APIs before
exposing them.")

Re: [PATCH 06/17] revert: Eliminate global "commit" variable

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Sounds fine to me.  I could nitpick by saying that "Functions which
> act on commits" is a little vague (I think the actual story is
> something like "The global 'commit' variable used to represent one of
> the arguments passed to 'git cherry-pick' or 'git revert' and thus was
> static along with the options.  When cherry-pick learned to replay
> multiple commits in sequence, that wasn't changed, resulting in a
> somewhat unnatural calling sequence that uses a global to pass in a
> parameters:
>
>        for each commit c {
>                commit = c;
>                res = do_pick_commit();
>                if (res)
>                        return res;
>        }
>
> Teach do_pick_commit and the functions it calls to accept the commit
> to act on as an argument, like we always should have.  Part of a
> campaign to clean up cherry-pick/revert's internal APIs before
> exposing them.")

Yes.  Thanks.  How about this?

revert: Eliminate global "commit" variable

Prior to v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than one
commit, 2010-06-02), the file-scope static "commit" variable used to
represent one of the arguments passed to the "git cherry-pick" or "git
revert".  This was not changed after cherry-pick learnt to replay
multiple commits in a sequence, resulting in ugly and unclear
callsites which rely on setting the global variable before calling a
function to act on it:

for each c in commits {
    commit = c;
    act_on_commit();
}


Remove the global variable and modify the API of various functions to
pass "commit" around as an argument, so that the callsites become
clearer:

for each c in commits {
    act_on_commit(c);
}

The change is also in line with our long-term goal of exposing the
cherry-picking machinery through a public API.

-- Ram

Re: [PATCH 06/17] revert: Eliminate global "commit" variable

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Yes.  Thanks.  How about this?
>
> revert: Eliminate global "commit" variable
>
> Prior to v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than one
> commit, 2010-06-02), the file-scope static "commit" variable used to
> represent one of the arguments passed to the "git cherry-pick" or "git
> revert".  This was not changed after cherry-pick learnt to replay

Meh, I prefer the version starting with "Functions which act on
commits", which was already readable and not too confusing.

The point of the just-so story I wrote is that the "commit" variable
was probably static _because_ of an analogy with command-line options
that needed to be static to be pointed to by the "struct options"
initializer.  But that is not a very important detail; it just helped
my understanding.

Regards,
Jonatahn

[PATCH 07/17] revert: Introduce struct to keep command-line options

[Ramkumar Ramachandra]

The current code uses a set of file-scope static variables to tell the
cherry-pick/ revert machinery how to replay the changes, and
initializes them by parsing the command-line arguments.  In later
steps in this series, we would like to introduce an API function that
calls into this machinery directly and have a way to tell it what to
do.  Hence, introduce a structure to group these variables, so that
the API can take them as a single replay_options parameter.

The variable "me" is left as a file-scope static variable because it
is not an independent option.  "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.

Unfortunately, this patch introduces a minor regression.  Parsing
strategy-option violates a C89 rule: Initializers cannot refer to
variables whose address is not known at compile time.  Currently, this
rule is violated by some other parts of Git as well, and it is
possible to get GCC to report these instances using the "-std=c89
-pedantic" option.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  180 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 101 insertions(+), 79 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 840e2a3..a95cfbb 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,90 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
-static enum { REVERT, CHERRY_PICK } action;
-static int commit_argc;
-static const char **commit_argv;
-static int allow_rerere_auto;
-
 static const char *me;
-
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+enum replay_action { REVERT, CHERRY_PICK };
+
+struct replay_opts {
+	enum replay_action action;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+	int commit_argc;
+	const char **commit_argv;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+};
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static char *get_encoding(const char *message);
 
-static const char * const *revert_or_cherry_pick_usage(void)
+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
 			  const char *arg, int unset)
 {
+	struct replay_opts **opts_ptr = opt->value;
+	struct replay_opts *opts = *opts_ptr;
+
 	if (unset)
 		return 0;
 
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
+	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
 	return 0;
 }
 
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
-	const char * const * usage_str = revert_or_cherry_pick_usage();
+	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	struct option options[] = {
-		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
-		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
+		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
-		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
-		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
-		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
-		OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
-		OPT_CALLBACK('X', "strategy-option", &xopts, "option",
+		OPT_BOOLEAN('s', "signoff", &opts->signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
+		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &opts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (opts->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
 			die(_("program error"));
 	}
 
-	commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-				    PARSE_OPT_KEEP_ARGV0 |
-				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+					PARSE_OPT_KEEP_ARGV0 |
+					PARSE_OPT_KEEP_UNKNOWN);
+	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	opts->commit_argv = argv;
 }
 
 struct commit_message {
@@ -240,7 +254,7 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index(const char *me)
+static int error_dirty_index(const char *me, enum replay_action action)
 {
 	if (read_cache_unmerged())
 		return error_resolve_conflict(me);
@@ -269,7 +283,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
 
 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)
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
 {
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -290,7 +305,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	next_tree = next ? next->tree : empty_tree();
 	base_tree = base ? base->tree : empty_tree();
 
-	for (xopt = xopts; xopt != xopts + xopts_nr; xopt++)
+	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
 		parse_merge_opt(&o, *xopt);
 
 	clean = merge_trees(&o,
@@ -330,7 +345,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * 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)
+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];
@@ -338,9 +353,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (opts->signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!opts->edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -349,7 +364,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit)
+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -359,7 +374,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
 
-	if (no_commit) {
+	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
@@ -372,7 +387,7 @@ static int do_pick_commit(struct commit *commit)
 		if (get_sha1("HEAD", head))
 			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(me);
+			return error_dirty_index(me, opts->action);
 	}
 	discard_cache();
 
@@ -384,25 +399,25 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		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 != mainline && p;
+		     cnt != opts->mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != opts->mainline || !p)
 			return error(_("Commit %s does not have parent %d"),
-				sha1_to_hex(commit->object.sha1), mainline);
+				sha1_to_hex(commit->object.sha1), opts->mainline);
 		parent = p->item;
-	} else if (0 < mainline)
+	} 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 (allow_ff && parent && !hashcmp(parent->object.sha1, head))
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
@@ -424,7 +439,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (opts->action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -450,18 +465,18 @@ static int do_pick_commit(struct commit *commit)
 		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
 		strbuf_addstr(&msgbuf, p);
 
-		if (record_origin) {
+		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 (!no_commit)
+		if (!opts->no_commit)
 			write_cherry_pick_head(commit);
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf);
+					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
 	} else {
 		struct commit_list *common = NULL;
@@ -471,23 +486,23 @@ static int do_pick_commit(struct commit *commit)
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(strategy, xopts_nr, xopts, common,
-					sha1_to_hex(head), 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 (res) {
-		error(action == REVERT
+		error(opts->action == REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
 		print_advice();
-		rerere(allow_rerere_auto);
+		rerere(opts->allow_rerere_auto);
 	} else {
-		if (!no_commit)
-			res = run_git_commit(defmsg);
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
 	}
 
 	free_message(&msg);
@@ -496,18 +511,18 @@ static int do_pick_commit(struct commit *commit)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 {
 	int argc;
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (opts->action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
 	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+		usage(*revert_or_cherry_pick_usage(opts));
 
 	if (prepare_revision_walk(revs))
 		die(_("revision walk setup failed"));
@@ -516,7 +531,7 @@ static void prepare_revs(struct rev_info *revs)
 		die(_("empty commit set passed"));
 }
 
-static void read_and_refresh_cache(const char *me)
+static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
@@ -531,33 +546,34 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv,
+				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
+	parse_args(argc, argv, opts);
 
-	if (allow_ff) {
-		if (signoff)
+	if (opts->allow_ff) {
+		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (no_commit)
+		if (opts->no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (record_origin)
+		if (opts->record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (opts->edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me);
+	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs);
+	prepare_revs(&revs, opts);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit);
+		int res = do_pick_commit(commit, opts);
 		if (res)
 			return res;
 	}
@@ -568,10 +584,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
 	if (isatty(0))
-		edit = 1;
-	action = REVERT;
-	res = revert_or_cherry_pick(argc, argv);
+		opts.edit = 1;
+	opts.action = REVERT;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -580,8 +599,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	int res;
-	action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv);
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
+	opts.action = CHERRY_PICK;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
-- 
1.7.5.GIT

Re: [PATCH 07/17] revert: Introduce struct to keep command-line options

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The variable "me" is left as a file-scope static variable because it
> is not an independent option.  "me" is simply a string that needs to
> be inferred from the "action" option, and is kept global to save each
> function the trouble of determining it independently.

Why not do something like this[1]?

[1] http://thread.gmane.org/gmane.comp.version-control.git/176647/focus=176730

diff --git i/builtin/revert.c w/builtin/revert.c
index 63baca85..3161e696 100644
--- i/builtin/revert.c
+++ w/builtin/revert.c
@@ -36,7 +36,6 @@ static const char * const cherry_pick_usage[] = {
 };
 
 static struct commit *commit;
-static const char *me;
 enum replay_action { REVERT, CHERRY_PICK };
 
 struct replay_opts {
@@ -69,6 +68,11 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
+static const char *action_name(const struct replay_opts *opts)
+{
+	return opts->action == REVERT ? "revert" : "cherry-pick";
+}
+
 static int option_parse_x(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -280,20 +284,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(const char *me, enum replay_action action)
+static NORETURN void die_dirty_index(const struct replay_opts *opts)
 {
 	if (read_cache_unmerged()) {
-		die_resolve_conflict(me);
+		die_resolve_conflict(action_name(opts));
 	} else {
 		if (advice_commit_before_merge) {
-			if (action == REVERT)
+			if (opts->action == REVERT)
 				die(_("Your local changes would be overwritten by revert.\n"
 					  "Please, commit your changes or stash them to proceed."));
 			else
 				die(_("Your local changes would be overwritten by cherry-pick.\n"
 					  "Please, commit your changes or stash them to proceed."));
 		} else {
-			if (action == REVERT)
+			if (opts->action == REVERT)
 				die(_("Your local changes would be overwritten by revert.\n"));
 			else
 				die(_("Your local changes would be overwritten by cherry-pick.\n"));
@@ -347,7 +351,8 @@ 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"), me);
+		die(_("%s: Unable to write new index file"),
+		    action_name(opts));
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -418,7 +423,7 @@ static int do_pick_commit(struct replay_opts *opts)
 		if (get_sha1("HEAD", head))
 			die (_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me, opts->action);
+			die_dirty_index(opts);
 	}
 	discard_cache();
 
@@ -455,7 +460,7 @@ static int do_pick_commit(struct replay_opts *opts)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		die(_("%s: cannot parse parent commit %s"),
-		    me, sha1_to_hex(parent->object.sha1));
+		    action_name(opts), sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, &msg) != 0)
 		die(_("Cannot get commit message for %s"),
@@ -557,10 +562,15 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 		die(_("empty commit set passed"));
 }
 
-static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
+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);
+	int index_fd;
+	const char *me;
+
+	me = action_name(opts);
+
+	index_fd = hold_locked_index(&index_lock, 0);
 	if (read_index_preload(&the_index, NULL) < 0)
 		die(_("git %s: failed to read the index"), me);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
@@ -578,8 +588,7 @@ static int revert_or_cherry_pick(int argc, const char **argv,
 	struct rev_info revs;
 
 	git_config(git_default_config, NULL);
-	me = opts->action == REVERT ? "revert" : "cherry-pick";
-	setenv(GIT_REFLOG_ACTION, me, 0);
+	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
 	parse_args(argc, argv, opts);
 
 	if (opts->allow_ff) {
@@ -593,7 +602,7 @@ static int revert_or_cherry_pick(int argc, const char **argv,
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me, opts);
+	read_and_refresh_cache(opts);
 
 	prepare_revs(&revs, opts);
 

Re: [PATCH 07/17] revert: Introduce struct to keep command-line options

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> The variable "me" is left as a file-scope static variable because it
>> is not an independent option.  "me" is simply a string that needs to
>> be inferred from the "action" option, and is kept global to save each
>> function the trouble of determining it independently.
>
> Why not do something like this[1]?
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/176647/focus=176730

Fixed and rebased all.  Thanks.

revert: Introduce struct to keep command-line options

The current code uses a set of file-scope static variables to tell the
cherry-pick/ revert machinery how to replay the changes, and
initializes them by parsing the command-line arguments.  In later
steps in this series, we would like to introduce an API function that
calls into this machinery directly and have a way to tell it what to
do.  Hence, introduce a structure to group these variables, so that
the API can take them as a single replay_options parameter.  The only
exception is the variable "me" -- remove it since it not an
independent option, and can be inferred from the action.

Unfortunately, this patch introduces a minor regression.  Parsing
strategy-option violates a C89 rule: Initializers cannot refer to
variables whose address is not known at compile time.  Currently, this
rule is violated by some other parts of Git as well, and it is
possible to get GCC to report these instances using the "-std=c89
-pedantic" option.

-- Ram

[PATCH 08/17] revert: Separate cmdline parsing from functional code

[Ramkumar Ramachandra]

Currently, revert_or_cherry_pick does too many things including
argument parsing and setting up to pick the commits; this doesn't make
a good API.  Simplify and rename the function to pick_commits, so that
it just has the responsibility of setting up the revision walker and
calling do_pick_commit in a loop.  Transfer the remaining work to its
callers cmd_cherry_pick and cmd_revert.  Later in the series,
pick_commits will serve as the starting point for continuing the
cherry-pick or revert.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index a95cfbb..da55ba4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -546,17 +546,12 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv,
-				struct replay_opts *opts)
+static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
-	git_config(git_default_config, NULL);
-	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv, opts);
-
 	if (opts->allow_ff) {
 		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -590,7 +585,10 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	if (isatty(0))
 		opts.edit = 1;
 	opts.action = REVERT;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "revert";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -603,7 +601,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(struct replay_opts));
 	opts.action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "cherry-pick";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
-- 
1.7.5.GIT

Re: [PATCH 08/17] revert: Separate cmdline parsing from functional code

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> Currently, revert_or_cherry_pick does too many things including
> argument parsing and setting up to pick the commits; this doesn't make
> a good API.

On the contrary, generally speaking single-call APIs are often very
pleasant to work with.  Consider the difference between

	struct compressor c;
	memset(&c, 0, sizeof(c));
	init_compressor(&c);
	set_compressor_opts(c, COMPRESSOR_OPT_FAST);
	set_compressor_input("foo.c");
	set_compressor_output("foo.c.compressed");
	while (compressor_pump() != COMPRESSOR_DONE)
		; /* just looping */
	fini_compressor(&c);

and

	compress_file("foo.c.compressed", "foo.c", COMPRESSOR_OPT_FAST);

The latter is more pleasant, right?

So this is probably not about the function doing too many things but
something else.

By the way, the API still has the problem described in [1]: it could
not be used from another translation unit, since one of the parameters
(the "me" variable) is passed through a global instead of on the stack
or in registers.

If the goal is a pleasant API, a good way to explain it is sometimes

	Currently you have to do:

		...

	After this change, you can write:

		...

	Isn't that nice?

[1] http://thread.gmane.org/gmane.comp.version-control.git/176647/focus=176668

Re: [PATCH 08/17] revert: Separate cmdline parsing from functional code

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Currently, revert_or_cherry_pick does too many things including
>> argument parsing and setting up to pick the commits; this doesn't make
>> a good API.
>
> On the contrary, generally speaking single-call APIs are often very
> pleasant to work with.
[...]
> So this is probably not about the function doing too many things but
> something else.

Took me some time to figure this out.  I suppose I could also phrase
this as "prepare to make revert_or_cherry_pick the starting point for
continuing in future", but it might result in a huge explanation about
what continuing means, and why continuing won't need to do argument
parsing.  I think I'll put in a more superficial intent (Justified
because I like this change even if we never support continuing in the
future).

New commit message.

revert: Separate cmdline parsing from functional code

Currently, revert_or_cherry_pick sets up a default git config, parses
command-line arguments, before preparing to pick commits.  This makes
for a bad API as the central worry of callers is to assert whether or
not a conflict occured while cherry picking.  The current API is like:

if (revert_or_cherry_pick(argc, argv, opts) < 0)
   print "Something failed, I'm not sure what"

Simplify and rename revert_or_cherry_pick to pick_commits so that it
only has the responsibility of setting up the revision walker and
picking commits in a loop.  Transfer the remaining work to its
callers.  Now, the API is simplified as:

if (parse_args(argc, argv, opts) < 0)
   print "Can't parse arguments"
if (pick_commits(opts) < 0)
   print "Something in the picking mechanism failed"

Later in the series, pick_commits will serve as the starting point for
continuing a cherry-pick or revert.

-- Ram

Re: [PATCH 08/17] revert: Separate cmdline parsing from functional code

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> New commit message.
>
> revert: Separate cmdline parsing from functional code
>
> Currently, revert_or_cherry_pick sets up a default git config, parses
> command-line arguments, before preparing to pick commits.  This makes
> for a bad API as the central worry of callers is to assert whether or
> not a conflict occured while cherry picking.  The current API is like:
>
> if (revert_or_cherry_pick(argc, argv, opts) < 0)
>    print "Something failed, I'm not sure what"

Nice.  That's much clearer.

[...]
> Later in the series, pick_commits will serve as the starting point for
> continuing a cherry-pick or revert.

With the addition of "also" between will and serve, it looks good to
me now.  Thank you.

[PATCH 09/17] revert: Don't create invalid replay_opts in parse_args

[Ramkumar Ramachandra]

The "--ff" command-line option cannot be used with four other
command-line options.  However, parse_args still parses these
incompatible options into a replay_opts structure for use by the rest
of the program.  Although pick_commits, the current gatekeeper to the
cherry-pick machinery, checks the validity of the replay_opts
structure before before starting its operation, there will be multiple
entry points to the cherry-pick machinery in future.  To futureproof
the code and catch these errors in one place, make sure that an
invalid replay_opts structure is not created by parse_args in the
first place.  Also ensure that regressions in maintaining this
invariant are caught in the future by adding an assertion in
pick_commits.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   37 ++++++++++++++++++++++++++-----------
 1 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index da55ba4..ef5b097 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -82,6 +82,22 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void verify_opt_compatible(const char *me, const char *base_opt, ...)
+{
+	const char *this_opt;
+	va_list ap;
+	int set;
+
+	va_start(ap, base_opt);
+	while ((this_opt = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			die(_("%s: %s cannot be used with %s"),
+				me, this_opt, base_opt);
+	}
+	va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
@@ -118,6 +134,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
+	if (opts->allow_ff)
+		verify_opt_compatible(me, "--ff",
+				"--signoff", opts->signoff,
+				"--no-commit", opts->no_commit,
+				"-x", opts->record_origin,
+				"--edit", opts->edit,
+				NULL);
 	opts->commit_argv = argv;
 }
 
@@ -552,17 +575,9 @@ static int pick_commits(struct replay_opts *opts)
 	struct commit *commit;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	if (opts->allow_ff) {
-		if (opts->signoff)
-			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (opts->no_commit)
-			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (opts->record_origin)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (opts->edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);
-- 
1.7.5.GIT

Re: [PATCH 09/17] revert: Don't create invalid replay_opts in parse_args

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> structure before before starting its operation, there will be multiple

before before

Re: [PATCH 09/17] revert: Don't create invalid replay_opts in parse_args

[Ramkumar Ramachandra]

Hi,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> structure before before starting its operation, there will be multiple
>
> before before

Fixed.  Thanks.

-- Ram

Re: [PATCH 09/17] revert: Don't create invalid replay_opts in parse_args

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The "--ff" command-line option cannot be used with four other
> command-line options.

The above sounds like --ff is a bit of a diva and does not like to
be on a commandline with argc >= 6.  But I get the idea.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 09/17] revert: Don't create invalid replay_opts in parse_args

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> The "--ff" command-line option cannot be used with four other
>> command-line options.
>
> The above sounds like --ff is a bit of a diva and does not like to
> be on a commandline with argc >= 6.  But I get the idea.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.  New commit message de-emphasizing "four":

revert: Don't create invalid replay_opts in parse_args

The "--ff" command-line option cannot be used with some other
command-line options.  However, parse_args still parses these
incompatible options into a replay_opts structure for use by the rest
of the program.  Although pick_commits, the current gatekeeper to the
cherry-pick machinery, checks the validity of the replay_opts
structure before before starting its operation, there will be multiple
entry points to the cherry-pick machinery in future.  To futureproof
the code and catch these errors in one place, make sure that an
invalid replay_opts structure is not created by parse_args in the
first place.  Also ensure that regressions in maintaining this
invariant are caught in the future by adding an assertion in
pick_commits.

-- Ram

Re: [PATCH 09/17] revert: Don't create invalid replay_opts in parse_args

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Thanks.  New commit message de-emphasizing "four":

Looks good.  While at it, it might be nice to simplify the final
sentence:

> revert: Don't create invalid replay_opts in parse_args
>
> The "--ff" command-line option cannot be used with some other
> command-line options.  However, parse_args still parses these
> incompatible options into a replay_opts structure for use by the rest
> of the program.  Although pick_commits, the current gatekeeper to the
> cherry-pick machinery, checks the validity of the replay_opts
> structure before before starting its operation, there will be multiple
> entry points to the cherry-pick machinery in future.  To futureproof
> the code and catch these errors in one place, make sure that an
> invalid replay_opts structure is not created by parse_args in the
> first place.  Also ensure that regressions in maintaining this
> invariant are caught in the future by adding an assertion in
> pick_commits.

I think the idea is "we still check the options struct for validity
in pick_commits, but this is an assert() call now to emphasize that
it's the caller's responsibility to get it right".

[PATCH 10/17] sequencer: Announce sequencer state location

[Ramkumar Ramachandra]

The plan to build a generic sequencer involves fist writing the
functionality into builtin/revert.c (for cherry-pick and revert), and
then factoring it out into sequencer.c and exposing it through
sequencer.h as a nice API.  As a prelude to this, announce the
location of the sequencer state in sequencer.h and write a function to
remove it.  Later in the series, two indepenent Git programs, namely
cherry-pick/ revert and reset, will use this to save and clear
sequencer state in a uniform manner.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile    |    2 ++
 sequencer.c |   17 +++++++++++++++++
 sequencer.h |   14 ++++++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)
 create mode 100644 sequencer.c
 create mode 100644 sequencer.h

diff --git a/Makefile b/Makefile
index f8c72e1..4ac09df 100644
--- a/Makefile
+++ b/Makefile
@@ -551,6 +551,7 @@ LIB_H += rerere.h
 LIB_H += resolve-undo.h
 LIB_H += revision.h
 LIB_H += run-command.h
+LIB_H += sequencer.h
 LIB_H += sha1-array.h
 LIB_H += sha1-lookup.h
 LIB_H += sideband.h
@@ -655,6 +656,7 @@ LIB_OBJS += revision.o
 LIB_OBJS += run-command.o
 LIB_OBJS += server-info.o
 LIB_OBJS += setup.o
+LIB_OBJS += sequencer.o
 LIB_OBJS += sha1-array.o
 LIB_OBJS += sha1-lookup.o
 LIB_OBJS += sha1_file.o
diff --git a/sequencer.c b/sequencer.c
new file mode 100644
index 0000000..8c1de63
--- /dev/null
+++ b/sequencer.c
@@ -0,0 +1,17 @@
+#include "cache.h"
+#include "sequencer.h"
+#include "strbuf.h"
+#include "dir.h"
+
+void remove_sequencer_state(void)
+{
+	struct strbuf seq_dir = STRBUF_INIT;
+	struct strbuf seq_old_dir = STRBUF_INIT;
+
+	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
+	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
+	remove_dir_recursively(&seq_old_dir, 0);
+	rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
+	strbuf_release(&seq_dir);
+	strbuf_release(&seq_old_dir);
+}
diff --git a/sequencer.h b/sequencer.h
new file mode 100644
index 0000000..673616b
--- /dev/null
+++ b/sequencer.h
@@ -0,0 +1,14 @@
+#ifndef SEQUENCER_H
+#define SEQUENCER_H
+
+#define SEQ_DIR		"sequencer"
+#define SEQ_OLD_DIR	"sequencer-old"
+#define SEQ_HEAD_FILE	"sequencer/head"
+#define SEQ_TODO_FILE	"sequencer/todo"
+
+/* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
+ * any errors.  Intended to be used by 'git reset --hard'.
+ */
+void remove_sequencer_state(void);
+
+#endif
-- 
1.7.5.GIT

Re: [PATCH 10/17] sequencer: Announce sequencer state location

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The plan to build a generic sequencer involves fist writing the
> functionality into builtin/revert.c (for cherry-pick and revert), and
> then factoring it out into sequencer.c and exposing it through
> sequencer.h as a nice API.  As a prelude to this, announce the
> location of the sequencer state in sequencer.h and write a function to
> remove it.

Wait, so what is the actual impact of this patch?

As far as I can tell, it is to introduce a remove_sequencer_state()
function whose eventual purpose would be to remove the .git/sequencer
directory used by "git cherry-pick" to record its progress.  Paraphrased,
the function does this:

	rm -fr "$GIT_DIR/sequencer-old"
	mv "$GIT_DIR/sequencer" "$GIT_DIR/sequencer-old"

This way, a person has one level of "undo" possible, assuming git
commands and scripts use the function carefully enough not to call it
twice.

Is the "undo" feature advertised anywhere?  Should this function write
a message to stderr to inform the operator about how to get back the
precious cherry-pick state?

If I were doing it, I'd squash this with the patch that introduces
"git cherry-pick --quit", to give an example of how the new function is
meant to be used (and tests!).

> --- /dev/null
> +++ b/sequencer.h
> @@ -0,0 +1,14 @@
[...]
> +/* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
> + * any errors.  Intended to be used by 'git reset --hard'.
> + */
> +void remove_sequencer_state(void);

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=scripts/checkpatch.pl

Re: [PATCH 10/17] sequencer: Announce sequencer state location

[Sverre Rabbelier]

Heya,

On Tue, Jul 12, 2011 at 20:56, Jonathan Nieder <jrnieder@gmail.com> wrote:
> As far as I can tell, it is to introduce a remove_sequencer_state()
> function whose eventual purpose would be to remove the .git/sequencer
> directory used by "git cherry-pick" to record its progress.  Paraphrased,
> the function does this:

I think we can learn from hg here and just use a datetime stamp. So
instead of moving to sequencer-old move to sequencer-20110713-141022
or such. That way there's as many undo as you need.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 10/17] sequencer: Announce sequencer state location

[Ramkumar Ramachandra]

Hi Jonathan and Sverre,

Jonathan Nieder writes:
> Wait, so what is the actual impact of this patch?
>
> As far as I can tell, it is to introduce a remove_sequencer_state()
> function whose eventual purpose would be to remove the .git/sequencer
> directory used by "git cherry-pick" to record its progress.  Paraphrased,
> the function does this:
>
>        rm -fr "$GIT_DIR/sequencer-old"
>        mv "$GIT_DIR/sequencer" "$GIT_DIR/sequencer-old"
>
> This way, a person has one level of "undo" possible, assuming git
> commands and scripts use the function carefully enough not to call it
> twice.
>
> Is the "undo" feature advertised anywhere?  Should this function write
> a message to stderr to inform the operator about how to get back the
> precious cherry-pick state?

I don't think we should advertise it on stdout or stderr.  I think of
a safeguard that only very few people will use.  Ofcourse, documenting
it somewhere like Documentation/technical will make sense once we have
something usable.

> If I were doing it, I'd squash this with the patch that introduces
> "git cherry-pick --quit", to give an example of how the new function is
> meant to be used (and tests!).

I would have done that too, except that the "reset: Make hard reset
remove the sequencer state" patch depends on this.  So, I don't think
it's fair to squash it into either patch.
I can definitely write a better commit message though (wait for the
next iteration to see it).

Sverre Rabbelier writes:
> I think we can learn from hg here and just use a datetime stamp. So
> instead of moving to sequencer-old move to sequencer-20110713-141022
> or such. That way there's as many undo as you need.

That's a great example in general, but I'm wondering if it would
really be useful for this particular usecase.  Are sequencer states so
valuable that we want to allow so many levels of undo?

-- Ram

Re: [PATCH 10/17] sequencer: Announce sequencer state location

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> I don't think we should advertise it on stdout or stderr.  I think of
> a safeguard that only very few people will use.  Ofcourse, documenting
> it somewhere like Documentation/technical will make sense once we have
> something usable.

Yuck.  Documentation/technical is for API, protocol, and design
documentation.  One should not need to read Documentation/technical in
order to be able to use git!

If the worry is that such a hint would be too noisy, it can be
protected by a configuration variable using the usual advice
mechanism.

>> If I were doing it, I'd squash this with the patch that introduces
>> "git cherry-pick --quit", to give an example of how the new function is
>> meant to be used (and tests!).
>
> I would have done that too, except that the "reset: Make hard reset
> remove the sequencer state" patch depends on this.  So, I don't think
> it's fair to squash it into either patch.
> I can definitely write a better commit message though (wait for the
> next iteration to see it).

Would it be possible to introduce "cherry-pick --quit" here, and make
the later patch only do one thing, namely adding the check to prevent
a cherry-pick in the middle of a pending cherry-pick?

In other words, I don't understand why the "reset --hard" workaround
would need to come before "cherry-pick --quit".

Thanks for explaining.
Jonathan

Re: [PATCH 10/17] sequencer: Announce sequencer state location

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> I don't think we should advertise it on stdout or stderr.  I think of
>> a safeguard that only very few people will use.  Ofcourse, documenting
>> it somewhere like Documentation/technical will make sense once we have
>> something usable.
>
> Yuck.  Documentation/technical is for API, protocol, and design
> documentation.  One should not need to read Documentation/technical in
> order to be able to use git!
>
> If the worry is that such a hint would be too noisy, it can be
> protected by a configuration variable using the usual advice
> mechanism.

Okay, although I'm not inclined to do this right away.

>>> If I were doing it, I'd squash this with the patch that introduces
>>> "git cherry-pick --quit", to give an example of how the new function is
>>> meant to be used (and tests!).
>>
>> I would have done that too, except that the "reset: Make hard reset
>> remove the sequencer state" patch depends on this.  So, I don't think
>> it's fair to squash it into either patch.
>> I can definitely write a better commit message though (wait for the
>> next iteration to see it).
>
> Would it be possible to introduce "cherry-pick --quit" here, and make
> the later patch only do one thing, namely adding the check to prevent
> a cherry-pick in the middle of a pending cherry-pick?
>
> In other words, I don't understand why the "reset --hard" workaround
> would need to come before "cherry-pick --quit".

*pants* That was quite a challenging rebase.
I think I managed to get it right after several attempts -- we'll find
out soon enough.

-- Ram

[PATCH 11/17] revert: Save data for continuing after conflict resolution

[Ramkumar Ramachandra]

Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
one commit, 2010-06-02), a single invocation of "git cherry-pick" or
"git revert" can perform picks of several individual commits.  To
implement features like "--continue" to continue the whole operation,
we will need to store some information about the state and the plan at
the beginning.  Introduce a ".git/sequencer/head" file to store this
state, and ".git/sequencer/todo" file to store the plan.  The head
file contains the SHA-1 of the HEAD before the start of the operation,
and the todo file contains an instruction sheet whose format is
inspired by the format of the "rebase -i" instruction sheet.  As a
result, a typical todo file looks like:

pick 8537f0e submodule add: test failure when url is not configured
pick 4d68932 submodule add: allow relative repository path
pick f22a17e submodule add: clean up duplicated code
pick 59a5775 make copy_ref globally available

Since SHA-1 hex is abbreviated using an find_unique_abbrev(), it is
unambiguous.  This does not guarantee that there will be no ambiguity
when more objects are added to the repository.

It is imperative to note that these two files alone are not enough to
implement "--continue"; we will also need to persist the options that
were specified on the command-line, and this is done later in the
series.

These new files are unrelated to the existing .git/CHERRY_PICK_HEAD,
which will still be useful while committing after a conflict
resolution.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |  202 ++++++++++++++++++++++++++++++++++++++-
 t/t3510-cherry-pick-sequence.sh |   48 +++++++++
 2 files changed, 246 insertions(+), 4 deletions(-)
 create mode 100755 t/t3510-cherry-pick-sequence.sh

diff --git a/builtin/revert.c b/builtin/revert.c
index ef5b097..856ee97 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -13,6 +13,8 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
+#include "dir.h"
+#include "sequencer.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -569,10 +571,192 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int pick_commits(struct replay_opts *opts)
+/* Insert into todo_list in same order; commit_list_insert reverses
+ * the order
+ *
+ * Usage example:
+ *
+ *     struct commit_list *list;
+ *     struct commit_list **next = &list;
+ *
+ *     next = commit_list_append(c1, next);
+ *     next = commit_list_append(c2, next);
+ *     *next = NULL;
+ *     assert(commit_list_count(list) == 2);
+ *     return list;
+ *
+ * Don't forget to NULL-terminate!
+ */
+struct commit_list **commit_list_append(struct commit *commit,
+					struct commit_list **next)
+{
+	struct commit_list *new = xmalloc(sizeof(struct commit_list));
+	new->item = commit;
+	*next = new;
+	return &new->next;
+}
+
+static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
+			struct replay_opts *opts)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1_abbrev = NULL;
+	const char *action;
+
+	action = (opts->action == REVERT ? "revert" : "pick");
+	for (cur = todo_list; cur; cur = cur->next) {
+		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->item, &msg))
+			die(_("Cannot get commit message for %s"), sha1_abbrev);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
+	}
+}
+
+static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+{
+	unsigned char commit_sha1[20];
+	char sha1_abbrev[40];
+	struct commit *commit;
+	enum replay_action action;
+	int insn_len = 0;
+	char *p;
+
+	p = start;
+	if (!(p = strchr(p, ' ')))
+		return NULL;
+	insn_len = p - start;
+	if (!(p = strchr(p + 1, ' ')))
+		return NULL;
+	p += 1;
+	strlcpy(sha1_abbrev, start + insn_len + 1,
+		p - (start + insn_len + 1));
+
+	if (!strncmp(start, "pick", insn_len))
+		action = CHERRY_PICK;
+	else if (!strncmp(start, "revert", insn_len))
+		action = REVERT;
+	else
+		return NULL;
+
+	/*
+	 * Verify that the action matches up with the one in
+	 * opts; we don't support arbitrary instructions
+	 */
+	if (action != opts->action)
+		return NULL;
+
+	if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
+		|| !(commit = lookup_commit_reference(commit_sha1)))
+		return NULL;
+
+	return commit;
+}
+
+static void read_populate_todo(struct commit_list **todo_list,
+			struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct commit_list *new;
+	struct commit_list **next;
+	struct commit *commit;
+	char *p;
+	int fd;
+
+	fd = open(git_path(SEQ_TODO_FILE), O_RDONLY);
+	if (fd < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not open %s."), git_path(SEQ_TODO_FILE));
+	}
+	if (strbuf_read(&buf, fd, 0) < buf.len) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), git_path(SEQ_TODO_FILE));
+	}
+	close(fd);
+
+	next = todo_list;
+	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
+		if (!(commit = parse_insn_line(p, opts)))
+			goto error;
+		new = xmalloc(sizeof(struct commit_list));
+		new->item = commit;
+		*next = new;
+		next = &new->next;
+	}
+	*next = NULL;
+	if (!*todo_list)
+		goto error;
+	strbuf_release(&buf);
+	return;
+error:
+	strbuf_release(&buf);
+	die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
+}
+
+static void walk_revs_populate_todo(struct commit_list **todo_list,
+				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	struct commit_list **next;
+
+	prepare_revs(&revs, opts);
+
+	next = todo_list;
+	while ((commit = get_revision(&revs)))
+		next = commit_list_append(commit, next);
+	*next = NULL;
+}
+
+static void create_seq_dir(void)
+{
+	if (file_exists(git_path(SEQ_DIR))) {
+		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
+			die(_("Could not remove %s"), git_path(SEQ_DIR));
+	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
+		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
+}
+
+static void save_head(const char *head)
+{
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&head_lock, git_path(SEQ_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."), git_path(SEQ_HEAD_FILE));
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), git_path(SEQ_HEAD_FILE));
+}
+
+static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	static struct lock_file todo_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, git_path(SEQ_TODO_FILE), LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s."), git_path(SEQ_TODO_FILE));
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s"), git_path(SEQ_TODO_FILE));
+	}
+	strbuf_release(&buf);
+}
+
+static int pick_commits(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+	struct commit_list *cur;
+	int res;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	if (opts->allow_ff)
@@ -580,14 +764,24 @@ static int pick_commits(struct replay_opts *opts)
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs, opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	create_seq_dir();
+	if (!get_sha1("HEAD", sha1))
+		save_head(sha1_to_hex(sha1));
+	save_todo(todo_list, opts);
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit, opts);
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_todo(cur, opts);
+		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
 	}
 
+	/*
+	 * Sequence of picks finished successfully; cleanup by
+	 * removing the .git/sequencer directory
+	 */
+	remove_sequencer_state();
 	return 0;
 }
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
new file mode 100755
index 0000000..64eaa20
--- /dev/null
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='Test cherry-pick continuation features
+
+  + anotherpick: rewrites foo to d
+  + picked: rewrites foo to c
+  + unrelatedpick: rewrites unrelated to reallyunrelated
+  + base: rewrites foo to b
+  + initial: writes foo as a, unrelated as unrelated
+
+'
+
+. ./test-lib.sh
+
+pristine_detach () {
+	git checkout -f "$1^0" &&
+	git read-tree -u --reset HEAD &&
+	git clean -d -f -f -q -x
+}
+
+test_expect_success setup '
+	echo unrelated >unrelated &&
+	git add unrelated &&
+	test_commit initial foo a &&
+	test_commit base foo b &&
+	test_commit unrelatedpick unrelated reallyunrelated &&
+	test_commit picked foo c &&
+	test_commit anotherpick foo d &&
+	git config advice.detachedhead false
+
+'
+
+test_expect_success 'cherry-pick persists data on failure' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	test_path_is_dir .git/sequencer &&
+	test_path_is_file .git/sequencer/head &&
+	test_path_is_file .git/sequencer/todo &&
+	rm -rf .git/sequencer
+'
+
+test_expect_success 'cherry-pick cleans up sequencer state upon success' '
+	pristine_detach initial &&
+	git cherry-pick initial..picked &&
+	test_path_is_missing .git/sequencer
+'
+
+test_done
-- 
1.7.5.GIT

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +/* Insert into todo_list in same order; commit_list_insert reverses
> + * the order

Style: end the first line of multi-line comment at "/*".

As you say "in same order", you solicit a question "The same as what?".
As you say "insert reverses the order", you sound as if you are
complaining you do not want insert to do so.

And you do not need either of them. The function is "append", and if you
explain it as "append", you do not have to contrast it with "insert".
In other words, starting this comment with

        /*
         * Append a commit at the end of the commit_list.

is perfectly adequate, I think. More useful would be (although it could be
read from the usage example) to help callers what "next" means, perhaps
like:

         * next starts by pointing at the variable that holds the head of
         * the list when the for an empty commit_list, and is updated to
         * point at the "next" field of the last item on the list, as new
         * commits are appended to the list.

> + *
> + * Usage example:
> + *
> + *     struct commit_list *list;
> + *     struct commit_list **next = &list;
> + *
> + *     next = commit_list_append(c1, next);
> + *     next = commit_list_append(c2, next);
> + *     *next = NULL;
> + *     assert(commit_list_count(list) == 2);
> + *     return list;
> + *
> + * Don't forget to NULL-terminate!

I am still not convinced that making it the caller's responsibility to
NULL-terminate the list after it finishes to append is a good trade-off
between run-time performance and ease of API use.  If you are appending
thousands of commits to a commit list in a tight loop, surely you would
save the same thousands of assignment of NULL to the next field of the
element at the tail of the list, which may reduce the instruction count a
tiny bit, but that field was assigned in the last round in that tight loop
and the cacheline would likely to be owned by the CPU already, so it might
not make much practical difference.

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>          * next starts by pointing at the variable that holds the head of
>          * the list when the for an empty commit_list, and is updated to

s/when the//; sorry for the edit-o.

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> + *
>> + * Usage example:
>> + *
>> + *     struct commit_list *list;
>> + *     struct commit_list **next = &list;
>> + *
>> + *     next = commit_list_append(c1, next);
>> + *     next = commit_list_append(c2, next);
>> + *     *next = NULL;
>> + *     assert(commit_list_count(list) == 2);
>> + *     return list;
>> + *
>> + * Don't forget to NULL-terminate!
>
> I am still not convinced that making it the caller's responsibility to
> NULL-terminate the list after it finishes to append is a good trade-off
> between run-time performance and ease of API use.  If you are appending
> thousands of commits to a commit list in a tight loop, surely you would
> save the same thousands of assignment of NULL to the next field of the
> element at the tail of the list, which may reduce the instruction count a
> tiny bit, but that field was assigned in the last round in that tight loop
> and the cacheline would likely to be owned by the CPU already, so it might
> not make much practical difference.

Ah, I hadn't thought about it this deeply.  Changed now- thanks for
the explanation.

-- Ram

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> It is imperative to note that these two files alone are not enough to
> implement "--continue"; we will also need to persist the options that
> were specified on the command-line, and this is done later in the
> series.

I think you could remove the phrase "It is imperative to note that"
and this would say the same thing. :)

By the way, those two files are enough to implement --continue in some
sense, no?  One implementation of --continue would forget options
passed on the first run and require the user to specify them again; it
would just be less useful.  The reminder that "head" and "todo" are
not the only files in the .git/sequencer dir is useful, though; thanks
for it.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> +static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
> +			struct replay_opts *opts)
> +{
> +	struct commit_list *cur = NULL;
> +	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> +	const char *sha1_abbrev = NULL;
> +	const char *action;
> +
> +	action = (opts->action == REVERT ? "revert" : "pick");
> +	for (cur = todo_list; cur; cur = cur->next) {
> +		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
> +		if (get_message(cur->item, &msg))
> +			die(_("Cannot get commit message for %s"), sha1_abbrev);
> +		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);

This die() seems odd in the context of a series that starts by
converting various die() calls to "return error()".

> +static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
> +{
> +	unsigned char commit_sha1[20];
> +	char sha1_abbrev[40];
> +	struct commit *commit;
> +	enum replay_action action;
> +	int insn_len = 0;
> +	char *p;
> +
> +	p = start;
> +	if (!(p = strchr(p, ' ')))
> +		return NULL;

Style: it is much, much clearer to write

	p = strchr(start, ' ');
	if (!p)
		return NULL;

In the git codebase, assignments in "if" conditionals are very rare, so
code like the above sticks out.

> +	insn_len = p - start;
> +	if (!(p = strchr(p + 1, ' ')))
> +		return NULL;
> +	p += 1;

Likewise.  This could say:

	insn_len = p - start;
	p = strchr(p + 1, ' ');
	if (!p)
		return NULL;
	p++;

or:

	p = strchrnul(start, ' ');
	if (!*p)
		return error("no commit name in todo file line: %s", start);
	p++;
	q = strchrnul(p, ' ');
	if (q - p > sizeof(sha1_abbrev) - 1)
		return error("commit name in todo file is too long: %s", start);
	*mempcpy(sha1_abbrev, p, q - p) = '\0';

> +	if (!strncmp(start, "pick", insn_len))
> +		action = CHERRY_PICK;
> +	else if (!strncmp(start, "revert", insn_len))
> +		action = REVERT;

This code means that "p", "pi", and "pic" are accepted as
abbreviations for "pick".   Maybe a comment would help to clarify
that.  Are we okay with reserving these names, so "r" in a todo
file means to "revert" and not to "reword" like it does in a
rebase -i todo list?

> +	/*
> +	 * Verify that the action matches up with the one in
> +	 * opts; we don't support arbitrary instructions
> +	 */
> +	if (action != opts->action)
> +		return NULL;

If I try "git cherry-pick foo..bar" and then "git revert --continue",
what error message will I get?

> +static void read_populate_todo(struct commit_list **todo_list,
> +			struct replay_opts *opts)
> +{
[...]
> +	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
> +		if (!(commit = parse_insn_line(p, opts)))
> +			goto error;
> +		new = xmalloc(sizeof(struct commit_list));
> +		new->item = commit;
> +		*next = new;
> +		next = &new->next;

commit_list_append()?

> +static void create_seq_dir(void)
> +{
> +	if (file_exists(git_path(SEQ_DIR))) {
> +		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
> +			die(_("Could not remove %s"), git_path(SEQ_DIR));
> +	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
> +		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
> +}

As mentioned in [1], a local would make this more readable.  And it's
not clear to me why one would want to delete a .git/sequencer file
that the user carefully placed there (I don't think git itself ever
writes such a thing).

> +
> +static void save_head(const char *head)
> +{
> +	static struct lock_file head_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +
> +	fd = hold_lock_file_for_update(&head_lock, git_path(SEQ_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."), git_path(SEQ_HEAD_FILE));
> +	if (commit_lock_file(&head_lock) < 0)
> +		die(_("Error wrapping up %s"), git_path(SEQ_HEAD_FILE));
> +}

A local would be helpful here, too.

> +static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> +{
> +	static struct lock_file todo_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +
> +	fd = hold_lock_file_for_update(&todo_lock, git_path(SEQ_TODO_FILE), LOCK_DIE_ON_ERROR);
> +	format_todo(&buf, todo_list, opts);
> +	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> +		strbuf_release(&buf);
> +		die_errno(_("Could not write to %s."), git_path(SEQ_TODO_FILE));
> +	}
> +	if (commit_lock_file(&todo_lock) < 0) {
> +		strbuf_release(&buf);
> +		die(_("Error wrapping up %s"), git_path(SEQ_TODO_FILE));
> +	}
> +	strbuf_release(&buf);
> +}

Likewise here.  As mentioned before, git_path() clobbers errno, so the
above is likely to print a wrong error message on some systems (e.g.,
"fatal: Cannot write to .git/sequencer/todo: Success").

> +
> +static int pick_commits(struct replay_opts *opts)
> +{
> +	struct commit_list *todo_list = NULL;
> +	unsigned char sha1[20];
> +	struct commit_list *cur;
> +	int res;
>  
>  	setenv(GIT_REFLOG_ACTION, me, 0);
>  	if (opts->allow_ff)
> @@ -580,14 +764,24 @@ static int pick_commits(struct replay_opts *opts)
>  				opts->record_origin || opts->edit));
>  	read_and_refresh_cache(me, opts);
>  
> -	prepare_revs(&revs, opts);
> +	walk_revs_populate_todo(&todo_list, opts);
> +	create_seq_dir();
> +	if (!get_sha1("HEAD", sha1))
> +		save_head(sha1_to_hex(sha1));

What happens if the .git/sequencer dir already exists (e.g., we were
in the middle of a multiple-cherry-pick)?  What happens if I try to
cherry-pick onto an unborn branch, or get_sha1() fails for some other
reason?

Regards,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/176647/focus=176675

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>
>> It is imperative to note that these two files alone are not enough to
>> implement "--continue"; we will also need to persist the options that
>> were specified on the command-line, and this is done later in the
>> series.
>
> I think you could remove the phrase "It is imperative to note that"
> and this would say the same thing. :)
>
> By the way, those two files are enough to implement --continue in some
> sense, no?  One implementation of --continue would forget options
> passed on the first run and require the user to specify them again; it
> would just be less useful.  The reminder that "head" and "todo" are
> not the only files in the .git/sequencer dir is useful, though; thanks
> for it.

New commit message:

revert: Save data for continuing after conflict resolution

Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
one commit, 2010-06-02), a single invocation of "git cherry-pick" or
"git revert" can perform picks of several individual commits.  To
implement features like "--continue" to continue the whole operation,
we will need to store some information about the state and the plan at
the beginning.  Introduce a ".git/sequencer/head" file to store this
state, and ".git/sequencer/todo" file to store the plan.  The head
file contains the SHA-1 of the HEAD before the start of the operation,
and the todo file contains an instruction sheet whose format is
inspired by the format of the "rebase -i" instruction sheet.  As a
result, a typical todo file looks like:

pick 8537f0e submodule add: test failure when url is not configured
pick 4d68932 submodule add: allow relative repository path
pick f22a17e submodule add: clean up duplicated code
pick 59a5775 make copy_ref globally available

Since SHA-1 hex is abbreviated using an find_unique_abbrev(), it is
unambiguous.  This does not guarantee that there will be no ambiguity
when more objects are added to the repository.

That these two files alone are not enough to implement a "--continue"
that remembers the command-line options specified; later patches in
the series save them too.

These new files are unrelated to the existing .git/CHERRY_PICK_HEAD,
which will still be useful while committing after a conflict
resolution.

>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> +static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
>> +                     struct replay_opts *opts)
>> +{
>> +     struct commit_list *cur = NULL;
>> +     struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>> +     const char *sha1_abbrev = NULL;
>> +     const char *action;
>> +
>> +     action = (opts->action == REVERT ? "revert" : "pick");
>> +     for (cur = todo_list; cur; cur = cur->next) {
>> +             sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
>> +             if (get_message(cur->item, &msg))
>> +                     die(_("Cannot get commit message for %s"), sha1_abbrev);
>> +             strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
>
> This die() seems odd in the context of a series that starts by
> converting various die() calls to "return error()".

Fixed.  I made format_todo return an int now.

>> +static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
>> +{
>> +     unsigned char commit_sha1[20];
>> +     char sha1_abbrev[40];
>> +     struct commit *commit;
>> +     enum replay_action action;
>> +     int insn_len = 0;
>> +     char *p;
>> +
>> +     p = start;
>> +     if (!(p = strchr(p, ' ')))
>> +             return NULL;
>
> Style: it is much, much clearer to write
>
>        p = strchr(start, ' ');
>        if (!p)
>                return NULL;
>
> In the git codebase, assignments in "if" conditionals are very rare, so
> code like the above sticks out.
>
>> +     insn_len = p - start;
>> +     if (!(p = strchr(p + 1, ' ')))
>> +             return NULL;
>> +     p += 1;
>
> Likewise.  This could say:
>
>        insn_len = p - start;
>        p = strchr(p + 1, ' ');
>        if (!p)
>                return NULL;
>        p++;

Right, got it.  Fixed.

>> +     if (!strncmp(start, "pick", insn_len))
>> +             action = CHERRY_PICK;
>> +     else if (!strncmp(start, "revert", insn_len))
>> +             action = REVERT;
>
> This code means that "p", "pi", and "pic" are accepted as
> abbreviations for "pick".   Maybe a comment would help to clarify
> that.  Are we okay with reserving these names, so "r" in a todo
> file means to "revert" and not to "reword" like it does in a
> rebase -i todo list?

That was the original intent, but I clearly hadn't thought about it
hard enough.  Changed this to accept only full words.

>> +     /*
>> +      * Verify that the action matches up with the one in
>> +      * opts; we don't support arbitrary instructions
>> +      */
>> +     if (action != opts->action)
>> +             return NULL;
>
> If I try "git cherry-pick foo..bar" and then "git revert --continue",
> what error message will I get?

fatal: Malformed instruction sheet: .git/sequencer/todo
Technically speaking, this is correct.  However, this may not be ideal
from an end-user's perspective.  Anyway, this is going to change soon
-- do you think this is worth correcting here and now?

>> +static void read_populate_todo(struct commit_list **todo_list,
>> +                     struct replay_opts *opts)
>> +{
> [...]
>> +     for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
>> +             if (!(commit = parse_insn_line(p, opts)))
>> +                     goto error;
>> +             new = xmalloc(sizeof(struct commit_list));
>> +             new->item = commit;
>> +             *next = new;
>> +             next = &new->next;
>
> commit_list_append()?

Ugh.  Fixed.

>> +static void create_seq_dir(void)
>> +{
>> +     if (file_exists(git_path(SEQ_DIR))) {
>> +             if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
>> +                     die(_("Could not remove %s"), git_path(SEQ_DIR));
>> +     } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
>> +             die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
>> +}
>
> As mentioned in [1], a local would make this more readable.  And it's
> not clear to me why one would want to delete a .git/sequencer file
> that the user carefully placed there (I don't think git itself ever
> writes such a thing).

Okay,  removed that hunk.

>> +static void save_head(const char *head)
>> +{
>> +     static struct lock_file head_lock;
>> +     struct strbuf buf = STRBUF_INIT;
>> +     int fd;
>> +
>> +     fd = hold_lock_file_for_update(&head_lock, git_path(SEQ_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."), git_path(SEQ_HEAD_FILE));
>> +     if (commit_lock_file(&head_lock) < 0)
>> +             die(_("Error wrapping up %s"), git_path(SEQ_HEAD_FILE));
>> +}
>
> A local would be helpful here, too.
>
>> +static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>> +{
>> +     static struct lock_file todo_lock;
>> +     struct strbuf buf = STRBUF_INIT;
>> +     int fd;
>> +
>> +     fd = hold_lock_file_for_update(&todo_lock, git_path(SEQ_TODO_FILE), LOCK_DIE_ON_ERROR);
>> +     format_todo(&buf, todo_list, opts);
>> +     if (write_in_full(fd, buf.buf, buf.len) < 0) {
>> +             strbuf_release(&buf);
>> +             die_errno(_("Could not write to %s."), git_path(SEQ_TODO_FILE));
>> +     }
>> +     if (commit_lock_file(&todo_lock) < 0) {
>> +             strbuf_release(&buf);
>> +             die(_("Error wrapping up %s"), git_path(SEQ_TODO_FILE));
>> +     }
>> +     strbuf_release(&buf);
>> +}
>
> Likewise here.  As mentioned before, git_path() clobbers errno, so the
> above is likely to print a wrong error message on some systems (e.g.,
> "fatal: Cannot write to .git/sequencer/todo: Success").

Fixed these.  Thanks.

>> +static int pick_commits(struct replay_opts *opts)
>> +{
>> +     struct commit_list *todo_list = NULL;
>> +     unsigned char sha1[20];
>> +     struct commit_list *cur;
>> +     int res;
>>
>>       setenv(GIT_REFLOG_ACTION, me, 0);
>>       if (opts->allow_ff)
>> @@ -580,14 +764,24 @@ static int pick_commits(struct replay_opts *opts)
>>                               opts->record_origin || opts->edit));
>>       read_and_refresh_cache(me, opts);
>>
>> -     prepare_revs(&revs, opts);
>> +     walk_revs_populate_todo(&todo_list, opts);
>> +     create_seq_dir();
>> +     if (!get_sha1("HEAD", sha1))
>> +             save_head(sha1_to_hex(sha1));
>
> What happens if the .git/sequencer dir already exists (e.g., we were
> in the middle of a multiple-cherry-pick)?  What happens if I try to
> cherry-pick onto an unborn branch, or get_sha1() fails for some other
> reason?

Oops.  I lost this somewhere along the way -- fixed now.  Thanks :)

-- Ram

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> fatal: Malformed instruction sheet: .git/sequencer/todo
> Technically speaking, this is correct.  However, this may not be ideal
> from an end-user's perspective.  Anyway, this is going to change soon
> -- do you think this is worth correcting here and now?

Yes, thanks.  (A bird in the hand and all that.)

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> fatal: Malformed instruction sheet: .git/sequencer/todo
>> Technically speaking, this is correct.  However, this may not be ideal
>> from an end-user's perspective.  Anyway, this is going to change soon
>> -- do you think this is worth correcting here and now?
>
> Yes, thanks.  (A bird in the hand and all that.)

Okay, I thought about this for a while.  Either we can:
1. Assume that there'll never be a stray "revert" somewhere in the
middle of the instruction sheet when we invoke a "cherry-pick
--continue" and viceversa (ie. the instruction sheet is not corrupt/
hand-edited).  We can return a special exit status from the parser the
moment we encounter a "revert" during a "cherry-pick" assuming that
the entire sheet is filled with "revert" instructions, and die in
process_subcommand (now changed to pick_revisions) with a beautiful
message like "You're trying to resume a previous revert using
cherry-pick --continue, and we don't currently support this".
2. We can modify the API of the parser heavily to parse action +
commit instead of just commits.  Then, we can iterate through the list
and find/ report inconsistencies in process_subcommand.  In this case,
we can say "You're trying to resume a cherry-pick, but your
instruction sheet contains some revert instructions" or "You're trying
to resume a previous revert using cherry-pick --continue, and we don't
currently support this" as appropriate (after counting).  Although
this has a very beautiful end-user effect, I don't like the idea of
returning action + commit and counting.
3. Instead of just saying "fatal: Malformed instruction sheet" like we
do now, we can put in something a little more helpful like "error:
Cannot cherry-pick during a revert" before this message.  So:

$ git cherry-pick moo
... conflict ...
# resolve conflict
$ git revert --continue
error: Cannot cherry-pick during a revert
fatal: Malformed instruction sheet

This makes it clear to the user that the instruction sheet is to
blame, and in what way.  This is simple, and makes no assumptions
about whether or not the instruction sheet was hand-edited by the
user, so I like this.

Thoughts?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
diff --git a/builtin/revert.c b/builtin/revert.c
index 0df724f..fcbf2a1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -667,8 +667,13 @@ static struct commit *parse_insn_line(char
*start, struct replay_opts *opts)
 	 * Verify that the action matches up with the one in
 	 * opts; we don't support arbitrary instructions
 	 */
-	if (action != opts->action)
+	if (action != opts->action) {
+		const char *action_str;
+		action_str = action == REVERT ? "revert" : "cherry-pick";
+		error(_("Cannot %s during a %s"), action_str,
+			action_name(opts));
 		return NULL;
+	}

 	if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
 		|| !(commit = lookup_commit_reference(commit_sha1)))

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 3. Instead of just saying "fatal: Malformed instruction sheet" like we
> do now, we can put in something a little more helpful like "error:
> Cannot cherry-pick during a revert" before this message.  So:
>
> $ git cherry-pick moo
> ... conflict ...
> # resolve conflict
> $ git revert --continue
> error: Cannot cherry-pick during a revert
> fatal: Malformed instruction sheet
>
> This makes it clear to the user that the instruction sheet is to
> blame, and in what way.

Makes sense.  Would it be possible to rephrase the message "Malformed
instruction sheet" to encompass this error?  After all, the
instruction sheet is grammatically well formed and the only mistake it
made was to do something unexpected.

Maybe

	error: cannot resume a cherry-pick with "git revert --continue"
	fatal: unusable instruction sheet

or something along those lines.  I dunno.

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Maybe
>
>        error: cannot resume a cherry-pick with "git revert --continue"
>        fatal: unusable instruction sheet

Good suggestion.  Implemented.  Thanks.

-- Ram

[PATCH 12/17] revert: Save command-line options for continuing operation

[Ramkumar Ramachandra]

In the same spirit as ".git/sequencer/head" and ".git/sequencer/todo",
introduce ".git/sequencer/opts" to persist the replay_opts structure
for continuing after a conflict resolution.  Use a simple "key =
value" format for this file, using "|" to separate multiple values so
that the file looks like:

signoff = true
record-origin = true
mainline = 1
strategy = recursive
strategy-option = patience | ours

Write the file just after writing ".git/sequencer/head", and parse it
out just after parsing out ".git/sequencer/head".  There is now enough
data for "--continue" to work.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 856ee97..edf74fb 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -596,6 +596,32 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new->next;
 }
 
+static void format_opts(struct strbuf *buf, struct replay_opts *opts)
+{
+	int i;
+
+	if (opts->no_commit)
+		strbuf_addstr(buf, "no-commit = true\n");
+	if (opts->edit)
+		strbuf_addstr(buf, "edit = true\n");
+	if (opts->signoff)
+		strbuf_addstr(buf, "signoff = true\n");
+	if (opts->record_origin)
+		strbuf_addstr(buf, "record-origin = true\n");
+	if (opts->allow_ff)
+		strbuf_addstr(buf, "ff = true\n");
+	if (opts->mainline)
+		strbuf_addf(buf, "mainline = %d\n", opts->mainline);
+	if (opts->strategy)
+		strbuf_addf(buf, "strategy = %s\n", opts->strategy);
+	if (opts->xopts) {
+		strbuf_addf(buf, "strategy-option = ");
+		for (i = 0; i < opts->xopts_nr - 1; i ++)
+			strbuf_addf(buf, "%s | ", opts->xopts[i]);
+		strbuf_addf(buf, "%s\n", opts->xopts[i]);
+	}
+}
+
 static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 			struct replay_opts *opts)
 {
@@ -694,6 +720,184 @@ error:
 	die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
 }
 
+static struct strbuf *parse_value(const char *start, char **end_ptr)
+{
+	static struct strbuf value = STRBUF_INIT;
+	int quote = 0;
+	char *p = (char *)start;
+	char *end;
+
+	/* Find and strip '\n', '\r' */
+	if ((end = strchr(start, '\n'))) {
+		*end = '\0'; /* Remove trailing '\n' */
+		if (*(end - 1) == '\r')
+			*(end - 1) = '\0'; /* Remove trailing '\r' */
+	} else if (strchr(start, '\0'))
+		end = NULL;
+	else
+		goto error;
+
+	for (; *p != '\0'; p ++) {
+		if (!quote && (*p == ';' || *p == '#'))
+			/* Ignore comments */
+			goto ok;
+		if (*p == '\\') {
+			p += 1;
+			switch (*p) {
+			case '\n':
+				continue;
+			case 't':
+				strbuf_addch(&value, '\t');
+				break;
+			case 'b':
+				strbuf_addch(&value, '\b');
+				break;
+			case 'n':
+				strbuf_addch(&value, '\n');
+				break;
+			/* Some pharapters escape as themselves */
+			case '\\': case '"':
+				break;
+			/* Reject unknown escape sequences */
+			default:
+				goto error;
+			}
+			continue;
+		}
+		if (*p == '"') {
+			quote = 1 - quote;
+			continue;
+		}
+		strbuf_addch(&value, *p);
+	}
+	/* Quote not closed */
+	if (quote)
+		goto error;
+
+	if (end)
+		*end_ptr = end + 1;
+	else
+		*end_ptr = NULL;
+ok:
+	strbuf_trim(&value);
+	return &value;
+error:
+	strbuf_release(&value);
+	return NULL;
+}
+
+static char *parse_opt_value(const char *start, void *key,
+			enum parse_opt_type type, parse_opt_cb *cb_function)
+{
+	struct strbuf *value, *subvalue;
+	struct option opt;
+	char *p, *cur, *val, *end;
+
+	/* Remove spaces before '=' */
+	for (p = (char *)start; isspace(*p); p++);
+	if (*p != '=')
+		goto error;
+
+	if (!(value = parse_value(p + 1, &end)))
+		goto error;
+	val = value->buf;
+
+	switch (type) {
+	case OPTION_BOOLEAN:
+		if (!strcasecmp(val, "true")
+			|| !strcasecmp(val, "yes")
+			|| !strcasecmp(val, "on"))
+			*(int *)key = 1;
+		else if (!strcasecmp(val, "false")
+			|| !strcasecmp(val, "no")
+			|| !strcasecmp(val, "off"))
+			*(int *)key = 0;
+		else
+			goto error;
+		break;
+	case OPTION_INTEGER:
+		*(int *)key = strtol(val, NULL, 10);
+		break;
+	case OPTION_STRING:
+		*(char **)key = xstrdup(val);
+		break;
+	case OPTION_CALLBACK:
+		opt.value = (struct replay_opts **)key;
+		while (val) {
+			if ((cur = strchr(val, '|'))) {
+				*cur = '\0';
+				subvalue = parse_value(val, &end);
+				(*cb_function)(&opt, subvalue->buf, 0);
+				strbuf_release(subvalue);
+				val = cur + 1;
+			} else {
+				(*cb_function)(&opt, val, 0);
+				break;
+			}
+		}
+		break;
+	default:
+		die(_("program error"));
+	}
+	strbuf_release(value);
+	return end;
+error:
+	die(_("Malformed options sheet: %s"), git_path(SEQ_OPTS_FILE));
+}
+
+static void read_populate_opts(struct replay_opts **opts_ptr)
+{
+	struct replay_opts *opts = *opts_ptr;
+	struct strbuf buf = STRBUF_INIT;
+	char *p;
+	int fd;
+
+	fd = open(git_path(SEQ_OPTS_FILE), O_RDONLY);
+	if (fd < 0)
+		goto ok;
+	if (strbuf_read(&buf, fd, 0) < buf.len) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), git_path(SEQ_OPTS_FILE));
+	}
+	close(fd);
+
+	for (p = buf.buf; p && *p != '\0';) {
+		if (!prefixcmp(p, "no-commit"))
+			p = parse_opt_value(p + strlen("no-commit"),
+					&opts->no_commit, OPTION_BOOLEAN, NULL);
+		else if (!prefixcmp(p, "edit"))
+			p = parse_opt_value(p + strlen("edit"),
+					&opts->edit, OPTION_BOOLEAN, NULL);
+		else if (!prefixcmp(p, "signoff"))
+			p = parse_opt_value(p + strlen("signoff"),
+					&opts->signoff, OPTION_BOOLEAN, NULL);
+		else if (!prefixcmp(p, "mainline"))
+			p = parse_opt_value(p + strlen("mainline"),
+					&opts->mainline, OPTION_INTEGER, NULL);
+		else if (!prefixcmp(p, "record-origin"))
+			p = parse_opt_value(p + strlen("record-origin"),
+					&opts->record_origin, OPTION_BOOLEAN, NULL);
+		else if (!prefixcmp(p, "ff"))
+			p = parse_opt_value(p + strlen("ff"),
+					&opts->allow_ff, OPTION_BOOLEAN, NULL);
+		else if (!prefixcmp(p, "strategy-option"))
+			p = parse_opt_value(p + strlen("strategy-option"),
+					&opts, OPTION_CALLBACK, option_parse_x);
+		else if (!prefixcmp(p, "strategy"))
+			p = parse_opt_value(p + strlen("strategy"),
+					&opts->strategy, OPTION_STRING, NULL);
+		else
+			goto error;
+	}
+ok:
+	strbuf_release(&buf);
+	return;
+error:
+	strbuf_release(&buf);
+	die(_("Malformed options sheet: %s"), git_path(SEQ_OPTS_FILE));
+}
+
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
@@ -751,6 +955,27 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	strbuf_release(&buf);
 }
 
+static void save_opts(struct replay_opts *opts)
+{
+	static struct lock_file opts_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&opts_lock, git_path(SEQ_OPTS_FILE), LOCK_DIE_ON_ERROR);
+	format_opts(&buf, opts);
+	if (!buf.len)
+		return;
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s."), git_path(SEQ_OPTS_FILE));
+	}
+	if (commit_lock_file(&opts_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s"), git_path(SEQ_OPTS_FILE));
+	}
+	strbuf_release(&buf);
+}
+
 static int pick_commits(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
@@ -768,6 +993,7 @@ static int pick_commits(struct replay_opts *opts)
 	create_seq_dir();
 	if (!get_sha1("HEAD", sha1))
 		save_head(sha1_to_hex(sha1));
+	save_opts(opts);
 	save_todo(todo_list, opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
diff --git a/sequencer.h b/sequencer.h
index 673616b..dcb51b1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -5,6 +5,7 @@
 #define SEQ_OLD_DIR	"sequencer-old"
 #define SEQ_HEAD_FILE	"sequencer/head"
 #define SEQ_TODO_FILE	"sequencer/todo"
+#define SEQ_OPTS_FILE	"sequencer/opts"
 
 /* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset --hard'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 64eaa20..100cbb1 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -32,10 +32,28 @@ test_expect_success setup '
 
 test_expect_success 'cherry-pick persists data on failure' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_must_fail git cherry-pick -s base..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
+	test_path_is_file .git/sequencer/opts &&
+	rm -rf .git/sequencer
+'
+
+test_expect_success 'cherry-pick persists opts correctly' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+	test_path_is_dir .git/sequencer &&
+	test_path_is_file .git/sequencer/head &&
+	test_path_is_file .git/sequencer/todo &&
+	test_path_is_file .git/sequencer/opts &&
+	cat >expect <<-\EOF &&
+	signoff = true
+	mainline = 1
+	strategy = recursive
+	strategy-option = patience | ours
+	EOF
+	test_cmp expect .git/sequencer/opts &&
 	rm -rf .git/sequencer
 '
 
-- 
1.7.5.GIT

Re: [PATCH 12/17] revert: Save command-line options for continuing operation

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +	die(_("Malformed options sheet: %s"), git_path(SEQ_OPTS_FILE));
> +}
> +
> +static void read_populate_opts(struct replay_opts **opts_ptr)

It would make sense to refactor config.c:git_parse_file() to be easier to
use, rename it to be more descriptive (it should be a function that takes
a struct that holds these fields

        static FILE *config_file;
        static const char *config_file_name;
        static int config_linenr;
        static int config_file_eof;

in addition to the callback "fn" and callback "data", expects the file to
be in something similar to the .ini format, parses it with the help from
the callback function), and use it here, without rolling your own.

Also I get

builtin/revert.c:682: error: 'read_populate_todo' defined but not used
builtin/revert.c:848: error: 'read_populate_opts' defined but not used

at this step in the series, which is not quite optimal.

Re: [PATCH 12/17] revert: Save command-line options for continuing operation

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> +     die(_("Malformed options sheet: %s"), git_path(SEQ_OPTS_FILE));
>> +}
>> +
>> +static void read_populate_opts(struct replay_opts **opts_ptr)
>
> It would make sense to refactor config.c:git_parse_file() to be easier to
> use, rename it to be more descriptive (it should be a function that takes
> a struct that holds these fields
>
>        static FILE *config_file;
>        static const char *config_file_name;
>        static int config_linenr;
>        static int config_file_eof;
>
> in addition to the callback "fn" and callback "data", expects the file to
> be in something similar to the .ini format, parses it with the help from
> the callback function), and use it here, without rolling your own.

Yes, and I'm working on it.  However, it'll probably take me more than
a few iterations to get the patch right- I didn't want the options
parser to be blocked by that.

> Also I get
>
> builtin/revert.c:682: error: 'read_populate_todo' defined but not used
> builtin/revert.c:848: error: 'read_populate_opts' defined but not used
>
> at this step in the series, which is not quite optimal.

Ah, yes.  I was going to ask about this:
"revert: Save data for continuing after conflict resolution"
introduces read_ and save_ counterparts for todo
"revert: Save command-line options for continuing operation"
introduces read_ and save_ counterparts for opts
The read_ counterparts of both are only used much later in "revert:
Introduce --continue to continue the operation"

Now I could introduce the read_ functions in the last patch, but that
wouldn't be very elegant -- I want to show people how to read the
todo/ opts file in the same patch where I first announce and show
people how to write them.  Does that make sense?  What do you suggest?
 Something like the GCC "unused"?

Thanks.

-- Ram

Re: [PATCH 12/17] revert: Save command-line options for continuing operation

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -596,6 +596,32 @@ struct commit_list **commit_list_append(struct commit *commit,
>  	return &new->next;
>  }
>  
> +static void format_opts(struct strbuf *buf, struct replay_opts *opts)
> +{
> +	int i;
> +
> +	if (opts->no_commit)
> +		strbuf_addstr(buf, "no-commit = true\n");
> +	if (opts->edit)
> +		strbuf_addstr(buf, "edit = true\n");
[...]

Somewhat repetitive.  Would it make sense to do something like

	add_opt_bool(buf, "no-commit", opts->no_commit);
	add_opt_bool(buf, "edit", opts->edit);
	add_opt_bool(buf, "signoff", opts->signoff);
	if (opts->mainline)
		add_opt_int(buf, "mainline", opts->mainline);
	add_opt_string(buf, "strategy", opts->strategy);
	add_opt_strings(buf, "strategy-option", opts->xopts, opts->xopts_nr);

> @@ -694,6 +720,184 @@ error:
>  	die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
>  }
>  
> +static struct strbuf *parse_value(const char *start, char **end_ptr)
> +{
> +	static struct strbuf value = STRBUF_INIT;
> +	int quote = 0;
> +	char *p = (char *)start;

Are all callers passing a modifiable buffer?  If so, why not make the
parameter non-const?  Is it necessary to modify the buffer to parse it?

> +	char *end;
> +
> +	/* Find and strip '\n', '\r' */
> +	if ((end = strchr(start, '\n'))) {

Style:

	end = strchrnul(start, '\n');
	if (*end && end > start && end[-1] == '\r')
		end--;

	for (p = start; p != end; p++) {
		...
	}

> +	for (; *p != '\0'; p ++) {
> +		if (!quote && (*p == ';' || *p == '#'))
> +			/* Ignore comments */
> +			goto ok;
> +		if (*p == '\\') {

What is this syntax?

*thinks*  Oh, this looks eerily similar to config.c::parse_value().
Shouldn't the commit message say so?  Why not factor out a function so
they can share code?

> +			p += 1;
[...]
> +			/* Some pharapters escape as themselves */

Substitution error.

> +static char *parse_opt_value(const char *start, void *key,
> +			enum parse_opt_type type, parse_opt_cb *cb_function)
> +{
> +	struct strbuf *value, *subvalue;
> +	struct option opt;
> +	char *p, *cur, *val, *end;
> +
> +	/* Remove spaces before '=' */
> +	for (p = (char *)start; isspace(*p); p++);

Same questions apply.  Stopping here.

Hope that helps,
Jonathan

Re: [PATCH 12/17] revert: Save command-line options for continuing operation

[Ramkumar Ramachandra]

Hi,

> Subject: [PATCH 12/17] revert: Save command-line options for continuing operation

I threw away my custom parser and re-used gitconfig as Jeff suggested.
 Complete rewrite.

-- Ram

[PATCH 13/17] revert: Introduce a layer of indirection over pick_commits

[Ramkumar Ramachandra]

Write a new function called process_subcommand to prepare a todo_list
to call pick_commits with; the job of pick_commits is simplified into
performing the tasks listed in todo_list.  This will be useful when
subcommands like "--reset" and "--continue" are introduced later in
the series.

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index edf74fb..f9f5e3a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -976,10 +976,8 @@ static void save_opts(struct replay_opts *opts)
 	strbuf_release(&buf);
 }
 
-static int pick_commits(struct replay_opts *opts)
+static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
-	unsigned char sha1[20];
 	struct commit_list *cur;
 	int res;
 
@@ -989,13 +987,6 @@ static int pick_commits(struct replay_opts *opts)
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	create_seq_dir();
-	if (!get_sha1("HEAD", sha1))
-		save_head(sha1_to_hex(sha1));
-	save_opts(opts);
-	save_todo(todo_list, opts);
-
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
@@ -1011,6 +1002,23 @@ static int pick_commits(struct replay_opts *opts)
 	return 0;
 }
 
+static int process_subcommand(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	read_and_refresh_cache(me, opts);
+
+	walk_revs_populate_todo(&todo_list, opts);
+	create_seq_dir();
+	if (!get_sha1("HEAD", sha1))
+		save_head(sha1_to_hex(sha1));
+	save_opts(opts);
+	save_todo(todo_list, opts);
+
+	return pick_commits(todo_list, opts);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
@@ -1023,7 +1031,13 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "revert";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&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
+	 */
+	res = process_subcommand(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -1039,7 +1053,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "cherry-pick";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_subcommand(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
-- 
1.7.5.GIT

Re: [PATCH 13/17] revert: Introduce a layer of indirection over pick_commits

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Write a new function called process_subcommand to prepare a todo_list
> to call pick_commits with; the job of pick_commits is simplified into
> performing the tasks listed in todo_list.  This will be useful when
> subcommands like "--reset" and "--continue" are introduced later in
> the series.

Could you give an example of how this function is used?  The name
"process_foo" does not mean much more to me than "do_it" --- i.e., it
does not tell what the function actually does.

Looking at this code, it does not just prepare a todo_list (so
fill_todo_list would not be a good name for it); it actually calls
pick_commits.  This is the main (only?) entry point to the cherry-pick
machinery, so it's worthwhile to give it a good name.  How about
pick_commits?

Meanwhile I would prefer to have multiple entry points to the
cherry-pick machinery, which would avoid this question altogether,
though I imagine you have some reason not to do that.

The mechanics of the patch (splitting out a separate function for
the actual "for each commit" loop) look like a good idea.

Re: [PATCH 13/17] revert: Introduce a layer of indirection over pick_commits

[Ramkumar Ramachandra]

Hi,

New commit message.  The "--reset" part might jump out, but it's a
result of the recent rebasing.

revert: Make pick_commits functionally act on a commit list

Apart from its central objective of calling into the picking
mechanism, pick_commits creates a sequencer directory, prepares a todo
list, and even acts upon the "--reset" subcommand.  This makes for a
bad API since the central worry of callers is to figure out whether or
not any conflicts were encountered during the cherry picking.  The
current API is like:

if (pick_commits(opts) < 0)
   print "Something failed, we're not sure what"

So, change pick_commits so that it's only responsible for picking
commits in a loop and reporting any errors, leaving the rest to a new
function called pick_revisions.  Consequently, the API of pick_commits
becomes much clearer:

act_on_subcommand(opts->subcommand);
todo_list = prepare_todo_list();
if (pick_commits(todo_list, opts) < 0)
   print "Error encountered while picking commits"

Now, callers can easily call-in to the cherry-picking machinery by
constructing an arbitrary todo list along with some options.

-- Ram

[PATCH 14/17] reset: Make hard reset remove the sequencer state

[Ramkumar Ramachandra]

Years of muscle memory have trained users to use "git reset --hard" to
remove away the branch state after any sort of operation.  In
retrospect, while this may not have been the best design decision, we
are stuck with it for historical reasons.  This sort of workflow is
now possible:

$ git cherry-pick foo..bar
... conflict encountered ...
$ git reset --hard # Oops, I didn't mean that
$ git cherry-pick quux..bar
... cherry-pick succeeded ...

Also, guard against accidental removal of the sequencer state by
moving ".git/sequencer" to ".git/sequencer-old" in the first "git
reset --hard" call, and only remove it completely only after the
second call.

Additionally, this patch ensures that some existing tests don't break
when features like "--reset" and "--continue" are introduced later in
the series.  Without this patch, we would expect the last cherry-pick
call in the example to fail with the complaint that an existing
cherry-pick operation is in progress.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/reset.c          |    2 ++
 t/7106-reset-sequence.sh |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100755 t/7106-reset-sequence.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..899e250 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -20,6 +20,7 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "sequencer.h"
 
 static const char * const git_reset_usage[] = {
 	"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]",
@@ -367,6 +368,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	switch (reset_type) {
 	case HARD:
+		remove_sequencer_state();
 		if (!update_ref_status && !quiet)
 			print_new_head_line(commit);
 		break;
diff --git a/t/7106-reset-sequence.sh b/t/7106-reset-sequence.sh
new file mode 100755
index 0000000..5aa8b71
--- /dev/null
+++ b/t/7106-reset-sequence.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='Test interaction of reset --hard with sequencer
+
+  + anotherpick: rewrites foo to d
+  + picked: rewrites foo to c
+  + unrelatedpick: rewrites unrelated to reallyunrelated
+  + base: rewrites foo to b
+  + initial: writes foo as a, unrelated as unrelated
+'
+
+. ./test-lib.sh
+
+pristine_detach () {
+	git checkout -f "$1^0" &&
+	git read-tree -u --reset HEAD &&
+	git clean -d -f -f -q -x
+}
+
+test_expect_success setup '
+	echo unrelated >unrelated &&
+	git add unrelated &&
+	test_commit initial foo a &&
+	test_commit base foo b &&
+	test_commit unrelatedpick unrelated reallyunrelated &&
+	test_commit picked foo c &&
+	test_commit anotherpick foo d &&
+	git config advice.detachedhead false
+
+'
+
+test_expect_success 'reset --hard cleans up sequencer state' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	test_path_is_dir .git/sequencer &&
+	git reset --hard &&
+	test_path_is_missing .git/sequencer
+'
+
+test_done
-- 
1.7.5.GIT

Re: [PATCH 14/17] reset: Make hard reset remove the sequencer state

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> Years of muscle memory have trained users to use "git reset --hard" to
> remove away the branch state after any sort of operation.  In
> retrospect, while this may not have been the best design decision, we
> are stuck with it for historical reasons.

Wait, wait!  If that was a bad design decision, we should try to find
a way to smoothly transition to a world without it, instead of
layering workarounds on top of it.

But actually I think it is good UI.  When you do "git merge", it works
like this:

	git merge <foo>; # conflicts!
	... hack hack hack ...
	# Oh, bother, let me go back to a state I know well and am
	# comfortable with.
	git reset --hard <bar>

And the same psychological effect applies in the cherry-pick case:

	git cherry-pick <foo>; # conflicts!
	... hack hack hack ...
	# Oh, bother, let me go back to a state I know well and am
	# comfortable with.
	git reset --hard <bar>

See, it's about the tool working with you.  When I abandon a merge,
I don't want to have to search through the owner's manual for the
button to get git to clear away this unpleasant and unfamiliar state.

Now, by contrast, sometimes one wants something less aggressive.
For example, to abandon a partial merge conflict resolution but
keep unrelated changes in the worktree:

	git reset --merge HEAD

Or to get out of the "mergy" state but leave the worktree alone:

	git reset

There's no porcelain or plumbing to abandon a rebase without
additional side-effects, but "git status" suggests a command for
it if I remember correctly.

	rm -fr .git/rebase-merge

[...]
> Additionally, this patch ensures that some existing tests don't break
> when features like "--reset" and "--continue" are introduced later in
> the series.

That's not "Additionally" --- it's the same problem.  One way to
phrase it would be "Noticed by running such-and-such test after
such-and-such change".

> +test_expect_success 'reset --hard cleans up sequencer state' '

Hoorah!  Thanks.

Why isn't the .git/sequencer removal in remove_branch_state() like
MERGE_MSG and other similar examples are?  (Not a rhetorical question;
it would be interesting to know.)

Re: [PATCH 14/17] reset: Make hard reset remove the sequencer state

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Years of muscle memory have trained users to use "git reset --hard" to
>> remove away the branch state after any sort of operation.  In
>> retrospect, while this may not have been the best design decision, we
>> are stuck with it for historical reasons.
>
> Wait, wait!  If that was a bad design decision, we should try to find
> a way to smoothly transition to a world without it, instead of
> layering workarounds on top of it.
>
> But actually I think it is good UI.  When you do "git merge", it works
> like this:
>
>        git merge <foo>; # conflicts!
>        ... hack hack hack ...
>        # Oh, bother, let me go back to a state I know well and am
>        # comfortable with.
>        git reset --hard <bar>
>
> And the same psychological effect applies in the cherry-pick case:
>
>        git cherry-pick <foo>; # conflicts!
>        ... hack hack hack ...
>        # Oh, bother, let me go back to a state I know well and am
>        # comfortable with.
>        git reset --hard <bar>
>
> See, it's about the tool working with you.  When I abandon a merge,
> I don't want to have to search through the owner's manual for the
> button to get git to clear away this unpleasant and unfamiliar state.

Fair enough.  It's a large hammer to solve the problem, and it's used
by many people; I use it a lot myself.

> Now, by contrast, sometimes one wants something less aggressive.
> For example, to abandon a partial merge conflict resolution but
> keep unrelated changes in the worktree:
>
>        git reset --merge HEAD
>
> Or to get out of the "mergy" state but leave the worktree alone:
>
>        git reset
>
> There's no porcelain or plumbing to abandon a rebase without
> additional side-effects, but "git status" suggests a command for
> it if I remember correctly.
>
>        rm -fr .git/rebase-merge

We're not making this mistake with the sequencer :)

>> Additionally, this patch ensures that some existing tests don't break
>> when features like "--reset" and "--continue" are introduced later in
>> the series.
>
> That's not "Additionally" --- it's the same problem.  One way to
> phrase it would be "Noticed by running such-and-such test after
> such-and-such change".

Right.  Minor tweaks made to the commit message.

reset: Make reset remove the sequencer state

Years of muscle memory have trained users to use "git reset --hard" to
remove the branch state after any sort operation.  Make it also remove
the sequencer state so that this sort of workflow is now possible:

$ git cherry-pick foo..bar
... conflict encountered ...
$ git reset --hard # Oops, I didn't mean that
$ git cherry-pick quux..bar
... cherry-pick succeeded ...

Also, guard against accidental removal of the sequencer state by
moving ".git/sequencer" to ".git/sequencer-old" in the first "git
reset --hard" call, and only remove it completely only after the
second call.

This patch ensures that some existing tests don't break when features
like "--reset" and "--continue" are introduced later in the series.
Without this patch, we would expect the last cherry-pick call in the
example to fail with the complaint that an existing cherry-pick
operation is in progress.

>> +test_expect_success 'reset --hard cleans up sequencer state' '
>
> Hoorah!  Thanks.
>
> Why isn't the .git/sequencer removal in remove_branch_state() like
> MERGE_MSG and other similar examples are?  (Not a rhetorical question;
> it would be interesting to know.)

I thought we should only make a hard reset clear it, but all the reset
codepaths reach remove_branch_state().  However, in view of
consistency, I've put the removal code in remove_branch_state() now.

-- Ram

[PATCH 15/17] revert: Remove sequencer state when no commits are pending

[Ramkumar Ramachandra]

When cherry-pick or revert is called on a list of commits, and a
conflict encountered somewhere in the middle, the data in
".git/sequencer" is required to continue the operation.  However, when
a conflict is encountered in the very last commit, the user will have
to "continue" after resolving the conflict and committing just so that
the sequencer state is removed.  This is how the current "rebase -i"
script works as well.

$ git cherry-pick foo..bar
... conflict encountered while picking "bar" ...
$ echo "resolved" >problematicfile
$ git add problematicfile
$ git commit
$ git cherry-pick --continue # This would be a noop

Change this so that the sequencer state is cleared when a conflict is
encountered in the last commit.  Incidentally, this patch makes sure
that some existing tests don't break when features like "--reset" and
"--continue" are implemented later in the series.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |   12 +++++++++++-
 t/t3510-cherry-pick-sequence.sh |   25 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index f9f5e3a..3936516 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -990,8 +990,18 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
-		if (res)
+		if (res) {
+			if (!cur->next)
+				/*
+				 * An error was encountered while
+				 * picking the last commit; the
+				 * sequencer state is useless now --
+				 * the user simply needs to resolve
+				 * the conflict and commit
+				 */
+				remove_sequencer_state();
 			return res;
+		}
 	}
 
 	/*
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 100cbb1..0a8b093 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -63,4 +63,29 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' '
 	test_path_is_missing .git/sequencer
 '
 
+test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	test_path_is_missing .git/sequencer &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/[0-9a-f]\{40\}/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	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.5.GIT

Re: [PATCH 15/17] revert: Remove sequencer state when no commits are pending

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/builtin/revert.c b/builtin/revert.c
> index f9f5e3a..3936516 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -990,8 +990,18 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>  	for (cur = todo_list; cur; cur = cur->next) {
>  		save_todo(cur, opts);
>  		res = do_pick_commit(cur->item, opts);
> -		if (res)
> +		if (res) {
> +			if (!cur->next)
> +				/*
> +				 * An error was encountered while
> +				 * picking the last commit; the
> +				 * sequencer state is useless now --
> +				 * the user simply needs to resolve
> +				 * the conflict and commit
> +				 */
> +				remove_sequencer_state();
>  			return res;
> +		}
>  	}

It may be useless for --continue, but wouldn't --abort need some clue on
what you were doing?

Re: [PATCH 15/17] revert: Remove sequencer state when no commits are pending

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wites:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index f9f5e3a..3936516 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -990,8 +990,18 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>>       for (cur = todo_list; cur; cur = cur->next) {
>>               save_todo(cur, opts);
>>               res = do_pick_commit(cur->item, opts);
>> -             if (res)
>> +             if (res) {
>> +                     if (!cur->next)
>> +                             /*
>> +                              * An error was encountered while
>> +                              * picking the last commit; the
>> +                              * sequencer state is useless now --
>> +                              * the user simply needs to resolve
>> +                              * the conflict and commit
>> +                              */
>> +                             remove_sequencer_state();
>>                       return res;
>> +             }
>>       }
>
> It may be useless for --continue, but wouldn't --abort need some clue on
> what you were doing?

Jonathan pointed this out too.  I'd like to know what you think of the
alternatives
1. Layering a hack on top: writing .git/sequencer/to-remove or
something as ugly.
2. Teaching "git commit" to remove the sequencer state instead.

Actually, an "--abort" can be taught to work even here by reading
.git/sequencer-old/head, but that's arguably ugly too.

-- Ram

[PATCH 16/17] revert: Introduce --reset to remove sequencer state

[Ramkumar Ramachandra]

Protect the sequencer state from accidentally being stomped by a new
cherry-pick or revert invocation by ensuring that an existing one
isn't in progress.  While this patch would normally be expected to
break many tests, the earlier patches "reset: Make hard reset remove
the sequencer state" and "revert: Remove sequencer state when no
commits are pending" make sure that they don't.

To explicitly remove the sequencer state for a fresh cherry-pick or
revert invocation, introduce a new subcommand called '--reset' which
really removes the sequencer state on the very first invocation.

Ensure that the "rebase -i" script which invokes cherry-pick or revert
doesn't change its behavior by using '--reset' to to clear the state
after every failed pick.  Effectively, the script will remain unaware
and unaffected by introduction of further subcommands like
'--continue'.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-cherry-pick.txt |    5 +++
 Documentation/git-revert.txt      |    5 +++
 Documentation/sequencer.txt       |    4 ++
 builtin/reset.c                   |    2 +-
 builtin/revert.c                  |   66 ++++++++++++++++++++++++++++++------
 git-rebase--interactive.sh        |   25 +++++++++++---
 sequencer.c                       |    4 ++-
 sequencer.h                       |    6 +++-
 t/t3510-cherry-pick-sequence.sh   |   17 ++++++++-
 9 files changed, 113 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/sequencer.txt

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 9d8fe0d..138a292 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -8,6 +8,7 @@ git-cherry-pick - Apply the changes introduced by some existing commits
 SYNOPSIS
 --------
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
+'git cherry-pick' --reset
 
 DESCRIPTION
 -----------
@@ -109,6 +110,10 @@ effect to your index in a row.
 	Pass the merge strategy-specific option through to the
 	merge strategy.  See linkgit:git-merge[1] for details.
 
+SEQUENCER SUBCOMMANDS
+---------------------
+include::sequencer.txt[]
+
 EXAMPLES
 --------
 git cherry-pick master::
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index ac10cfb..783ffb4 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -8,6 +8,7 @@ git-revert - Revert some existing commits
 SYNOPSIS
 --------
 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] <commit>...
+'git revert' --reset
 
 DESCRIPTION
 -----------
@@ -90,6 +91,10 @@ effect to your index in a row.
 	Pass the merge strategy-specific option through to the
 	merge strategy.  See linkgit:git-merge[1] for details.
 
+SEQUENCER SUBCOMMANDS
+---------------------
+include::sequencer.txt[]
+
 EXAMPLES
 --------
 git revert HEAD~3::
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
new file mode 100644
index 0000000..16ce88c
--- /dev/null
+++ b/Documentation/sequencer.txt
@@ -0,0 +1,4 @@
+--reset::
+	Forget about the current operation in progress.  Can be used
+	to clear the sequencer state after a failed cherry-pick or
+	revert.
diff --git a/builtin/reset.c b/builtin/reset.c
index 899e250..ec0e5d0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -368,7 +368,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	switch (reset_type) {
 	case HARD:
-		remove_sequencer_state();
+		remove_sequencer_state(0);
 		if (!update_ref_status && !quiet)
 			print_new_head_line(commit);
 		break;
diff --git a/builtin/revert.c b/builtin/revert.c
index 3936516..409d88f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -29,19 +29,23 @@
 
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
+	"git revert <subcommand>",
 	NULL
 };
 
 static const char * const cherry_pick_usage[] = {
 	"git cherry-pick [options] <commit-ish>",
+	"git cherry-pick <subcommand>",
 	NULL
 };
 
 static const char *me;
 enum replay_action { REVERT, CHERRY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET };
 
 struct replay_opts {
 	enum replay_action action;
+	enum replay_subcommand subcommand;
 
 	/* Boolean options */
 	int edit;
@@ -104,7 +108,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
+	int reset = 0;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -133,7 +139,27 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
-	if (opts->commit_argc < 2)
+
+	/* Set the subcommand */
+	if (reset)
+		opts->subcommand = REPLAY_RESET;
+	else
+		opts->subcommand = REPLAY_NONE;
+
+	/* Check for incompatible command line arguments */
+	if (opts->subcommand == REPLAY_RESET) {
+		verify_opt_compatible(me, "--reset",
+				"--no-commit", opts->no_commit,
+				"--signoff", opts->signoff,
+				"--mainline", opts->mainline,
+				"--strategy", opts->strategy ? 1 : 0,
+				"--strategy-option", opts->xopts ? 1 : 0,
+				"-x", opts->record_origin,
+				"--ff", opts->allow_ff,
+				NULL);
+	}
+
+	else if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (opts->allow_ff)
@@ -916,8 +942,11 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,
 static void create_seq_dir(void)
 {
 	if (file_exists(git_path(SEQ_DIR))) {
-		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
-			die(_("Could not remove %s"), git_path(SEQ_DIR));
+		error(_("%s already exists."), git_path(SEQ_DIR));
+		advise(_("This usually means that a %s operation is in progress."), me);
+		advise(_("Use %s --continue to continue the operation"), me);
+		advise(_("or use %s --reset to forget about it"), me);
+		die(_("%s failed"), me);
 	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
 		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
 }
@@ -999,7 +1028,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 				 * the user simply needs to resolve
 				 * the conflict and commit
 				 */
-				remove_sequencer_state();
+				remove_sequencer_state(0);
 			return res;
 		}
 	}
@@ -1008,7 +1037,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	 * Sequence of picks finished successfully; cleanup by
 	 * removing the .git/sequencer directory
 	 */
-	remove_sequencer_state();
+	remove_sequencer_state(1);
 	return 0;
 }
 
@@ -1019,13 +1048,28 @@ static int process_subcommand(struct replay_opts *opts)
 
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	create_seq_dir();
-	if (!get_sha1("HEAD", sha1))
-		save_head(sha1_to_hex(sha1));
-	save_opts(opts);
-	save_todo(todo_list, opts);
+	if (opts->subcommand == REPLAY_RESET) {
+		remove_sequencer_state(1);
+		return 0;
+	} else {
+		/*
+		 * Start a new cherry-pick/ revert sequence; but
+		 * first, make sure that an existing one isn't in
+		 * progress
+		 */
+		if (file_exists(git_path(SEQ_TODO_FILE))) {
+			error(_("A %s is already in progress"), me);
+			advise(_("Use %s --reset to forget about it"), me);
+			return -1;
+		}
 
+		walk_revs_populate_todo(&todo_list, opts);
+		create_seq_dir();
+		if (!get_sha1("HEAD", sha1))
+			save_head(sha1_to_hex(sha1));
+		save_opts(opts);
+		save_todo(todo_list, opts);
+	}
 	return pick_commits(todo_list, opts);
 }
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 65690af..14c57dd 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -136,6 +136,10 @@ make_patch () {
 		get_author_ident_from_commit "$1" > "$author_script"
 }
 
+clear_cherry_pick_state () {
+	git cherry-pick --reset
+}
+
 die_with_patch () {
 	echo "$1" > "$state_dir"/stopped-sha
 	make_patch "$1"
@@ -279,8 +283,10 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick "$@" || {
+				clear_cherry_pick_state
 				die_with_patch $sha1 "Could not pick $sha1"
+			}
 			;;
 		esac
 		;;
@@ -385,16 +391,20 @@ do_next () {
 		comment_for_reflog pick
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		record_in_rewritten $sha1
 		;;
 	reword|r)
 		comment_for_reflog reword
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		git commit --amend --no-post-rewrite
 		record_in_rewritten $sha1
 		;;
@@ -402,8 +412,10 @@ do_next () {
 		comment_for_reflog edit
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		echo "$sha1" > "$state_dir"/stopped-sha
 		make_patch $sha1
 		git rev-parse --verify HEAD > "$amend"
@@ -438,7 +450,10 @@ do_next () {
 		echo "$author_script_content" > "$author_script"
 		eval "$author_script_content"
 		output git reset --soft HEAD^
-		pick_one -n $sha1 || die_failed_squash $sha1 "$rest"
+		pick_one -n $sha1 || {
+			clear_cherry_pick_state
+			die_failed_squash $sha1 "$rest"
+		}
 		case "$(peek_next_command)" in
 		squash|s|fixup|f)
 			# This is an intermediate commit; its message will only be
diff --git a/sequencer.c b/sequencer.c
index 8c1de63..bc2c046 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3,7 +3,7 @@
 #include "strbuf.h"
 #include "dir.h"
 
-void remove_sequencer_state(void)
+void remove_sequencer_state(int aggressive)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
 	struct strbuf seq_old_dir = STRBUF_INIT;
@@ -12,6 +12,8 @@ void remove_sequencer_state(void)
 	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
 	remove_dir_recursively(&seq_old_dir, 0);
 	rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
+	if (aggressive)
+		remove_dir_recursively(&seq_old_dir, 0);
 	strbuf_release(&seq_dir);
 	strbuf_release(&seq_old_dir);
 }
diff --git a/sequencer.h b/sequencer.h
index dcb51b1..e1951cc 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -9,7 +9,11 @@
 
 /* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset --hard'.
+ *
+ * With the aggressive flag, it additionally removes SEQ_OLD_DIR,
+ * ignoring any errors.  Inteded to be used by the sequencer's
+ * '--reset' subcommand.
  */
-void remove_sequencer_state(void);
+void remove_sequencer_state(int aggressive);
 
 #endif
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 0a8b093..c21b345 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -37,7 +37,7 @@ test_expect_success 'cherry-pick persists data on failure' '
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
 	test_path_is_file .git/sequencer/opts &&
-	rm -rf .git/sequencer
+	git cherry-pick --reset
 '
 
 test_expect_success 'cherry-pick persists opts correctly' '
@@ -54,7 +54,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	strategy-option = patience | ours
 	EOF
 	test_cmp expect .git/sequencer/opts &&
-	rm -rf .git/sequencer
+	git cherry-pick --reset
 '
 
 test_expect_success 'cherry-pick cleans up sequencer state upon success' '
@@ -88,4 +88,17 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
 	test_cmp expect actual
 '
 
+test_expect_success '--reset does not complain when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	git cherry-pick --reset
+'
+
+test_expect_success '--reset cleans up sequencer state' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	git cherry-pick --reset &&
+	test_path_is_missing .git/sequencer
+'
+
 test_done
-- 
1.7.5.GIT

Re: [PATCH 16/17] revert: Introduce --reset to remove sequencer state

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Protect the sequencer state from accidentally being stomped by a new
> cherry-pick or revert invocation by ensuring that an existing one
> isn't in progress.

I first read this as "an existing sequencer state isn't in progress".
But anyway, if I understand correctly the goal isn't to protect the
sequencer state from corruption but to protect the user from forgetting
about a pending cherry-pick.

> While this patch would normally be expected to
> break many tests, the earlier patches "reset: Make hard reset remove
> the sequencer state" and "revert: Remove sequencer state when no
> commits are pending" make sure that they don't.

Why would I expect a nice change to break tests?

I suppose you mean: "A naive version of this would break the following
established way of working:

	git cherry-pick X; # has conflicts
	git reset --hard; # no, no!
	git cherry-pick Y

Or even:

	git cherry-pick X; # has conflicts
	# ... resolve ...
	git commit
	git cherry-pick Y

But a previous patch takes care of that by making "git reset --hard"
cancel the pending cherry-pick and by making "git commit" clean up
after a pending cherry-pick when making the commit that would finish
it."

The above text quoted with ">" describes the main impact of the
change.  The subject line, on the other hand, describes a less
important part:

> [Subject: revert: Introduce --reset to remove sequencer state]

Is that fixable?  Maybe this could be split into two patches (since it
does two different things), or maybe the subject line could be tweaked
to describe both.

[...]
> Ensure that the "rebase -i" script which invokes cherry-pick or revert
> doesn't change its behavior by using '--reset' to to clear the state
> after every failed pick.

This should be avoidable by noticing that commands like "rebase -i"
use GIT_CHERRY_PICK_HELP to clobber our nice instructions about how to
resume a cherry-pick anyway and therefore are unlikely to need
cherry-pick --continue/--abort facilities, no?

Regards,
Jonathan

Re: [PATCH 16/17] revert: Introduce --reset to remove sequencer state

[Ramkumar Ramachandra]

Hi,

On Wed, Jul 13, 2011 at 2:00 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> Protect the sequencer state from accidentally being stomped by a new
>> cherry-pick or revert invocation by ensuring that an existing one
>> isn't in progress.
>
> I first read this as "an existing sequencer state isn't in progress".
> But anyway, if I understand correctly the goal isn't to protect the
> sequencer state from corruption but to protect the user from forgetting
> about a pending cherry-pick.
>
>> While this patch would normally be expected to
>> break many tests, the earlier patches "reset: Make hard reset remove
>> the sequencer state" and "revert: Remove sequencer state when no
>> commits are pending" make sure that they don't.
>
> Why would I expect a nice change to break tests?
>
> I suppose you mean: "A naive version of this would break the following
> established way of working:
>
>        git cherry-pick X; # has conflicts
>        git reset --hard; # no, no!
>        git cherry-pick Y
>
> Or even:
>
>        git cherry-pick X; # has conflicts
>        # ... resolve ...
>        git commit
>        git cherry-pick Y
>
> But a previous patch takes care of that by making "git reset --hard"
> cancel the pending cherry-pick and by making "git commit" clean up
> after a pending cherry-pick when making the commit that would finish
> it."
>
> The above text quoted with ">" describes the main impact of the
> change.  The subject line, on the other hand, describes a less
> important part:
>
>> [Subject: revert: Introduce --reset to remove sequencer state]
>
> Is that fixable?  Maybe this could be split into two patches (since it
> does two different things), or maybe the subject line could be tweaked
> to describe both.

New commit message.

revert: Don't clobber sequencer state; introduce --reset

Protect the user from forgetting about a pending operation by
immediately erroring out when an existing cherry-pick or revert
operation is in progress.  A naive version of this would break the
following established ways of working:

$ git cherry-pick foo
... conflict ...
$ git reset --hard  # I actually meant "moo" when I said "foo"
$ git cherry-pick moo

$ git cherry-pick foo
... conflict ...
# resolve the conflict
$ git commit # commit the resolution
$ git cherry-pick moo # New operation

But the previous patches "reset: Make hard reset remove the sequencer
state" and "revert: Remove sequencer state when no commits are
pending" make sure that this does not happen.

To explicitly remove the sequencer state for a fresh cherry-pick or
revert invocation, introduce a new subcommand called '--reset' which
really removes the sequencer state on the very first invocation.

>> Ensure that the "rebase -i" script which invokes cherry-pick or revert
>> doesn't change its behavior by using '--reset' to to clear the state
>> after every failed pick.
>
> This should be avoidable by noticing that commands like "rebase -i"
> use GIT_CHERRY_PICK_HELP to clobber our nice instructions about how to
> resume a cherry-pick anyway and therefore are unlikely to need
> cherry-pick --continue/--abort facilities, no?

Right.  Removed hunk and corresponding segment in commit message.

Thanks.

-- Ram

Re: [PATCH 16/17] revert: Introduce --reset to remove sequencer state

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> New commit message.

Much clearer.  Thank you.

[PATCH 17/17] revert: Introduce --continue to continue the operation

[Ramkumar Ramachandra]

Using the information in ".git/sequencer", it is now possible to
continue a cherry-pick or continue after a conflict.  To do this, we
have to parse the information in ".git/sequencer/opts" into the
replay_opts structure and build a commit_list from the information in
".git/sequencer/todo" before calling into the cherry-picking
machinery.  Introduce a new subcommand called '--continue' to
facilitate this.  It is imperative to note that this subcommand will
blindly drop the first instruction and attempt to perform the rest of
the instructions in ".git/sequencer/todo" to affect the current HEAD
provided that the index matches HEAD.  The resulting end-user workflow
is as follows:

$ git cherry-pick foo..bar
... conflict in commit moo ...
$ git cherry-pick --continue
error: 'cherry-pick' is not possible because you have unmerged files.
...
$ echo "resolved" >conflictingfile
$ git commit # this is a replacement for "moo"
$ git cherry-pick --continue # drops "moo" and replays everything else

During the "git commit" stage, CHERRY_PICK_HEAD will aid by providing
the commit message from the conflicting "moo" commit.  Note that the
cherry-pick mechanism has no control at this stage, so the user is
free to violate anything that was specified during the first
cherry-pick invocation.  For example, if "-x" was specified during the
first cherry-pick invocation, the user is free to edit out the message
during commit time.  One glitch to note is that the "--signoff" option
specified at cherry-pick invocation time is not reflected in the
commit message provided by CHERRY_PICK_HEAD; the user must take care
to add "--signoff" during the "git commit" invocation.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-cherry-pick.txt |    1 +
 Documentation/git-revert.txt      |    1 +
 Documentation/sequencer.txt       |    5 ++
 builtin/revert.c                  |   60 ++++++++++++++++++++++--
 t/t3510-cherry-pick-sequence.sh   |   93 +++++++++++++++++++++++++++++++++++++
 5 files changed, 156 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 138a292..663186b 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
 'git cherry-pick' --reset
+'git cherry-pick' --continue
 
 DESCRIPTION
 -----------
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 783ffb4..9be2fe2 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] <commit>...
 'git revert' --reset
+'git revert' --continue
 
 DESCRIPTION
 -----------
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 16ce88c..3e6df33 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -2,3 +2,8 @@
 	Forget about the current operation in progress.  Can be used
 	to clear the sequencer state after a failed cherry-pick or
 	revert.
+
+--continue::
+	Continue the operation in progress using the information in
+	'.git/sequencer'.  Can be used to continue after resolving
+	conflicts in a failed cherry-pick or revert.
diff --git a/builtin/revert.c b/builtin/revert.c
index 409d88f..f974ee0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -41,7 +41,7 @@ static const char * const cherry_pick_usage[] = {
 
 static const char *me;
 enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
 
 struct replay_opts {
 	enum replay_action action;
@@ -104,13 +104,39 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 	va_end(ap);
 }
 
+static void verify_opt_mutually_compatible(const char *me, ...)
+{
+       const char *opt1, *opt2;
+       va_list ap;
+       int set;
+
+       va_start(ap, me);
+       while ((opt1 = va_arg(ap, const char *))) {
+	       set = va_arg(ap, int);
+	       if (set)
+		       break;
+       }
+       if (!opt1)
+	       goto ok;
+       while ((opt2 = va_arg(ap, const char *))) {
+	       set = va_arg(ap, int);
+	       if (set)
+		       die(_("%s: %s cannot be used with %s"),
+			       me, opt1, opt2);
+       }
+ok:
+       va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	int reset = 0;
+	int contin = 0;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
+		OPT_BOOLEAN(0, "continue", &contin, "continue the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -140,15 +166,29 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
 
+	/* Check for incompatible subcommands */
+	verify_opt_mutually_compatible(me,
+				"--reset", reset,
+				"--continue", contin,
+				NULL);
+
 	/* Set the subcommand */
 	if (reset)
 		opts->subcommand = REPLAY_RESET;
+	else if (contin)
+		opts->subcommand = REPLAY_CONTINUE;
 	else
 		opts->subcommand = REPLAY_NONE;
 
 	/* Check for incompatible command line arguments */
-	if (opts->subcommand == REPLAY_RESET) {
-		verify_opt_compatible(me, "--reset",
+	if (opts->subcommand != REPLAY_NONE) {
+		char *this_operation;
+		if (opts->subcommand == REPLAY_RESET)
+			this_operation = "--reset";
+		else
+			this_operation = "--continue";
+
+		verify_opt_compatible(me, this_operation,
 				"--no-commit", opts->no_commit,
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
@@ -1051,6 +1091,15 @@ static int process_subcommand(struct replay_opts *opts)
 	if (opts->subcommand == REPLAY_RESET) {
 		remove_sequencer_state(1);
 		return 0;
+	} else if (opts->subcommand == REPLAY_CONTINUE) {
+		if (!file_exists(git_path(SEQ_TODO_FILE)))
+			goto error;
+		read_populate_opts(&opts);
+		read_populate_todo(&todo_list, opts);
+
+		/* Verify that the conflict has been resolved */
+		if (!index_differs_from("HEAD", 0))
+			todo_list = todo_list->next;
 	} else {
 		/*
 		 * Start a new cherry-pick/ revert sequence; but
@@ -1059,7 +1108,8 @@ static int process_subcommand(struct replay_opts *opts)
 		 */
 		if (file_exists(git_path(SEQ_TODO_FILE))) {
 			error(_("A %s is already in progress"), me);
-			advise(_("Use %s --reset to forget about it"), me);
+			advise(_("Use %s --continue to continue the operation"), me);
+			advise(_("or use %s --reset to forget about it"), me);
 			return -1;
 		}
 
@@ -1071,6 +1121,8 @@ static int process_subcommand(struct replay_opts *opts)
 		save_todo(todo_list, opts);
 	}
 	return pick_commits(todo_list, opts);
+error:
+	return error(_("No %s in progress"), me);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c21b345..635a45e 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -101,4 +101,97 @@ test_expect_success '--reset cleans up sequencer state' '
 	test_path_is_missing .git/sequencer
 '
 
+test_expect_success '--continue complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success '--continue complains when there are unresolved conflicts' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	test_must_fail git cherry-pick --continue &&
+	git cherry-pick --reset
+'
+
+test_expect_success '--continue continues after conflicts are resolved' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/[0-9a-f]\{40\}/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	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_expect_success '--continue respects opts' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick -s -x base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit -s &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	git cat-file commit HEAD >anotherpick_msg &&
+	git cat-file commit HEAD~1 >picked_msg &&
+	git cat-file commit HEAD~2 >unrelatedpick_msg &&
+	git cat-file commit HEAD~3 >initial_msg &&
+	test_must_fail test_i18ngrep "Signed-off-by:" initial_msg &&
+	test_i18ngrep "Signed-off-by:" unrelatedpick_msg &&
+	test_i18ngrep "Signed-off-by:" picked_msg &&
+	test_i18ngrep "Signed-off-by:" anotherpick_msg &&
+	test_must_fail test_i18ngrep "cherry picked from" initial_msg &&
+	test_i18ngrep "cherry picked from" unrelatedpick_msg &&
+	test_i18ngrep "cherry picked from" picked_msg &&
+	test_i18ngrep "cherry picked from" anotherpick_msg
+'
+
+test_expect_success 'malformed instruction sheet 1' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick /pick/" .git/sequencer/todo >new_sheet
+	cp new_sheet .git/sequencer/todo
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	git cherry-pick --reset &&
+	test_i18ngrep "fatal" actual
+'
+
+test_expect_success 'malformed instruction sheet 2' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick/revert/" .git/sequencer/todo >new_sheet
+	cp new_sheet .git/sequencer/todo
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	git cherry-pick --reset &&
+	test_i18ngrep "fatal" actual
+'
+
 test_done
-- 
1.7.5.GIT

Re: [PATCH 17/17] revert: Introduce --continue to continue the operation

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Using the information in ".git/sequencer", it is now possible to
> continue a cherry-pick or continue after a conflict.  To do this, we
> have to parse the information in ".git/sequencer/opts" into the
> replay_opts structure and
[...]

Might be simpler to say:

	Introduce a new "git cherry-pick --continue" command which uses
	the information in ".git/sequencer" to continue a cherry-pick that
	stopped because of a conflict or other error.  It works by dropping
	the first instruction from .git/sequencer/todo and performing the
	remaining cherry-picks listed there, with options (think "-s" and
	"-X") from the initial command listed in .git/sequencer/opts.

	So now you can do:

		$ git cherry-pick -Xpatience foo..bar
		... description conflict in commit moo ...
		$ git cherry-pick --continue
		error: 'cherry-pick' is not possible because you have unmerged files.
		fatal: failed to resume cherry-pick
		$ echo resolved >conflictingfile
		$ git add conflictingfile && git commit
		$ git cherry-pick --continue; # resumes with the commit after "moo"

> During the "git commit" stage, CHERRY_PICK_HEAD will aid by providing
> the commit message from the conflicting "moo" commit.  Note that the
> cherry-pick mechanism has no control at this stage, so the user is
> free to violate anything that was specified during the first
> cherry-pick invocation.  For example, if "-x" was specified during the
> first cherry-pick invocation, the user is free to edit out the message
> during commit time.  One glitch to note is that the "--signoff" option
> specified at cherry-pick invocation time is not reflected in the
> commit message provided by CHERRY_PICK_HEAD; the user must take care
> to add "--signoff" during the "git commit" invocation.

The -s thing doesn't have much to do with this change.  But is it a
bug or not?  If it's not a bug, then this is not so much a glitch to
note as an important feature to ensure people don't sign off on a
conflict resolution without thinking about it.  (I guess I think it's
a bug.  It's hard to decide.)

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -101,4 +101,97 @@ test_expect_success '--reset cleans up sequencer state' '
[...]
> +test_expect_success '--continue complains when no cherry-pick is in progress' '
> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick --continue >actual 2>&1 &&
> +	test_i18ngrep "error" actual

This would start to fail if the message ever changed from "error" to
"fatal", right?  I don't think that's a good thing.

> +test_expect_success '--continue complains when there are unresolved conflicts' '
> +	pristine_detach initial &&
> +	head=$(git rev-parse HEAD) &&
> +	test_must_fail git cherry-pick base..picked &&
> +	test_must_fail git cherry-pick --continue &&
> +	git cherry-pick --reset
> +'
> +
> +test_expect_success '--continue continues after conflicts are resolved' '
> +	pristine_detach initial &&
> +	head=$(git rev-parse HEAD) &&

How is $head used?

> +	test_must_fail git cherry-pick base..anotherpick &&
> +	echo "c" >foo &&
> +	git add foo &&
> +	git commit &&
> +	git cherry-pick --continue &&
> +	test_path_is_missing .git/sequencer &&
> +	{
> +		git rev-list HEAD |
> +		git diff-tree --root --stdin |
> +		sed "s/[0-9a-f]\{40\}/OBJID/g"
> +	} >actual &&

$_x40 is idiomatic and safer with old seds.

> +test_expect_success '--continue respects opts' '
> +	pristine_detach initial &&
> +	head=$(git rev-parse HEAD) &&
> +	test_must_fail git cherry-pick -s -x base..anotherpick &&
> +	echo "c" >foo &&
> +	git add foo &&
> +	git commit -s &&
> +	git cherry-pick --continue &&
> +	test_path_is_missing .git/sequencer &&
> +	git cat-file commit HEAD >anotherpick_msg &&
> +	git cat-file commit HEAD~1 >picked_msg &&
> +	git cat-file commit HEAD~2 >unrelatedpick_msg &&
> +	git cat-file commit HEAD~3 >initial_msg &&
> +	test_must_fail test_i18ngrep "Signed-off-by:" initial_msg &&

This will break when GETTEXT_POISON is set --- test_i18ngrep
automatically succeeds in that case.

Is "Signed-off-by" meant to be translated anyway?  I would use

	! grep

if testing that.

By the way, that probably should go in a separate test assertion
("-s is not automatically propagated to resolved conflict") to make
it easier to change the behavior later.

Re: [PATCH 17/17] revert: Introduce --continue to continue the operation

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Using the information in ".git/sequencer", it is now possible to
>> continue a cherry-pick or continue after a conflict.  To do this, we
>> have to parse the information in ".git/sequencer/opts" into the
>> replay_opts structure and
> [...]
>
> Might be simpler to say:
>
>        Introduce a new "git cherry-pick --continue" command which uses
>        the information in ".git/sequencer" to continue a cherry-pick that
>        stopped because of a conflict or other error.  It works by dropping
>        the first instruction from .git/sequencer/todo and performing the
>        remaining cherry-picks listed there, with options (think "-s" and
>        "-X") from the initial command listed in .git/sequencer/opts.
>
>        So now you can do:
>
>                $ git cherry-pick -Xpatience foo..bar
>                ... description conflict in commit moo ...
>                $ git cherry-pick --continue
>                error: 'cherry-pick' is not possible because you have unmerged files.
>                fatal: failed to resume cherry-pick
>                $ echo resolved >conflictingfile
>                $ git add conflictingfile && git commit
>                $ git cherry-pick --continue; # resumes with the commit after "moo"

Yep, thanks for the pretty commit message :)

>> During the "git commit" stage, CHERRY_PICK_HEAD will aid by providing
>> the commit message from the conflicting "moo" commit.  Note that the
>> cherry-pick mechanism has no control at this stage, so the user is
>> free to violate anything that was specified during the first
>> cherry-pick invocation.  For example, if "-x" was specified during the
>> first cherry-pick invocation, the user is free to edit out the message
>> during commit time.  One glitch to note is that the "--signoff" option
>> specified at cherry-pick invocation time is not reflected in the
>> commit message provided by CHERRY_PICK_HEAD; the user must take care
>> to add "--signoff" during the "git commit" invocation.
>
> The -s thing doesn't have much to do with this change.  But is it a
> bug or not?  If it's not a bug, then this is not so much a glitch to
> note as an important feature to ensure people don't sign off on a
> conflict resolution without thinking about it.  (I guess I think it's
> a bug.  It's hard to decide.)

Bug, definitely.  It happens because unlike "-x" where the
cherry-picking machinery appends to the commit message, "-s" is
handled at commit-time (when it spawns `git commit`).  Ofcourse, if I
were never to write the sequencing features, this would never been
seen as a bug -- hence the term "glitch"; an implementation detail
that doesn't suit our future plans (namely, this series) very well.

>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -101,4 +101,97 @@ test_expect_success '--reset cleans up sequencer state' '
> [...]
>> +test_expect_success '--continue complains when no cherry-pick is in progress' '
>> +     pristine_detach initial &&
>> +     test_must_fail git cherry-pick --continue >actual 2>&1 &&
>> +     test_i18ngrep "error" actual
>
> This would start to fail if the message ever changed from "error" to
> "fatal", right?  I don't think that's a good thing.

Okay, removed this and all similar checks.

>> +test_expect_success '--continue complains when there are unresolved conflicts' '
>> +     pristine_detach initial &&
>> +     head=$(git rev-parse HEAD) &&
>> +     test_must_fail git cherry-pick base..picked &&
>> +     test_must_fail git cherry-pick --continue &&
>> +     git cherry-pick --reset
>> +'
>> +
>> +test_expect_success '--continue continues after conflicts are resolved' '
>> +     pristine_detach initial &&
>> +     head=$(git rev-parse HEAD) &&
>
> How is $head used?

Stray line -- copy-pasting from an older version on my own tests.
Removed everywhere.

>> +     test_must_fail git cherry-pick base..anotherpick &&
>> +     echo "c" >foo &&
>> +     git add foo &&
>> +     git commit &&
>> +     git cherry-pick --continue &&
>> +     test_path_is_missing .git/sequencer &&
>> +     {
>> +             git rev-list HEAD |
>> +             git diff-tree --root --stdin |
>> +             sed "s/[0-9a-f]\{40\}/OBJID/g"
>> +     } >actual &&
>
> $_x40 is idiomatic and safer with old seds.

I see.  I actually thought '[0-9a-f]\{40\}' would be more traditional,
and that's why I picked in the first place :p

>> +test_expect_success '--continue respects opts' '
>> +     pristine_detach initial &&
>> +     head=$(git rev-parse HEAD) &&
>> +     test_must_fail git cherry-pick -s -x base..anotherpick &&
>> +     echo "c" >foo &&
>> +     git add foo &&
>> +     git commit -s &&
>> +     git cherry-pick --continue &&
>> +     test_path_is_missing .git/sequencer &&
>> +     git cat-file commit HEAD >anotherpick_msg &&
>> +     git cat-file commit HEAD~1 >picked_msg &&
>> +     git cat-file commit HEAD~2 >unrelatedpick_msg &&
>> +     git cat-file commit HEAD~3 >initial_msg &&
>> +     test_must_fail test_i18ngrep "Signed-off-by:" initial_msg &&
>
> This will break when GETTEXT_POISON is set --- test_i18ngrep
> automatically succeeds in that case.
>
> Is "Signed-off-by" meant to be translated anyway?  I would use
>
>        ! grep
>
> if testing that.

My bad.  Replaced with grep.  Thanks.

> By the way, that probably should go in a separate test assertion
> ("-s is not automatically propagated to resolved conflict") to make
> it easier to change the behavior later.

Separated into another test.  Thanks.

-- Ram

Re: [PATCH 17/17] revert: Introduce --continue to continue the operation

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> The -s thing doesn't have much to do with this change.  But is it a
>> bug or not?  If it's not a bug, then this is not so much a glitch to
>> note as an important feature to ensure people don't sign off on a
>> conflict resolution without thinking about it.  (I guess I think it's
>> a bug.  It's hard to decide.)
>
> Bug, definitely.  It happens because unlike "-x" where the
> cherry-picking machinery appends to the commit message, "-s" is
> handled at commit-time (when it spawns `git commit`).  Ofcourse, if I
> were never to write the sequencing features, this would never been
> seen as a bug -- hence the term "glitch"; an implementation detail
> that doesn't suit our future plans (namely, this series) very well.

Hmm, I thought I remembered this coming up previously and Junio
mentioning that it was intentional (maybe in the context of
CHERRY_HEAD?), but I haven't been able to find the relevant message.

Re: [PATCH 17/17] revert: Introduce --continue to continue the operation

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder writes:
>>> The -s thing doesn't have much to do with this change.  But is it a
>>> bug or not?  If it's not a bug, then this is not so much a glitch to
>>> note as an important feature to ensure people don't sign off on a
>>> conflict resolution without thinking about it.  (I guess I think it's
>>> a bug.  It's hard to decide.)
>>
>> Bug, definitely.  It happens because unlike "-x" where the
>> cherry-picking machinery appends to the commit message, "-s" is
>> handled at commit-time (when it spawns `git commit`).  Ofcourse, if I
>> were never to write the sequencing features, this would never been
>> seen as a bug -- hence the term "glitch"; an implementation detail
>> that doesn't suit our future plans (namely, this series) very well.
>
> Hmm, I thought I remembered this coming up previously and Junio
> mentioning that it was intentional (maybe in the context of
> CHERRY_HEAD?), but I haven't been able to find the relevant message.

Hm, not automatically signing off on a commit resolution.  I'm not
sure I understand why.  What does it mean to signoff? It proof of
origin, more than anything else -- it means that you didn't pick up
the code from some random place, right? Why would you want to signoff
on commits you're picking from another branch? A topic branch that you
want to merge into 'master' -- all the commits in the topic branch are
yours (or you atleast know where the code came from), but there are
probably commits from other developers in 'master'.  So what happens
when you resolve a conflict?
1. You modify some of your own code to fit into the code in 'master'.
Auto-signoff is alright here.
2. You modify someone else's code to accommodate your code in
'master'.  Here, your commit contains hunks which either modify or
remove someone else's code, but how is this sort of thing different
from a normal patch? Intent is different- I would've never made that
change unless I tried accommodating this topic branch in 'master', but
it has no effect on the origin of the code.  Are you saying that
auto-signoff is not alright here? Why?

Wait.  There's another explanation.  You resolved the conflict and got
everything into 'master' successfully, but some bugs crept into the
program (that aren't present in the topic branch) because you resolved
the conflict in a hurry.  Still, what does all this have to do with
certificate of origin?

Thanks.

-- Ram

Re: [PATCH 17/17] revert: Introduce --continue to continue the operation

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> Hmm, I thought I remembered this coming up previously and Junio
>> mentioning that it was intentional (maybe in the context of
>> CHERRY_HEAD?), but I haven't been able to find the relevant message.
>
> Hm, not automatically signing off on a commit resolution.  I'm not
> sure I understand why.

Well, "intentional" doesn't mean "good behavior".  I've already said I
don't like it. :)  If you remind me, I'll write a patch to make the
sign-off from cherry-pick -s persist.

[...]
> Wait.

Thanks for a nice explanation.  The detail that seems to be missing is
that the meaning of --signoff is intended to be per-project.  In some
projects, it might represent "I reviewed this" or something else
rather than the developer's certificate of origin.

Re: [GSoC update] Sequencer for inclusion

[Jonathan Nieder]

Hi Ram!

Ramkumar Ramachandra wrote:

> I'm excited to announce the first iteration of a fresh series

The basic questions:

 - has the result of applying each patch in the new iteration been
   tested (for example by rebasing interactively with "exec make test"
   after each "pick" line)?

 - what changed since last time, for each patch?  (For the future, the
   space under the "---" is generally a good place to put that
   information.)

 - what proposed changes did not make it in, for each patch?  If any,
   did they not fit well with the design, or was it more a matter of
   "sure, that would be nice, but let's get this in first"?

[...]
> I would have liked to reuse the gitconfig parser as-is for the opts
> parsing, but it's too tangled up in config.c.  I think it's safe to
> say that the opts file format deviates only slightly from the
> gitconfig format, and I'm quite happy with the end result.

To be precise, the format used includes

	strategy-option = patience | renormalize

to represent the effect of "-Xpatience -Xrenormalize".  My only worry
about that is that the "|" can sound like "or", which would seem
strange to a user that does not necessarily develop software (so is
not thinking about bitfields).  The format used in config files puts

	strategy-option = patience
	strategy-option = renormalize

as separate lines.  

> 4. New tests and documentation.  There's really no end to this

Once each new feature has been documented and each new feature or
fixed bug has an associated test, you've reached the end of this.

Meanwhile, it's true that it's possible to improve tests and
documentation beyond that, but that would not fit well in the context
of this series anyway.

[...]
> The series is becoming large and unmanagable --
> we can fix minor issues after the merge.
[...]
>   advice: Introduce error_resolve_conflict
>   revert: Inline add_message_to_msg function
>   revert: Don't check lone argument in get_encoding
>   revert: Rename no_replay to record_origin
>   revert: Propogate errors upwards from do_pick_commit
>   revert: Eliminate global "commit" variable
>   revert: Introduce struct to keep command-line options
>   revert: Separate cmdline parsing from functional code
>   revert: Don't create invalid replay_opts in parse_args
>   sequencer: Announce sequencer state location
>   revert: Save data for continuing after conflict resolution
>   revert: Save command-line options for continuing operation
>   revert: Introduce a layer of indirection over pick_commits
>   reset: Make hard reset remove the sequencer state
>   revert: Remove sequencer state when no commits are pending
>   revert: Introduce --reset to remove sequencer state
>   revert: Introduce --continue to continue the operation

My main worry is still the commit messages.  They don't need to be
elaborate but they should explain the purpose and effect of each
patch.  Part of the reason I care is that it makes the life of future
readers using "git log -S" or "git bisect" before changing that code
much easier.  Think of them like a special kind of comments that don't
interfere with reading the code straight through.

The main reason I care is that that information makes the code itself
easier to review.

I don't know how to move forward on that.  I can explain what's
missing in each message, but I get the impression that you already
understand that and there's something else (e.g., time) preventing
them from getting fixed.  I could rewrite each commit message, but I
am likely to miss things and come up with something that sounds
vaguely plausible but doesn't accurately explain the intent.  What do
you suggest?

Jonathan

Re: [GSoC update] Sequencer for inclusion

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> I'm excited to announce the first iteration of a fresh series
>  - has the result of applying each patch in the new iteration been
>   tested (for example by rebasing interactively with "exec make test"
>   after each "pick" line)?

Tests are still running.  I'll let you know the results in the morning.

>  - what changed since last time, for each patch?  (For the future, the
>   space under the "---" is generally a good place to put that
>   information.)

Will comment a little more on this in the morning.  The main focus of
this series is to showcase the new option parser, and show how it fits
into the rest of the series.

>  - what proposed changes did not make it in, for each patch?  If any,
>   did they not fit well with the design, or was it more a matter of
>   "sure, that would be nice, but let's get this in first"?

A combination of both.  More on this later.

>> I would have liked to reuse the gitconfig parser as-is for the opts
>> parsing, but it's too tangled up in config.c.  I think it's safe to
>> say that the opts file format deviates only slightly from the
>> gitconfig format, and I'm quite happy with the end result.
>
> To be precise, the format used includes
>
>        strategy-option = patience | renormalize
>
> to represent the effect of "-Xpatience -Xrenormalize".  My only worry
> about that is that the "|" can sound like "or", which would seem
> strange to a user that does not necessarily develop software (so is
> not thinking about bitfields).  The format used in config files puts
>
>        strategy-option = patience
>        strategy-option = renormalize
>
> as separate lines.

Okay, I can change to that if it's desirable.  My rationale for using
"|" is that lines like "key = value1" and "key = value2" tend to look
odd -- it's like I'm reassigning the key a different value.

>> 4. New tests and documentation.  There's really no end to this
>
> Once each new feature has been documented and each new feature or
> fixed bug has an associated test, you've reached the end of this.

It depends on how rigorously you want to document and test things, no?
 For example, I haven't documented the formats of the configuration
files anywhere but in the commit messages.  Something in
Documentation/technical would be nice, but I think we should wait
until the format evolves a bit.  Since I haven't exposed anything like
a "--interactive" functionality, the user will never see it and we can
change it as and when we like.

Also, for things like the option parser, how far do you want to go
with testing? How many kinds of malformed instruction sheets do you
want to test with?  I'll include some more basic tests soon, but I
don't think we should go too deep, due to time constraints.

As you can see, I've included basic usage documentation.  Do let me
know what else is required right now.

> My main worry is still the commit messages.  They don't need to be
> elaborate but they should explain the purpose and effect of each
> patch.  Part of the reason I care is that it makes the life of future
> readers using "git log -S" or "git bisect" before changing that code
> much easier.  Think of them like a special kind of comments that don't
> interfere with reading the code straight through.
>
> The main reason I care is that that information makes the code itself
> easier to review.

I completely agree.  We should not compromise on commit messages --
they can't be fixed later.

> I don't know how to move forward on that.  I can explain what's
> missing in each message, but I get the impression that you already
> understand that and there's something else (e.g., time) preventing
> them from getting fixed.  I could rewrite each commit message, but I
> am likely to miss things and come up with something that sounds
> vaguely plausible but doesn't accurately explain the intent.  What do
> you suggest?

I have updated many of the commit messages.  Do let me know what's
missing where.  As usual, you can leave quick comments on IRC -- I'll
check the logs in the morning.

Thanks!

-- Ram

Re: [GSoC update] Sequencer for inclusion

[Ramkumar Ramachandra]

Hi again,

Ramkumar Ramachandra writes:
> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:
>>> I'm excited to announce the first iteration of a fresh series
>>  - has the result of applying each patch in the new iteration been
>>   tested (for example by rebasing interactively with "exec make test"
>>   after each "pick" line)?
>
> Tests are still running.  I'll let you know the results in the morning.

Quick update: Yes! All of them pass all tests :)

-- Ram

Re: [GSoC update] Sequencer for inclusion

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> To be precise, the format used includes
>>
>>        strategy-option = patience | renormalize
>>
>> to represent the effect of "-Xpatience -Xrenormalize".  My only worry
>> about that is that the "|" can sound like "or", which would seem
>> strange to a user that does not necessarily develop software (so is
>> not thinking about bitfields).  The format used in config files puts
>>
>>        strategy-option = patience
>>        strategy-option = renormalize
>>
>> as separate lines.
>
> Okay, I can change to that if it's desirable.  My rationale for using
> "|" is that lines like "key = value1" and "key = value2" tend to look
> odd -- it's like I'm reassigning the key a different value.

On second thought, I don't think it matters, since this is not meant
for humans anyway, right?

I.e., it could be

	gibberish=patience renormalize

and that would work just as well.  Feel free to forget I said anything.

>> Once each new feature has been documented and each new feature or
>> fixed bug has an associated test, you've reached the end of this.
>
> It depends on how rigorously you want to document and test things, no?
>  For example, I haven't documented the formats of the configuration
> files anywhere but in the commit messages.  Something in
> Documentation/technical would be nice, but I think we should wait
> until the format evolves a bit.  Since I haven't exposed anything like
> a "--interactive" functionality, the user will never see it and we can
> change it as and when we like.

Right, I haven't looked through carefully but this didn't look
underdocumented.

I mostly meant about tests:

> Also, for things like the option parser, how far do you want to go
> with testing? How many kinds of malformed instruction sheets do you
> want to test with?  I'll include some more basic tests soon, but I
> don't think we should go too deep, due to time constraints.

There are other reasons not to test too much, too: the longer tests
are, the less pleasant an experience it is to run them or to modify
the testsuite later.  So just the minimum to make sure the feature and
checks you carefully introduced continue to work as later changes are
made is not only enough but ideal.

> I have updated many of the commit messages.  Do let me know what's
> missing where.

Will send relevant links to previous reviews.  Thanks.

Re: [GSoC update] Sequencer for inclusion

[Ramkumar Ramachandra]

Hi again,

Ramkumar Ramachandra writes:
> Jonathan Nieder writes:
>>  - what changed since last time, for each patch?  (For the future, the
>>   space under the "---" is generally a good place to put that
>>   information.)
>
> Will comment a little more on this in the morning.  The main focus of
> this series is to showcase the new option parser, and show how it fits
> into the rest of the series.

Here's the full change manifest (from memory; might have missed a few):
Patch 1: Exposed advise
Patch 2: No changes.
Patch 3: No changes.
Patch 4: No changes.
Patch 5: No changes.
Patch 6: No changes (forgot to update commit message)
Patch 7: No changes.
Patch 8: No changes.
Patch 9: Rewrote commit message
Patch 10: New!
Patch 11: Updated commit message, rebased on 10.
Patch 12: New!
Patch 13: process_continuation => process_subcommand
Patch 14: New!
Patch 15: New!
Patch 16: Near complete rewrite (subcommand, documentation, tests)
Patch 17: Complete rewrite.

Thanks.

-- Ram

Re: [GSoC update] Sequencer for inclusion

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The basic questions:
> ...

All good suggestions.

> To be precise, the format used includes
> 
> 	strategy-option = patience | renormalize
>
> to represent the effect of "-Xpatience -Xrenormalize".  My only worry
> about that is that the "|" can sound like "or", which would seem
> strange to a user that does not necessarily develop software (so is
> not thinking about bitfields).  The format used in config files puts
>
> 	strategy-option = patience
> 	strategy-option = renormalize
>
> as separate lines.  

A very good point again. I do not think anywhere in our codebase we use
"|" as a separator for state files (either used for internal or exposed to
the end user), and we probably would want to be consistent across
commands.

>> 4. New tests and documentation.  There's really no end to this
>
> Once each new feature has been documented and each new feature or
> fixed bug has an associated test, you've reached the end of this.

Yes.

> My main worry is still the commit messages.  They don't need to be
> elaborate but they should explain the purpose and effect of each
> patch.

Doubly yes.

> I don't know how to move forward on that.  I can explain what's
> missing in each message, but I get the impression that you already
> understand that and there's something else (e.g., time) preventing
> them from getting fixed.

Understanding that does not lead to an action does not really count, does
it? After all, this series was labeled "for inclusion", not an RFC, which
would mean the submitter, helpers, and reviewers all made reasonable
effort to perfect the series to their ability, no?

Re: [GSoC update] Sequencer for inclusion

[Jeff King]

On Mon, Jul 11, 2011 at 01:07:20PM -0700, Junio C Hamano wrote:

> > To be precise, the format used includes
> > 
> > 	strategy-option = patience | renormalize
> >
> > to represent the effect of "-Xpatience -Xrenormalize".  My only worry
> > about that is that the "|" can sound like "or", which would seem
> > strange to a user that does not necessarily develop software (so is
> > not thinking about bitfields).  The format used in config files puts
> >
> > 	strategy-option = patience
> > 	strategy-option = renormalize
> >
> > as separate lines.  
> 
> A very good point again. I do not think anywhere in our codebase we use
> "|" as a separator for state files (either used for internal or exposed to
> the end user), and we probably would want to be consistent across
> commands.

I had the same feeling about the "|". I'm a little confused about what
these options are good for, though.

Is it purely about saving some persistent data between runs, and nobody
else is going to look at it? In that case, I don't think it matters what
the format is. Use some unambiguous encoding that we already have
available (like sq_quote_argv and sq_dequote_argv), and write as little
code as possible.

Or is it something that, like the information in .git/rebase-apply,
people might want to read or tweak? In that case, might it make sense to
follow that lead and split things into one file per item? That keeps the
parsing burden extremely low for things like shell scripts (or shell
users) who might want to read or tweak.

-Peff

Re: [GSoC update] Sequencer for inclusion

[Ramkumar Ramachandra]

Hi Jeff, Junio, Jonathan, and Miles,

Jeff King writes:
> On Mon, Jul 11, 2011 at 01:07:20PM -0700, Junio C Hamano wrote:
>> > To be precise, the format used includes
>> >
>> >     strategy-option = patience | renormalize
>> >
>> > to represent the effect of "-Xpatience -Xrenormalize".  My only worry
>> > about that is that the "|" can sound like "or", which would seem
>> > strange to a user that does not necessarily develop software (so is
>> > not thinking about bitfields).  The format used in config files puts
>> >
>> >     strategy-option = patience
>> >     strategy-option = renormalize
>> >
>> > as separate lines.
>>
>> A very good point again. I do not think anywhere in our codebase we use
>> "|" as a separator for state files (either used for internal or exposed to
>> the end user), and we probably would want to be consistent across
>> commands.
>
> I had the same feeling about the "|". I'm a little confused about what
> these options are good for, though.

Miles Bader writes:
> How about using "+" instead of "|"...?
>
> I think that would make sense both for bitfield-thinking and
> non-bitfield-thinking readers ... :]

Frankly, I don't like the idea of separate lines like "key = value1"
and "key = value 2" -- even a generic *nix configuration file parser
would break here.  Yes, I know that we don't use "|" anywhere else; we
don't persist options anywhere else either, so I was trying to be
inventive :p

I didn't realize "|" would confuse users -- if that's the case, should
we use a different separator like Miles' "+" suggestion?

Jeff King writes:
> Is it purely about saving some persistent data between runs, and nobody
> else is going to look at it? In that case, I don't think it matters what
> the format is. Use some unambiguous encoding that we already have
> available (like sq_quote_argv and sq_dequote_argv), and write as little
> code as possible.
>
> Or is it something that, like the information in .git/rebase-apply,
> people might want to read or tweak? In that case, might it make sense to
> follow that lead and split things into one file per item? That keeps the
> parsing burden extremely low for things like shell scripts (or shell
> users) who might want to read or tweak.

Yes, it's a little like the information in .git/rebase-apply.  We
should give the user the ability to tweak it by hand: the result from
sq_quote_argv and sq_dequote_argv would look very ugly.  Hm, I don't
like the one-file-per-command-line-option approach because: we might
only have a few options now, but when the sequencer is built to
support many actions and options, the directory will be polluted with
lots of files.  I was thinking more along the lines of something that
can be parsed using gitconfig.

Thanks.

-- Ram

Re: [GSoC update] Sequencer for inclusion

[Jeff King]

On Tue, Jul 12, 2011 at 12:11:44PM +0530, Ramkumar Ramachandra wrote:

> > Or is it something that, like the information in .git/rebase-apply,
> > people might want to read or tweak? In that case, might it make sense to
> > follow that lead and split things into one file per item? That keeps the
> > parsing burden extremely low for things like shell scripts (or shell
> > users) who might want to read or tweak.
> 
> Yes, it's a little like the information in .git/rebase-apply.  We
> should give the user the ability to tweak it by hand: the result from
> sq_quote_argv and sq_dequote_argv would look very ugly.  Hm, I don't
> like the one-file-per-command-line-option approach because: we might
> only have a few options now, but when the sequencer is built to
> support many actions and options, the directory will be polluted with
> lots of files.  I was thinking more along the lines of something that
> can be parsed using gitconfig.

I don't consider having lots of files there "pollution", but rather "a
well-used key/value store". But I suppose it is in the eye of the
beholder. :)

Using git-config is maybe a little more self-documenting than something
like "sq_quote_argv". And probably not much more code (maybe even less,
since it can handle the file update for you).

I recently used the config code to write out a non-standard config file.
My two complaints were:

  1. You can't queue up a bunch of changes and then write the file once.
     Every time you call git_config_set, it rewrites the whole file.

  2. There's no way to write to a nonstandard file short of the horribly
     hack-ish:

       const char *saved = config_exclusive_filename;
       config_exclusive_filename = "foo.conf";
       git_config_set(...);
       config_exclusive_filename = saved;

Point (2) is pretty easy to fix. But point (1) might be a bit more
involved. I haven't really looked yet.

-Peff

Re: [GSoC update] Sequencer for inclusion

[Ramkumar Ramachandra]

Hi Jeff,

Jeff King writes:
> On Tue, Jul 12, 2011 at 12:11:44PM +0530, Ramkumar Ramachandra wrote:
>> Yes, it's a little like the information in .git/rebase-apply.  We
>> should give the user the ability to tweak it by hand: the result from
>> sq_quote_argv and sq_dequote_argv would look very ugly.  Hm, I don't
>> like the one-file-per-command-line-option approach because: we might
>> only have a few options now, but when the sequencer is built to
>> support many actions and options, the directory will be polluted with
>> lots of files.  I was thinking more along the lines of something that
>> can be parsed using gitconfig.
>
> I don't consider having lots of files there "pollution", but rather "a
> well-used key/value store". But I suppose it is in the eye of the
> beholder. :)

Ha, true.  After all .git/objects/ is "polluted" with lots of files :p
A couple of questions arise:
1. Would opening, writing, closing many files not put too much strain
on I/O, and hence affect performance significantly?
2. Will two options from different instructions (when we extend the
instruction sheet to accommodate them) have the same name, but
different effects?  Having a gitconfig-like configuration file doesn't
solve this problem either, so it's not a "configfile versus key-value
store" question.

> Using git-config is maybe a little more self-documenting than something
> like "sq_quote_argv". And probably not much more code (maybe even less,
> since it can handle the file update for you).

Yeah :)

> I recently used the config code to write out a non-standard config file.
> My two complaints were:
>
>  1. You can't queue up a bunch of changes and then write the file once.
>     Every time you call git_config_set, it rewrites the whole file.
>
>  2. There's no way to write to a nonstandard file short of the horribly
>     hack-ish:
>
>       const char *saved = config_exclusive_filename;
>       config_exclusive_filename = "foo.conf";
>       git_config_set(...);
>       config_exclusive_filename = saved;
>
> Point (2) is pretty easy to fix. But point (1) might be a bit more
> involved. I haven't really looked yet.

Thanks for pointing these out.  Yes, I'm aware of these issues, and
it's part of the reason I implemented my own parser.  It'll take some
time to refactor config.c so that it's usable by others in a sane
manner, no?  What do you suggest I do until then?  Can I try to get my
custom parser merged (and replaced by a more generic configparser when
it's ready), or should I throw away my parser and go with the
key-value store?

Just to clarify, here's an example to illustrate what I understand
when you say key-value store (please correct me if I got the idea
wrong):
.git/sequencer/opts/ will have files like:
$ cat ff
true
$ cat record-origin
true
$ cat mainline
1
$ cat strategy
ours
$ cat strategy-option
patience + renormalize

-- Ram

Re: [GSoC update] Sequencer for inclusion

[Jeff King]

On Wed, Jul 13, 2011 at 03:11:54PM +0530, Ramkumar Ramachandra wrote:

> Ha, true.  After all .git/objects/ is "polluted" with lots of files :p
> A couple of questions arise:
> 1. Would opening, writing, closing many files not put too much strain
> on I/O, and hence affect performance significantly?

In a tight loop, perhaps. At the begining of invoking a program,
probably not. We're talking about what, a dozen or so files?

If you have doubts, then measure.

> 2. Will two options from different instructions (when we extend the
> instruction sheet to accommodate them) have the same name, but
> different effects?  Having a gitconfig-like configuration file doesn't
> solve this problem either, so it's not a "configfile versus key-value
> store" question.

If you have per-instruction options, then I think you would want your
storage of options to be per-instruction. Whether it's a config file or
files in a directory, you'd want to be accessing ".../opts/$hash" as a
config file (or ".../opts/$hash/*" for a directory).

But I have only been paying a little attention to the deeper parts of
this topic, so I may be misunderstanding what you're asking.

> > Using git-config is maybe a little more self-documenting than something
> > like "sq_quote_argv". And probably not much more code (maybe even less,
> > since it can handle the file update for you).
> 
> Yeah :)

One other advantage of "git config" over sq_quote_argv is that it is
fairly transparent to outside programs. You can just say "git config
--file=.git/sequencer/opts --get-all strategy" or whatever.

That transparency is one of the things I liked about the "files in a
directory" approach taken by rebase currently. But given that the "git
config" command exists, they are probably equivalent in that respect.

> > I recently used the config code to write out a non-standard config file.
> > My two complaints were:
> >
> >  1. You can't queue up a bunch of changes and then write the file once.
> >     Every time you call git_config_set, it rewrites the whole file.
> >
> >  2. There's no way to write to a nonstandard file short of the horribly
> >     hack-ish:
> >
> >       const char *saved = config_exclusive_filename;
> >       config_exclusive_filename = "foo.conf";
> >       git_config_set(...);
> >       config_exclusive_filename = saved;
> >
> > Point (2) is pretty easy to fix. But point (1) might be a bit more
> > involved. I haven't really looked yet.
> 
> Thanks for pointing these out.  Yes, I'm aware of these issues, and
> it's part of the reason I implemented my own parser.  It'll take some
> time to refactor config.c so that it's usable by others in a sane
> manner, no?  What do you suggest I do until then?  Can I try to get my
> custom parser merged (and replaced by a more generic configparser when
> it's ready), or should I throw away my parser and go with the
> key-value store?

I think a reasonable strategy is:

  1. Refactor git_config_set into git_config_set_in_file with a file
     argument (and make git_config_set a wrapper that uses the current
     file-selection). This should be easy to do.

  2. Ignore the fact that you are writing the file multiple times. It's
     not a correctness issue, but rather an optimization. If and when
     the config code shows itself to be too slow, then we can do that
     optimization (which will benefit not just your code, but all config
     writers).

And then throw away your parser and just use git_config_from_file and
git_config_set_in_file.

Between config and a key/value store of files, I think it is largely a
matter of taste.

> Just to clarify, here's an example to illustrate what I understand
> when you say key-value store (please correct me if I got the idea
> wrong):
> .git/sequencer/opts/ will have files like:
> $ cat ff
> true
> $ cat record-origin
> true
> $ cat mainline
> 1
> $ cat strategy
> ours
> $ cat strategy-option
> patience + renormalize

Yeah, exactly. Though I would probably use "\n" to split list items in
strategy-options, which is a little more traditional. I.e.,:

  $ cat strategy-option
  patience
  renormalize

-Peff

[RFC PATCH] config: Introduce functions to write non-standard file

[Ramkumar Ramachandra]

Introduce two new functions corresponding to "git_config_set" and
"git_config_set_multivar" to write a non-standard configuration file.
Expose thse new functions in cache.h for other git programs to use.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Although this patch is ready for inclusion as far as I'm concerned,
 I'm marking it an RFC because I will post it along with the latest
 iteration of the Sequencer -- there's some usage examples in that
 series that can justify the creation of two new functions.

 It may be classified as a workaround -- the more we encourage patches
 like this, the more cruft config.c will accumulate, and the harder
 it'll become to refactor completely.

 cache.h  |    2 ++
 config.c |   29 ++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index bc9e5eb..833ee75 100644
--- a/cache.h
+++ b/cache.h
@@ -1056,9 +1056,11 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_maybe_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_parse_key(const char *, char **, int *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
+extern int git_config_set_multivar_in_file(const char *, const char *, const char *, int, const char *);
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
diff --git a/config.c b/config.c
index 1fc063b..3d9da6b 100644
--- a/config.c
+++ b/config.c
@@ -1092,9 +1092,22 @@ contline:
 	return offset;
 }
 
+int git_config_set_in_file(const char *key, const char *value,
+			const char *filename)
+{
+	const char *saved;
+	int ret;
+
+	saved = config_exclusive_filename;
+	config_exclusive_filename = filename;
+	ret = git_config_set_multivar(key, value, NULL, 0);
+	config_exclusive_filename = saved;
+	return ret;
+}
+
 int git_config_set(const char *key, const char *value)
 {
-	return git_config_set_multivar(key, value, NULL, 0);
+	return git_config_set_in_file(key, value, config_exclusive_filename);
 }
 
 /*
@@ -1387,6 +1400,20 @@ write_err_out:
 
 }
 
+int git_config_set_multivar_in_file(const char *key, const char *value,
+				const char *value_regex, int multi_replace,
+				const char *filename)
+{
+	const char *saved;
+	int ret;
+
+	saved = config_exclusive_filename;
+	config_exclusive_filename = filename;
+	ret = git_config_set_multivar(key, value, value_regex, multi_replace);
+	config_exclusive_filename = saved;
+	return ret;
+}
+
 static int section_name_match (const char *buf, const char *name)
 {
 	int i = 0, j = 0, dot = 0;
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [RFC PATCH] config: Introduce functions to write non-standard file

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +int git_config_set_in_file(const char *key, const char *value,
> +			const char *filename)
> +{
> +	const char *saved;
> +	int ret;
> +
> +	saved = config_exclusive_filename;
> +	config_exclusive_filename = filename;
> +	ret = git_config_set_multivar(key, value, NULL, 0);
> +	config_exclusive_filename = saved;
> +	return ret;
> +}

Yuck, what an ugly hack when you are so close.  Why not restructure the
code in such a way that the updated config-set-multivar and set-in-file
share the code better?  If the former takes a hardcoded logic to decide
which file to write to while the latter takes the filename to write to as
its argument, wouldn't it be a lot more natural to make config-set-multivar
call set-in-file after computing what filename parameter to pass?

Re: [RFC PATCH] config: Introduce functions to write non-standard file

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
[...]
> Yuck, what an ugly hack when you are so close.  Why not restructure the
> code in such a way that the updated config-set-multivar and set-in-file
> share the code better?  If the former takes a hardcoded logic to decide
> which file to write to while the latter takes the filename to write to as
> its argument, wouldn't it be a lot more natural to make config-set-multivar
> call set-in-file after computing what filename parameter to pass?

Ugh, sorry. Fixed now.

-- Ram

Re: [GSoC update] Sequencer for inclusion

[Jonathan Nieder]

Junio C Hamano wrote:

> After all, this series was labeled "for inclusion", not an RFC, which
> would mean the submitter, helpers, and reviewers all made reasonable
> effort to perfect the series to their ability, no?

Yep, that is how I interpret the subject line, too.

Wow, the series has been through a lot of iterations.  I finally found
a moment to dig up the ones I replied to (a full list would be harder
since search.gmane.org is still down).  Maybe they can save others
some time, too.

 - RFC: Sequencer Foundations [1]
   The basic themes are introduced and Daniel explains the
   two-levels concept.

 - Sequencer Foundations[2]
   Basic discussion of the purpose of die()-to-error() conversion and
   the suggestion of an almost crash-only design to avoid some of its
   problems.

 - Better error handling around revert[3]
   A quick discussion of die() versus assert() and how to unwind
   after errors.

 - Sequencer Foundations v3, v4[4]
   Capturing globals in a struct.  Whether to save error codes before
   propagating them (versus the simpler "return -1").  Another round
   of die()-to-error() versus crash-only code.  Pacing and the idea of
   merging bit-by-bit.

 - Implementing --abort processing[5][6]
   UI design is difficult.  The easiest way is to teach the porcelain
   to do what you were doing by hand already.

 - Sequencer with continuation features[7]
   Commit messages.  Variable names.  commit_list_append().  It seems
   we are closing in.

 - Sequencer: the insn sheet format[8]
   Various loose ends: i18n, commit messages, relationship between
   breakage of scripts and workflows and the need to patch tests, the
   "me" variable, advise(), variable names.

I suspect that getting the workarounds that keep existing scripts
working will take another round.  However, some of the early patches
might be ready.  My review will focus on that possibility.

Ram, thanks for your hard work.

[1] http://thread.gmane.org/gmane.comp.version-control.git/171255
[2] http://thread.gmane.org/gmane.comp.version-control.git/173408
[3] http://thread.gmane.org/gmane.comp.version-control.git/174043/focus=174050
[4] http://thread.gmane.org/gmane.comp.version-control.git/174393/focus=174540
[5] http://thread.gmane.org/gmane.comp.version-control.git/174874
[6] http://thread.gmane.org/gmane.comp.version-control.git/175638
[7] http://thread.gmane.org/gmane.comp.version-control.git/176139
[8] http://thread.gmane.org/gmane.comp.version-control.git/176647

Re: [GSoC update] Sequencer for inclusion

[Miles Bader]

Jonathan Nieder <jrnieder@gmail.com> writes:
> To be precise, the format used includes
>
> 	strategy-option = patience | renormalize
>
> to represent the effect of "-Xpatience -Xrenormalize".  My only worry
> about that is that the "|" can sound like "or", which would seem
> strange to a user that does not necessarily develop software (so is
> not thinking about bitfields).

How about using "+" instead of "|"...?

I think that would make sense both for bitfield-thinking and
non-bitfield-thinking readers ... :]

-Miles

-- 
Occam's razor split hairs so well, I bought the whole argument!