From: Sebastien Marie <semarie@online.fr>
Subject: Re: fix bug where outdated files cannot be updated
To: gameoftrees@openbsd.org
Date: Sat, 26 Jun 2021 10:59:31 +0200

On Wed, Jun 23, 2021 at 03:10:31PM +0200, Stefan Sperling wrote:
> This patch fixes a bug where outdated files cannot be updated.
> See the regression test added in this patch for a reproduction recipe.
> (I think semarie saw a similar issue recently; Perhaps it was this bug?)
> 
> The problem happens after 'got update' skips a conflicted file,
> which looks like this:
> 
>   $ got update
>   #  beta
>   Updated to commit abcde12345...
>   Files not updated because of existing merge conflicts: 1
>  
> When this occurs, the file index entry for 'beta' should remain unchanged.
> However, the base commit ID for this entry is bumped along with any other
> updated entries. This is wrong since the file was not actually updated.
> The file index is now slightly corrupt: The base blob recorded for 'beta'
> doesn't actually exist in the base commit recorded for 'beta', and the
> recorded base commit is incorrect.
> 
> A subsequent 'got update' will see the incorrect base commit ID on
> beta's entry and consider the file as already up-to-date.
> 
> We can fix this issue by simply leaving the entry's base commit ID as-is.
> 
> ok?

I only lightly reviewed the diff, but logic makes sense and it
corrects real bug, ok semarie@

