* [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
@ 2016-10-10 2:53 Jeremy Huddleston Sequoia
2016-10-10 3:04 ` Josh Triplett
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jeremy Huddleston Sequoia @ 2016-10-10 2:53 UTC (permalink / raw)
To: git; +Cc: Jeremy Huddleston Sequoia, Josh Triplett, Junio C Hamano
Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Junio C Hamano <gitster@pobox.com>
---
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 //")"
signature() {
printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
--
2.10.1 (Apple Git-99)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
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:42 ` Junio C Hamano
2 siblings, 1 reply; 9+ messages in thread
From: Josh Triplett @ 2016-10-10 3:04 UTC (permalink / raw)
To: Jeremy Huddleston Sequoia, git; +Cc: Junio C Hamano
On October 9, 2016 7:53:23 PM PDT, Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:
>Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
>Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>CC: Josh Triplett <josh@joshtriplett.org>
>CC: Junio C Hamano <gitster@pobox.com>
Looks reasonable to me. Didn't realize git versions could have spaces.
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> 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 //")"
>
> signature() {
> printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
2016-10-10 3:04 ` Josh Triplett
@ 2016-10-10 4:08 ` Jeremy Huddleston Sequoia
0 siblings, 0 replies; 9+ messages in thread
From: Jeremy Huddleston Sequoia @ 2016-10-10 4:08 UTC (permalink / raw)
To: Josh Triplett; +Cc: git, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]
> On Oct 9, 2016, at 20:04, Josh Triplett <josh@joshtriplett.org> wrote:
>
> On October 9, 2016 7:53:23 PM PDT, Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:
>> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> CC: Josh Triplett <josh@joshtriplett.org>
>> CC: Junio C Hamano <gitster@pobox.com>
>
> Looks reasonable to me. Didn't realize git versions could have spaces.
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Thanks, Josh.
If anyone feels strongly that they shouldn't, I'd be happy to change our DEF_VER patch to play nicer.
cf https://github.com/jeremyhu/git/commit/f99905d0752d923e5ec61e14c675a300c6d04284
>
>> 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 //")"
>>
>> signature() {
>> printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
>
>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4465 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
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 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
2 siblings, 2 replies; 9+ messages in thread
From: Jeff King @ 2016-10-10 13:10 UTC (permalink / raw)
To: Jeremy Huddleston Sequoia; +Cc: git, Josh Triplett, Junio C Hamano
On Sun, Oct 09, 2016 at 07:53:23PM -0700, Jeremy Huddleston Sequoia wrote:
> Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better
> handle distro changes to DEF_VER
>
> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
I see there was a discussion elsewhere on the list about exactly what
you are putting into DEF_VAR that causes the problem. Perhaps the commit
message here would be a good place to mention that, why the current
regex breaks it, and why your new version fixes not only it, but other
possible values of DEF_VAR.
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
2016-10-10 13:10 ` Jeff King
@ 2016-10-10 16:37 ` Jeremy Huddleston Sequoia
2016-10-10 16:46 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Jeremy Huddleston Sequoia @ 2016-10-10 16:37 UTC (permalink / raw)
To: Jeff King; +Cc: git, Josh Triplett, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
> On Oct 10, 2016, at 06:10, Jeff King <peff@peff.net> wrote:
>
> On Sun, Oct 09, 2016 at 07:53:23PM -0700, Jeremy Huddleston Sequoia wrote:
>
>> Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better
>> handle distro changes to DEF_VER
>>
>> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>
> I see there was a discussion elsewhere on the list about exactly what
> you are putting into DEF_VAR that causes the problem. Perhaps the commit
> message here would be a good place to mention that, why the current
> regex breaks it, and why your new version fixes not only it, but other
> possible values of DEF_VAR.
Thanks, I've added this blurb:
For example, git distributed with Apple's Xcode reports a version like:
git version 2.9.3 (Apple Git-75)
Apple started doing this to help customers distinguish between different
versions of their packaged git which have the same base version (eg: with
different patches applied).
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4465 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
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 13:10 ` Jeff King
@ 2016-10-10 16:42 ` Junio C Hamano
2016-10-10 17:01 ` Jeremy Huddleston Sequoia
2016-10-11 20:06 ` Eric Wong
2 siblings, 2 replies; 9+ messages in thread
From: Junio C Hamano @ 2016-10-10 16:42 UTC (permalink / raw)
To: Jeremy Huddleston Sequoia; +Cc: git, Josh Triplett
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}"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
2016-10-10 13:10 ` Jeff King
2016-10-10 16:37 ` Jeremy Huddleston Sequoia
@ 2016-10-10 16:46 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2016-10-10 16:46 UTC (permalink / raw)
To: Jeff King; +Cc: Jeremy Huddleston Sequoia, git, Josh Triplett
Jeff King <peff@peff.net> writes:
> On Sun, Oct 09, 2016 at 07:53:23PM -0700, Jeremy Huddleston Sequoia wrote:
>
>> Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better
>> handle distro changes to DEF_VER
>>
>> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>
> I see there was a discussion elsewhere on the list about exactly what
> you are putting into DEF_VAR that causes the problem. Perhaps the commit
> message here would be a good place to mention that, why the current
> regex breaks it, and why your new version fixes not only it, but other
> possible values of DEF_VAR.
Ah, should have known that you'd already be helping with this.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
2016-10-10 16:42 ` Junio C Hamano
@ 2016-10-10 17:01 ` Jeremy Huddleston Sequoia
2016-10-11 20:06 ` Eric Wong
1 sibling, 0 replies; 9+ messages in thread
From: Jeremy Huddleston Sequoia @ 2016-10-10 17:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Josh Triplett
[-- Attachment #1: Type: text/plain, Size: 3355 bytes --]
> On Oct 10, 2016, at 09:42, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
Ok, I'll add that explanation to the commit message, thanks.
>
>> 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.
Sorry, we do that consistently on other projects during the review process, so git send-email picks them up and adds them automatically for each patch being sent out. It's quite useful, actually.
>
>> ---
>> 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 //"
Ok.
>
>>
>> signature() {
>> printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4465 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER
2016-10-10 16:42 ` Junio C Hamano
2016-10-10 17:01 ` Jeremy Huddleston Sequoia
@ 2016-10-11 20:06 ` Eric Wong
1 sibling, 0 replies; 9+ messages in thread
From: Eric Wong @ 2016-10-11 20:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeremy Huddleston Sequoia, git, Josh Triplett
Junio C Hamano <gitster@pobox.com> wrote:
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>
> > 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.
Fwiw, I prefer having these trailers. It makes it easier to
maintain the Cc: list through multiple iterations/authors and is
also common practice on LKML.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-10-11 20:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2016-10-10 17:01 ` Jeremy Huddleston Sequoia
2016-10-11 20:06 ` Eric Wong
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).