From: Jeff King <peff@peff.net>
Date: Thu, 23 Aug 2012 08:54:10 GMT
Subject: Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads
Message-ID: <20120823085409.GD6963@sigill.intra.peff.net>
In-Reply-To: <1345709442-16046-7-git-send-email-mhagger@alum.mit.edu>

On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhagger@alum.mit.edu wrote:

> From: Michael Haggerty <mhagger@alum.mit.edu>
> 
> fetch_pack() remotes duplicates from the list (nr_heads, heads),
> thereby shrinking the list.  But previously, the caller was not
> informed about the shrinkage.  This would cause a spurious error
> message to be emitted by cmd_fetch_pack() if "git fetch-pack" is
> called with duplicate refnames.
> 
> So change the signature of fetch_pack() to accept nr_heads by
> reference, and if any duplicates were removed then modify it to
> reflect the number of remaining references.
> 
> The last test of t5500 inexplicably *required* "git fetch-pack" to
> fail when fetching a list of references that contains duplicates;
> i.e., it insisted on the buggy behavior.  So change the test to expect
> the correct behavior.

Eek, yeah, the current behavior is obviously wrong. The
remove_duplicates code comes from 310b86d (fetch-pack: do not barf when
duplicate re patterns are given, 2006-11-25) and clearly meant for
fetch-pack to handle this case gracefully.

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 3cc3346..0d4edcb 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' '
>  test_expect_success 'test duplicate refs from stdin' '
>  	(
>  	cd client &&
> -	test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup
> +	git fetch-pack --stdin --no-progress .. <../input.dup
>  	) >output &&
>  	cut -d " " -f 2 <output | sort >actual &&
>  	test_cmp expect actual

It's interesting that the output was the same before and after the fix.
I guess that is because the error comes at the very end, when we are
making sure all of the provided heads have been consumed.

-Peff