From: Stefan Sperling <stsp@stsp.name>
Subject: Re: always fetch remote HEAD except when -b is used
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Tue, 14 Feb 2023 13:42:51 +0100

On Tue, Feb 14, 2023 at 06:07:56PM +1100, Mark Jamsek wrote:
> As discussed with stsp on irc, this tweaks 'got fetch' again so that the
> branch resolved via the remote's HEAD ref is not just a fallback--it is
> always fetched except in the 'got fetch -b branch' case.
> 
> This necessitated passing the bflag to got_fetch_pack from cmd_clone(),
> too, so that we don't fetch HEAD when 'got clone -b branch' is used.
> 
> The change is simple, but there's a bit of churn in regress. Given this
> behaviour is somewhat simpler than the previous where we'd only fetch
> HEAD as a fallback, I think some of the recent tests can be dropped as
> they're essentially testing the same thing, but I've kept them for now.
> 
> diffstat /home/mark/src/got
>  M  got/got.1                                |   2+   5-
>  M  got/got.c                                |   3+   2-
>  M  libexec/got-fetch-pack/got-fetch-pack.c  |  11+  21-
>  M  regress/cmdline/fetch.sh                 |  56+  22-
> 
> 4 files changed, 72 insertions(+), 50 deletions(-)

ok stsp@

