From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 09 Jan 2021 02:10:55 GMT
Subject: Re: [PATCH v4 1/2] bundle: lost objects when removing duplicate pendings
Message-ID: <xmqqv9c6g8r4.fsf@gitster.c.googlers.com>
In-Reply-To: <20210108144514.24805-2-worldhello.net@gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
> new file mode 100755
> index 0000000000..c4447ca88f
> --- /dev/null
> +++ b/t/t6020-bundle-misc.sh
> @@ -0,0 +1,477 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2021 Jiang Xin
> +#
> +
> +test_description='Test git-bundle'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +# Check count of objects in a bundle file.
> +# We can use "--thin" opiton to check thin pack, which must be fixed by
> +# command `git-index-pack --fix-thin --stdin`.
> +test_bundle_object_count () {
> +	thin= &&
> +	if test "$1" = "--thin"
> +	then
> +		thin=yes
> +		shift
> +	fi &&
> +	if test $# -ne 2
> +	then
> +		echo >&2 "args should be: <bundle> <count>"
> +		return 1
> +	fi
> +	bundle=$1 &&
> +	pack=${bundle%.bdl}.pack &&
> +	convert_bundle_to_pack <"$bundle" >"$pack" &&
> +	if test -n "$thin"
> +	then
> +		test_must_fail git index-pack "$pack" &&

This is overly strict, isn't it?  

Imagine a case where the objects newer revisions introduce have *no*
resemblance to the objects in the prerequisites' trees---the
resulting pack will have no object that is expressed as a delta
against anything outside the pack, and the above "index-pack" would
succeed.

Besides, "git pack-objects --thin" is *not* obligated to create a
pack that lacks one or more objects.  The "--thin" option merely
*allows* pack-objects to omit base objects if it is convenient to do
so.

> +		mv "$pack" "$pack"-thin &&
> +		cat "$pack"-thin |
> +			git index-pack --stdin --fix-thin "$pack"

This side is good, but do not cat a single file into a pipe.
The whole "then" clause would become

	then
		mv "$pack" "$pack-thin" &&
		git index-pack --stdin --fix-thin "$pack" <"$pack-thin"
	else

I would think.

> +	else
> +		git index-pack "$pack"
> +	fi &&
> +	git verify-pack -v "$pack" >verify.out
> +	if test $? -ne 0
> +	then
> +		echo >&2 "error: fail to convert $bundle to $pack"
> +		return 1
> +	fi

At this point, we are not testing the bundle subcommand, but is
testing "git index-pack --fix-thin" that we run ourselves.  Is it
essential to ensure $pack is sane here?  I doubt it.

> +	count=$(grep -c "^$OID_REGEX " verify.out) &&

And if there is no need to run verify-pack, then we can do
count=$(git show-index "${pack%pack}idx" | wc -l) instead, perhaps?

> +	test $2 = $count && return 0
> +	echo >&2 "error: object count for $bundle is $count, not $2"
> +	return 1
> +}
> +
> +# Display the pack data contained in the bundle file, bypassing the
> +# header that contains the signature, prerequisites and references.
> +convert_bundle_to_pack () {
> +	while read x && test -n "$x"
> +	do
> +		:;
> +	done
> +	cat
> +}

This looks somewhat familiar.  Perhaps extract out necessary helpers
including this one into t/test-bundle-lib or something similar in a
preparatory step before this patch?

> +# Create a commit or tag and set the variable with the object ID.
> +test_commit_setvar () {
> +	notick= &&
> +	signoff= &&
> +	indir= &&
> +	merge= &&
> +	tag= &&
> +	var= &&
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		--merge)
> +			merge=yes
> +			;;
> +		--tag)
> +			tag=yes
> +			;;
> +		--notick)
> +			notick=yes
> +			;;
> +		--signoff)
> +			signoff="$1"
> +			;;
> +		-C)
> +			indir="$2"
> +			shift
> +			;;
> +		-*)
> +			echo >&2 "error: unknown option $1"
> +			return 1
> +			;;
> +		*)
> +			test -n "$var" && break
> +			var=$1
> +			;;
> +		esac
> +		shift
> +	done &&

At this point, if $var is still empty, the caller is buggy, and ...

> +	indir=${indir:+"$indir"/} &&
> +	if test $# -eq 0
> +	then
> +		echo >&2 "no args provided"
> +		return 1
> +	fi &&
> +	if test -z "$notick"
> +	then
> +		test_tick
> +	fi &&
> +	if test -n "$merge"
> +	then
> +		git ${indir:+ -C "$indir"} merge --no-edit --no-ff \
> +			${2:+-m "$2"} "$1" &&
> +		oid=$(git ${indir:+ -C "$indir"} rev-parse HEAD)
> +	elif test -n "$tag"
> +	then
> +		git ${indir:+ -C "$indir"} tag -m "$1" "$1" &&
> +		oid=$(git ${indir:+ -C "$indir"} rev-parse "$1")
> +	else
> +		file=${2:-"$1.t"} &&
> +		echo "${3-$1}" > "$indir$file" &&
> +		git ${indir:+ -C "$indir"} add "$file" &&
> +		git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
> +		oid=$(git ${indir:+ -C "$indir"} rev-parse HEAD)
> +	fi &&
> +	eval $var=$oid
> +}

