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

From: Stefano Lattarini <stefano.lattarini@gmail.com>
Date: Nov 2, 2012, 13:55 UTC
Message-ID: <5093D0DD.3050801@gmail.com>
In-Reply-To: <1351821738-17526-5-git-send-email-felipe.contreras@gmail.com>

On 11/02/2012 03:02 AM, Felipe Contreras wrote:

Show 29 quoted lines

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

Show 14 quoted lines

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

Show 5 quoted lines

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

Show 27 quoted lines

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

Show 5 quoted lines

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

Here too.

Show 5 quoted lines

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