[PATCH 0/6] Sequencer fixups mini-series

[PATCH 0/6] Sequencer fixups mini-series

[Ramkumar Ramachandra]

Hi,

Now that the original sequencer series has hit 'master' (cd4093b6), we
can now build on it!  Unfortunately, as outlined in $gmane/179613,
there are several UI design difficulties that we need to surmount.  As
a prelude, I've decided to prepare this mini-series for fixing up a
few minor issues before attacking the problem; please see
$gmane/179304 for relevant discussions.

The differences are:
1. I've dropped the last two parts in the previous iteration.
2. Part 2 is new.  Thanks to Jonathan for the suggestion.
3. Minor fixups and commit message improvements in response to
reviews.

p.s- I'm travelling this week, and won't be able to respond to
reviews until the 16th.

Thanks.

-- Ram

Jonathan Nieder (1):
  revert: Simplify passing command-line arguments around

Ramkumar Ramachandra (5):
  revert: Free memory after get_message call
  revert: Simplify getting commit subject
  revert: Fix buffer overflow in insn sheet parser
  revert: Make commit descriptions in insn sheet optional
  revert: Allow mixed pick and revert instructions

 builtin/revert.c                |  209 ++++++++++++++++++++-------------------
 sequencer.h                     |    8 ++
 t/t3510-cherry-pick-sequence.sh |   86 ++++++++++++++++
 3 files changed, 200 insertions(+), 103 deletions(-)

-- 
1.7.4.1

[PATCH 1/6] revert: Free memory after get_message call

[Ramkumar Ramachandra]

The format_todo function leaks memory because it forgets to call
free_message after get_message.  It is a potentially big leak, because
fresh memory is allocated to store the commit message message for each
commit.  Fix this.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index ba27cf1..a2c304d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -682,6 +682,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 		if (get_message(cur->item, &msg))
 			return error(_("Cannot get commit message for %s"), sha1_abbrev);
 		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+		free_message(&msg);
 	}
 	return 0;
 }
-- 
1.7.4.1

[PATCH 2/6] revert: Simplify getting commit subject

[Ramkumar Ramachandra]

The heavy parsing and memory allocations performed by get_message is
unnecessary when only the commit subject is desired.  Use
find_commit_subject instead.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index a2c304d..b3c5e0e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -673,16 +673,16 @@ static int 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_str = opts->action == REVERT ? "revert" : "pick";
+	const char *subject;
+	int subject_len;
 
 	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))
-			return error(_("Cannot get commit message for %s"), sha1_abbrev);
-		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
-		free_message(&msg);
+		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+			subject_len, subject);
 	}
 	return 0;
 }
-- 
1.7.4.1

[PATCH 3/6] revert: Fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Check that the commit name argument to a "pick" or "revert" action in
'.git/sequencer/todo' is not too long, to avoid overflowing an
on-stack buffer.  This fixes a regression introduced by 5a5d80f4
(revert: Introduce --continue to continue the operation, 2011-08-04).

Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |    2 +-
 t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index b3c5e0e..6451089 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -707,7 +707,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 		return NULL;
 
 	q = strchr(p, ' ');