> diff /home/mark/src/got
> commit - a4515c6608c1b685bfa9187b2517b44773a11068
> path + /home/mark/src/got
> blob - e074f7da3728404dc7163c7f239e256bf44a522c
> file + got/got.1
> --- got/got.1
> +++ got/got.1
> @@ -348,16 +348,13 @@ for the
>  By default, any branches configured in
>  .Xr got.conf 5
>  for the
> -.Ar remote-repository
> +.Ar remote-repository ,
> +and the branch resolved via the remote repository's HEAD reference,
>  will be fetched.
>  If
>  .Cm got fetch
>  is invoked in a work tree then this work tree's current branch will be
>  fetched, too, provided it is present on the server.
> -If no branches to fetch can be found in
> -.Xr got.conf 5
> -or via a work tree, or said branches are not found on the server, a branch
> -resolved via the remote repository's HEAD reference will be fetched.
>  This default behaviour can be overridden with the
>  .Fl a
>  and
> blob - 1ef76ea95b69bf7e45b91f9053a232d241269e7d
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -1544,7 +1544,7 @@ cmd_clone(int argc, char *argv[])
>  	struct got_fetch_progress_arg fpa;
>  	char *git_url = NULL;
>  	int verbosity = 0, fetch_all_branches = 0, mirror_references = 0;
> -	int list_refs_only = 0;
> +	int bflag = 0, list_refs_only = 0;
>  	int *pack_fds = NULL;
>  
>  	TAILQ_INIT(&refs);
> @@ -1562,6 +1562,7 @@ cmd_clone(int argc, char *argv[])
>  			    optarg, NULL);
>  			if (error)
>  				return error;
> +			bflag = 1;
>  			break;
>  		case 'l':
>  			list_refs_only = 1;
> @@ -1717,7 +1718,7 @@ cmd_clone(int argc, char *argv[])
>  	error = got_fetch_pack(&pack_hash, &refs, &symrefs,
>  	    GOT_FETCH_DEFAULT_REMOTE_NAME, mirror_references,
>  	    fetch_all_branches, &wanted_branches, &wanted_refs,
> -	    list_refs_only, verbosity, fetchfd, repo, NULL, 0,
> +	    list_refs_only, verbosity, fetchfd, repo, NULL, bflag,
>  	    fetch_progress, &fpa);
>  	if (error)
>  		goto done;
> blob - 2a22df95ff37a15b31f41c7c01f263af824ddc15
> file + libexec/got-fetch-pack/got-fetch-pack.c
> --- libexec/got-fetch-pack/got-fetch-pack.c
> +++ libexec/got-fetch-pack/got-fetch-pack.c
> @@ -342,13 +342,12 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
>  	int is_firstpkt = 1, nref = 0, refsz = 16;
>  	int i, n, nwant = 0, nhave = 0, acked = 0;
>  	off_t packsz = 0, last_reported_packsz = 0;
> -	char *id_str = NULL, *default_id_str = NULL, *refname = NULL;
> +	char *id_str = NULL, *refname = NULL;
>  	char *server_capabilities = NULL, *my_capabilities = NULL;
>  	const char *default_branch = NULL;
>  	struct got_pathlist_head symrefs;
>  	struct got_pathlist_entry *pe;
>  	int sent_my_capabilites = 0, have_sidebands = 0;
> -	int found_branch = 0;
>  	SHA1_CTX sha1_ctx;
>  	uint8_t sha1_buf[SHA1_DIGEST_LENGTH];
>  	size_t sha1_buf_len = 0;
> @@ -439,13 +438,17 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
>  			}
>  			continue;
>  		}
> -		if (default_branch && default_id_str == NULL &&
> -		    strcmp(refname, default_branch) == 0) {
> -			default_id_str = strdup(id_str);
> -			if (default_id_str == NULL) {
> -				err = got_error_from_errno("strdup");
> +
> +		/* always fetch remote HEAD unless -b was specified */
> +		if (!no_head && default_branch &&
> +		    strcmp(refname, default_branch) == 0 &&
> +		    strncmp(default_branch, "refs/heads/", 11) == 0) {
> +			err = fetch_ref(ibuf, have_refs, &have[nref],
> +			    &want[nref], default_branch, id_str);
> +			if (err)
>  				goto done;
> -			}
> +			nref++;
> +			continue;
>  		}
>  
>  		if (list_refs_only || strncmp(refname, "refs/tags/", 10) == 0) {
> @@ -461,7 +464,6 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
>  				if (err)
>  					goto done;
>  				nref++;
> -				found_branch = 1;
>  				continue;
>  			}
>  			TAILQ_FOREACH(pe, wanted_branches, entry) {
> @@ -475,7 +477,6 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
>  				if (err)
>  					goto done;
>  				nref++;
> -				found_branch = 1;
>  			} else if (chattygot) {
>  				fprintf(stderr, "%s: ignoring %s\n",
>  				    getprogname(), refname);
> @@ -501,16 +502,6 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
>  	if (list_refs_only)
>  		goto done;
>  
> -	if (!found_branch && !no_head && default_branch && default_id_str &&
> -	    strncmp(default_branch, "refs/heads/", 11) == 0) {
> -		err = fetch_ref(ibuf, have_refs, &have[nref],
> -		    &want[nref], default_branch, default_id_str);
> -		if (err)
> -			goto done;
> -		nref++;
> -		found_branch = 1;
> -	}
> -
>  	/* Abort if we haven't found anything to fetch. */
>  	if (nref == 0) {
>  		struct got_pathlist_entry *pe;
> @@ -811,7 +802,6 @@ done:
>  	free(have);
>  	free(want);
>  	free(id_str);
> -	free(default_id_str);
>  	free(refname);
>  	free(server_capabilities);
>  	return err;
> blob - b5089c370c1eb445b14ae243faad7fb9fefbb907
> file + regress/cmdline/fetch.sh
> --- regress/cmdline/fetch.sh
> +++ regress/cmdline/fetch.sh
> @@ -211,9 +211,12 @@ test_fetch_branch() {
>  	got ref -l -r $testroot/repo-clone > $testroot/stdout
>  
>  	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/foo: $commit_id3" >> $testroot/stdout.expected
>  	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> -	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
> +	echo "refs/remotes/origin/HEAD: refs/remotes/origin/foo" \
>  		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/foo: $commit_id3" \
> +		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/master: $commit_id2" \
>  		>> $testroot/stdout.expected
>  	# refs/hoo/boo/zoo is missing because it is outside of refs/heads
> @@ -1113,9 +1116,12 @@ test_fetch_update_headref() {
>  	got ref -l -r $testroot/repo-clone > $testroot/stdout
>  
>  	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/foo: $commit_id" >> $testroot/stdout.expected
>  	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> -	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
> +	echo "refs/remotes/origin/HEAD: refs/remotes/origin/foo" \
>  		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/foo: $commit_id" \
> +		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/master: $commit_id" \
>  		>> $testroot/stdout.expected
>  
> @@ -1426,7 +1432,7 @@ test_fetch_honor_wt_conf_bflag() {
>  }
>  
>  test_fetch_honor_wt_conf_bflag() {
> -	local testroot=`test_init fetch_branch`
> +	local testroot=`test_init fetch_honor_wt_conf_bflag`
>  	local testurl=ssh://127.0.0.1/$testroot
>  
>  	# server will have 'boo', 'hoo', and 'master'
> @@ -1496,11 +1502,15 @@ test_fetch_honor_wt_conf_bflag() {
>  		return 1
>  	fi
>  
> +	(cd $testroot/repo && git checkout -q master)
> +	echo "alpha master" > $testroot/repo/alpha
> +	git_commit $testroot/repo -m "alpha master"
> +	local commit_id_alpha_master=`git_show_head $testroot/repo`
> +
>  	(cd $testroot/repo && git checkout -q boo)
> -	# from repo: fetch got.conf branch not repo HEAD
> -	# boo is the default HEAD in $testroot/repo, which is not up-to-date
> -	# on the clone, but we fetch got.conf "master" which is up-to-date
> -	got fetch -r $testroot/repo-clone > $testroot/stdout
> +
> +	# from repo: no -b used, fetch got.conf "master" and repo HEAD "boo"
> +	got fetch -q -r $testroot/repo-clone > $testroot/stdout
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
>  		echo "got fetch command failed unexpectedly" >&2
> @@ -1508,10 +1518,20 @@ test_fetch_honor_wt_conf_bflag() {
>  		return 1
>  	fi
>  
> -	echo "Connecting to \"origin\" ssh://127.0.0.1$testroot/repo" \
> -	    > $testroot/stdout.expected
> -	echo "Already up-to-date" >> $testroot/stdout.expected
> +	got ref -l -r $testroot/repo-clone > $testroot/stdout
>  
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/bar: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/heads/boo: $commit_id2" >> $testroot/stdout.expected
> +	echo "refs/heads/foo: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/remotes/origin/HEAD: refs/remotes/origin/boo" \
> +		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/boo: $commit_id2" \
> +		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/master: $commit_id_alpha_master" \
> +		>> $testroot/stdout.expected
> +
>  	cmp -s $testroot/stdout $testroot/stdout.expected
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
> @@ -1543,14 +1563,17 @@ test_fetch_honor_wt_conf_bflag() {
>  
>  	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
>  	echo "refs/heads/bar: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/heads/boo: $commit_id2" >> $testroot/stdout.expected
>  	echo "refs/heads/foo: $commit_id" >> $testroot/stdout.expected
>  	echo "refs/heads/hoo: $commit_id3" >> $testroot/stdout.expected
>  	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> -	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
> +	echo "refs/remotes/origin/HEAD: refs/remotes/origin/boo" \
>  		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/boo: $commit_id2" \
> +		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/hoo: $commit_id3" \
>  		>> $testroot/stdout.expected
> -	echo "refs/remotes/origin/master: $commit_id" \
> +	echo "refs/remotes/origin/master: $commit_id_alpha_master" \
>  		>> $testroot/stdout.expected
>  
>  	cmp -s $testroot/stdout $testroot/stdout.expected
> @@ -1589,11 +1612,15 @@ test_fetch_honor_wt_conf_bflag() {
>  		return 1
>  	fi
>  
> -	# from repo: fetch got.conf branch which doesn't exist, so fallback
> -	# to repo HEAD "boo"
> +	# from repo: fetch got.conf branch "foo", which
> +	# doesn't exist on the server, and repo HEAD "boo"
>  	# change default branch in got.conf from "master" to "foo"
>  	sed -i "s/master/foo/" $testroot/repo-clone/got.conf
>  
> +	echo "modified alpha again on boo" > $testroot/repo/alpha
> +	git_commit $testroot/repo -m "modified alpha again on boo"
> +	commit_id_alpha=`git_show_head $testroot/repo`
> +
>  	got fetch -q -r $testroot/repo-clone > $testroot/stdout
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
> @@ -1622,11 +1649,11 @@ test_fetch_honor_wt_conf_bflag() {
>  	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
>  	echo "refs/remotes/origin/HEAD: refs/remotes/origin/boo" \
>  		>> $testroot/stdout.expected
> -	echo "refs/remotes/origin/boo: $commit_id2" \
> +	echo "refs/remotes/origin/boo: $commit_id_alpha" \
>  		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/hoo: $commit_id3" \
>  		>> $testroot/stdout.expected
> -	echo "refs/remotes/origin/master: $commit_id" \
> +	echo "refs/remotes/origin/master: $commit_id_alpha_master" \
>  		>> $testroot/stdout.expected
>  
>  	cmp -s $testroot/stdout $testroot/stdout.expected
> @@ -1637,13 +1664,16 @@ test_fetch_honor_wt_conf_bflag() {
>  		return 1
>  	fi
>  
> -	# from wt: fetch got.conf "foo", which doesn't exist on the server,
> -	# and implicit wt branch "boo", not repo HEAD "master"
> +	# from wt: fetch got.conf "foo", which doesn't exist on the
> +	# server, and implicit wt branch "boo", and repo HEAD "master"
>  	echo "modified delta on boo" > $testroot/repo/gamma/delta
>  	git_commit $testroot/repo -m "modified delta"
>  	local commit_id4=`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 "modified zeta on master"
> +	local commit_id_zeta=`git_show_head $testroot/repo`
>  
>  	got checkout -b boo $testroot/repo-clone $testroot/wt > /dev/null
>  	(cd $testroot/wt && got fetch -q > $testroot/stdout)
> @@ -1677,7 +1707,7 @@ test_fetch_honor_wt_conf_bflag() {
>  		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/hoo: $commit_id3" \
>  		>> $testroot/stdout.expected
> -	echo "refs/remotes/origin/master: $commit_id" \
> +	echo "refs/remotes/origin/master: $commit_id_zeta" \
>  		>> $testroot/stdout.expected
>  
>  	cmp -s $testroot/stdout $testroot/stdout.expected
> @@ -1688,7 +1718,7 @@ test_fetch_honor_wt_conf_bflag() {
>  		return 1
>  	fi
>  
> -	# from wt: fetch got.conf "master" and wt "boo", not repo HEAD "hoo"
> +	# from wt: fetch got.conf "master" and wt "boo", and repo HEAD "hoo"
>  	# change default branch in got.conf from "foo" to "master"
>  	sed -i "s/foo/master/" $testroot/repo-clone/got.conf
>  	echo "modified delta on master" > $testroot/repo/gamma/delta
> @@ -1731,7 +1761,7 @@ test_fetch_honor_wt_conf_bflag() {
>  		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/boo: $commit_id6" \
>  		>> $testroot/stdout.expected
> -	echo "refs/remotes/origin/hoo: $commit_id3" \
> +	echo "refs/remotes/origin/hoo: $commit_id7" \
>  		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/master: $commit_id5" \
>  		>> $testroot/stdout.expected
> @@ -1746,6 +1776,10 @@ test_fetch_honor_wt_conf_bflag() {
>  
>  	# from wt: fetch -b hoo not got.conf "master" or wt "boo" or
>  	# repo HEAD "boo"
> +	echo "hoo delta!" > $testroot/repo/gamma/delta
> +	git_commit $testroot/repo -m "hoo delta!"
> +	local commit_id_delta=`git_show_head $testroot/repo`
> +
>  	(cd $testroot/repo && git checkout -q boo)
>  	echo "modified alpha again on boo" > $testroot/repo/alpha
>  	git_commit $testroot/repo -m "modified alpha again on boo"
> @@ -1777,7 +1811,7 @@ test_fetch_honor_wt_conf_bflag() {
>  		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/boo: $commit_id6" \
>  		>> $testroot/stdout.expected
> -	echo "refs/remotes/origin/hoo: $commit_id7" \
> +	echo "refs/remotes/origin/hoo: $commit_id_delta" \
>  		>> $testroot/stdout.expected
>  	echo "refs/remotes/origin/master: $commit_id5" \
>  		>> $testroot/stdout.expected
> 
> -- 
> Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
> GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68
>