git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Ann T Ropea <bedhanger@gmx.de>
Cc: Philip Oakley <philipoakley@iee.org>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Git Mailing List <git@vger.kernel.org>,
	Daniel Barkalow <barkalow@iabervon.org>
Subject: Re: [PATCH v6 4/7] checkout: describe_detached_head: remove ellipsis after committish
Date: Tue, 05 Dec 2017 08:03:20 -0800	[thread overview]
Message-ID: <xmqq8tehjh6f.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20171204231313.17929-1-bedhanger@gmx.de> (Ann T. Ropea's message of "Tue, 5 Dec 2017 00:13:13 +0100")

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.

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

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


  reply	other threads:[~2017-12-05 16:03 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-05 16:27 [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish Ann T Ropea
2017-11-05 16:27 ` [PATCH 2/3] Documentation: user-manual: limit potentially confusing usage of 3dots (and 2dots) Ann T Ropea
2017-11-05 16:27 ` [PATCH 3/3] Documentation: revisions: add note about 3dots usages as continuation indications Ann T Ropea
2017-11-06  4:34   ` Junio C Hamano
2017-11-06  2:45 ` [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish Junio C Hamano
2017-11-07  0:30   ` Philip Oakley
2017-11-07  0:52 ` Junio C Hamano
2017-11-07  2:53 ` Ann T Ropea
2017-11-07 23:25   ` Philip Oakley
2017-11-08  1:59     ` Junio C Hamano
2017-11-09 23:15       ` Philip Oakley
2017-11-13 22:36         ` [PATCH v2 1/6] config: introduce core.printsha1ellipsis Ann T Ropea
2017-11-13 22:36         ` [PATCH v2 2/6] checkout: describe_detached_head: remove ellipsis after committish Ann T Ropea
2017-11-13 22:36         ` [PATCH v2 3/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value Ann T Ropea
2017-11-14  3:08           ` Junio C Hamano
2017-11-19 17:38             ` Ann T Ropea
2017-11-20  1:48               ` Junio C Hamano
2017-11-19 18:41             ` [PATCH v3 1/5] checkout: describe_detached_head: remove ellipsis after committish Ann T Ropea
2017-11-20  3:35               ` Junio C Hamano
2017-11-19 18:41             ` [PATCH v3 2/5] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value Ann T Ropea
2017-11-19 19:11               ` Eric Sunshine
2017-11-19 18:41             ` [PATCH v3 3/5] Documentation: user-manual: limit usage of ellipsis Ann T Ropea
2017-11-19 19:15               ` Eric Sunshine
2017-11-24 23:53                 ` [PATCH v4 1/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value Ann T Ropea
2017-11-25  5:01                   ` Junio C Hamano
2017-11-26  3:17                     ` Junio C Hamano
2017-11-26  3:19                       ` Junio C Hamano
2017-11-26  3:25                       ` Junio C Hamano
2017-12-03 21:27                       ` [PATCH v5 1/7] Documentation: revisions: fix typo: "three dot" ---> "three-dot" (in line with "two-dot") Ann T Ropea
2017-12-04 16:52                         ` Junio C Hamano
2017-12-03 21:27                       ` [PATCH v5 2/7] Documentation: user-manual: limit usage of ellipsis Ann T Ropea
2017-12-03 21:27                       ` [PATCH v5 3/7] print_sha1_ellipsis: introduce helper Ann T Ropea
2017-12-03 21:27                       ` [PATCH v5 4/7] checkout: describe_detached_head: remove ellipsis after committish Ann T Ropea
2017-12-04 16:46                         ` Junio C Hamano
2017-12-04 23:13                           ` [PATCH v6 " Ann T Ropea
2017-12-05 16:03                             ` Junio C Hamano [this message]
2017-12-06  0:20                               ` [PATCH v7 " Ann T Ropea
2017-12-06 16:47                                 ` Junio C Hamano
2017-12-06 22:02                                   ` Ann T Ropea
2017-12-03 21:27                       ` [PATCH v5 5/7] t4013: prepare for upcoming "diff --raw --abbrev" output format change Ann T Ropea
2017-12-03 21:27                       ` [PATCH v5 6/7] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value Ann T Ropea
2017-12-03 21:27                       ` [PATCH v5 7/7] t4013: test new output from diff --abbrev --raw Ann T Ropea
2017-11-24 23:53                 ` [PATCH v4 2/6] checkout: describe_detached_head: remove ellipsis after committish Ann T Ropea
2017-11-24 23:53                 ` [PATCH v4 3/6] Documentation: user-manual: limit usage of ellipsis Ann T Ropea
2017-11-24 23:53                 ` [PATCH v4 4/6] Documentation: revisions: fix typo: "three dot" ---> "three-dot" (in line with "two-dot") Ann T Ropea
2017-11-24 23:53                 ` [PATCH v4 5/6] Documentation: git: document GIT_PRINT_SHA1_ELLIPSIS Ann T Ropea
2017-11-24 23:53                 ` [PATCH v4 6/6] Testing: provide existing tests requiring them with ellipses after SHA-1 values Ann T Ropea
2017-11-19 18:41             ` [PATCH v3 4/5] Documentation: revisions: fix typo: "three dot" ---> "three-dot" (in line with "two-dot") Ann T Ropea
2017-11-19 18:41             ` [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values Ann T Ropea
2017-11-20  4:06               ` Junio C Hamano
2017-11-20 12:25                 ` Philip Oakley
2017-11-22  5:53                   ` Junio C Hamano
2017-11-22 23:41                     ` Philip Oakley
2017-11-24  0:40                       ` Junio C Hamano
2017-11-13 22:36         ` [PATCH v2 4/6] Documentation: user-manual: limit usage of ellipsis Ann T Ropea
2017-11-13 22:36         ` [PATCH v2 5/6] Documentation: revisions: fix typo: "three dot" ---> "three-dot" (in line with "two-dot") Ann T Ropea
2017-11-13 22:36         ` [PATCH v2 6/6] Testing: provide tests requiring them with ellipses after SHA-1 values Ann T Ropea
2017-11-14  3:20           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8tehjh6f.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=barkalow@iabervon.org \
    --cc=bedhanger@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=philipoakley@iee.org \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).