Re: [PATCH v4 1/8] t5520: prevent field splitting in content comparisons

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2015, 18:07 UTC
Message-ID: <xmqqa8x1dajl.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1431955978-17890-2-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 29 quoted lines

> Many tests in t5520 used the following to test the contents of files:
>
> 	test `cat file` = expected
>
> or
>
> 	test $(cat file) = expected
>
> These 2 forms, however, will be affected by field splitting and,
> depending on the value of $IFS, may be split into multiple arguments,
> making the test fail in mysterious ways.
>
> Replace the above 2 forms with:
>
> 	test "$(cat file)" = expected
>
> as quoting the command substitution will prevent field splitting.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> * Removed use of "verbose"
>
> * The use of "wc -l" is not quoted, as the output of "wc -l" on a Mac contains
>   leading whitespace. See [1].
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/268950/focus=269052
>
>  t/t5520-pull.sh | 70 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)

Overall the series was a pleasant read.

Some common patterns (my mind is still continuing the verbose_test topic) I noticed were

 (1) comparing output from two rev-parse output for object names. We
     have test_cmp_rev we can use for better readability (but its
     implementaiton may want to be updated not to contaminate the
     working tree unnecessarily). e.g

	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)"
     => test_cmp_rev HEAD^ copy
 
 (2) checking contents of a file, either from a working tree, from
     the index or from a rev, with a fixed short string.

	test_contents () {
                case "$1" in
                *:*)	git cat-file blob "$1" ;;
                *)	cat "$1" ;;
                esac >actual &&
		printf "%s" "$2" >expect &&
		if ! cmp -s expect actual
                then
			echo "Unexpected contents in '$1'"
			test_cmp expect actual
		else
			: good ;
		fi
	}

     or something.

I think it is a good idea *not* to address these patterns within this series, and have people come back to them _after_ the series settles, to reduce code churn and unnecessary conflicts.

Thanks.