Re: [PATCH v4 04/14] Add new simplified git-remote-testgit

[Stefano Lattarini <stefano.lattarini@gmail.com>]

On 11/02/2012 03:02 AM, Felipe Contreras wrote:
> It's way simpler. It exerceises the same features of remote helpers.
> It's easy to read and understand. It doesn't depend on python.
> 
> It does _not_ exercise the python remote helper framework; there's
> another tool and another test for that.
> 
> For now let's just copy the old remote-helpers test script, although
> some of those tests don't make sense for this testgit (they still pass).
> 
> In addition, this script would be able to test other features not
> currently being tested.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-remote-testgit.txt |   2 +-
>  git-remote-testgit                   |  62 ++++++++++++++++
>  t/t5801-remote-helpers.sh            | 134 +++++++++++++++++++++++++++++++++++
>  3 files changed, 197 insertions(+), 1 deletion(-)
>  create mode 100755 git-remote-testgit
>  create mode 100755 t/t5801-remote-helpers.sh
> 
> diff --git a/git-remote-testgit b/git-remote-testgit
> new file mode 100755
> index 0000000..6650402
> --- /dev/null
> +++ b/git-remote-testgit
> @@ -0,0 +1,62 @@
> +#!/bin/bash
>
I think git can't assume the existence of bash unconditionally, neither
in its scripts, nor in its tests (the exception being the tests on
bash completion, of course).  This script probably need to be re-written
to be a valid POSIX shell script.

It almost is, anyway, apart from the nits below ...

> +# Copyright (c) 2012 Felipe Contreras
> +
> +alias="$1"
>
Just FYI: the double quoting here (and in several variable assignments
below) is redundant.  You can portably write it as:

    alias=$1

and still be safe in the face of spaces and metacharacters in $1.
I'm not sure whether the Git coding guidelines suggest the use of
quoting in this situation though; if this is the case, feel free
to disregard my observation.

> +url="$2"
> +
> +# huh?
> +url="${url#file://}"
> +
> +dir="$GIT_DIR/testgit/$alias"
> +prefix="refs/testgit/$alias"
> +refspec="refs/heads/*:${prefix}/heads/*"
> +
> +gitmarks="$dir/git.marks"
> +testgitmarks="$dir/testgit.marks"
> +
> +export GIT_DIR="$url/.git"
> +
I believe this should be rewritten as:

  GIT_DIR="$url/.git"; export GIT_DIR

in order to be portable to all the POSIX shells targeted by Git.

> +mkdir -p "$dir"
> +
> +test -e "$gitmarks" || echo -n > "$gitmarks"
> +test -e "$testgitmarks" || echo -n > "$testgitmarks"
> +
The '-n' option to echo is not portable.  To create an empty
file, you can just use

   : > file

or

   true > file

> +while read line; do
> +    case "$line" in
>
Useless double quoting (my previous observation about Git coding
guidelines applies here as well, of course).

> +    capabilities)
> +        echo 'import'
> +        echo 'export'
> +        echo "refspec $refspec"
> +        echo "*import-marks $gitmarks"
> +        echo "*export-marks $gitmarks"
> +        echo
> +        ;;
> +    list)
> +        git for-each-ref --format='? %(refname)' 'refs/heads/'
> +        head=$(git symbolic-ref HEAD)
> +        echo "@$head HEAD"
> +        echo
> +        ;;
> +    import*)
> +        # read all import lines
> +        while true; do
> +            ref="${line#* }"
> +            refs="$refs $ref"
> +            read line
> +            test "${line%% *}" != "import" && break
> +        done
> +
> +        echo "feature import-marks=$gitmarks"
> +        echo "feature export-marks=$gitmarks"
> +        git fast-export --use-done-feature --{import,export}-marks="$testgitmarks" $refs | \
>
Better avoid the tricky {foo,bar} bashism:

    git fast-export --use-done-feature \
                    --import-marks="$testgitmarks" \
                    --export-marks="$testgitmarks" \
		    $refs | \

> +            sed -e "s#refs/heads/#${prefix}/heads/#g"
> +        ;;
> +    export)
> +        git fast-import --{import,export}-marks="$testgitmarks" --quiet
>
Here too.

> +        echo
> +        ;;
> +    '')
> +        exit
>
I'd put an explicit exit status here, for clarity (this is a matter
of personal preference, so feel free to disregard this nit).

> +        ;;
> +    esac
> +done

Regards,
  Stefano