[01/13] apply: mark include/exclude options as NONEG
diff mbox series

Message ID 20181105063819.GA25864@sigill.intra.peff.net
State New
Headers show
Series
  • parseopt fixes from -Wunused-parameters
Related show

Commit Message

Jeff King Nov. 5, 2018, 6:38 a.m. UTC
The options callback for "git apply --no-include" is not ready to handle
the "unset" parameter, and as a result will segfault when it adds a NULL
argument to the include list (likewise for "--no-exclude").

In theory this might be used to clear the list, but since both
"--include" and "--exclude" add to the same list, it's not immediately
obvious what the semantics should be. Let's punt on that for now and
just disallow the broken options.

Signed-off-by: Jeff King <peff@peff.net>
---
 apply.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Nov. 5, 2018, 7:07 a.m. UTC | #1
Jeff King <peff@peff.net> writes:

> The options callback for "git apply --no-include" is not ready to handle
> the "unset" parameter, and as a result will segfault when it adds a NULL
> argument to the include list (likewise for "--no-exclude").
>
> In theory this might be used to clear the list, but since both
> "--include" and "--exclude" add to the same list, it's not immediately
> obvious what the semantics should be. Let's punt on that for now and
> just disallow the broken options.

Thanks.  I agree with the conclusion to leave it to later outside
this series to define what --no-(include|exclude) should do.

I suspect something along the lines of

    Each element on the single list is marked as either include or
    exclude, and "--no-include" would remove the accumulated
    "include" entries in the list without touching any "exclude"
    elements.

would be sufficiently clear and useful, perhaps.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  apply.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 073d5f0451..d1ca6addeb 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,
>  	struct option builtin_apply_options[] = {
>  		{ OPTION_CALLBACK, 0, "exclude", state, N_("path"),
>  			N_("don't apply changes matching the given path"),
> -			0, apply_option_parse_exclude },
> +			PARSE_OPT_NONEG, apply_option_parse_exclude },
>  		{ OPTION_CALLBACK, 0, "include", state, N_("path"),
>  			N_("apply changes matching the given path"),
> -			0, apply_option_parse_include },
> +			PARSE_OPT_NONEG, apply_option_parse_include },
>  		{ OPTION_CALLBACK, 'p', NULL, state, N_("num"),
>  			N_("remove <num> leading slashes from traditional diff paths"),
>  			0, apply_option_parse_p },

Patch
diff mbox series

diff --git a/apply.c b/apply.c
index 073d5f0451..d1ca6addeb 100644
--- a/apply.c
+++ b/apply.c
@@ -4939,10 +4939,10 @@  int apply_parse_options(int argc, const char **argv,
 	struct option builtin_apply_options[] = {
 		{ OPTION_CALLBACK, 0, "exclude", state, N_("path"),
 			N_("don't apply changes matching the given path"),
-			0, apply_option_parse_exclude },
+			PARSE_OPT_NONEG, apply_option_parse_exclude },
 		{ OPTION_CALLBACK, 0, "include", state, N_("path"),
 			N_("apply changes matching the given path"),
-			0, apply_option_parse_include },
+			PARSE_OPT_NONEG, apply_option_parse_include },
 		{ OPTION_CALLBACK, 'p', NULL, state, N_("num"),
 			N_("remove <num> leading slashes from traditional diff paths"),
 			0, apply_option_parse_p },