Re: [PATCH v5 1/3] test: add helper functions for git-bundle

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2021, 20:09 UTC
Message-ID: <xmqq1rer8cbz.fsf@gitster.c.googlers.com>
In-Reply-To: <20210110143019.5625-2-worldhello.net@gmail.com>

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

Show 46 quoted lines

> +# 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=t
> +			;;
> +		--tag)
> +			tag=t
> +			;;
> +		--notick)
> +			notick=t
> +			;;
> +		--signoff)
> +			signoff="$1"
> +			;;
> +		-C)
> +			shift
> +			indir="$1"
> +			;;
> +		-*)
> +			echo >&2 "error: unknown option $1"
> +			return 1
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done
> +
> +	var=$1
> +	shift
> +	if test -z "$var"
> +	then
> +		echo >&2 "error: var is not defined"
> +		return 1
> +	fi

We need to check $# immediately after the loop to ensure that we can carve out $var and at least another arg. [*Nit 1*]

The previous round required the command line to have at least one after the loop (including parsing of $var) parsed it, but now we fall through from here when a command line were:

    test_commit_setvar --merge -C there VAR

and because "$1" does not exist, such an error is propagated down to "git merge" not getting the side branch, "git tag" not getting the object to tag, etc.

Show 17 quoted lines

> +	indir=${indir:+"$indir"/}
> +	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" &&

Style?  [*Nit 2*]

Show 6 quoted lines

> +		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
> +}

Show 8 quoted lines

> +
> +
> +# 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.
> +make_user_friendly_and_stable_output () {
> +	sed \
> +		-e "s/$(echo $A | cut -c1-7)[0-9a-f]*/<COMMIT-A>/g" \

Is "$(echo $A | cut -c1-7)" the same as "${A%${A#???????}}"? If so, the latter may be a bit shorter.

Show 43 quoted lines

> diff --git a/t/test-bundle-functions.sh b/t/test-bundle-functions.sh
> new file mode 100644
> index 0000000000..0853eb1eca
> --- /dev/null
> +++ b/t/test-bundle-functions.sh
> @@ -0,0 +1,47 @@
> +# Library of git-bundle related functions.
> +
> +# 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
> +}
> +
> +# 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=t
> +		shift
> +	fi
> +	if test $# -ne 2
> +	then
> +		echo >&2 "args should be: <bundle> <count>"
> +		return 1
> +	fi
> +	bundle=$1
> +	pack=$bundle.pack
> +	convert_bundle_to_pack <"$bundle" >"$pack" &&
> +	if test -n "$thin"
> +	then
> +		mv "$pack" "$bundle.thin.pack" &&
> +		git index-pack --stdin --fix-thin "$pack" <"$bundle.thin.pack"
> +	else
> +		git index-pack "$pack"
> +	fi

I wonder why we shouldn't always do "--fix-thin", so that the caller does not even have to bother passing the "--thin" option.

Is this to protect us from "git bundle" creating a bundle that contains a thin pack when it should not? A caller that knows it is storing a fully connected history can deliberately omit "--thin" from the command line and make sure "index-pack" that is not asked to do "--fix-thin" indeed finds the pack data fully self-contained, so it may be a good idea to have these two separate codepaths after all. OK.

Show 5 quoted lines

> +	if test $? -ne 0
> +	then
> +		echo >&2 "error: fail to convert $bundle or index-pack"
> +		return 1
> +	fi

Do we even need the "error" message? "git index-pack" would have already given some error message to its standard error stream, no? If so

	if test -n "$thin"
	then
		...
	fi || return 1

would be sufficient, I guess.

Show 5 quoted lines

> +	count=$(git show-index <"${pack%pack}idx" | wc -l) &&
> +	test $2 = $count && return 0
> +	echo >&2 "error: object count for $bundle is $count, not $2"
> +	return 1
> +}

Looking good except for a few minor nits I mentioned above.

Thanks.