On Fri, Nov 2, 2012 at 2:55 PM, Stefano Lattarini
<stefano.lattarini@xxxxxxxxx> wrote:
>> +#!/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.
> 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"
>> +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.
>> +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.
>> +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.
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html