[PATCH 0/3] cherry-pick: improvments

[PATCH 0/3] cherry-pick: improvments

[Felipe Contreras]

Hi,

Here's a bunch of changes to make cherry-pick (and revert) more useful.

Felipe Contreras (3):
  cherry-pick: add support to copy notes
  revert/cherry-pick: add --quiet option
  revert/cherry-pick: add --skip option

 Documentation/git-cherry-pick.txt |   7 +-
 Documentation/git-revert.txt      |   7 +-
 Documentation/sequencer.txt       |   3 +
 builtin/revert.c                  |   9 +++
 sequencer.c                       | 162 +++++++++++++++++++++++++++++++++++++-
 sequencer.h                       |   6 +-
 t/t3500-cherry.sh                 |  32 ++++++++
 t/t3510-cherry-pick-sequence.sh   |  12 +++
 8 files changed, 232 insertions(+), 6 deletions(-)

-- 
1.8.3.rc3.312.g47657de

[PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/revert.c  |   2 +
 sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 sequencer.h       |   2 +
 t/t3500-cherry.sh |  32 +++++++++++++
 4 files changed, 169 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0e5ce71..b977124 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -119,6 +119,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
@@ -129,6 +130,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
 			OPT_BOOLEAN(0, "skip-empty", &opts->skip_empty, N_("skip empty commits")),
+			OPT_BOOLEAN(0, "copy-notes", &opts->copy_notes, N_("copy notes")),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
diff --git a/sequencer.c b/sequencer.c
index edf141d..35a84ad 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,18 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
+struct rewritten_list_item {
+	unsigned char from[20];
+	unsigned char to[20];
+};
+
+struct rewritten_list {
+	struct rewritten_list_item *items;
+	unsigned int nr, alloc;
+};
+
+static struct rewritten_list rewritten;
+
 static int is_rfc2822_line(const char *buf, int len)
 {
 	int i;
@@ -102,6 +114,40 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	return 1;
 }
 
+static void add_rewritten(unsigned char *from, unsigned char *to)
+{
+	struct rewritten_list_item *item;
+	if (rewritten.nr + 1 >= rewritten.alloc) {
+		rewritten.alloc += 32;
+		rewritten.items = xrealloc(rewritten.items, rewritten.alloc * sizeof(*rewritten.items));
+	}
+	item = &rewritten.items[rewritten.nr];
+	hashcpy(item->from, from);
+	hashcpy(item->to, to);
+	rewritten.nr++;
+}
+
+static void copy_notes(void)
+{
+	unsigned char note_commit[20];
+	struct strbuf buf = STRBUF_INIT;
+	struct notes_tree *t = &default_notes_tree;
+	int i;
+
+	init_notes(t, NULL, NULL, 0);
+
+	for (i = 0; i < rewritten.nr; i++) {
+		struct rewritten_list_item *item = &rewritten.items[i];
+		copy_note(t, item->from, item->to, 0, NULL);
+	}
+
+	strbuf_addstr(&buf, "Notes added by 'git cherry-pick'\n");
+	create_notes_commit(&default_notes_tree, NULL, &buf, note_commit);
+	strbuf_insert(&buf, 0, "notes: ", 7);
+	update_ref(buf.buf, t->ref, note_commit, NULL, 0, MSG_ON_ERR);
+	strbuf_release(&buf);
+}
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -641,6 +687,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			res = run_git_commit(defmsg, opts, allow);
 	}
 
+	if (!res && opts->copy_notes) {
+		unsigned char to[20];
+
+		if (read_ref("HEAD", to))
+			goto leave;
+
+		add_rewritten(commit->object.sha1, to);
+	}
 leave:
 	free_message(&msg);
 	free(defmsg);
@@ -786,6 +840,40 @@ static void read_populate_todo(struct commit_list **todo_list,
 		die(_("Unusable instruction sheet: %s"), todo_file);
 }
 
+static void read_populate_rewritten(void)
+{
+	const char *rewritten_file = git_path(SEQ_REWR_FILE);
+	struct strbuf buf = STRBUF_INIT;
+	char *p;
+	int fd;
+
+	fd = open(rewritten_file, O_RDONLY);
+	if (fd < 0)
+		return;
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		return;
+	}
+	close(fd);
+
+	for (p = buf.buf; *p;) {
+		unsigned char from[20];
+		unsigned char to[20];
+		char *eol = strchrnul(p, '\n');
+		if (eol - p != 81)
+			/* wrong size */
+			break;
+		if (get_sha1_hex(p, from))
+			break;
+		if (get_sha1_hex(p + 41, to))
+			break;
+		add_rewritten(from, to);
+		p = *eol ? eol + 1 : eol;
+	}
+	strbuf_release(&buf);
+}
+
 static int populate_opts_cb(const char *key, const char *value, void *data)
 {
 	struct replay_opts *opts = data;
@@ -810,7 +898,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	else if (!strcmp(key, "options.strategy-option")) {
 		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
 		opts->xopts[opts->xopts_nr++] = xstrdup(value);
-	} else
+	}
+	else if (!strcmp(key, "options.copy-notes"))
+		opts->copy_notes = git_config_bool_or_int(key, value, &error_flag);
+	else
 		return error(_("Invalid key: %s"), key);
 
 	if (!error_flag)
@@ -958,6 +1049,29 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	strbuf_release(&buf);
 }
 
