From: Omar Polo <op@omarpolo.com>
Subject: Re: fix packed vs. on-disk reference with got_ref_list()
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 05 Sep 2022 18:40:27 +0200

On 2022/09/05 18:34:05 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> I came across a bug where 'got branch -lt' shows the same branch twice:
> 
> $ got branch -lt
> * zoo: 844bdd2dc1594079c107b206fc594f7df1bfbe6f
>   master: 53094754dfbdac666a73a2bc15fbe51962a1f407
> ~ zoo: 53094754dfbdac666a73a2bc15fbe51962a1f407
> 
> This patch fixes the problem and adds a test for the issue. ok?

ok

> diff e15c42decfa8a80fb91cc1e19b467efc34a8c05d 954554d7bb97f20f338512045d2d27a5ec872a2e
> commit - e15c42decfa8a80fb91cc1e19b467efc34a8c05d
> commit + 954554d7bb97f20f338512045d2d27a5ec872a2e
> blob - 78eb9ebab2c20cab529d7f47d39f8d4829ac8699
> blob + a7ca5b927daf30815b370c13f3ec304c395a1051
> --- lib/reference.c
> +++ lib/reference.c
> @@ -860,6 +860,27 @@ got_reflist_insert(struct got_reflist_entry **newp,
>  	new->ref = ref;
>  	*newp = new;
>  
> +	if (cmp_cb != got_ref_cmp_by_name &&
> +	    (new->ref->flags & GOT_REF_IS_PACKED)) {
> +		/*
> +		 * If we are not sorting elements by name then we must still
> +		 * detect collisions between a packed ref and an on-disk ref
> +		 * using the same name. On-disk refs take precedence and are
> +		 * already present on the list before packed refs get added.
> +		 */
> +		TAILQ_FOREACH(re, refs, entry) {
> +			err = got_ref_cmp_by_name(NULL, &cmp,
> +			    re->ref, new->ref);
> +			if (err)
> +				return err;
> +			if (cmp == 0) {
> +				free(new);
> +				*newp = NULL;
> +				return NULL;
> +			}
> +		}
> +	}
> +
>  	/*
>  	 * We must de-duplicate entries on insert because packed-refs may
>  	 * contain redundant entries. On-disk refs take precedence.
> blob - a025c5170abf06b7f707406ee2d9193c3fa6ff43
> blob + cab7171d5632c0bdf72e14e1d6dda98d3a4ce810
> --- regress/cmdline/branch.sh
> +++ regress/cmdline/branch.sh
> @@ -499,6 +499,44 @@ test_branch_show() {
>  
>  }
>  
> +test_branch_packed_ref_collision() {
> +	local testroot=`test_init branch_packed_ref_collision`
> +	local commit_id=`git_show_head $testroot/repo`
> +
> +	got br -r $testroot/repo zoo > $testroot/stdout
> +	got co -b zoo $testroot/repo $testroot/wt > /dev/null
> +	echo "modified alpha" > $testroot/wt/alpha
> +
> +	# sleep in order to ensure that a significant fraction of time
> +	# passes between commits; required for got branch -t option below
> +	sleep 1
> +
> +	(cd $testroot/wt && got commit -m "modified alpha" >/dev/null)
> +	local commit_id2=`git_show_branch_head $testroot/repo zoo`
> +
> +	# Fabricate a packed reference which points to an older commit
> +	# and collides with the existing on-disk reference
> +	echo '# pack-refs with: peeled fully-peeled sorted' > \
> +		$testroot/repo/.git/packed-refs
> +	echo "$commit_id refs/heads/zoo" >> $testroot/repo/.git/packed-refs
> +
> +	# Bug: This command used to show both packed and on-disk
> +	# variants of ref/heads/zoo:
> +	(cd $testroot/wt && got br -lt > $testroot/stdout)
> +
> +	echo "* zoo: $commit_id2" > $testroot/stdout.expected
> +	echo "  master: $commit_id" >> $testroot/stdout.expected
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_branch_create
>  run_test test_branch_list
> @@ -506,3 +544,4 @@ run_test test_branch_delete
>  run_test test_branch_delete_current_branch
>  run_test test_branch_delete_packed
>  run_test test_branch_show
> +run_test test_branch_packed_ref_collision