Re: [PATCH v2 1/2] rev-parse: fix segfault with missing --path-format argument

[Jeff King <peff@peff.net>]

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