| Message ID | e2a758eb4a5df0fab189b3dd235a1651a0c10342.1690903412.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When a rebase stops for the user to resolve conflicts it writes a patch > for the conflicting commit to .git/rebase-merge/patch. This file has > been written since the introduction of "git-rebase-interactive.sh" in > 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the > idea was to enable the user inspect the conflicting commit in the same > way as they could for the patch based rebase. This file should be > deleted when the rebase continues as if the rebase stops for a failed > "exec" command or a "break" command it is confusing to the user if there > is a stale patch lying around from an unrelated command. Unlike the previous step, this describes the change in end-user observable behaviour *and* asserts that it is an intended change. Very good. > As the path is > now used in two different places rebase_path_patch() is added and used > to obtain the path for the patch. The get_dir() function gives different paths, between "rebase-merge" (for "rebase -i") and "sequencer" (for everything else), and that is the parent directory of "/patch" output make_patch() uses. error_with_patch() is the only caller of make_patch(), and error_with_patch() is called by error_failed_squash() - called from pick_commits() pick_commits() - when TODO_EDIT stops the sequence, or a non fix-up insn failed when is_rebase_i(), or a merge insn failed, or a reschedule happened. Are we sure that it is the right thing to do to hardcode "rebase-merge/patch"? Unless "rebase -i" is the only thing that calls pick_commits() and reaches error_with_patch() to cause make_patch() to be called, this changes the behaviour for cases the tests added by this patch do not cover, doesn't it? I would feel safer if this change is accompanied by something like diff --git i/sequencer.c w/sequencer.c index cc9821ece2..a5ec8538fa 100644 --- i/sequencer.c +++ w/sequencer.c @@ -3502,6 +3502,9 @@ static int make_patch(struct repository *r, char hex[GIT_MAX_HEXSZ + 1]; int res = 0; + if (!is_rebase_i(opts)) + BUG("make-patch should only be triggered during rebase -i"); + oid_to_hex_r(hex, &commit->object.oid); if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0) return -1; to make sure that future changes to "git cherry-pick A..B" that makes it easier to edit .git/sequencer/todo and tweak "pick" into "edit" (aka "git cherry-pick -i") would not happen unless the author of such a change considerts its ramifications first. Alternatively, we could still introduce a handy path function, but call it sequencer_path_patch() that does get_dir() + "/patch", i.e. return different paths honoring is_rebase_i(), to make sure we will behave the same way as before. That might be safer. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > sequencer.c | 13 +++++++++---- > t/t3418-rebase-continue.sh | 18 ++++++++++++++++++ > 2 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index de66bda9d5b..70b0a7023b0 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4659,6 +4663,7 @@ static int pick_commits(struct repository *r, > unlink(rebase_path_message()); > unlink(rebase_path_stopped_sha()); > unlink(rebase_path_amend()); > + unlink(rebase_path_patch()); > > while (todo_list->current < todo_list->nr) { > struct todo_item *item = todo_list->items + todo_list->current; Other hunks are about "get_dir() + /patch" -> "rebase_path_patch()", but this hunk is about the intended behaviour change. We clear the leftover patch file from the previous round before we enter the loop to process new insn from the list, which makes sense. > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh > index 2d0789e554b..261e7cd754c 100755 > --- a/t/t3418-rebase-continue.sh > +++ b/t/t3418-rebase-continue.sh > @@ -244,6 +244,24 @@ test_expect_success 'the todo command "break" works' ' > test_path_is_file execed > ' > > +test_expect_success 'patch file is removed before break command' ' > + test_when_finished "git rebase --abort" && > + cat >todo <<-\EOF && > + pick commit-new-file-F2-on-topic-branch > + break > + EOF > + > + ( > + set_replace_editor todo && > + test_must_fail git rebase -i --onto commit-new-file-F2 HEAD > + ) && > + test_path_is_file .git/rebase-merge/patch && > + echo 22>F2 && > + git add F2 && > + git rebase --continue && > + test_path_is_missing .git/rebase-merge/patch > +' > + > test_expect_success '--reschedule-failed-exec' ' > test_when_finished "git rebase --abort" && > test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
On 01/08/2023 18:23, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> When a rebase stops for the user to resolve conflicts it writes a patch >> for the conflicting commit to .git/rebase-merge/patch. This file has >> been written since the introduction of "git-rebase-interactive.sh" in >> 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the >> idea was to enable the user inspect the conflicting commit in the same >> way as they could for the patch based rebase. This file should be >> deleted when the rebase continues as if the rebase stops for a failed >> "exec" command or a "break" command it is confusing to the user if there >> is a stale patch lying around from an unrelated command. > > Unlike the previous step, this describes the change in end-user > observable behaviour *and* asserts that it is an intended change. There is no intended change in the observable behavior in the previous patch. > Very good. > >> As the path is >> now used in two different places rebase_path_patch() is added and used >> to obtain the path for the patch. > > The get_dir() function gives different paths, between "rebase-merge" > (for "rebase -i") and "sequencer" (for everything else), and that is > the parent directory of "/patch" output make_patch() uses. Good point - the patch file is only ever created by "rebase -i". I'll add the assertion you suggest below. Best Wishes Phillip > error_with_patch() is the only caller of make_patch(), and > error_with_patch() is called by > > error_failed_squash() - called from pick_commits() > pick_commits() - when TODO_EDIT stops the sequence, or > a non fix-up insn failed when is_rebase_i(), or > a merge insn failed, or > a reschedule happened. > > Are we sure that it is the right thing to do to hardcode > "rebase-merge/patch"? Unless "rebase -i" is the only thing that > calls pick_commits() and reaches error_with_patch() to cause > make_patch() to be called, this changes the behaviour for cases the > tests added by this patch do not cover, doesn't it? > > I would feel safer if this change is accompanied by something like > > diff --git i/sequencer.c w/sequencer.c > index cc9821ece2..a5ec8538fa 100644 > --- i/sequencer.c > +++ w/sequencer.c > @@ -3502,6 +3502,9 @@ static int make_patch(struct repository *r, > char hex[GIT_MAX_HEXSZ + 1]; > int res = 0; > > + if (!is_rebase_i(opts)) > + BUG("make-patch should only be triggered during rebase -i"); > + > oid_to_hex_r(hex, &commit->object.oid); > if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0) > return -1; > > to make sure that future changes to "git cherry-pick A..B" that > makes it easier to edit .git/sequencer/todo and tweak "pick" into > "edit" (aka "git cherry-pick -i") would not happen unless the author > of such a change considerts its ramifications first. > > Alternatively, we could still introduce a handy path function, but > call it sequencer_path_patch() that does get_dir() + "/patch", i.e. > return different paths honoring is_rebase_i(), to make sure we will > behave the same way as before. That might be safer. > >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> sequencer.c | 13 +++++++++---- >> t/t3418-rebase-continue.sh | 18 ++++++++++++++++++ >> 2 files changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index de66bda9d5b..70b0a7023b0 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -4659,6 +4663,7 @@ static int pick_commits(struct repository *r, >> unlink(rebase_path_message()); >> unlink(rebase_path_stopped_sha()); >> unlink(rebase_path_amend()); >> + unlink(rebase_path_patch()); >> >> while (todo_list->current < todo_list->nr) { >> struct todo_item *item = todo_list->items + todo_list->current; > > Other hunks are about "get_dir() + /patch" -> "rebase_path_patch()", > but this hunk is about the intended behaviour change. We clear the > leftover patch file from the previous round before we enter the loop > to process new insn from the list, which makes sense. > >> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> index 2d0789e554b..261e7cd754c 100755 >> --- a/t/t3418-rebase-continue.sh >> +++ b/t/t3418-rebase-continue.sh >> @@ -244,6 +244,24 @@ test_expect_success 'the todo command "break" works' ' >> test_path_is_file execed >> ' >> >> +test_expect_success 'patch file is removed before break command' ' >> + test_when_finished "git rebase --abort" && >> + cat >todo <<-\EOF && >> + pick commit-new-file-F2-on-topic-branch >> + break >> + EOF >> + >> + ( >> + set_replace_editor todo && >> + test_must_fail git rebase -i --onto commit-new-file-F2 HEAD >> + ) && >> + test_path_is_file .git/rebase-merge/patch && >> + echo 22>F2 && >> + git add F2 && >> + git rebase --continue && >> + test_path_is_missing .git/rebase-merge/patch >> +' >> + >> test_expect_success '--reschedule-failed-exec' ' >> test_when_finished "git rebase --abort" && >> test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
diff --git a/sequencer.c b/sequencer.c index de66bda9d5b..70b0a7023b0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -138,6 +138,11 @@ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") * the commit object name of the corresponding patch. */ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha") +/* + * When we stop for the user to resolve conflicts this file contains + * the patch of the commit that is being picked. + */ +static GIT_PATH_FUNC(rebase_path_patch, "rebase-merge/patch") /* * For the post-rewrite hook, we make a list of rewritten commits and * their new sha1s. The rewritten-pending list keeps the sha1s of @@ -3507,7 +3512,6 @@ static int make_patch(struct repository *r, return -1; res |= write_rebase_head(&commit->object.oid); - strbuf_addf(&buf, "%s/patch", get_dir(opts)); memset(&log_tree_opt, 0, sizeof(log_tree_opt)); repo_init_revisions(r, &log_tree_opt, NULL); log_tree_opt.abbrev = 0; @@ -3515,15 +3519,15 @@ static int make_patch(struct repository *r, log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH; log_tree_opt.disable_stdin = 1; log_tree_opt.no_commit_id = 1; - log_tree_opt.diffopt.file = fopen(buf.buf, "w"); + log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w"); log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER; if (!log_tree_opt.diffopt.file) - res |= error_errno(_("could not open '%s'"), buf.buf); + res |= error_errno(_("could not open '%s'"), + rebase_path_patch()); else { res |= log_tree_commit(&log_tree_opt, commit); fclose(log_tree_opt.diffopt.file); } - strbuf_reset(&buf); strbuf_addf(&buf, "%s/message", get_dir(opts)); if (!file_exists(buf.buf)) { @@ -4659,6 +4663,7 @@ static int pick_commits(struct repository *r, unlink(rebase_path_message()); unlink(rebase_path_stopped_sha()); unlink(rebase_path_amend()); + unlink(rebase_path_patch()); while (todo_list->current < todo_list->nr) { struct todo_item *item = todo_list->items + todo_list->current; diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 2d0789e554b..261e7cd754c 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -244,6 +244,24 @@ test_expect_success 'the todo command "break" works' ' test_path_is_file execed ' +test_expect_success 'patch file is removed before break command' ' + test_when_finished "git rebase --abort" && + cat >todo <<-\EOF && + pick commit-new-file-F2-on-topic-branch + break + EOF + + ( + set_replace_editor todo && + test_must_fail git rebase -i --onto commit-new-file-F2 HEAD + ) && + test_path_is_file .git/rebase-merge/patch && + echo 22>F2 && + git add F2 && + git rebase --continue && + test_path_is_missing .git/rebase-merge/patch +' + test_expect_success '--reschedule-failed-exec' ' test_when_finished "git rebase --abort" && test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&