Re: [PATCH v3 2/3] submodule--helper: teach config subcommand --unset
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 7, 2019, 20:05 UTC
- Message-ID
- <xmqqmun7nz3d.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <e90dfe992e96b33f167d08fe51df49ab1d10ef23.1549534460.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
Show 23 quoted lines
> 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.
Show 13 quoted lines
>
> 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.
Show 12 quoted lines
> 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 &&