[PATCH 0/5] Re-roll rr/revert-cherry-pick

[PATCH 0/5] Re-roll rr/revert-cherry-pick

[Ramkumar Ramachandra]

Hi,

This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
($gname/186365) implemented.  My new branch rr/sequencer will be based
on this branch.

Thanks.

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

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

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

-- 
1.7.7.3

[PATCH 1/5] revert: free msg in format_todo()

[Ramkumar Ramachandra]

Memory allocated to the fields of msg by get_message() isn't freed.
This is potentially a big leak, because fresh memory is allocated to
store the commit message for each commit.  Fix this using
free_message().

Reported-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 |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c6d3d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -706,6 +706,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.7.3

[PATCH 2/5] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

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

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

While at it, also fix a bug: currently, we use a commit-id-shaped
buffer to store the word after "pick" in '.git/sequencer/todo'.  This
is both wasteful and wrong because it places an artificial limit on
the line length.  Eliminate the need for the buffer altogether, and
add a test demonstrating this.

[jc: simplify parsing]

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |   37 ++++++++++++++++---------------------
 t/t3510-cherry-pick-sequence.sh |   28 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0c6d3d8..70055e5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
 {
 	unsigned char commit_sha1[20];
-	char sha1_abbrev[40];
 	enum replay_action action;
-	int insn_len = 0;
-	char *p, *q;
+	char *end_of_object_name;
+	int saved, status;
 
-	if (!prefixcmp(start, "pick ")) {
+	if (!prefixcmp(bol, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
-	} else if (!prefixcmp(start, "revert ")) {
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		bol += strlen("revert ");
 	} else
 		return NULL;
 
-	q = strchr(p, ' ');
-	if (!q)
-		return NULL;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
+	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;
 
 	/*
 	 * Verify that the action matches up with the one in
@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 		return NULL;
 	}
 
-	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+	if (status < 0)
 		return NULL;
 
 	return lookup_commit_reference(commit_sha1);
@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	int i;
 
 	for (i = 1; *p; i++) {
-		commit = parse_insn_line(p, opts);
+		char *eol = strchrnul(p, '\n');
+		commit = parse_insn_line(p, eol, opts);
 		if (!commit)
 			return error(_("Could not parse line %d."), i);
 		next = commit_list_append(commit, next);
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
+		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
 		return error(_("No commits parsed."));
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c8..6390f2a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,6 +13,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 --quit &&
 	git checkout -f "$1^0" &&
@@ -328,4 +331,29 @@ 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 &&
+	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.7.3

[PATCH 3/5] revert: simplify getting commit subject in format_todo()

[Ramkumar Ramachandra]

format_todo() calls get_message(), but uses only the subject line of
the commit message.  Save work and unnecessary memory allocations by
using find_commit_subject() instead.  Also, remove the spurious check
on cur->item: the previous patch makes sure that instruction sheets
with missing commit subjects are parsable.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 70055e5..706b8d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,16 +697,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.7.3

[PATCH 4/5] revert: allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all instructions use
the same action.  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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |  136 +++++++++++++++++++--------------------
 sequencer.h                     |    8 ++
 t/t3510-cherry-pick-sequence.sh |   58 +++++++++++++++++
 3 files changed, 133 insertions(+), 69 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 706b8d4..f2bf198 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_REMOVE_STATE,
@@ -73,14 +72,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,
@@ -159,7 +158,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"),
@@ -363,7 +362,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."));
@@ -467,7 +466,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;
@@ -542,7 +542,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;
@@ -583,7 +583,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
 		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->action == REVERT
+		error(action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -637,7 +637,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);
@@ -683,49 +683,55 @@ 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 *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
 {
 	unsigned char commit_sha1[20];
-	enum replay_action action;
 	char *end_of_object_name;
 	int saved, status;
 
 	if (!prefixcmp(bol, "pick ")) {
-		action = CHERRY_PICK;
+		item->action = REPLAY_PICK;
 		bol += strlen("pick ");
 	} else if (!prefixcmp(bol, "revert ")) {
-		action = REVERT;
+		item->action = REPLAY_REVERT;
 		bol += strlen("revert ");
-	} else
-		return NULL;
+	} else {
+		size_t len = strchrnul(bol, '\n') - bol;
+		if (len > 255)
+			len = 255;
+		return error(_("Line %d: Unrecognized action: %.*s"),
+			lineno, (int)len, bol);
+	}
 
 	end_of_object_name = bol + strcspn(bol, " \n");
 	saved = *end_of_object_name;
@@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if (action != 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 (status < 0)
-		return NULL;
+		return error(_("Line %d: Malformed object name: %s"), lineno, bol);
 
-	return lookup_commit_reference(commit_sha1);
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return error(_("Line %d: Not a valid commit: %s"), lineno, bol);
+
+	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++) {
 		char *eol = strchrnul(p, '\n');
-		commit = parse_insn_line(p, eol, opts);
-		if (!commit)
-			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		if (parse_insn_line(p, eol, &item, i) < 0)
+			return -1;
+		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
@@ -771,8 +769,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;
@@ -788,7 +785,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);
@@ -837,18 +834,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)
@@ -946,7 +943,7 @@ fail:
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
@@ -954,7 +951,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);
@@ -998,9 +995,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);
@@ -1010,8 +1008,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)
 				/*
@@ -1036,7 +1034,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);
@@ -1056,7 +1054,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			return error(_("No %s in progress"), action_name(opts));
 		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))
@@ -1074,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)
 	if (create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1)) {
-		if (opts->action == REVERT)
+		if (opts->action == REPLAY_REVERT)
 			return error(_("Can't revert as initial commit"));
 		return error(_("Can't cherry-pick into empty head"));
 	}
@@ -1091,7 +1089,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);
@@ -1106,7 +1104,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 f435fdb..71cfcae 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 6390f2a..d52ad59 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -356,4 +356,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.7.3

[PATCH 5/5] revert: simplify communicating command-line arguments

[Ramkumar Ramachandra]

From: Jonathan Nieder <jrnieder@gmail.com>

Currently, command-line arguments are communicated using (argc, argv)
until a prepare_revs() turns it into a terse structure.  However,
since we plan to expose the cherry-picking machinery through a public
API in the future, we want callers to be able to call in with a
filled-in structure.  For the revert builtin, this means that the
chief argument parser, parse_args(), should parse into such a
structure.  Make this change.

[rr: minor improvements, commit message]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index f2bf198..3b25a0c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -59,13 +59,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"
@@ -168,9 +169,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,
@@ -212,9 +213,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,
@@ -222,7 +220,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 {
@@ -631,23 +642,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"));
 }
 
@@ -837,14 +840,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);
 }
 
@@ -1037,6 +1039,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.7.3

Re: [PATCH 1/5] revert: free msg in format_todo()

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Memory allocated to the fields of msg by get_message() isn't freed.
> This is potentially a big leak,
[because it is in a loop over commits being picked]

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

>  builtin/revert.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1ea525c..0c6d3d8 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -706,6 +706,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;
>  }

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Change the instruction sheet format subtly so that the subject of the
> commit message that follows the object name is optional.  As a result,
> an instruction sheet like this is now perfectly valid:
>
>   pick 35b0426
>   pick fbd5bbcbc2e
>   pick 7362160f
>
> While at it, also fix a bug: currently, we use a commit-id-shaped
> buffer to store the word after "pick" in '.git/sequencer/todo'.  This
> is both wasteful and wrong because it places an artificial limit on
> the line length.  Eliminate the need for the buffer altogether, and
> add a test demonstrating this.
>
> [jc: simplify parsing]

Reading the above does not make it at all obvious that I should want
to apply this patch because otherwise my prankster friend can cause my
copy of git to crash or run arbitrary code by putting a long commit
name in .git/sequencer/todo in our NFS-mounted shared checkout.

(Yes, I know there are other problems with such a setup, especially if
.git/hooks or .git/config is writable by untrusted people.  So it is
not actually a security bug, but a robustness one.)

Re: [PATCH 3/5] revert: simplify getting commit subject in format_todo()

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> format_todo() calls get_message(), but uses only the subject line of
> the commit message.  Save work and unnecessary memory allocations by
> using find_commit_subject() instead.  Also, remove the spurious check
> on cur->item: the previous patch makes sure that instruction sheets
> with missing commit subjects are parsable.

So, what effect will this patch actually have?  Is it an optimization
with no other observable effect?

Re: [PATCH 4/5] revert: allow mixed pick and revert instructions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all instructions use
> the same action.  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.

Good idea.

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

I don't know what this part is about...

[...]
> --- 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 };

Removing the enum, to expose it in sequencer.h.  What does this have
to do with the stated goal of the patch?

>  enum replay_subcommand {
>  	REPLAY_NONE,
>  	REPLAY_REMOVE_STATE,
> @@ -73,14 +72,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";
[... lots of similar changes snipped ...]
> @@ -467,7 +466,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)

Ok, now we're at the title feature.

Passing "action" as a separate parameter in addition to "opts" makes
sense since it can vary from item to item on the todo list.  It
unfortunately makes reviewers' lives hard since it is easy to miss
additional unconverted uses of opts->action in the function and
compilers will not warn about that.

[...]
>		if (index_differs_from("HEAD", 0))
>			return error_dirty_index(opts);
[...]
>	if (parent && parse_commit(parent) < 0)
>		/* TRANSLATORS: The first %s will be "revert" or
>		   "cherry-pick", the second %s a SHA1 */
>		return error(_("%s: cannot parse parent commit %s"),
>			action_name(opts), sha1_to_hex(parent->object.sha1));
[...]
>		res = do_recursive_merge(base, next, base_label, next_label,
>					 head, &msgbuf, opts);

Makes sense.  This is the command name passed on the command line, not
the action name from the todo list, so it is left alone.

[...]
> @@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
[...]
> -		error(opts->action == REVERT
> +		error(action == REPLAY_REVERT
>  		      ? _("could not revert %s... %s")
>  		      : _("could not apply %s... %s"),

... and that's the last use of "action" in this function on the
current master branch.  Good.

[... more distracting s/REVERT/REPLAY_REVERT/ stuff snipped...]
> @@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)
[...]
> -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)

Looks good.

[...]
> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
> +static int parse_insn_line(char *bol, char *eol,
> +			struct replay_insn_list *item, int lineno)
[...]
> -	} else
> -		return NULL;
> +	} else {
> +		size_t len = strchrnul(bol, '\n') - bol;
> +		if (len > 255)
> +			len = 255;
> +		return error(_("Line %d: Unrecognized action: %.*s"),
> +			lineno, (int)len, bol);
> +	}

A new error message when an insn line does not start with "pick" or
"revert".  Looks like a good change, but what does it have to do with
the subject of the patch?

[...]
> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
[...]
>  	status = get_sha1(bol, commit_sha1);
>  	*end_of_object_name = saved;
[...]
>  	if (status < 0)
> -		return NULL;
> +		return error(_("Line %d: Malformed object name: %s"), lineno, bol);

(Not about this patch, but an earlier one)  What is this idiom about?
Why not

	if (get_sha1(bol, commit_sha1))
		return error(_(...));

?

[...]
> -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)

