Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

From: Jeff King <peff@peff.net>
Date: Feb 14, 2009, 17:54 UTC
Message-ID: <20090214175420.GA3457@coredump.intra.peff.net>
In-Reply-To: <1234607430-5403-1-git-send-email-jaysoffian@gmail.com>

On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:

Show 8 quoted lines

> In this situation, git remote set-head --auto should not try to guess
> which HEAD the user wants. This patch causes set-head to provide a
> useful error instead:
> 
> $ git remote set-head origin --auto
> error: Multiple remote HEAD branches. Please choose one explicitly with:
>   git remote set-head origin another
>   git remote set-head origin master

Thanks. The patch looks good to me, with two comments and one style nit:

Show 5 quoted lines

> +		else if (states.heads.nr == 1)
> +			printf("  HEAD branch: %s\n",
> +				states.heads.items[0].string);
> +		else
> +			show_list("  HEAD branch%s:", &states.heads, "");

I was happy to see the common case of "we unambiguously determined HEAD" falls back to nicer output (though I admit I did a double-take seeing both show_list and the states.heads.nr check, I see it is because show_list always insists on a newline).

That should help current users with simple setups, but also support unambiguous HEAD reporting in the future (and based on what Daniel said earlier, http should just need a client patch to pass the information up the callstack).

> +		if (opt_a)
> +			printf("%s/HEAD set to %s\n", argv[0], head_name);

This was a surprise based on reading the commit message, but I think it is a sensible enhancement.

Show 10 quoted lines

> +cat > test/expect <<EOF
> +origin/HEAD set to master
> +EOF
> +
> +test_expect_success 'set-head --auto' '
> +	(cd test &&
> +	 git remote set-head --auto origin > output &&
> +	 git symbolic-ref refs/remotes/origin/HEAD &&
> +	test_cmp expect output)
> +'

I had to read this test a few times to convince myself it was right, since you throw away the output of symbolic-ref. I think it makes more sense to just test the post-command state, which is what you actually care about (and then you are also not dependent on the human-readable output of "remote set-head"). I.e.:

cat > test/expect <<EOF refs/remotes/origin/master EOF

test_expect_success 'set-head --auto' '
	(cd test &&
	 git remote set-head --auto origin &&
	 git symbolic-ref refs/remotes/origin/HEAD > output &&
	test_cmp expect output)
'

-Peff