Re: [PATCH (Apple Git) 04/13] t4014: git --version can have SP in it

[Johannes Schindelin <Johannes.Schindelin@xxxxxx>]

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@xxxxxxxxx>
> Reviewed-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> ---
>  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)
> 
>