+static void save_rewritten(void)
+{
+	const char *todo_file = git_path(SEQ_REWR_FILE);
+	static struct lock_file rewritten_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd, i;
+
+	fd = hold_lock_file_for_update(&rewritten_lock, todo_file, LOCK_DIE_ON_ERROR);
+	for (i = 0; i < rewritten.nr; i++) {
+		struct rewritten_list_item *item = &rewritten.items[i];
+		strbuf_addf(&buf, "%s %s\n", sha1_to_hex(item->from), sha1_to_hex(item->to));
+	}
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s"), todo_file);
+	}
+	if (commit_lock_file(&rewritten_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s."), todo_file);
+	}
+	strbuf_release(&buf);
+}
+
 static void save_opts(struct replay_opts *opts)
 {
 	const char *opts_file = git_path(SEQ_OPTS_FILE);
@@ -987,6 +1101,8 @@ static void save_opts(struct replay_opts *opts)
 							"options.strategy-option",
 							opts->xopts[i], "^$", 0);
 	}
+	if (opts->copy_notes)
+		git_config_set_in_file(opts_file, "options.copy-notes", "true");
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
@@ -1003,10 +1119,16 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
-		if (res)
+		if (res) {
+			if (opts->copy_notes)
+				save_rewritten();
 			return res;
+		}
 	}
 
+	if (opts->copy_notes)
+		copy_notes();
+
 	/*
 	 * Sequence of picks finished successfully; cleanup by
 	 * removing the .git/sequencer directory
@@ -1033,6 +1155,8 @@ static int sequencer_continue(struct replay_opts *opts)
 		return continue_single_pick();
 	read_populate_opts(&opts);
 	read_populate_todo(&todo_list, opts);
+	if (opts->copy_notes)
+		read_populate_rewritten();
 
 	/* Verify that the conflict has been resolved */
 	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
@@ -1049,8 +1173,14 @@ static int sequencer_continue(struct replay_opts *opts)
 
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
+	int ret;
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	return do_pick_commit(cmit, opts);
+	ret = do_pick_commit(cmit, opts);
+	if (ret)
+		return ret;
+	if (opts->copy_notes)
+		copy_notes();
+	return 0;
 }
 
 int sequencer_pick_revisions(struct replay_opts *opts)
diff --git a/sequencer.h b/sequencer.h
index 3b04844..6cc072c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -5,6 +5,7 @@
 #define SEQ_HEAD_FILE	"sequencer/head"
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
+#define SEQ_REWR_FILE	"sequencer/rewritten"
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
@@ -35,6 +36,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int skip_empty;
+	int copy_notes;
 
 	int mainline;
 
diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
index f038f34..79c1219 100755
--- a/t/t3500-cherry.sh
+++ b/t/t3500-cherry.sh
@@ -55,4 +55,36 @@ test_expect_success \
      expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
 '
 
+test_expect_success \
+    'copy notes' \
+    'git checkout master &&
+    echo notes > C &&
+    test_tick &&
+    git commit -a -m "Update C" &&
+    git notes add -m "a note" &&
+    git checkout -b note-test HEAD^ &&
+    git cherry-pick --copy-notes -x master &&
+    test "a note" = "$(git notes show HEAD)"
+'
+
+test_expect_success \
+    'copy multiple notes' \
+    'git checkout master &&
+    echo "multiple notes" > C &&
+    git commit -a -m "Update C again" &&
+    git notes add -m "another note" &&
+    git commit -a -m "Empty" --allow-empty &&
+    echo "more notes" > C &&
+    git commit -a -m "Update C once more" &&
+    git notes add -m "final note" &&
+    git checkout note-test &&
+    git reset --hard master~3 &&
+    test_expect_code 1 git cherry-pick --copy-notes -x HEAD..master &&
+    git reset --hard &&
+    git cherry-pick --continue
+    test "a note" = "$(git notes show HEAD~2)" &&
+    test "another note" = "$(git notes show HEAD~1)" &&
+    test "final note" = "$(git notes show HEAD)"
+'
+
 test_done
-- 
1.8.3.rc3.312.g47657de

[PATCH 2/3] revert/cherry-pick: add --quiet option

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-cherry-pick.txt | 6 +++++-
 Documentation/git-revert.txt      | 6 +++++-
 builtin/revert.c                  | 1 +
 sequencer.c                       | 2 ++
 sequencer.h                       | 1 +
 5 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index fccd936..da0bd81 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -8,7 +8,7 @@ git-cherry-pick - Apply the changes introduced by some existing commits
 SYNOPSIS
 --------
 [verse]
