"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix file index corruption after rebase
To:
gameoftrees@openbsd.org
Date:
Fri, 24 Apr 2020 10:59:50 +0200

Download raw body.

Thread
Tracey has discovered that 'got rebase' can corrupt the file index.
This happens if a file gets deleted, added, and deleted again in
the commit history being rebased.

This adds a test for the bug and a fix.

-----------------------------------------------
commit f8c22fb8662a04d5bddd793b39ab74d560f3ba7e (fileindex)
from: Stefan Sperling <stsp@stsp.name>
date: Fri Apr 24 08:22:46 2020 UTC
 
 add a test for rebase file index corruption problem fix in previous commit
 
diff dd0dd14a87d9a70da54a219cfab181dfff5f3e4e 0eb1956e309d9b69ef92dc44770cbbe11d637647
blob - 34f088073605cb3bb15c99c42ee9655c75a994da
blob + ff453b223f4e883bf285ec5a0e4e0c465ba3dd3c
--- regress/cmdline/common.sh
+++ regress/cmdline/common.sh
@@ -84,7 +84,8 @@ function git_show_tagger_time
 function git_show_parent_commit
 {
 	local repo="$1"
-	(cd $repo && git show --no-patch --pretty='format:%P')
+	local commit="$2"
+	(cd $repo && git show --no-patch --pretty='format:%P' $commit)
 }
 
 function git_show_tree
blob - e6f7a88e48abfbf74fffea06c088a955d610baf1
blob + e54de3f2dd29851f2675c480f96c1061d6e4df17
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -1163,6 +1163,113 @@ function test_rebase_delete_missing_file {
 	test_done "$testroot" "$ret"
 }
 
+function test_rebase_rm_add_rm_file {
+	local testroot=`test_init rebase_rm_add_rm_file`
+
+	(cd $testroot/repo && git checkout -q -b newbranch)
+	(cd $testroot/repo && git rm -q beta)
+	git_commit $testroot/repo -m "removing beta from newbranch"
+	local orig_commit1=`git_show_head $testroot/repo`
+
+	echo 'restored beta' > $testroot/repo/beta
+	(cd $testroot/repo && git add beta)
+	git_commit $testroot/repo -m "restoring beta on newbranch"
+	local orig_commit2=`git_show_head $testroot/repo`
+
+	(cd $testroot/repo && git rm -q beta)
+	git_commit $testroot/repo -m "removing beta from newbranch again"
+	local orig_commit3=`git_show_head $testroot/repo`
+
+	(cd $testroot/repo && git checkout -q master)
+	echo "modified zeta on master" > $testroot/repo/epsilon/zeta
+	git_commit $testroot/repo -m "committing to zeta on master"
+	local master_commit=`git_show_head $testroot/repo`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got rebase newbranch > $testroot/stdout)
+
+	# this would error out with 'got: file index is corrupt'
+	(cd $testroot/wt && got status > /dev/null)
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "got status command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/repo && git checkout -q newbranch)
+	local new_commit3=`git_show_head $testroot/repo`
+	local new_commit2=`git_show_parent_commit $testroot/repo`
+	local new_commit1=`git_show_parent_commit $testroot/repo $new_commit2`
+
+	(cd $testroot/repo && git checkout -q newbranch)
+
+	local short_orig_commit1=`trim_obj_id 28 $orig_commit1`
+	local short_orig_commit2=`trim_obj_id 28 $orig_commit2`
+	local short_orig_commit3=`trim_obj_id 28 $orig_commit3`
+	local short_new_commit1=`trim_obj_id 28 $new_commit1`
+	local short_new_commit2=`trim_obj_id 28 $new_commit2`
+	local short_new_commit3=`trim_obj_id 28 $new_commit3`
+
+	echo "D  beta" > $testroot/stdout.expected
+	echo -n "$short_orig_commit1 -> $short_new_commit1" \
+		>> $testroot/stdout.expected
+	echo ": removing beta from newbranch" >> $testroot/stdout.expected
+	echo "A  beta" >> $testroot/stdout.expected
+	echo -n "$short_orig_commit2 -> $short_new_commit2" \
+		>> $testroot/stdout.expected
+	echo ": restoring beta on newbranch" >> $testroot/stdout.expected
+	echo "D  beta" >> $testroot/stdout.expected
+	echo -n "$short_orig_commit3 -> $short_new_commit3" \
+		>> $testroot/stdout.expected
+	echo ": removing beta from newbranch again" >> $testroot/stdout.expected
+	echo "Switching work tree to refs/heads/newbranch" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got status > $testroot/stdout)
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "got status command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got log -l4 | grep ^commit > $testroot/stdout)
+	echo "commit $new_commit3 (newbranch)" > $testroot/stdout.expected
+	echo "commit $new_commit2" >> $testroot/stdout.expected
+	echo "commit $new_commit1" >> $testroot/stdout.expected
+	echo "commit $master_commit (master)" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
 run_test test_rebase_basic
 run_test test_rebase_ancestry_check
 run_test test_rebase_continue
