git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG] 'git ls-files --no-exclude' segfault & co
@ 2018-08-06 16:07 SZEDER Gábor
  2018-08-06 16:36 ` Jeff King
  0 siblings, 1 reply; 2+ messages in thread
From: SZEDER Gábor @ 2018-08-06 16:07 UTC (permalink / raw)
  To: Git mailing list

'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.

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [BUG] 'git ls-files --no-exclude' segfault & co
  2018-08-06 16:07 [BUG] 'git ls-files --no-exclude' segfault & co SZEDER Gábor
@ 2018-08-06 16:36 ` Jeff King
  0 siblings, 0 replies; 2+ messages in thread
From: Jeff King @ 2018-08-06 16:36 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Git mailing list

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

^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-08-06 16:36 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-06 16:07 [BUG] 'git ls-files --no-exclude' segfault & co SZEDER Gábor
2018-08-06 16:36 ` Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).