On Fri, Sep 11, 2020 at 3:56 PM Jeff King <peff@xxxxxxxx> 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
