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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Fri, Nov 2, 2012 at 5:03 PM, Stefano Lattarini
<stefano.lattarini@gmail.com> wrote:
> On 11/02/2012 04:42 PM, Felipe Contreras wrote:

>> What happens when you call this with:
>>
>>  ./script "alias with spaces"
>>
> '$alias' will correctly expand to "alias with spaces".  Try out:
>
>   $ sh -c 'alias=$1; echo "$alias"' dummy '1   2*3'
>   1   2*3
>
> This works consistently with every known shell (even non-POSIX
> relics like Solaris /bin/sh).

All right.

>> _If_ we want this as POSIX, yeah.
>>
> Why don't we?  Why add an extra requirement for a test that
>
>  1. can be easily written in POSIX shell, and
>  2. tests a feature that doesn't require bash to work (unless
>     I'm sorely mistaken, that is)?
>
> Honest question.  But of course, if the Git active contributors
> deem the extra requirement (which is not an invasive one, given
> how often bash is installed even on non-Linux systems) acceptable
> in order to have the test case simpler and clearer, feel free to
> disregard all my observations in this thread.

Because the code will be more complicated. Most of the changes
required are relatively small, so it's not a big issue, but I would
like to see how you replace the code that uses associative arrays. I
don't know know of a clean, simple way to do it in POSIX. If you can
find one, I don't see any strong reason to use bash.

>>>> +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?
>>
> Still no problem, as in the case of the 'alias=$1' assignment before:
>
>   $ sh -c 'case $1 in *x"  "x*) echo ok;; *) exit 1;; esac' dummy 'x  x'
>   ok

All right.

>>>> +        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.
>>
> Honestly, I find my longer-and-more-explicit version clearer, even
> if you can assume bash for your script.  But that's a matter of
> personal preference (sorry for not stating that right away), so
> feel free to ignore it if you decide to keep the bash requirement
> in the end.

And I prefer the other form. I fact, I would prefer if both tools
simply had a --marks option set both, and didn't require the file to
be created beforehand. I've _never_ seen a situation where separate
marks for import and export made sense. But that's off-topic.

If you find a way to replace the associative arrays code, I have no
trouble in switching this to the POSIX version.

Cheers.

-- 
Felipe Contreras