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

On Fri, Nov 2, 2012 at 5:03 PM, Stefano Lattarini
[off-list ref] wrote:

On 11/02/2012 04:42 PM, Felipe Contreras wrote:

quoted

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.

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

+        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