{"thread":{"id":"52096","subject":"[PATCH 0/3] commit: fix advice for empty commits during rebases","startedAt":"2019-10-22T23:30:26Z","lastAt":"2020-02-26T19:45:51Z","messageCount":31,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Phillip Wood","Johannes Schindelin","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"384640","messageId":"pull.417.git.1571787022.gitgitgadget@gmail.com","threadId":"52096","inReplyTo":null,"subject":"[PATCH 0/3] commit: fix advice for empty commits during rebases","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-22T23:30:19Z","receivedAt":"2019-10-22T23:30:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we\nintroduced a helpful message that suggests to run git cherry-pick --skip \n(instead of the previous message that talked about git reset) when a\ncherry-pick failed due to an empty patch.\n\nHowever, the same message is displayed during a rebase, when the patch\nto-be-committed is empty. In this case, git reset would also have worked,\nbut git cherry-pick --skip does not work. This is a regression introduced in\nthis cycle.\n\nLet's be more careful here.\n\nJohannes Schindelin (3):\n cherry-pick: add test for `--skip` advice in `git commit`\n sequencer: export the function to get the path of `.git/rebase-merge/`\n commit: give correct advice for empty commit during a rebase\n\n builtin/commit.c | 33 ++++++++++++++++++++++++---------\n sequencer.c | 4 ++--\n sequencer.h | 1 +\n t/t3403-rebase-skip.sh | 9 +++++++++\n t/t3510-cherry-pick-sequence.sh | 3 ++-\n 5 files changed, 38 insertions(+), 12 deletions(-)\n\n\nbase-commit: d966095db01190a2196e31195ea6fa0c722aa732\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-417%2Fdscho%2Ffix-commit-advice-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-417/dscho/fix-commit-advice-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/417\n-- \ngitgitgadget\n"},{"id":"384641","messageId":"43211b82bcc5b18c90106ee4f01e180cad35ac87.1571787022.git.gitgitgadget@gmail.com","threadId":"52096","inReplyTo":"pull.417.git.1571787022.gitgitgadget@gmail.com","subject":"[PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-22T23:30:21Z","receivedAt":"2019-10-22T23:30:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe presence of this path will be used in the next commit to fix an\nincorrect piece of advice in `git commit` when in the middle of a\nrebase.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 4 ++--\n sequencer.h | 1 +\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 9d5964fd81..5bd7e9d05a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -47,7 +47,7 @@ static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n static GIT_PATH_FUNC(git_path_abort_safety_file, \"sequencer/abort-safety\")\n \n-static GIT_PATH_FUNC(rebase_path, \"rebase-merge\")\n+GIT_PATH_FUNC(git_path_rebase_merge_dir, \"rebase-merge\")\n /*\n * The file containing rebase commands, comments, and empty lines.\n * This file is created by \"git rebase -i\" then edited by the user. As\n@@ -218,7 +218,7 @@ static inline int is_rebase_i(const struct replay_opts *opts)\n static const char *get_dir(const struct replay_opts *opts)\n {\n \tif (is_rebase_i(opts))\n-\t\treturn rebase_path();\n+\t\treturn git_path_rebase_merge_dir();\n \treturn git_path_seq_dir();\n }\n \ndiff --git a/sequencer.h b/sequencer.h\nindex 574260f621..505852d7d1 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -9,6 +9,7 @@ struct repository;\n \n const char *git_path_commit_editmsg(void);\n const char *git_path_seq_dir(void);\n+const char *git_path_rebase_merge_dir(void);\n const char *rebase_path_todo(void);\n const char *rebase_path_todo_backup(void);\n \n-- \ngitgitgadget\n\n"},{"id":"384642","messageId":"7e7bb43e0803aefed984b0206ab8ecbdde83190d.1571787022.git.gitgitgadget@gmail.com","threadId":"52096","inReplyTo":"pull.417.git.1571787022.gitgitgadget@gmail.com","subject":"[PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-22T23:30:20Z","receivedAt":"2019-10-22T23:30:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n`git commit` learned to suggest to run `git cherry-pick --skip` when\ntrying to cherry-pick an empty patch, but that was never tested for.\n\nHere is a test that verifies that a message is given to the user that\ncontains the correct invocation.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3510-cherry-pick-sequence.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 793bcc7fe3..5b94fdaa67 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -123,7 +123,8 @@ test_expect_success 'revert --skip to skip commit' '\n test_expect_success 'skip \"empty\" commit' '\n \tpristine_detach picked &&\n \ttest_commit dummy foo d &&\n-\ttest_must_fail git cherry-pick anotherpick &&\n+\ttest_must_fail git cherry-pick anotherpick 2>err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n \tgit cherry-pick --skip &&\n \ttest_cmp_rev dummy HEAD\n '\n-- \ngitgitgadget\n\n"},{"id":"384643","messageId":"0d168b4a75c65e786f4b14f5da723957c32fa390.1571787022.git.gitgitgadget@gmail.com","threadId":"52096","inReplyTo":"pull.417.git.1571787022.gitgitgadget@gmail.com","subject":"[PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-22T23:30:22Z","receivedAt":"2019-10-22T23:30:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n`git commit` learned to suggest to run `git cherry-pick --skip` when\ntrying to cherry-pick an empty patch.\n\nHowever, it was overlooked that there are more conditions than just a\n`git cherry-pick` when this advice is printed (which originally\nsuggested the neutral `git reset`): the same can happen during a rebase.\n\nLet's suggest the correct command, even during a rebase.\n\nWhile at it, we adjust more places in `builtin/commit.c` that\nincorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that\nsurely this must be a `cherry-pick` in progress.\n\nNote: we take pains to handle the situation when a user runs a `git\ncherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\nline in an interactive rebase). On the other hand, it is not possible to\nrun a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n`sequencer/` exist, we still want to advise to use `git cherry-pick\n--skip`.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/commit.c | 33 ++++++++++++++++++++++++---------\n t/t3403-rebase-skip.sh | 9 +++++++++\n 2 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e588bc6ad3..2beae13620 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -59,6 +59,9 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \" git commit --allow-empty\\n\"\n \"\\n\");\n \n+static const char empty_rebase_advice[] =\n+N_(\"Otherwise, please use 'git rebase --skip'\\n\");\n+\n static const char empty_cherry_pick_advice_single[] =\n N_(\"Otherwise, please use 'git cherry-pick --skip'\\n\");\n \n@@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;\n static const char *cleanup_arg;\n \n static enum commit_whence whence;\n-static int sequencer_in_use;\n+static int sequencer_in_use, rebase_in_progress;\n static int use_editor = 1, include_status = 1;\n static int have_option_m;\n static struct strbuf message = STRBUF_INIT;\n@@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)\n \t\twhence = FROM_CHERRY_PICK;\n \t\tif (file_exists(git_path_seq_dir()))\n \t\t\tsequencer_in_use = 1;\n+\t\tif (file_exists(git_path_rebase_merge_dir()))\n+\t\t\trebase_in_progress = 1;\n \t}\n \telse\n \t\twhence = FROM_COMMIT;\n@@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tif (whence != FROM_COMMIT) {\n \t\tif (whence == FROM_MERGE)\n \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n-\t\telse if (whence == FROM_CHERRY_PICK)\n+\t\telse if (whence == FROM_CHERRY_PICK) {\n+\t\t\tif (rebase_in_progress && !sequencer_in_use)\n+\t\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n+\t\t}\n \t}\n \n \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n@@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\tfputs(_(empty_amend_advice), stderr);\n \t\telse if (whence == FROM_CHERRY_PICK) {\n \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n-\t\t\tif (!sequencer_in_use)\n-\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n-\t\t\telse\n+\t\t\tif (sequencer_in_use)\n \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n+\t\t\telse if (rebase_in_progress)\n+\t\t\t\tfputs(_(empty_rebase_advice), stderr);\n+\t\t\telse\n+\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (amend && whence != FROM_COMMIT) {\n \t\tif (whence == FROM_MERGE)\n \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n-\t\telse if (whence == FROM_CHERRY_PICK)\n+\t\telse if (whence == FROM_CHERRY_PICK) {\n+\t\t\tif (rebase_in_progress && !sequencer_in_use)\n+\t\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n+\t\t}\n \t}\n \tif (fixup_message && squash_message)\n \t\tdie(_(\"Options --squash and --fixup cannot be used together\"));\n@@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treduce_heads_replace(&parents);\n \t} else {\n \t\tif (!reflog_msg)\n-\t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n-\t\t\t\t\t? \"commit (cherry-pick)\"\n-\t\t\t\t\t: \"commit\";\n+\t\t\treflog_msg = (whence != FROM_CHERRY_PICK)\n+\t\t\t\t\t? \"commit\"\n+\t\t\t\t\t: rebase_in_progress && !sequencer_in_use\n+\t\t\t\t\t? \"commit (rebase)\"\n+\t\t\t\t\t: \"commit (cherry-pick)\";\n \t\tcommit_list_insert(current_head, &parents);\n \t}\n \ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex 1f5122b632..77b03ac49f 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '\n \n test_debug 'gitk --all & sleep 1'\n \n+test_expect_success 'correct advice upon empty commit' '\n+\tgit checkout -b rebase-skip &&\n+\ttest_commit a1 &&\n+\ttest_tick &&\n+\tgit commit --amend -m amended --no-edit &&\n+\ttest_must_fail git rebase -m --onto a1 HEAD^ 2>err &&\n+\ttest_i18ngrep \"git rebase --skip\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"384666","messageId":"xmqqv9sgi3in.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"0d168b4a75c65e786f4b14f5da723957c32fa390.1571787022.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-23T02:45:04Z","receivedAt":"2019-10-23T02:45:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Note: we take pains to handle the situation when a user runs a `git\n> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\n> line in an interactive rebase). On the other hand, it is not possible to\n> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n> `sequencer/` exist, we still want to advise to use `git cherry-pick\n> --skip`.\n\nGood description of why the code does what it does, that future\nreaders will surely appreciate. Nice.\n\n"},{"id":"384667","messageId":"xmqqr234i2q0.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"pull.417.git.1571787022.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] commit: fix advice for empty commits during rebases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-23T03:02:15Z","receivedAt":"2019-10-23T03:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we\n> introduced a helpful message that suggests to run git cherry-pick --skip \n> (instead of the previous message that talked about git reset) when a\n> cherry-pick failed due to an empty patch.\n>\n> However, the same message is displayed during a rebase, when the patch\n> to-be-committed is empty. In this case, git reset would also have worked,\n> but git cherry-pick --skip does not work. This is a regression introduced in\n> this cycle.\n>\n> Let's be more careful here.\n>\n> Johannes Schindelin (3):\n> cherry-pick: add test for `--skip` advice in `git commit`\n> sequencer: export the function to get the path of `.git/rebase-merge/`\n> commit: give correct advice for empty commit during a rebase\n\nOverall they looked nicely done. The last one may have started its\nlife as \"let's fix advice for empty\", but no longer is.\n\nThe old code used the sequencer_in_use boolean to tell between two\nstates (\"are we doing a single pick, or a range pick?\"), but now we\nhave another boolean rebase_in_progress, and depending on the shape\nof the if statements existing codepaths happen to have, these two\nare combined in different ways to deal with new states. E.g. some\nplaces say\n\n\tif (rebase_in_progress && !sequencer_in_use)\n\t\twe are doing a rebase;\n\telse\n\t\twe are doing a cherry-pick;\n\nand some others say\n\n\tif (sequencer_in_use)\n\t\twe are doing a multi pick;\n\telse if (rebase_in_progress)\n\t\twe are doing a rebase;\n\telse\n\t\twe are doing a single pick;\n\nI wonder if it makes the resulting logic simpler to reason about if\nthese combination of two variables are rewritten to use a single\nvariable that enumerates three (or is it four, counting \"we are\ndoing neither a cherry-pick or a rebase\"?) possible state.\n\nOther than that, looked sensible. Will queue.\n\nThanks.\n"},{"id":"384754","messageId":"9d550cd9-08a2-eee5-7d7f-63678285accc@gmail.com","threadId":"52096","inReplyTo":"0d168b4a75c65e786f4b14f5da723957c32fa390.1571787022.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-10-24T10:15:13Z","receivedAt":"2019-10-24T10:15:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n> `git commit` learned to suggest to run `git cherry-pick --skip` when\n> trying to cherry-pick an empty patch.\n> \n> However, it was overlooked that there are more conditions than just a\n> `git cherry-pick` when this advice is printed (which originally\n> suggested the neutral `git reset`): the same can happen during a rebase.\n> \n> Let's suggest the correct command, even during a rebase.\n> \n> While at it, we adjust more places in `builtin/commit.c` that\n> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that\n> surely this must be a `cherry-pick` in progress.\n> \n> Note: we take pains to handle the situation when a user runs a `git\n> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\n> line in an interactive rebase). On the other hand, it is not possible to\n> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n> `sequencer/` exist, we still want to advise to use `git cherry-pick\n> --skip`.\n\nThanks for working on this. It's unfortunate that rebase does not remove\nCHERRY_PICK_HEAD for empty commits as it does if the commit is not\nempty. I think this is because 'rebase --continue' will skip an empty\ncommit so the user _has_ to run 'git commit' manually to keep it. If it\nhad been designed so that 'rebase --continue' kept the empty commit and\n'rebase --skip' skipped it then we would not have this problem but it's\na bit late to worry about that now.\n\nI don't this patch can distinguish between an empty cherry-pick\nperformed by the user while a rebase is in progress and an empty pick\nperformed by rebase as both create CHERRY_PICK_HEAD while\n.git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was\ncreated by rebase and prints advise based on that which may or may not\nbe the correct. I think we could distinguish the two by checking if\nCHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.\n\nBest Wishes\n\n\nPhillip\n\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/commit.c | 33 ++++++++++++++++++++++++---------\n> t/t3403-rebase-skip.sh | 9 +++++++++\n> 2 files changed, 33 insertions(+), 9 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e588bc6ad3..2beae13620 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -59,6 +59,9 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n> \" git commit --allow-empty\\n\"\n> \"\\n\");\n> \n> +static const char empty_rebase_advice[] =\n> +N_(\"Otherwise, please use 'git rebase --skip'\\n\");\n> +\n> static const char empty_cherry_pick_advice_single[] =\n> N_(\"Otherwise, please use 'git cherry-pick --skip'\\n\");\n> \n> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;\n> static const char *cleanup_arg;\n> \n> static enum commit_whence whence;\n> -static int sequencer_in_use;\n> +static int sequencer_in_use, rebase_in_progress;\n> static int use_editor = 1, include_status = 1;\n> static int have_option_m;\n> static struct strbuf message = STRBUF_INIT;\n> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)\n> \t\twhence = FROM_CHERRY_PICK;\n> \t\tif (file_exists(git_path_seq_dir()))\n> \t\t\tsequencer_in_use = 1;\n> +\t\tif (file_exists(git_path_rebase_merge_dir()))\n> +\t\t\trebase_in_progress = 1;\n> \t}\n> \telse\n> \t\twhence = FROM_COMMIT;\n> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> \tif (whence != FROM_COMMIT) {\n> \t\tif (whence == FROM_MERGE)\n> \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n> -\t\telse if (whence == FROM_CHERRY_PICK)\n> +\t\telse if (whence == FROM_CHERRY_PICK) {\n> +\t\t\tif (rebase_in_progress && !sequencer_in_use)\n> +\t\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n> \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n> +\t\t}\n> \t}\n> \n> \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\t\tfputs(_(empty_amend_advice), stderr);\n> \t\telse if (whence == FROM_CHERRY_PICK) {\n> \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n> -\t\t\tif (!sequencer_in_use)\n> -\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n> -\t\t\telse\n> +\t\t\tif (sequencer_in_use)\n> \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n> +\t\t\telse if (rebase_in_progress)\n> +\t\t\t\tfputs(_(empty_rebase_advice), stderr);\n> +\t\t\telse\n> +\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n> \t\t}\n> \t\treturn 0;\n> \t}\n> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],\n> \tif (amend && whence != FROM_COMMIT) {\n> \t\tif (whence == FROM_MERGE)\n> \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n> -\t\telse if (whence == FROM_CHERRY_PICK)\n> +\t\telse if (whence == FROM_CHERRY_PICK) {\n> +\t\t\tif (rebase_in_progress && !sequencer_in_use)\n> +\t\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n> \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n> +\t\t}\n> \t}\n> \tif (fixup_message && squash_message)\n> \t\tdie(_(\"Options --squash and --fixup cannot be used together\"));\n> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t\t\treduce_heads_replace(&parents);\n> \t} else {\n> \t\tif (!reflog_msg)\n> -\t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n> -\t\t\t\t\t? \"commit (cherry-pick)\"\n> -\t\t\t\t\t: \"commit\";\n> +\t\t\treflog_msg = (whence != FROM_CHERRY_PICK)\n> +\t\t\t\t\t? \"commit\"\n> +\t\t\t\t\t: rebase_in_progress && !sequencer_in_use\n> +\t\t\t\t\t? \"commit (rebase)\"\n> +\t\t\t\t\t: \"commit (cherry-pick)\";\n> \t\tcommit_list_insert(current_head, &parents);\n> \t}\n> \n> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> index 1f5122b632..77b03ac49f 100755\n> --- a/t/t3403-rebase-skip.sh\n> +++ b/t/t3403-rebase-skip.sh\n> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '\n> \n> test_debug 'gitk --all & sleep 1'\n> \n> +test_expect_success 'correct advice upon empty commit' '\n> +\tgit checkout -b rebase-skip &&\n> +\ttest_commit a1 &&\n> +\ttest_tick &&\n> +\tgit commit --amend -m amended --no-edit &&\n> +\ttest_must_fail git rebase -m --onto a1 HEAD^ 2>err &&\n> +\ttest_i18ngrep \"git rebase --skip\" err\n> +'\n> +\n> test_done\n> \n\n"},{"id":"384855","messageId":"nycvar.QRO.7.76.6.1910251341550.46@tvgsbejvaqbjf.bet","threadId":"52096","inReplyTo":"9d550cd9-08a2-eee5-7d7f-63678285accc@gmail.com","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-25T11:48:13Z","receivedAt":"2019-10-25T11:48:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 24 Oct 2019, Phillip Wood wrote:\n\n> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n> > `git commit` learned to suggest to run `git cherry-pick --skip` when\n> > trying to cherry-pick an empty patch.\n> >\n> > However, it was overlooked that there are more conditions than just a\n> > `git cherry-pick` when this advice is printed (which originally\n> > suggested the neutral `git reset`): the same can happen during a rebase.\n> >\n> > Let's suggest the correct command, even during a rebase.\n> >\n> > While at it, we adjust more places in `builtin/commit.c` that\n> > incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that\n> > surely this must be a `cherry-pick` in progress.\n> >\n> > Note: we take pains to handle the situation when a user runs a `git\n> > cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\n> > line in an interactive rebase). On the other hand, it is not possible to\n> > run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n> > `sequencer/` exist, we still want to advise to use `git cherry-pick\n> > --skip`.\n>\n> Thanks for working on this. It's unfortunate that rebase does not remove\n> CHERRY_PICK_HEAD for empty commits as it does if the commit is not\n> empty.\n\nOh, so that's what it is that bites me all the time? I frequently run\ninteractive rebases and sometimes mess up authorship (taking authorship\nof patches that I did not, in fact, author). I guess what happens is\nthat I `git commit` in the middle of a rebase that was interrupted by\nmerge conflicts.\n\nSo I would actually like to see the _exact opposite_ of what you want to\nsee: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case.\n\n> I think this is because 'rebase --continue' will skip an empty commit\n> so the user _has_ to run 'git commit' manually to keep it. If it had\n> been designed so that 'rebase --continue' kept the empty commit and\n> 'rebase --skip' skipped it then we would not have this problem but\n> it's a bit late to worry about that now.\n\nTrue.\n\n> I don't this patch can distinguish between an empty cherry-pick\n> performed by the user while a rebase is in progress and an empty pick\n> performed by rebase as both create CHERRY_PICK_HEAD while\n> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was\n> created by rebase and prints advise based on that which may or may not\n> be the correct. I think we could distinguish the two by checking if\n> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.\n\nI guess we could, but then, I would rather worry about that in the next\ncycle. In this cycle, I would rather fix the common case, which is that\na `git rebase -i` fails and tells me to `git cherry-pick --skip` instead\nof `git rebase --skip`.\n\nAnd even if I performed a `git cherry-pick` during a `git rebase` and\nthe result would be an empty commit, I'd rather be told to `git rebase\n--skip` to continue...\n\nBut if you feel strongly that this should be fixed differently, I'll\ngladly leave it to you ;-)\n\nCiao,\nDscho\n\n>\n> Best Wishes\n>\n>\n> Phillip\n>\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/commit.c | 33 ++++++++++++++++++++++++---------\n> > t/t3403-rebase-skip.sh | 9 +++++++++\n> > 2 files changed, 33 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index e588bc6ad3..2beae13620 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -59,6 +59,9 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n> > \" git commit --allow-empty\\n\"\n> > \"\\n\");\n> >\n> > +static const char empty_rebase_advice[] =\n> > +N_(\"Otherwise, please use 'git rebase --skip'\\n\");\n> > +\n> > static const char empty_cherry_pick_advice_single[] =\n> > N_(\"Otherwise, please use 'git cherry-pick --skip'\\n\");\n> >\n> > @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;\n> > static const char *cleanup_arg;\n> >\n> > static enum commit_whence whence;\n> > -static int sequencer_in_use;\n> > +static int sequencer_in_use, rebase_in_progress;\n> > static int use_editor = 1, include_status = 1;\n> > static int have_option_m;\n> > static struct strbuf message = STRBUF_INIT;\n> > @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)\n> > \t\twhence = FROM_CHERRY_PICK;\n> > \t\tif (file_exists(git_path_seq_dir()))\n> > \t\t\tsequencer_in_use = 1;\n> > +\t\tif (file_exists(git_path_rebase_merge_dir()))\n> > +\t\t\trebase_in_progress = 1;\n> > \t}\n> > \telse\n> > \t\twhence = FROM_COMMIT;\n> > @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> > \tif (whence != FROM_COMMIT) {\n> > \t\tif (whence == FROM_MERGE)\n> > \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n> > -\t\telse if (whence == FROM_CHERRY_PICK)\n> > +\t\telse if (whence == FROM_CHERRY_PICK) {\n> > +\t\t\tif (rebase_in_progress && !sequencer_in_use)\n> > +\t\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n> > \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n> > +\t\t}\n> > \t}\n> >\n> > \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n> > @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> > \t\t\tfputs(_(empty_amend_advice), stderr);\n> > \t\telse if (whence == FROM_CHERRY_PICK) {\n> > \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n> > -\t\t\tif (!sequencer_in_use)\n> > -\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n> > -\t\t\telse\n> > +\t\t\tif (sequencer_in_use)\n> > \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n> > +\t\t\telse if (rebase_in_progress)\n> > +\t\t\t\tfputs(_(empty_rebase_advice), stderr);\n> > +\t\t\telse\n> > +\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n> > \t\t}\n> > \t\treturn 0;\n> > \t}\n> > @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],\n> > \tif (amend && whence != FROM_COMMIT) {\n> > \t\tif (whence == FROM_MERGE)\n> > \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n> > -\t\telse if (whence == FROM_CHERRY_PICK)\n> > +\t\telse if (whence == FROM_CHERRY_PICK) {\n> > +\t\t\tif (rebase_in_progress && !sequencer_in_use)\n> > +\t\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n> > \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n> > +\t\t}\n> > \t}\n> > \tif (fixup_message && squash_message)\n> > \t\tdie(_(\"Options --squash and --fixup cannot be used together\"));\n> > @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> > \t\t\treduce_heads_replace(&parents);\n> > \t} else {\n> > \t\tif (!reflog_msg)\n> > -\t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n> > -\t\t\t\t\t? \"commit (cherry-pick)\"\n> > -\t\t\t\t\t: \"commit\";\n> > +\t\t\treflog_msg = (whence != FROM_CHERRY_PICK)\n> > +\t\t\t\t\t? \"commit\"\n> > +\t\t\t\t\t: rebase_in_progress && !sequencer_in_use\n> > +\t\t\t\t\t? \"commit (rebase)\"\n> > +\t\t\t\t\t: \"commit (cherry-pick)\";\n> > \t\tcommit_list_insert(current_head, &parents);\n> > \t}\n> >\n> > diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> > index 1f5122b632..77b03ac49f 100755\n> > --- a/t/t3403-rebase-skip.sh\n> > +++ b/t/t3403-rebase-skip.sh\n> > @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '\n> >\n> > test_debug 'gitk --all & sleep 1'\n> >\n> > +test_expect_success 'correct advice upon empty commit' '\n> > +\tgit checkout -b rebase-skip &&\n> > +\ttest_commit a1 &&\n> > +\ttest_tick &&\n> > +\tgit commit --amend -m amended --no-edit &&\n> > +\ttest_must_fail git rebase -m --onto a1 HEAD^ 2>err &&\n> > +\ttest_i18ngrep \"git rebase --skip\" err\n> > +'\n> > +\n> > test_done\n> >\n>\n>\n"},{"id":"384856","messageId":"nycvar.QRO.7.76.6.1910251402190.46@tvgsbejvaqbjf.bet","threadId":"52096","inReplyTo":"xmqqr234i2q0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] commit: fix advice for empty commits during rebases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-25T12:11:13Z","receivedAt":"2019-10-25T12:11:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 23 Oct 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we\n> > introduced a helpful message that suggests to run git cherry-pick --skip\n> > (instead of the previous message that talked about git reset) when a\n> > cherry-pick failed due to an empty patch.\n> >\n> > However, the same message is displayed during a rebase, when the patch\n> > to-be-committed is empty. In this case, git reset would also have worked,\n> > but git cherry-pick --skip does not work. This is a regression introduced in\n> > this cycle.\n> >\n> > Let's be more careful here.\n> >\n> > Johannes Schindelin (3):\n> > cherry-pick: add test for `--skip` advice in `git commit`\n> > sequencer: export the function to get the path of `.git/rebase-merge/`\n> > commit: give correct advice for empty commit during a rebase\n>\n> Overall they looked nicely done.\n\nThank you!\n\n> The last one may have started its life as \"let's fix advice for\n> empty\", but no longer is.\n\nIndeed. Sorry, I should have said so in the commit message...\n\n> The old code used the sequencer_in_use boolean to tell between two\n> states (\"are we doing a single pick, or a range pick?\"), but now we\n> have another boolean rebase_in_progress, and depending on the shape\n> of the if statements existing codepaths happen to have, these two\n> are combined in different ways to deal with new states. E.g. some\n> places say\n>\n> \tif (rebase_in_progress && !sequencer_in_use)\n> \t\twe are doing a rebase;\n> \telse\n> \t\twe are doing a cherry-pick;\n>\n> and some others say\n>\n> \tif (sequencer_in_use)\n> \t\twe are doing a multi pick;\n> \telse if (rebase_in_progress)\n> \t\twe are doing a rebase;\n> \telse\n> \t\twe are doing a single pick;\n>\n> I wonder if it makes the resulting logic simpler to reason about if\n> these combination of two variables are rewritten to use a single\n> variable that enumerates three (or is it four, counting \"we are\n> doing neither a cherry-pick or a rebase\"?) possible state.\n\nThat's a good idea! I'd like to postpone that until after the -rc\nperiod, as it is not strictly necessary to fix the bug.\n\nAs the bug was introduced in this cycle, I would like to see the bug fix\nin v2.24.0, though; I frequently rebase interactively, usually Git for\nWindows' patch thicket on top of one of git.git's branches, which often\nresults in now-empty patches, and I'd like to see the correct advice ;-)\n\nSo as not to forget about introducing that `enum`, I opened a ticket at\nhttps://github.com/gitgitgadget/git/issues/426.\n\nThanks,\nDscho\n\n>\n> Other than that, looked sensible. Will queue.\n>\n> Thanks.\n>\n"},{"id":"384862","messageId":"df0e77e9-b764-690e-c814-5d90fb141def@gmail.com","threadId":"52096","inReplyTo":"nycvar.QRO.7.76.6.1910251341550.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-10-25T14:01:19Z","receivedAt":"2019-10-25T14:01:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 25/10/2019 12:48, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Thu, 24 Oct 2019, Phillip Wood wrote:\n> \n>> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n>>> `git commit` learned to suggest to run `git cherry-pick --skip` when\n>>> trying to cherry-pick an empty patch.\n>>>\n>>> However, it was overlooked that there are more conditions than just a\n>>> `git cherry-pick` when this advice is printed (which originally\n>>> suggested the neutral `git reset`): the same can happen during a rebase.\n>>>\n>>> Let's suggest the correct command, even during a rebase.\n>>>\n>>> While at it, we adjust more places in `builtin/commit.c` that\n>>> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that\n>>> surely this must be a `cherry-pick` in progress.\n>>>\n>>> Note: we take pains to handle the situation when a user runs a `git\n>>> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\n>>> line in an interactive rebase). On the other hand, it is not possible to\n>>> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n>>> `sequencer/` exist, we still want to advise to use `git cherry-pick\n>>> --skip`.\n>>\n>> Thanks for working on this. It's unfortunate that rebase does not remove\n>> CHERRY_PICK_HEAD for empty commits as it does if the commit is not\n>> empty.\n> \n> Oh, so that's what it is that bites me all the time? I frequently run\n> interactive rebases and sometimes mess up authorship (taking authorship\n> of patches that I did not, in fact, author). I guess what happens is\n> that I `git commit` in the middle of a rebase that was interrupted by\n> merge conflicts.\n\nI used to do that a lot, I eventually trained myself not to commit after \nconflicts during a rebase and leave it to `rebase --continue` but it is \nannoying that it does not work\n\n> So I would actually like to see the _exact opposite_ of what you want to\n> see: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case.\n\nI don't necessarily object - having it consistent one way or the other \nwould be a distinct improvement, it has been removed when there are \nconflicts since it's inception [1] (That's v2 of the series which \nstarted removing CHERRY_PICK_HEAD in the case of rebase conflicts. I \ncouldn't find any discussion of v1 that recommended that change though). \nThere is also another thread [2].\n\n[1] \nhttps://public-inbox.org/git/1297876835-70613-1-git-send-email-jaysoffian@gmail.com/\n[2] \nhttps://public-inbox.org/git/CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com/ \n\n\n\n>> I think this is because 'rebase --continue' will skip an empty commit\n>> so the user _has_ to run 'git commit' manually to keep it. If it had\n>> been designed so that 'rebase --continue' kept the empty commit and\n>> 'rebase --skip' skipped it then we would not have this problem but\n>> it's a bit late to worry about that now.\n> \n> True.\n> \n>> I don't this patch can distinguish between an empty cherry-pick\n>> performed by the user while a rebase is in progress and an empty pick\n>> performed by rebase as both create CHERRY_PICK_HEAD while\n>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was\n>> created by rebase and prints advise based on that which may or may not\n>> be the correct. I think we could distinguish the two by checking if\n>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.\n> \n> I guess we could, but then, I would rather worry about that in the next\n> cycle. In this cycle, I would rather fix the common case, which is that\n> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead\n> of `git rebase --skip`.\n> \n> And even if I performed a `git cherry-pick` during a `git rebase` and\n> the result would be an empty commit, I'd rather be told to `git rebase\n> --skip` to continue...\n> \n> But if you feel strongly that this should be fixed differently, I'll\n> gladly leave it to you ;-)\n\nI'm happy to wait until the next cycle once we've decided what to do \nabout CHERRY_PICK_HEAD during rebases.\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n> \n>>\n>> Best Wishes\n>>\n>>\n>> Phillip\n>>\n>>>\n>>> Signed-off-by: Johannes Schindelin \n>>> ---\n>>> builtin/commit.c | 33 ++++++++++++++++++++++++---------\n>>> t/t3403-rebase-skip.sh | 9 +++++++++\n>>> 2 files changed, 33 insertions(+), 9 deletions(-)\n>>>\n>>> diff --git a/builtin/commit.c b/builtin/commit.c\n>>> index e588bc6ad3..2beae13620 100644\n>>> --- a/builtin/commit.c\n>>> +++ b/builtin/commit.c\n>>> @@ -59,6 +59,9 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n>>> \" git commit --allow-empty\\n\"\n>>> \"\\n\");\n>>>\n>>> +static const char empty_rebase_advice[] =\n>>> +N_(\"Otherwise, please use 'git rebase --skip'\\n\");\n>>> +\n>>> static const char empty_cherry_pick_advice_single[] =\n>>> N_(\"Otherwise, please use 'git cherry-pick --skip'\\n\");\n>>>\n>>> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;\n>>> static const char *cleanup_arg;\n>>>\n>>> static enum commit_whence whence;\n>>> -static int sequencer_in_use;\n>>> +static int sequencer_in_use, rebase_in_progress;\n>>> static int use_editor = 1, include_status = 1;\n>>> static int have_option_m;\n>>> static struct strbuf message = STRBUF_INIT;\n>>> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)\n>>> \t\twhence = FROM_CHERRY_PICK;\n>>> \t\tif (file_exists(git_path_seq_dir()))\n>>> \t\t\tsequencer_in_use = 1;\n>>> +\t\tif (file_exists(git_path_rebase_merge_dir()))\n>>> +\t\t\trebase_in_progress = 1;\n>>> \t}\n>>> \telse\n>>> \t\twhence = FROM_COMMIT;\n>>> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n>>> \tif (whence != FROM_COMMIT) {\n>>> \t\tif (whence == FROM_MERGE)\n>>> \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n>>> -\t\telse if (whence == FROM_CHERRY_PICK)\n>>> +\t\telse if (whence == FROM_CHERRY_PICK) {\n>>> +\t\t\tif (rebase_in_progress && !sequencer_in_use)\n>>> +\t\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n>>> \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n>>> +\t\t}\n>>> \t}\n>>>\n>>> \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n>>> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>>> \t\t\tfputs(_(empty_amend_advice), stderr);\n>>> \t\telse if (whence == FROM_CHERRY_PICK) {\n>>> \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n>>> -\t\t\tif (!sequencer_in_use)\n>>> -\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n>>> -\t\t\telse\n>>> +\t\t\tif (sequencer_in_use)\n>>> \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n>>> +\t\t\telse if (rebase_in_progress)\n>>> +\t\t\t\tfputs(_(empty_rebase_advice), stderr);\n>>> +\t\t\telse\n>>> +\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n>>> \t\t}\n>>> \t\treturn 0;\n>>> \t}\n>>> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],\n>>> \tif (amend && whence != FROM_COMMIT) {\n>>> \t\tif (whence == FROM_MERGE)\n>>> \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n>>> -\t\telse if (whence == FROM_CHERRY_PICK)\n>>> +\t\telse if (whence == FROM_CHERRY_PICK) {\n>>> +\t\t\tif (rebase_in_progress && !sequencer_in_use)\n>>> +\t\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n>>> \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n>>> +\t\t}\n>>> \t}\n>>> \tif (fixup_message && squash_message)\n>>> \t\tdie(_(\"Options --squash and --fixup cannot be used together\"));\n>>> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>> \t\t\treduce_heads_replace(&parents);\n>>> \t} else {\n>>> \t\tif (!reflog_msg)\n>>> -\t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n>>> -\t\t\t\t\t? \"commit (cherry-pick)\"\n>>> -\t\t\t\t\t: \"commit\";\n>>> +\t\t\treflog_msg = (whence != FROM_CHERRY_PICK)\n>>> +\t\t\t\t\t? \"commit\"\n>>> +\t\t\t\t\t: rebase_in_progress && !sequencer_in_use\n>>> +\t\t\t\t\t? \"commit (rebase)\"\n>>> +\t\t\t\t\t: \"commit (cherry-pick)\";\n>>> \t\tcommit_list_insert(current_head, &parents);\n>>> \t}\n>>>\n>>> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n>>> index 1f5122b632..77b03ac49f 100755\n>>> --- a/t/t3403-rebase-skip.sh\n>>> +++ b/t/t3403-rebase-skip.sh\n>>> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '\n>>>\n>>> test_debug 'gitk --all & sleep 1'\n>>>\n>>> +test_expect_success 'correct advice upon empty commit' '\n>>> +\tgit checkout -b rebase-skip &&\n>>> +\ttest_commit a1 &&\n>>> +\ttest_tick &&\n>>> +\tgit commit --amend -m amended --no-edit &&\n>>> +\ttest_must_fail git rebase -m --onto a1 HEAD^ 2>err &&\n>>> +\ttest_i18ngrep \"git rebase --skip\" err\n>>> +'\n>>> +\n>>> test_done\n>>>\n>>\n>>\n"},{"id":"385035","messageId":"xmqq8sp48fxl.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"nycvar.QRO.7.76.6.1910251402190.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/3] commit: fix advice for empty commits during rebases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-29T02:05:10Z","receivedAt":"2019-10-29T02:05:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> As the bug was introduced in this cycle, I would like to see the bug fix\n> in v2.24.0, though;...\n\nDo you mean that before the change you blame for the \"bug\" things\nworked better than what we have at the tip of 'master', or do you\nmean that the change you blame for the \"bug\" changed the behaviour\nrelative to the previous released version but the updated behavior\nis not what you consider correct?\n"},{"id":"385075","messageId":"nycvar.QRO.7.76.6.1910291358420.46@tvgsbejvaqbjf.bet","threadId":"52096","inReplyTo":"xmqq8sp48fxl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] commit: fix advice for empty commits during rebases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-29T13:00:22Z","receivedAt":"2019-10-29T13:00:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 29 Oct 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > As the bug was introduced in this cycle, I would like to see the bug fix\n> > in v2.24.0, though;...\n>\n> Do you mean that before the change you blame for the \"bug\" things\n> worked better than what we have at the tip of 'master', or do you\n> mean that the change you blame for the \"bug\" changed the behaviour\n> relative to the previous released version but the updated behavior\n> is not what you consider correct?\n\nWhat I mean is this: previously, when an interactive rebase interrupted\ndue to a now-empty patch, the advice given was correct, of clunky. Now\nit is incorrect.\n\n(Previously, the advice was to `git reset`, which was not completely\nhelpful, but now it suggests to `git cherry-pick --continue`, which is\nincorrect.)\n\nCiao,\nDscho\n"},{"id":"385758","messageId":"xmqqwocblywf.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"df0e77e9-b764-690e-c814-5d90fb141def@gmail.com","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-08T05:28:48Z","receivedAt":"2019-11-08T05:28:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> I don't this patch can distinguish between an empty cherry-pick\n>>> performed by the user while a rebase is in progress and an empty pick\n>>> performed by rebase as both create CHERRY_PICK_HEAD while\n>>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was\n>>> created by rebase and prints advise based on that which may or may not\n>>> be the correct. I think we could distinguish the two by checking if\n>>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.\n>>\n>> I guess we could, but then, I would rather worry about that in the next\n>> cycle. In this cycle, I would rather fix the common case, which is that\n>> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead\n>> of `git rebase --skip`.\n>>\n>> And even if I performed a `git cherry-pick` during a `git rebase` and\n>> the result would be an empty commit, I'd rather be told to `git rebase\n>> --skip` to continue...\n>>\n>> But if you feel strongly that this should be fixed differently, I'll\n>> gladly leave it to you ;-)\n>\n> I'm happy to wait until the next cycle once we've decided what to do\n> about CHERRY_PICK_HEAD during rebases.\n\nSo, is that agreed between the two? \n\nShould I eject js/advise-rebase-skip topic out of my tree and wait\nfor the decision wrt CHERRY_PICK_HEAD?\n\nThanks.\n"},{"id":"385784","messageId":"nycvar.QRO.7.76.6.1911081508560.46@tvgsbejvaqbjf.bet","threadId":"52096","inReplyTo":"xmqqwocblywf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-08T14:09:41Z","receivedAt":"2019-11-08T14:10:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Nov 2019, Junio C Hamano wrote:\n\n> Phillip Wood writes:\n>\n> >>> I don't this patch can distinguish between an empty cherry-pick\n> >>> performed by the user while a rebase is in progress and an empty pick\n> >>> performed by rebase as both create CHERRY_PICK_HEAD while\n> >>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was\n> >>> created by rebase and prints advise based on that which may or may not\n> >>> be the correct. I think we could distinguish the two by checking if\n> >>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.\n> >>\n> >> I guess we could, but then, I would rather worry about that in the next\n> >> cycle. In this cycle, I would rather fix the common case, which is that\n> >> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead\n> >> of `git rebase --skip`.\n> >>\n> >> And even if I performed a `git cherry-pick` during a `git rebase` and\n> >> the result would be an empty commit, I'd rather be told to `git rebase\n> >> --skip` to continue...\n> >>\n> >> But if you feel strongly that this should be fixed differently, I'll\n> >> gladly leave it to you ;-)\n> >\n> > I'm happy to wait until the next cycle once we've decided what to do\n> > about CHERRY_PICK_HEAD during rebases.\n>\n> So, is that agreed between the two?\n>\n> Should I eject js/advise-rebase-skip topic out of my tree and wait\n> for the decision wrt CHERRY_PICK_HEAD?\n\nPhillip, if you have some time to spend on that, I'd be very grateful, I\nam a bit under the weather and in dear need for an offline weekend.\n\nThanks,\nDscho\n"},{"id":"385912","messageId":"64bd1151-d2cd-4e8c-51f6-21e779d333a3@gmail.com","threadId":"52096","inReplyTo":"nycvar.QRO.7.76.6.1911081508560.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-11T16:13:37Z","receivedAt":"2019-11-11T16:13:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho & Junio\n\nOn 08/11/2019 14:09, Johannes Schindelin wrote:\n> Hi,\n> \n> On Fri, 8 Nov 2019, Junio C Hamano wrote:\n> \n>> Phillip Wood writes:\n>>\n>>>>> I don't this patch can distinguish between an empty cherry-pick\n>>>>> performed by the user while a rebase is in progress and an empty pick\n>>>>> performed by rebase as both create CHERRY_PICK_HEAD while\n>>>>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was\n>>>>> created by rebase and prints advise based on that which may or may not\n>>>>> be the correct. I think we could distinguish the two by checking if\n>>>>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.\n>>>>\n>>>> I guess we could, but then, I would rather worry about that in the next\n>>>> cycle. In this cycle, I would rather fix the common case, which is that\n>>>> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead\n>>>> of `git rebase --skip`.\n>>>>\n>>>> And even if I performed a `git cherry-pick` during a `git rebase` and\n>>>> the result would be an empty commit, I'd rather be told to `git rebase\n>>>> --skip` to continue...\n\nIt depends if you want to continue immediately or do something else in \nwhich case running reset is probably better advice. I'm not sure that \nthere's an easy solution for all possible scenarios though.\n\n>>>>\n>>>> But if you feel strongly that this should be fixed differently, I'll\n>>>> gladly leave it to you ;-)\n>>>\n>>> I'm happy to wait until the next cycle once we've decided what to do\n>>> about CHERRY_PICK_HEAD during rebases.\n>>\n>> So, is that agreed between the two?\n>>\n>> Should I eject js/advise-rebase-skip topic out of my tree and wait\n>> for the decision wrt CHERRY_PICK_HEAD?\n> \n> Phillip, if you have some time to spend on that, I'd be very grateful, I\n> am a bit under the weather and in dear need for an offline weekend.\n\nI'm happy to have a look at it but it probably wont be for a couple of \nweeks. I've been thinking about keeping CHERRY_PICK_HEAD and it's not \ntotally straight forward. CHERRY_PICK_HEAD needs to be removed when \ncommitting fixups to keep the author data from the commit that's being \namended (also I think `git commit --amend` errors out if \nCHERRY_PICK_HEAD is present). It's further complicated by --reset-date \nand --committer-date-is-author-date as we want commit to respect those \nwhen they've been passed to the rebase command.\n\nJunio, if you drop this series for now I'll rework it with an enum as \ndiscussed elsewhere in this thread.\n\nBest Wishes\n\nPhillip\n\n> Thanks,\n> Dscho\n> \n"},{"id":"387621","messageId":"20191206160614.631724-3-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:07Z","receivedAt":"2019-12-06T16:08:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Johannes Schindelin \n\nIn dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n`git commit` learned to suggest to run `git cherry-pick --skip` when\ntrying to cherry-pick an empty patch, but that was never tested for.\n\nHere is a test that verifies that a message is given to the user that\ncontains the correct invocation.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3510-cherry-pick-sequence.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 793bcc7fe3..5b94fdaa67 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -123,7 +123,8 @@ test_expect_success 'revert --skip to skip commit' '\n test_expect_success 'skip \"empty\" commit' '\n \tpristine_detach picked &&\n \ttest_commit dummy foo d &&\n-\ttest_must_fail git cherry-pick anotherpick &&\n+\ttest_must_fail git cherry-pick anotherpick 2>err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n \tgit cherry-pick --skip &&\n \ttest_cmp_rev dummy HEAD\n '\n-- \n2.24.0\n\n"},{"id":"387622","messageId":"20191206160614.631724-5-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:09Z","receivedAt":"2019-12-06T16:08:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n`git commit` relies on the presence of CHERRY_PICK_HEAD to show the\ncorrect error message in the case of an empty pick. This fixes a\nregression introduced by the conversion from shell to C. In the shell\nversion everything was a cherry-pick as far as the sequencer code was\nconcerned so it always wrote CHERRY_PICK_HEAD. The conversion to C\nforgot to update the code that creates CHERRY_PICK_HEAD. We do not want\nto create CHERRY_PICK_HEAD for fixup and squash commands as that would\nprevent `git commit --amend` from running.\n\nNote that the error message shown by `git commit` for an empty pick\nduring a rebase is currently wrong as it talks about running `git\ncherry-pick --skip` rather than `git rebase --skip`. This will be fixed\nin a future commit which is why the tests are in t3403-rebase-skip.sh.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 4 +++-\n t/t3403-rebase-skip.sh | 53 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 408b7885c7..4e0370277b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1916,7 +1916,9 @@ static int do_pick_commit(struct repository *r,\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) &&\n+\tif ((command == TODO_PICK || command == TODO_REWORD ||\n+\t command == TODO_EDIT) && !opts->no_commit &&\n+\t (res == 0 || res == 1) &&\n \t update_ref(NULL, \"CHERRY_PICK_HEAD\", &commit->object.oid, NULL,\n \t\t REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n \t\tres = -1;\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex ee8a8dba52..db7e917248 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -29,6 +29,13 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m reverted-goodbye &&\n \tgit tag reverted-goodbye &&\n+\tgit checkout goodbye &&\n+\ttest_tick &&\n+\tGIT_AUTHOR_NAME=\"Another Author\" \\\n+\t\tGIT_AUTHOR_EMAIL=\"another.author@example.com\" \\\n+\t\tgit commit --amend --no-edit -m amended-goodbye &&\n+\ttest_tick &&\n+\tgit tag amended-goodbye &&\n \n \tgit checkout -f skip-reference &&\n \techo moo > hello &&\n@@ -85,6 +92,52 @@ test_expect_success 'moved back to branch correctly' '\n \n test_debug 'gitk --all & sleep 1'\n \n+test_expect_success 'correct advice upon picking empty commit' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -i --onto goodbye \\\n+\t\tamended-goodbye^ amended-goodbye 2>err &&\n+\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_must_fail git commit &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err\n+'\n+\n+test_expect_success 'correct authorship when committing empty pick' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_must_fail git rebase -i --onto goodbye \\\n+\t\tamended-goodbye^ amended-goodbye &&\n+\tgit commit --allow-empty &&\n+\tgit log --pretty=format:\"%an <%ae>%n%ad%B\" -1 amended-goodbye >expect &&\n+\tgit log --pretty=format:\"%an <%ae>%n%ad%B\" -1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'correct advice upon rewording empty commit' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"reword 1\" git rebase -i \\\n+\t\t\t--onto goodbye amended-goodbye^ amended-goodbye 2>err\n+\t) &&\n+\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_must_fail git commit &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err\n+'\n+\n+test_expect_success 'correct advice upon editing empty commit' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"edit 1\" git rebase -i \\\n+\t\t\t--onto goodbye amended-goodbye^ amended-goodbye 2>err\n+\t) &&\n+\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_must_fail git commit &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err\n+'\n+\n test_expect_success 'fixup that empties commit fails' '\n \ttest_when_finished \"git rebase --abort\" &&\n \t(\n-- \n2.24.0\n\n"},{"id":"387623","messageId":"20191206160614.631724-4-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 3/9] cherry-pick: check commit error messages","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:08Z","receivedAt":"2019-12-06T16:08:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWe disallow partial commits and amending when CHERRY_PICK_HEAD\nexists. Add a couple of tests to check the error message printed in each\ncase before we refactor the code responsible for this.\n\nSigned-off-by: Phillip Wood \n---\n t/t3507-cherry-pick-conflict.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 9b9b4ca8d4..c9715bf674 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -161,6 +161,29 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '\n \n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n+\n+test_expect_success 'partial commit of cherry-pick fails' '\n+\tpristine_detach initial &&\n+\n+\ttest_must_fail git cherry-pick picked &&\n+\techo resolved >foo &&\n+\tgit add foo &&\n+\ttest_must_fail git commit foo 2>err &&\n+\n+\ttest_i18ngrep \"cannot do a partial commit during a cherry-pick.\" err\n+'\n+\n+test_expect_success 'commit --amend of cherry-pick fails' '\n+\tpristine_detach initial &&\n+\n+\ttest_must_fail git cherry-pick picked &&\n+\techo resolved >foo &&\n+\tgit add foo &&\n+\ttest_must_fail git commit --amend 2>err &&\n+\n+\ttest_i18ngrep \"in the middle of a cherry-pick -- cannot amend.\" err\n+'\n+\n test_expect_success 'successful final commit clears cherry-pick state' '\n \tpristine_detach initial &&\n \n-- \n2.24.0\n\n"},{"id":"387624","messageId":"20191206160614.631724-1-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"pull.417.git.1571787022.gitgitgadget@gmail.com","subject":"[PATCH v2 0/9] commit: fix advice for empty commits during rebases","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:05Z","receivedAt":"2019-12-06T16:08:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nI've added some more commits to improve the test coverage and handle\nedit & reword as well as pick commands. I've also changed the advice\nprinted by an empty cherry-pick performed during a rebase to be the\nsame whether it is part of a sequence of cherry-picks or just a single\npick.\n\nThere are a couple of RFC patches at the end that change the advice\nfor fixups that empty HEAD and change the handling of CHERRY_PICK_HEAD\nso if a user commits a conflict resolution the authorship is\npreserved. They can be split into a separate series if necessary to\navoid holding up the earlier patches.\n\nThe first patch is a general cleanup, not really related to the rest\nof the series\n\nThese patches are based on a merge of ra/cherry-pick-revert-skip and\npw/sequencer-compare-with-right-parent-to-check-empty-commits\n\n\nJohannes Schindelin (1):\n cherry-pick: add test for `--skip` advice in `git commit`\n\nPhillip Wood (8):\n t3404: use test_cmp_rev\n cherry-pick: check commit error messages\n sequencer: write CHERRY_PICK_HEAD for reword and edit\n commit: use enum value for multiple cherry-picks\n commit: encapsulate determine_whence() for sequencer\n commit: give correct advice for empty commit during a rebase\n [RFC] rebase: fix advice when a fixup creates an empty commit\n [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts\n\n builtin/commit.c | 68 +++++++++----\n sequencer.c | 93 +++++++++++++++---\n sequencer.h | 4 +-\n t/t3403-rebase-skip.sh | 97 ++++++++++++++++--\n t/t3404-rebase-interactive.sh | 168 +++++++++++++++++++++++---------\n t/t3507-cherry-pick-conflict.sh | 23 +++++\n t/t3510-cherry-pick-sequence.sh | 3 +-\n t/t7512-status-help.sh | 2 -\n wt-status.h | 16 ++-\n 9 files changed, 387 insertions(+), 87 deletions(-)\n\nRange-diff to what is in pu at the moment\n\n -: ---------- > 1: a4ad3e798c t3404: use test_cmp_rev\n 1: b9f97083f1 = 2: 1e9ea48348 cherry-pick: add test for `--skip` advice in `git commit`\n 2: 8eff6be234 < -: ---------- sequencer: export the function to get the path of `.git/rebase-merge/`\n -: ---------- > 3: f9be4dc3ae cherry-pick: check commit error messages\n -: ---------- > 4: fe15c61f1e sequencer: write CHERRY_PICK_HEAD for reword and edit\n -: ---------- > 5: d2cc4a59f1 commit: use enum value for multiple cherry-picks\n -: ---------- > 6: 06ab99b367 commit: encapsulate determine_whence() for sequencer\n 3: 116a408b6f ! 7: 637f17212b commit: give correct advice for empty commit during a rebase\n @@\n ## Metadata ##\n -Author: Johannes Schindelin \n +Author: Phillip Wood \n \n ## Commit message ##\n commit: give correct advice for empty commit during a rebase\n @@ Commit message\n cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\n line in an interactive rebase). On the other hand, it is not possible to\n run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n - `sequencer/` exist, we still want to advise to use `git cherry-pick\n - --skip`.\n + `sequencer/` exist or CHERRY_PICK_HEAD and REBASE_HEAD point to the same\n + commit , we still want to advise to use `git cherry-pick --skip`.\n \n - Signed-off-by: Johannes Schindelin \n + Original-patch-by: Johannes Schindelin \n + Signed-off-by: Phillip Wood \n \n ## builtin/commit.c ##\n @@ builtin/commit.c: N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \" git commit --allow-empty\\n\"\n \"\\n\");\n \n -+static const char empty_rebase_advice[] =\n ++static const char empty_rebase_pick_advice[] =\n +N_(\"Otherwise, please use 'git rebase --skip'\\n\");\n +\n static const char empty_cherry_pick_advice_single[] =\n N_(\"Otherwise, please use 'git cherry-pick --skip'\\n\");\n \n -@@ builtin/commit.c: static enum commit_msg_cleanup_mode cleanup_mode;\n - static const char *cleanup_arg;\n - \n - static enum commit_whence whence;\n --static int sequencer_in_use;\n -+static int sequencer_in_use, rebase_in_progress;\n - static int use_editor = 1, include_status = 1;\n - static int have_option_m;\n - static struct strbuf message = STRBUF_INIT;\n -@@ builtin/commit.c: static void determine_whence(struct wt_status *s)\n - \t\twhence = FROM_CHERRY_PICK;\n - \t\tif (file_exists(git_path_seq_dir()))\n - \t\t\tsequencer_in_use = 1;\n -+\t\tif (file_exists(git_path_rebase_merge_dir()))\n -+\t\t\trebase_in_progress = 1;\n - \t}\n - \telse\n - \t\twhence = FROM_COMMIT;\n @@ builtin/commit.c: static const char *prepare_index(int argc, const char **argv, const char *prefix\n - \tif (whence != FROM_COMMIT) {\n - \t\tif (whence == FROM_MERGE)\n \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n --\t\telse if (whence == FROM_CHERRY_PICK)\n -+\t\telse if (whence == FROM_CHERRY_PICK) {\n -+\t\t\tif (rebase_in_progress && !sequencer_in_use)\n -+\t\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n + \t\telse if (is_from_cherry_pick(whence))\n \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n -+\t\t}\n ++\t\telse if (is_from_rebase(whence))\n ++\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n \t}\n \n \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n + \t */\n + \telse if (whence == FROM_MERGE)\n + \t\thook_arg1 = \"merge\";\n +-\telse if (is_from_cherry_pick(whence)) {\n ++\telse if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {\n + \t\thook_arg1 = \"commit\";\n + \t\thook_arg2 = \"CHERRY_PICK_HEAD\";\n + \t}\n +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n + \t\trun_status(stdout, index_file, prefix, 0, s);\n + \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n - \t\telse if (whence == FROM_CHERRY_PICK) {\n +-\t\telse if (is_from_cherry_pick(whence)) {\n ++\t\telse if (is_from_cherry_pick(whence) ||\n ++\t\t\t whence == FROM_REBASE_PICK) {\n \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n --\t\t\tif (!sequencer_in_use)\n --\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n + \t\t\tif (whence == FROM_CHERRY_PICK_SINGLE)\n + \t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n -\t\t\telse\n -+\t\t\tif (sequencer_in_use)\n ++\t\t\telse if (whence == FROM_CHERRY_PICK_MULTI)\n \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n -+\t\t\telse if (rebase_in_progress)\n -+\t\t\t\tfputs(_(empty_rebase_advice), stderr);\n +\t\t\telse\n -+\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n ++\t\t\t\tfputs(_(empty_rebase_pick_advice), stderr);\n \t\t}\n \t\treturn 0;\n \t}\n @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n - \tif (amend && whence != FROM_COMMIT) {\n - \t\tif (whence == FROM_MERGE)\n \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n --\t\telse if (whence == FROM_CHERRY_PICK)\n -+\t\telse if (whence == FROM_CHERRY_PICK) {\n -+\t\t\tif (rebase_in_progress && !sequencer_in_use)\n -+\t\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n + \t\telse if (is_from_cherry_pick(whence))\n \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n -+\t\t}\n ++\t\telse if (whence == FROM_REBASE_PICK)\n ++\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n \t}\n \tif (fixup_message && squash_message)\n \t\tdie(_(\"Options --squash and --fixup cannot be used together\"));\n +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n + \t\tuse_message = edit_message;\n + \tif (amend && !use_message && !fixup_message)\n + \t\tuse_message = \"HEAD\";\n +-\tif (!use_message && !is_from_cherry_pick(whence) && renew_authorship)\n ++\tif (!use_message && !is_from_cherry_pick(whence) &&\n ++\t !is_from_rebase(whence) && renew_authorship)\n + \t\tdie(_(\"--reset-author can be used only with -C, -c or --amend.\"));\n + \tif (use_message) {\n + \t\tuse_message_buffer = read_commit_message(use_message);\n +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n + \t\t\tauthor_message_buffer = use_message_buffer;\n + \t\t}\n + \t}\n +-\tif (is_from_cherry_pick(whence) && !renew_authorship) {\n ++\tif ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) &&\n ++\t !renew_authorship) {\n + \t\tauthor_message = \"CHERRY_PICK_HEAD\";\n + \t\tauthor_message_buffer = read_commit_message(author_message);\n + \t}\n @@ builtin/commit.c: int cmd_commit(int argc, const char **argv, const char *prefix)\n - \t\t\treduce_heads_replace(&parents);\n - \t} else {\n \t\tif (!reflog_msg)\n --\t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n --\t\t\t\t\t? \"commit (cherry-pick)\"\n --\t\t\t\t\t: \"commit\";\n -+\t\t\treflog_msg = (whence != FROM_CHERRY_PICK)\n -+\t\t\t\t\t? \"commit\"\n -+\t\t\t\t\t: rebase_in_progress && !sequencer_in_use\n + \t\t\treflog_msg = is_from_cherry_pick(whence)\n + \t\t\t\t\t? \"commit (cherry-pick)\"\n ++\t\t\t\t\t: is_from_rebase(whence)\n +\t\t\t\t\t? \"commit (rebase)\"\n -+\t\t\t\t\t: \"commit (cherry-pick)\";\n + \t\t\t\t\t: \"commit\";\n \t\tcommit_list_insert(current_head, &parents);\n \t}\n +\n + ## sequencer.c ##\n +@@ sequencer.c: static int try_to_commit(struct repository *r,\n + \treturn res;\n + }\n + \n ++static int write_rebase_head(struct object_id *oid)\n ++{\n ++\tif (update_ref(\"rebase\", \"REBASE_HEAD\", oid,\n ++\t\t NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n ++\t\treturn error(_(\"could not update %s\"), \"REBASE_HEAD\");\n ++\n ++\treturn 0;\n ++}\n ++\n + static int do_commit(struct repository *r,\n + \t\t const char *msg_file, const char *author,\n +-\t\t struct replay_opts *opts, unsigned int flags)\n ++\t\t struct replay_opts *opts, unsigned int flags,\n ++\t\t struct object_id *oid)\n + {\n + \tint res = 1;\n + \n +@@ sequencer.c: static int do_commit(struct repository *r,\n + \t\t\treturn res;\n + \t\t}\n + \t}\n +-\tif (res == 1)\n ++\tif (res == 1) {\n ++\t\tif (is_rebase_i(opts) && oid)\n ++\t\t\tif (write_rebase_head(oid))\n ++\t\t\t return -1;\n + \t\treturn run_git_commit(r, msg_file, opts, flags);\n ++\t}\n + \n + \treturn res;\n + }\n +@@ sequencer.c: static int do_pick_commit(struct repository *r,\n + \t\tflags |= ALLOW_EMPTY;\n + \tif (!opts->no_commit) {\n + \t\tif (author || command == TODO_REVERT || (flags & AMEND_MSG))\n +-\t\t\tres = do_commit(r, msg_file, author, opts, flags);\n ++\t\t\tres = do_commit(r, msg_file, author, opts, flags,\n ++\t\t\t\t\tcommit? &commit->object.oid : NULL);\n + \t\telse\n + \t\t\tres = error(_(\"unable to parse commit author\"));\n + \t\t*check_todo = !!(flags & EDIT_MSG);\n +@@ sequencer.c: static int make_patch(struct repository *r,\n + \tp = short_commit_name(commit);\n + \tif (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)\n + \t\treturn -1;\n +-\tif (update_ref(\"rebase\", \"REBASE_HEAD\", &commit->object.oid,\n +-\t\t NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n +-\t\tres |= error(_(\"could not update %s\"), \"REBASE_HEAD\");\n ++\tres |= write_rebase_head(&commit->object.oid);\n + \n + \tstrbuf_addf(&buf, \"%s/patch\", get_dir(opts));\n + \tmemset(&log_tree_opt, 0, sizeof(log_tree_opt));\n +@@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)\n + int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n + {\n + \tif (file_exists(git_path_cherry_pick_head(r))) {\n +-\t\t*whence = file_exists(git_path_seq_dir()) ?\n +-\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n ++\t\tstruct object_id cherry_pick_head, rebase_head;\n ++\n ++\t\tif (file_exists(git_path_seq_dir()))\n ++\t\t\t*whence = FROM_CHERRY_PICK_MULTI;\n ++\t\tif (file_exists(rebase_path()) &&\n ++\t\t !get_oid(\"REBASE_HEAD\", &rebase_head) &&\n ++\t\t !get_oid(\"CHERRY_PICK_HEAD\", &cherry_pick_head) &&\n ++\t\t oideq(&rebase_head, &cherry_pick_head))\n ++\t\t\t*whence = FROM_REBASE_PICK;\n ++\t\telse\n ++\t\t\t*whence = FROM_CHERRY_PICK_SINGLE;\n ++\n + \t\treturn 1;\n + \t}\n \n \n ## t/t3403-rebase-skip.sh ##\n -@@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '\n +@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon picking empty commit' '\n + \ttest_must_fail git rebase -i --onto goodbye \\\n + \t\tamended-goodbye^ amended-goodbye 2>err &&\n + \ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n +-\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n ++\ttest_i18ngrep \"git rebase --skip\" err &&\n + \ttest_must_fail git commit &&\n +-\ttest_i18ngrep \"git cherry-pick --skip\" err\n ++\ttest_i18ngrep \"git rebase --skip\" err\n + '\n \n - test_debug 'gitk --all & sleep 1'\n + test_expect_success 'correct authorship when committing empty pick' '\n +@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon rewording empty commit' '\n + \t\t\t--onto goodbye amended-goodbye^ amended-goodbye 2>err\n + \t) &&\n + \ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n +-\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n ++\ttest_i18ngrep \"git rebase --skip\" err &&\n + \ttest_must_fail git commit &&\n +-\ttest_i18ngrep \"git cherry-pick --skip\" err\n ++\ttest_i18ngrep \"git rebase --skip\" err\n + '\n \n -+test_expect_success 'correct advice upon empty commit' '\n -+\tgit checkout -b rebase-skip &&\n -+\ttest_commit a1 &&\n -+\ttest_tick &&\n -+\tgit commit --amend -m amended --no-edit &&\n -+\ttest_must_fail git rebase -m --onto a1 HEAD^ 2>err &&\n + test_expect_success 'correct advice upon editing empty commit' '\n +@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon editing empty commit' '\n + \t\t\t--onto goodbye amended-goodbye^ amended-goodbye 2>err\n + \t) &&\n + \ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n +-\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n ++\ttest_i18ngrep \"git rebase --skip\" err &&\n + \ttest_must_fail git commit &&\n +\ttest_i18ngrep \"git rebase --skip\" err\n +'\n +\n - test_done\n ++test_expect_success 'correct advice upon cherry-picking an empty commit during a rebase' '\n ++\ttest_when_finished \"git rebase --abort\" &&\n ++\t(\n ++\t\tset_fake_editor &&\n ++\t\ttest_must_fail env FAKE_LINES=\"1 exec_git_cherry-pick_amended-goodbye\" \\\n ++\t\t\tgit rebase -i goodbye^ goodbye 2>err\n ++\t) &&\n ++\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n ++\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n ++\ttest_must_fail git commit 2>err &&\n ++\ttest_i18ngrep \"git cherry-pick --skip\" err\n ++'\n ++\n ++test_expect_success 'correct advice upon multi cherry-pick picking an empty commit during a rebase' '\n ++\ttest_when_finished \"git rebase --abort\" &&\n ++\t(\n ++\t\tset_fake_editor &&\n ++\t\ttest_must_fail env FAKE_LINES=\"1 exec_git_cherry-pick_goodbye_amended-goodbye\" \\\n ++\t\t\tgit rebase -i goodbye^^ goodbye 2>err\n ++\t) &&\n ++\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n ++\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n ++\ttest_must_fail git commit 2>err &&\n + \ttest_i18ngrep \"git cherry-pick --skip\" err\n + '\n + \n +\n + ## t/t3404-rebase-interactive.sh ##\n +@@ t/t3404-rebase-interactive.sh: test_expect_success 'post-commit hook is called' '\n + \ttest_cmp expect actual\n + '\n + \n ++test_expect_success 'correct error message for partial commit after empty pick' '\n ++\ttest_when_finished \"git rebase --abort\" &&\n ++\t(\n ++\t\tset_fake_editor &&\n ++\t\tFAKE_LINES=\"2 1 1\" &&\n ++\t\texport FAKE_LINES &&\n ++\t\ttest_must_fail git rebase -i A D\n ++\t) &&\n ++\techo x >file1 &&\n ++\ttest_must_fail git commit file1 2>err &&\n ++\ttest_i18ngrep \"cannot do a partial commit during a rebase.\" err\n ++'\n ++\n ++test_expect_success 'correct error message for commit --amend after empty pick' '\n ++\ttest_when_finished \"git rebase --abort\" &&\n ++\t(\n ++\t\tset_fake_editor &&\n ++\t\tFAKE_LINES=\"1 1\" &&\n ++\t\texport FAKE_LINES &&\n ++\t\ttest_must_fail git rebase -i A D\n ++\t) &&\n ++\techo x>file1 &&\n ++\ttest_must_fail git commit -a --amend 2>err &&\n ++\ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n ++'\n ++\n + # This must be the last test in this file\n + test_expect_success '$EDITOR and friends are unchanged' '\n + \ttest_editor_unchanged\n +\n + ## wt-status.h ##\n +@@ wt-status.h: enum commit_whence {\n + \tFROM_COMMIT, /* normal */\n + \tFROM_MERGE, /* commit came from merge */\n + \tFROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */\n +-\tFROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */\n ++\tFROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */\n ++\tFROM_REBASE_PICK /* commit came from a pick/reword/edit */\n + };\n + \n + static inline int is_from_cherry_pick(enum commit_whence whence)\n +@@ wt-status.h: static inline int is_from_cherry_pick(enum commit_whence whence)\n + \t\twhence == FROM_CHERRY_PICK_MULTI;\n + }\n + \n ++static inline int is_from_rebase(enum commit_whence whence)\n ++{\n ++\treturn whence == FROM_REBASE_PICK;\n ++}\n ++\n + struct wt_status_change_data {\n + \tint worktree_status;\n + \tint index_status;\n -: ---------- > 8: 6a00263595 [RFC] rebase: fix advice when a fixup creates an empty commit\n -: ---------- > 9: 8b5ca6b60b [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts\n\n-- \n2.24.0\n"},{"id":"387625","messageId":"20191206160614.631724-2-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 1/9] t3404: use test_cmp_rev","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:06Z","receivedAt":"2019-12-06T16:08:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThere are a number of places where we compare two revisions with\n test $(git rev-parse rev1) = $(git rev-parse rev2)\nwhen these fail there's no indication what has gone wrong and you need\nto be running with `-x` to see where the test has failed. Lets use\ntest_cmp_rev instead.\n\nSigned-off-by: Phillip Wood \n---\n t/t3404-rebase-interactive.sh | 38 +++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex c573c99069..d8a05fd439 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -191,7 +191,7 @@ test_expect_success 'no changes are a nop' '\n \tgit checkout branch2 &&\n \tgit rebase -i F &&\n \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch2\" &&\n-\ttest $(git rev-parse I) = $(git rev-parse HEAD)\n+\ttest_cmp_rev I HEAD\n '\n \n test_expect_success 'test the [branch] option' '\n@@ -200,16 +200,16 @@ test_expect_success 'test the [branch] option' '\n \tgit commit -m \"stop here\" &&\n \tgit rebase -i F branch2 &&\n \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch2\" &&\n-\ttest $(git rev-parse I) = $(git rev-parse branch2) &&\n-\ttest $(git rev-parse I) = $(git rev-parse HEAD)\n+\ttest_cmp_rev I branch2 &&\n+\ttest_cmp_rev I HEAD\n '\n \n test_expect_success 'test --onto ' '\n \tgit checkout -b test-onto branch2 &&\n \tgit rebase -i --onto branch1 F &&\n \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/test-onto\" &&\n-\ttest $(git rev-parse HEAD^) = $(git rev-parse branch1) &&\n-\ttest $(git rev-parse I) = $(git rev-parse branch2)\n+\ttest_cmp_rev HEAD^ branch1 &&\n+\ttest_cmp_rev I branch2\n '\n \n test_expect_success 'rebase on top of a non-conflicting commit' '\n@@ -218,12 +218,12 @@ test_expect_success 'rebase on top of a non-conflicting commit' '\n \tgit rebase -i branch2 &&\n \ttest file6 = $(git diff --name-only original-branch1) &&\n \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch1\" &&\n-\ttest $(git rev-parse I) = $(git rev-parse branch2) &&\n-\ttest $(git rev-parse I) = $(git rev-parse HEAD~2)\n+\ttest_cmp_rev I branch2 &&\n+\ttest_cmp_rev I HEAD~2\n '\n \n test_expect_success 'reflog for the branch shows state before rebase' '\n-\ttest $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)\n+\ttest_cmp_rev branch1@{1} original-branch1\n '\n \n test_expect_success 'reflog for the branch shows correct finish message' '\n@@ -280,7 +280,7 @@ test_expect_success 'show conflicted patch' '\n \n test_expect_success 'abort' '\n \tgit rebase --abort &&\n-\ttest $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&\n+\ttest_cmp_rev new-branch1 HEAD &&\n \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch1\" &&\n \ttest_path_is_missing .git/rebase-merge\n '\n@@ -323,7 +323,7 @@ test_expect_success 'retain authorship w/ conflicts' '\n \techo resolved >conflict &&\n \tgit add conflict &&\n \tgit rebase --continue &&\n-\ttest $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&\n+\ttest_cmp_rev conflict-a^0 HEAD^ &&\n \tgit show >out &&\n \tgrep AttributeMe out\n '\n@@ -340,7 +340,7 @@ test_expect_success 'squash' '\n \t\t\tgit rebase -i --onto master HEAD~2\n \t) &&\n \ttest B = $(cat file7) &&\n-\ttest $(git rev-parse HEAD^) = $(git rev-parse master)\n+\ttest_cmp_rev HEAD^ master\n '\n \n test_expect_success 'retain authorship when squashing' '\n@@ -399,9 +399,9 @@ test_expect_success REBASE_P 'preserve merges with -p' '\n \tgit update-index --refresh &&\n \tgit diff-files --quiet &&\n \tgit diff-index --quiet --cached HEAD -- &&\n-\ttest $(git rev-parse HEAD~6) = $(git rev-parse branch1) &&\n-\ttest $(git rev-parse HEAD~4^2) = $(git rev-parse to-be-preserved) &&\n-\ttest $(git rev-parse HEAD^^2^) = $(git rev-parse HEAD^^^) &&\n+\ttest_cmp_rev HEAD~6 branch1 &&\n+\ttest_cmp_rev HEAD~4^2 to-be-preserved &&\n+\ttest_cmp_rev HEAD^^2^ HEAD^^^ &&\n \ttest $(git show HEAD~5:file1) = B &&\n \ttest $(git show HEAD~3:file1) = C &&\n \ttest $(git show HEAD:file1) = E &&\n@@ -433,7 +433,7 @@ test_expect_success '--continue tries to commit' '\n \t\tgit add file1 &&\n \t\tFAKE_COMMIT_MESSAGE=\"chouette!\" git rebase --continue\n \t) &&\n-\ttest $(git rev-parse HEAD^) = $(git rev-parse new-branch1) &&\n+\ttest_cmp_rev HEAD^ new-branch1 &&\n \tgit show HEAD | grep chouette\n '\n \n@@ -740,7 +740,7 @@ test_expect_success 'do \"noop\" when there is nothing to cherry-pick' '\n \t\t--author=\"Somebody else \" &&\n \ttest $(git rev-parse branch3) != $(git rev-parse branch4) &&\n \tgit rebase -i branch3 &&\n-\ttest $(git rev-parse branch3) = $(git rev-parse branch4)\n+\ttest_cmp_rev branch3 branch4\n \n '\n \n@@ -799,7 +799,7 @@ test_expect_success 'rebase -i continue with unstaged submodule' '\n \ttest_must_fail git rebase -i submodule-base &&\n \tgit reset &&\n \tgit rebase --continue &&\n-\ttest $(git rev-parse submodule-base) = $(git rev-parse HEAD)\n+\ttest_cmp_rev submodule-base HEAD\n '\n \n test_expect_success 'avoid unnecessary reset' '\n@@ -822,7 +822,7 @@ test_expect_success 'reword' '\n \t\t\tgit rebase -i A &&\n \t\tgit show HEAD | grep \"E changed\" &&\n \t\ttest $(git rev-parse master) != $(git rev-parse HEAD) &&\n-\t\ttest $(git rev-parse master^) = $(git rev-parse HEAD^) &&\n+\t\ttest_cmp_rev master^ HEAD^ &&\n \t\tFAKE_LINES=\"1 2 reword 3 4\" FAKE_COMMIT_MESSAGE=\"D changed\" \\\n \t\t\tgit rebase -i A &&\n \t\tgit show HEAD^ | grep \"D changed\" &&\n@@ -886,7 +886,7 @@ test_expect_success 'always cherry-pick with --no-ff' '\n \t\tgit diff HEAD~$p original-no-ff-branch~$p > out &&\n \t\ttest_must_be_empty out\n \tdone &&\n-\ttest $(git rev-parse HEAD~3) = $(git rev-parse original-no-ff-branch~3) &&\n+\ttest_cmp_rev HEAD~3 original-no-ff-branch~3 &&\n \tgit diff HEAD~3 original-no-ff-branch~3 > out &&\n \ttest_must_be_empty out\n '\n-- \n2.24.0\n\n"},{"id":"387626","messageId":"20191206160614.631724-7-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:11Z","receivedAt":"2019-12-06T16:08:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWorking out which command wants to create a commit requires detailed\nknowledge of the sequencer internals and that knowledge is going to\nincrease in subsequent commits. With that in mind lets encapsulate that\nknowledge in sequencer.c rather than spreading it into builtin/commit.c.\n\nSigned-off-by: Phillip Wood \n---\n builtin/commit.c | 5 +----\n sequencer.c | 13 ++++++++++++-\n sequencer.h | 3 ++-\n 3 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3b463522be..d8d4c8e419 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head(the_repository)))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path_cherry_pick_head(the_repository)))\n-\t\twhence = file_exists(git_path_seq_dir()) ?\n-\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n-\telse\n+\telse if (!sequencer_determine_whence(the_repository, &whence))\n \t\twhence = FROM_COMMIT;\n \tif (s)\n \t\ts->whence = whence;\ndiff --git a/sequencer.c b/sequencer.c\nindex 4e0370277b..98e007556c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n \n-GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n+static GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n \n static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n@@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \n \treturn 0;\n }\n+\n+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n+{\n+\tif (file_exists(git_path_cherry_pick_head(r))) {\n+\t\t*whence = file_exists(git_path_seq_dir()) ?\n+\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 56881a6baa..8d451dbfcb 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -3,12 +3,12 @@\n \n #include \"cache.h\"\n #include \"strbuf.h\"\n+#include \"wt-status.h\"\n \n struct commit;\n struct repository;\n \n const char *git_path_commit_editmsg(void);\n-const char *git_path_seq_dir(void);\n const char *rebase_path_todo(void);\n const char *rebase_path_todo_backup(void);\n \n@@ -202,4 +202,5 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n void sequencer_post_commit_cleanup(struct repository *r);\n int sequencer_get_last_command(struct repository* r,\n \t\t\t enum replay_action *action);\n+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);\n #endif /* SEQUENCER_H */\n-- \n2.24.0\n\n"},{"id":"387627","messageId":"20191206160614.631724-6-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 5/9] commit: use enum value for multiple cherry-picks","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:10Z","receivedAt":"2019-12-06T16:08:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAdd FROM_CHERRY_PICK_MULTI for a sequence of cherry-picks rather than\nusing a separate variable.\n\nSigned-off-by: Phillip Wood \n---\n builtin/commit.c | 25 +++++++++++--------------\n wt-status.h | 9 ++++++++-\n 2 files changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d85e0ad560..3b463522be 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -122,7 +122,6 @@ static enum commit_msg_cleanup_mode cleanup_mode;\n static const char *cleanup_arg;\n \n static enum commit_whence whence;\n-static int sequencer_in_use;\n static int use_editor = 1, include_status = 1;\n static int have_option_m;\n static struct strbuf message = STRBUF_INIT;\n@@ -179,11 +178,9 @@ static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head(the_repository)))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path_cherry_pick_head(the_repository))) {\n-\t\twhence = FROM_CHERRY_PICK;\n-\t\tif (file_exists(git_path_seq_dir()))\n-\t\t\tsequencer_in_use = 1;\n-\t}\n+\telse if (file_exists(git_path_cherry_pick_head(the_repository)))\n+\t\twhence = file_exists(git_path_seq_dir()) ?\n+\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n \telse\n \t\twhence = FROM_COMMIT;\n \tif (s)\n@@ -453,7 +450,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tif (whence != FROM_COMMIT) {\n \t\tif (whence == FROM_MERGE)\n \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n-\t\telse if (whence == FROM_CHERRY_PICK)\n+\t\telse if (is_from_cherry_pick(whence))\n \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n \t}\n \n@@ -771,7 +768,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t */\n \telse if (whence == FROM_MERGE)\n \t\thook_arg1 = \"merge\";\n-\telse if (whence == FROM_CHERRY_PICK) {\n+\telse if (is_from_cherry_pick(whence)) {\n \t\thook_arg1 = \"commit\";\n \t\thook_arg2 = \"CHERRY_PICK_HEAD\";\n \t}\n@@ -948,9 +945,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n-\t\telse if (whence == FROM_CHERRY_PICK) {\n+\t\telse if (is_from_cherry_pick(whence)) {\n \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n-\t\t\tif (!sequencer_in_use)\n+\t\t\tif (whence == FROM_CHERRY_PICK_SINGLE)\n \t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n \t\t\telse\n \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n@@ -1143,7 +1140,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (amend && whence != FROM_COMMIT) {\n \t\tif (whence == FROM_MERGE)\n \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n-\t\telse if (whence == FROM_CHERRY_PICK)\n+\t\telse if (is_from_cherry_pick(whence))\n \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n \t}\n \tif (fixup_message && squash_message)\n@@ -1166,7 +1163,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tuse_message = edit_message;\n \tif (amend && !use_message && !fixup_message)\n \t\tuse_message = \"HEAD\";\n-\tif (!use_message && whence != FROM_CHERRY_PICK && renew_authorship)\n+\tif (!use_message && !is_from_cherry_pick(whence) && renew_authorship)\n \t\tdie(_(\"--reset-author can be used only with -C, -c or --amend.\"));\n \tif (use_message) {\n \t\tuse_message_buffer = read_commit_message(use_message);\n@@ -1175,7 +1172,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\tauthor_message_buffer = use_message_buffer;\n \t\t}\n \t}\n-\tif (whence == FROM_CHERRY_PICK && !renew_authorship) {\n+\tif (is_from_cherry_pick(whence) && !renew_authorship) {\n \t\tauthor_message = \"CHERRY_PICK_HEAD\";\n \t\tauthor_message_buffer = read_commit_message(author_message);\n \t}\n@@ -1589,7 +1586,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treduce_heads_replace(&parents);\n \t} else {\n \t\tif (!reflog_msg)\n-\t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n+\t\t\treflog_msg = is_from_cherry_pick(whence)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n \t\tcommit_list_insert(current_head, &parents);\ndiff --git a/wt-status.h b/wt-status.h\nindex 64f1ddc9fd..0098fdb0b5 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -39,9 +39,16 @@ enum show_ignored_type {\n enum commit_whence {\n \tFROM_COMMIT, /* normal */\n \tFROM_MERGE, /* commit came from merge */\n-\tFROM_CHERRY_PICK /* commit came from cherry-pick */\n+\tFROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */\n+\tFROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */\n };\n \n+static inline int is_from_cherry_pick(enum commit_whence whence)\n+{\n+\treturn whence == FROM_CHERRY_PICK_SINGLE ||\n+\t\twhence == FROM_CHERRY_PICK_MULTI;\n+}\n+\n struct wt_status_change_data {\n \tint worktree_status;\n \tint index_status;\n-- \n2.24.0\n\n"},{"id":"387628","messageId":"20191206160614.631724-10-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:14Z","receivedAt":"2019-12-06T16:08:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSince the inception of CHERRY_PICK_HEAD in d7e5c0cbfb (\"Introduce\nCHERRY_PICK_HEAD\", 2011-02-19) 'rebase -i' has removed it when there are\nconflicts. The rationale for this was that the rebase wanted to handle\nthe conflicts itself. However sometimes (e.g. after an edit command) the\nuser wants to commit the conflict resolution before making some other\nchanges or running some tests. Without CHERRY_PICK_HEAD the authorship\ninformation is lost when the user makes the commit. Fix this by leaving\nCHERRY_PICK_HEAD when we're not amending.\n\nNote that this changes the output of `git status`. The advice to run\n`git reset` is not appropriate for rebase as we do not allow partial\ncommits.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n This has semantic conflicts with ra/rebase-i-more-options as it does not\n respect the options passed to rebase when the user commits\n \n I haven't checked how this affects the shell prompt in contrib yet, I\n suspect it may need changing to cope the presence of CHERRY_PICK_HEAD\n during a rebase.\n \n I'd like to change the existing authorship tests to rely on the \"Original\n Author\" changes here, but they are a web of hidden interdependencies which is\n hard to untangle.\n\n sequencer.c | 12 ++--\n t/t3404-rebase-interactive.sh | 104 +++++++++++++++++++++++++---------\n t/t7512-status-help.sh | 2 -\n 3 files changed, 85 insertions(+), 33 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 64242f4ce7..624e96c930 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -372,11 +372,15 @@ static void print_advice(struct repository *r, int show_hint,\n \tif (msg) {\n \t\tfprintf(stderr, \"%s\\n\", msg);\n \t\t/*\n-\t\t * A conflict has occurred but the porcelain\n-\t\t * (typically rebase --interactive) wants to take care\n-\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n+\t\t * A conflict has occurred but the porcelain wants to take care\n+\t\t * of the commit itself so remove CHERRY_PICK_HEAD. Note that we\n+\t\t * do not do this for interactive rebases anymore in order to\n+\t\t * preserve the author identity when the user runs 'git commit'\n+\t\t * to commit the conflict resolution rather than relying on\n+\t\t * 'rebase --continue' to do it for them.\n \t\t */\n-\t\tunlink(git_path_cherry_pick_head(r));\n+\t\tif (!is_rebase_i(opts))\n+\t\t\tunlink(git_path_cherry_pick_head(r));\n \t\treturn;\n \t}\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 5afa6f28cd..5cd7db18f8 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -33,31 +33,35 @@ Initial setup:\n # in the expect2 file for the 'stop on conflicting pick' test.\n \n test_expect_success 'setup' '\n-\ttest_commit A file1 &&\n-\ttest_commit B file1 &&\n-\ttest_commit C file2 &&\n-\ttest_commit D file1 &&\n-\ttest_commit E file3 &&\n-\tgit checkout -b branch1 A &&\n-\ttest_commit F file4 &&\n-\ttest_commit G file1 &&\n-\ttest_commit H file5 &&\n-\tgit checkout -b branch2 F &&\n-\ttest_commit I file6 &&\n-\tgit checkout -b conflict-branch A &&\n-\ttest_commit one conflict &&\n-\ttest_commit two conflict &&\n-\ttest_commit three conflict &&\n-\ttest_commit four conflict &&\n-\tgit checkout -b no-conflict-branch A &&\n-\ttest_commit J fileJ &&\n-\ttest_commit K fileK &&\n-\ttest_commit L fileL &&\n-\ttest_commit M fileM &&\n-\tgit checkout -b no-ff-branch A &&\n-\ttest_commit N fileN &&\n-\ttest_commit O fileO &&\n-\ttest_commit P fileP\n+\t(\n+\t\tGIT_AUTHOR_NAME=\"Original Author\" &&\n+\t\tGIT_AUTHOR_EMAIL=\"original.author@example.com\" &&\n+\t\ttest_commit A file1 &&\n+\t\ttest_commit B file1 &&\n+\t\ttest_commit C file2 &&\n+\t\ttest_commit D file1 &&\n+\t\ttest_commit E file3 &&\n+\t\tgit checkout -b branch1 A &&\n+\t\ttest_commit F file4 &&\n+\t\ttest_commit G file1 &&\n+\t\ttest_commit H file5 &&\n+\t\tgit checkout -b branch2 F &&\n+\t\ttest_commit I file6 &&\n+\t\tgit checkout -b conflict-branch A &&\n+\t\ttest_commit one conflict &&\n+\t\ttest_commit two conflict &&\n+\t\ttest_commit three conflict &&\n+\t\ttest_commit four conflict &&\n+\t\tgit checkout -b no-conflict-branch A &&\n+\t\ttest_commit J fileJ &&\n+\t\ttest_commit K fileK &&\n+\t\ttest_commit L fileL &&\n+\t\ttest_commit M fileM &&\n+\t\tgit checkout -b no-ff-branch A &&\n+\t\ttest_commit N fileN &&\n+\t\ttest_commit O fileO &&\n+\t\ttest_commit P fileP\n+\t)\n '\n \n # \"exec\" commands are run with the user shell by default, but this may\n@@ -252,12 +256,12 @@ test_expect_success 'stop on conflicting pick' '\n \t-A\n \t+G\n \tEOF\n-\tcat >expect2 <<-\\EOF &&\n+\tcat >expect2 <<-EOF &&\n \t<<<<<<< HEAD\n \tD\n \t=======\n \tG\n-\t>>>>>>> 5d18e54... G\n+\t>>>>>>> $(git rev-parse --short HEAD)... G\n \tEOF\n \tgit tag new-branch1 &&\n \ttest_must_fail git rebase -i master &&\n@@ -1628,6 +1632,52 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n '\n \n+test_expect_success 'correct error message for partial commit after confilct' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\tgit checkout D &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"2 3\" &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i A\n+\t) &&\n+\techo x >file1 &&\n+\techo y >file2 &&\n+\tgit add file1 file2 &&\n+\ttest_must_fail git commit file1 2>err &&\n+\ttest_i18ngrep \"cannot do a partial commit during a rebase.\" err\n+'\n+\n+test_expect_success 'correct error message for commit --amend after conflict' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\tgit checkout D &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=3 &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i A\n+\t) &&\n+\techo x>file1 &&\n+\ttest_must_fail git commit -a --amend 2>err &&\n+\ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n+'\n+\n+test_expect_success 'correct authorship and message after conflict' '\n+\tgit checkout D &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=3 &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i A\n+\t) &&\n+\techo x >file1 &&\n+\tgit commit -a &&\n+\tgit log --pretty=format:\"%an <%ae>%n%ad%n%B\" -1 D >expect &&\n+\tgit log --pretty=format:\"%an <%ae>%n%ad%n%B\" -1 HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit rebase --continue\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex c1eb72555d..2adceb35e2 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -148,7 +148,6 @@ You are currently rebasing branch '\\''rebase_i_conflicts_second'\\'' on '\\''$ONTO\n (use \"git rebase --abort\" to check out the original branch)\n \n Unmerged paths:\n- (use \"git reset HEAD ...\" to unstage)\n (use \"git add ...\" to mark resolution)\n \n \tboth modified: main.txt\n@@ -176,7 +175,6 @@ You are currently rebasing branch '\\''rebase_i_conflicts_second'\\'' on '\\''$ONTO\n (all conflicts fixed: run \"git rebase --continue\")\n \n Changes to be committed:\n- (use \"git reset HEAD ...\" to unstage)\n \n \tmodified: main.txt\n \n-- \n2.24.0\n\n"},{"id":"387629","messageId":"20191206160614.631724-8-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 7/9] commit: give correct advice for empty commit during a rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:12Z","receivedAt":"2019-12-06T16:08:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIn dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),\n`git commit` learned to suggest to run `git cherry-pick --skip` when\ntrying to cherry-pick an empty patch.\n\nHowever, it was overlooked that there are more conditions than just a\n`git cherry-pick` when this advice is printed (which originally\nsuggested the neutral `git reset`): the same can happen during a rebase.\n\nLet's suggest the correct command, even during a rebase.\n\nWhile at it, we adjust more places in `builtin/commit.c` that\nincorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that\nsurely this must be a `cherry-pick` in progress.\n\nNote: we take pains to handle the situation when a user runs a `git\ncherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`\nline in an interactive rebase). On the other hand, it is not possible to\nrun a rebase during a cherry-pick, meaning: if both `rebase-merge/` and\n`sequencer/` exist or CHERRY_PICK_HEAD and REBASE_HEAD point to the same\ncommit , we still want to advise to use `git cherry-pick --skip`.\n\nOriginal-patch-by: Johannes Schindelin \nSigned-off-by: Phillip Wood \n---\n builtin/commit.c | 24 ++++++++++++++++-----\n sequencer.c | 39 ++++++++++++++++++++++++++++-------\n t/t3403-rebase-skip.sh | 36 +++++++++++++++++++++++++++-----\n t/t3404-rebase-interactive.sh | 26 +++++++++++++++++++++++\n wt-status.h | 8 ++++++-\n 5 files changed, 114 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d8d4c8e419..c3b6b8a6bd 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -59,6 +59,9 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \" git commit --allow-empty\\n\"\n \"\\n\");\n \n+static const char empty_rebase_pick_advice[] =\n+N_(\"Otherwise, please use 'git rebase --skip'\\n\");\n+\n static const char empty_cherry_pick_advice_single[] =\n N_(\"Otherwise, please use 'git cherry-pick --skip'\\n\");\n \n@@ -449,6 +452,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tdie(_(\"cannot do a partial commit during a merge.\"));\n \t\telse if (is_from_cherry_pick(whence))\n \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n+\t\telse if (is_from_rebase(whence))\n+\t\t\tdie(_(\"cannot do a partial commit during a rebase.\"));\n \t}\n \n \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n@@ -765,7 +770,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t */\n \telse if (whence == FROM_MERGE)\n \t\thook_arg1 = \"merge\";\n-\telse if (is_from_cherry_pick(whence)) {\n+\telse if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {\n \t\thook_arg1 = \"commit\";\n \t\thook_arg2 = \"CHERRY_PICK_HEAD\";\n \t}\n@@ -942,12 +947,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n-\t\telse if (is_from_cherry_pick(whence)) {\n+\t\telse if (is_from_cherry_pick(whence) ||\n+\t\t\t whence == FROM_REBASE_PICK) {\n \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n \t\t\tif (whence == FROM_CHERRY_PICK_SINGLE)\n \t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n-\t\t\telse\n+\t\t\telse if (whence == FROM_CHERRY_PICK_MULTI)\n \t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n+\t\t\telse\n+\t\t\t\tfputs(_(empty_rebase_pick_advice), stderr);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -1139,6 +1147,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\tdie(_(\"You are in the middle of a merge -- cannot amend.\"));\n \t\telse if (is_from_cherry_pick(whence))\n \t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n+\t\telse if (whence == FROM_REBASE_PICK)\n+\t\t\tdie(_(\"You are in the middle of a rebase -- cannot amend.\"));\n \t}\n \tif (fixup_message && squash_message)\n \t\tdie(_(\"Options --squash and --fixup cannot be used together\"));\n@@ -1160,7 +1170,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tuse_message = edit_message;\n \tif (amend && !use_message && !fixup_message)\n \t\tuse_message = \"HEAD\";\n-\tif (!use_message && !is_from_cherry_pick(whence) && renew_authorship)\n+\tif (!use_message && !is_from_cherry_pick(whence) &&\n+\t !is_from_rebase(whence) && renew_authorship)\n \t\tdie(_(\"--reset-author can be used only with -C, -c or --amend.\"));\n \tif (use_message) {\n \t\tuse_message_buffer = read_commit_message(use_message);\n@@ -1169,7 +1180,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\tauthor_message_buffer = use_message_buffer;\n \t\t}\n \t}\n-\tif (is_from_cherry_pick(whence) && !renew_authorship) {\n+\tif ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) &&\n+\t !renew_authorship) {\n \t\tauthor_message = \"CHERRY_PICK_HEAD\";\n \t\tauthor_message_buffer = read_commit_message(author_message);\n \t}\n@@ -1585,6 +1597,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = is_from_cherry_pick(whence)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n+\t\t\t\t\t: is_from_rebase(whence)\n+\t\t\t\t\t? \"commit (rebase)\"\n \t\t\t\t\t: \"commit\";\n \t\tcommit_list_insert(current_head, &parents);\n \t}\ndiff --git a/sequencer.c b/sequencer.c\nindex 98e007556c..6e153fea76 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1430,9 +1430,19 @@ static int try_to_commit(struct repository *r,\n \treturn res;\n }\n \n+static int write_rebase_head(struct object_id *oid)\n+{\n+\tif (update_ref(\"rebase\", \"REBASE_HEAD\", oid,\n+\t\t NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n+\t\treturn error(_(\"could not update %s\"), \"REBASE_HEAD\");\n+\n+\treturn 0;\n+}\n+\n static int do_commit(struct repository *r,\n \t\t const char *msg_file, const char *author,\n-\t\t struct replay_opts *opts, unsigned int flags)\n+\t\t struct replay_opts *opts, unsigned int flags,\n+\t\t struct object_id *oid)\n {\n \tint res = 1;\n \n@@ -1457,8 +1467,12 @@ static int do_commit(struct repository *r,\n \t\t\treturn res;\n \t\t}\n \t}\n-\tif (res == 1)\n+\tif (res == 1) {\n+\t\tif (is_rebase_i(opts) && oid)\n+\t\t\tif (write_rebase_head(oid))\n+\t\t\t return -1;\n \t\treturn run_git_commit(r, msg_file, opts, flags);\n+\t}\n \n \treturn res;\n }\n@@ -1945,7 +1959,8 @@ static int do_pick_commit(struct repository *r,\n \t\tflags |= ALLOW_EMPTY;\n \tif (!opts->no_commit) {\n \t\tif (author || command == TODO_REVERT || (flags & AMEND_MSG))\n-\t\t\tres = do_commit(r, msg_file, author, opts, flags);\n+\t\t\tres = do_commit(r, msg_file, author, opts, flags,\n+\t\t\t\t\tcommit? &commit->object.oid : NULL);\n \t\telse\n \t\t\tres = error(_(\"unable to parse commit author\"));\n \t\t*check_todo = !!(flags & EDIT_MSG);\n@@ -2960,9 +2975,7 @@ static int make_patch(struct repository *r,\n \tp = short_commit_name(commit);\n \tif (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)\n \t\treturn -1;\n-\tif (update_ref(\"rebase\", \"REBASE_HEAD\", &commit->object.oid,\n-\t\t NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n-\t\tres |= error(_(\"could not update %s\"), \"REBASE_HEAD\");\n+\tres |= write_rebase_head(&commit->object.oid);\n \n \tstrbuf_addf(&buf, \"%s/patch\", get_dir(opts));\n \tmemset(&log_tree_opt, 0, sizeof(log_tree_opt));\n@@ -5260,8 +5273,18 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n {\n \tif (file_exists(git_path_cherry_pick_head(r))) {\n-\t\t*whence = file_exists(git_path_seq_dir()) ?\n-\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n+\t\tstruct object_id cherry_pick_head, rebase_head;\n+\n+\t\tif (file_exists(git_path_seq_dir()))\n+\t\t\t*whence = FROM_CHERRY_PICK_MULTI;\n+\t\tif (file_exists(rebase_path()) &&\n+\t\t !get_oid(\"REBASE_HEAD\", &rebase_head) &&\n+\t\t !get_oid(\"CHERRY_PICK_HEAD\", &cherry_pick_head) &&\n+\t\t oideq(&rebase_head, &cherry_pick_head))\n+\t\t\t*whence = FROM_REBASE_PICK;\n+\t\telse\n+\t\t\t*whence = FROM_CHERRY_PICK_SINGLE;\n+\n \t\treturn 1;\n \t}\n \ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex db7e917248..a927774910 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -97,9 +97,9 @@ test_expect_success 'correct advice upon picking empty commit' '\n \ttest_must_fail git rebase -i --onto goodbye \\\n \t\tamended-goodbye^ amended-goodbye 2>err &&\n \ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n-\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_i18ngrep \"git rebase --skip\" err &&\n \ttest_must_fail git commit &&\n-\ttest_i18ngrep \"git cherry-pick --skip\" err\n+\ttest_i18ngrep \"git rebase --skip\" err\n '\n \n test_expect_success 'correct authorship when committing empty pick' '\n@@ -120,9 +120,9 @@ test_expect_success 'correct advice upon rewording empty commit' '\n \t\t\t--onto goodbye amended-goodbye^ amended-goodbye 2>err\n \t) &&\n \ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n-\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_i18ngrep \"git rebase --skip\" err &&\n \ttest_must_fail git commit &&\n-\ttest_i18ngrep \"git cherry-pick --skip\" err\n+\ttest_i18ngrep \"git rebase --skip\" err\n '\n \n test_expect_success 'correct advice upon editing empty commit' '\n@@ -133,8 +133,34 @@ test_expect_success 'correct advice upon editing empty commit' '\n \t\t\t--onto goodbye amended-goodbye^ amended-goodbye 2>err\n \t) &&\n \ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n-\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_i18ngrep \"git rebase --skip\" err &&\n \ttest_must_fail git commit &&\n+\ttest_i18ngrep \"git rebase --skip\" err\n+'\n+\n+test_expect_success 'correct advice upon cherry-picking an empty commit during a rebase' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"1 exec_git_cherry-pick_amended-goodbye\" \\\n+\t\t\tgit rebase -i goodbye^ goodbye 2>err\n+\t) &&\n+\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_must_fail git commit 2>err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err\n+'\n+\n+test_expect_success 'correct advice upon multi cherry-pick picking an empty commit during a rebase' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"1 exec_git_cherry-pick_goodbye_amended-goodbye\" \\\n+\t\t\tgit rebase -i goodbye^^ goodbye 2>err\n+\t) &&\n+\ttest_i18ngrep \"previous cherry-pick is now empty\" err &&\n+\ttest_i18ngrep \"git cherry-pick --skip\" err &&\n+\ttest_must_fail git commit 2>err &&\n \ttest_i18ngrep \"git cherry-pick --skip\" err\n '\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d8a05fd439..5afa6f28cd 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1602,6 +1602,32 @@ test_expect_success 'post-commit hook is called' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'correct error message for partial commit after empty pick' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"2 1 1\" &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i A D\n+\t) &&\n+\techo x >file1 &&\n+\ttest_must_fail git commit file1 2>err &&\n+\ttest_i18ngrep \"cannot do a partial commit during a rebase.\" err\n+'\n+\n+test_expect_success 'correct error message for commit --amend after empty pick' '\n+\ttest_when_finished \"git rebase --abort\" &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"1 1\" &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i A D\n+\t) &&\n+\techo x>file1 &&\n+\ttest_must_fail git commit -a --amend 2>err &&\n+\ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\ndiff --git a/wt-status.h b/wt-status.h\nindex 0098fdb0b5..5f81bf7507 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -40,7 +40,8 @@ enum commit_whence {\n \tFROM_COMMIT, /* normal */\n \tFROM_MERGE, /* commit came from merge */\n \tFROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */\n-\tFROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */\n+\tFROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */\n+\tFROM_REBASE_PICK /* commit came from a pick/reword/edit */\n };\n \n static inline int is_from_cherry_pick(enum commit_whence whence)\n@@ -49,6 +50,11 @@ static inline int is_from_cherry_pick(enum commit_whence whence)\n \t\twhence == FROM_CHERRY_PICK_MULTI;\n }\n \n+static inline int is_from_rebase(enum commit_whence whence)\n+{\n+\treturn whence == FROM_REBASE_PICK;\n+}\n+\n struct wt_status_change_data {\n \tint worktree_status;\n \tint index_status;\n-- \n2.24.0\n\n"},{"id":"387630","messageId":"20191206160614.631724-9-phillip.wood123@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-1-phillip.wood123@gmail.com","subject":"[PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T16:06:13Z","receivedAt":"2019-12-06T16:08:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAdd a specific message to advise the user what to do when a fixup or\nsquash command would create an empty commit.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n I'm slightly nervous of moving the call to determine_whence() to\n finalized_deferred_config(). I think it is ok but need to do a more\n thorough audit of the code to be sure.\n\n builtin/commit.c | 32 ++++++++++++++++++++++++++++----\n sequencer.c | 31 ++++++++++++++++++++++++++++++-\n sequencer.h | 3 ++-\n t/t3403-rebase-skip.sh | 18 ++++++++++++------\n wt-status.h | 5 +++--\n 5 files changed, 75 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c3b6b8a6bd..4ae984c470 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -52,6 +52,20 @@ N_(\"You asked to amend the most recent commit, but doing so would make\\n\"\n \"it empty. You can repeat your command with --allow-empty, or you can\\n\"\n \"remove the commit entirely with \\\"git reset HEAD^\\\".\\n\");\n \n+static const char empty_rebase_fixup_advice[] =\n+N_(\"The fixup would make the commit empty\\n\"\n+\"If you wish to commit it anyway use:\\n\"\n+\"\\n\"\n+\" git commit --amend --allow-empty\\n\"\n+\" git rebase --continue\\n\"\n+\"\\n\"\n+\"To remove the commit entirely use:\\n\"\n+\"\\n\"\n+\" git reset HEAD^\\n\"\n+\" git rebase --continue\\n\"\n+\"\\n\"\n+\"Otherwise, please use 'git rebase --skip' to skip it\\n\");\n+\n static const char empty_cherry_pick_advice[] =\n N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\n\"\n \"If you wish to commit it anyway, use:\\n\"\n@@ -181,8 +195,14 @@ static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head(the_repository)))\n \t\twhence = FROM_MERGE;\n-\telse if (!sequencer_determine_whence(the_repository, &whence))\n-\t\twhence = FROM_COMMIT;\n+\telse {\n+\t\tint res = sequencer_determine_whence(the_repository, &whence,\n+\t\t\t\t\t\t amend);\n+\t\tif (res < 0)\n+\t\t\tdie(_(\"could not read sequencer state\"));\n+\t\telse if (!res)\n+\t\t\twhence = FROM_COMMIT;\n+\t}\n \tif (s)\n \t\ts->whence = whence;\n }\n@@ -192,7 +212,6 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)\n \twt_status_prepare(the_repository, s);\n \tinit_diff_ui_defaults();\n \tgit_config(fn, s);\n-\tdetermine_whence(s);\n \ts->hints = advice_status_hints; /* must come after git_config() */\n }\n \n@@ -943,9 +962,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t */\n \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n \t !(amend && is_a_merge(current_head))) {\n+\t\tfprintf(stderr, \"\\nwhence = %d\\n\", whence);\n \t\ts->display_comment_prefix = old_display_comment_prefix;\n \t\trun_status(stdout, index_file, prefix, 0, s);\n-\t\tif (amend)\n+\t\tif (whence == FROM_REBASE_FIXUP)\n+\t\t\tfputs(_(empty_rebase_fixup_advice), stderr);\n+\t\telse if (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n \t\telse if (is_from_cherry_pick(whence) ||\n \t\t\t whence == FROM_REBASE_PICK) {\n@@ -1114,6 +1136,8 @@ static void finalize_deferred_config(struct wt_status *s)\n \n \tif (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)\n \t\ts->ahead_behind_flags = AHEAD_BEHIND_FULL;\n+\n+\tdetermine_whence(s);\n }\n \n static int parse_and_validate_options(int argc, const char *argv[],\ndiff --git a/sequencer.c b/sequencer.c\nindex 6e153fea76..64242f4ce7 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5270,7 +5270,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \treturn 0;\n }\n \n-int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,\n+\t\t\t int amending)\n {\n \tif (file_exists(git_path_cherry_pick_head(r))) {\n \t\tstruct object_id cherry_pick_head, rebase_head;\n@@ -5286,6 +5287,34 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n \t\t\t*whence = FROM_CHERRY_PICK_SINGLE;\n \n \t\treturn 1;\n+\t} else if (amending && file_exists(rebase_path_current_fixups()) &&\n+\t\t (file_exists(git_path_squash_msg(r)) ||\n+\t\t file_exists(git_path_merge_msg(r)))) {\n+\t\t/*\n+\t\t * If rebase_path_amend() exists the user is running `git\n+\t\t * commit`, if not we're committing a fixup/squash directly from\n+\t\t * the sequencer\n+\t\t */\n+\t\tif (file_exists(rebase_path_amend())) {\n+\t\t\tstruct strbuf rev = STRBUF_INIT;\n+\t\t\tstruct object_id to_amend, head;\n+\n+\t\t\tif (get_oid(\"HEAD\", &head))\n+\t\t\t\treturn error(_(\"amending invalid head\")); /* let commit deal with error */\n+\t\t\tif (!read_oneliner(&rev, rebase_path_amend(), 0))\n+\t\t\t\treturn error(_(\"invalid file: '%s'\"),\n+\t\t\t\t\t rebase_path_amend());\n+\t\t\tif (get_oid_hex(rev.buf, &to_amend))\n+\t\t\t\treturn error(_(\"invalid contents: '%s'\"),\n+\t\t\t\t\t rebase_path_amend());\n+\t\t\tif (oideq(&head, &to_amend)) {\n+\t\t\t\t*whence = FROM_REBASE_FIXUP;\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\t\t} else {\n+\t\t\t*whence = FROM_REBASE_FIXUP;\n+\t\t\treturn 1;\n+\t\t}\n \t}\n \n \treturn 0;\ndiff --git a/sequencer.h b/sequencer.h\nindex 8d451dbfcb..4e631900b4 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -202,5 +202,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n void sequencer_post_commit_cleanup(struct repository *r);\n int sequencer_get_last_command(struct repository* r,\n \t\t\t enum replay_action *action);\n-int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);\n+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,\n+\t\t\t int amending);\n #endif /* SEQUENCER_H */\ndiff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\nindex a927774910..bc110b66b3 100755\n--- a/t/t3403-rebase-skip.sh\n+++ b/t/t3403-rebase-skip.sh\n@@ -164,22 +164,28 @@ test_expect_success 'correct advice upon multi cherry-pick picking an empty comm\n \ttest_i18ngrep \"git cherry-pick --skip\" err\n '\n \n-test_expect_success 'fixup that empties commit fails' '\n+test_expect_success 'correct advice when fixup empties commit' '\n \ttest_when_finished \"git rebase --abort\" &&\n \t(\n \t\tset_fake_editor &&\n \t\ttest_must_fail env FAKE_LINES=\"1 fixup 2\" git rebase -i \\\n-\t\t\tgoodbye^ reverted-goodbye\n-\t)\n+\t\t\tgoodbye^ reverted-goodbye 2>err\n+\t) &&\n+\ttest_i18ngrep \"git rebase --skip\" err &&\n+\ttest_must_fail git commit --amend --no-edit 2>err &&\n+\ttest_i18ngrep \"git rebase --skip\" err\n '\n \n-test_expect_success 'squash that empties commit fails' '\n+test_expect_success 'correct advice when squash empties commit' '\n \ttest_when_finished \"git rebase --abort\" &&\n \t(\n \t\tset_fake_editor &&\n \t\ttest_must_fail env FAKE_LINES=\"1 squash 2\" git rebase -i \\\n-\t\t\tgoodbye^ reverted-goodbye\n-\t)\n+\t\t\tgoodbye^ reverted-goodbye 2>err\n+\t) &&\n+\ttest_i18ngrep \"git rebase --skip\" err &&\n+\ttest_must_fail git commit --amend --no-edit 2>err &&\n+\ttest_i18ngrep \"git rebase --skip\" err\n '\n \n # Must be the last test in this file\ndiff --git a/wt-status.h b/wt-status.h\nindex 5f81bf7507..a4b7fe6de9 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -41,7 +41,8 @@ enum commit_whence {\n \tFROM_MERGE, /* commit came from merge */\n \tFROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */\n \tFROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */\n-\tFROM_REBASE_PICK /* commit came from a pick/reword/edit */\n+\tFROM_REBASE_PICK, /* commit came from a pick/reword/edit */\n+\tFROM_REBASE_FIXUP /* commit came from fixup/squash */\n };\n \n static inline int is_from_cherry_pick(enum commit_whence whence)\n@@ -52,7 +53,7 @@ static inline int is_from_cherry_pick(enum commit_whence whence)\n \n static inline int is_from_rebase(enum commit_whence whence)\n {\n-\treturn whence == FROM_REBASE_PICK;\n+\treturn whence == FROM_REBASE_PICK || whence == FROM_REBASE_FIXUP;\n }\n \n struct wt_status_change_data {\n-- \n2.24.0\n\n"},{"id":"387633","messageId":"xmqq5zitxsjq.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"20191206160614.631724-2-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 1/9] t3404: use test_cmp_rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-06T17:39:05Z","receivedAt":"2019-12-06T17:39:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> There are a number of places where we compare two revisions with\n> test $(git rev-parse rev1) = $(git rev-parse rev2)\n> when these fail there's no indication what has gone wrong and you need\n> to be running with `-x` to see where the test has failed. Lets use\n> test_cmp_rev instead.\n\nMakes sense. Wonder if we can have a version of coccinelle for\nshell scripts ;-)\n\n>\n> Signed-off-by: Phillip Wood \n> ---\n> t/t3404-rebase-interactive.sh | 38 +++++++++++++++++------------------\n> 1 file changed, 19 insertions(+), 19 deletions(-)\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index c573c99069..d8a05fd439 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -191,7 +191,7 @@ test_expect_success 'no changes are a nop' '\n> \tgit checkout branch2 &&\n> \tgit rebase -i F &&\n> \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch2\" &&\n> -\ttest $(git rev-parse I) = $(git rev-parse HEAD)\n> +\ttest_cmp_rev I HEAD\n> '\n> \n> test_expect_success 'test the [branch] option' '\n> @@ -200,16 +200,16 @@ test_expect_success 'test the [branch] option' '\n> \tgit commit -m \"stop here\" &&\n> \tgit rebase -i F branch2 &&\n> \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch2\" &&\n> -\ttest $(git rev-parse I) = $(git rev-parse branch2) &&\n> -\ttest $(git rev-parse I) = $(git rev-parse HEAD)\n> +\ttest_cmp_rev I branch2 &&\n> +\ttest_cmp_rev I HEAD\n> '\n> \n> test_expect_success 'test --onto ' '\n> \tgit checkout -b test-onto branch2 &&\n> \tgit rebase -i --onto branch1 F &&\n> \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/test-onto\" &&\n> -\ttest $(git rev-parse HEAD^) = $(git rev-parse branch1) &&\n> -\ttest $(git rev-parse I) = $(git rev-parse branch2)\n> +\ttest_cmp_rev HEAD^ branch1 &&\n> +\ttest_cmp_rev I branch2\n> '\n> \n> test_expect_success 'rebase on top of a non-conflicting commit' '\n> @@ -218,12 +218,12 @@ test_expect_success 'rebase on top of a non-conflicting commit' '\n> \tgit rebase -i branch2 &&\n> \ttest file6 = $(git diff --name-only original-branch1) &&\n> \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch1\" &&\n> -\ttest $(git rev-parse I) = $(git rev-parse branch2) &&\n> -\ttest $(git rev-parse I) = $(git rev-parse HEAD~2)\n> +\ttest_cmp_rev I branch2 &&\n> +\ttest_cmp_rev I HEAD~2\n> '\n> \n> test_expect_success 'reflog for the branch shows state before rebase' '\n> -\ttest $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)\n> +\ttest_cmp_rev branch1@{1} original-branch1\n> '\n> \n> test_expect_success 'reflog for the branch shows correct finish message' '\n> @@ -280,7 +280,7 @@ test_expect_success 'show conflicted patch' '\n> \n> test_expect_success 'abort' '\n> \tgit rebase --abort &&\n> -\ttest $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&\n> +\ttest_cmp_rev new-branch1 HEAD &&\n> \ttest \"$(git symbolic-ref -q HEAD)\" = \"refs/heads/branch1\" &&\n> \ttest_path_is_missing .git/rebase-merge\n> '\n> @@ -323,7 +323,7 @@ test_expect_success 'retain authorship w/ conflicts' '\n> \techo resolved >conflict &&\n> \tgit add conflict &&\n> \tgit rebase --continue &&\n> -\ttest $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&\n> +\ttest_cmp_rev conflict-a^0 HEAD^ &&\n> \tgit show >out &&\n> \tgrep AttributeMe out\n> '\n> @@ -340,7 +340,7 @@ test_expect_success 'squash' '\n> \t\t\tgit rebase -i --onto master HEAD~2\n> \t) &&\n> \ttest B = $(cat file7) &&\n> -\ttest $(git rev-parse HEAD^) = $(git rev-parse master)\n> +\ttest_cmp_rev HEAD^ master\n> '\n> \n> test_expect_success 'retain authorship when squashing' '\n> @@ -399,9 +399,9 @@ test_expect_success REBASE_P 'preserve merges with -p' '\n> \tgit update-index --refresh &&\n> \tgit diff-files --quiet &&\n> \tgit diff-index --quiet --cached HEAD -- &&\n> -\ttest $(git rev-parse HEAD~6) = $(git rev-parse branch1) &&\n> -\ttest $(git rev-parse HEAD~4^2) = $(git rev-parse to-be-preserved) &&\n> -\ttest $(git rev-parse HEAD^^2^) = $(git rev-parse HEAD^^^) &&\n> +\ttest_cmp_rev HEAD~6 branch1 &&\n> +\ttest_cmp_rev HEAD~4^2 to-be-preserved &&\n> +\ttest_cmp_rev HEAD^^2^ HEAD^^^ &&\n> \ttest $(git show HEAD~5:file1) = B &&\n> \ttest $(git show HEAD~3:file1) = C &&\n> \ttest $(git show HEAD:file1) = E &&\n> @@ -433,7 +433,7 @@ test_expect_success '--continue tries to commit' '\n> \t\tgit add file1 &&\n> \t\tFAKE_COMMIT_MESSAGE=\"chouette!\" git rebase --continue\n> \t) &&\n> -\ttest $(git rev-parse HEAD^) = $(git rev-parse new-branch1) &&\n> +\ttest_cmp_rev HEAD^ new-branch1 &&\n> \tgit show HEAD | grep chouette\n> '\n> \n> @@ -740,7 +740,7 @@ test_expect_success 'do \"noop\" when there is nothing to cherry-pick' '\n> \t\t--author=\"Somebody else \" &&\n> \ttest $(git rev-parse branch3) != $(git rev-parse branch4) &&\n> \tgit rebase -i branch3 &&\n> -\ttest $(git rev-parse branch3) = $(git rev-parse branch4)\n> +\ttest_cmp_rev branch3 branch4\n> \n> '\n> \n> @@ -799,7 +799,7 @@ test_expect_success 'rebase -i continue with unstaged submodule' '\n> \ttest_must_fail git rebase -i submodule-base &&\n> \tgit reset &&\n> \tgit rebase --continue &&\n> -\ttest $(git rev-parse submodule-base) = $(git rev-parse HEAD)\n> +\ttest_cmp_rev submodule-base HEAD\n> '\n> \n> test_expect_success 'avoid unnecessary reset' '\n> @@ -822,7 +822,7 @@ test_expect_success 'reword' '\n> \t\t\tgit rebase -i A &&\n> \t\tgit show HEAD | grep \"E changed\" &&\n> \t\ttest $(git rev-parse master) != $(git rev-parse HEAD) &&\n> -\t\ttest $(git rev-parse master^) = $(git rev-parse HEAD^) &&\n> +\t\ttest_cmp_rev master^ HEAD^ &&\n> \t\tFAKE_LINES=\"1 2 reword 3 4\" FAKE_COMMIT_MESSAGE=\"D changed\" \\\n> \t\t\tgit rebase -i A &&\n> \t\tgit show HEAD^ | grep \"D changed\" &&\n> @@ -886,7 +886,7 @@ test_expect_success 'always cherry-pick with --no-ff' '\n> \t\tgit diff HEAD~$p original-no-ff-branch~$p > out &&\n> \t\ttest_must_be_empty out\n> \tdone &&\n> -\ttest $(git rev-parse HEAD~3) = $(git rev-parse original-no-ff-branch~3) &&\n> +\ttest_cmp_rev HEAD~3 original-no-ff-branch~3 &&\n> \tgit diff HEAD~3 original-no-ff-branch~3 > out &&\n> \ttest_must_be_empty out\n> '\n"},{"id":"387635","messageId":"xmqq1rthxqyc.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"20191206160614.631724-6-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 5/9] commit: use enum value for multiple cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-06T18:13:31Z","receivedAt":"2019-12-06T18:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> Add FROM_CHERRY_PICK_MULTI for a sequence of cherry-picks rather than\n> using a separate variable.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> builtin/commit.c | 25 +++++++++++--------------\n> wt-status.h | 9 ++++++++-\n> 2 files changed, 19 insertions(+), 15 deletions(-)\n\nMakes sense. The checks have become quite pleasant to read, thanks\nto the new helper function.\n\n> ...\n> -\tif (whence == FROM_CHERRY_PICK && !renew_authorship) {\n> +\tif (is_from_cherry_pick(whence) && !renew_authorship) {\n> \t\tauthor_message = \"CHERRY_PICK_HEAD\";\n> \t\tauthor_message_buffer = read_commit_message(author_message);\n> \t}\n> diff --git a/wt-status.h b/wt-status.h\n> index 64f1ddc9fd..0098fdb0b5 100644\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -39,9 +39,16 @@ enum show_ignored_type {\n> enum commit_whence {\n> \tFROM_COMMIT, /* normal */\n> \tFROM_MERGE, /* commit came from merge */\n> -\tFROM_CHERRY_PICK /* commit came from cherry-pick */\n> +\tFROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */\n> +\tFROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */\n> };\n\nIt might be worth to end PICK_MULTI with a trailing comma to\nfuture-proof, but not worth a reroll for this alone.\n\n"},{"id":"387636","messageId":"xmqqwob9wbwe.fsf@gitster-ct.c.googlers.com","threadId":"52096","inReplyTo":"20191206160614.631724-7-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-06T18:24:01Z","receivedAt":"2019-12-06T18:24:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> Working out which command wants to create a commit requires detailed\n> knowledge of the sequencer internals and that knowledge is going to\n> increase in subsequent commits. With that in mind lets encapsulate that\n> knowledge in sequencer.c rather than spreading it into builtin/commit.c.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> builtin/commit.c | 5 +----\n> sequencer.c | 13 ++++++++++++-\n> sequencer.h | 3 ++-\n> 3 files changed, 15 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 3b463522be..d8d4c8e419 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)\n> {\n> \tif (file_exists(git_path_merge_head(the_repository)))\n> \t\twhence = FROM_MERGE;\n> -\telse if (file_exists(git_path_cherry_pick_head(the_repository)))\n> -\t\twhence = file_exists(git_path_seq_dir()) ?\n> -\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n> -\telse\n> +\telse if (!sequencer_determine_whence(the_repository, &whence))\n> \t\twhence = FROM_COMMIT;\n> \tif (s)\n> \t\ts->whence = whence;\n> diff --git a/sequencer.c b/sequencer.c\n> index 4e0370277b..98e007556c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n> \n> GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n> \n> -GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n> +static GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n> \n> static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n> static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n> @@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \n> \treturn 0;\n> }\n> +\n> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n> +{\n> +\tif (file_exists(git_path_cherry_pick_head(r))) {\n> +\t\t*whence = file_exists(git_path_seq_dir()) ?\n> +\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nI am not sure if this is a good move---determine_whence() that can\ntell not just we are in the middle of cherry-pick (either a single\nor multi) but also during a merge may be at the right abstraction\nlevel. Why would we want to invent a separate function that says \"I\ndunno\" during a merge, instead of moving the logic for merge to the\nnew helper as well? The original determine_whence that takes\nwt_status and populates it still has to call the new helper either\nway. Also for the matter FROM_COMMIT may also want to be part of\nthe helper. This all depends on the new callers you plan to invent,\nof course.\n\nNot part of this topic, but the call to file_exists() may want to\nbecome a call to dir_exists() as git-path-seq-dir is clearly a\ndirectory and cannot be a file, right?\n"},{"id":"388441","messageId":"141f95b0-cae0-06f6-2c29-618dc22ae000@gmail.com","threadId":"52096","inReplyTo":"xmqqwob9wbwe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-18T14:26:56Z","receivedAt":"2019-12-18T14:27:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/12/2019 18:24, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> From: Phillip Wood \n>>\n>> Working out which command wants to create a commit requires detailed\n>> knowledge of the sequencer internals and that knowledge is going to\n>> increase in subsequent commits. With that in mind lets encapsulate that\n>> knowledge in sequencer.c rather than spreading it into builtin/commit.c.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>> builtin/commit.c | 5 +----\n>> sequencer.c | 13 ++++++++++++-\n>> sequencer.h | 3 ++-\n>> 3 files changed, 15 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 3b463522be..d8d4c8e419 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)\n>> {\n>> \tif (file_exists(git_path_merge_head(the_repository)))\n>> \t\twhence = FROM_MERGE;\n>> -\telse if (file_exists(git_path_cherry_pick_head(the_repository)))\n>> -\t\twhence = file_exists(git_path_seq_dir()) ?\n>> -\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n>> -\telse\n>> +\telse if (!sequencer_determine_whence(the_repository, &whence))\n>> \t\twhence = FROM_COMMIT;\n>> \tif (s)\n>> \t\ts->whence = whence;\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 4e0370277b..98e007556c 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n>> \n>> GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n>> \n>> -GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n>> +static GIT_PATH_FUNC(git_path_seq_dir, \"sequencer\")\n>> \n>> static GIT_PATH_FUNC(git_path_todo_file, \"sequencer/todo\")\n>> static GIT_PATH_FUNC(git_path_opts_file, \"sequencer/opts\")\n>> @@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n>> \n>> \treturn 0;\n>> }\n>> +\n>> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n>> +{\n>> +\tif (file_exists(git_path_cherry_pick_head(r))) {\n>> +\t\t*whence = file_exists(git_path_seq_dir()) ?\n>> +\t\t\tFROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n> \n> I am not sure if this is a good move---determine_whence() that can\n> tell not just we are in the middle of cherry-pick (either a single\n> or multi) but also during a merge may be at the right abstraction\n> level. Why would we want to invent a separate function that says \"I\n> dunno\" during a merge, instead of moving the logic for merge to the\n> new helper as well? The original determine_whence that takes\n> wt_status and populates it still has to call the new helper either\n> way. Also for the matter FROM_COMMIT may also want to be part of\n> the helper. This all depends on the new callers you plan to invent,\n> of course.\n\nThe idea was for determine_whence() to be able to delegate to the \nsequencer to ask if it is doing something without having to expose all \nthe implementation details of that check in builtin/commit.c. It is \nsimple enough at this stage but the next patches add more complexity \nwhich would mean exposing various sequencer state files to \nbuiltin/commit.c. This function is only meant to be called from \ndetermine_whence() - callers that want to know if any operations (merge, \ncherry-pick etc.) are in progress should be calling that not the \nfunction added here.\n\n> Not part of this topic, but the call to file_exists() may want to\n> become a call to dir_exists() as git-path-seq-dir is clearly a\n> directory and cannot be a file, right?\n\nYes\n\nBest Wishes\n\nPhillip\n"},{"id":"388442","messageId":"7e1b92f5-48df-e202-ebcc-5b15987a7d63@gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-10-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-18T14:35:58Z","receivedAt":"2019-12-18T14:36:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/12/2019 16:06, Phillip Wood wrote:\n> From: Phillip Wood \n> \n> Since the inception of CHERRY_PICK_HEAD in d7e5c0cbfb (\"Introduce\n> CHERRY_PICK_HEAD\", 2011-02-19) 'rebase -i' has removed it when there are\n> conflicts. The rationale for this was that the rebase wanted to handle\n> the conflicts itself. However sometimes (e.g. after an edit command) the\n> user wants to commit the conflict resolution before making some other\n> changes or running some tests. Without CHERRY_PICK_HEAD the authorship\n> information is lost when the user makes the commit. Fix this by leaving\n> CHERRY_PICK_HEAD when we're not amending.\n\nI'm not so sure about this approach as it wont work with 'merge' \ncommands when rebasing. I wonder if it would be better to add a new file \nCOMMIT_AUTHOR (or maybe MERGE_AUTHOR) that can be parsed by \nsplit_ident() and sets the authorship for a commit. The file would \noverride $GIT_AUTHOR_NAME/EMAIL/DATE but could be overridden on the \ncommandline by --author/date/reset-author\n\nBest Wishes\n\nPhillip\n\n> Note that this changes the output of `git status`. The advice to run\n> `git reset` is not appropriate for rebase as we do not allow partial\n> commits.\n> \n> Signed-off-by: Phillip Wood \n> ---\n> \n> Notes:\n> This has semantic conflicts with ra/rebase-i-more-options as it does not\n> respect the options passed to rebase when the user commits\n> \n> I haven't checked how this affects the shell prompt in contrib yet, I\n> suspect it may need changing to cope the presence of CHERRY_PICK_HEAD\n> during a rebase.\n> \n> I'd like to change the existing authorship tests to rely on the \"Original\n> Author\" changes here, but they are a web of hidden interdependencies which is\n> hard to untangle.\n> \n> sequencer.c | 12 ++--\n> t/t3404-rebase-interactive.sh | 104 +++++++++++++++++++++++++---------\n> t/t7512-status-help.sh | 2 -\n> 3 files changed, 85 insertions(+), 33 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 64242f4ce7..624e96c930 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -372,11 +372,15 @@ static void print_advice(struct repository *r, int show_hint,\n> \tif (msg) {\n> \t\tfprintf(stderr, \"%s\\n\", msg);\n> \t\t/*\n> -\t\t * A conflict has occurred but the porcelain\n> -\t\t * (typically rebase --interactive) wants to take care\n> -\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n> +\t\t * A conflict has occurred but the porcelain wants to take care\n> +\t\t * of the commit itself so remove CHERRY_PICK_HEAD. Note that we\n> +\t\t * do not do this for interactive rebases anymore in order to\n> +\t\t * preserve the author identity when the user runs 'git commit'\n> +\t\t * to commit the conflict resolution rather than relying on\n> +\t\t * 'rebase --continue' to do it for them.\n> \t\t */\n> -\t\tunlink(git_path_cherry_pick_head(r));\n> +\t\tif (!is_rebase_i(opts))\n> +\t\t\tunlink(git_path_cherry_pick_head(r));\n> \t\treturn;\n> \t}\n> \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 5afa6f28cd..5cd7db18f8 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -33,31 +33,35 @@ Initial setup:\n> # in the expect2 file for the 'stop on conflicting pick' test.\n> \n> test_expect_success 'setup' '\n> -\ttest_commit A file1 &&\n> -\ttest_commit B file1 &&\n> -\ttest_commit C file2 &&\n> -\ttest_commit D file1 &&\n> -\ttest_commit E file3 &&\n> -\tgit checkout -b branch1 A &&\n> -\ttest_commit F file4 &&\n> -\ttest_commit G file1 &&\n> -\ttest_commit H file5 &&\n> -\tgit checkout -b branch2 F &&\n> -\ttest_commit I file6 &&\n> -\tgit checkout -b conflict-branch A &&\n> -\ttest_commit one conflict &&\n> -\ttest_commit two conflict &&\n> -\ttest_commit three conflict &&\n> -\ttest_commit four conflict &&\n> -\tgit checkout -b no-conflict-branch A &&\n> -\ttest_commit J fileJ &&\n> -\ttest_commit K fileK &&\n> -\ttest_commit L fileL &&\n> -\ttest_commit M fileM &&\n> -\tgit checkout -b no-ff-branch A &&\n> -\ttest_commit N fileN &&\n> -\ttest_commit O fileO &&\n> -\ttest_commit P fileP\n> +\t(\n> +\t\tGIT_AUTHOR_NAME=\"Original Author\" &&\n> +\t\tGIT_AUTHOR_EMAIL=\"original.author@example.com\" &&\n> +\t\ttest_commit A file1 &&\n> +\t\ttest_commit B file1 &&\n> +\t\ttest_commit C file2 &&\n> +\t\ttest_commit D file1 &&\n> +\t\ttest_commit E file3 &&\n> +\t\tgit checkout -b branch1 A &&\n> +\t\ttest_commit F file4 &&\n> +\t\ttest_commit G file1 &&\n> +\t\ttest_commit H file5 &&\n> +\t\tgit checkout -b branch2 F &&\n> +\t\ttest_commit I file6 &&\n> +\t\tgit checkout -b conflict-branch A &&\n> +\t\ttest_commit one conflict &&\n> +\t\ttest_commit two conflict &&\n> +\t\ttest_commit three conflict &&\n> +\t\ttest_commit four conflict &&\n> +\t\tgit checkout -b no-conflict-branch A &&\n> +\t\ttest_commit J fileJ &&\n> +\t\ttest_commit K fileK &&\n> +\t\ttest_commit L fileL &&\n> +\t\ttest_commit M fileM &&\n> +\t\tgit checkout -b no-ff-branch A &&\n> +\t\ttest_commit N fileN &&\n> +\t\ttest_commit O fileO &&\n> +\t\ttest_commit P fileP\n> +\t)\n> '\n> \n> # \"exec\" commands are run with the user shell by default, but this may\n> @@ -252,12 +256,12 @@ test_expect_success 'stop on conflicting pick' '\n> \t-A\n> \t+G\n> \tEOF\n> -\tcat >expect2 <<-\\EOF &&\n> +\tcat >expect2 <<-EOF &&\n> \t<<<<<<< HEAD\n> \tD\n> \t=======\n> \tG\n> -\t>>>>>>> 5d18e54... G\n> +\t>>>>>>> $(git rev-parse --short HEAD)... G\n> \tEOF\n> \tgit tag new-branch1 &&\n> \ttest_must_fail git rebase -i master &&\n> @@ -1628,6 +1632,52 @@ test_expect_success 'correct error message for commit --amend after empty pick'\n> \ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n> '\n> \n> +test_expect_success 'correct error message for partial commit after confilct' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> +\tgit checkout D &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_LINES=\"2 3\" &&\n> +\t\texport FAKE_LINES &&\n> +\t\ttest_must_fail git rebase -i A\n> +\t) &&\n> +\techo x >file1 &&\n> +\techo y >file2 &&\n> +\tgit add file1 file2 &&\n> +\ttest_must_fail git commit file1 2>err &&\n> +\ttest_i18ngrep \"cannot do a partial commit during a rebase.\" err\n> +'\n> +\n> +test_expect_success 'correct error message for commit --amend after conflict' '\n> +\ttest_when_finished \"git rebase --abort\" &&\n> +\tgit checkout D &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_LINES=3 &&\n> +\t\texport FAKE_LINES &&\n> +\t\ttest_must_fail git rebase -i A\n> +\t) &&\n> +\techo x>file1 &&\n> +\ttest_must_fail git commit -a --amend 2>err &&\n> +\ttest_i18ngrep \"middle of a rebase -- cannot amend.\" err\n> +'\n> +\n> +test_expect_success 'correct authorship and message after conflict' '\n> +\tgit checkout D &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_LINES=3 &&\n> +\t\texport FAKE_LINES &&\n> +\t\ttest_must_fail git rebase -i A\n> +\t) &&\n> +\techo x >file1 &&\n> +\tgit commit -a &&\n> +\tgit log --pretty=format:\"%an <%ae>%n%ad%n%B\" -1 D >expect &&\n> +\tgit log --pretty=format:\"%an <%ae>%n%ad%n%B\" -1 HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit rebase --continue\n> +'\n> +\n> # This must be the last test in this file\n> test_expect_success '$EDITOR and friends are unchanged' '\n> \ttest_editor_unchanged\n> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\n> index c1eb72555d..2adceb35e2 100755\n> --- a/t/t7512-status-help.sh\n> +++ b/t/t7512-status-help.sh\n> @@ -148,7 +148,6 @@ You are currently rebasing branch '\\''rebase_i_conflicts_second'\\'' on '\\''$ONTO\n> (use \"git rebase --abort\" to check out the original branch)\n> \n> Unmerged paths:\n> - (use \"git reset HEAD ...\" to unstage)\n> (use \"git add ...\" to mark resolution)\n> \n> \tboth modified: main.txt\n> @@ -176,7 +175,6 @@ You are currently rebasing branch '\\''rebase_i_conflicts_second'\\'' on '\\''$ONTO\n> (all conflicts fixed: run \"git rebase --continue\")\n> \n> Changes to be committed:\n> - (use \"git reset HEAD ...\" to unstage)\n> \n> \tmodified: main.txt\n> \n> \n"},{"id":"392556","messageId":"CABPp-BHbn=0UpR=0RWyxsX_zFEx0v1VUamA3vMcQgDY+2454aw@mail.gmail.com","threadId":"52096","inReplyTo":"20191206160614.631724-9-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-26T19:45:36Z","receivedAt":"2020-02-26T19:45:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Dec 6, 2019 at 8:10 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Add a specific message to advise the user what to do when a fixup or\n> squash command would create an empty commit.\n>\n> Signed-off-by: Phillip Wood \n> ---\n>\n> Notes:\n> I'm slightly nervous of moving the call to determine_whence() to\n> finalized_deferred_config(). I think it is ok but need to do a more\n> thorough audit of the code to be sure.\n\nAny update?\n\n>\n> builtin/commit.c | 32 ++++++++++++++++++++++++++++----\n> sequencer.c | 31 ++++++++++++++++++++++++++++++-\n> sequencer.h | 3 ++-\n> t/t3403-rebase-skip.sh | 18 ++++++++++++------\n> wt-status.h | 5 +++--\n> 5 files changed, 75 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index c3b6b8a6bd..4ae984c470 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -52,6 +52,20 @@ N_(\"You asked to amend the most recent commit, but doing so would make\\n\"\n> \"it empty. You can repeat your command with --allow-empty, or you can\\n\"\n> \"remove the commit entirely with \\\"git reset HEAD^\\\".\\n\");\n>\n> +static const char empty_rebase_fixup_advice[] =\n> +N_(\"The fixup would make the commit empty\\n\"\n> +\"If you wish to commit it anyway use:\\n\"\n> +\"\\n\"\n> +\" git commit --amend --allow-empty\\n\"\n> +\" git rebase --continue\\n\"\n> +\"\\n\"\n> +\"To remove the commit entirely use:\\n\"\n> +\"\\n\"\n> +\" git reset HEAD^\\n\"\n> +\" git rebase --continue\\n\"\n> +\"\\n\"\n> +\"Otherwise, please use 'git rebase --skip' to skip it\\n\");\n\nHow does skipping differ from removing the commit entirely? Which one\ntosses just the changes from the fixup commit, and which tosses both\nthe fixup and the commit it is fixing? Or do they both do the same\nthing?\n\n> static const char empty_cherry_pick_advice[] =\n> N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\n\"\n> \"If you wish to commit it anyway, use:\\n\"\n> @@ -181,8 +195,14 @@ static void determine_whence(struct wt_status *s)\n> {\n> if (file_exists(git_path_merge_head(the_repository)))\n> whence = FROM_MERGE;\n> - else if (!sequencer_determine_whence(the_repository, &whence))\n> - whence = FROM_COMMIT;\n> + else {\n> + int res = sequencer_determine_whence(the_repository, &whence,\n> + amend);\n> + if (res < 0)\n> + die(_(\"could not read sequencer state\"));\n\nsequencer_determine_whence() tries to determine what type of sequencer\noperation is in effect, and can return a range of values in whence.\nIt can also fail to determine which type of sequencer operation is in\neffect -- in multiple ways. And it distinguishes those with via res <\n0 vs. res == 0. I guess it makes sense, but it seems a bit weird.\n\nNot sure what to suggest. One possibly bad idea just to get the\ncreative juices flowing: Maybe change the call signature to\n\nenum commit_whence sequencer_determine_whence(struct repository *r,\nint amending, enum commit_whence default);\n\nwith callers passing default == FROM_COMMIT, and the function can\nreturn FROM_COMMIT if there is no sequencer state, or FROM_ERROR\n(FROM_INVALID? FROM_FAILURE?) if there is sequencer state but it\ncannot be read.\n\n\n(And maybe do something similar with patch 6?)\n\n> + else if (!res)\n> + whence = FROM_COMMIT;\n> + }\n> if (s)\n> s->whence = whence;\n> }\n> @@ -192,7 +212,6 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)\n> wt_status_prepare(the_repository, s);\n> init_diff_ui_defaults();\n> git_config(fn, s);\n> - determine_whence(s);\n> s->hints = advice_status_hints; /* must come after git_config() */\n> }\n>\n> @@ -943,9 +962,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> */\n> if (!committable && whence != FROM_MERGE && !allow_empty &&\n> !(amend && is_a_merge(current_head))) {\n> + fprintf(stderr, \"\\nwhence = %d\\n\", whence);\n\nLeftover debugging?\n\n> s->display_comment_prefix = old_display_comment_prefix;\n> run_status(stdout, index_file, prefix, 0, s);\n> - if (amend)\n> + if (whence == FROM_REBASE_FIXUP)\n> + fputs(_(empty_rebase_fixup_advice), stderr);\n> + else if (amend)\n> fputs(_(empty_amend_advice), stderr);\n> else if (is_from_cherry_pick(whence) ||\n> whence == FROM_REBASE_PICK) {\n> @@ -1114,6 +1136,8 @@ static void finalize_deferred_config(struct wt_status *s)\n>\n> if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)\n> s->ahead_behind_flags = AHEAD_BEHIND_FULL;\n> +\n> + determine_whence(s);\n> }\n>\n> static int parse_and_validate_options(int argc, const char *argv[],\n> diff --git a/sequencer.c b/sequencer.c\n> index 6e153fea76..64242f4ce7 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5270,7 +5270,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> return 0;\n> }\n>\n> -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,\n> + int amending)\n> {\n> if (file_exists(git_path_cherry_pick_head(r))) {\n> struct object_id cherry_pick_head, rebase_head;\n> @@ -5286,6 +5287,34 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n> *whence = FROM_CHERRY_PICK_SINGLE;\n>\n> return 1;\n> + } else if (amending && file_exists(rebase_path_current_fixups()) &&\n> + (file_exists(git_path_squash_msg(r)) ||\n> + file_exists(git_path_merge_msg(r)))) {\n> + /*\n> + * If rebase_path_amend() exists the user is running `git\n> + * commit`, if not we're committing a fixup/squash directly from\n> + * the sequencer\n> + */\n> + if (file_exists(rebase_path_amend())) {\n> + struct strbuf rev = STRBUF_INIT;\n> + struct object_id to_amend, head;\n> +\n> + if (get_oid(\"HEAD\", &head))\n> + return error(_(\"amending invalid head\")); /* let commit deal with error */\n> + if (!read_oneliner(&rev, rebase_path_amend(), 0))\n> + return error(_(\"invalid file: '%s'\"),\n> + rebase_path_amend());\n> + if (get_oid_hex(rev.buf, &to_amend))\n> + return error(_(\"invalid contents: '%s'\"),\n> + rebase_path_amend());\n> + if (oideq(&head, &to_amend)) {\n> + *whence = FROM_REBASE_FIXUP;\n> + return 1;\n> + }\n> + } else {\n> + *whence = FROM_REBASE_FIXUP;\n> + return 1;\n> + }\n> }\n>\n> return 0;\n> diff --git a/sequencer.h b/sequencer.h\n> index 8d451dbfcb..4e631900b4 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -202,5 +202,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n> void sequencer_post_commit_cleanup(struct repository *r);\n> int sequencer_get_last_command(struct repository* r,\n> enum replay_action *action);\n> -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);\n> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,\n> + int amending);\n> #endif /* SEQUENCER_H */\n> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh\n> index a927774910..bc110b66b3 100755\n> --- a/t/t3403-rebase-skip.sh\n> +++ b/t/t3403-rebase-skip.sh\n> @@ -164,22 +164,28 @@ test_expect_success 'correct advice upon multi cherry-pick picking an empty comm\n> test_i18ngrep \"git cherry-pick --skip\" err\n> '\n>\n> -test_expect_success 'fixup that empties commit fails' '\n> +test_expect_success 'correct advice when fixup empties commit' '\n> test_when_finished \"git rebase --abort\" &&\n> (\n> set_fake_editor &&\n> test_must_fail env FAKE_LINES=\"1 fixup 2\" git rebase -i \\\n> - goodbye^ reverted-goodbye\n> - )\n> + goodbye^ reverted-goodbye 2>err\n> + ) &&\n> + test_i18ngrep \"git rebase --skip\" err &&\n> + test_must_fail git commit --amend --no-edit 2>err &&\n> + test_i18ngrep \"git rebase --skip\" err\n> '\n>\n> -test_expect_success 'squash that empties commit fails' '\n> +test_expect_success 'correct advice when squash empties commit' '\n> test_when_finished \"git rebase --abort\" &&\n> (\n> set_fake_editor &&\n> test_must_fail env FAKE_LINES=\"1 squash 2\" git rebase -i \\\n> - goodbye^ reverted-goodbye\n> - )\n> + goodbye^ reverted-goodbye 2>err\n> + ) &&\n> + test_i18ngrep \"git rebase --skip\" err &&\n> + test_must_fail git commit --amend --no-edit 2>err &&\n> + test_i18ngrep \"git rebase --skip\" err\n> '\n>\n> # Must be the last test in this file\n> diff --git a/wt-status.h b/wt-status.h\n> index 5f81bf7507..a4b7fe6de9 100644\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -41,7 +41,8 @@ enum commit_whence {\n> FROM_MERGE, /* commit came from merge */\n> FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */\n> FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */\n> - FROM_REBASE_PICK /* commit came from a pick/reword/edit */\n> + FROM_REBASE_PICK, /* commit came from a pick/reword/edit */\n> + FROM_REBASE_FIXUP /* commit came from fixup/squash */\n> };\n>\n> static inline int is_from_cherry_pick(enum commit_whence whence)\n> @@ -52,7 +53,7 @@ static inline int is_from_cherry_pick(enum commit_whence whence)\n>\n> static inline int is_from_rebase(enum commit_whence whence)\n> {\n> - return whence == FROM_REBASE_PICK;\n> + return whence == FROM_REBASE_PICK || whence == FROM_REBASE_FIXUP;\n> }\n>\n> struct wt_status_change_data {\n> --\n> 2.24.0\n>\n"}]}