> diff 867b03e66abeaeadc44ef9978e89d71504e15cba 784f2e14750a58132ab6e650e20d483ab120cc48
> blob - d1e3839c06a7f70e71410592024f426053df2ced
> blob + aec06af5925a0cd63ffc6b150466bad036d5abd8
> --- lib/fileindex.c
> +++ lib/fileindex.c
> @@ -46,6 +46,7 @@
>  #define GOT_FILEIDX_F_NO_COMMIT		0x00040000
>  #define GOT_FILEIDX_F_NO_FILE_ON_DISK	0x00080000
>  #define GOT_FILEIDX_F_REMOVE_ON_FLUSH	0x00100000
> +#define GOT_FILEIDX_F_SKIPPED		0x00200000
>  
>  struct got_fileindex {
>  	struct got_fileindex_tree entries;
> @@ -146,6 +147,12 @@ got_fileindex_entry_mark_deleted_from_disk(struct got_
>  	ie->flags |= GOT_FILEIDX_F_NO_FILE_ON_DISK;
>  }
>  
> +void
> +got_fileindex_entry_mark_skipped(struct got_fileindex_entry *ie)
> +{
> +	ie->flags |= GOT_FILEIDX_F_SKIPPED;
> +}
> +
>  const struct got_error *
>  got_fileindex_entry_alloc(struct got_fileindex_entry **ie,
>      const char *relpath)
> @@ -245,6 +252,12 @@ got_fileindex_entry_has_file_on_disk(struct got_filein
>  	return (ie->flags & GOT_FILEIDX_F_NO_FILE_ON_DISK) == 0;
>  }
>  
> +int
> +got_fileindex_entry_was_skipped(struct got_fileindex_entry *ie)
> +{
> +	return (ie->flags & GOT_FILEIDX_F_SKIPPED) != 0;
> +}
> +
>  static const struct got_error *
>  add_entry(struct got_fileindex *fileindex, struct got_fileindex_entry *ie)
>  {
> @@ -497,6 +510,7 @@ got_fileindex_write(struct got_fileindex *fileindex, F
>  
>  	RB_FOREACH_SAFE(ie, got_fileindex_tree, &fileindex->entries, tmp) {
>  		ie->flags &= ~GOT_FILEIDX_F_NOT_FLUSHED;
> +		ie->flags &= ~GOT_FILEIDX_F_SKIPPED;
>  		if (ie->flags & GOT_FILEIDX_F_REMOVE_ON_FLUSH) {
>  			RB_REMOVE(got_fileindex_tree, &fileindex->entries, ie);
>  			got_fileindex_entry_free(ie);
> blob - 537374d719e84293f1b449465ab63e092b44a20a
> blob + d03058e83c5f5c0a69baf66795455eb3ab8b7af7
> --- lib/got_lib_fileindex.h
> +++ lib/got_lib_fileindex.h
> @@ -109,6 +109,7 @@ mode_t got_fileindex_perms_to_st(struct got_fileindex_
>  
>  const struct got_error *got_fileindex_entry_update(struct got_fileindex_entry *,
>      int, const char *, uint8_t *, uint8_t *, int);
> +void got_fileindex_entry_mark_skipped(struct got_fileindex_entry *);
>  const struct got_error *got_fileindex_entry_alloc(struct got_fileindex_entry **,
>      const char *);
>  void got_fileindex_entry_free(struct got_fileindex_entry *);
> @@ -164,6 +165,7 @@ const struct got_error *got_fileindex_diff_dir(struct 
>  int got_fileindex_entry_has_blob(struct got_fileindex_entry *);
>  int got_fileindex_entry_has_commit(struct got_fileindex_entry *);
>  int got_fileindex_entry_has_file_on_disk(struct got_fileindex_entry *);
> +int got_fileindex_entry_was_skipped(struct got_fileindex_entry *);
>  uint32_t got_fileindex_entry_stage_get(const struct got_fileindex_entry *);
>  void got_fileindex_entry_stage_set(struct got_fileindex_entry *ie, uint32_t);
>  int got_fileindex_entry_filetype_get(struct got_fileindex_entry *);
> blob - c6a9ee611f39f7106cbf6b33215f75f9f7a5c715
> blob + 385e8a46c03c7be4c81ce76af599d43f216ed948
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -1951,6 +1951,8 @@ update_blob(struct got_worktree *worktree,
>  		goto done;
>  	}
>  	if (status == GOT_STATUS_CONFLICT) {
> +		if (ie)
> +			got_fileindex_entry_mark_skipped(ie);
>  		err = (*progress_cb)(progress_arg, GOT_STATUS_CANNOT_UPDATE,
>  		    path);
>  		goto done;
> @@ -2474,6 +2476,9 @@ bump_base_commit_id(void *arg, struct got_fileindex_en
>  	} else if (!got_path_is_child(ie->path, a->path, a->path_len))
>  		return NULL;
>  
> +	if (got_fileindex_entry_was_skipped(ie))
> +		return NULL;
> +
>  	if (memcmp(ie->commit_sha1, a->base_commit_id->sha1,
>  	    SHA1_DIGEST_LENGTH) == 0)
>  		return NULL;
> blob - fa0398814a0312e4d8d4d343c490f6b7b4129804
> blob + 18691664018f8a6afd325e890af93afbcdd51b2b
> --- regress/cmdline/update.sh
> +++ regress/cmdline/update.sh
> @@ -2391,7 +2391,188 @@ test_update_single_file() {
>  	return 0
>  }
>  
> +test_update_file_skipped_due_to_conflict() {
> +	local testroot=`test_init update_file_skipped_due_to_conflict`
> +	local commit_id0=`git_show_head $testroot/repo`
> +	blob_id0=`get_blob_id $testroot/repo "" beta`
>  
> +	echo "changed beta" > $testroot/repo/beta
> +	git_commit $testroot/repo -m "changed beta"
> +	local commit_id1=`git_show_head $testroot/repo`
> +	blob_id1=`get_blob_id $testroot/repo "" beta`
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	blob_id=`(cd $testroot/wt && got info beta | grep 'blob:' | \
> +		cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$blob_id" != "$blob_id1" ]; then
> +		echo "file beta has the wrong base blob ID" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	commit_id=`(cd $testroot/wt && got info beta | \
> +		grep 'based on commit:' | cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$commit_id" != "$commit_id1" ]; then
> +		echo "file beta has the wrong base commit ID" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo "modified beta" > $testroot/wt/beta
> +
> +	(cd $testroot/wt && got update -c $commit_id0 > $testroot/stdout)
> +
> +	echo "C  beta" > $testroot/stdout.expected
> +	echo "Updated to commit $commit_id0" >> $testroot/stdout.expected
> +	echo "Files with new merge conflicts: 1" >> $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
> +
> +	echo "<<<<<<< merged change: commit $commit_id0" \
> +		> $testroot/content.expected
> +	echo "beta" >> $testroot/content.expected
> +	echo "||||||| 3-way merge base: commit $commit_id1" \
> +		>> $testroot/content.expected
> +	echo "changed beta" >> $testroot/content.expected
> +	echo "=======" >> $testroot/content.expected
> +	echo "modified beta" >> $testroot/content.expected
> +	echo ">>>>>>>" >> $testroot/content.expected
> +
> +	cat $testroot/wt/beta > $testroot/content
> +
> +	cmp -s $testroot/content.expected $testroot/content
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/content.expected $testroot/content
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	blob_id=`(cd $testroot/wt && got info beta | grep 'blob:' | \
> +		cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$blob_id" != "$blob_id0" ]; then
> +		echo "file beta has the wrong base blob ID" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	commit_id=`(cd $testroot/wt && got info beta | \
> +		grep 'based on commit:' | cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$commit_id" != "$commit_id0" ]; then
> +		echo "file beta has the wrong base commit ID" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# update to the latest commit again; this skips beta
> +	(cd $testroot/wt && got update > $testroot/stdout)
> +	echo "#  beta" > $testroot/stdout.expected
> +	echo "Updated to commit $commit_id1" >> $testroot/stdout.expected
> +	echo "Files not updated because of existing merge conflicts: 1" \
> +		>> $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
> +
> +	# blob ID of beta should not have changed
> +	blob_id=`(cd $testroot/wt && got info beta | grep 'blob:' | \
> +		cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$blob_id" != "$blob_id0" ]; then
> +		echo "file beta has the wrong base blob ID" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# commit ID of beta should not have changed; There was a bug
> +	# here where the commit ID had been changed even though the
> +	# file was not updated.
> +	commit_id=`(cd $testroot/wt && got info beta | \
> +		grep 'based on commit:' | cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$commit_id" != "$commit_id0" ]; then
> +		echo "file beta has the wrong base commit ID: $commit_id" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# beta is still conflicted and based on commit 0
> +	echo 'C  beta' > $testroot/stdout.expected
> +	(cd $testroot/wt && got status > $testroot/stdout)
> +	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
> +
> +	# resolve the conflict via revert
> +	(cd $testroot/wt && got revert beta >/dev/null)
> +
> +	# beta now matches its base blob which is still from commit 0
> +	echo "beta" > $testroot/content.expected
> +	cat $testroot/wt/beta > $testroot/content
> +	cmp -s $testroot/content.expected $testroot/content
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/content.expected $testroot/content
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# updating to the latest commit should now update beta
> +	(cd $testroot/wt && got update > $testroot/stdout)
> +	echo "U  beta" > $testroot/stdout.expected
> +	echo "Updated to commit $commit_id1" >> $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
> +
> +	blob_id=`(cd $testroot/wt && got info beta | grep 'blob:' | \
> +		cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$blob_id" != "$blob_id1" ]; then
> +		echo "file beta has the wrong base blob ID" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	commit_id=`(cd $testroot/wt && got info beta | \
> +		grep 'based on commit:' | cut -d ':' -f 2 | tr -d ' ')`
> +	if [ "$commit_id" != "$commit_id1" ]; then
> +		echo "file beta has the wrong base commit ID: $commit_id" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo "changed beta" > $testroot/content.expected
> +	cat $testroot/wt/beta > $testroot/content
> +	cmp -s $testroot/content.expected $testroot/content
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/content.expected $testroot/content
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
> +
>  test_parseargs "$@"
>  run_test test_update_basic
>  run_test test_update_adds_file
> @@ -2432,3 +2613,4 @@ run_test test_update_adds_symlink
>  run_test test_update_deletes_symlink
>  run_test test_update_symlink_conflicts
>  run_test test_update_single_file
> +run_test test_update_file_skipped_due_to_conflict
> 
> 

-- 
Sebastien Marie