| Message ID | 20220301044132.39474-14-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: convert parts of 'update' to C | expand |
On Mon, Feb 28 2022, Glen Choo wrote: > "git submodule update --filter" also requires the "--init" option. Teach > update-clone to do this usage check in C and remove the check from > git-submodule.sh. > > In addition, change update-clone's usage string so that it teaches users > about "git submodule update" instead of "git submodule--helper > update-clone" (the string is copied from git-submodule.sh). This should > be more helpful to users since they don't invoke update-clone directly. > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > Since we expect users to act upon the usage string, I've updated it to > reflect "git submodule update" [1] (since that's what users actually > invoke), but I feel a bit iffy about not being able to use > usage_with_options() (because the options and usage string are for > different commands). > > This might indicate that this is work we should put off until the > conversion to C is mostly complete, but on the other hand, the usage > string is still more helpful than it used to be because we never > presented users with the options anyway. > > [1] It's not immediately obvious which command we prefer to show - some > other commands use "git submodule--helper" and others use "git > submodule". > > builtin/submodule--helper.c | 20 +++++++++++++++++++- > git-submodule.sh | 5 ----- > 2 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 2ffc070319..3e8a05a052 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2543,7 +2543,12 @@ static int update_clone(int argc, const char **argv, const char *prefix) > }; > > const char *const git_submodule_helper_usage[] = { > - N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"), > + N_("git submodule [--quiet] update" > + "[--init [--filter=<filter-spec>]] [--remote]" > + "[-N|--no-fetch] [-f|--force]" > + "[--checkout|--merge|--rebase]" > + "[--[no-]recommend-shallow] [--reference <repository>]" > + "[--recursive] [--[no-]single-branch] [--] [<path>...]"), Since this has <repository>, <path> etc. it should still be marked for translation with N_().
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 2ffc070319..3e8a05a052 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -2543,7 +2543,12 @@ static int update_clone(int argc, const char **argv, const char *prefix) >> }; >> >> const char *const git_submodule_helper_usage[] = { >> - N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"), >> + N_("git submodule [--quiet] update" >> + "[--init [--filter=<filter-spec>]] [--remote]" >> + "[-N|--no-fetch] [-f|--force]" >> + "[--checkout|--merge|--rebase]" >> + "[--[no-]recommend-shallow] [--reference <repository>]" >> + "[--recursive] [--[no-]single-branch] [--] [<path>...]"), > > Since this has <repository>, <path> etc. it should still be marked for > translation with N_(). Yeah, that sounds like a good idea. Isn't it already inside N_()?
Junio C Hamano <gitster@pobox.com> writes: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >>> index 2ffc070319..3e8a05a052 100644 >>> --- a/builtin/submodule--helper.c >>> +++ b/builtin/submodule--helper.c >>> @@ -2543,7 +2543,12 @@ static int update_clone(int argc, const char **argv, const char *prefix) >>> }; >>> >>> const char *const git_submodule_helper_usage[] = { >>> - N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"), >>> + N_("git submodule [--quiet] update" >>> + "[--init [--filter=<filter-spec>]] [--remote]" >>> + "[-N|--no-fetch] [-f|--force]" >>> + "[--checkout|--merge|--rebase]" >>> + "[--[no-]recommend-shallow] [--reference <repository>]" >>> + "[--recursive] [--[no-]single-branch] [--] [<path>...]"), >> >> Since this has <repository>, <path> etc. it should still be marked for >> translation with N_(). > > Yeah, that sounds like a good idea. Isn't it already inside N_()? Did I do this correctly? e.g. an alternative interpretation is that Ævar misread this as: const char *const git_submodule_helper_usage[] = { N_("git submodule [--quiet] update"), "[--init [--filter=<filter-spec>]] [--remote]", "[-N|--no-fetch] [-f|--force]", "[--checkout|--merge|--rebase]", "[--[no-]recommend-shallow] [--reference <repository>]", "[--recursive] [--[no-]single-branch] [--] [<path>...]",
On Tue, Mar 01 2022, Glen Choo wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >>>> index 2ffc070319..3e8a05a052 100644 >>>> --- a/builtin/submodule--helper.c >>>> +++ b/builtin/submodule--helper.c >>>> @@ -2543,7 +2543,12 @@ static int update_clone(int argc, const char **argv, const char *prefix) >>>> }; >>>> >>>> const char *const git_submodule_helper_usage[] = { >>>> - N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"), >>>> + N_("git submodule [--quiet] update" >>>> + "[--init [--filter=<filter-spec>]] [--remote]" >>>> + "[-N|--no-fetch] [-f|--force]" >>>> + "[--checkout|--merge|--rebase]" >>>> + "[--[no-]recommend-shallow] [--reference <repository>]" >>>> + "[--recursive] [--[no-]single-branch] [--] [<path>...]"), >>> >>> Since this has <repository>, <path> etc. it should still be marked for >>> translation with N_(). >> >> Yeah, that sounds like a good idea. Isn't it already inside N_()? > > Did I do this correctly? e.g. an alternative interpretation is that Ævar > misread this as: > > const char *const git_submodule_helper_usage[] = { > N_("git submodule [--quiet] update"), > "[--init [--filter=<filter-spec>]] [--remote]", > "[-N|--no-fetch] [-f|--force]", > "[--checkout|--merge|--rebase]", > "[--[no-]recommend-shallow] [--reference <repository>]", > "[--recursive] [--[no-]single-branch] [--] [<path>...]", (Even if a v3 has already been sent out). Yes, sorry. I just misread this. The pre-image is correct.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 2ffc070319..3e8a05a052 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2543,7 +2543,12 @@ static int update_clone(int argc, const char **argv, const char *prefix) }; const char *const git_submodule_helper_usage[] = { - N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"), + N_("git submodule [--quiet] update" + "[--init [--filter=<filter-spec>]] [--remote]" + "[-N|--no-fetch] [-f|--force]" + "[--checkout|--merge|--rebase]" + "[--[no-]recommend-shallow] [--reference <repository>]" + "[--recursive] [--[no-]single-branch] [--] [<path>...]"), NULL }; suc.prefix = prefix; @@ -2554,6 +2559,19 @@ static int update_clone(int argc, const char **argv, const char *prefix) memset(&filter_options, 0, sizeof(filter_options)); argc = parse_options(argc, argv, prefix, module_update_clone_options, git_submodule_helper_usage, 0); + + if (filter_options.choice && !suc.init) { + /* + * NEEDSWORK: Don't use usage_with_options() because the + * usage string is for "git submodule update", but the + * options are for "git submodule--helper update-clone". + * + * This will no longer be an issue when "update-clone" + * is replaced by "git submodule--helper update". + */ + usage(git_submodule_helper_usage[0]); + } + suc.filter_options = &filter_options; if (update) diff --git a/git-submodule.sh b/git-submodule.sh index 51be7c7f7e..aa8bdfca9d 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -356,11 +356,6 @@ cmd_update() shift done - if test -n "$filter" && test "$init" != "1" - then - usage - fi - { git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \ ${GIT_QUIET:+--quiet} \
"git submodule update --filter" also requires the "--init" option. Teach update-clone to do this usage check in C and remove the check from git-submodule.sh. In addition, change update-clone's usage string so that it teaches users about "git submodule update" instead of "git submodule--helper update-clone" (the string is copied from git-submodule.sh). This should be more helpful to users since they don't invoke update-clone directly. Signed-off-by: Glen Choo <chooglen@google.com> --- Since we expect users to act upon the usage string, I've updated it to reflect "git submodule update" [1] (since that's what users actually invoke), but I feel a bit iffy about not being able to use usage_with_options() (because the options and usage string are for different commands). This might indicate that this is work we should put off until the conversion to C is mostly complete, but on the other hand, the usage string is still more helpful than it used to be because we never presented users with the options anyway. [1] It's not immediately obvious which command we prefer to show - some other commands use "git submodule--helper" and others use "git submodule". builtin/submodule--helper.c | 20 +++++++++++++++++++- git-submodule.sh | 5 ----- 2 files changed, 19 insertions(+), 6 deletions(-)