Re: [BUG] 'git ls-files --no-exclude' segfault & co
- From
Jeff King <peff@peff.net>
- Date
- Aug 6, 2018, 16:36 UTC
- Message-ID
- <20180806163636.GA19053@sigill.intra.peff.net>
- In-Reply-To
- <CAM0VKj=DDOPDxo_xDvhk2-HUPHfkUcLQTimqmY31yjq5eZiM2Q@mail.gmail.com>
On Mon, Aug 06, 2018 at 06:07:06PM +0200, SZEDER Gábor wrote:
Show 15 quoted lines
> '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