Re: [PATCH v2 1/2] t5100-mailinfo: replace common path prefix with variable

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2016, 21:16 UTC
Message-ID: <xmqqzin3d1zs.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160919185440.18234-2-me@ikke.info>

Kevin Daudt <me@ikke.info> writes:

> Many tests need to store data in a file, and repeat the same pattern to
> refer to that path:
>
>     "$TEST_DATA"/t5100/

That obviously is a typo of

	"$TEST_DIRECTORY/t5100"

It is a good change, even though I would have chosen a name that is a bit more descriptive than "$DATA".

>  test_expect_success 'split sample box' \
> -	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
> +	'git mailsplit -o. "$DATA"/sample.mbox >last &&

You are just following the pattern, and this instance is not too bad, but lines like these

Show 6 quoted lines

> -	test_cmp "$TEST_DIRECTORY"/t5100/msg$mo msg$mo &&
> -	test_cmp "$TEST_DIRECTORY"/t5100/patch$mo patch$mo &&
> -	test_cmp "$TEST_DIRECTORY"/t5100/info$mo info$mo
> +	test_cmp "$DATA"/msg$mo msg$mo &&
> +	test_cmp "$DATA"/patch$mo patch$mo &&
> +	test_cmp "$DATA"/info$mo info$mo

make me wonder why we don't quote the whole thing, i.e.

	test_cmp "$TEST_DATA/info$mo" "info$mo"

as leaving $mo part unquoted forces reader to wonder if it is our deliberate attempt to allow shell $IFS in $mo and have the argument split when that happens, which can be avoided if we quoted more explicitly.

Perhaps we'd leave that as a low-hanging fruit for future people.

Thanks.