Jiang Xin <worldhello.net@xxxxxxxxx> 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.
> 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.
> + 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.
