Re: [PATCH v3 2/3] submodule--helper: teach config subcommand --unset

[Junio C Hamano <gitster@pobox.com>]

Denton Liu <liu.denton@gmail.com> writes:

> This teaches submodule--helper config the --unset option, which removes
> the specified configuration key from the .gitmodule file.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  builtin/submodule--helper.c | 18 ++++++++++++------
>  t/t7411-submodule-config.sh |  9 +++++++++
>  2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b80fc4ba3d..a86eacf167 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2148,17 +2148,22 @@ static int check_name(int argc, const char **argv, const char *prefix)
>  static int module_config(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> -		CHECK_WRITEABLE = 1
> -	} command = 0;
> +		NONE,
> +		CHECK_WRITEABLE,
> +		DO_UNSET
> +	} command = NONE;

I do not think the above, except for addition of DO_UNSET, is a good
change.  The language spec may guarantee that NONE is 0, but in the
way the original is written, it is much more obvious that integer
zero is a special value and the variable is initialized to that
special value, and that is important.  The above rewrite makes it
look as if there are a bunch of symbolic constants defined by this
particular caller and one random value NONE, whose only significance
is that it is different from any other value, is picked as its
initial value---but that is a wrong impression to give to the
readers.  parse-options.c::get_value() special cases integer zero as
"unset" for any CMDMODE, so inventing a symbolic constant used by
this particular user is counter-productive.  Needless to say, it is
not such a great idea to use such an overly generic word NONE here.

The way the original did to make sure all enum values are non-zero
(by explicitly documenting that its first value is 1) and used
literal 0 as "not specified" is much better aligned to the way how
OPT_CMDMODE() is to be used, I think.

>  
>  	struct option module_config_options[] = {
>  		OPT_CMDMODE(0, "check-writeable", &command,
>  			    N_("check if it is safe to write to the .gitmodules file"),
>  			    CHECK_WRITEABLE),
> +		OPT_CMDMODE(0, "unset", &command,
> +			    N_("unset the config in the .gitmodules file"),
> +			    DO_UNSET),
>  		OPT_END()
>  	};
>  	const char *const git_submodule_helper_usage[] = {
> -		N_("git submodule--helper config name [value]"),
> +		N_("git submodule--helper config name [--unset] [value]"),

That gives an impression that "config name --unset value" is a valid
input; isn't it more like "you can unset, or you can set to a
value"?  The way to spell it would be "... config name [--unset | value]"
but it raises a larger question.  What if you want to set to a value
that is a string "--unset"?  Actually, allowing an option that comes
after "name" (which is an arbitrary end-user supplied thing) is one
thing, but writing it in the documentation as if we are encouraging
it is probably not a good idea.  Shouldn't it be "config --unset name"?

In any case, seeing the new test in 7411, I think the best way to
write the above usage text would be to add one new line without
mucking with the existing "show it, or set it to the given value"
and add

	git submodule--helper config --unset name

as a separate item to the list.

> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 89690b7adb..fcc0fb82d8 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -142,6 +142,15 @@ test_expect_success 'reading submodules config from the working tree with "submo
>  	)
>  '
>  
> +test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
> +	(cd super &&
> +		git submodule--helper config --unset submodule.submodule.url &&
> +		git submodule--helper config submodule.submodule.url >actual &&