-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
+'git cherry-pick' [-q] [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
 'git cherry-pick' --continue
 'git cherry-pick' --quit
 'git cherry-pick' --abort
@@ -51,6 +51,10 @@ OPTIONS
 	feed all <commit>... arguments to a single revision walk
 	(see a later example that uses 'maint master..next').
 
+-q::
+--quiet::
+	Quiet, suppress feedback messages.
+
 -e::
 --edit::
 	With this option, 'git cherry-pick' will let you edit the commit
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index f79c9d8..98a8e7a 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -8,7 +8,7 @@ git-revert - Revert some existing commits
 SYNOPSIS
 --------
 [verse]
-'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
+'git revert' [-q] [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
 'git revert' --continue
 'git revert' --quit
 'git revert' --abort
@@ -40,6 +40,10 @@ OPTIONS
 	default, see linkgit:git-rev-list[1] and its '--no-walk'
 	option.
 
+-q::
+--quiet::
+	Quiet, suppress feedback messages.
+
 -e::
 --edit::
 	With this option, 'git revert' will let you edit the commit
diff --git a/builtin/revert.c b/builtin/revert.c
index b977124..d63b4a6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -100,6 +100,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	int contin = 0;
 	int rollback = 0;
 	struct option options[] = {
+		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
 		OPT_BOOLEAN(0, "quit", &remove_state, N_("end revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "continue", &contin, N_("resume revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "abort", &rollback, N_("cancel revert or cherry-pick sequence")),
diff --git a/sequencer.c b/sequencer.c
index 35a84ad..b4e395a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -435,6 +435,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_init(&array);
 	argv_array_push(&array, "commit");
 	argv_array_push(&array, "-n");
+	if (opts->quiet)
+		argv_array_push(&array, "-q");
 
 	if (opts->signoff)
 		argv_array_push(&array, "-s");
diff --git a/sequencer.h b/sequencer.h
index 6cc072c..41e19d0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -37,6 +37,7 @@ struct replay_opts {
 	int keep_redundant_commits;
 	int skip_empty;
 	int copy_notes;
+	int quiet;
 
 	int mainline;
 
-- 
1.8.3.rc3.312.g47657de

[PATCH 3/3] revert/cherry-pick: add --skip option

[Felipe Contreras]

Akin to 'am --skip' and 'rebase --skip'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-cherry-pick.txt |  1 +
 Documentation/git-revert.txt      |  1 +
 Documentation/sequencer.txt       |  3 +++
 builtin/revert.c                  |  6 ++++++
 sequencer.c                       | 24 ++++++++++++++++++++++++
 sequencer.h                       |  3 ++-
 t/t3510-cherry-pick-sequence.sh   | 12 ++++++++++++
 7 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index da0bd81..d95c63c 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git cherry-pick' [-q] [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
 'git cherry-pick' --continue
+'git cherry-pick' --skip
 'git cherry-pick' --quit
 'git cherry-pick' --abort
 
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 98a8e7a..52e146e 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git revert' [-q] [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
 'git revert' --continue
+'git revert' --skip
 'git revert' --quit
 'git revert' --abort
 
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 5747f44..df2d355 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -3,6 +3,9 @@
 	'.git/sequencer'.  Can be used to continue after resolving
 	conflicts in a failed cherry-pick or revert.
 
+--skip::
+	Skip the current commit, and then continue.
+
 --quit::
 	Forget about the current operation in progress.  Can be used
 	to clear the sequencer state after a failed cherry-pick or
diff --git a/builtin/revert.c b/builtin/revert.c
index d63b4a6..6afd990 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -99,11 +99,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	int remove_state = 0;
 	int contin = 0;
 	int rollback = 0;
+	int skip = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
 		OPT_BOOLEAN(0, "quit", &remove_state, N_("end revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "continue", &contin, N_("resume revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "abort", &rollback, N_("cancel revert or cherry-pick sequence")),
+		OPT_BOOLEAN(0, "skip", &skip, N_("skip current commit in the sequence")),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOLEAN('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
@@ -160,6 +162,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		opts->subcommand = REPLAY_CONTINUE;
 	else if (rollback)
 		opts->subcommand = REPLAY_ROLLBACK;
+	else if (skip)
+		opts->subcommand = REPLAY_SKIP;
 	else
 		opts->subcommand = REPLAY_NONE;
 
@@ -170,6 +174,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			this_operation = "--quit";
 		else if (opts->subcommand == REPLAY_CONTINUE)
 			this_operation = "--continue";
+		else if (opts->subcommand == REPLAY_SKIP)
+			this_operation = "--skip";
 		else {
 			assert(opts->subcommand == REPLAY_ROLLBACK);
 			this_operation = "--abort";
diff --git a/sequencer.c b/sequencer.c
index b4e395a..971cab2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1173,6 +1173,28 @@ static int sequencer_continue(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
+static int sequencer_skip(struct replay_opts *opts)
+{
+	const char *argv[4]; /* reset --hard HEAD + NULL */
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+	int ret;
+
+	if (setup_rerere(&merge_rr, 0) >= 0) {
+		rerere_clear(&merge_rr);
+		string_list_clear(&merge_rr, 1);
+	}
+
+	argv[0] = "reset";
+	argv[1] = "--hard";
+	argv[2] = "HEAD";
+	argv[3] = NULL;
+	ret = run_command_v_opt(argv, RUN_GIT_CMD);
+	if (ret)
+		return ret;
+
+	return sequencer_continue(opts);
+}
+
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
 	int ret;
@@ -1209,6 +1231,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return sequencer_rollback(opts);
 	if (opts->subcommand == REPLAY_CONTINUE)
 		return sequencer_continue(opts);
+	if (opts->subcommand == REPLAY_SKIP)
+		return sequencer_skip(opts);
 
 	for (i = 0; i < opts->revs->pending.nr; i++) {
 		unsigned char sha1[20];
diff --git a/sequencer.h b/sequencer.h
index 41e19d0..b9e09ac 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -18,7 +18,8 @@ enum replay_subcommand {
 	REPLAY_NONE,
 	REPLAY_REMOVE_STATE,
 	REPLAY_CONTINUE,
-	REPLAY_ROLLBACK
+	REPLAY_ROLLBACK,
+	REPLAY_SKIP
 };
 
 struct replay_opts {
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89d..f58a83d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -511,4 +511,16 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+test_expect_success 'skip' '
+	pristine_detach conflicting &&
+	test_must_fail git cherry-pick initial..picked &&
+
+	git checkout HEAD -- unrelated &&
+	test_must_fail git cherry-pick --continue &&
+	git cherry-pick --skip &&
+
+	git rev-list initial..HEAD >commits &&
+	test_line_count = 3 commits
+'
+
 test_done
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/revert.c  |   2 +
>  sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  sequencer.h       |   2 +
>  t/t3500-cherry.sh |  32 +++++++++++++
>  4 files changed, 169 insertions(+), 3 deletions(-)

"git cherry-pick" should help maintaining notes just like amend and
rebase, but how should this interact with notes.rewrite.<command>,
where the command is capable of doing this without an explicit
option once you tell which notes need to be maintained?

Thomas Rast Cc'ed as he has been the primary force behind this line
of "notes" usability.

It probably is not sensible to carry over note from a reverted
commit to its revert, but I didn't immediately spot how the patch
does this only for cherry-pick but not for revert (the codepath in
do_pick_commit() is shared between them, no?).

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 0e5ce71..b977124 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -119,6 +119,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  		OPT_END(),
>  		OPT_END(),
>  		OPT_END(),
> +		OPT_END(),
>  	};
>  
>  	if (opts->action == REPLAY_PICK) {
> @@ -129,6 +130,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
>  			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
>  			OPT_BOOLEAN(0, "skip-empty", &opts->skip_empty, N_("skip empty commits")),
> +			OPT_BOOLEAN(0, "copy-notes", &opts->copy_notes, N_("copy notes")),
>  			OPT_END(),
>  		};
>  		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
> diff --git a/sequencer.c b/sequencer.c
> index edf141d..35a84ad 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -20,6 +20,18 @@
>  const char sign_off_header[] = "Signed-off-by: ";
>  static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>  
> +struct rewritten_list_item {
> +	unsigned char from[20];
> +	unsigned char to[20];
> +};
> +
> +struct rewritten_list {
> +	struct rewritten_list_item *items;
> +	unsigned int nr, alloc;
> +};
> +
> +static struct rewritten_list rewritten;
> +
>  static int is_rfc2822_line(const char *buf, int len)
>  {
>  	int i;
> @@ -102,6 +114,40 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	return 1;
>  }
>  
> +static void add_rewritten(unsigned char *from, unsigned char *to)
> +{
> +	struct rewritten_list_item *item;
> +	if (rewritten.nr + 1 >= rewritten.alloc) {
> +		rewritten.alloc += 32;
> +		rewritten.items = xrealloc(rewritten.items, rewritten.alloc * sizeof(*rewritten.items));
> +	}
> +	item = &rewritten.items[rewritten.nr];
> +	hashcpy(item->from, from);
> +	hashcpy(item->to, to);
> +	rewritten.nr++;
> +}
> +
> +static void copy_notes(void)
> +{
> +	unsigned char note_commit[20];
> +	struct strbuf buf = STRBUF_INIT;
> +	struct notes_tree *t = &default_notes_tree;
> +	int i;
> +
> +	init_notes(t, NULL, NULL, 0);
> +
> +	for (i = 0; i < rewritten.nr; i++) {
> +		struct rewritten_list_item *item = &rewritten.items[i];
> +		copy_note(t, item->from, item->to, 0, NULL);
> +	}
> +
> +	strbuf_addstr(&buf, "Notes added by 'git cherry-pick'\n");
> +	create_notes_commit(&default_notes_tree, NULL, &buf, note_commit);
> +	strbuf_insert(&buf, 0, "notes: ", 7);
> +	update_ref(buf.buf, t->ref, note_commit, NULL, 0, MSG_ON_ERR);
> +	strbuf_release(&buf);
> +}
> +
>  static void remove_sequencer_state(void)
>  {
>  	struct strbuf seq_dir = STRBUF_INIT;
> @@ -641,6 +687,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  			res = run_git_commit(defmsg, opts, allow);
>  	}
>  
> +	if (!res && opts->copy_notes) {
> +		unsigned char to[20];
> +
> +		if (read_ref("HEAD", to))
> +			goto leave;
> +
> +		add_rewritten(commit->object.sha1, to);
> +	}
>  leave:
>  	free_message(&msg);
>  	free(defmsg);
> @@ -786,6 +840,40 @@ static void read_populate_todo(struct commit_list **todo_list,
>  		die(_("Unusable instruction sheet: %s"), todo_file);
>  }
>  
> +static void read_populate_rewritten(void)
> +{
> +	const char *rewritten_file = git_path(SEQ_REWR_FILE);
> +	struct strbuf buf = STRBUF_INIT;
> +	char *p;
> +	int fd;
> +
> +	fd = open(rewritten_file, O_RDONLY);
> +	if (fd < 0)
> +		return;
> +	if (strbuf_read(&buf, fd, 0) < 0) {
> +		close(fd);
> +		strbuf_release(&buf);
> +		return;
> +	}
> +	close(fd);
> +
> +	for (p = buf.buf; *p;) {
> +		unsigned char from[20];
> +		unsigned char to[20];
> +		char *eol = strchrnul(p, '\n');
> +		if (eol - p != 81)
> +			/* wrong size */
> +			break;
> +		if (get_sha1_hex(p, from))
> +			break;
> +		if (get_sha1_hex(p + 41, to))
> +			break;
> +		add_rewritten(from, to);
> +		p = *eol ? eol + 1 : eol;
> +	}
> +	strbuf_release(&buf);
> +}
> +
>  static int populate_opts_cb(const char *key, const char *value, void *data)
>  {
>  	struct replay_opts *opts = data;
> @@ -810,7 +898,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  	else if (!strcmp(key, "options.strategy-option")) {
>  		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
>  		opts->xopts[opts->xopts_nr++] = xstrdup(value);
> -	} else
> +	}
> +	else if (!strcmp(key, "options.copy-notes"))
> +		opts->copy_notes = git_config_bool_or_int(key, value, &error_flag);
> +	else
>  		return error(_("Invalid key: %s"), key);
>  
>  	if (!error_flag)
> @@ -958,6 +1049,29 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>  	strbuf_release(&buf);
>  }
>  
> +static void save_rewritten(void)
> +{
> +	const char *todo_file = git_path(SEQ_REWR_FILE);
> +	static struct lock_file rewritten_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd, i;
> +
> +	fd = hold_lock_file_for_update(&rewritten_lock, todo_file, LOCK_DIE_ON_ERROR);
> +	for (i = 0; i < rewritten.nr; i++) {
> +		struct rewritten_list_item *item = &rewritten.items[i];
> +		strbuf_addf(&buf, "%s %s\n", sha1_to_hex(item->from), sha1_to_hex(item->to));
> +	}
> +	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> +		strbuf_release(&buf);
> +		die_errno(_("Could not write to %s"), todo_file);
> +	}
> +	if (commit_lock_file(&rewritten_lock) < 0) {
> +		strbuf_release(&buf);
> +		die(_("Error wrapping up %s."), todo_file);
> +	}
> +	strbuf_release(&buf);
> +}
> +
>  static void save_opts(struct replay_opts *opts)
>  {
>  	const char *opts_file = git_path(SEQ_OPTS_FILE);
> @@ -987,6 +1101,8 @@ static void save_opts(struct replay_opts *opts)
>  							"options.strategy-option",
>  							opts->xopts[i], "^$", 0);
>  	}
> +	if (opts->copy_notes)
> +		git_config_set_in_file(opts_file, "options.copy-notes", "true");
>  }
>  
>  static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
> @@ -1003,10 +1119,16 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>  	for (cur = todo_list; cur; cur = cur->next) {
>  		save_todo(cur, opts);
>  		res = do_pick_commit(cur->item, opts);
> -		if (res)
> +		if (res) {
> +			if (opts->copy_notes)
> +				save_rewritten();
>  			return res;
> +		}
>  	}
>  
> +	if (opts->copy_notes)
> +		copy_notes();
> +
>  	/*
>  	 * Sequence of picks finished successfully; cleanup by
>  	 * removing the .git/sequencer directory
> @@ -1033,6 +1155,8 @@ static int sequencer_continue(struct replay_opts *opts)
>  		return continue_single_pick();
>  	read_populate_opts(&opts);
>  	read_populate_todo(&todo_list, opts);
> +	if (opts->copy_notes)
> +		read_populate_rewritten();
>  
>  	/* Verify that the conflict has been resolved */
>  	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
> @@ -1049,8 +1173,14 @@ static int sequencer_continue(struct replay_opts *opts)
>  
>  static int single_pick(struct commit *cmit, struct replay_opts *opts)
>  {
> +	int ret;
>  	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> -	return do_pick_commit(cmit, opts);
> +	ret = do_pick_commit(cmit, opts);
> +	if (ret)
> +		return ret;
> +	if (opts->copy_notes)
> +		copy_notes();
> +	return 0;
>  }
>  
>  int sequencer_pick_revisions(struct replay_opts *opts)
> diff --git a/sequencer.h b/sequencer.h
> index 3b04844..6cc072c 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -5,6 +5,7 @@
>  #define SEQ_HEAD_FILE	"sequencer/head"
>  #define SEQ_TODO_FILE	"sequencer/todo"
>  #define SEQ_OPTS_FILE	"sequencer/opts"
> +#define SEQ_REWR_FILE	"sequencer/rewritten"
>  
>  #define APPEND_SIGNOFF_DEDUP (1u << 0)
>  
> @@ -35,6 +36,7 @@ struct replay_opts {
>  	int allow_empty_message;
>  	int keep_redundant_commits;
>  	int skip_empty;
> +	int copy_notes;
>  
>  	int mainline;
>  
> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34..79c1219 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh
> @@ -55,4 +55,36 @@ test_expect_success \
>       expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
>  '
>  
> +test_expect_success \
> +    'copy notes' \
> +    'git checkout master &&
> +    echo notes > C &&
> +    test_tick &&
> +    git commit -a -m "Update C" &&
> +    git notes add -m "a note" &&
> +    git checkout -b note-test HEAD^ &&
> +    git cherry-pick --copy-notes -x master &&
> +    test "a note" = "$(git notes show HEAD)"
> +'
> +
> +test_expect_success \
> +    'copy multiple notes' \
> +    'git checkout master &&
> +    echo "multiple notes" > C &&
> +    git commit -a -m "Update C again" &&
> +    git notes add -m "another note" &&
> +    git commit -a -m "Empty" --allow-empty &&
> +    echo "more notes" > C &&
> +    git commit -a -m "Update C once more" &&
> +    git notes add -m "final note" &&
> +    git checkout note-test &&
> +    git reset --hard master~3 &&
> +    test_expect_code 1 git cherry-pick --copy-notes -x HEAD..master &&
> +    git reset --hard &&
> +    git cherry-pick --continue
> +    test "a note" = "$(git notes show HEAD~2)" &&
> +    test "another note" = "$(git notes show HEAD~1)" &&
> +    test "final note" = "$(git notes show HEAD)"
> +'
> +
>  test_done

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Thomas Rast]

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

> Thomas Rast Cc'ed as he has been the primary force behind this line
> of "notes" usability.

Thanks for pointing this out to me.

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin/revert.c  |   2 +
>>  sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  sequencer.h       |   2 +
>>  t/t3500-cherry.sh |  32 +++++++++++++
>>  4 files changed, 169 insertions(+), 3 deletions(-)
>
> "git cherry-pick" should help maintaining notes just like amend and
> rebase, but how should this interact with notes.rewrite.<command>,
> where the command is capable of doing this without an explicit
> option once you tell which notes need to be maintained?

Since we already have the notes.rewrite.<command> convention, it would
seem the obvious choice to line it up with the others.  The main
bikeshedding opportunity is whether this should be an exception and
default to false (all other commands default it to true).

Also: how does this interact with notes.rewriteRef and the corresponding
env vars?  Why?

How does it interact with 'cherry-pick -n' if this is done in sequence,
effectively squashing several commits (this use-case is actually
suggested by the manpage), if multiple source commits had notes?  Should
it respect notes.rewriteMode (and by default concatenate)?  (I don't
know if the sequencer state is expressive enough already to carry this
in a meaningful way across cherry-pick commands.)

A commit message and some docs would be a nice idea, too.

> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34..79c1219 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh

This file starts out with

  test_description='git cherry should detect patches integrated upstream

  This test cherry-picks one local change of two into master branch, and
  checks that git cherry only returns the second patch in the local branch
  '

So either your tests should go to a different file or the description
becomes stale and needs to be updated.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/revert.c  |   2 +
> >  sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  sequencer.h       |   2 +
> >  t/t3500-cherry.sh |  32 +++++++++++++
> >  4 files changed, 169 insertions(+), 3 deletions(-)
> 
> "git cherry-pick" should help maintaining notes just like amend and
> rebase, but how should this interact with notes.rewrite.<command>,
> where the command is capable of doing this without an explicit
> option once you tell which notes need to be maintained?
> 
> Thomas Rast Cc'ed as he has been the primary force behind this line
> of "notes" usability.
> 
> It probably is not sensible to carry over note from a reverted
> commit to its revert, but I didn't immediately spot how the patch
> does this only for cherry-pick but not for revert (the codepath in
> do_pick_commit() is shared between them, no?).

The option is enabled from the UI, and only 'git cherry-pick' has the
--copy-notes option.

-- 
Felipe Contreras

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

Thomas Rast wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Thomas Rast Cc'ed as he has been the primary force behind this line
> > of "notes" usability.
> 
> Thanks for pointing this out to me.
> 
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  builtin/revert.c  |   2 +
> >>  sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >>  sequencer.h       |   2 +
> >>  t/t3500-cherry.sh |  32 +++++++++++++
> >>  4 files changed, 169 insertions(+), 3 deletions(-)
> >
> > "git cherry-pick" should help maintaining notes just like amend and
> > rebase, but how should this interact with notes.rewrite.<command>,
> > where the command is capable of doing this without an explicit
> > option once you tell which notes need to be maintained?
> 
> Since we already have the notes.rewrite.<command> convention, it would
> seem the obvious choice to line it up with the others.  The main
> bikeshedding opportunity is whether this should be an exception and
> default to false (all other commands default it to true).
> 
> Also: how does this interact with notes.rewriteRef and the corresponding
> env vars?  Why?
> 
> How does it interact with 'cherry-pick -n' if this is done in sequence,
> effectively squashing several commits (this use-case is actually
> suggested by the manpage), if multiple source commits had notes?  Should
> it respect notes.rewriteMode (and by default concatenate)?  (I don't
> know if the sequencer state is expressive enough already to carry this
> in a meaningful way across cherry-pick commands.)

Feel free to implement that. I'm just interested in 'git cherry-pick' being
usable for 'git rebase' purposes.

-- 
Felipe Contreras

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Thomas Rast wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > Thomas Rast Cc'ed as he has been the primary force behind this line
>> > of "notes" usability.
>> 
>> Thanks for pointing this out to me.
>> 
>> > Felipe Contreras <felipe.contreras@gmail.com> writes:
>> >
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> ---
>> >>  builtin/revert.c  |   2 +
>> >>  sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> >>  sequencer.h       |   2 +
>> >>  t/t3500-cherry.sh |  32 +++++++++++++
>> >>  4 files changed, 169 insertions(+), 3 deletions(-)
>> >
>> > "git cherry-pick" should help maintaining notes just like amend and
>> > rebase, but how should this interact with notes.rewrite.<command>,
>> > where the command is capable of doing this without an explicit
>> > option once you tell which notes need to be maintained?
>> 
>> Since we already have the notes.rewrite.<command> convention, it would
>> seem the obvious choice to line it up with the others.  The main
>> bikeshedding opportunity is whether this should be an exception and
>> default to false (all other commands default it to true).
>> 
>> Also: how does this interact with notes.rewriteRef and the corresponding
>> env vars?  Why?
>> 
>> How does it interact with 'cherry-pick -n' if this is done in sequence,
>> effectively squashing several commits (this use-case is actually
>> suggested by the manpage), if multiple source commits had notes?  Should
>> it respect notes.rewriteMode (and by default concatenate)?  (I don't
>> know if the sequencer state is expressive enough already to carry this
>> in a meaningful way across cherry-pick commands.)
>
> Feel free to implement that. I'm just interested in 'git cherry-pick' being
> usable for 'git rebase' purposes.

Which would have been obvious to all but the most casual readers, eh?

We've been over this already:

  The body should provide a meaningful commit message, which:

    . explains the problem the change tries to solve, iow, what is wrong
      with the current code without the change.

    . justifies the way the change solves the problem, iow, why the
      result with the change is better.

    . alternate solutions considered but discarded, if any.

I'll gladly review your patches again once you have done that.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

On Wed, May 29, 2013 at 3:09 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

>> Feel free to implement that. I'm just interested in 'git cherry-pick' being
>> usable for 'git rebase' purposes.
>
> Which would have been obvious to all but the most casual readers, eh?

My motivations are irrelevant, the patch is good as it is.

> We've been over this already:
>
>   The body should provide a meaningful commit message, which:
>
>     . explains the problem the change tries to solve, iow, what is wrong
>       with the current code without the change.

Obvious; there is no support to copy notes.

>     . justifies the way the change solves the problem, iow, why the
>       result with the change is better.

Obvious: because it now actually copies the notes.

>     . alternate solutions considered but discarded, if any.

There are no alternate solutions.

-- 
Felipe Contreras

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Wed, May 29, 2013 at 3:09 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> Feel free to implement that. I'm just interested in 'git cherry-pick' being
>>> usable for 'git rebase' purposes.
>>
>> Which would have been obvious to all but the most casual readers, eh?
>
> My motivations are irrelevant, the patch is good as it is.

You fooled both Junio (AFAICT anyway) and me, who both reviewed the
patch under the assumption that it implements note copying *along the
lines of existing note copying*.  This proved to be a wrong, and
time-wasting, assumption.

I think all the points in this discussion have been made in the lengthy
thread about remote-{bzr,hg} already.  So much so in fact that your
continuing defiance of the project standards, knowing that they will
elicit exactly this response, amounts to trolling.

So until this changes, my $0.02 is a blanket NAK and a refusal to spend
my time reviewing.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

On Wed, May 29, 2013 at 3:40 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Wed, May 29, 2013 at 3:09 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>>> Feel free to implement that. I'm just interested in 'git cherry-pick' being
>>>> usable for 'git rebase' purposes.
>>>
>>> Which would have been obvious to all but the most casual readers, eh?
>>
>> My motivations are irrelevant, the patch is good as it is.
>
> You fooled both Junio (AFAICT anyway) and me, who both reviewed the
> patch under the assumption that it implements note copying *along the
> lines of existing note copying*.  This proved to be a wrong, and
> time-wasting, assumption.

Whatever arbitrary rules you are talking about, they are not codified in tests.

If you care so much about "*the lines of existing note copying*", why
don't you implement tests that check for those? This not only would
prevent that some shmuck who is not well versed in the tradition of
"the lines of existing note copying" from breaking that tradition,
which is precisely what tests are for.

If this was done, we wouldn't be "time-wasting" here.

This patch makes 'git cherry-pick' pass all the tests that 'git
rebase' passes. Period.

Even if it was against "the lines of existing note copying", it's much
better than the current situation, which is to do ABSOLUTELY NOTHING.

If you were a productive person, you would take this patch and
implement the code that makes it align to "the lines of existing note
copying" that you care so much about, which should be easy, if the
note framework was properly implement, but you won't.

Why isn't this implemented?

void copy_notes_for_rewrite(const char *rewrite_cmd, struct rewritten *list);

If there was such a thing, other clients wouldn't need to implement
their own methods of copying notes.

But you didn't implement that, did you?

You are punishing me because the notes framework is lacking, and
because the testing framework is missing what you think is the proper
behavior.

Strike that, you are punishing the users.

-- 
Felipe Contreras

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Wed, May 29, 2013 at 3:40 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Wed, May 29, 2013 at 3:09 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>>> Feel free to implement that. I'm just interested in 'git cherry-pick' being
>>>>> usable for 'git rebase' purposes.
>>>>
>>>> Which would have been obvious to all but the most casual readers, eh?
>>>
>>> My motivations are irrelevant, the patch is good as it is.
>>
>> You fooled both Junio (AFAICT anyway) and me, who both reviewed the
>> patch under the assumption that it implements note copying *along the
>> lines of existing note copying*.  This proved to be a wrong, and
>> time-wasting, assumption.
>
> Whatever arbitrary rules you are talking about, they are not codified in tests.

Tests or code don't have a thing to do with it.  This is about how you
are presenting your changes to the rest of the git community.  As
evidenced above, said presentation is not clear enough to communicate
your goals to at least two experienced git developers (if I may say so
myself).  How are we supposed to review a change if it is not even clear
what goal it satisfies?

Again: I'll be happy to review your proposed changes if and when you
resend the series with commit messages.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

On Wed, May 29, 2013 at 6:34 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Wed, May 29, 2013 at 3:40 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> On Wed, May 29, 2013 at 3:09 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>>> Feel free to implement that. I'm just interested in 'git cherry-pick' being
>>>>>> usable for 'git rebase' purposes.
>>>>>
>>>>> Which would have been obvious to all but the most casual readers, eh?
>>>>
>>>> My motivations are irrelevant, the patch is good as it is.
>>>
>>> You fooled both Junio (AFAICT anyway) and me, who both reviewed the
>>> patch under the assumption that it implements note copying *along the
>>> lines of existing note copying*.  This proved to be a wrong, and
>>> time-wasting, assumption.
>>
>> Whatever arbitrary rules you are talking about, they are not codified in tests.
>
> Tests or code don't have a thing to do with it.  This is about how you
> are presenting your changes to the rest of the git community.  As
> evidenced above, said presentation is not clear enough to communicate
> your goals to at least two experienced git developers (if I may say so
> myself).  How are we supposed to review a change if it is not even clear
> what goal it satisfies?

My goals are irrelevant. This patch is good.

It implements a missing feature, if you don't like how it is implemented it:

a) Implement the code in the note framework that does it properly so
other people can just call that.

b) Implement the tests for other commands that check that the behavior
you talk about does happen indeed.

c) Implement it yourself

This has nothing to do with the way I presented the patch. I presented
the patch as I thought it was; implementing the note copying as it was
intended. Now you came along and say I wasted your time because I
didn't say I implemented it wrongly, and you assume bad faith and what
not.

This wouldn't have happened because you didn't do a) or b). I realized
that by replacing 'am' with 'cherry-pick' in 'git rebase'  the notes
were not copied, according to the testing framework, so I implemented
that, and the tests pass. Now you come along and say it should
implemented some note.rewrite.command stuff, but the tests didn't
check for that, so how was I supposed to know?

And then you have the audacity to claim that *I*, the one who just
wrote a bunch of code to implement a missing feature, is wasting
*your* time, you, the one who is simply replying to an email and
shooting from the hip.

> Again: I'll be happy to review your proposed changes if and when you
> resend the series with commit messages.

I won't, I'll keep working on my actual objective.

Plus, this patch does have a commit message, and the commit message
says *EXACTLY* what the patch is doing: add support to copy notes.

If *you* are interested in certain behavior, why don't you lift a
finger and do something to make sure that such behavior is easy to
implement and the test framework actually checks for that?

-- 
Felipe Contreras

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Ramkumar Ramachandra]

