Hi Glen
On 21/06/2023 21:49, Glen Choo wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
When rebasing commands are moved from the todo list in "git-rebase-todo"
to the "done" file just before they are executed. This means that if a
command fails because it would overwrite an untracked file it has to be
added back into the todo list before the rebase stops for the user to
fix the problem. Unfortunately the way this is done results in the
failed pick being recorded as rewritten.
I could not make the connection from the described problem to the
proposed solution. In particular, I couldn't tell what about "the way
this is done" that causes the incorrect behavior (e.g. are we failing to
clean up something? are we writing the wrong set of metadata?).
Yes, on reflection that first paragraph is not very helpful. I've
updated it to
git rebase keeps a list that maps the OID of each commit before
it was rebased to the OID of the equivalent commit after the rebase.
This list is used to drive the "post-rewrite" hook that is called at the
end of a successful rebase. When a rebase stops for the user to resolve
merge conflicts the OID of the commit being picked is written to
".git/rebase-merge/stopped-sha1" and when the rebase is continued that
OID is added to the list of rewritten commits. Unfortunately when a
commit cannot be picked because it would overwrite an untracked file we
still write the "stopped-sha1" file and so when the rebase is continued
the commit is added into the list of rewritten commits even though it
has not been picked yet.
Hopefully that is more helpful
Fix this by not calling error_with_patch() for failed commands.
So unfortunately , I wasn't sure how this solution would fix the
problem, and I didn't dive too deeply into this patch.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c1fe55dc2c1..a657167befd 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1289,6 +1289,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
test_cmp_rev HEAD F &&
rm file6 &&
test_path_is_missing .git/rebase-merge/author-script &&
+ test_path_is_missing .git/rebase-merge/patch &&
+ test_path_is_missing .git/MERGE_MSG &&
+ test_path_is_missing .git/rebase-merge/message &&
+ test_path_is_missing .git/rebase-merge/stopped-sha &&
This also seems to be testing implementation details, and if so, it
would be worth removing them.
With the exception of the "patch" file which exists solely for the
benefit of the user this is testing an invariant of the implementation
which isn't ideal. I'm worried that removing these checks will mask some
subtle regression in the future. I think it is unlikely that the names
of these files will change in the future as we try to avoid changes that
would cause a rebase to fail if git is upgraded while it has stopped for
the user to resolve conflicts. I did think about whether we could add
some BUG() statements to sequencer.c instead. Unfortunately I don't
think it is that easy for the sequencer to know when these files should
be missing without relying on the logic that we are tying to test.
Best Wishes
Phillip