-	if (!q)
+	if (!q || q - p + 1 > sizeof(sha1_abbrev))
 		return NULL;
 	q++;
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3bca2b3..2113308 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -12,6 +12,9 @@ test_description='Test cherry-pick continuation features
 
 . ./test-lib.sh
 
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
 pristine_detach () {
 	git cherry-pick --reset &&
 	git checkout -f "$1^0" &&
@@ -211,4 +214,15 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	test_must_fail git cherry-pick --continue
+'
+
 test_done
-- 
1.7.4.1

[PATCH 4/6] revert: Make commit descriptions in insn sheet optional

[Ramkumar Ramachandra]

Change the instruction sheet format subtly so that a description of
the commit after the object name is optional.  As a result, an
instruction sheet like this is now perfectly valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |   19 ++++++++-----------
 t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 6451089..aa6c34e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -692,26 +692,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
 	enum replay_action action;
-	int insn_len = 0;
-	char *p, *q;
+	const char *p, *q;
 
+	p = start;
 	if (!prefixcmp(start, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
+		p += strlen("pick ");
 	} else if (!prefixcmp(start, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		p += strlen("revert ");
 	} else
 		return NULL;
 
-	q = strchr(p, ' ');
-	if (!q || q - p + 1 > sizeof(sha1_abbrev))
+	q = p + strcspn(p, " \n");
+	if (q - p + 1 > sizeof(sha1_abbrev))
 		return NULL;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
+	memcpy(sha1_abbrev, p, q - p);
+	sha1_abbrev[q - p] = '\0';
 
 	/*
 	 * Verify that the action matches up with the one in
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2113308..39b55c1 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -225,4 +225,18 @@ test_expect_success 'malformed instruction sheet 3' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'commit descriptions in insn sheet are optional' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	git rev-list HEAD >commits
+	test_line_count = 4 commits
+'
+
 test_done
-- 
1.7.4.1

Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Change the instruction sheet format subtly so that a description of
> the commit after the object name is optional.  As a result, an
> instruction sheet like this is now perfectly valid:
>
>   pick 35b0426
>   pick fbd5bbcbc2e
>   pick 7362160f
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/revert.c                |   19 ++++++++-----------
>  t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 6451089..aa6c34e 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -692,26 +692,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
>  	unsigned char commit_sha1[20];
>  	char sha1_abbrev[40];
>  	enum replay_action action;
> -	int insn_len = 0;
> -	char *p, *q;
> +	const char *p, *q;
>  
> +	p = start;
>  	if (!prefixcmp(start, "pick ")) {
>  		action = CHERRY_PICK;
> -		insn_len = strlen("pick");
> -		p = start + insn_len + 1;
> +		p += strlen("pick ");
>  	} else if (!prefixcmp(start, "revert ")) {
>  		action = REVERT;
> -		insn_len = strlen("revert");
> -		p = start + insn_len + 1;
> +		p += strlen("revert ");
>  	} else
>  		return NULL;
>  
> -	q = strchr(p, ' ');
> -	if (!q || q - p + 1 > sizeof(sha1_abbrev))
> +	q = p + strcspn(p, " \n");
> +	if (q - p + 1 > sizeof(sha1_abbrev))
>  		return NULL;
> -	q++;
> -
> -	strlcpy(sha1_abbrev, p, q - p);
> +	memcpy(sha1_abbrev, p, q - p);
> +	sha1_abbrev[q - p] = '\0';

Since this is a part of clean-up series...

Do you even need to have a sha1_abbrev[] local array that is limited to 40
bytes here? The incoming _line_ is not "const char *start", so you should
at least be able to temporarily terminate the commit object name with NUL
(while remembering what byte there was before), give it to get_sha1(), and
then restore the byte at the end before returning from this function.

A bonus point would be to introduce a variant of get_sha1() that can take
(a pointer + len) not (a pointer to NUL terminated string). While I think
that would be a right thing to do in the longer term, it is outside of the
scope of this series.

Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional

[Junio C Hamano]

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

> Since this is a part of clean-up series...
>
> Do you even need to have a sha1_abbrev[] local array that is limited to 40
> bytes here? The incoming _line_ is not "const char *start", so you should
> at least be able to temporarily terminate the commit object name with NUL
> (while remembering what byte there was before), give it to get_sha1(), and
> then restore the byte at the end before returning from this function.

Like this, perhaps.  I did this on top of the whole series only as a
demonstration but the change should be squashed into this step when the
series is rerolled.

 builtin/revert.c |   47 +++++++++++++++++++----------------------------
 1 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index b28c3ca..170a6c1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -691,42 +691,34 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 	return 0;
 }
 
-static int parse_insn_line(char *start, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 {
 	unsigned char commit_sha1[20];
-	char sha1_abbrev[40];
-	const char *p, *q;
+	char *end_of_object_name;
+	int saved, status;
 
-	p = start;
-	if (!prefixcmp(start, "pick ")) {
+	if (!prefixcmp(bol, "pick ")) {
 		item->action = REPLAY_PICK;
-		p += strlen("pick ");
-	} else if (!prefixcmp(start, "revert ")) {
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ")) {
 		item->action = REPLAY_REVERT;
-		p += strlen("revert ");
+		bol += strlen("revert ");
 	} else {
-		size_t len = strchrnul(p, '\n') - p;
-		if (len > 255)
-			len = 255;
-		return error(_("Unrecognized action: %.*s"), (int)len, p);
+		return error(_("Unrecognized action: %s"), bol);
 	}
 
-	q = p + strcspn(p, " \n");
-	if (q - p + 1 > sizeof(sha1_abbrev)) {
-		size_t len = q - p;
-		if (len > 255)
-			len = 255;
-		return error(_("Object name too large: %.*s"), (int)len, p);
-	}
-	memcpy(sha1_abbrev, p, q - p);
-	sha1_abbrev[q - p] = '\0';
+	end_of_object_name = bol + strcspn(bol, " \n");
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	status = get_sha1(bol, commit_sha1);
+	*end_of_object_name = saved;
 
-	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
-		return error(_("Malformed object name: %s"), sha1_abbrev);
+	if (status < 0)
+		return error(_("Malformed object name: %s"), bol);
 
 	item->operand = lookup_commit_reference(commit_sha1);
 	if (!item->operand)
-		return error(_("Not a valid commit: %s"), sha1_abbrev);
+		return error(_("Not a valid commit: %s"), bol);
 
 	item->next = NULL;
 	return 0;
@@ -740,12 +732,11 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 	int i;
 
 	for (i = 1; *p; i++) {
-		if (parse_insn_line(p, &item) < 0)
+		char *eol = strchrnul(p, '\n');
+		if (parse_insn_line(p, eol, &item) < 0)
 			return error(_("on line %d."), i);
 		next = replay_insn_list_append(item.action, item.operand, next);
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
+		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
 		return error(_("No commits parsed."));

Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Like this, perhaps.  I did this on top of the whole series only as a
> demonstration but the change should be squashed into this step when the
> series is rerolled.
>
>  builtin/revert.c |   47 +++++++++++++++++++----------------------------
> [...]

Thanks!  This is fantastic.  I'll re-roll soon with better commit messages :)

-- Ram

[PATCH 5/6] revert: Allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Parse the instruction list in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming all instructions use the
same action.  So now, you can do:

  pick fdc0b12 picked
  revert 965fed4 anotherpick

For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.
This patch lays the foundation for extending the parser to support
more actions so 'git rebase -i' can reuse this machinery in the
future.  While at it, also improve the error messages reported by the
insn sheet parser.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index aa6c34e..a9dd210 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_action { REVERT, CHERRY_PICK };
 enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
 
 struct replay_opts {
@@ -68,14 +67,14 @@ struct replay_opts {
 
 static const char *action_name(const struct replay_opts *opts)
 {
-	return opts->action == REVERT ? "revert" : "cherry-pick";
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
 static char *get_encoding(const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -154,7 +153,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 	};
 
-	if (opts->action == CHERRY_PICK) {
+	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -348,7 +347,7 @@ static int error_dirty_index(struct replay_opts *opts)
 		return error_resolve_conflict(action_name(opts));
 
 	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == CHERRY_PICK)
+	if (opts->action == REPLAY_PICK)
 		error(_("Your local changes would be overwritten by cherry-pick."));
 	else
 		error(_("Your local changes would be overwritten by revert."));
@@ -452,7 +451,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -527,7 +527,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REVERT) {
+	if (action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -570,7 +570,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			write_cherry_pick_head(commit);
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -589,7 +589,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}
 
 	if (res) {
-		error(opts->action == REVERT
+		error(action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -613,7 +613,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (opts->action != REVERT)
+	if (opts->action != REPLAY_REVERT)
 		revs->reverse = 1;
 
 	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -659,87 +659,87 @@ static void read_and_refresh_cache(struct replay_opts *opts)
  *     assert(commit_list_count(list) == 2);
  *     return list;
  */
-static struct commit_list **commit_list_append(struct commit *commit,
-					       struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next)
 {
-	struct commit_list *new = xmalloc(sizeof(struct commit_list));
-	new->item = commit;
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
 	*next = new;
 	new->next = NULL;
 	return &new->next;
 }
 
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
-		struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 {
-	struct commit_list *cur = NULL;
-	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REVERT ? "revert" : "pick";
-	const char *subject;
-	int subject_len;
+	struct replay_insn_list *cur;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
 		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
 			subject_len, subject);
 	}
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static int parse_insn_line(char *start, struct replay_insn_list *item)
 {
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
-	enum replay_action action;
 	const char *p, *q;
 
 	p = start;
 	if (!prefixcmp(start, "pick ")) {
-		action = CHERRY_PICK;
+		item->action = REPLAY_PICK;
 		p += strlen("pick ");
 	} else if (!prefixcmp(start, "revert ")) {
-		action = REVERT;
+		item->action = REPLAY_REVERT;
 		p += strlen("revert ");
-	} else
-		return NULL;
+	} else {
+		size_t len = strchrnul(p, '\n') - p;
+		if (len > 255)
+			len = 255;
+		return error(_("Unrecognized action: %.*s"), len, p);
+	}
 
 	q = p + strcspn(p, " \n");
-	if (q - p + 1 > sizeof(sha1_abbrev))
-		return NULL;
+	if (q - p + 1 > sizeof(sha1_abbrev)) {
+		size_t len = q - p;
+		if (len > 255)
+			len = 255;
+		return error(_("Object name too large: %.*s"), len, p);
+	}
 	memcpy(sha1_abbrev, p, q - p);
 	sha1_abbrev[q - p] = '\0';
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if (action != opts->action) {
-		const char *action_str;
-		action_str = action == 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)
-		return NULL;
+		return error(_("Malformed object name: %s"), sha1_abbrev);
 
-	return lookup_commit_reference(commit_sha1);
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return error(_("Not a valid commit: %s"), sha1_abbrev);
+
+	item->next = NULL;
+	return 0;
 }
 
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
-			struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 {
-	struct commit_list **next = todo_list;
-	struct commit *commit;
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = {0, NULL, NULL};
 	char *p = buf;
 	int i;
 
 	for (i = 1; *p; i++) {
-		commit = parse_insn_line(p, opts);
-		if (!commit)
-			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		if (parse_insn_line(p, &item) < 0)
+			return error(_("on line %d."), i);
+		next = replay_insn_list_append(item.action, item.operand, next);
 		p = strchrnul(p, '\n');
 		if (*p)
 			p++;
@@ -749,8 +749,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
-			struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
@@ -766,7 +765,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	}
 	close(fd);
 
-	res = parse_insn_buffer(buf.buf, todo_list, opts);
+	res = parse_insn_buffer(buf.buf, todo_list);
 	strbuf_release(&buf);
 	if (res)
 		die(_("Unusable instruction sheet: %s"), todo_file);
@@ -815,18 +814,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	struct commit_list **next;
+	struct replay_insn_list **next;
 
 	prepare_revs(&revs, opts);
 
 	next = todo_list;
 	while ((commit = get_revision(&revs)))
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(opts->action, commit, next);
 }
 
 static int create_seq_dir(void)
@@ -855,7 +854,7 @@ static void save_head(const char *head)
 		die(_("Error wrapping up %s."), head_file);
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
@@ -863,7 +862,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	int fd;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
+	if (format_todo(&buf, todo_list) < 0)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
@@ -907,9 +906,10 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
 {
-	struct commit_list *cur;
+	struct replay_insn_list *cur;
 	int res;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -919,8 +919,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
-		res = do_pick_commit(cur->item, opts);
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
 		if (res) {
 			if (!cur->next)
 				/*
@@ -945,7 +945,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 
 static int pick_revisions(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
 	read_and_refresh_cache(opts);
@@ -962,7 +962,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			goto error;
 		read_populate_opts(&opts);
-		read_populate_todo(&todo_list, opts);
+		read_populate_todo(&todo_list);
 
 		/* Verify that the conflict has been resolved */
 		if (!index_differs_from("HEAD", 0))
@@ -982,7 +982,7 @@ static int pick_revisions(struct replay_opts *opts)
 			return -1;
 		}
 		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REVERT)
+			if (opts->action == REPLAY_REVERT)
 				return error(_("Can't revert as initial commit"));
 			return error(_("Can't cherry-pick into empty head"));
 		}