Thomas Rast wrote:
> So until this changes, my $0.02 is a blanket NAK and a refusal to spend
> my time reviewing.

Then don't review the damn thing.  With Felipe, I have the following
rule of thumb: make some concrete suggestions and forget about
follow-ups.  He's not going to accept any general guidelines, unless
you're quoting Documentation/SubmittingPatches (and even then, it's
subject to interpretation); so provide a commit message and hope that
either he or Junio will use it.  There is no guarantee that he will
take any of your suggestions, no matter how sensible you think they
might be.  However, he is a productive programmer, and submits fixes
to real issues.  He's stubborn, and we can't do much to change that:
just learn to work with him.  I'm disappointed that I have to point
this out: haven't you learnt anything from previous discussions with
him?

Felipe, I suggest you put this in your commit message:

   This patch implements --copy-notes for 'git cherry-pick' so it can
copy notes in the same way that 'git rebase' does.

That is, if it's not too much trouble.

Stop this back-and-fourth nonsense, both of you.  It's degrading the
community, and hitting everyone's inboxes with garbage.

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

On Wed, May 29, 2013 at 7:09 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Thomas Rast wrote:
>> So until this changes, my $0.02 is a blanket NAK and a refusal to spend
>> my time reviewing.
>
> Then don't review the damn thing.  With Felipe, I have the following
> rule of thumb: make some concrete suggestions and forget about
> follow-ups.

