Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Sep 11, 2020 at 3:56 PM Jeff King <peff@peff.net> wrote:
> And the beginning. :)
>
> It's matching the surrounding test, which are written in an older
> inconsistent style. I think it's OK to match them, but cleaning them
> would be welcome, too.
>
> While I'm looking at this hunk, two other things:
>
>  - do we really care about code 128, or just failure? test_must_fail
>    might be a better choice
>
>  - I didn't even know we had test_debug. ;) The last time somebody added
>    a call to it was in 2012. I think it's being used as intended here,
>    but I'm not sure that the clutter to the test is worth it (we have
>    other tools like "-i" to stop at the right spot and let you inspect
>    the broken state).
>
>  - the backslash escapes confused me for a moment. I guess they are
>    trying to hide the dashes from grep's option parser. That's OK,
>    though I'd have probably just started with "bare" since we're
>    matching a substring anyway. I think you could also use "-e" with
>    test_i18ngrep.

Thanks, you said everything I was going to say about this, thus saving
me some typing.

A couple more observations related to a few of the subsequent tests. This:

    template="$TRASH_DIRECTORY/template-with-config" &&

probably doesn't need $TRASH_DIRECTORY since that happens to be the
current working directory anyhow, so:

    template=template-with-config &&

should suffice (unless you had a problem doing it that way). Or you
could drop the variable altogether and use the literal name where you
need it.

Also, instead of:

    (cd clone-template-config && test "$(git config --local foo.bar)"
= "from_template")

would probably be written these days as:

    echo from_template >expect &&
    git -C clone-template-config config --local foo.bar >actual &&
    test_cmp expect actual