@@ -1002,7 +1002,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
 		opts.edit = 1;
-	opts.action = REVERT;
+	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
@@ -1017,7 +1017,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	int res;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.action = CHERRY_PICK;
+	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index 905d295..f4db257 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,14 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+
+struct replay_insn_list {
+	enum replay_action action;
+	struct commit *operand;
+	struct replay_insn_list *next;
+};
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 39b55c1..4b12244 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -239,4 +239,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+test_expect_success 'revert --continue continues after cherry-pick' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	git revert --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	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 'mixed pick and revert instructions' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	oldsha=`git rev-parse --short HEAD~1` &&
+	echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:000000 100644 OBJID OBJID A	foo
+	:000000 100644 OBJID OBJID A	unrelated
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.4.1

[PATCH 6/6] revert: Simplify passing command-line arguments around

[Ramkumar Ramachandra]

From: Jonathan Nieder <jrnieder@gmail.com>

The chief command-line argument parser, parse_args, currently parses
arguments into an (argc, argv) and lets prepare_revs, a later
function, turn it into a structure.  Change this so that callers of
the cherry-pick machinery will have to fill in an options structure
instead of crafting command-line arguments.

[rr: minor improvements, commit message]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index a9dd210..a72b20d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -54,13 +54,14 @@ struct replay_opts {
 	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;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
 };
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -163,9 +164,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			die(_("program error"));
 	}
 