He didn't make any suggestions.

> He's not going to accept any general guidelines, unless
> you're quoting Documentation/SubmittingPatches (and even then, it's
> subject to interpretation); so provide a commit message and hope that
> either he or Junio will use it.  There is no guarantee that he will
> take any of your suggestions, no matter how sensible you think they
> might be.

This is bullshit.

Let's look at some of the suggestions you have made:

== git-related ==

> s/l/line/?

I said fine, and I implemented it.

> Still no -CCC?

I said I forgot, and I implemented it.

What you are really complaining about is that I don't agree with
*every* single suggestion you make. And since you made them, they must
be sensible, and single I don't agree with you, I must not be
sensible, is that right?

And stop bringing irrelevant garbage to this discussion. The
discussion about the coding-style not about guidelines, because there
is no guideline for that open parenthesis, ad obviated by the fact
that there's over *FIVE HUNDRED* instances where it's not aligned that
way.

Nobody is denying the notes.rewritten.* guideline here, I didn't
because that is *actually* a guideline. So your comment about
guidelines is an irrelevant straw man.

> However, he is a productive programmer, and submits fixes
> to real issues.  He's stubborn, and we can't do much to change that:
> just learn to work with him.  I'm disappointed that I have to point
> this out: haven't you learnt anything from previous discussions with
> him?
>
> Felipe, I suggest you put this in your commit message:
>
>    This patch implements --copy-notes for 'git cherry-pick' so it can
> copy notes in the same way that 'git rebase' does.
>
> That is, if it's not too much trouble.
>
> Stop this back-and-fourth nonsense, both of you.  It's degrading the
> community, and hitting everyone's inboxes with garbage.

