Re: [PATCH v6 4/7] checkout: describe_detached_head: remove ellipsis after committish

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2017, 16:03 UTC
Message-ID: <xmqq8tehjh6f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171204231313.17929-1-bedhanger@gmx.de>

Ann T Ropea <bedhanger@gmx.de> writes:

> v6: polish to take Junio's comments from <xmqqshcqmoe7.fsf@gitster.mtv.corp.google.com> into account

>  t/t2020-checkout-detach.sh    | 114 ++++++++++++++++++++++++++++++++++++++++++
> ...

Thanks; with this one replaced, I'd expect that poisoned gettext test to pass now.

I saw some style issues, so I'll queue a tentative fix-up on top.

Show 15 quoted lines

> +	# The first detach operation is more chatty than the following ones.
> +	cat 1>1st_detach <<'EOF' &&
> +Note: checking out 'HEAD^'.
> +
> +You are in 'detached HEAD' state. You can look around, make experimental
> +changes and commit them, and you can discard any commits you make in this
> +state without impacting any branches by performing another checkout.
> +
> +If you want to create a new branch to retain commits you create, you may
> +do so (now or later) by using -b with the checkout command again. Example:
> +
> +  git checkout -b <new-branch-name>
> +
> +HEAD is now at 7c7cd714e262 three
> +EOF

It looks somewhat strange to explicitly say 1> when redirecting the standard output. Also we prefer to indent our here-doc to the same depth as commands by using "<<-", i.e.

	cat >1st_detach <<-'EOF' &&
	Note: checking out 'HEAD^'.
	...
	EOF

> +	reset && check_not_detached && sane_unset GIT_PRINT_SHA1_ELLIPSIS &&

It also was a bit irritating to see multiple commands form an overlong single line, like this one.

Show 5 quoted lines

> +	test_i18ncmp 1st_detach actual && sane_unset GIT_PRINT_SHA1_ELLIPSIS &&
> +
> +	GIT_PRINT_SHA1_ELLIPSIS="no" git -c 'core.abbrev=12' checkout HEAD^ 1>actual 2>&1 &&
> +	check_detached &&
> +	test_i18ncmp 2nd_detach actual && sane_unset GIT_PRINT_SHA1_ELLIPSIS &&

This part uses "set and export only for a single invocation of git", and because the variable is unset at the end of the previous step after 1st_detach and actual are compared, this unset at the end feels redundant.

> +	GIT_PRINT_SHA1_ELLIPSIS='nope' && export GIT_PRINT_SHA1_ELLIPSIS && git -c 'core.abbrev=12' checkout HEAD^ 1>actual 2>&1 &&

But now this part sets and exports the variable for the remainder of the script (until it is unset).

Is the use of these two styles, i.e.

	VAR=value && export VAR && git -c core.abbrev=12 subcmd
	VAR=value git -c core.abbrev=12 subcmd

intended? If so for what purpose? It's not like we are testing if the shell implements environment variables correctly, so I'd expect that the result would be easier to follow if it stuck to a single style and used it consistently.