Re: [PATCH v2 3/4] diff: use skip_to_optional_val()

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2017, 14:23 UTC
Message-ID: <xmqqh8t6o9me.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171204125617.4931-3-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 11 quoted lines

> @@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options,
>  		return stat_opt(options, av);
>  
>  	/* renames options */
> -	else if (starts_with(arg, "-B") || starts_with(arg, "--break-rewrites=") ||
> -		 !strcmp(arg, "--break-rewrites")) {
> +	else if (starts_with(arg, "-B") ||
> +		 skip_to_optional_val(arg, "--break-rewrites", &optarg)) {
>  		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
>  			return error("invalid argument to -B: %s", arg+2);
>  	}

This is curious; "optarg" gets something, but it is not used (what is passed to scoreopt_parse() is still "arg".

It merely is curious and not wrong; the actual parsing of the whole thing is done in scoreopt_parse() and skip_to_optional_val() is used merely as a substitute for "the thing must either be --break-rewrites or must begin with --break-rewrites=" check that is done with starts_with() and !strcmp().

It probably makes sense to allow skip_to_optional() to take a NULL for the third parameter to clarify a callsite like this. Otherwise the readers will wonder who consumes optarg, and why it is OK for it to be sometimes set and sometimes left undefined.

> -	else if (starts_with(arg, "-M") || starts_with(arg, "--find-renames=") ||
> -		 !strcmp(arg, "--find-renames")) {
> +	else if (starts_with(arg, "-M") ||
> +		 skip_to_optional_val(arg, "--find-renames", &optarg)) {

Likewise.

Show 11 quoted lines

> @@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options,
>  	else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) {
>  		options->irreversible_delete = 1;
>  	}
> -	else if (starts_with(arg, "-C") || starts_with(arg, "--find-copies=") ||
> -		 !strcmp(arg, "--find-copies")) {
> +	else if (starts_with(arg, "-C") ||
> +		 skip_to_optional_val(arg, "--find-copies", &optarg)) {
>  		if (options->detect_rename == DIFF_DETECT_COPY)
>  			options->flags.find_copies_harder = 1;
>  		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)

Likewise.