Nice to see.

>  {
> -	struct commit_list **next = todo_list;
> -	struct commit *commit;
> +	struct replay_insn_list **next = todo_list;
> +	struct replay_insn_list item = {0, NULL, NULL};

Style: elsewhere in this file, initializers look like "{ 0, NULL, NULL }"
(with space between the braces and values).

[...]
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '

Thanks for writing these.

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

Stops in "picked", asking user to resolve conflicts.

> +	echo "c" >foo &&
> +	git add foo &&
> +	git commit &&

User resolves conflicts, as in the previous test.

> +	git revert --continue &&

And asks to 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

I wonder if there is some simpler way to check the result of resuming
a cherry-pick sequence, though that doesn't have much to do with the
patch.  I guess I'd find

	echo d >expect &&
	... &&
	# index is clean
	git update-index --refresh &&
	git rev-list HEAD >commits &&
	test_line_count = 4 commits &&
	test_cmp expect foo &&
	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD

a more convincing demonstration that the cherry-pick finished without
incident.

> +'
> +
> +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 &&

Same setup.

> +	oldsha=`git rev-parse --short HEAD~1` &&
> +	echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&

Adding to the insn sheet.  Makes sense.

Summing up: I quite like the idea of this patch, but I would be a lot
happier if changes unrelated to its goal were split off to be
considered separately.

> Acked-by: Jonathan Nieder <jrnider@gmail.com>

No, not acked.

Thanks,
Jonathan

Re: [PATCH 5/5] revert: simplify communicating command-line arguments

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> From: Jonathan Nieder <jrnieder@gmail.com>
>
> Currently, command-line arguments are communicated using (argc, argv)
> until a prepare_revs() turns it into a terse structure.  However,
> since we plan to expose the cherry-picking machinery through a public
> API in the future, we want callers to be able to call in with a
> filled-in structure.  For the revert builtin, this means that the
> chief argument parser, parse_args(), should parse into such a
> structure.  Make this change.

Fine.  I guess it can be said more clearly while emphasizing public
interfaces over implementation details:

	Since the "multiple cherry-pick" feature was introduced (commit
	7e2bfd3f, 2010-07-02), the pick/revert machinery has kept track
	of the set of commits to be cherry-picked or reverted using
	commit_argc and commit_argv variables storing the relevant
	command-line parameters.

	Future callers as other commands are built in (am, rebase,
	sequencer) may find it easier to pass rev-list options to this
	machinery in already-parsed form, though.  So teach
	cmd_cherry_pick and cmd_revert to parse the rev-list arguments
	in advance and pass the commit set to pick_revisions() as a
	value of type "struct rev_info".

[...]
> @@ -222,7 +220,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);
>  }