-	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-					PARSE_OPT_KEEP_ARGV0 |
-					PARSE_OPT_KEEP_UNKNOWN);
+	argc = parse_options(argc, argv, NULL, options, usage_str,
+			PARSE_OPT_KEEP_ARGV0 |
+			PARSE_OPT_KEEP_UNKNOWN);
 
 	/* Check for incompatible subcommands */
 	verify_opt_mutually_compatible(me,
@@ -200,9 +201,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 	}
 
-	else if (opts->commit_argc < 2)
-		usage_with_options(usage_str, options);
-
 	if (opts->allow_ff)
 		verify_opt_compatible(me, "--ff",
 				"--signoff", opts->signoff,
@@ -210,7 +208,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
-	opts->commit_argv = argv;
+
+	if (opts->subcommand == REPLAY_NONE) {
+		opts->revs = xmalloc(sizeof(*opts->revs));
+		init_revisions(opts->revs, NULL);
+		opts->revs->no_walk = 1;
+		if (argc < 2)
+			usage_with_options(usage_str, options);
+		argc = setup_revisions(argc, argv, opts->revs, NULL);
+	} else
+		opts->revs = NULL;
+
+	/* Forbid stray command-line arguments */
+	if (argc > 1)
+		usage_with_options(usage_str, options);
 }
 
 struct commit_message {
@@ -607,23 +618,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action,
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
 {
-	int argc;
-
-	init_revisions(revs, NULL);
-	revs->no_walk = 1;
 	if (opts->action != REPLAY_REVERT)
-		revs->reverse = 1;
+		opts->revs->reverse ^= 1;
 
-	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(opts));
-
-	if (prepare_revision_walk(revs))
+	if (prepare_revision_walk(opts->revs))
 		die(_("revision walk setup failed"));
 
-	if (!revs->commits)
+	if (!opts->revs->commits)
 		die(_("empty commit set passed"));
 }
 
