Phillip Wood <phillip.wood123@xxxxxxxxx> writes: >>> 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 Ah, yes that is much easier to visualise and understand. Thanks so much. >>> 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. Unfortunately, it's been a while since I reviewed this patch, so forgive me if I'm rusty. So you're saying that this test is about checking invariants that we want to preserve between Git versions. I think that's a reasonable goal - I am slightly skeptical of whether we should be doing that ad-hoc like this, but I don't feel strongly about it. IIRC, there was an earlier patch would be different from an where we tested that author-script is missing, but what we really want is for the pick to stop. Is the same thing happening here? E.g. is 'testing for missing stopped-sha' a stand-in for 'testing that the rewritten list is correct'? If so, it would be nice to test that specifically, but if that's infeasible, a clarifying comment will probably suffice.