@@ -1176,3 +1283,4 @@ run_test test_rebase_forward
 run_test test_rebase_out_of_date
 run_test test_rebase_trims_empty_dir
 run_test test_rebase_delete_missing_file
+run_test test_rebase_rm_add_rm_file

-----------------------------------------------
commit c965e9bd3a1c5efec760787528b3756f7ab7afc7
from: Stefan Sperling <stsp@stsp.name>
date: Fri Apr 24 08:22:46 2020 UTC
 
 remove file index entries from RB tree upon flush to disk
 
 Fixes a file index corruption problem with 'got rebase' found by tracey.
 
diff 07912ed061a0a4946a026a5ca01322fd20dd2dd5 dd0dd14a87d9a70da54a219cfab181dfff5f3e4e
blob - 61af7dd7264c91833efaded48290d683fc4500cc
blob + 460afc6a44656e2606c3799dcc340b57e57cf037
--- lib/fileindex.c
+++ lib/fileindex.c
@@ -222,8 +222,8 @@ got_fileindex_entry_remove(struct got_fileindex *filei
 	/*
 	 * Removing an entry from the RB tree immediately breaks
 	 * in-progress iterations over file index entries.
-	 * So flag this entry for removal and skip it once the index
-	 * is written out to disk, and pretend this entry no longer
+	 * So flag this entry for removal and remove it once the index
+	 * is written out to disk. Meanwhile, pretend this entry no longer
 	 * exists if we get queried for it again before then.
 	 */
 	ie->flags |= GOT_FILEIDX_F_REMOVE_ON_FLUSH;
@@ -423,7 +423,7 @@ got_fileindex_write(struct got_fileindex *fileindex, F
 	SHA1_CTX ctx;
 	uint8_t sha1[SHA1_DIGEST_LENGTH];
 	size_t n;
-	struct got_fileindex_entry *ie;
+	struct got_fileindex_entry *ie, *tmp;
 
 	SHA1Init(&ctx);
 
@@ -444,10 +444,13 @@ got_fileindex_write(struct got_fileindex *fileindex, F
 	if (n != sizeof(hdr.nentries))
 		return got_ferror(outfile, GOT_ERR_IO);
 
-	RB_FOREACH(ie, got_fileindex_tree, &fileindex->entries) {
+	RB_FOREACH_SAFE(ie, got_fileindex_tree, &fileindex->entries, tmp) {
 		ie->flags &= ~GOT_FILEIDX_F_NOT_FLUSHED;
-		if (ie->flags & GOT_FILEIDX_F_REMOVE_ON_FLUSH)
+		if (ie->flags & GOT_FILEIDX_F_REMOVE_ON_FLUSH) {
+			RB_REMOVE(got_fileindex_tree, &fileindex->entries, ie);
+			got_fileindex_entry_free(ie);
 			continue;
+		}
 		err = write_fileindex_entry(&ctx, ie, outfile);
 		if (err)
 			return err;