| Message ID | 20190129193818.8645-5-jeremyhu@apple.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Differences between git-2.20.1 and Apple Git-116 | expand |
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 //'
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) > >
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}"
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(-)