git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Cc: git@vger.kernel.org, Josh Triplett <josh@joshtriplett.org>
Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
Date: Mon, 10 Oct 2016 09:42:48 -0700	[thread overview]
Message-ID: <xmqqpon8gnp3.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20161010025323.9415-1-jeremyhu@apple.com> (Jeremy Huddleston Sequoia's message of "Sun, 9 Oct 2016 19:53:23 -0700")

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae

Please be considerate to future readers of "git log" to help them
avoid mistakes earlier commits made that caused you troubles.

This line by itself without any explanation of what was broken is
quite useless as a commit message.  What can the readers do?  They'd
go back and say "git show 480871e09" and then what?  The test added
or modified by the commit has been working quite well for others
since it was made, so it is very likely the reader wouldn't be able
to tell if anything is wrong in it.

You would help if you said what is different in _your_ environment
from others who have happily been running and passing this test for
others to understand and learn from your fix.  What is it?

The "Adjust ... distro changes" in the title offers some hint but it
could be more explicit.  Please write something along this line
instead.

    Subject: t4014: git --version can have SP in it

    480871e09e ("format-patch: show base info before email
    signature", 2016-09-07) added a helper function to recreate the
    signature at the end of the e-mail, i.e. "-- " line followed by
    the version string of Git, using output from "git --version" and
    stripping everything before the last SP.

    Because the default Git version string looks like "git version
    2.10.0-1-g480871e09e", this was mostly OK, but people can change
    this version string to arbitrary thing while compiling, which
    can break the assumption if they had SP in it.  Notably, Apple
    ships modified Git with " (Apple Git-xx)" appended to its
    version number.

    Instead, come up with the version string by stripping the "git
    version " from the beginning.

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>

Good.

> CC: Josh Triplett <josh@joshtriplett.org>
> CC: Junio C Hamano <gitster@pobox.com>

Please don't do this in your log message.  These belong to your
e-mail headers, not here.

> ---
>  t/t4014-format-patch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 8d90a6e..33f6940 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -754,7 +754,7 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
>  	git format-patch --ignore-if-in-upstream HEAD
>  '
>  
> -git_version="$(git --version | sed "s/.* //")"
> +git_version="$(git --version | sed "s/git version //")"

Anchor the fixed prefix to the beginning, so that we can protect
ourselves from another distro that would add "git version" in the
middle of their arbitrary versioning scheme ;-).  I.e.

    sed "s/^git version //"

>  
>  signature() {
>  	printf "%s\n%s\n\n" "-- " "${1:-$git_version}"

  parent reply	other threads:[~2016-10-10 16:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-10  2:53 [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER Jeremy Huddleston Sequoia
2016-10-10  3:04 ` Josh Triplett
2016-10-10  4:08   ` Jeremy Huddleston Sequoia
2016-10-10 13:10 ` Jeff King
2016-10-10 16:37   ` Jeremy Huddleston Sequoia
2016-10-10 16:46   ` Junio C Hamano
2016-10-10 16:42 ` Junio C Hamano [this message]
2016-10-10 17:01   ` Jeremy Huddleston Sequoia
2016-10-11 20:06   ` Eric Wong

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=xmqqpon8gnp3.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jeremyhu@apple.com \
    --cc=josh@joshtriplett.org \
    /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).