Re: [BUG] 'git ls-files --no-exclude' segfault & co

[Jeff King <peff@peff.net>]

On Mon, Aug 06, 2018 at 06:07:06PM +0200, SZEDER Gábor wrote:

> 'git ls-files' has the options '--exclude', '--exclude-from',
> '--exclude-per-directory', and '--exclude-standard', which all work
> fine.  However, it also allows the negated version of all these four
> options, and they definitely don't work very well:
> 
>   $ git ls-files --no-exclude
>   Segmentation fault
>   $ git ls-files --no-exclude-from
>   warning: unable to access '(null)': Bad address
>   fatal: cannot use (null) as an exclude file
> 
> And '--no-exclude-standard' has the same effect as
> '--exclude-standard', because its parseopt callback function
> option_parse_exclude_standard() doesn't bother to look at its 'unset'
> parameter.

I think --exclude-per-directory is fine, since it uses OPT_STRING().
Using "--no-exclude-per-directory" just means we'll cancel any
previously found option.

In an ideal world we'd perhaps do something useful with the negated
forms for the others, but I don't think the underlying code is set up to
do that (i.e., how do you undo "setup_standard_excludes()"). Possibly we
could switch to setting a flag (which could then be cleared), and
resolve the flags after parsing the whole command line. But often
options with callbacks list this have subtle user-visible timing effects
(e.g., that the command line options need to take effect in the order
they were found).

So unless somebody actually wants these negated forms to do something
useful, it's probably not worth the trouble and risk of regression.  But
we obviously should at least disallow them explicitly rather than
segfaulting.

I thought adding PARSE_OPT_NONEG like this would work:

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 88bb2019ad..9adee62358 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -547,15 +547,16 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			    N_("show resolve-undo information")),
 		{ OPTION_CALLBACK, 'x', "exclude", &exclude_list, N_("pattern"),
 			N_("skip files matching pattern"),
-			0, option_parse_exclude },
+			PARSE_OPT_NONEG, option_parse_exclude },
 		{ OPTION_CALLBACK, 'X', "exclude-from", &dir, N_("file"),
 			N_("exclude patterns are read from <file>"),
-			0, option_parse_exclude_from },
+			PARSE_OPT_NONEG, option_parse_exclude_from },
 		OPT_STRING(0, "exclude-per-directory", &dir.exclude_per_dir, N_("file"),
 			N_("read additional per-directory exclude patterns in <file>")),
 		{ OPTION_CALLBACK, 0, "exclude-standard", &dir, NULL,
 			N_("add the standard git exclusions"),
-			PARSE_OPT_NOARG, option_parse_exclude_standard },
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			option_parse_exclude_standard },
 		OPT_SET_INT_F(0, "full-name", &prefix_len,
 			      N_("make the output relative to the project top directory"),
 			      0, PARSE_OPT_NONEG),

But it actually does something quite interesting. Because of the NONEG
flag, we know that the user cannot mean "--no-exclude" itself. So our
liberal prefix-matching kicks in, and we treat it as
--no-exclude-per-directory. I.e., it becomes a silent noop and the user
gets no warning.

I think parse_options() may be overly liberal here, and we might want to
change that. But in the interim, it probably makes sense to just detect
the "unset" case in the callbacks, report an error, and return -1.

-Peff