Looks good.

Tests?  What happens if I try "git cherry-pick --all"?  How about "git
cherry-pick HEAD..HEAD"?

FWIW, with the commit message clarified and with or without new tests,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
> ($gname/186365) implemented.

Thanks for this.  I am worried that some of my comments in previous
reviews (especially about change descriptions) were not addressed,
which could mean one of a few things:

 - you didn't notice them
 - they were wrong
 - you noticed them and they were describing real problems, but it
   wasn't worth the time to fix them

Fine.  But I would like to know which case they fell into, so I can
learn in order to do a better job reviewing the future and know my
time is well spent.

The series makes various changes, all essentially good, which leaves
me all the more motivated to figure out how to get this sort of thing
in smoothly without making life difficult for people in the future
tracking down regressions.

Ciao,
Jonathan

Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this.  I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
>  - you didn't notice them
>  - they were wrong
>  - you noticed them and they were describing real problems, but it
>   wasn't worth the time to fix them
>
> Fine.  But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.

I suppose they were a combination of all three.  I'm sorry- I'll try
to be more careful and communicative in the future.

Thanks for this round of reviews!

-- Ram

Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick

[Jonathan Nieder]

Jonathan Nieder wrote:

> Fine.  But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
                                             ^in
> time is well spent.

Sorry for the nonsense. :)  And now that I look back over previous
revisions of the patches, I see that I _hadn't_ clearly complained
about the same aspects of these particular commit messages before.
So what am I talking about here?

