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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Nov 2, 2012, 15:42 UTC
Message-ID: <CAMP44s0n_O-7j329WyTxsWgVbCV2mEAnPG-WQeWBFdmHdfOjnQ@mail.gmail.com>
In-Reply-To: <5093D0DD.3050801@gmail.com>

On Fri, Nov 2, 2012 at 2:55 PM, Stefano Lattarini <stefano.lattarini@gmail.com> wrote:

Show 6 quoted lines

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

Well, this is a _reference_ script, and that is used only for testing purposes. The test itself can be like the bash completion tests, and simply be skipped.

The reason I chose bash is because associative arrays, which you see in a later patch.

Show 15 quoted lines

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

What happens when you call this with:

 ./script "alias with spaces"

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

_If_ we want this as POSIX, yeah.

Show 13 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

All right, thanks.

Show 5 quoted lines

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

What if line has multiple spaces? To me it makes sense to quote it.

Show 10 quoted lines

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

If that's what we want, yeah.

-- 
Felipe Contreras