Re: [PATCH 6/7] t1303 (config): style tweaks

[Jeff King <peff@peff.net>]

On Mon, Sep 06, 2010 at 08:53:18PM -0500, Jonathan Nieder wrote:

> This test already has impeccable style,

I don't think my style has ever been called impeccable...

> with one exception: there is an unnecessary use of a subshell.  Use a
> {} block instead.

OK. Does this actually matter for anything? I know Windows has slow
fork, but I doubt it is measurable here.

> While at it:
> 
>  - guard setup commands with test_expect_success, so the commands
>    are printed when the test is run with "-v" and errors in setup
>    can be caught;

This confuses me. The setup is _already_ run inside test_expect_success.
It is only the shell function definitions you are moving inside. Do we
really care about this? If so, aren't there a zillion other places where
we define shell functions in test scripts? Try "git grep
'^[a-z][a-z]* *('".

You do move some variable definitions inside, too,, but IMO you are
making at least one of them less readable, because you have to deal with
the extra quoting layer of test_expect_success. IOW:

> -SECTION="test.q\"s\\sq'sp e.key"
>  test_expect_success 'make sure git config escapes section names properly' '
> +       SECTION="test.q\"s\\sq'\''sp e.key" &&

seems like a net loss to me.

>  - use echo instead of printf to print simple text ending with a
>    newline, so the later use of printf stands out more;

No complaint here.

>  - put a single space before () in function definitions, for
>    consistency with other shell scripts in git;

Again, I am not sure we care, but if we do, there are a ton of other
places: git grep '^[a-z][a-z]*('.

>  - reorder arguments to test_cmp as "test_cmp expected actual".

Yeah, I prefer it as "test_cmp expected actual". I'm surprised I ever
wrote it the other way (actually, I think it is because my writing
predates test_cmp, even).

So I dunno. Most of it I am fine with, though I question whether it is
really worth the effort. But I really don't want to be too draconian
about "everything must go into test_expect_success". Sure, if you are
executing commands that might have output, or might be of interest to
the user, put them there. But I find this a lot more readable:

cat >expect <<'EOF'
... some expected output ...
EOF
test_expect_success 'frob it' '
        git frob &&
        test_cmp expect actual
'

than:

test_expect_success 'frob it' '
        cat >expect <<"EOF" &&
... some expected output ...
EOF
        git frob &&
        test_cmp expect actual
'

-Peff