Thanks.

But let's take a step backwards, what are we trying to achieve here?
We are trying to improve Git, and the indisputable fact is that 'git
cherry-pick' is missing a way to copy the notes.

It's indisputable that this patch implements that, and I did it by
following existing code, and by running the whole test suit for 'git
rebase'. I've done my job already.

Thomas Rast doesn't like the way this is implemented, and nothing in
the commit message would change that.

This is was a sensible community, you would stop ganging up on me,
Thomas Rast would implement copy_notes_for_rewrite(), and add tests
for other commits (git am, git rebase) to check that the functionality
he claims to be so worried about is working properly.

And this was a sensible community you wouldn't complain about me
choosing how to spend my time however I see fit.

I did some work, I sent a patch, Thomas Rast has some issues, I'm not
interested enough in this patch to investigate them, I work on
something else. What's wrong with that?

Eventually I might come back to this patch, and eventually I might
implement copy_notes_for_rewrite, and I might implement the tests that
check for the behavior that I missed, if nobody beats me to it, which
is usually the case, but I think Thomas should put his personal issues
aside, put his money where his mouth is, and implement it himself.

There's nothing wrong with me choosing how best to spend my time. Really.

-- 
Felipe Contreras

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> What you are really complaining about is that I don't agree with
> *every* single suggestion you make. And since you made them, they must
> be sensible, and single I don't agree with you, I must not be
> sensible, is that right?

