git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] p5332: drop "+" from --stdin-packs input

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 22, 2025, 15:49 UTC
Message-ID
<xmqqv7qw42na.fsf@gitster.g>
In-Reply-To
<20250422111632.GA1855088@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 33 quoted lines
>
> -- >8 --
> Subject: [PATCH] p5332: drop "+" from --stdin-packs input
>
> This perf script creates a midx by running "git multi-pack-index write"
> with the "--stdin-packs" option. We feed that stdin by running "find" on
> .git/objects/pack, using sed to strip off everything but the basename.
>
> But that sed invocation also does something peculiar: it adds a "+" to
> the start of each pack name. This causes the multi-pack-index command to
> barf. The modified name does not match any pack it knows about, so it
> ends up with an empty list of packs to put in the midx. And thus nothing
> matches the --preferred-pack option we pass, which causes it die().
>
> The fix is to remove the extra "+" (which also lets us simplify the sed
> invocation a bit, as it is now just stripping the leading directories).
>
> But that leaves the mystery of why it was ever there in the first place.
> The answer is that an earlier iteration of the patch series had a
> concept of "disjoint" packs in the midx. And one of its patches here:
>
>   https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/
>
> taught read_packs_from_stdin() to treat a leading "+" as marking a
> disjoint pack. But in the second version of the series, which was
> ultimately merged, that disjoint concept went away, and the code to
> parse "+" did likewise. The regular regression tests were adjusted to
> match, but this case in t/perf was forgotten.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/perf/p5332-multi-pack-reuse.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks. I wonder if we had some tools and mechanisms people were discussing to track changes on changsets, such a mishap could have been caught more easily. [jc: random folks from that discussion CC'ed, just in case they are interested].

Show 14 quoted lines
>
> diff --git a/t/perf/p5332-multi-pack-reuse.sh b/t/perf/p5332-multi-pack-reuse.sh
> index d1c89a8b7d..0a2525db44 100755
> --- a/t/perf/p5332-multi-pack-reuse.sh
> +++ b/t/perf/p5332-multi-pack-reuse.sh
> @@ -58,7 +58,7 @@ do
>  	'
>  
>  	test_expect_success "setup bitmaps for $nr_packs-pack scenario" '
> -		find $packdir -type f -name "*.idx" | sed -e "s/.*\/\(.*\)$/+\1/g" |
> +		find $packdir -type f -name "*.idx" | sed -e "s/.*\///" |
>  		git multi-pack-index write --stdin-packs --bitmap \
>  			--preferred-pack="$(find_pack $(git rev-parse HEAD))"
>  	'
Previous: Jeff KingNext: Jeff King
Message 4 of 6 in “Test failure in p5332-multi-pack-reuse.sh”
  1. Philippe BlainApr 22, 2025
  2. Junio C HamanoApr 22, 2025
  3. p5332: drop "+" from --stdin-packs inputJeff King, Apr 22, 2025
  4. Junio C HamanoApr 22, 2025
  5. Jeff KingMay 1, 2025
  6. Taylor BlauApr 22, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.