git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 2/3] submodule--helper: teach config subcommand --unset
Date: Thu, 07 Feb 2019 12:05:42 -0800	[thread overview]
Message-ID: <xmqqmun7nz3d.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <e90dfe992e96b33f167d08fe51df49ab1d10ef23.1549534460.git.liu.denton@gmail.com> (Denton Liu's message of "Thu, 7 Feb 2019 02:18:57 -0800")

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 &&

  reply	other threads:[~2019-02-07 20:05 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-06 10:59 [PATCH 0/3] Teach submodule set-branch subcommand Denton Liu
2019-02-06 10:59 ` [PATCH 1/3] git-submodule.txt: document default behavior without --branch Denton Liu
2019-02-06 18:46   ` Junio C Hamano
2019-02-06 10:59 ` [PATCH 2/3] submodule--helper: teach config subcommand --unset Denton Liu
2019-02-06 19:07   ` Junio C Hamano
2019-02-06 10:59 ` [PATCH 3/3] submodule: teach set-branch subcommand Denton Liu
2019-02-07  6:32 ` [PATCH v2 0/3] Teach submodule " Denton Liu
2019-02-07  6:32   ` [PATCH v2 1/3] git-submodule.txt: document default behavior without --branch Denton Liu
2019-02-07  6:32   ` [PATCH v2 2/3] submodule--helper: teach config subcommand --unset Denton Liu
2019-02-07  6:33   ` [PATCH v2 3/3] submodule: teach set-branch subcommand Denton Liu
2019-02-07 10:18   ` [PATCH v3 0/3] Teach submodule " Denton Liu
2019-02-07 10:18     ` [PATCH v3 1/3] git-submodule.txt: "--branch <branch>" option defaults to 'master' Denton Liu
2019-02-07 10:18     ` [PATCH v3 2/3] submodule--helper: teach config subcommand --unset Denton Liu
2019-02-07 20:05       ` Junio C Hamano [this message]
2019-02-07 22:29       ` Junio C Hamano
2019-02-07 10:19     ` [PATCH v3 3/3] submodule: teach set-branch subcommand Denton Liu
2019-02-07 22:26       ` Junio C Hamano
2019-02-07 18:01     ` [PATCH v3 0/3] Teach submodule " Junio C Hamano
2019-02-08  5:31       ` Denton Liu
2019-02-08 18:43         ` Junio C Hamano
2019-02-08 11:21     ` [PATCH v4 " Denton Liu
2019-02-08 11:21       ` [PATCH v4 1/3] git-submodule.txt: "--branch <branch>" option defaults to 'master' Denton Liu
2019-02-08 11:21       ` [PATCH v4 2/3] submodule--helper: teach config subcommand --unset Denton Liu
2019-02-08 11:21       ` [PATCH v4 3/3] submodule: teach set-branch subcommand Denton Liu
2019-02-08 23:51         ` Denton Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmun7nz3d.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --subject='Re: [PATCH v3 2/3] submodule--helper: teach config subcommand --unset' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).