[(Apple,Git),04/13] t4014: git --version can have SP in it
diff mbox series

Message ID 20190129193818.8645-5-jeremyhu@apple.com
State New
Headers show
Series
  • Differences between git-2.20.1 and Apple Git-116
Related show

Commit Message

Jeremy Sequoia Jan. 29, 2019, 7:38 p.m. UTC
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.

Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 t/t4014-format-patch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Jan. 29, 2019, 10:58 p.m. UTC | #1
Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

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

I am not sure if that customization is a sensible thing to do in the
first place, but ...

>  
> -git_version="$(git --version | sed "s/.* //")"
> +git_version="$(git --version | sed "s/git version //")"
>  

... this is good, simply because in help.c::cmd_version() we see

        int cmd_version(int argc, const char **argv, const char *prefix)
        {
                ...
                printf("git version %s\n", git_version_string);

i.e. no matter how heavily modified git_version_string[] is, we will
always show "git version" at the beginning (unless a builder goes
one step further to customize the version string by modifying the
source, at which point all bets are off).

To save reviewers and readers from wasting time wondering what
happens when a company, which is even less reasonable than Apple,
modifies the version number to include "git version" in it, the
updated sed expression probably should anchor the pattern to the
left edge to clarify the intention, even though it would not make
any difference in practice, i.e.

	sed 's/^git version //'
Johannes Schindelin Jan. 30, 2019, 1:30 p.m. UTC | #2
Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

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

Here would be a fine place to add Junio's explanation that `git version`
always prefixes "git version " to the `git_version_string` and that the
default signature in `builtin/log.c` is defined as said
`git_version_string`.

> 
> Instead, come up with the version string by stripping the "git version "
> from the beginning.
> 
> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae

This is really not a good  way to reference a commit, what with our
intention to switch to SHA-256 at some stage.

Besides, this footer is completely redundant with the information that
starts the very first paragraph.

Ciao,
Johannes

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> 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 909c743c13..414c56fcff 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -757,7 +757,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 //")"

As Junio said, this should be anchored. And for extra safety, in case some
even more unreasonable company decides to change the output of `git
--version` itself, it should probably use

	git_version="$(expr "$(git --version)" : "^git version \(.*\)")"

Ciao,
Johannes

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

Patch
diff mbox series

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 909c743c13..414c56fcff 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -757,7 +757,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}"