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

Jiang Xin [off-list ref] writes:

+# 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.

+	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*]

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

+
+
+# 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.

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.

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

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