Re: [PATCH v2 1/8] t4051: rewrite, add more tests

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2016, 23:55 UTC
Message-ID: <xmqqinxwpgfn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <5749B1EA.10707@web.de>

René Scharfe <l.s.r@web.de> writes:

> +commit_and_tag () {
> +	message=$1 &&
> +	shift &&
> +	git add $@ &&

Lack of dq around $@ makes me wonder if there is something funny going on (looking at the callers, there isn't, so we'd better quote it to avoid wasting time, I think).

> +	test_tick &&
> +	git commit -m $message &&
> +	git tag $message
>  }

The use of $message as the sole argument to "git tag" makes the readers guess that it must be a single token without any funny character, so the readers would probably do not waste too much time wondreing if the lack of dq around $message in the last two is problematic.

> +last_context_line () {
> +	sed -n '$ p'
>  }

I have a vague recollection that some implementations of sed are unhappy to see that space between the address and the operation; I'd feel safer without it.

Show 12 quoted lines

> +check_diff () {
> +	name=$1
> +	desc=$2
> +	options="-W $3"
> +
> +	test_expect_success "$desc" '
> +		git diff $options "$name^" "$name" >"$name.diff"
> +	'
> +
> +	test_expect_success ' diff applies' '
> +		test_when_finished "git reset --hard" &&
> +		git checkout --detach "$name^" &&

With the presence of ^ there, --detach is unnecessary; it would not hurt, though.

> +		git apply "$name.diff" &&
> +		git diff --exit-code "$name"

Even though we may know that $name.diff" will never have a creation of new paths, I'd feel safer if "apply" is run with "--index".

Thanks.