I guess it is just a pattern: commit messages I have looked at in the
sequencer series lately seem to focus too much on implementation
details and not enough on the "big picture" of what the user or
internal API user will experience.  One symptom is that I get lost in
reading the commit message without looking at the patch.  Another
symptom is that the commit messages tend to mention the particular
(private) functions that were changed, instead of the more prominent
(often public) callers that the reader might have cause to call.

Hoping that clarifies a little,
Jonathan

Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> I suppose they were a combination of all three.  I'm sorry- I'll try
> to be more careful and communicative in the future.

Hm, that is not exactly what I was going for, either.  First, I didn't
mean to harp on anything in the past --- it was a request regarding
this current review.  And on the other hand, I am not sure it is a
matter of you being uncareful or uncommunicative, exactly.  It's more
that _I_ am doing a poor job of clearly conveying what Junio described
as easier to learn on one's own at [1].

To put it more concretely:

I am going to be stubborn.  I will not ack a patch unless I can
imagine myself, years from now, after running into a bug in a related
area, not being confused by it.

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

Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this.  I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
>  - you didn't notice them
>  - they were wrong
>  - you noticed them and they were describing real problems, but it
>    wasn't worth the time to fix them
>
> Fine.  But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.

Another thing I often notice after raising an issue during a review (this
is not limited to Ram) is that I do so in the form of a question "why is
this function done this way?" expecting either "ah, doing it that way
instead is simpler and easier to read" or "because of such and such, which
you may have missed. after all this is a tricky codepath with this and
that constraints" as a response. The former should and often does result
in an update of the code in the re-rolled series, but the latter often
results in a frustrating nothing in a re-roll, when the fact that a
reviewer needed to ask the question was a sure sign that the code needs
explanation either in the log message or in-code comment. 

> The series makes various changes, all essentially good, which leaves
> me all the more motivated to figure out how to get this sort of thing
> in smoothly without making life difficult for people in the future
> tracking down regressions.

