On Mon, May 17, 2021 at 10:02:42AM +0200, Wolfgang Müller wrote:
> Calling "git rev-parse --path-format" without an argument segfaults
> instead of giving an error message. Commit fac60b8925 (rev-parse: add
> option for absolute or relative path formatting, 2020-12-13) added the
> argument parsing code but forgot to handle NULL.
>
> Returning an error makes sense here because there is no default value we
> could use. Add a test case to verify.
>
> Signed-off-by: Wolfgang Müller <wolf@oriole.systems>
> ---
> builtin/rev-parse.c | 2 ++
> t/t1500-rev-parse.sh | 4 ++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 85bad9052e..7af8dab8bc 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -759,6 +759,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> continue;
> }
> if (opt_with_value(arg, "--path-format", &arg)) {
> + if (!arg)
> + die("--path-format requires an argument");
> if (!strcmp(arg, "absolute")) {
> format = FORMAT_CANONICAL;
> } else if (!strcmp(arg, "relative")) {
This looks like a fine solution, but I do think this code using
opt_with_value() is a bit of a curiosity in the first place. I looked at
the other callers (because I wanted to see if there were similar
problems), and they are all cases where the argument is truly optional
(so matching "--foo" or "--foo=bar" is correct, and matching "--foo bar"
would be actively wrong).
For cases where the argument is not optional, we seem to use
skip_prefix(), like:
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c4263404bd..9553cc7c10 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -760,7 +760,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
show(arg);
continue;
}
- if (opt_with_value(arg, "--path-format", &arg)) {
+ if (skip_prefix(arg, "--path-format=", &arg)) {
if (!strcmp(arg, "absolute")) {
format = FORMAT_CANONICAL;
} else if (!strcmp(arg, "relative")) {
I don't have a strong preference for one or the other. It is maybe
helpful to diagnose "--path-format" without an equals as an error, as
your patch would, rather than quietly passing it along as an unknown (as
the hunk above would). I dunno. That perhaps applies to the rest of the
options, too. :)
(In an ideal world, we would probably match "--path-format <arg>" here,
as our usual parse-options API does. But that is a much bigger change).
-Peff
