From: Phillip Wood Date: Tue, 01 Aug 2023 18:47:08 GMT Subject: Re: [PATCH v3 2/7] rebase -i: remove patch file after conflict resolution Message-ID: <8588c7d5-6ba1-7916-4131-bcf388452254@gmail.com> In-Reply-To: On 01/08/2023 18:23, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood >> >> 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 >> --- >> 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^ &&