I looked the series over with your comments and tend to agree with many of
them. Perhaps I should wait for another round before picking it up again
(I've already dropped the old series I kept before 1.7.8).

Re: [PATCH 4/5] revert: allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> 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.
>
> I don't know what this part is about...

I'll separate out these changes in the re-roll.  Thanks.

>> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
> [...]
>>       status = get_sha1(bol, commit_sha1);
>>       *end_of_object_name = saved;
> [...]
>>       if (status < 0)
>> -             return NULL;
>> +             return error(_("Line %d: Malformed object name: %s"), lineno, bol);
>
> (Not about this patch, but an earlier one)  What is this idiom about?
> Why not
>
>        if (get_sha1(bol, commit_sha1))
>                return error(_(...));
>
> ?

I'm first detecting the end of the object name and terminating it with
a '\0', before using using get_sha1() to read out the object name.
Then, I restore the instruction sheet to the original state by
restoring the character from 'saved'.  If I use the idiom you
suggested, I'll leave midway after editing the instruction sheet, and
this is undesirable.

-- Ram

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> [...]
>> While at it, also fix a bug: currently, we use a commit-id-shaped
>> buffer to store the word after "pick" in '.git/sequencer/todo'.  This
>> is both wasteful and wrong because it places an artificial limit on
>> the line length.  Eliminate the need for the buffer altogether, and
>> add a test demonstrating this.
>
> Reading the above does not make it at all obvious that I should want
> to apply this patch because otherwise my prankster friend can cause my
> copy of git to crash or run arbitrary code by putting a long commit

Working backwards:
get_sha1() is what will finally misbehave: how?  It uses strlen() and
let's assume that the number returned by it is too big to fit in a
size_t.  Surely, this means that we should only use get_sha1() on
something whose length is bounded.  So, do we ever try to get to the
end of the line?  Yes!  Let's assume that the problem starts when
end_of_object_name calls strcspn which returns something too big to
fit in a size_t.  From the manpage it has no standard way of reporting
failure.  I'm not sure what to do; I think I have two choices:
1. Implement the strscpn() using two strchrnul() calls.
2. Drop this patch and use strbuf to replace the fixed-size buffer.

I think I'll go with the second option.  What do you think?

-- Ram

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

Hi,

Ramkumar Ramachandra wrote:
> 1. Implement the strscpn() using two strchrnul() calls.
> 2. Drop this patch and use strbuf to replace the fixed-size buffer.
>
> I think I'll go with the second option.  What do you think?
> [...]


Er, I have an 'eol' which I'm not using.  Making sure that 'eol' is
valid is good enough to avoid unexpected failures: if strchrnul() is
able to find a '\n', strcspn() should have no trouble finding either a
' ' or '\n' (whichever comes first).

Sorry for the nonsense.

-- Ram

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:

>>> [...]
>>> While at it, also fix a bug: currently, we use a commit-id-shaped
>>> buffer to store the word after "pick" in '.git/sequencer/todo'.  This
>>> is both wasteful and wrong because it places an artificial limit on
>>> the line length.  Eliminate the need for the buffer altogether, and
>>> add a test demonstrating this.
>>
>> Reading the above does not make it at all obvious that I should want
>> to apply this patch because otherwise my prankster friend can cause my
>> copy of git to crash or run arbitrary code by putting a long commit
>
> Working backwards:
> get_sha1() is what will finally misbehave: how?

Not sure what you're talking about.  I was saying that any commit that
goes from "git can segfault in such-and-such circumstance" to "git no
longer segfaults in that circumstance" should loudly proclaim so!
Otherwise, when my script is causing git to segfault, I will not know
which commits to point my risk-averse sysadmin to when advocating
upgrading our copy of git.

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
>>> Ramkumar Ramachandra wrote:
>
>>>> [...]
>>>> While at it, also fix a bug: currently, we use a commit-id-shaped
>>>> buffer to store the word after "pick" in '.git/sequencer/todo'.  This
>>>> is both wasteful and wrong because it places an artificial limit on
>>>> the line length.  Eliminate the need for the buffer altogether, and
>>>> add a test demonstrating this.
>>>
>>> Reading the above does not make it at all obvious that I should want
>>> to apply this patch because otherwise my prankster friend can cause my
>>> copy of git to crash or run arbitrary code by putting a long commit
>>
>> Working backwards:
>> get_sha1() is what will finally misbehave: how?
>
> Not sure what you're talking about.  I was saying that any commit that
> goes from "git can segfault in such-and-such circumstance" to "git no
> longer segfaults in that circumstance" should loudly proclaim so!

That's what I'm confused about: how can I make git misbehave (let
alone segfault) after applying this commit?  In the previous message,
I was trying to work backwards to figure that out -- I clearly didn't
get anywhere.  In other words: what happens if an overly long (a
length that doesn't fit into a size_t) line is present?  How does
strchrnul() behave?  How do we work around this without imposing some
kind of artificial hard limit?

Thanks.

-- Ram

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>> Not sure what you're talking about.  I was saying that any commit that
>> goes from "git can segfault in such-and-such circumstance" to "git no
>> longer segfaults in that circumstance" should loudly proclaim so!
>
> That's what I'm confused about: how can I make git misbehave (let
> alone segfault) after applying this commit?

You can make git misbehave before applying the commit.  Applying the
patch fixes it.  Unfortunately the patch description does not actually
say so, and at least my small mind is not able to read between the
lines in a hurry to deduce it.

Sorry for the lack of clarity,
Jonathan

Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

Hey,

Jonathan Nieder wrote:
> You can make git misbehave before applying the commit.

Right, it's all in the discussion surrounding $gmane/184031.  I
couldn't recall because it was so long ago :\

Thanks.

-- Ram