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.

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

[Kevin Daudt]

Many tests need to store data in a file, and repeat the same pattern to
refer to that path:

"$TEST_DATA"/t5100/

Create a variable that contains this path, and use that instead.

Signed-off-by: Kevin Daudt 
---
 t/t5100-mailinfo.sh | 56 +++--
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..27bf3b8 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'
 
 . ./test-lib.sh
 
+DATA="$TEST_DIRECTORY/t5100"
+
 test_expect_success 'split sample box' \
-   'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
+   'git mailsplit -o. "$DATA"/sample.mbox >last &&
last=$(cat last) &&
echo total is $last &&
test $(cat last) = 17'
@@ -17,9 +19,9 @@ check_mailinfo () {
mail=$1 opt=$2
mo="$mail$opt"
git mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&
-   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
 }
 
 
@@ -27,15 +29,15 @@ for mail in 00*
 do
test_expect_success "mailinfo $mail" '
check_mailinfo $mail "" &&
-   if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
+   if test -f "$DATA"/msg$mail--scissors
then
check_mailinfo $mail --scissors
fi &&
-   if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers
+   if test -f "$DATA"/msg$mail--no-inbody-headers
then
check_mailinfo $mail --no-inbody-headers
fi &&
-   if test -f "$TEST_DIRECTORY"/t5100/msg$mail--message-id
+   if test -f "$DATA"/msg$mail--message-id
then
check_mailinfo $mail --message-id
fi
@@ -45,7 +47,7 @@ done
 
 test_expect_success 'split box with rfc2047 samples' \
'mkdir rfc2047 &&
-   git mailsplit -orfc2047 "$TEST_DIRECTORY"/t5100/rfc2047-samples.mbox \
+   git mailsplit -orfc2047 "$DATA"/rfc2047-samples.mbox \
  >rfc2047/last &&
last=$(cat rfc2047/last) &&
echo total is $last &&
@@ -56,18 +58,18 @@ do
test_expect_success "mailinfo $mail" '
git mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&
echo msg &&
-   test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-msg &&
+   test_cmp "$DATA"/empty $mail-msg &&
echo patch &&
-   test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-patch &&
+   test_cmp "$DATA"/empty $mail-patch &&
echo info &&
-   test_cmp "$TEST_DIRECTORY"/t5100/rfc2047-info-$(basename $mail) 
$mail-info
+   test_cmp "$DATA"/rfc2047-info-$(basename $mail) $mail-info
'
 done
 
 test_expect_success 'respect NULs' '
 
-   git mailsplit -d3 -o. "$TEST_DIRECTORY"/t5100/nul-plain &&
-   test_cmp "$TEST_DIRECTORY"/t5100/nul-plain 001 &&
+   git mailsplit -d3 -o. "$DATA"/nul-plain &&
+   test_cmp "$DATA"/nul-plain 001 &&
(cat 001 | git mailinfo msg patch) &&
test_line_count = 4 patch
 
@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '
 
 test_expect_success 'Preserve NULs out of MIME encoded message' '
 
-   git mailsplit -d5 -o. "$TEST_DIRECTORY"/t5100/nul-b64.in &&
-   test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.in 1 &&
+   git mailsplit -d5 -o. "$DATA"/nul-b64.in &&
+   test_cmp "$DATA"/nul-b64.in 1 &&
git mailinfo msg patch <1 &&
-   test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.expect patch
+   test_cmp "$DATA"/nul-b64.expect patch
 
 '
 
 test_expect_success 'mailinfo on from header without name works' '
 
mkdir info-from &&
-   git mailsplit -oinfo-from "$TEST_DIRECTORY"/t5100/info-from.in &&
-   test_cmp "$TEST_DIRECTORY"/t5100/info-from.in info-from/0001 &&
+   git mailsplit -oinfo-from "$DATA"/info-from.in &&
+   test_cmp "$DATA"/info-from.in info-from/0001 &&
git mailinfo info-from/msg info-from/patch \
  info-from/out &&
-   test_cmp "$TEST_DIRECTORY"/t5100/info-from.expect info-from/out
+   test_cmp "$DATA"/info-from.expect info-from/out
 
 '
 
 test_expect_success 'mailinfo finds headers after embedded From line' '
mkdir embed-from &&
-   git mailsplit -oembed-from "$TEST_DIRECTORY"/t5100/embed-from.in &&
-   test_cmp "$TEST_DIRECTORY"/t5100/embed-from.in embed-from/0001 &&
+   git mailsplit -oembed-from "$DATA"/embed-from.in &&
+   test_cmp "$DATA"/embed-from.in embed-from/0001 &&
git mailinfo