From: Mark Jamsek Subject: Re: rebase suffers '?' conflicts To: Christian Weisgerber , gameoftrees@openbsd.org Date: Thu, 13 Apr 2023 00:21:29 +1000 On 23-04-12 02:26PM, Stefan Sperling wrote: > On Sat, Apr 01, 2023 at 04:13:54PM +0200, Christian Weisgerber wrote: > > Christian Weisgerber: > > > > > git clone --bare https://github.com/openbsd/src.git > > > cd src.git > > > git config remote.origin.fetch '+refs/heads/*:refs/remotes/origin/*' > > > git remote add jca https://github.com/jcourreges/openbsd-src.git > > > git fetch --all > > > cd .. > > > got co src.git > > > cd src > > > got br tmp > > > got br -c jca/llvm15-12 llvm15 > > > got up -b tmp > > > got rb llvm15 > > > got rb -a > > > > The problem is here, after the aborted rebase. Despite "got status" > > and "got diff" showing nothing, the worktree is in reality full of > > changes. > > Yes, this is where the bug is. Essentially this is a bug in the > 'got update' machinery. Let me try to explain: > > The work tree does indeed have no local changes. However, the > base commit ID of some files is left at a wrong value. > > What happens is that the first commit is rebased ok, i.e. changes > are merged into the work tree and then a new commit is created on > a temporary branch which accumulates rebased commits. > When this commit is created the files which are changed by this > commit get a new base commit ID assigned; the ID of the new tip > commit of the temporary branch. > > Then another commit is rebased, and this time there are merge > conflicts, and the rebase operation stops. At this point the work > tree's base commit ID is still pointing at the temporary branch tip > and all state is consistent. > > When we abort the rebase operation the work tree's global base commit > ID gets reset to that of the pre-rebase branch (but keep in mind that > each file has a separate base commit ID in the file index). > Any local changes are reverted, which doesn't affect the files which > received a new base commit ID because these files appear unmodified > in the work tree (still relative to the first rebased commit). > > Then all files are checked out to ensure that files in the work tree > return to the state from before the rebase operation. > The bug is that base commit IDs of affeted files are left as-is. > This base commit ID is of course the wrong one because the files are > now based on the pre-rebase branch again. > > The fix is trivial and I've added test coverage for this issue. > > When the rebase_abort test is run without the fix in place then > it now fails like this: > > $ ./rebase.sh test_rebase_abort > test_rebase_abort --- /tmp/got-test-rebase_abort-p8vJzncg/stdout.expected Wed Apr 12 13:25:15 2023 > +++ /tmp/got-test-rebase_abort-p8vJzncg/stdout Wed Apr 12 13:25:15 2023 > @@ -1 +1,2 @@ > based on commit: 10162cdd439d90b5c3bcd725cae498d10dfd3dd3 > +based on commit: ac3716263a661f70593e8e6ddd192e231612961e > test failed; leaving test data in /tmp/got-test-rebase_abort-p8vJzncg > $ > > And if we make the test skip this base ID commit check by commenting > the relevant lines then the update which the test now runs at the end > is not a no-op. Instead, it updates the file beta. Which matches what > you saw on your llvm branch. > > ok? Nice! Thanks for the explanation, it all makes sense and is good to know. I thought this was going to be a lot more work to fix. ok > ----------------------------------------------- > fix rebase -a not resetting base commit IDs of files changed during rebase > > Update base commit IDs of unmodified files while checking out files. > This avoids spurious merge conflicts when the work tree is later used > for another rebase operation. It also makes 'got update' right after > 'rebase -a' a no-op, as it should be. > > Problem found by naddy@ while rebasing jca's llvm15 branch > > diff cc88020e952af813c1e01b91ab6516969562e972 6a06538c34c30a7ad5174e4f2bf1d669224391ef > commit - cc88020e952af813c1e01b91ab6516969562e972 > commit + 6a06538c34c30a7ad5174e4f2bf1d669224391ef > blob - 9e87414b038cb6aa0ef965f162999f1108c5d82a > blob + 9ac685c7ccc7e64878f575b0e825c1fce2c67f19 > --- lib/worktree.c > +++ lib/worktree.c > @@ -1996,7 +1996,13 @@ update_blob(struct got_worktree *worktree, > if (got_fileindex_entry_has_blob(ie) && > memcmp(ie->blob_sha1, te->id.sha1, > SHA1_DIGEST_LENGTH) == 0) { > - /* Different commit but the same blob. */ > + /* > + * Different commit but the same blob. > + * Update the base commit ID of this file. > + */ > + memcpy(ie->commit_sha1, > + worktree->base_commit_id->sha1, > + sizeof(ie->commit_sha1)); > err = sync_timestamps(worktree->root_fd, > path, status, ie, &sb); > if (err) > blob - 6a4b676aff1b3de6b3384590c038eb5b5045981b > blob + d664d34f3511e6a4a2023e233230f89f2bcec896 > --- regress/cmdline/rebase.sh > +++ regress/cmdline/rebase.sh > @@ -413,11 +413,16 @@ test_rebase_abort() { > local init_commit=`git_show_head $testroot/repo` > > (cd $testroot/repo && git checkout -q -b newbranch) > - echo "modified alpha on branch" > $testroot/repo/alpha > - git_commit $testroot/repo -m "committing to alpha on newbranch" > + echo "modified beta on branch" > $testroot/repo/beta > + git_commit $testroot/repo -m "committing to beta on newbranch" > local orig_commit1=`git_show_head $testroot/repo` > local short_orig_commit1=`trim_obj_id 28 $orig_commit1` > > + echo "modified alpha on branch" > $testroot/repo/alpha > + git_commit $testroot/repo -m "committing to alpha on newbranch" > + local orig_commit2=`git_show_head $testroot/repo` > + local short_orig_commit2=`trim_obj_id 28 $orig_commit2` > + > (cd $testroot/repo && git checkout -q master) > echo "modified alpha on master" > $testroot/repo/alpha > git_commit $testroot/repo -m "committing to alpha on master" > @@ -436,9 +441,17 @@ test_rebase_abort() { > (cd $testroot/wt && got rebase newbranch > $testroot/stdout \ > 2> $testroot/stderr) > > - echo "C alpha" > $testroot/stdout.expected > + new_commit1=$(cd $testroot/wt && got info beta | \ > + grep '^based on commit:' | cut -d' ' -f4) > + local short_new_commit1=`trim_obj_id 28 $new_commit1` > + > + echo "G beta" > $testroot/stdout.expected > + echo -n "$short_orig_commit1 -> $short_new_commit1" \ > + >> $testroot/stdout.expected > + echo ": committing to beta on newbranch" >> $testroot/stdout.expected > + echo "C alpha" >> $testroot/stdout.expected > echo "Files with new merge conflicts: 1" >> $testroot/stdout.expected > - echo -n "$short_orig_commit1 -> merge conflict" \ > + echo -n "$short_orig_commit2 -> merge conflict" \ > >> $testroot/stdout.expected > echo ": committing to alpha on newbranch" >> $testroot/stdout.expected > cmp -s $testroot/stdout.expected $testroot/stdout > @@ -461,12 +474,12 @@ test_rebase_abort() { > > echo '<<<<<<<' > $testroot/content.expected > echo "modified alpha on master" >> $testroot/content.expected > - echo "||||||| 3-way merge base: commit $init_commit" \ > + echo "||||||| 3-way merge base: commit $orig_commit1" \ > >> $testroot/content.expected > echo "alpha" >> $testroot/content.expected > echo "=======" >> $testroot/content.expected > echo "modified alpha on branch" >> $testroot/content.expected > - echo ">>>>>>> merged change: commit $orig_commit1" \ > + echo ">>>>>>> merged change: commit $orig_commit2" \ > >> $testroot/content.expected > cat $testroot/wt/alpha > $testroot/content > cmp -s $testroot/content.expected $testroot/content > @@ -519,13 +532,36 @@ test_rebase_abort() { > > (cd $testroot/wt && got log -l3 -c newbranch \ > | grep ^commit > $testroot/stdout) > - echo "commit $orig_commit1 (newbranch)" > $testroot/stdout.expected > + echo "commit $orig_commit2 (newbranch)" > $testroot/stdout.expected > + echo "commit $orig_commit1" >> $testroot/stdout.expected > echo "commit $init_commit" >> $testroot/stdout.expected > cmp -s $testroot/stdout.expected $testroot/stdout > ret=$? > if [ $ret -ne 0 ]; then > diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > fi > + > + (cd $testroot/wt && got info .| \ > + grep '^based on commit:' | sort | uniq > $testroot/stdout) > + echo "based on commit: $master_commit" > $testroot/stdout.expected > + cmp -s $testroot/stdout.expected $testroot/stdout > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + # A subsequent update should be a no-op. > + (cd $testroot/wt && got update > $testroot/stdout) > + echo 'Already up-to-date' > $testroot/stdout.expected > + cmp -s $testroot/stdout.expected $testroot/stdout > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + fi > test_done "$testroot" "$ret" > } > > blob - 822844175d6069cb0ad59c5bd870b63d886b9990 > blob + c08237c6cd1b14453b8e4f8e39789a3c6c017f1c > --- regress/cmdline/update.sh > +++ regress/cmdline/update.sh > @@ -1899,12 +1899,7 @@ test_update_modified_submodules() { > (cd $testroot/repo && git add repo2) > git_commit $testroot/repo -m "modified submodule link" > > - # This update only records the new base commit. Otherwise it is a > - # no-op change because Got's file index does not track submodules. > - echo -n "Updated to refs/heads/master: " > $testroot/stdout.expected > - git_show_head $testroot/repo >> $testroot/stdout.expected > - echo >> $testroot/stdout.expected > - > + echo "Already up-to-date" > $testroot/stdout.expected > (cd $testroot/wt && got update > $testroot/stdout) > > cmp -s $testroot/stdout.expected $testroot/stdout > -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68