Re: [PATCH (Apple Git) 04/13] t4014: git --version can have SP in it
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 30, 2019, 13:30 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1901301419030.41@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20190129193818.8645-5-jeremyhu@apple.com>
Hi Jeremy,
On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
Show 11 quoted lines
> 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`.
Show 5 quoted lines
> > 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
Show 16 quoted lines
> 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
Show 7 quoted lines
>
> signature() {
> printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
> --
> 2.20.0 (Apple Git-115)
>
>