| Message ID | f24f20b024f4d41f106ed014a31508fab4fa5eb2.1549450636.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Teach submodule set-branch subcommand | expand |
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 | 15 +++++++++++---- > t/t7411-submodule-config.sh | 9 +++++++++ > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 0e140f176c..336e4429e6 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2149,15 +2149,21 @@ static int module_config(int argc, const char **argv, const char *prefix) > enum { > CHECK_WRITEABLE = 1 > } command = 0; > + enum { > + DO_UNSET = 1 > + } unset = 0; > > 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", &unset, > + N_("unset the config in the .gitmodules file"), > + DO_UNSET), > OPT_END() > }; The way this patch uses OPT_CMDMODE() is wrong. The situation in which CMDMODE is meant to be used is that there are multiple (that's two or more) choices the end user can make, and the end user can choose only one of them at a time, iow, giving more than one is an error. In such a case, the programmer would - prepare a single variable to store the single choice the end user makes the choice in and initialize it to zero. - have one OPT_CMDMODE() element for each valid choice, all pointing at the same variable, but with different value that is not zero. The parse_options() call would then make sure that the variable is set to non-zero value only once, to detect conflicting command modes given from the command line. The program then can read from the single variable to see if the end user made any choice (or left it 0). An example of typical and good use of OPT_CMDMODE() mechanism can be seen in builtin/tag.c; the 'l(ist)', 'd(elete)' and 'v(erify)' are the distinct operating modes of the "git tag" command (i.e. you do not delete a tag while listing them), so the &cmdmode variable is used to ensure only one of them (or none of them) is given. The existing use of OPT_CMDMODE we see in this function anticipates that there would be new operating modes added other than the check-writable mode, so if you are making a new command mode that cannot be used with the existing check-writable, then the right way to use the OPT_CMDMODE() is to add to the command enum a new non-zero value distinct from CHECK_WRITABLE, without introducing a new "unset" variable. If this --unset thing is truly independent from --check-writable, i.e. if all four possible combinations of having and not having these two options are valid, then you would not want to muck with the &command variable, but in that case, wouldn't it make more sense for the new --unset thing to simply be using OPT_BOOL()? After all, it is not like you are planning to add new oprating modes in the "unset" family that is not "--unset" in the future, no?
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 0e140f176c..336e4429e6 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2149,15 +2149,21 @@ static int module_config(int argc, const char **argv, const char *prefix) enum { CHECK_WRITEABLE = 1 } command = 0; + enum { + DO_UNSET = 1 + } unset = 0; 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", &unset, + 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]"), N_("git submodule--helper config --check-writeable"), NULL }; @@ -2169,15 +2175,16 @@ static int module_config(int argc, const char **argv, const char *prefix) return is_writing_gitmodules_ok() ? 0 : -1; /* Equivalent to ACTION_GET in builtin/config.c */ - if (argc == 2) + if (argc == 2 && unset != DO_UNSET) return print_config_from_gitmodules(the_repository, argv[1]); /* Equivalent to ACTION_SET in builtin/config.c */ - if (argc == 3) { + if (argc == 3 || (argc == 2 && unset == DO_UNSET)) { if (!is_writing_gitmodules_ok()) die(_("please make sure that the .gitmodules file is in the working tree")); - return config_set_in_gitmodules_file_gently(argv[1], argv[2]); + const char *value = (argc == 3) ? argv[2] : NULL; + return config_set_in_gitmodules_file_gently(argv[1], value); } usage_with_options(git_submodule_helper_usage, module_config_options); 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 && + test_must_be_empty actual + ) +' + + test_expect_success 'writing submodules config with "submodule--helper config"' ' (cd super && echo "new_url" >expect &&
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 | 15 +++++++++++---- t/t7411-submodule-config.sh | 9 +++++++++ 2 files changed, 20 insertions(+), 4 deletions(-)