From: Jeff King <peff@peff.net>
To: mhagger@alum.mit.edu
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads
Date: Thu, 23 Aug 2012 04:54:10 -0400 [thread overview]
Message-ID: <20120823085409.GD6963@sigill.intra.peff.net> (raw)
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
next prev parent reply other threads:[~2012-08-23 8:54 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-23 8:10 [PATCH 00/17] Clean up how fetch_pack() handles the heads list mhagger
2012-08-23 8:10 ` [PATCH 01/17] t5500: add tests of error output for missing refs mhagger
2012-08-23 8:10 ` [PATCH 02/17] Rename static function fetch_pack() to http_fetch_pack() mhagger
2012-08-23 8:10 ` [PATCH 03/17] Fix formatting mhagger
2012-08-23 8:10 ` [PATCH 04/17] Name local variables more consistently mhagger
2012-08-23 8:39 ` Jeff King
2012-08-24 7:05 ` Michael Haggerty
2012-08-26 17:39 ` Junio C Hamano
2012-08-27 9:22 ` Michael Haggerty
2012-08-27 9:25 ` Jeff King
2012-08-27 16:55 ` Junio C Hamano
2012-08-27 16:50 ` Junio C Hamano
2012-08-23 8:10 ` [PATCH 05/17] Do not check the same match_pos twice mhagger
2012-08-23 8:42 ` Jeff King
2012-08-23 8:10 ` [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads mhagger
2012-08-23 8:54 ` Jeff King [this message]
2012-08-25 5:05 ` Michael Haggerty
2012-08-23 8:10 ` [PATCH 07/17] Pass nr_heads to do_pack_ref() by reference mhagger
2012-08-23 8:10 ` [PATCH 08/17] Pass nr_heads to everything_local() " mhagger
2012-08-23 8:10 ` [PATCH 09/17] Pass nr_heads to filter_refs() " mhagger
2012-08-23 8:10 ` [PATCH 10/17] Remove ineffective optimization mhagger
2012-08-23 8:10 ` [PATCH 11/17] filter_refs(): do not leave gaps in return_refs mhagger
2012-08-23 8:10 ` [PATCH 12/17] filter_refs(): compress unmatched refs in heads array mhagger
2012-08-23 8:10 ` [PATCH 13/17] cmd_fetch_pack: return early if finish_connect() returns an error mhagger
2012-08-23 8:10 ` [PATCH 14/17] Report missing refs even if no existing refs were received mhagger
2012-08-23 8:10 ` [PATCH 15/17] cmd_fetch_pack(): simplify computation of return value mhagger
2012-08-23 8:10 ` [PATCH 16/17] fetch_pack(): free matching heads mhagger
2012-08-23 9:04 ` Jeff King
2012-08-23 8:10 ` [PATCH 17/17] fetch_refs(): simplify logic mhagger
2012-08-23 9:07 ` Jeff King
2012-08-25 6:37 ` Michael Haggerty
2012-08-23 9:26 ` [PATCH 00/17] Clean up how fetch_pack() handles the heads list Jeff King
2012-08-23 19:13 ` Philip Oakley
2012-08-23 19:56 ` Jeff King
2012-08-23 20:31 ` Jeff King
2012-08-25 7:05 ` Michael Haggerty
2012-09-02 7:02 ` Michael Haggerty
2012-08-23 22:09 ` Philip Oakley
2012-08-24 4:23 ` Junio C Hamano
2012-08-24 12:46 ` Philip Oakley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120823085409.GD6963@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.