... it will cause a failure in 'eval' we have here.  Not good.

> +# Format the output of git commands to make a user-friendly and stable
> +# text.  We can easily prepare the expect text without having to worry
> +# about future changes of the commit ID and spaces of the output.

Hmph.  This relies on 7 hexdigits being sufficient to uniquely
identify all objects involved in the test?  It should be OK in
practice.

Is there a point in having both <COMMIT-A> and <OID-A>?  I would
have expected that all these "full object name" conversions are
unneeded.

> +make_user_friendly_and_stable_output () {
> +	sed \
> +		-e "s/$A/<COMMIT-A>/" \
> +		-e "s/$B/<COMMIT-B>/" \
> +		-e "s/$C/<COMMIT-C>/" \
> +		-e "s/$D/<COMMIT-D>/" \
> +		-e "s/$E/<COMMIT-E>/" \
> +		-e "s/$F/<COMMIT-F>/" \
> +		-e "s/$G/<COMMIT-G>/" \
> +		-e "s/$H/<COMMIT-H>/" \
> +		-e "s/$I/<COMMIT-I>/" \
> +		-e "s/$J/<COMMIT-J>/" \
> +		-e "s/$K/<COMMIT-K>/" \
> +		-e "s/$L/<COMMIT-L>/" \
> +		-e "s/$M/<COMMIT-M>/" \
> +		-e "s/$N/<COMMIT-N>/" \
> +		-e "s/$O/<COMMIT-O>/" \
> +		-e "s/$P/<COMMIT-P>/" \
> +		-e "s/$TAG1/<TAG-1>/" \
> +		-e "s/$TAG2/<TAG-2>/" \
> +		-e "s/$TAG3/<TAG-3>/" \
> +		-e "s/$(echo $A | cut -c1-7)[0-9a-f]*/<OID-A>/g" \
> +		-e "s/$(echo $B | cut -c1-7)[0-9a-f]*/<OID-B>/g" \
> +		-e "s/$(echo $C | cut -c1-7)[0-9a-f]*/<OID-C>/g" \
> +		-e "s/$(echo $D | cut -c1-7)[0-9a-f]*/<OID-D>/g" \
> +		-e "s/$(echo $E | cut -c1-7)[0-9a-f]*/<OID-E>/g" \
> +		-e "s/$(echo $F | cut -c1-7)[0-9a-f]*/<OID-F>/g" \
> +		-e "s/$(echo $G | cut -c1-7)[0-9a-f]*/<OID-G>/g" \
> +		-e "s/$(echo $H | cut -c1-7)[0-9a-f]*/<OID-H>/g" \
> +		-e "s/$(echo $I | cut -c1-7)[0-9a-f]*/<OID-I>/g" \
> +		-e "s/$(echo $J | cut -c1-7)[0-9a-f]*/<OID-J>/g" \
> +		-e "s/$(echo $K | cut -c1-7)[0-9a-f]*/<OID-K>/g" \
> +		-e "s/$(echo $L | cut -c1-7)[0-9a-f]*/<OID-L>/g" \
> +		-e "s/$(echo $M | cut -c1-7)[0-9a-f]*/<OID-M>/g" \
> +		-e "s/$(echo $N | cut -c1-7)[0-9a-f]*/<OID-N>/g" \
> +		-e "s/$(echo $O | cut -c1-7)[0-9a-f]*/<OID-O>/g" \
> +		-e "s/$(echo $P | cut -c1-7)[0-9a-f]*/<OID-P>/g" \
> +		-e "s/$(echo $TAG1 | cut -c1-7)[0-9a-f]*/<OID-TAG-1>/g" \
> +		-e "s/$(echo $TAG2 | cut -c1-7)[0-9a-f]*/<OID-TAG-2>/g" \
> +		-e "s/$(echo $TAG3 | cut -c1-7)[0-9a-f]*/<OID-TAG-3>/g" \
> +		-e "s/ *\$//"
> +}
> ...
> +test_expect_success 'create bundle from special rev: main^!' '
> +	git bundle create special-rev.bdl "main^!" &&
> +
> +	git bundle list-heads special-rev.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-P> refs/heads/main
> +		EOF

We prefer to indent these more like so:

	cat >expect <<-\EOF &&
	<COMMIT-P> refs/heads/main
	EOF

i.e. the indent of the line with <<EOF on it and the indent of the
line with the matching EOF are the same.  Also, quote EOF to signal
that the body of the here text should be taken as-is without $var
substitution.