| Message ID | e90dfe992e96b33f167d08fe51df49ab1d10ef23.1549534460.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 | 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 &&
Denton Liu <liu.denton@gmail.com> writes: > + if (argc == 3 || (argc == 2 && command == 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; This introduces decl-after-stmt. Move it before the "is it OK to write?" check. > + return config_set_in_gitmodules_file_gently(argv[1], value); > }
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; 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]"), N_("git submodule--helper config --check-writeable"), NULL }; @@ -2170,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 && command != 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 && command == 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 | 18 ++++++++++++------ t/t7411-submodule-config.sh | 9 +++++++++ 2 files changed, 21 insertions(+), 6 deletions(-)