{"thread":{"id":"44930","subject":"[RFC PATCH] Option to allow cherry-pick to skip empty commits","startedAt":"2017-01-21T13:16:50Z","lastAt":"2017-01-23T22:33:59Z","messageCount":7,"participants":["Giuseppe Bilotta","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"309889","messageId":"20170121131629.16326-1-giuseppe.bilotta@gmail.com","threadId":"44930","inReplyTo":null,"subject":"[RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2017-01-21T13:16:29Z","receivedAt":"2017-01-21T13:16:50Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This allows cherry-picking a set of commits, some of which may be\nredundant, without stopping to ask for the user intervention.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n Documentation/git-cherry-pick.txt |  4 ++++\n builtin/revert.c                  |  1 +\n sequencer.c                       | 45 +++++++++++++++++++++++++++++++--------\n sequencer.h                       |  1 +\n 4 files changed, 42 insertions(+), 9 deletions(-)\n\n\nI would like to add to cherry-pick the ability to skip patches. To this\nend, I'm working on two options: a general `--skip-empty` option to\nhandle redundant and empty commits by simply skipping them (no user\nintervention), and a `--skip` option as an alternative form to\n`--continue` to skip the ongoing (conflicting or empty) commit.\n\nThe patch here presents my implementation of the `--skip-empty` option,\nincluding documentation. Comments welcome.\n\nHowever, it has been some time since I dwelved into git internals and\nI'm having issues with the `--skip` option. My idea was to rely on the\nexisting implementation for `--continue`, and have it run the rollback\ninstead of the commit. However, I'm finding that after the rollback is\ndone (I'm using the existing rollback function in sequencer, which calls\n`git reset --merge`, but I've also tried a `reset --hard`) the sequencer\ndoes not continue because e.g. if the cherry-pick had stopped due to a\nconflict, it still finds unmerged paths, and I can't find a way to tell\nthe sequencer to just re-read the index from scratch. Opinions welcome\n(I can provide my WIP patch to make discussion easier).\n\nOn the same note, I've noticed that `git reset` clears CHERRY_PICK_HEAD.\nI find this a little confusing, both for humans for the code itself\n(since it doesn't really abort an ongoing cherry-pick, yet the sequencer\nwill think there is no running cherry-pick). This is particularly bad\nbecause `git reset` is one of the strategies proposed to solve conflicts\nin cherry-picking. So I was wondering if this was intentional or a side\neffect of something else that I might look into cleaning up during this\npatch series.\n\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex d35d771fc8..ffced816d6 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -138,6 +138,10 @@ effect to your index in a row.\n \texamine the commit. This option overrides that behavior and\n \tcreates an empty commit object.  Implies `--allow-empty`.\n \n+--skip-empty::\n+\tThis option causes empty and redundant cherry-picked commits to\n+\tbe skipped without requesting the user intervention.\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 4ca5b51544..ffdd367f99 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -102,6 +102,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOL(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n+\t\t\tOPT_BOOL(0, \"skip-empty\", &opts->skip_empty, N_(\"skip redundant, empty commits\")),\n \t\t\tOPT_END(),\n \t\t};\n \t\toptions = parse_options_concat(options, cp_extra);\ndiff --git a/sequencer.c b/sequencer.c\nindex 9adb7bbf1d..72962fd2fa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -550,22 +550,32 @@ static int is_original_commit_empty(struct commit *commit)\n \n /*\n  * Do we run \"git commit\" with \"--allow-empty\"?\n+ *\n+ * Or do we just skip this empty commit?\n+ *\n+ * Returns 1 if a commit should be done with --allow-empty,\n+ *         0 if a commit should be done without --allow-empty,\n+ *         2 if no commit should be done at all (skip empty commit)\n+ *         negative values in case of error\n+ *\n  */\n-static int allow_empty(struct replay_opts *opts, struct commit *commit)\n+static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)\n {\n \tint index_unchanged, empty_commit;\n \n \t/*\n-\t * Three cases:\n+\t * Four cases:\n \t *\n-\t * (1) we do not allow empty at all and error out.\n+\t * (1) we do not allow empty at all and error out;\n \t *\n-\t * (2) we allow ones that were initially empty, but\n+\t * (2) we skip empty commits altogether;\n+\t *\n+\t * (3) we allow ones that were initially empty, but\n \t * forbid the ones that become empty;\n \t *\n-\t * (3) we allow both.\n+\t * (4) we allow both.\n \t */\n-\tif (!opts->allow_empty)\n+\tif (!opts->allow_empty && !opts->skip_empty)\n \t\treturn 0; /* let \"git commit\" barf as necessary */\n \n \tindex_unchanged = is_index_unchanged();\n@@ -574,6 +584,9 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)\n \tif (!index_unchanged)\n \t\treturn 0; /* we do not have to say --allow-empty */\n \n+\tif (opts->skip_empty)\n+\t\treturn 2;\n+\n \tif (opts->keep_redundant_commits)\n \t\treturn 1;\n \n@@ -612,7 +625,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \tconst char *base_label, *next_label;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0, allow;\n+\tint res = 0, unborn = 0, allow;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -771,12 +784,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tgoto leave;\n \t}\n \n-\tallow = allow_empty(opts, commit);\n+\tallow = allow_or_skip_empty(opts, commit);\n \tif (allow < 0) {\n \t\tres = allow;\n \t\tgoto leave;\n \t}\n-\tif (!opts->no_commit)\n+\t/* allow == 2 means skip this commit */\n+\tif (allow != 2 && !opts->no_commit)\n \t\tres = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),\n \t\t\t\t     opts, allow, opts->edit, 0, 0);\n \n@@ -983,6 +997,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.record-origin\"))\n \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.skip-empty\"))\n+\t\topts->skip_empty = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.allow-ff\"))\n \t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.mainline\"))\n@@ -1231,6 +1247,8 @@ static int save_opts(struct replay_opts *opts)\n \t\tres |= git_config_set_in_file_gently(opts_file, \"options.signoff\", \"true\");\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file, \"options.record-origin\", \"true\");\n+\tif (opts->skip_empty)\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.skip-empty\", \"true\");\n \tif (opts->allow_ff)\n \t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n@@ -1306,6 +1324,14 @@ int sequencer_continue(struct replay_opts *opts)\n \tif ((res = read_populate_todo(&todo_list, opts)))\n \t\tgoto release_todo_list;\n \n+\t/* check if there is something to commit */\n+\tres = is_index_unchanged();\n+\tif (res < 0)\n+\t\tgoto release_todo_list;\n+\n+\tif (res && opts->skip_empty)\n+\t\tgoto skip_this_commit;\n+\n \t/* Verify that the conflict has been resolved */\n \tif (file_exists(git_path_cherry_pick_head()) ||\n \t    file_exists(git_path_revert_head())) {\n@@ -1317,6 +1343,7 @@ int sequencer_continue(struct replay_opts *opts)\n \t\tres = error_dirty_index(opts);\n \t\tgoto release_todo_list;\n \t}\n+skip_this_commit:\n \ttodo_list.current++;\n \tres = pick_commits(&todo_list, opts);\n release_todo_list:\ndiff --git a/sequencer.h b/sequencer.h\nindex 7a513c576b..c747cfcfc7 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -23,6 +23,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-- \n2.11.0.585.g56041942c3.dirty\n\n"},{"id":"309965","messageId":"xmqqpojeu14h.fsf@gitster.mtv.corp.google.com","threadId":"44930","inReplyTo":"20170121131629.16326-1-giuseppe.bilotta@gmail.com","subject":"Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-22T23:27:10Z","receivedAt":"2017-01-22T23:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> This allows cherry-picking a set of commits, some of which may be\n> redundant, without stopping to ask for the user intervention.\n>\n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n> ---\n>  Documentation/git-cherry-pick.txt |  4 ++++\n>  builtin/revert.c                  |  1 +\n>  sequencer.c                       | 45 +++++++++++++++++++++++++++++++--------\n>  sequencer.h                       |  1 +\n>  4 files changed, 42 insertions(+), 9 deletions(-)\n>\n>\n> I would like to add to cherry-pick the ability to skip patches. To this\n> end, I'm working on two options: a general `--skip-empty` option to\n> handle redundant and empty commits by simply skipping them (no user\n> intervention), and a `--skip` option as an alternative form to\n> `--continue` to skip the ongoing (conflicting or empty) commit.\n>\n> The patch here presents my implementation of the `--skip-empty` option,\n> including documentation. Comments welcome.\n\nSounds like a useful thing to do.\n\n> +static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)\n>  {\n>  \tint index_unchanged, empty_commit;\n>  \n>  \t/*\n> -\t * Three cases:\n> +\t * Four cases:\n>  \t *\n> -\t * (1) we do not allow empty at all and error out.\n> +\t * (1) we do not allow empty at all and error out;\n>  \t *\n> -\t * (2) we allow ones that were initially empty, but\n> +\t * (2) we skip empty commits altogether;\n> +\t *\n> +\t * (3) we allow ones that were initially empty, but\n>  \t * forbid the ones that become empty;\n>  \t *\n> -\t * (3) we allow both.\n> +\t * (4) we allow both.\n>  \t */\n\nThe original gave callers the choice to tell two cases (a commit was\nempty in the original history, and a commit that was not empty in\nthe original history turns out to be redundant) apart and handle\nthem differently.  I tend to agree that skipping the former should\nbe the norm, and also I think it is sensible to drop the latter, and\nthat is what your updated (2) gives us, I think.\n\nBut I would suspect that it would rather be common to have a\ndeliberately empty commit in the original as a marker in a history\nand want to keep that across cherry-picking a series, while wanting\nto discard/skip patches that are already applied in an updated base.\nShouldn't that be supported as the fifth case?\n"},{"id":"309971","messageId":"CAOxFTczut_CGGxmbrVFzhn_o4=StTOY6N1mEAw75Ro2Q4tzWgQ@mail.gmail.com","threadId":"44930","inReplyTo":"xmqqpojeu14h.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2017-01-23T10:18:37Z","receivedAt":"2017-01-23T10:19:04Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Jan 23, 2017 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n>> +static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)\n>>  {\n>>       int index_unchanged, empty_commit;\n>>\n>>       /*\n>> -      * Three cases:\n>> +      * Four cases:\n>>        *\n>> -      * (1) we do not allow empty at all and error out.\n>> +      * (1) we do not allow empty at all and error out;\n>>        *\n>> -      * (2) we allow ones that were initially empty, but\n>> +      * (2) we skip empty commits altogether;\n>> +      *\n>> +      * (3) we allow ones that were initially empty, but\n>>        * forbid the ones that become empty;\n>>        *\n>> -      * (3) we allow both.\n>> +      * (4) we allow both.\n>>        */\n>\n> The original gave callers the choice to tell two cases (a commit was\n> empty in the original history, and a commit that was not empty in\n> the original history turns out to be redundant) apart and handle\n> them differently.  I tend to agree that skipping the former should\n> be the norm, and also I think it is sensible to drop the latter, and\n> that is what your updated (2) gives us, I think.\n>\n> But I would suspect that it would rather be common to have a\n> deliberately empty commit in the original as a marker in a history\n> and want to keep that across cherry-picking a series, while wanting\n> to discard/skip patches that are already applied in an updated base.\n> Shouldn't that be supported as the fifth case?\n\nI was actually wondering about that. I guess the best approach\n(symmetric wrt to the --allow) would be to intro introduce\n--skip-empty _and_ --skip-redundant, with the former implying the\nlatter.\n\nBy the way, I noticed going over the code that the -allow options are\nnot stored, so that in case of interruption they will be reset, is\nthis intentional or a bug?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"309990","messageId":"xmqqd1fdtzgv.fsf@gitster.mtv.corp.google.com","threadId":"44930","inReplyTo":"CAOxFTczut_CGGxmbrVFzhn_o4=StTOY6N1mEAw75Ro2Q4tzWgQ@mail.gmail.com","subject":"Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-23T18:15:12Z","receivedAt":"2017-01-23T18:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> By the way, I noticed going over the code that the -allow options are\n> not stored, so that in case of interruption they will be reset, is\n> this intentional or a bug?\n\nI do not know offhand, but given the history of the two commands, I\nwould guess it was a bug simply overlooked when people bolted \"a\nseries of commits\" mode onto these commands that originally worked\nonly on a single commit.\n"},{"id":"310008","messageId":"CAOxFTcxRjWS-=wyGBVOtg-tfCHrqqAr9rVOcvkyxXwJHonN_Tg@mail.gmail.com","threadId":"44930","inReplyTo":"xmqqd1fdtzgv.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2017-01-23T20:05:36Z","receivedAt":"2017-01-23T20:06:04Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Jan 23, 2017 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>\n>> By the way, I noticed going over the code that the -allow options are\n>> not stored, so that in case of interruption they will be reset, is\n>> this intentional or a bug?\n>\n> I do not know offhand, but given the history of the two commands, I\n> would guess it was a bug simply overlooked when people bolted \"a\n> series of commits\" mode onto these commands that originally worked\n> only on a single commit.\n\nIt seems like a bug to me. I'll prepare a new series that also fixes\nthis. I still don't see how to force a complete reread of the index\nafter running a git reset (which I need for the --skip command), but\nmaybe I'll add a WIP of what I have for --skip so far and people can\ncomment on that.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"310009","messageId":"xmqqfuk9r0zw.fsf@gitster.mtv.corp.google.com","threadId":"44930","inReplyTo":"CAOxFTcxRjWS-=wyGBVOtg-tfCHrqqAr9rVOcvkyxXwJHonN_Tg@mail.gmail.com","subject":"Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-23T20:10:27Z","receivedAt":"2017-01-23T20:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> ... I still don't see how to force a complete reread of the index\n> after running a git reset (which I need for the --skip command), ...\n\nDo you mean discard_index() or discard_cache() followed by\nread_index() or read_cache(), or do you mean something more\nelaborate?\n"},{"id":"310042","messageId":"CAOxFTcybikUwqn8kTTkuKPo3B7KiYJvneoDrr=m2fpN5RefbGQ@mail.gmail.com","threadId":"44930","inReplyTo":"xmqqfuk9r0zw.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2017-01-23T22:33:07Z","receivedAt":"2017-01-23T22:33:59Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Jan 23, 2017 at 9:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>\n>> ... I still don't see how to force a complete reread of the index\n>> after running a git reset (which I need for the --skip command), ...\n>\n> Do you mean discard_index() or discard_cache() followed by\n> read_index() or read_cache(), or do you mean something more\n> elaborate?\n\nApparently discard_cache() followed by read_cache() is exactly what I\nneeded. New patchset incoming 8-)\n\n\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}