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

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

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

Show 5 quoted lines

> -	} else if (!strcmp(arg, "--submodule"))
> -		options->submodule_format = DIFF_SUBMODULE_LOG;
> -	else if (skip_prefix(arg, "--submodule=", &arg))
> +	} else if (skip_to_optional_val_default(arg, "--submodule", &arg, "log"))
>  		return parse_submodule_opt(options, arg);

It may be annoying if we later diagnose that DIFF_SUBMODULE_LOG may not be used in the context (e.g. together with some other optoins that are set in "options" variable) and have to throw an error message at the user. parse_submodule_opt() would not know if that string "log" came from the user or substituted by the helper, and in the latter case, "don't use --submodule=log here" is a message that is not quite appropriate.

This may be minor enough not matter in practice, but I dunno. I didn't look at other hunks in this step very carefully but I would expect that whenever you use "default" to substitute, you'll have the same information lossage.