{"thread":{"id":"49815","subject":"[PATCH 0/5] Assorted fixes revolving around rebase and merges","startedAt":"2018-11-12T23:25:59Z","lastAt":"2018-11-13T12:56:40Z","messageCount":14,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"363100","messageId":"pull.75.git.gitgitgadget@gmail.com","threadId":"49815","inReplyTo":null,"subject":"[PATCH 0/5] Assorted fixes revolving around rebase and merges","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T23:25:56Z","receivedAt":"2018-11-12T23:25:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I noticed a couple of weeks ago that I had bogus merge commits after my\nrebases, where the original commits had been regular commits.\n\nThis set me out on the adventure that is reflected in this patch series.\n\nOf course, the thing I wanted to fix is demonstrated by 1/5 and fixed in\n2/5. But while at it, I ran into other issues and fixed them since I was at\nit anyway.\n\nJohannes Schindelin (5):\n rebase -r: demonstrate bug with conflicting merges\n rebase -r: do not write MERGE_HEAD unless needed\n rebase -i: include MERGE_HEAD into files to clean up\n built-in rebase --skip/--abort: clean up stale .git/ files\n status: rebase and merge can be in progress at the same time\n\n builtin/rebase.c | 3 +++\n sequencer.c | 10 ++++++----\n t/t3430-rebase-merges.sh | 16 ++++++++++++++++\n wt-status.c | 9 +++++++--\n 4 files changed, 32 insertions(+), 6 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-75%2Fdscho%2Frebase-r-and-merge-head-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-75/dscho/rebase-r-and-merge-head-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/75\n-- \ngitgitgadget\n"},{"id":"363101","messageId":"efdd3736a96f90a4ab52acaf2e5efbe3435bcb89.1542065154.git.gitgitgadget@gmail.com","threadId":"49815","inReplyTo":"pull.75.git.gitgitgadget@gmail.com","subject":"[PATCH 1/5] rebase -r: demonstrate bug with conflicting merges","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T23:25:57Z","receivedAt":"2018-11-12T23:26:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen calling `merge` on a branch that has already been merged, that\n`merge` is skipped quietly, but currently a MERGE_HEAD file is being\nleft behind and will then be grabbed by the next `pick` (that did\nnot want to create a *merge* commit).\n\nDemonstrate this.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3430-rebase-merges.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex aa7bfc88ec..1f08a33687 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '\n \tgrep \"G: +G\" actual\n '\n \n+test_expect_failure '--continue after resolving conflicts after a merge' '\n+\tgit checkout -b already-has-g E &&\n+\tgit cherry-pick E..G &&\n+\ttest_commit H2 &&\n+\n+\tgit checkout -b conflicts-in-merge H &&\n+\ttest_commit H2 H2.t conflicts H2-conflict &&\n+\ttest_must_fail git rebase -r already-has-g &&\n+\tgrep conflicts H2.t &&\n+\techo resolved >H2.t &&\n+\tgit add -u &&\n+\tgit rebase --continue &&\n+\ttest_must_fail git rev-parse --verify HEAD^2 &&\n+\ttest_path_is_missing .git/MERGE_HEAD\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"363102","messageId":"8aa3dac3147fdd2483dd530792baf3c575d3d3aa.1542065154.git.gitgitgadget@gmail.com","threadId":"49815","inReplyTo":"pull.75.git.gitgitgadget@gmail.com","subject":"[PATCH 2/5] rebase -r: do not write MERGE_HEAD unless needed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T23:25:58Z","receivedAt":"2018-11-12T23:26:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen we detect that a `merge` can be skipped because the merged commit\nis already an ancestor of HEAD, we do not need to commit, therefore\nwriting the MERGE_HEAD file is useless.\n\nIt is actually worse than useless: a subsequent `git commit` will pick\nit up and think that we want to merge that commit, still.\n\nTo avoid that, move the code that writes the MERGE_HEAD file to a\nlocation where we already know that the `merge` cannot be skipped.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 8 ++++----\n t/t3430-rebase-merges.sh | 2 +-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 9e1ab3a2a7..7a9cd81afb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3191,10 +3191,6 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,\n \t}\n \n \tmerge_commit = to_merge->item;\n-\twrite_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,\n-\t\t git_path_merge_head(the_repository), 0);\n-\twrite_message(\"no-ff\", 5, git_path_merge_mode(the_repository), 0);\n-\n \tbases = get_merge_bases(head_commit, merge_commit);\n \tif (bases && oideq(&merge_commit->object.oid,\n \t\t\t &bases->item->object.oid)) {\n@@ -3203,6 +3199,10 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,\n \t\tgoto leave_merge;\n \t}\n \n+\twrite_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,\n+\t\t git_path_merge_head(the_repository), 0);\n+\twrite_message(\"no-ff\", 5, git_path_merge_mode(the_repository), 0);\n+\n \tfor (j = bases; j; j = j->next)\n \t\tcommit_list_insert(j->item, &reversed);\n \tfree_commit_list(bases);\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 1f08a33687..cc5646836f 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -396,7 +396,7 @@ test_expect_success 'with --autosquash and --exec' '\n \tgrep \"G: +G\" actual\n '\n \n-test_expect_failure '--continue after resolving conflicts after a merge' '\n+test_expect_success '--continue after resolving conflicts after a merge' '\n \tgit checkout -b already-has-g E &&\n \tgit cherry-pick E..G &&\n \ttest_commit H2 &&\n-- \ngitgitgadget\n\n"},{"id":"363103","messageId":"65f02628f6ef96e88485b66cd2c6e5e57212b4bb.1542065154.git.gitgitgadget@gmail.com","threadId":"49815","inReplyTo":"pull.75.git.gitgitgadget@gmail.com","subject":"[PATCH 3/5] rebase -i: include MERGE_HEAD into files to clean up","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T23:25:59Z","receivedAt":"2018-11-12T23:26:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEvery once in a while, the interactive rebase makes sure that no stale\nfiles are lying around. These days, we need to include MERGE_HEAD into\nthat set of files, as the `merge` command will generate them.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7a9cd81afb..2f526390ac 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3459,6 +3459,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t\tunlink(rebase_path_author_script());\n \t\t\tunlink(rebase_path_stopped_sha());\n \t\t\tunlink(rebase_path_amend());\n+\t\t\tunlink(git_path_merge_head(the_repository));\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n \t\t\tif (item->command == TODO_BREAK)\n@@ -3829,6 +3830,7 @@ static int commit_staged_changes(struct replay_opts *opts,\n \t\t\t opts, flags))\n \t\treturn error(_(\"could not commit staged changes.\"));\n \tunlink(rebase_path_amend());\n+\tunlink(git_path_merge_head(the_repository));\n \tif (final_fixup) {\n \t\tunlink(rebase_path_fixup_msg());\n \t\tunlink(rebase_path_squash_msg());\n-- \ngitgitgadget\n\n"},{"id":"363104","messageId":"8d1dec51b704c45bf36af24f657cc40f006989e2.1542065154.git.gitgitgadget@gmail.com","threadId":"49815","inReplyTo":"pull.75.git.gitgitgadget@gmail.com","subject":"[PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/ files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T23:26:01Z","receivedAt":"2018-11-12T23:26:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe scripted version of the rebase used to execute `git reset --hard`\nwhen skipping or aborting. When we ported this to C, we did update the\nworktree and some reflogs, but we failed to imitate `git reset --hard`'s\nbehavior regarding files in .git/ such as MERGE_HEAD.\n\nLet's address this oversight.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..017dce1b50 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -23,6 +23,7 @@\n #include \"revision.h\"\n #include \"commit-reach.h\"\n #include \"rerere.h\"\n+#include \"branch.h\"\n \n static char const * const builtin_rebase_usage[] = {\n \tN_(\"git rebase [-i] [options] [--exec ] [--onto ] \"\n@@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n \t\t\tdie(_(\"could not discard worktree changes\"));\n+\t\tremove_branch_state();\n \t\tif (read_basic_state(&options))\n \t\t\texit(1);\n \t\tgoto run_rebase;\n@@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t options.head_name, 0, NULL, NULL) < 0)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t oid_to_hex(&options.orig_head));\n+\t\tremove_branch_state();\n \t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"363105","messageId":"7b2f5e37f4dca7cafb4d6ea5eebafc31e120a339.1542065154.git.gitgitgadget@gmail.com","threadId":"49815","inReplyTo":"pull.75.git.gitgitgadget@gmail.com","subject":"[PATCH 5/5] status: rebase and merge can be in progress at the same time","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-12T23:26:02Z","receivedAt":"2018-11-12T23:26:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSince `git rebase -r` was introduced, that is possible. But our\nmachinery did not think that possible, and failed to say anything about\nthe rebase in progress when in the middle of a merge.\n\nLet's work around that in the minimal fashion.\n\nSigned-off-by: Johannes Schindelin \n---\n wt-status.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 187568a112..a24711374c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1559,6 +1559,7 @@ void wt_status_get_state(struct wt_status_state *state,\n \tstruct object_id oid;\n \n \tif (!stat(git_path_merge_head(the_repository), &st)) {\n+\t\twt_status_check_rebase(NULL, state);\n \t\tstate->merge_in_progress = 1;\n \t} else if (wt_status_check_rebase(NULL, state)) {\n \t\t;\t\t/* all set */\n@@ -1583,9 +1584,13 @@ static void wt_longstatus_print_state(struct wt_status *s)\n \tconst char *state_color = color(WT_STATUS_HEADER, s);\n \tstruct wt_status_state *state = &s->state;\n \n-\tif (state->merge_in_progress)\n+\tif (state->merge_in_progress) {\n+\t\tif (state->rebase_interactive_in_progress) {\n+\t\t\tshow_rebase_information(s, state_color);\n+\t\t\tfputs(\"\\n\", s->fp);\n+\t\t}\n \t\tshow_merge_in_progress(s, state_color);\n-\telse if (state->am_in_progress)\n+\t} else if (state->am_in_progress)\n \t\tshow_am_in_progress(s, state_color);\n \telse if (state->rebase_in_progress || state->rebase_interactive_in_progress)\n \t\tshow_rebase_in_progress(s, state_color);\n-- \ngitgitgadget\n"},{"id":"363121","messageId":"xmqqpnv9ybkh.fsf@gitster-ct.c.googlers.com","threadId":"49815","inReplyTo":"65f02628f6ef96e88485b66cd2c6e5e57212b4bb.1542065154.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] rebase -i: include MERGE_HEAD into files to clean up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T02:07:42Z","receivedAt":"2018-11-13T02:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Every once in a while, the interactive rebase makes sure that no stale\n> files are lying around. These days, we need to include MERGE_HEAD into\n> that set of files, as the `merge` command will generate them.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 2 ++\n> 1 file changed, 2 insertions(+)\n\nMakes sense.\n\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 7a9cd81afb..2f526390ac 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3459,6 +3459,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n> \t\t\tunlink(rebase_path_author_script());\n> \t\t\tunlink(rebase_path_stopped_sha());\n> \t\t\tunlink(rebase_path_amend());\n> +\t\t\tunlink(git_path_merge_head(the_repository));\n> \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n> \n> \t\t\tif (item->command == TODO_BREAK)\n> @@ -3829,6 +3830,7 @@ static int commit_staged_changes(struct replay_opts *opts,\n> \t\t\t opts, flags))\n> \t\treturn error(_(\"could not commit staged changes.\"));\n> \tunlink(rebase_path_amend());\n> +\tunlink(git_path_merge_head(the_repository));\n> \tif (final_fixup) {\n> \t\tunlink(rebase_path_fixup_msg());\n> \t\tunlink(rebase_path_squash_msg());\n"},{"id":"363123","messageId":"xmqqlg5xybdg.fsf@gitster-ct.c.googlers.com","threadId":"49815","inReplyTo":"8d1dec51b704c45bf36af24f657cc40f006989e2.1542065154.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/ files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T02:11:55Z","receivedAt":"2018-11-13T02:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The scripted version of the rebase used to execute `git reset --hard`\n> when skipping or aborting. When we ported this to C, we did update the\n> worktree and some reflogs, but we failed to imitate `git reset --hard`'s\n> behavior regarding files in .git/ such as MERGE_HEAD.\n>\n> Let's address this oversight.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/rebase.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 0ee06aa363..017dce1b50 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -23,6 +23,7 @@\n> #include \"revision.h\"\n> #include \"commit-reach.h\"\n> #include \"rerere.h\"\n> +#include \"branch.h\"\n> \n> static char const * const builtin_rebase_usage[] = {\n> \tN_(\"git rebase [-i] [options] [--exec ] [--onto ] \"\n> @@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \n> \t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n> \t\t\tdie(_(\"could not discard worktree changes\"));\n> +\t\tremove_branch_state();\n> \t\tif (read_basic_state(&options))\n> \t\t\texit(1);\n> \t\tgoto run_rebase;\n> @@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t options.head_name, 0, NULL, NULL) < 0)\n> \t\t\tdie(_(\"could not move back to %s\"),\n> \t\t\t oid_to_hex(&options.orig_head));\n> +\t\tremove_branch_state();\n\nHmph. Among 5 or so callsites of reset_head(), only these two\nplaces need it, so reset_head() is clearly not a substitute for\n\"reset --hard HEAD\", and it sometimes used to switch branches or\nsomething? Perhaps we may need to rename it to avoid confusion but\nit can wait until we actually decide to make it externally\navailable. Until then, it's OK as-is, I would think.\n\n> \t\tret = finish_rebase(&options);\n> \t\tgoto cleanup;\n> \t}\n"},{"id":"363124","messageId":"xmqqh8glyajx.fsf@gitster-ct.c.googlers.com","threadId":"49815","inReplyTo":"efdd3736a96f90a4ab52acaf2e5efbe3435bcb89.1542065154.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T02:29:38Z","receivedAt":"2018-11-13T02:29:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When calling `merge` on a branch that has already been merged, that\n> `merge` is skipped quietly, but currently a MERGE_HEAD file is being\n> left behind and will then be grabbed by the next `pick` (that did\n> not want to create a *merge* commit).\n>\n> Demonstrate this.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t3430-rebase-merges.sh | 16 ++++++++++++++++\n> 1 file changed, 16 insertions(+)\n\nFor a trivially small change/fix like this, it is OK and even\npreferrable to make 1+2 a single step, as applying t/ part only to\ntry to see the breakage (or \"am\"ing everything and then \"diff |\napply -R\" the part outside t/ for the same purpose) is easy enough.\n\nBecause the patch 2 with your method ends up showing only the test\nset-up part in the context by changing _failure to _success, without\nshowing what end-user visible breakage the step fixed, which usually\ncomes near the end of the added test piece. A single patch that\ngives tests that ought to succeed would not force the readers to\nswitch between patches 1 and 2 while reading the fix.\n\nOf course, the above would not apply for a more involved case where\nthe actual fix to the code needs to span multiple patches.\n\nThanks.\n\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index aa7bfc88ec..1f08a33687 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '\n> \tgrep \"G: +G\" actual\n> '\n> \n> +test_expect_failure '--continue after resolving conflicts after a merge' '\n> +\tgit checkout -b already-has-g E &&\n> +\tgit cherry-pick E..G &&\n> +\ttest_commit H2 &&\n> +\n> +\tgit checkout -b conflicts-in-merge H &&\n> +\ttest_commit H2 H2.t conflicts H2-conflict &&\n> +\ttest_must_fail git rebase -r already-has-g &&\n> +\tgrep conflicts H2.t &&\n\nIs this making sure that the above test_must_fail succeeded because\nof a conflict and not due to any other failure? I would have used\n\"ls-files -u H2.t\" to see if the index is unmerged, which probably\nis a more direct way to test what this is trying to test, but if we\nare in the conflicted state, the one side of << == >> has this\nstring (the other has \"H2\" in it, presumably?), so in practice this\nshould be good enough.\n\n> +\techo resolved >H2.t &&\n> +\tgit add -u &&\n\nand we resolve to continue.\n\n> +\tgit rebase --continue &&\n> +\ttest_must_fail git rev-parse --verify HEAD^2 &&\n\nEven if we made an octopus by mistake, the above will catch it,\nwhich is good.\n\n> +\ttest_path_is_missing .git/MERGE_HEAD\n> +'\n> +\n> test_done\n\nAnd from the proposed log message, I am reading that the last two\nthings (i.e. resulting tip is a child with a single parent and there\nis no leftover MERGE_HEAD file) fail without the fix. \n\nThis is enough material to convince me or anybody that the bug is\nworth fixing. Thanks for being careful noticing a glitch during\nyour real (and otherwise unrelated to the bug) work and following\nthrough.\n"},{"id":"363141","messageId":"nycvar.QRO.7.76.6.1811131109360.39@tvgsbejvaqbjf.bet","threadId":"49815","inReplyTo":"xmqqh8glyajx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-13T10:12:18Z","receivedAt":"2018-11-13T10:12:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 13 Nov 2018, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > When calling `merge` on a branch that has already been merged, that\n> > `merge` is skipped quietly, but currently a MERGE_HEAD file is being\n> > left behind and will then be grabbed by the next `pick` (that did\n> > not want to create a *merge* commit).\n> >\n> > Demonstrate this.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > t/t3430-rebase-merges.sh | 16 ++++++++++++++++\n> > 1 file changed, 16 insertions(+)\n> \n> For a trivially small change/fix like this, it is OK and even\n> preferrable to make 1+2 a single step, as applying t/ part only to\n> try to see the breakage (or \"am\"ing everything and then \"diff |\n> apply -R\" the part outside t/ for the same purpose) is easy enough.\n\nI disagree. It helps both development and porting to different branches to\nbe able to cherry-pick the regression test individually. Please do not ask\nme to violate this hard-learned principle.\n\n> Because the patch 2 with your method ends up showing only the test\n> set-up part in the context by changing _failure to _success, without\n> showing what end-user visible breakage the step fixed, which usually\n> comes near the end of the added test piece. A single patch that\n> gives tests that ought to succeed would not force the readers to\n> switch between patches 1 and 2 while reading the fix.\n\nThat is why I put in a verbose commit message, so that you do not have to\nguess. And even the test title talks about this.\n\nSeriously, I am very much opposed to changing the patches in the direction\nyou suggested. In my mind, they would make the story substantially worse.\n\nThank you for your review,\nDscho\n\n> \n> Of course, the above would not apply for a more involved case where\n> the actual fix to the code needs to span multiple patches.\n> \n> Thanks.\n> \n> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> > index aa7bfc88ec..1f08a33687 100755\n> > --- a/t/t3430-rebase-merges.sh\n> > +++ b/t/t3430-rebase-merges.sh\n> > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '\n> > \tgrep \"G: +G\" actual\n> > '\n> > \n> > +test_expect_failure '--continue after resolving conflicts after a merge' '\n> > +\tgit checkout -b already-has-g E &&\n> > +\tgit cherry-pick E..G &&\n> > +\ttest_commit H2 &&\n> > +\n> > +\tgit checkout -b conflicts-in-merge H &&\n> > +\ttest_commit H2 H2.t conflicts H2-conflict &&\n> > +\ttest_must_fail git rebase -r already-has-g &&\n> > +\tgrep conflicts H2.t &&\n> \n> Is this making sure that the above test_must_fail succeeded because\n> of a conflict and not due to any other failure? I would have used\n> \"ls-files -u H2.t\" to see if the index is unmerged, which probably\n> is a more direct way to test what this is trying to test, but if we\n> are in the conflicted state, the one side of << == >> has this\n> string (the other has \"H2\" in it, presumably?), so in practice this\n> should be good enough.\n> \n> > +\techo resolved >H2.t &&\n> > +\tgit add -u &&\n> \n> and we resolve to continue.\n> \n> > +\tgit rebase --continue &&\n> > +\ttest_must_fail git rev-parse --verify HEAD^2 &&\n> \n> Even if we made an octopus by mistake, the above will catch it,\n> which is good.\n> \n> > +\ttest_path_is_missing .git/MERGE_HEAD\n> > +'\n> > +\n> > test_done\n> \n> And from the proposed log message, I am reading that the last two\n> things (i.e. resulting tip is a child with a single parent and there\n> is no leftover MERGE_HEAD file) fail without the fix. \n> \n> This is enough material to convince me or anybody that the bug is\n> worth fixing. Thanks for being careful noticing a glitch during\n> your real (and otherwise unrelated to the bug) work and following\n> through.\n> \n"},{"id":"363142","messageId":"nycvar.QRO.7.76.6.1811131112580.39@tvgsbejvaqbjf.bet","threadId":"49815","inReplyTo":"xmqqlg5xybdg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/ files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-13T10:14:30Z","receivedAt":"2018-11-13T10:14:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 13 Nov 2018, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > The scripted version of the rebase used to execute `git reset --hard`\n> > when skipping or aborting. When we ported this to C, we did update the\n> > worktree and some reflogs, but we failed to imitate `git reset --hard`'s\n> > behavior regarding files in .git/ such as MERGE_HEAD.\n> >\n> > Let's address this oversight.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/rebase.c | 3 +++\n> > 1 file changed, 3 insertions(+)\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 0ee06aa363..017dce1b50 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -23,6 +23,7 @@\n> > #include \"revision.h\"\n> > #include \"commit-reach.h\"\n> > #include \"rerere.h\"\n> > +#include \"branch.h\"\n> > \n> > static char const * const builtin_rebase_usage[] = {\n> > \tN_(\"git rebase [-i] [options] [--exec ] [--onto ] \"\n> > @@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > \n> > \t\tif (reset_head(NULL, \"reset\", NULL, 0, NULL, NULL) < 0)\n> > \t\t\tdie(_(\"could not discard worktree changes\"));\n> > +\t\tremove_branch_state();\n> > \t\tif (read_basic_state(&options))\n> > \t\t\texit(1);\n> > \t\tgoto run_rebase;\n> > @@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > \t\t\t options.head_name, 0, NULL, NULL) < 0)\n> > \t\t\tdie(_(\"could not move back to %s\"),\n> > \t\t\t oid_to_hex(&options.orig_head));\n> > +\t\tremove_branch_state();\n> \n> Hmph. Among 5 or so callsites of reset_head(), only these two\n> places need it, so reset_head() is clearly not a substitute for\n> \"reset --hard HEAD\", and it sometimes used to switch branches or\n> something?\n\nIndeed. There is also the `git reset --hard` call in the scripted\nversion's autostash code path. But that definitely does not need to remove\nthe branch state, as it is not recovering from a merge or cherry-pick or\nrevert.\n\n> Perhaps we may need to rename it to avoid confusion but it can wait\n> until we actually decide to make it externally available. Until then,\n> it's OK as-is, I would think.\n\nOkay.\n\nCiao,\nDscho\n\n> \n> > \t\tret = finish_rebase(&options);\n> > \t\tgoto cleanup;\n> > \t}\n> \n"},{"id":"363144","messageId":"xmqqtvkluqpb.fsf@gitster-ct.c.googlers.com","threadId":"49815","inReplyTo":"nycvar.QRO.7.76.6.1811131109360.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T12:06:40Z","receivedAt":"2018-11-13T12:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> For a trivially small change/fix like this, it is OK and even\n>> preferrable to make 1+2 a single step, as applying t/ part only to\n>> try to see the breakage (or \"am\"ing everything and then \"diff |\n>> apply -R\" the part outside t/ for the same purpose) is easy enough.\n>\n> I disagree. It helps both development and porting to different branches to\n> be able to cherry-pick the regression test individually. Please do not ask\n> me to violate this hard-learned principle.\n\nA trivially small change/fix like this, by definition (of \"trivial\"\nand \"small\" ness), it is trivial to develop and port to different\nbranches a single patch, and test with just one half (either the\ntest part or the code-change part) of the change reversed, to ensure\nthat the codebase is broken without the code-change and to\ndemonstrate that the code-change does fix the problem revealed by\nthe test change. And \"porting\" by cherry-picking a single patch is\nalways easier than two patch series.\n\nSo you may disagree all you want in your project, but do not make\nreviewer's lives unnecessarily harder in this project.\n\nThanks.\n"},{"id":"363148","messageId":"nycvar.QRO.7.76.6.1811131345080.39@tvgsbejvaqbjf.bet","threadId":"49815","inReplyTo":"xmqqtvkluqpb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-13T12:47:34Z","receivedAt":"2018-11-13T12:47:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 13 Nov 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> For a trivially small change/fix like this, it is OK and even\n> >> preferrable to make 1+2 a single step, as applying t/ part only to\n> >> try to see the breakage (or \"am\"ing everything and then \"diff |\n> >> apply -R\" the part outside t/ for the same purpose) is easy enough.\n> >\n> > I disagree. It helps both development and porting to different branches to\n> > be able to cherry-pick the regression test individually. Please do not ask\n> > me to violate this hard-learned principle.\n> \n> A trivially small change/fix like this, by definition (of \"trivial\"\n> and \"small\" ness), it is trivial to develop and port to different\n> branches a single patch, and test with just one half (either the\n> test part or the code-change part) of the change reversed, to ensure\n> that the codebase is broken without the code-change and to\n> demonstrate that the code-change does fix the problem revealed by\n> the test change. And \"porting\" by cherry-picking a single patch is\n> always easier than two patch series.\n> \n> So you may disagree all you want in your project, but do not make\n> reviewer's lives unnecessarily harder in this project.\n\nYou misunderstand. In this case it is crucial to read the regression test\nfirst. The fix does not make much sense before one understands the\ncondition under which the order of the code statements matters.\n\nBy trying to force me to smoosh them together, you are trying to force me\nto combine them in one patch where you would read the (now seemingly\nnon-sensical) fix first, and only then the test.\n\nThat's just really unhelpful. If I were a reviewer, I would want it\npresented in the way it *was* presented. I firmly believe most reviewers\nwould.\n\nCiao,\nDscho\n"},{"id":"363149","messageId":"xmqqpnv9uoe4.fsf@gitster-ct.c.googlers.com","threadId":"49815","inReplyTo":"nycvar.QRO.7.76.6.1811131345080.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T12:56:35Z","receivedAt":"2018-11-13T12:56:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> You misunderstand. In this case it is crucial to read the regression test\n> first. The fix does not make much sense before one understands the\n> condition under which the order of the code statements matters.\n\nI am not sure what you mean. It sounds as if you want to use\ndiff-orderfile to present change for t/ before changes to other\nfiles are presented in format-patch output to help readers, and I\nthink that may make sense for certain cases. It may even include\nthis case.\n\nBut that is not incompatible with \"avoid showing the patch that\nupdates the code to fix breakages separately, ending up with showing\nthe changes to t/ that are mostly about s/_failure/_success/ and\nreaders are forced to go back to the previous patch to remind\nthemselves what the broken scenario was about; by keeping it in a\nsingle patch, the readers can get the tests in one piece\".\n"}]}