| Message ID | patch-5.8-2b8afd73b9b-20221102T074148Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: tests, cleanup to prepare for built-in | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Remove the "----recursive" option to "git submodule--helper > absorbgitdirs" (yes, with 4 dashes, not 2). o.O At least this makes it pretty easy to grep for usage, and it makes sense that we've never used it (otherwise this would've been caught). > diff --git a/submodule.c b/submodule.c > index fe1e3f03905..8fa2ad457b2 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path) > * having its git directory within the working tree to the git dir nested > * in its superprojects git dir under modules/. > */ > -void absorb_git_dir_into_superproject(const char *path, > - unsigned flags) > +void absorb_git_dir_into_superproject(const char *path) > { > int err_code; > const char *sub_git_dir; > @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path, > } > strbuf_release(&gitdir); > > - if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) { > - if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES) > - BUG("we don't know how to pass the flags down?"); > - > - absorb_git_dir_into_superproject_recurse(path); > - } > + absorb_git_dir_into_superproject_recurse(path); > } Maybe I'm misreading, but I don't follow this change. Before, we recursed into the submodule only if the ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is never), but now we unconditionally recurse into the submodule. Wouldn't it be more accurate to get rid of this recursing behavior altogether? i.e. drop the previous patch and delete that code when we delete this conditional.
On Thu, Nov 03 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Remove the "----recursive" option to "git submodule--helper >> absorbgitdirs" (yes, with 4 dashes, not 2). > > o.O > > At least this makes it pretty easy to grep for usage, and it makes sense > that we've never used it (otherwise this would've been caught). > >> diff --git a/submodule.c b/submodule.c >> index fe1e3f03905..8fa2ad457b2 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path) >> * having its git directory within the working tree to the git dir nested >> * in its superprojects git dir under modules/. >> */ >> -void absorb_git_dir_into_superproject(const char *path, >> - unsigned flags) >> +void absorb_git_dir_into_superproject(const char *path) >> { >> int err_code; >> const char *sub_git_dir; >> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path, >> } >> strbuf_release(&gitdir); >> >> - if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) { >> - if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES) >> - BUG("we don't know how to pass the flags down?"); >> - >> - absorb_git_dir_into_superproject_recurse(path); >> - } >> + absorb_git_dir_into_superproject_recurse(path); >> } > > Maybe I'm misreading, but I don't follow this change. > > Before, we recursed into the submodule only if the > ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is > never), but now we unconditionally recurse into the submodule. No, it's always set. I.e. ----recursive did nothing, but the default was to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set (and there was no --no---recursive user). So we should be unconditionally going on this recursive path.
Hello. I do not wish to get these messages any longer. Thank you. Phyllis P Simpson | Application Systems Analyst Programmer - Int Office of Technology & Cybersecurity office: 317-233-8477 psimpson@health.in.gov health.in.gov -----Original Message----- From: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Sent: Thursday, November 3, 2022 9:43 PM To: Glen Choo <chooglen@google.com> Cc: git@vger.kernel.org; Junio C Hamano <gitster@pobox.com>; Emily Shaffer <emilyshaffer@google.com> Subject: Re: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option **** This is an EXTERNAL email. Exercise caution. DO NOT open attachments or click links from unknown senders or unexpected email. **** ________________________________ On Thu, Nov 03 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Remove the "----recursive" option to "git submodule--helper >> absorbgitdirs" (yes, with 4 dashes, not 2). > > o.O > > At least this makes it pretty easy to grep for usage, and it makes > sense that we've never used it (otherwise this would've been caught). > >> diff --git a/submodule.c b/submodule.c index fe1e3f03905..8fa2ad457b2 >> 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path) >> * having its git directory within the working tree to the git dir nested >> * in its superprojects git dir under modules/. >> */ >> -void absorb_git_dir_into_superproject(const char *path, >> - unsigned flags) >> +void absorb_git_dir_into_superproject(const char *path) >> { >> int err_code; >> const char *sub_git_dir; >> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path, >> } >> strbuf_release(&gitdir); >> >> - if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) { >> - if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES) >> - BUG("we don't know how to pass the flags down?"); >> - >> - absorb_git_dir_into_superproject_recurse(path); >> - } >> + absorb_git_dir_into_superproject_recurse(path); >> } > > Maybe I'm misreading, but I don't follow this change. > > Before, we recursed into the submodule only if the > ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is > never), but now we unconditionally recurse into the submodule. No, it's always set. I.e. ----recursive did nothing, but the default was to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set (and there was no --no---recursive user). So we should be unconditionally going on this recursive path.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Nov 03 2022, Glen Choo wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> Remove the "----recursive" option to "git submodule--helper >>> absorbgitdirs" (yes, with 4 dashes, not 2). >> >> o.O >> >> At least this makes it pretty easy to grep for usage, and it makes sense >> that we've never used it (otherwise this would've been caught). >> >>> diff --git a/submodule.c b/submodule.c >>> index fe1e3f03905..8fa2ad457b2 100644 >>> --- a/submodule.c >>> +++ b/submodule.c >>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path) >>> * having its git directory within the working tree to the git dir nested >>> * in its superprojects git dir under modules/. >>> */ >>> -void absorb_git_dir_into_superproject(const char *path, >>> - unsigned flags) >>> +void absorb_git_dir_into_superproject(const char *path) >>> { >>> int err_code; >>> const char *sub_git_dir; >>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path, >>> } >>> strbuf_release(&gitdir); >>> >>> - if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) { >>> - if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES) >>> - BUG("we don't know how to pass the flags down?"); >>> - >>> - absorb_git_dir_into_superproject_recurse(path); >>> - } >>> + absorb_git_dir_into_superproject_recurse(path); >>> } >> >> Maybe I'm misreading, but I don't follow this change. >> >> Before, we recursed into the submodule only if the >> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is >> never), but now we unconditionally recurse into the submodule. > > No, it's always set. I.e. ----recursive did nothing, but the default was > to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set > (and there was no --no---recursive user). > > So we should be unconditionally going on this recursive path. Ah, because we initialize flags to ABSORB_GITDIR_RECURSE_SUBMODULES. I see that this is also covered by t/t7412-submodule-absorbgitdirs.sh, which has a few nested submodules tests. Thanks! Since it's clear that recursing should be unconditional, I think we don't need the previous patch, but I'm fine either way.
diff --git a/builtin/rm.c b/builtin/rm.c index f0d025a4e23..05bfe20a469 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -86,8 +86,7 @@ static void submodules_absorb_gitdir_if_needed(void) continue; if (!submodule_uses_gitfile(name)) - absorb_git_dir_into_superproject(name, - ABSORB_GITDIR_RECURSE_SUBMODULES); + absorb_git_dir_into_superproject(name); } } diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6250b95a6f7..8b4af8430dc 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1378,8 +1378,7 @@ static void deinit_submodule(const char *path, const char *prefix, ".git file by using absorbgitdirs."), displaypath); - absorb_git_dir_into_superproject(path, - ABSORB_GITDIR_RECURSE_SUBMODULES); + absorb_git_dir_into_superproject(path); } @@ -2830,13 +2829,10 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) int i; struct pathspec pathspec = { 0 }; struct module_list list = MODULE_LIST_INIT; - unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES; struct option embed_gitdir_options[] = { OPT_STRING(0, "prefix", &prefix, N_("path"), N_("path into the working tree")), - OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"), - ABSORB_GITDIR_RECURSE_SUBMODULES), OPT_END() }; const char *const git_submodule_helper_usage[] = { @@ -2852,7 +2848,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) goto cleanup; for (i = 0; i < list.nr; i++) - absorb_git_dir_into_superproject(list.entries[i]->name, flags); + absorb_git_dir_into_superproject(list.entries[i]->name); ret = 0; cleanup: diff --git a/submodule.c b/submodule.c index fe1e3f03905..8fa2ad457b2 100644 --- a/submodule.c +++ b/submodule.c @@ -2139,8 +2139,7 @@ int submodule_move_head(const char *path, if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) { if (old_head) { if (!submodule_uses_gitfile(path)) - absorb_git_dir_into_superproject(path, - ABSORB_GITDIR_RECURSE_SUBMODULES); + absorb_git_dir_into_superproject(path); } else { struct strbuf gitdir = STRBUF_INIT; submodule_name_to_gitdir(&gitdir, the_repository, @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path) * having its git directory within the working tree to the git dir nested * in its superprojects git dir under modules/. */ -void absorb_git_dir_into_superproject(const char *path, - unsigned flags) +void absorb_git_dir_into_superproject(const char *path) { int err_code; const char *sub_git_dir; @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path, } strbuf_release(&gitdir); - if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) { - if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES) - BUG("we don't know how to pass the flags down?"); - - absorb_git_dir_into_superproject_recurse(path); - } + absorb_git_dir_into_superproject_recurse(path); } int get_superproject_working_tree(struct strbuf *buf) diff --git a/submodule.h b/submodule.h index 6a9fec6de11..b52a4ff1e73 100644 --- a/submodule.h +++ b/submodule.h @@ -164,9 +164,7 @@ void submodule_unset_core_worktree(const struct submodule *sub); */ void prepare_submodule_repo_env(struct strvec *env); -#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0) -void absorb_git_dir_into_superproject(const char *path, - unsigned flags); +void absorb_git_dir_into_superproject(const char *path); /* * Return the absolute path of the working tree of the superproject, which this
Remove the "----recursive" option to "git submodule--helper absorbgitdirs" (yes, with 4 dashes, not 2). This option and all the "else" when "flags & ABSORB_GITDIR_RECURSE_SUBMODULES" is false has never been used since it was added in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12), which we'd have had to do as "----recursive", a "--recursive" would have errored out. It would be nice to follow-up with an optbug() assertion to parse-options.c for such funnily named options, I manually validated that this was the only long option whose name started with "-", but let's skip adding such an assertion for now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/rm.c | 3 +-- builtin/submodule--helper.c | 8 ++------ submodule.c | 13 +++---------- submodule.h | 4 +--- 4 files changed, 7 insertions(+), 21 deletions(-)