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

[Jeff King]

On Mon, Sep 19, 2016 at 02:16:23PM -0700, Junio C Hamano wrote:

> Kevin Daudt  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".

The name "$DATA" was my suggestion. I was shooting for something short
since this is used a lot and is really a script-local variable (I'd have
kept it lowercase to indicate that, but maybe that is just me).
Something like "$root" would also work. I dunno.

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

Yeah, I agree that quoting the whole thing makes it more obvious (though
I guess quoting the second info$mo does add two characters).

-Peff

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

[Junio C Hamano]

Kevin Daudt  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

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