Oh, I have no problems: I reviewed git-related, and it resulted in
massive simplifications and improvements; those threads didn't run
wild.  I was mainly criticizing Thomas' review style (when it comes to
reviewing your patches in particular), and was prodding him to "make
concrete suggestions" in a one-email review, and leave it at that.
What started out as a pointer to the guidelines:

Thomas Rast wrote:
> We've been over this already:
>
>   The body should provide a meaningful commit message, which:

has resulted in this thread running wild.

> There's nothing wrong with me choosing how best to spend my time. Really.

Ofcourse you are.  You have arguably spent it very productively
solving a lot of user issues (especially remote-bzr).

Personally, I try to do the minimum amount of boring work required to
make sure that a good series gets in.  Sometimes this is a little high
(for a recent example, see my pickaxe-doc series).  The result being
that I just won't work on documentation in the future because doing
iterations is so piss boring: the git community needs to recognize
this problem and make amends.

Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Felipe Contreras]

On Wed, May 29, 2013 at 8:48 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:

>> There's nothing wrong with me choosing how best to spend my time. Really.
>
> Ofcourse you are.  You have arguably spent it very productively
> solving a lot of user issues (especially remote-bzr).
>
> Personally, I try to do the minimum amount of boring work required to
> make sure that a good series gets in.  Sometimes this is a little high
> (for a recent example, see my pickaxe-doc series).  The result being
> that I just won't work on documentation in the future because doing
> iterations is so piss boring: the git community needs to recognize
> this problem and make amends.

I don't mind doing as many iterations as it takes, as long as it's
about meaningful issues.

I don't particularly enjoy, but I'm OK with discussing non-meaningful issues.

What I'm not OK with is disagreements that end up breaking the
communication, specially when a crystal-clear case has been made
(IMO), and the patch goes to limbo for no reason. That I think is a
real problem.

For this particular patch, I don't care if it goes in at the moment. I
have something big on the pipeline, and I would rather drop this than
loose penguin points, although I probably don't have any left.

Cheers.

-- 
Felipe Contreras