@@ -817,14 +820,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
-	struct rev_info revs;
 	struct commit *commit;
 	struct replay_insn_list **next;
 
-	prepare_revs(&revs, opts);
+	prepare_revs(opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
+	while ((commit = get_revision(opts->revs)))
 		next = replay_insn_list_append(opts->action, commit, next);
 }
 
@@ -948,6 +950,9 @@ static int pick_revisions(struct replay_opts *opts)
 	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
 	read_and_refresh_cache(opts);
 
 	/*
-- 
1.7.4.1

Re: [PATCH 6/6] revert: Simplify passing command-line arguments around

[Tay Ray Chuan]

On Sun, Oct 9, 2011 at 1:36 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> [snip]
> [rr: minor improvements, commit message]

This "[]" could go below, under the 3-dash (but before the stat):

> [snip]
> ---
>  builtin/revert.c |   53 +++++++++++++++++++++++++++++------------------------
>  1 files changed, 29 insertions(+), 24 deletions(-)

-- 
Cheers,
Ray Chuan

Re: [PATCH 6/6] revert: Simplify passing command-line arguments around

[Ramkumar Ramachandra]

Hi Tay,

Tay Ray Chuan writes:
> On Sun, Oct 9, 2011 at 1:36 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> [rr: minor improvements, commit message]
>
> This "[]" could go below, under the 3-dash (but before the stat):

Actually, I intended to put it in the commit message.  Seems to be
idiomatic: grep the log for "\[jc:" etc.

Thanks.

-- Ram

Re: [PATCH 6/6] revert: Simplify passing command-line arguments around

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Actually, I intended to put it in the commit message.  Seems to be
> idiomatic: grep the log for "\[jc:" etc.

More important than the idiom is the intent.  Presumably that intent
was something like "I wrote the commit message, so if it makes you
scratch your head, blame me, not Jonathan; and I made some other
(minor) improvements, so consider blaming me even if it's the
functional part that makes you scratch your head."

Sorry I haven't had a chance to look over the patch yet.  Is it
supposed to introduce a behavior change, does it prepare for some
future change, or is it just a cleanup?

Re: [PATCH 6/6] revert: Simplify passing command-line arguments around

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> More important than the idiom is the intent.  Presumably that intent
> was something like "I wrote the commit message, so if it makes you
> scratch your head, blame me, not Jonathan; and I made some other
> (minor) improvements, so consider blaming me even if it's the
> functional part that makes you scratch your head."

Exactly.

> Sorry I haven't had a chance to look over the patch yet.  Is it
> supposed to introduce a behavior change, does it prepare for some
> future change, or is it just a cleanup?

Prepare for some future change.  See $gmane/179282 for original discussion.

-- Ram

Re: [PATCH 6/6] revert: Simplify passing command-line arguments around

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> Sorry I haven't had a chance to look over the patch yet.  Is it
>> supposed to introduce a behavior change, does it prepare for some
>> future change, or is it just a cleanup?
>
> Prepare for some future change.  See $gmane/179282 for original discussion.

Thanks, but I shouldn't have had to ask.  Care to fix the commit
message? :)

Re: [PATCH 6/6] revert: Simplify passing command-line arguments around

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Thanks, but I shouldn't have had to ask.  Care to fix the commit
> message? :)

Ah, yes- my persistent lack of clarity in commit messages :|
Hopefully, the next iteration will have better commit messages.

-- Ram

[PATCH v2 0/6] Sequencer fixups mini-series

[Ramkumar Ramachandra]

Hi,

This is the second iteration of series I first posted in
$gmane/183157.  The improvements can be summarized as follows:
1. Junio wrote some extra code to squash into the fourth part.  It
took me some time to resolve conflicts correctly while rebasing.
2. Jonathan and Tay (off-list) have helped improve all the commit
messages.

Thanks.

Jonathan Nieder (1):
  revert: simplify communicating command-line arguments

Ramkumar Ramachandra (5):
  revert: free msg in format_todo()
  revert: simplify getting commit subject in format_todo()
  revert: fix buffer overflow in insn sheet parser
  revert: make commit subjects in insn sheet optional
  revert: allow mixed pick and revert instructions

 builtin/revert.c                |  219 +++++++++++++++++++--------------------
 sequencer.h                     |    8 ++
 t/t3510-cherry-pick-sequence.sh |   86 +++++++++++++++
 3 files changed, 202 insertions(+), 111 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

[PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Check that the commit name argument to a "pick" or "revert" action in
'.git/sequencer/todo' is not too long, to avoid overflowing an
on-stack buffer.  This fixes a regression introduced by 5a5d80f4
(revert: Introduce --continue to continue the operation, 2011-08-04).

Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |    2 +-
 t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index acb357d..474dda1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -705,7 +705,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 		return NULL;
 
 	q = strchr(p, ' ');
-	if (!q)
+	if (!q || q - p + 1 > sizeof(sha1_abbrev))
 		return NULL;
 	q++;
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3bca2b3..2113308 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -12,6 +12,9 @@ test_description='Test cherry-pick continuation features
 
 . ./test-lib.sh
 
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
 pristine_detach () {
 	git cherry-pick --reset &&
 	git checkout -f "$1^0" &&
@@ -211,4 +214,15 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	test_must_fail git cherry-pick --continue
+'
+
 test_done
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Check that the commit name argument to a "pick" or "revert" action in
> '.git/sequencer/todo' is not too long, to avoid overflowing an
> on-stack buffer.  This fixes a regression introduced by 5a5d80f4
> (revert: Introduce --continue to continue the operation, 2011-08-04).

Given that this function is going to be fixed properly so that it does not
even need to use the "on-stack buffer", is this really necessary?

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Jonathan Nieder]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> Check that the commit name argument to a "pick" or "revert" action in
>> '.git/sequencer/todo' is not too long
[...]
> Given that this function is going to be fixed properly so that it does not
> even need to use the "on-stack buffer", is this really necessary?

Right, I don't think it is.  Keeping a testcase sounds worthwhile,
though.

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Hi Jonathan and Junio,

Jonathan Nieder writes:
> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>> Check that the commit name argument to a "pick" or "revert" action in
>>> '.git/sequencer/todo' is not too long
> [...]
>> Given that this function is going to be fixed properly so that it does not
>> even need to use the "on-stack buffer", is this really necessary?
>
> Right, I don't think it is.  Keeping a testcase sounds worthwhile,
> though.

Okay.  How about putting this after 5/6?

-- 8< --
Subject: [PATCH] t3510: guard against buffer overflows in parser

To guard against a buffer overflow in the parser, verify that
instruction sheets with overly long object names are parsed.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 0e29e03..39b55c1 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -12,6 +12,9 @@ test_description='Test cherry-pick continuation features
 
 . ./test-lib.sh
 
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
 pristine_detach () {
        git cherry-pick --reset &&
        git checkout -f "$1^0" &&
@@ -211,6 +214,17 @@ test_expect_success 'malformed instruction sheet 2' '
        test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick base..anotherpick &&
+       echo "resolved" >foo &&
+       git add foo &&
+       git commit &&
+       sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+       cp new_sheet .git/sequencer/todo &&
+       test_must_fail git cherry-pick --continue
+'
+
 test_expect_success 'commit descriptions in insn sheet are optional' '
        pristine_detach initial &&
        test_must_fail git cherry-pick base..anotherpick &&
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Okay.  How about putting this after 5/6?
>
> -- 8< --
> Subject: [PATCH] t3510: guard against buffer overflows in parser
> 
> To guard against a buffer overflow in the parser, verify that
> instruction sheets with overly long object names are parsed.

Looks good, except I would explain it differently, to avoid referring
to hypothetical implementation details ("What buffer overflow?"):

	test: git cherry-pick --continue should cope with long object names

	A naive implementation that uses a commit-id-shaped buffer
	to store the word after "pick" in .git/sequencer/todo lines
	would crash often.  Our implementation is not so naive, but
	add a test anyway to futureproof it.

Or:

	test: make sure the "cherry-pick --continue" buffer overflow doesn't come back

	Before commit ..., "git cherry-pick --continue" would overflow
	under ... circumstance.  Add a test to make sure it doesn't
	happen again.

Though the implementation is actually better than that --- it can even
cope with a valid object name (e.g., a long name of a branch, or
something like "HEAD^{/refs.c: ensure struct whose member}") that is
that long, without truncating it.  So if you have time for it, I think
it would be worth a test where the "git cherry-pick --continue"
succeeds, too.

Thanks,
Jonathan

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> [...]
> Looks good, except I would explain it differently, to avoid referring
> to hypothetical implementation details ("What buffer overflow?"):
>
>        test: git cherry-pick --continue should cope with long object names
>
>        A naive implementation that uses a commit-id-shaped buffer
>        to store the word after "pick" in .git/sequencer/todo lines
>        would crash often.  Our implementation is not so naive, but
>        add a test anyway to futureproof it.
> [...]

I picked this one.

> Though the implementation is actually better than that --- it can even
> cope with a valid object name (e.g., a long name of a branch, or
> something like "HEAD^{/refs.c: ensure struct whose member}") that is
> that long, without truncating it.  So if you have time for it, I think
> it would be worth a test where the "git cherry-pick --continue"
> succeeds, too.

Good idea.  Will re-roll shortly.

Thanks.

-- Ram

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Looks good, except I would explain it differently, to avoid referring
> to hypothetical implementation details ("What buffer overflow?"):
>
> 	test: git cherry-pick --continue should cope with long object names
>
> 	A naive implementation that uses a commit-id-shaped buffer
> 	to store the word after "pick" in .git/sequencer/todo lines
> 	would crash often.  Our implementation is not so naive, but
> 	add a test anyway to futureproof it.
>
> Or:
>
> 	test: make sure the "cherry-pick --continue" buffer overflow doesn't come back
>
> 	Before commit ..., "git cherry-pick --continue" would overflow
> 	under ... circumstance.  Add a test to make sure it doesn't
> 	happen again.

I doubt you would need any of that.

You can just explain the commit that stops copying the lines into a
private, fixed buffer a bit better (e.g. "such copying is not just
wasteful but is wrong by unnecessary placing an artificial limit on the
line length"), and say "Incidentally, this fixes a bug in the earlier
round of this series that failed to read lines that are too long to fit on
the buffer, demonstrated by the test added by this patch", or something.

Then the additional test can become part of the patch that corrects the
parsing logic, no?

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Jonathan Nieder]

Junio C Hamano wrote:

[...]
> Then the additional test can become part of the patch that corrects the
> parsing logic, no?

Yes, that works, too.  All I was trying to say was that the
description in the patch I quoted didn't make sense to me, since it
included a mention of a buffer overflow without giving any explanation
of what it was talking about.  I don't actually care in this case
whether it is fixed by mentioning which patch this is testing the fix
from or by squashing the two patches (though the latter certainly
seems reasonable).

Incidentally, Ram might wonder why I fuss so much about commit
messages.  It's actually very simple --- I think of them as part of
the code.  Suppose someone discovers a regression was introduced by
such-and-such part of the patch 1.7.7 -> 1.7.8, but at first glance it
is not clear whether that code change was supposed to have any effect
on the behavior of the code at all.  Such a person is likely to make
mistakes in fixing it, right?  So after getting the right behavior,
patch authors spend a few extra minutes to make sure the code is
intuitive to humans, too, and this includes making sure the rationale
description is clear.

Just like the code for the computer, this is very much something that
isn't always going to be right the first time and sometimes takes some
debugging.  So, sorry for the fuss, but I hope it helps.

Jonathan

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Incidentally, Ram might wonder why I fuss so much about commit
> messages.  It's actually very simple --- I think of them as part of
> the code.

And another reason is because I do fuss about them too ;-)

It is easy to tell a good patch from a bad one by just reading the message
without actually reading the patch text itself.

When the log message justifies the cause and the approach in the right
way, the actual patch becomes self evident. Also I often find myself
coming up with a _better_ solution than the patch I originally prepared
while writing the commit log message to explain it, and redoing the patch
text to match the description.

Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Hi Junio and Jonathan,

Thanks for the good suggestions.  Will post the next iteration in a
few minutes (some tests running now).

Junio C Hamano writes:
> [...]
> When the log message justifies the cause and the approach in the right
> way, the actual patch becomes self evident. Also I often find myself
> coming up with a _better_ solution than the patch I originally prepared
> while writing the commit log message to explain it, and redoing the patch
> text to match the description.

Wow.  It looks like I have a long way to go :/
Maybe I should practice writing more Haskell.

-- Ram