{"thread":{"id":"33966","subject":"[PATCH v2 0/8] cherry-pick: improvements","startedAt":"2013-05-29T03:56:20Z","lastAt":"2013-06-06T05:01:26Z","messageCount":33,"participants":["Felipe Contreras","Ramkumar Ramachandra","Junio C Hamano","Antoine Pelisse","Thomas Rast"],"isPatch":true,"patchVersion":2,"patchTotal":8},"messages":[{"id":"218726","messageId":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":null,"subject":"[PATCH v2 0/8] cherry-pick: improvements","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:20Z","receivedAt":"2013-05-29T03:56:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nHere's a bunch of changes to make cherry-pick (and revert) more useful. In\nparticular; this makes it more friendly for 'git rebase.\n\nFelipe Contreras (8):\n  sequencer: remove useless indentation\n  sequencer: trivial fix\n  cherry-pick: add --skip-empty option\n  cherry-pick: store rewritten commits\n  sequencer: run post-rewrite hook\n  cherry-pick: add support to copy notes\n  revert/cherry-pick: add --quiet option\n  revert/cherry-pick: add --skip option\n\n Documentation/git-cherry-pick.txt   |  10 +-\n Documentation/git-revert.txt        |   7 +-\n Documentation/sequencer.txt         |   3 +\n builtin/revert.c                    |  11 ++\n sequencer.c                         | 230 ++++++++++++++++++++++++++++++++++--\n sequencer.h                         |   7 +-\n t/t3500-cherry.sh                   |  32 +++++\n t/t3508-cherry-pick-many-commits.sh |  13 ++\n t/t3510-cherry-pick-sequence.sh     |  12 ++\n 9 files changed, 310 insertions(+), 15 deletions(-)\n\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218727","messageId":"1369799788-24803-2-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/8] sequencer: remove useless indentation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:21Z","receivedAt":"2013-05-29T03:56:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"By using good ol' goto.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n sequencer.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ab6f8a7..b4989ba 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -474,7 +474,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0;\n+\tint res, unborn = 0, allow;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -624,14 +624,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t      msg.subject);\n \t\tprint_advice(res == 1, opts);\n \t\trerere(opts->allow_rerere_auto);\n-\t} else {\n-\t\tint allow = allow_empty(opts, commit);\n-\t\tif (allow < 0)\n-\t\t\treturn allow;\n-\t\tif (!opts->no_commit)\n-\t\t\tres = run_git_commit(defmsg, opts, allow);\n+\t\tgoto leave;\n \t}\n \n+\tallow = allow_empty(opts, commit);\n+\tif (allow < 0)\n+\t\treturn allow;\n+\tif (!opts->no_commit)\n+\t\tres = run_git_commit(defmsg, opts, allow);\n+\n+leave:\n \tfree_message(&msg);\n \tfree(defmsg);\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218729","messageId":"1369799788-24803-3-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/8] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:22Z","receivedAt":"2013-05-29T03:56:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We should free objects before leaving.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n sequencer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b4989ba..f7be7d8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -628,8 +628,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t}\n \n \tallow = allow_empty(opts, commit);\n-\tif (allow < 0)\n-\t\treturn allow;\n+\tif (allow < 0) {\n+\t\tres = allow;\n+\t\tgoto leave;\n+\t}\n \tif (!opts->no_commit)\n \t\tres = run_git_commit(defmsg, opts, allow);\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218728","messageId":"1369799788-24803-4-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:23Z","receivedAt":"2013-05-29T03:56:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Pretty much what it says on the tin.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/git-cherry-pick.txt   |  3 +++\n builtin/revert.c                    |  2 ++\n sequencer.c                         |  6 ++++++\n sequencer.h                         |  1 +\n t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n 5 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex c205d23..fccd936 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -129,6 +129,9 @@ effect to your index in a row.\n \tredundant commits are ignored.  This option overrides that behavior and\n \tcreates an empty commit object.  Implies `--allow-empty`.\n \n+--skip-empty::\n+\tInstead of failing, skip commits that are or become empty.\n+\n --strategy=<strategy>::\n \tUse the given merge strategy.  Should only be used once.\n \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0401fdb..0e5ce71 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n+\t\tOPT_END(),\n \t};\n \n \tif (opts->action == REPLAY_PICK) {\n@@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOLEAN(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n+\t\t\tOPT_BOOLEAN(0, \"skip-empty\", &opts->skip_empty, N_(\"skip empty commits\")),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\ndiff --git a/sequencer.c b/sequencer.c\nindex f7be7d8..d3c7d72 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -627,6 +627,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tgoto leave;\n \t}\n \n+\tif (opts->skip_empty && is_index_unchanged() == 1) {\n+\t\twarning(_(\"skipping %s... %s\"),\n+\t\t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n+\t\t\tmsg.subject);\n+\t\tgoto leave;\n+\t}\n \tallow = allow_empty(opts, commit);\n \tif (allow < 0) {\n \t\tres = allow;\ndiff --git a/sequencer.h b/sequencer.h\nindex 1fc22dc..3b04844 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -34,6 +34,7 @@ struct replay_opts {\n \tint allow_empty;\n \tint allow_empty_message;\n \tint keep_redundant_commits;\n+\tint skip_empty;\n \n \tint mainline;\n \ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 19c99d7..3dc19c6 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -187,4 +187,17 @@ test_expect_success 'cherry-pick --stdin works' '\n \tcheck_head_differs_from fourth\n '\n \n+test_expect_success 'cherry-pick skip empty' '\n+\tgit clean -fxd &&\n+\tgit checkout -b empty fourth &&\n+\tgit commit --allow-empty -m empty &&\n+\ttest_commit ontop &&\n+\tgit checkout -f master &&\n+\tgit reset --hard fourth &&\n+\tgit cherry-pick --skip-empty fourth..empty &&\n+\techo ontop > expected &&\n+\tgit log --format=%s fourth..HEAD > actual\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218731","messageId":"1369799788-24803-5-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 4/8] cherry-pick: store rewritten commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:24Z","receivedAt":"2013-05-29T03:56:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Will be useful for the next commits.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n sequencer.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n sequencer.h |  1 +\n 2 files changed, 95 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d3c7d72..c217716 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -20,6 +20,18 @@\n const char sign_off_header[] = \"Signed-off-by: \";\n static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n+struct rewritten_list_item {\n+\tunsigned char from[20];\n+\tunsigned char to[20];\n+};\n+\n+struct rewritten_list {\n+\tstruct rewritten_list_item *items;\n+\tunsigned int nr, alloc;\n+};\n+\n+static struct rewritten_list rewritten;\n+\n static int is_rfc2822_line(const char *buf, int len)\n {\n \tint i;\n@@ -102,6 +114,19 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \treturn 1;\n }\n \n+static void add_rewritten(unsigned char *from, unsigned char *to)\n+{\n+\tstruct rewritten_list_item *item;\n+\tif (rewritten.nr + 1 >= rewritten.alloc) {\n+\t\trewritten.alloc += 32;\n+\t\trewritten.items = xrealloc(rewritten.items, rewritten.alloc * sizeof(*rewritten.items));\n+\t}\n+\titem = &rewritten.items[rewritten.nr];\n+\thashcpy(item->from, from);\n+\thashcpy(item->to, to);\n+\trewritten.nr++;\n+}\n+\n static void remove_sequencer_state(void)\n {\n \tstruct strbuf seq_dir = STRBUF_INIT;\n@@ -641,6 +666,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tif (!opts->no_commit)\n \t\tres = run_git_commit(defmsg, opts, allow);\n \n+\tif (!res) {\n+\t\tunsigned char to[20];\n+\n+\t\tif (read_ref(\"HEAD\", to))\n+\t\t\tgoto leave;\n+\n+\t\tadd_rewritten(commit->object.sha1, to);\n+\t}\n leave:\n \tfree_message(&msg);\n \tfree(defmsg);\n@@ -786,6 +819,40 @@ static void read_populate_todo(struct commit_list **todo_list,\n \t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n }\n \n+static void read_populate_rewritten(void)\n+{\n+\tconst char *rewritten_file = git_path(SEQ_REWR_FILE);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tchar *p;\n+\tint fd;\n+\n+\tfd = open(rewritten_file, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn;\n+\tif (strbuf_read(&buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\tstrbuf_release(&buf);\n+\t\treturn;\n+\t}\n+\tclose(fd);\n+\n+\tfor (p = buf.buf; *p;) {\n+\t\tunsigned char from[20];\n+\t\tunsigned char to[20];\n+\t\tchar *eol = strchrnul(p, '\\n');\n+\t\tif (eol - p != 81)\n+\t\t\t/* wrong size */\n+\t\t\tbreak;\n+\t\tif (get_sha1_hex(p, from))\n+\t\t\tbreak;\n+\t\tif (get_sha1_hex(p + 41, to))\n+\t\t\tbreak;\n+\t\tadd_rewritten(from, to);\n+\t\tp = *eol ? eol + 1 : eol;\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n static int populate_opts_cb(const char *key, const char *value, void *data)\n {\n \tstruct replay_opts *opts = data;\n@@ -958,6 +1025,29 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tstrbuf_release(&buf);\n }\n \n+static void save_rewritten(void)\n+{\n+\tconst char *todo_file = git_path(SEQ_REWR_FILE);\n+\tstatic struct lock_file rewritten_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd, i;\n+\n+\tfd = hold_lock_file_for_update(&rewritten_lock, todo_file, LOCK_DIE_ON_ERROR);\n+\tfor (i = 0; i < rewritten.nr; i++) {\n+\t\tstruct rewritten_list_item *item = &rewritten.items[i];\n+\t\tstrbuf_addf(&buf, \"%s %s\\n\", sha1_to_hex(item->from), sha1_to_hex(item->to));\n+\t}\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not write to %s\"), todo_file);\n+\t}\n+\tif (commit_lock_file(&rewritten_lock) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n static void save_opts(struct replay_opts *opts)\n {\n \tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n@@ -1003,8 +1093,10 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur, opts);\n \t\tres = do_pick_commit(cur->item, opts);\n-\t\tif (res)\n+\t\tif (res) {\n+\t\t\tsave_rewritten();\n \t\t\treturn res;\n+\t\t}\n \t}\n \n \t/*\n@@ -1033,6 +1125,7 @@ static int sequencer_continue(struct replay_opts *opts)\n \t\treturn continue_single_pick();\n \tread_populate_opts(&opts);\n \tread_populate_todo(&todo_list, opts);\n+\tread_populate_rewritten();\n \n \t/* Verify that the conflict has been resolved */\n \tif (file_exists(git_path(\"CHERRY_PICK_HEAD\")) ||\ndiff --git a/sequencer.h b/sequencer.h\nindex 3b04844..84b9957 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -5,6 +5,7 @@\n #define SEQ_HEAD_FILE\t\"sequencer/head\"\n #define SEQ_TODO_FILE\t\"sequencer/todo\"\n #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n+#define SEQ_REWR_FILE\t\"sequencer/rewritten\"\n \n #define APPEND_SIGNOFF_DEDUP (1u << 0)\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218730","messageId":"1369799788-24803-6-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 5/8] sequencer: run post-rewrite hook","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:25Z","receivedAt":"2013-05-29T03:56:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"As we should.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n sequencer.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c217716..3aa480e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -127,6 +127,37 @@ static void add_rewritten(unsigned char *from, unsigned char *to)\n \trewritten.nr++;\n }\n \n+static void run_rewrite_hook(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct child_process proc;\n+\tconst char *argv[3];\n+\tint code, i;\n+\n+\targv[0] = find_hook(\"post-rewrite\");\n+\tif (!argv[0])\n+\t\treturn;\n+\n+\targv[1] = \"rebase\";\n+\targv[2] = NULL;\n+\n+\tmemset(&proc, 0, sizeof(proc));\n+\tproc.argv = argv;\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\n+\tcode = start_command(&proc);\n+\tif (code)\n+\t\treturn;\n+\tfor (i = 0; i < rewritten.nr; i++) {\n+\t\tstruct rewritten_list_item *item = &rewritten.items[i];\n+\t\tstrbuf_addf(&buf, \"%s %s\\n\", sha1_to_hex(item->from), sha1_to_hex(item->to));\n+\t}\n+\twrite_in_full(proc.in, buf.buf, buf.len);\n+\tclose(proc.in);\n+\tfinish_command(&proc);\n+}\n+\n static void remove_sequencer_state(void)\n {\n \tstruct strbuf seq_dir = STRBUF_INIT;\n@@ -1099,6 +1130,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \t\t}\n \t}\n \n+\trun_rewrite_hook();\n+\n \t/*\n \t * Sequence of picks finished successfully; cleanup by\n \t * removing the .git/sequencer directory\n@@ -1136,14 +1169,24 @@ static int sequencer_continue(struct replay_opts *opts)\n \t}\n \tif (index_differs_from(\"HEAD\", 0))\n \t\treturn error_dirty_index(opts);\n+\t{\n+\t\tunsigned char to[20];\n+\t\tif (!read_ref(\"HEAD\", to))\n+\t\t\tadd_rewritten(todo_list->item->object.sha1, to);\n+\t}\n \ttodo_list = todo_list->next;\n \treturn pick_commits(todo_list, opts);\n }\n \n static int single_pick(struct commit *cmit, struct replay_opts *opts)\n {\n+\tint ret;\n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n-\treturn do_pick_commit(cmit, opts);\n+\tret = do_pick_commit(cmit, opts);\n+\tif (ret)\n+\t\treturn ret;\n+\trun_rewrite_hook();\n+\treturn 0;\n }\n \n int sequencer_pick_revisions(struct replay_opts *opts)\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218732","messageId":"1369799788-24803-7-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 6/8] cherry-pick: add support to copy notes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:26Z","receivedAt":"2013-05-29T03:56:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/revert.c  |  2 ++\n sequencer.c       | 33 ++++++++++++++++++++++++++++++++-\n sequencer.h       |  1 +\n t/t3500-cherry.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 67 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0e5ce71..b977124 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -119,6 +119,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n+\t\tOPT_END(),\n \t};\n \n \tif (opts->action == REPLAY_PICK) {\n@@ -129,6 +130,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_BOOLEAN(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n \t\t\tOPT_BOOLEAN(0, \"skip-empty\", &opts->skip_empty, N_(\"skip empty commits\")),\n+\t\t\tOPT_BOOLEAN(0, \"copy-notes\", &opts->copy_notes, N_(\"copy notes\")),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\ndiff --git a/sequencer.c b/sequencer.c\nindex 3aa480e..cc9c2bc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -127,6 +127,27 @@ static void add_rewritten(unsigned char *from, unsigned char *to)\n \trewritten.nr++;\n }\n \n+static void copy_notes(void)\n+{\n+\tunsigned char note_commit[20];\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct notes_tree *t = &default_notes_tree;\n+\tint i;\n+\n+\tinit_notes(t, NULL, NULL, 0);\n+\n+\tfor (i = 0; i < rewritten.nr; i++) {\n+\t\tstruct rewritten_list_item *item = &rewritten.items[i];\n+\t\tcopy_note(t, item->from, item->to, 0, NULL);\n+\t}\n+\n+\tstrbuf_addstr(&buf, \"Notes added by 'git cherry-pick'\\n\");\n+\tcreate_notes_commit(&default_notes_tree, NULL, &buf, note_commit);\n+\tstrbuf_insert(&buf, 0, \"notes: \", 7);\n+\tupdate_ref(buf.buf, t->ref, note_commit, NULL, 0, MSG_ON_ERR);\n+\tstrbuf_release(&buf);\n+}\n+\n static void run_rewrite_hook(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -908,7 +929,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \telse if (!strcmp(key, \"options.strategy-option\")) {\n \t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n \t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n-\t} else\n+\t}\n+\telse if (!strcmp(key, \"options.copy-notes\"))\n+\t\topts->copy_notes = git_config_bool_or_int(key, value, &error_flag);\n+\telse\n \t\treturn error(_(\"Invalid key: %s\"), key);\n \n \tif (!error_flag)\n@@ -1108,6 +1132,8 @@ static void save_opts(struct replay_opts *opts)\n \t\t\t\t\t\t\t\"options.strategy-option\",\n \t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n \t}\n+\tif (opts->copy_notes)\n+\t\tgit_config_set_in_file(opts_file, \"options.copy-notes\", \"true\");\n }\n \n static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n@@ -1132,6 +1158,9 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \n \trun_rewrite_hook();\n \n+\tif (opts->copy_notes)\n+\t\tcopy_notes();\n+\n \t/*\n \t * Sequence of picks finished successfully; cleanup by\n \t * removing the .git/sequencer directory\n@@ -1186,6 +1215,8 @@ static int single_pick(struct commit *cmit, struct replay_opts *opts)\n \tif (ret)\n \t\treturn ret;\n \trun_rewrite_hook();\n+\tif (opts->copy_notes)\n+\t\tcopy_notes();\n \treturn 0;\n }\n \ndiff --git a/sequencer.h b/sequencer.h\nindex 84b9957..6cc072c 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -36,6 +36,7 @@ struct replay_opts {\n \tint allow_empty_message;\n \tint keep_redundant_commits;\n \tint skip_empty;\n+\tint copy_notes;\n \n \tint mainline;\n \ndiff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\nindex f038f34..79c1219 100755\n--- a/t/t3500-cherry.sh\n+++ b/t/t3500-cherry.sh\n@@ -55,4 +55,36 @@ test_expect_success \\\n      expr \"$(echo $(git cherry master my-topic-branch) )\" : \"+ [^ ]* - .*\"\n '\n \n+test_expect_success \\\n+    'copy notes' \\\n+    'git checkout master &&\n+    echo notes > C &&\n+    test_tick &&\n+    git commit -a -m \"Update C\" &&\n+    git notes add -m \"a note\" &&\n+    git checkout -b note-test HEAD^ &&\n+    git cherry-pick --copy-notes -x master &&\n+    test \"a note\" = \"$(git notes show HEAD)\"\n+'\n+\n+test_expect_success \\\n+    'copy multiple notes' \\\n+    'git checkout master &&\n+    echo \"multiple notes\" > C &&\n+    git commit -a -m \"Update C again\" &&\n+    git notes add -m \"another note\" &&\n+    git commit -a -m \"Empty\" --allow-empty &&\n+    echo \"more notes\" > C &&\n+    git commit -a -m \"Update C once more\" &&\n+    git notes add -m \"final note\" &&\n+    git checkout note-test &&\n+    git reset --hard master~3 &&\n+    test_expect_code 1 git cherry-pick --copy-notes -x HEAD..master &&\n+    git reset --hard &&\n+    git cherry-pick --continue\n+    test \"a note\" = \"$(git notes show HEAD~2)\" &&\n+    test \"another note\" = \"$(git notes show HEAD~1)\" &&\n+    test \"final note\" = \"$(git notes show HEAD)\"\n+'\n+\n test_done\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218733","messageId":"1369799788-24803-8-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 7/8] revert/cherry-pick: add --quiet option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:27Z","receivedAt":"2013-05-29T03:56:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/git-cherry-pick.txt | 6 +++++-\n Documentation/git-revert.txt      | 6 +++++-\n builtin/revert.c                  | 1 +\n sequencer.c                       | 9 ++++++---\n sequencer.h                       | 1 +\n 5 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex fccd936..da0bd81 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -8,7 +8,7 @@ git-cherry-pick - Apply the changes introduced by some existing commits\n SYNOPSIS\n --------\n [verse]\n-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...\n+'git cherry-pick' [-q] [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...\n 'git cherry-pick' --continue\n 'git cherry-pick' --quit\n 'git cherry-pick' --abort\n@@ -51,6 +51,10 @@ OPTIONS\n \tfeed all <commit>... arguments to a single revision walk\n \t(see a later example that uses 'maint master..next').\n \n+-q::\n+--quiet::\n+\tQuiet, suppress feedback messages.\n+\n -e::\n --edit::\n \tWith this option, 'git cherry-pick' will let you edit the commit\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex f79c9d8..98a8e7a 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -8,7 +8,7 @@ git-revert - Revert some existing commits\n SYNOPSIS\n --------\n [verse]\n-'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...\n+'git revert' [-q] [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...\n 'git revert' --continue\n 'git revert' --quit\n 'git revert' --abort\n@@ -40,6 +40,10 @@ OPTIONS\n \tdefault, see linkgit:git-rev-list[1] and its '--no-walk'\n \toption.\n \n+-q::\n+--quiet::\n+\tQuiet, suppress feedback messages.\n+\n -e::\n --edit::\n \tWith this option, 'git revert' will let you edit the commit\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex b977124..d63b4a6 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -100,6 +100,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tint contin = 0;\n \tint rollback = 0;\n \tstruct option options[] = {\n+\t\tOPT__QUIET(&opts->quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_BOOLEAN(0, \"quit\", &remove_state, N_(\"end revert or cherry-pick sequence\")),\n \t\tOPT_BOOLEAN(0, \"continue\", &contin, N_(\"resume revert or cherry-pick sequence\")),\n \t\tOPT_BOOLEAN(0, \"abort\", &rollback, N_(\"cancel revert or cherry-pick sequence\")),\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9c2bc..b7391d6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -466,6 +466,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \targv_array_init(&array);\n \targv_array_push(&array, \"commit\");\n \targv_array_push(&array, \"-n\");\n+\tif (opts->quiet)\n+\t\targv_array_push(&array, \"-q\");\n \n \tif (opts->signoff)\n \t\targv_array_push(&array, \"-s\");\n@@ -705,9 +707,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t}\n \n \tif (opts->skip_empty && is_index_unchanged() == 1) {\n-\t\twarning(_(\"skipping %s... %s\"),\n-\t\t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n-\t\t\tmsg.subject);\n+\t\tif (!opts->quiet)\n+\t\t\twarning(_(\"skipping %s... %s\"),\n+\t\t\t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n+\t\t\t\tmsg.subject);\n \t\tgoto leave;\n \t}\n \tallow = allow_empty(opts, commit);\ndiff --git a/sequencer.h b/sequencer.h\nindex 6cc072c..41e19d0 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -37,6 +37,7 @@ struct replay_opts {\n \tint keep_redundant_commits;\n \tint skip_empty;\n \tint copy_notes;\n+\tint quiet;\n \n \tint mainline;\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218734","messageId":"1369799788-24803-9-git-send-email-felipe.contreras@gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 8/8] revert/cherry-pick: add --skip option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T03:56:28Z","receivedAt":"2013-05-29T03:56:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Akin to 'am --skip' and 'rebase --skip'.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/git-cherry-pick.txt |  1 +\n Documentation/git-revert.txt      |  1 +\n Documentation/sequencer.txt       |  3 +++\n builtin/revert.c                  |  6 ++++++\n sequencer.c                       | 32 +++++++++++++++++++++++++++++---\n sequencer.h                       |  3 ++-\n t/t3510-cherry-pick-sequence.sh   | 12 ++++++++++++\n 7 files changed, 54 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex da0bd81..d95c63c 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n [verse]\n 'git cherry-pick' [-q] [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...\n 'git cherry-pick' --continue\n+'git cherry-pick' --skip\n 'git cherry-pick' --quit\n 'git cherry-pick' --abort\n \ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex 98a8e7a..52e146e 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n [verse]\n 'git revert' [-q] [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...\n 'git revert' --continue\n+'git revert' --skip\n 'git revert' --quit\n 'git revert' --abort\n \ndiff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt\nindex 5747f44..df2d355 100644\n--- a/Documentation/sequencer.txt\n+++ b/Documentation/sequencer.txt\n@@ -3,6 +3,9 @@\n \t'.git/sequencer'.  Can be used to continue after resolving\n \tconflicts in a failed cherry-pick or revert.\n \n+--skip::\n+\tSkip the current commit, and then continue.\n+\n --quit::\n \tForget about the current operation in progress.  Can be used\n \tto clear the sequencer state after a failed cherry-pick or\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex d63b4a6..6afd990 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -99,11 +99,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tint remove_state = 0;\n \tint contin = 0;\n \tint rollback = 0;\n+\tint skip = 0;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opts->quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_BOOLEAN(0, \"quit\", &remove_state, N_(\"end revert or cherry-pick sequence\")),\n \t\tOPT_BOOLEAN(0, \"continue\", &contin, N_(\"resume revert or cherry-pick sequence\")),\n \t\tOPT_BOOLEAN(0, \"abort\", &rollback, N_(\"cancel revert or cherry-pick sequence\")),\n+\t\tOPT_BOOLEAN(0, \"skip\", &skip, N_(\"skip current commit in the sequence\")),\n \t\tOPT_BOOLEAN('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOLEAN('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n \t\tOPT_NOOP_NOARG('r', NULL),\n@@ -160,6 +162,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\topts->subcommand = REPLAY_CONTINUE;\n \telse if (rollback)\n \t\topts->subcommand = REPLAY_ROLLBACK;\n+\telse if (skip)\n+\t\topts->subcommand = REPLAY_SKIP;\n \telse\n \t\topts->subcommand = REPLAY_NONE;\n \n@@ -170,6 +174,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tthis_operation = \"--quit\";\n \t\telse if (opts->subcommand == REPLAY_CONTINUE)\n \t\t\tthis_operation = \"--continue\";\n+\t\telse if (opts->subcommand == REPLAY_SKIP)\n+\t\t\tthis_operation = \"--skip\";\n \t\telse {\n \t\t\tassert(opts->subcommand == REPLAY_ROLLBACK);\n \t\t\tthis_operation = \"--abort\";\ndiff --git a/sequencer.c b/sequencer.c\nindex b7391d6..46bbe29 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1182,7 +1182,9 @@ static int continue_single_pick(void)\n \treturn run_command_v_opt(argv, RUN_GIT_CMD);\n }\n \n-static int sequencer_continue(struct replay_opts *opts)\n+static void add_rewritten(unsigned char *from, unsigned char *to);\n+\n+static int sequencer_continue(struct replay_opts *opts, int skip)\n {\n \tstruct commit_list *todo_list = NULL;\n \n@@ -1201,7 +1203,7 @@ static int sequencer_continue(struct replay_opts *opts)\n \t}\n \tif (index_differs_from(\"HEAD\", 0))\n \t\treturn error_dirty_index(opts);\n-\t{\n+\tif (!skip) {\n \t\tunsigned char to[20];\n \t\tif (!read_ref(\"HEAD\", to))\n \t\t\tadd_rewritten(todo_list->item->object.sha1, to);\n@@ -1210,6 +1212,28 @@ static int sequencer_continue(struct replay_opts *opts)\n \treturn pick_commits(todo_list, opts);\n }\n \n+static int sequencer_skip(struct replay_opts *opts)\n+{\n+\tconst char *argv[4]; /* reset --hard HEAD + NULL */\n+\tstruct string_list merge_rr = STRING_LIST_INIT_DUP;\n+\tint ret;\n+\n+\tif (setup_rerere(&merge_rr, 0) >= 0) {\n+\t\trerere_clear(&merge_rr);\n+\t\tstring_list_clear(&merge_rr, 1);\n+\t}\n+\n+\targv[0] = \"reset\";\n+\targv[1] = \"--hard\";\n+\targv[2] = \"HEAD\";\n+\targv[3] = NULL;\n+\tret = run_command_v_opt(argv, RUN_GIT_CMD);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\treturn sequencer_continue(opts, 1);\n+}\n+\n static int single_pick(struct commit *cmit, struct replay_opts *opts)\n {\n \tint ret;\n@@ -1246,7 +1270,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \tif (opts->subcommand == REPLAY_ROLLBACK)\n \t\treturn sequencer_rollback(opts);\n \tif (opts->subcommand == REPLAY_CONTINUE)\n-\t\treturn sequencer_continue(opts);\n+\t\treturn sequencer_continue(opts, 0);\n+\tif (opts->subcommand == REPLAY_SKIP)\n+\t\treturn sequencer_skip(opts);\n \n \tfor (i = 0; i < opts->revs->pending.nr; i++) {\n \t\tunsigned char sha1[20];\ndiff --git a/sequencer.h b/sequencer.h\nindex 41e19d0..b9e09ac 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -18,7 +18,8 @@ enum replay_subcommand {\n \tREPLAY_NONE,\n \tREPLAY_REMOVE_STATE,\n \tREPLAY_CONTINUE,\n-\tREPLAY_ROLLBACK\n+\tREPLAY_ROLLBACK,\n+\tREPLAY_SKIP\n };\n \n struct replay_opts {\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 7b7a89d..f58a83d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -511,4 +511,16 @@ test_expect_success 'commit descriptions in insn sheet are optional' '\n \ttest_line_count = 4 commits\n '\n \n+test_expect_success 'skip' '\n+\tpristine_detach conflicting &&\n+\ttest_must_fail git cherry-pick initial..picked &&\n+\n+\tgit checkout HEAD -- unrelated &&\n+\ttest_must_fail git cherry-pick --continue &&\n+\tgit cherry-pick --skip &&\n+\n+\tgit rev-list initial..HEAD >commits &&\n+\ttest_line_count = 3 commits\n+'\n+\n test_done\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218811","messageId":"CALkWK0m02XCVnrnFQ+mF8FFZEMD36_J3Tyjh-E4SuZ++xdcXHQ@mail.gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-9-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 8/8] revert/cherry-pick: add --skip option","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-29T12:27:57Z","receivedAt":"2013-05-29T12:27:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> Akin to 'am --skip' and 'rebase --skip'.\n\nThis ranged-cherry-pick can be useful for small ranges.  As pointed\nout by others on the list, it hemorrhages memory quite horribly (and\nthis problem is non-trivial to fix).  Perhaps we should document this\nin limitations or bugs if we intend to make it more useful?\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index d63b4a6..6afd990 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -99,11 +99,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>         int remove_state = 0;\n>         int contin = 0;\n>         int rollback = 0;\n> +       int skip = 0;\n\nUgh, one more integer.  Can't we use an OPT_BIT and store the action\nin one variable? No hurry ofcourse: just asking.\n\n> @@ -1201,7 +1203,7 @@ static int sequencer_continue(struct replay_opts *opts)\n>         }\n>         if (index_differs_from(\"HEAD\", 0))\n>                 return error_dirty_index(opts);\n> -       {\n> +       if (!skip) {\n>                 unsigned char to[20];\n>                 if (!read_ref(\"HEAD\", to))\n>                         add_rewritten(todo_list->item->object.sha1, to);\n\nCouldn't you just say if (skip) todo_list = todo_list -> next?\n\n> +       if (setup_rerere(&merge_rr, 0) >= 0) {\n> +               rerere_clear(&merge_rr);\n> +               string_list_clear(&merge_rr, 1);\n> +       }\n\nWhy exactly?  Why doesn't rebase --skip 'rerere clear'?\n\n> +       argv[0] = \"reset\";\n> +       argv[1] = \"--hard\";\n> +       argv[2] = \"HEAD\";\n> +       argv[3] = NULL;\n> +       ret = run_command_v_opt(argv, RUN_GIT_CMD);\n\nUnrelated to your patch, but any clue why reset doesn't have an api\nyet?  Does it leak memory too?\n"},{"id":"218813","messageId":"CALkWK0=QyznWZt4sJCok9OZxjrdjtMTtpStA8LHdCCbn0Oi6TA@mail.gmail.com","threadId":"33966","inReplyTo":"1369799788-24803-8-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 7/8] revert/cherry-pick: add --quiet option","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-29T12:33:25Z","receivedAt":"2013-05-29T12:33:25Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n>         if (opts->skip_empty && is_index_unchanged() == 1) {\n> -               warning(_(\"skipping %s... %s\"),\n> -                       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n> -                       msg.subject);\n> +               if (!opts->quiet)\n> +                       warning(_(\"skipping %s... %s\"),\n> +                               find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n> +                               msg.subject);\n>                 goto leave;\n>         }\n\nAll this trouble just to suppress the \"skipping ...\" message.  Do you\nsee anything else --quiet can be used to suppress?\n"},{"id":"218826","messageId":"CAMP44s1i9H9cp_RoOo-rDWL6b76=zxbwC19=DnUayK_Ti2CXFA@mail.gmail.com","threadId":"33966","inReplyTo":"CALkWK0m02XCVnrnFQ+mF8FFZEMD36_J3Tyjh-E4SuZ++xdcXHQ@mail.gmail.com","subject":"Re: [PATCH v2 8/8] revert/cherry-pick: add --skip option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T13:27:28Z","receivedAt":"2013-05-29T13:27:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 7:27 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>> Akin to 'am --skip' and 'rebase --skip'.\n>\n> This ranged-cherry-pick can be useful for small ranges.  As pointed\n> out by others on the list, it hemorrhages memory quite horribly (and\n> this problem is non-trivial to fix).  Perhaps we should document this\n> in limitations or bugs if we intend to make it more useful?\n\nIs there a test-case that triggers this?\n\nI don't see why we shouldn't try to fix this.\n\n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index d63b4a6..6afd990 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -99,11 +99,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>>         int remove_state = 0;\n>>         int contin = 0;\n>>         int rollback = 0;\n>> +       int skip = 0;\n>\n> Ugh, one more integer.  Can't we use an OPT_BIT and store the action\n> in one variable? No hurry ofcourse: just asking.\n\nI thought of switching to bit fields, but then I opened\nbuiltin/checkout.c to see how many options were implemented, and I saw\nmore ints, so I left it like that.\n\n>> @@ -1201,7 +1203,7 @@ static int sequencer_continue(struct replay_opts *opts)\n>>         }\n>>         if (index_differs_from(\"HEAD\", 0))\n>>                 return error_dirty_index(opts);\n>> -       {\n>> +       if (!skip) {\n>>                 unsigned char to[20];\n>>                 if (!read_ref(\"HEAD\", to))\n>>                         add_rewritten(todo_list->item->object.sha1, to);\n>\n> Couldn't you just say if (skip) todo_list = todo_list -> next?\n\nWe are already doing that. And after that we need to continue with the\nrest of the commits.\n\n>> +       if (setup_rerere(&merge_rr, 0) >= 0) {\n>> +               rerere_clear(&merge_rr);\n>> +               string_list_clear(&merge_rr, 1);\n>> +       }\n>\n> Why exactly?  Why doesn't rebase --skip 'rerere clear'?\n\nIt does, and so does 'git am', so why shouldn't 'git cherry-pick'.\n\nAlso, I think the same should happen in --abort.\n\n-- \nFelipe Contreras\n"},{"id":"218827","messageId":"CAMP44s0M8dg_8ke3p1wJtnPrjdtH0L-D3ZgnPPCSKH0Mx0Z=GA@mail.gmail.com","threadId":"33966","inReplyTo":"CALkWK0=QyznWZt4sJCok9OZxjrdjtMTtpStA8LHdCCbn0Oi6TA@mail.gmail.com","subject":"Re: [PATCH v2 7/8] revert/cherry-pick: add --quiet option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T13:28:40Z","receivedAt":"2013-05-29T13:28:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 7:33 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>>         if (opts->skip_empty && is_index_unchanged() == 1) {\n>> -               warning(_(\"skipping %s... %s\"),\n>> -                       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n>> -                       msg.subject);\n>> +               if (!opts->quiet)\n>> +                       warning(_(\"skipping %s... %s\"),\n>> +                               find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n>> +                               msg.subject);\n>>                 goto leave;\n>>         }\n>\n> All this trouble just to suppress the \"skipping ...\" message.  Do you\n> see anything else --quiet can be used to suppress?\n\nDid you miss the -q option passed to 'git commit'?\n\n-- \nFelipe Contreras\n"},{"id":"218829","messageId":"CALkWK0=VieCwZS4ua-eyg3t-cqC2Y+ifa-G7gwJ=Mxt0cfoD1g@mail.gmail.com","threadId":"33966","inReplyTo":"CAMP44s0M8dg_8ke3p1wJtnPrjdtH0L-D3ZgnPPCSKH0Mx0Z=GA@mail.gmail.com","subject":"Re: [PATCH v2 7/8] revert/cherry-pick: add --quiet option","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-29T13:32:47Z","receivedAt":"2013-05-29T13:32:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> Did you miss the -q option passed to 'git commit'?\n\nAh, yes.\n\nIt would help if you mentioned:\n\n    Introduce --quiet to suppress warning about skipped commits (when\nusing --skip-empty) and output from 'git commit'.\n\nin the commit message.\n\nThanks.\n"},{"id":"218832","messageId":"CAMP44s38xDmqtzLNB44=HKQd8R+3S=edN3LS40GFfp7wfqi3hw@mail.gmail.com","threadId":"33966","inReplyTo":"CALkWK0=VieCwZS4ua-eyg3t-cqC2Y+ifa-G7gwJ=Mxt0cfoD1g@mail.gmail.com","subject":"Re: [PATCH v2 7/8] revert/cherry-pick: add --quiet option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T13:40:08Z","receivedAt":"2013-05-29T13:40:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 8:32 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>> Did you miss the -q option passed to 'git commit'?\n>\n> Ah, yes.\n>\n> It would help if you mentioned:\n>\n>     Introduce --quiet to suppress warning about skipped commits (when\n> using --skip-empty) and output from 'git commit'.\n\nI would switch them around, as the former is not important, in fact,\nit's so unimportant that I wouldn't even mention it.\n\nAnd in fact, it wasn't even there when I wrote this commit message I\nsent for v1, it was only squashed later on.\n\nThe only really important thing this patch does is 'commit -q', and I\ncannot imagine what the purpose of a -q option to cherry-pick would be\nif it's not that. But yeah, it wouldn't hurt.\n\n-- \nFelipe Contreras\n"},{"id":"219270","messageId":"7vr4gjdomk.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"1369799788-24803-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T18:40:19Z","receivedAt":"2013-06-03T18:40:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Pretty much what it says on the tin.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  Documentation/git-cherry-pick.txt   |  3 +++\n>  builtin/revert.c                    |  2 ++\n>  sequencer.c                         |  6 ++++++\n>  sequencer.h                         |  1 +\n>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n>  5 files changed, 25 insertions(+)\n>\n> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> index c205d23..fccd936 100644\n> --- a/Documentation/git-cherry-pick.txt\n> +++ b/Documentation/git-cherry-pick.txt\n> @@ -129,6 +129,9 @@ effect to your index in a row.\n>  \tredundant commits are ignored.  This option overrides that behavior and\n>  \tcreates an empty commit object.  Implies `--allow-empty`.\n>  \n> +--skip-empty::\n> +\tInstead of failing, skip commits that are or become empty.\n\nNot quite sure.  Is this \"instead of recording an empty commit,\"\n(which may or may not fail depending on the allow-empty settings)?\n\nIf that is what this patch is meant to do, I think the change makes\nsense.\n\n>  --strategy=<strategy>::\n>  \tUse the given merge strategy.  Should only be used once.\n>  \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 0401fdb..0e5ce71 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>  \t\tOPT_END(),\n>  \t\tOPT_END(),\n>  \t\tOPT_END(),\n> +\t\tOPT_END(),\n>  \t};\n>  \n>  \tif (opts->action == REPLAY_PICK) {\n> @@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>  \t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n>  \t\t\tOPT_BOOLEAN(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n>  \t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n> +\t\t\tOPT_BOOLEAN(0, \"skip-empty\", &opts->skip_empty, N_(\"skip empty commits\")),\n>  \t\t\tOPT_END(),\n>  \t\t};\n>  \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\n> diff --git a/sequencer.c b/sequencer.c\n> index f7be7d8..d3c7d72 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -627,6 +627,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n>  \t\tgoto leave;\n>  \t}\n>  \n> +\tif (opts->skip_empty && is_index_unchanged() == 1) {\n> +\t\twarning(_(\"skipping %s... %s\"),\n> +\t\t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n> +\t\t\tmsg.subject);\n> +\t\tgoto leave;\n> +\t}\n>  \tallow = allow_empty(opts, commit);\n>  \tif (allow < 0) {\n>  \t\tres = allow;\n> diff --git a/sequencer.h b/sequencer.h\n> index 1fc22dc..3b04844 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -34,6 +34,7 @@ struct replay_opts {\n>  \tint allow_empty;\n>  \tint allow_empty_message;\n>  \tint keep_redundant_commits;\n> +\tint skip_empty;\n>  \n>  \tint mainline;\n>  \n> diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\n> index 19c99d7..3dc19c6 100755\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -187,4 +187,17 @@ test_expect_success 'cherry-pick --stdin works' '\n>  \tcheck_head_differs_from fourth\n>  '\n>  \n> +test_expect_success 'cherry-pick skip empty' '\n> +\tgit clean -fxd &&\n> +\tgit checkout -b empty fourth &&\n> +\tgit commit --allow-empty -m empty &&\n> +\ttest_commit ontop &&\n> +\tgit checkout -f master &&\n> +\tgit reset --hard fourth &&\n> +\tgit cherry-pick --skip-empty fourth..empty &&\n> +\techo ontop > expected &&\n> +\tgit log --format=%s fourth..HEAD > actual\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_done\n"},{"id":"219271","messageId":"7vmwr7do7s.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"1369799788-24803-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 4/8] cherry-pick: store rewritten commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T18:49:11Z","receivedAt":"2013-06-03T18:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> +static void add_rewritten(unsigned char *from, unsigned char *to)\n> +{\n> +\tstruct rewritten_list_item *item;\n> +\tif (rewritten.nr + 1 >= rewritten.alloc) {\n> +\t\trewritten.alloc += 32;\n> +\t\trewritten.items = xrealloc(rewritten.items, rewritten.alloc * sizeof(*rewritten.items));\n> +\t}\n\nIs there a compelling reason not to use ALLOC_GROW() here?\n"},{"id":"219272","messageId":"7vhahfdnu6.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"1369799788-24803-6-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 5/8] sequencer: run post-rewrite hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T18:57:21Z","receivedAt":"2013-06-03T18:57:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> As we should.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  sequencer.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-\n>  1 file changed, 44 insertions(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index c217716..3aa480e 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -127,6 +127,37 @@ static void add_rewritten(unsigned char *from, unsigned char *to)\n>  \trewritten.nr++;\n>  }\n>  \n> +static void run_rewrite_hook(void)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct child_process proc;\n> +\tconst char *argv[3];\n> +\tint code, i;\n> +\n> +\targv[0] = find_hook(\"post-rewrite\");\n> +\tif (!argv[0])\n> +\t\treturn;\n> +\n> +\targv[1] = \"rebase\";\n> +\targv[2] = NULL;\n\nWhen the end-user action is \"git cherry-pick A..B\", shouldn't\nthe rewrite hook be called with \"cherry-pick\" not \"rebase\" as its\nfirst argument?\n\nMore importantly, doesn't \"git revert A..B\" also trigger the\ncodepath for [4/8] and hence this function?\n\nI think [3/8] --skip-empty makes sense also for revert, but I do not\nthink this one does as-is.  As what [4/8] introduces is a record of\n\"rewrite\", the patch does not make sense either.  These two steps\nwould want to limit themselves to cherry-pick only, no?\n\n> +\tmemset(&proc, 0, sizeof(proc));\n> +\tproc.argv = argv;\n> +\tproc.in = -1;\n> +\tproc.stdout_to_stderr = 1;\n> +\n> +\tcode = start_command(&proc);\n> +\tif (code)\n> +\t\treturn;\n> +\tfor (i = 0; i < rewritten.nr; i++) {\n> +\t\tstruct rewritten_list_item *item = &rewritten.items[i];\n> +\t\tstrbuf_addf(&buf, \"%s %s\\n\", sha1_to_hex(item->from), sha1_to_hex(item->to));\n> +\t}\n> +\twrite_in_full(proc.in, buf.buf, buf.len);\n> +\tclose(proc.in);\n> +\tfinish_command(&proc);\n> +}\n> +\n>  static void remove_sequencer_state(void)\n>  {\n>  \tstruct strbuf seq_dir = STRBUF_INIT;\n> @@ -1099,6 +1130,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n>  \t\t}\n>  \t}\n>  \n> +\trun_rewrite_hook();\n> +\n>  \t/*\n>  \t * Sequence of picks finished successfully; cleanup by\n>  \t * removing the .git/sequencer directory\n> @@ -1136,14 +1169,24 @@ static int sequencer_continue(struct replay_opts *opts)\n>  \t}\n>  \tif (index_differs_from(\"HEAD\", 0))\n>  \t\treturn error_dirty_index(opts);\n> +\t{\n> +\t\tunsigned char to[20];\n> +\t\tif (!read_ref(\"HEAD\", to))\n> +\t\t\tadd_rewritten(todo_list->item->object.sha1, to);\n> +\t}\n>  \ttodo_list = todo_list->next;\n>  \treturn pick_commits(todo_list, opts);\n>  }\n>  \n>  static int single_pick(struct commit *cmit, struct replay_opts *opts)\n>  {\n> +\tint ret;\n>  \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> -\treturn do_pick_commit(cmit, opts);\n> +\tret = do_pick_commit(cmit, opts);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\trun_rewrite_hook();\n> +\treturn 0;\n>  }\n>  \n>  int sequencer_pick_revisions(struct replay_opts *opts)\n"},{"id":"219273","messageId":"7vd2s3dnqs.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"1369799788-24803-8-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 7/8] revert/cherry-pick: add --quiet option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T18:59:23Z","receivedAt":"2013-06-03T18:59:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Makes sense.\n"},{"id":"219280","messageId":"7vtxlfc861.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"7vr4gjdomk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T19:21:10Z","receivedAt":"2013-06-03T19:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> Pretty much what it says on the tin.\n>>\n>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> ---\n>>  Documentation/git-cherry-pick.txt   |  3 +++\n>>  builtin/revert.c                    |  2 ++\n>>  sequencer.c                         |  6 ++++++\n>>  sequencer.h                         |  1 +\n>>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n>>  5 files changed, 25 insertions(+)\n>>\n>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n>> index c205d23..fccd936 100644\n>> --- a/Documentation/git-cherry-pick.txt\n>> +++ b/Documentation/git-cherry-pick.txt\n>> @@ -129,6 +129,9 @@ effect to your index in a row.\n>>  \tredundant commits are ignored.  This option overrides that behavior and\n>>  \tcreates an empty commit object.  Implies `--allow-empty`.\n>>  \n>> +--skip-empty::\n>> +\tInstead of failing, skip commits that are or become empty.\n>\n> Not quite sure.  Is this \"instead of recording an empty commit,\"\n> (which may or may not fail depending on the allow-empty settings)?\n>\n> If that is what this patch is meant to do, I think the change makes\n> sense.\n\nAlso what I noticed while looking at 4-5/8.\n\nWouldn't \"revert --skip-empty A..B\" make sense as well?\n"},{"id":"219298","messageId":"CAMP44s0=qDmSt9y1JOPmDcUqjez=bVbwvLxz2W6W21W04zn2mw@mail.gmail.com","threadId":"33966","inReplyTo":"7vmwr7do7s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/8] cherry-pick: store rewritten commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-03T20:55:44Z","receivedAt":"2013-06-03T20:55:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Jun 3, 2013 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> +static void add_rewritten(unsigned char *from, unsigned char *to)\n>> +{\n>> +     struct rewritten_list_item *item;\n>> +     if (rewritten.nr + 1 >= rewritten.alloc) {\n>> +             rewritten.alloc += 32;\n>> +             rewritten.items = xrealloc(rewritten.items, rewritten.alloc * sizeof(*rewritten.items));\n>> +     }\n>\n> Is there a compelling reason not to use ALLOC_GROW() here?\n\nNope, I just copied the logic from another part of Git.\n\n-- \nFelipe Contreras\n"},{"id":"219299","messageId":"CAMP44s09aczdrObG2ZQxOXx_AodsfYRv1N9efLEg6Dj3Y7Fv9A@mail.gmail.com","threadId":"33966","inReplyTo":"7vhahfdnu6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 5/8] sequencer: run post-rewrite hook","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-03T21:01:44Z","receivedAt":"2013-06-03T21:01:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Jun 3, 2013 at 1:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> As we should.\n>>\n>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> ---\n>>  sequencer.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-\n>>  1 file changed, 44 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index c217716..3aa480e 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -127,6 +127,37 @@ static void add_rewritten(unsigned char *from, unsigned char *to)\n>>       rewritten.nr++;\n>>  }\n>>\n>> +static void run_rewrite_hook(void)\n>> +{\n>> +     struct strbuf buf = STRBUF_INIT;\n>> +     struct child_process proc;\n>> +     const char *argv[3];\n>> +     int code, i;\n>> +\n>> +     argv[0] = find_hook(\"post-rewrite\");\n>> +     if (!argv[0])\n>> +             return;\n>> +\n>> +     argv[1] = \"rebase\";\n>> +     argv[2] = NULL;\n>\n> When the end-user action is \"git cherry-pick A..B\", shouldn't\n> the rewrite hook be called with \"cherry-pick\" not \"rebase\" as its\n> first argument?\n\nIndeed.\n\n> More importantly, doesn't \"git revert A..B\" also trigger the\n> codepath for [4/8] and hence this function?\n\nTrue.\n\n> I think [3/8] --skip-empty makes sense also for revert, but I do not\n> think this one does as-is.  As what [4/8] introduces is a record of\n> \"rewrite\", the patch does not make sense either.  These two steps\n> would want to limit themselves to cherry-pick only, no?\n\nProbably.\n\n-- \nFelipe Contreras\n"},{"id":"219301","messageId":"CAMP44s1GQ2iwwscx4Sx1cxK0U6yWWYvOJqDk_FO_GkFcNKREYw@mail.gmail.com","threadId":"33966","inReplyTo":"7vr4gjdomk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-03T21:10:46Z","receivedAt":"2013-06-03T21:10:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Jun 3, 2013 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> Pretty much what it says on the tin.\n>>\n>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> ---\n>>  Documentation/git-cherry-pick.txt   |  3 +++\n>>  builtin/revert.c                    |  2 ++\n>>  sequencer.c                         |  6 ++++++\n>>  sequencer.h                         |  1 +\n>>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n>>  5 files changed, 25 insertions(+)\n>>\n>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n>> index c205d23..fccd936 100644\n>> --- a/Documentation/git-cherry-pick.txt\n>> +++ b/Documentation/git-cherry-pick.txt\n>> @@ -129,6 +129,9 @@ effect to your index in a row.\n>>       redundant commits are ignored.  This option overrides that behavior and\n>>       creates an empty commit object.  Implies `--allow-empty`.\n>>\n>> +--skip-empty::\n>> +     Instead of failing, skip commits that are or become empty.\n>\n> Not quite sure.  Is this \"instead of recording an empty commit,\"\n> (which may or may not fail depending on the allow-empty settings)?\n\nWe are explaining --skip-empty, not --allow-empty.\n\n-- \nFelipe Contreras\n"},{"id":"219304","messageId":"7vehcidg1i.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"CAMP44s1GQ2iwwscx4Sx1cxK0U6yWWYvOJqDk_FO_GkFcNKREYw@mail.gmail.com","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T21:45:45Z","receivedAt":"2013-06-03T21:45:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Mon, Jun 3, 2013 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>\n>>> Pretty much what it says on the tin.\n>>>\n>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>>> ---\n>>>  Documentation/git-cherry-pick.txt   |  3 +++\n>>>  builtin/revert.c                    |  2 ++\n>>>  sequencer.c                         |  6 ++++++\n>>>  sequencer.h                         |  1 +\n>>>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n>>>  5 files changed, 25 insertions(+)\n>>>\n>>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n>>> index c205d23..fccd936 100644\n>>> --- a/Documentation/git-cherry-pick.txt\n>>> +++ b/Documentation/git-cherry-pick.txt\n>>> @@ -129,6 +129,9 @@ effect to your index in a row.\n>>>       redundant commits are ignored.  This option overrides that behavior and\n>>>       creates an empty commit object.  Implies `--allow-empty`.\n>>>\n>>> +--skip-empty::\n>>> +     Instead of failing, skip commits that are or become empty.\n>>\n>> Not quite sure.  Is this \"instead of recording an empty commit,\"\n>> (which may or may not fail depending on the allow-empty settings)?\n>\n> We are explaining --skip-empty, not --allow-empty.\n\nExactly.  That is why I found the \"Instead of failing\" questionable.\nIt is very easy to read the above as \"commits that are or become\nempty usually causes the command to fail, and this is a way to cause\nit not to fail.\".\n\nIt is true that\n\n    cherry-pick A\n\nwhen A becomes empty, dies.  But\n\n    cherry-pick --allow-empty A\n\nwhen A becomes empty, does not fail, but still does create an empty\ncommit.  A large difference with --skip-empty, which applies to use\nscenario different from --allow-empty was meant to address, is that\n\n    cherry-pick --skip-empty A\n\nwhen A becomes empty, does not fail and does not create an empty\ncommit, either.\n\nI think just \"Skip commits that are or become empty\" without saying\n\"Instead of failing,\" is fine, too.\n"},{"id":"219327","messageId":"CALWbr2w+Rvf07MGhj7YaYea7fwjE-ikgf+2OrNvmZKcGsu1uJA@mail.gmail.com","threadId":"33966","inReplyTo":"CAMP44s1GQ2iwwscx4Sx1cxK0U6yWWYvOJqDk_FO_GkFcNKREYw@mail.gmail.com","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-06-04T06:31:29Z","receivedAt":"2013-06-04T06:31:29Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Mon, Jun 3, 2013 at 11:10 PM, Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n> On Mon, Jun 3, 2013 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>\n>>> Pretty much what it says on the tin.\n>>>\n>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>>> ---\n>>>  Documentation/git-cherry-pick.txt   |  3 +++\n>>>  builtin/revert.c                    |  2 ++\n>>>  sequencer.c                         |  6 ++++++\n>>>  sequencer.h                         |  1 +\n>>>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n>>>  5 files changed, 25 insertions(+)\n>>>\n>>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n>>> index c205d23..fccd936 100644\n>>> --- a/Documentation/git-cherry-pick.txt\n>>> +++ b/Documentation/git-cherry-pick.txt\n>>> @@ -129,6 +129,9 @@ effect to your index in a row.\n>>>       redundant commits are ignored.  This option overrides that behavior and\n>>>       creates an empty commit object.  Implies `--allow-empty`.\n>>>\n>>> +--skip-empty::\n>>> +     Instead of failing, skip commits that are or become empty.\n>>\n>> Not quite sure.  Is this \"instead of recording an empty commit,\"\n>> (which may or may not fail depending on the allow-empty settings)?\n>\n> We are explaining --skip-empty, not --allow-empty.\n\nBy the way, I guess both options are incompatible ? I think it would\nbe nice to fail if both are specified.\n\nAntoine,\n"},{"id":"219339","messageId":"877giaxn6i.fsf@linux-k42r.v.cablecom.net","threadId":"33966","inReplyTo":"7vhahfdnu6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 5/8] sequencer: run post-rewrite hook","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-04T09:03:33Z","receivedAt":"2013-06-04T09:03:33Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> +static void run_rewrite_hook(void)\n>> +{\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tstruct child_process proc;\n>> +\tconst char *argv[3];\n>> +\tint code, i;\n>> +\n>> +\targv[0] = find_hook(\"post-rewrite\");\n>> +\tif (!argv[0])\n>> +\t\treturn;\n>> +\n>> +\targv[1] = \"rebase\";\n>> +\targv[2] = NULL;\n>\n> When the end-user action is \"git cherry-pick A..B\", shouldn't\n> the rewrite hook be called with \"cherry-pick\" not \"rebase\" as its\n> first argument?\n\nThe whole function was copied from builtin/commit.c except for that\nstring.  So it should be refactored anyway, with the \"cherry-pick\" being\nan argument.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"219381","messageId":"CAMP44s2DvtiT6hi-RoPt_bVdGr01S367usz=089E1Ht+Bvh-tA@mail.gmail.com","threadId":"33966","inReplyTo":"7vehcidg1i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-04T17:10:00Z","receivedAt":"2013-06-04T17:10:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Jun 3, 2013 at 4:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> On Mon, Jun 3, 2013 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>>\n>>>> Pretty much what it says on the tin.\n>>>>\n>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>>>> ---\n>>>>  Documentation/git-cherry-pick.txt   |  3 +++\n>>>>  builtin/revert.c                    |  2 ++\n>>>>  sequencer.c                         |  6 ++++++\n>>>>  sequencer.h                         |  1 +\n>>>>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n>>>>  5 files changed, 25 insertions(+)\n>>>>\n>>>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n>>>> index c205d23..fccd936 100644\n>>>> --- a/Documentation/git-cherry-pick.txt\n>>>> +++ b/Documentation/git-cherry-pick.txt\n>>>> @@ -129,6 +129,9 @@ effect to your index in a row.\n>>>>       redundant commits are ignored.  This option overrides that behavior and\n>>>>       creates an empty commit object.  Implies `--allow-empty`.\n>>>>\n>>>> +--skip-empty::\n>>>> +     Instead of failing, skip commits that are or become empty.\n>>>\n>>> Not quite sure.  Is this \"instead of recording an empty commit,\"\n>>> (which may or may not fail depending on the allow-empty settings)?\n>>\n>> We are explaining --skip-empty, not --allow-empty.\n>\n> Exactly.  That is why I found the \"Instead of failing\" questionable.\n> It is very easy to read the above as \"commits that are or become\n> empty usually causes the command to fail, and this is a way to cause\n> it not to fail.\".\n>\n> It is true that\n>\n>     cherry-pick A\n>\n> when A becomes empty, dies.  But\n>\n>     cherry-pick --allow-empty A\n>\n> when A becomes empty, does not fail, but still does create an empty\n> commit.  A large difference with --skip-empty, which applies to use\n> scenario different from --allow-empty was meant to address, is that\n>\n>     cherry-pick --skip-empty A\n>\n> when A becomes empty, does not fail and does not create an empty\n> commit, either.\n\nWe are not explaining --allow-empty.\n\nWhat happens when you do --skip-empty --allow-empty? Somebody\nsuggested a new option, so we could do --foo-empty=skip,allow to\nclarify that.\n\n> I think just \"Skip commits that are or become empty\" without saying\n> \"Instead of failing,\" is fine, too.\n\nI think \"Instead of failing\" makes perfect sense, because it's not our\njob to describe what other options do, simply explain what this option\ndo. If the user is interested in other options, he can read them in\nthe help for those.\n\n-- \nFelipe Contreras\n"},{"id":"219386","messageId":"7vbo7l93t6.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"CAMP44s2DvtiT6hi-RoPt_bVdGr01S367usz=089E1Ht+Bvh-tA@mail.gmail.com","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-04T17:35:49Z","receivedAt":"2013-06-04T17:35:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> I think just \"Skip commits that are or become empty\" without saying\n>> \"Instead of failing,\" is fine, too.\n>\n> I think \"Instead of failing\" makes perfect sense, because it's not our\n> job to describe what other options do,...\n> ...\n> simply explain what this option\n> do.\n\nWhich means \"Skip commits\" and nothing else.  Saying \"Instead of\nfailing\" explains what would happen if you ran the command without\nany option, which is entirely irrelevant, just like the case when\nyou ran the command without an unrelated option --allow-empty.\n\nWe share the same \"the --skip-empty option does not have anything to\ndo with the --allow-empty option, and we do not have to say anything\nabout what happens when the command is run with that unrelated\noption\".\n\nBut we are viewing it from a different angle.\n"},{"id":"219387","messageId":"CAMP44s2_Xr4bymmDezTF8BMiMKQZFpNuxXpFZd_TGJ3BkJWdGw@mail.gmail.com","threadId":"33966","inReplyTo":"7vbo7l93t6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-04T17:40:14Z","receivedAt":"2013-06-04T17:40:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Jun 4, 2013 at 12:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>>> I think just \"Skip commits that are or become empty\" without saying\n>>> \"Instead of failing,\" is fine, too.\n>>\n>> I think \"Instead of failing\" makes perfect sense, because it's not our\n>> job to describe what other options do,...\n>> ...\n>> simply explain what this option\n>> do.\n>\n> Which means \"Skip commits\" and nothing else.  Saying \"Instead of\n> failing\" explains what would happen if you ran the command without\n> any option,\n\nWhich *BY FAR* the most widely use-case of cherry-pick. What? 99% of the time?\n\n> which is entirely irrelevant,\n\nIt's totally and completely relevant. It couldn't be more relevant.\n\n> We share the same \"the --skip-empty option does not have anything to\n> do with the --allow-empty option, and we do not have to say anything\n> about what happens when the command is run with that unrelated\n> option\".\n\nYou didn't answer, what happens when you run --skip-empty and --allow-empty?\n\n-- \nFelipe Contreras\n"},{"id":"219393","messageId":"7vvc5t7mpm.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"CAMP44s2_Xr4bymmDezTF8BMiMKQZFpNuxXpFZd_TGJ3BkJWdGw@mail.gmail.com","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-04T18:30:29Z","receivedAt":"2013-06-04T18:30:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> You didn't answer, what happens when you run --skip-empty and --allow-empty?\n\nI'll answer to a slightly different question: What should happen?\n\nI think it should error out, because --allow-empty is about\n\"allowing empty commits to be preserved\", and --skip-empty is about\n\"skipping empty commits (as it says on the package)\".\n"},{"id":"219454","messageId":"CAMP44s1X=M2sV9OiHS_ggVjmu6txBQVSdK+aK6PnQm5-9EpQgw@mail.gmail.com","threadId":"33966","inReplyTo":"7vvc5t7mpm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-05T14:52:27Z","receivedAt":"2013-06-05T14:52:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Jun 4, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> You didn't answer, what happens when you run --skip-empty and --allow-empty?\n>\n> I'll answer to a slightly different question: What should happen?\n>\n> I think it should error out, because --allow-empty is about\n> \"allowing empty commits to be preserved\", and --skip-empty is about\n> \"skipping empty commits (as it says on the package)\".\n\nExactly, so they are related after all. However, --allow-empty says this:\n\n\"By default, cherry-picking an empty commit will fail,\"\n\nShould we change that too if we introduce --skip-empty?\n\nI don't think so. I think it makes more sense to have a new\n--empty-commits=[keep|skip|fail] option, so we don't have to document\nhow each option affects the other, and what is the default. Or rather;\nthe option documents that.\n\nIn fact, it might make sense to change the default in v2.0 to\n--empty-commits=skip. If it makes sense in 'git rebase', why doesn't\nit in 'git cherry-pick'?\n\n-- \nFelipe Contreras\n"},{"id":"219466","messageId":"7vsj0w1l4v.fsf@alter.siamese.dyndns.org","threadId":"33966","inReplyTo":"CAMP44s1X=M2sV9OiHS_ggVjmu6txBQVSdK+aK6PnQm5-9EpQgw@mail.gmail.com","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-05T18:13:20Z","receivedAt":"2013-06-05T18:13:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Tue, Jun 4, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>\n>>> You didn't answer, what happens when you run --skip-empty and --allow-empty?\n>>\n>> I'll answer to a slightly different question: What should happen?\n>>\n>> I think it should error out, because --allow-empty is about\n>> \"allowing empty commits to be preserved\", and --skip-empty is about\n>> \"skipping empty commits (as it says on the package)\".\n>\n> Exactly, so they are related after all. However, --allow-empty says this:\n>\n> \"By default, cherry-picking an empty commit will fail,\"\n\nOK, that is a very good point.  Especially because by the time\nreader reaches this new description, --allow-empty has already\nmentioned this, we can just be brief and it is sufficient to say\n\"Instead of failing,\" upfront.\n\n> In fact, it might make sense to change the default in v2.0 to\n> --empty-commits=skip. If it makes sense in 'git rebase', why doesn't\n> it in 'git cherry-pick'?\n\nI think the primary reason behind the \"Why are you picking a no-op?\nLet me stop to make sure you didn't make a mistake.\" is because\ncherry-pick and revert have long been operations for a single commit\nexplicitly given by the user, not bunch of commits in a range.\n\nThese two operating modes are conceptually very different, even when\nwe consider scripted use.  A script that feeds one commit at a time\nhas a chance to do patch equivalence or user-interactive filtering\non its own, and would be helped by the \"are you sure you meant to\nrecord that no-op?\" check if it filtered in a wrong way (e.g. the\nuser specified an empty commit by mistake).  One that feeds a range,\non the other hand, relies on or at least expects cherry-pick to have\nsuch smart, and a smarter cherry-pick could select what to pick from\nthe given range.\n\nIn the longer term, especially if we envision to move large part of\nlogic to prepare the sequencer insn list from rebase to cherry-pick,\na ranged cherry-pick should learn a way to filter the range with\npatch equivalence, for example.  Once that happens, the behaviour\nwould become inconsistent at the end-user level if we stopped at a\ncommit only because it was not exactly patch equivalent to another\none that is already on the branch we are cherry-picking to, but\nended up to be a no-op, while we did not stop at another commit\nbecause patch equivalence filtered it out beforehand to skip it.\nSkipping a no-op by default would remove that inconsistency.\n\nSo in that sense, one could argue that it may be a bugfix to skip\ncommits that become no-op when replayed, when picking a range of\ncommits (but not a single one).  And I do not think you would need\nto wait until 2.0 for such a change.\n"},{"id":"219480","messageId":"CAMP44s3SxV-rjcjh+sbmjdu9sme8B=v5x2KbJ8T4oc1=1CZbFw@mail.gmail.com","threadId":"33966","inReplyTo":"7vsj0w1l4v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-06T05:01:26Z","receivedAt":"2013-06-06T05:01:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Jun 5, 2013 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> On Tue, Jun 4, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>>\n>>>> You didn't answer, what happens when you run --skip-empty and --allow-empty?\n>>>\n>>> I'll answer to a slightly different question: What should happen?\n>>>\n>>> I think it should error out, because --allow-empty is about\n>>> \"allowing empty commits to be preserved\", and --skip-empty is about\n>>> \"skipping empty commits (as it says on the package)\".\n>>\n>> Exactly, so they are related after all. However, --allow-empty says this:\n>>\n>> \"By default, cherry-picking an empty commit will fail,\"\n>\n> OK, that is a very good point.  Especially because by the time\n> reader reaches this new description, --allow-empty has already\n> mentioned this, we can just be brief and it is sufficient to say\n> \"Instead of failing,\" upfront.\n>\n>> In fact, it might make sense to change the default in v2.0 to\n>> --empty-commits=skip. If it makes sense in 'git rebase', why doesn't\n>> it in 'git cherry-pick'?\n>\n> I think the primary reason behind the \"Why are you picking a no-op?\n> Let me stop to make sure you didn't make a mistake.\" is because\n> cherry-pick and revert have long been operations for a single commit\n> explicitly given by the user, not bunch of commits in a range.\n\nYeah, but this has changed already.\n\n> These two operating modes are conceptually very different, even when\n> we consider scripted use.  A script that feeds one commit at a time\n> has a chance to do patch equivalence or user-interactive filtering\n> on its own, and would be helped by the \"are you sure you meant to\n> record that no-op?\" check if it filtered in a wrong way (e.g. the\n> user specified an empty commit by mistake).  One that feeds a range,\n> on the other hand, relies on or at least expects cherry-pick to have\n> such smart, and a smarter cherry-pick could select what to pick from\n> the given range.\n\nHow would a script know that a single pick ends up as a no-op? It\ncan't know what is the reason the cherry-pick failed. The only way to\nknow for sure is to check the last commit, and for that we don't need\nthe last cherry-pick to fail.\n\n> In the longer term, especially if we envision to move large part of\n> logic to prepare the sequencer insn list from rebase to cherry-pick,\n> a ranged cherry-pick should learn a way to filter the range with\n> patch equivalence, for example.  Once that happens, the behaviour\n> would become inconsistent at the end-user level if we stopped at a\n> commit only because it was not exactly patch equivalent to another\n> one that is already on the branch we are cherry-picking to, but\n> ended up to be a no-op, while we did not stop at another commit\n> because patch equivalence filtered it out beforehand to skip it.\n> Skipping a no-op by default would remove that inconsistency.\n>\n> So in that sense, one could argue that it may be a bugfix to skip\n> commits that become no-op when replayed, when picking a range of\n> commits (but not a single one).  And I do not think you would need\n> to wait until 2.0 for such a change.\n\nRight. But first we need to agree that failing an empty cherry-pick\nserves no purpose.\n\n-- \nFelipe Contreras\n"}]}