| Message ID | pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | submodule: remove "--recursive-prefix" | expand |
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes: > Glen Choo (6): > submodule--helper: eliminate internal "--update" option > submodule--helper update: use display path helper > submodule--helper: don't recreate recursive prefix > submodule--helper: use correct display path helper > submodule--helper update: use --super-prefix > submodule--helper: remove display path helper > > Ævar Arnfjörð Bjarmason (12): > git-submodule.sh: remove unused sanitize_submodule_env() > git-submodule.sh: remove unused $prefix variable > git-submodule.sh: make the "$cached" variable a boolean > git-submodule.sh: remove unused top-level "--branch" argument > submodule--helper: have --require-init imply --init > submodule update: remove "-v" option > submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs" > submodule--helper: report "submodule" as our name in some "-h" output > submodule--helper: understand --checkout, --merge and --rebase > synonyms > git-submodule.sh: use "$quiet", not "$GIT_QUIET" > git-sh-setup.sh: remove "say" function, change last users > submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags Looks like I accidentally submitted all of ab/submodule-cleanup... Let me try this again without GGG.
On Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote: > Thanks Ævar for the feedback! I've incorporated all of the suggestions, > except breaking up the tests in 1/6 into their own patch - I wasn't > convinced of the value of having prescriptive tests (with > test_expect_failure), and I found it difficult to discuss descriptive tests > (with test_expect_success) without also having the code in the same diff. > FWIW, this version (and the previous one) "git rebase -x 'make test'" > cleanly :) > > Note for Junio: this version is based on ab/submodule-cleanup (and so will > future versions). > > = Description > > This series is a refactor of "git submodule--helper update" that replaces > "--recursive-prefix" with "--super-prefix" (see Background). This was > initially motivated by: > > * Junio's suggestion to simplify the code [1] (in response to a memory leak > found by Johannes Schindelin [2]). > * A desire to use the module_list API + get_submodule_displaypath() outside > of builtin/submodule--helper.c (I expect to use this to check out > branches in each submodule). > > But it also happens to remove some overly complicated/duplicated code that > was literally converted from shell :) > > = Background > > When recursing into nested submodules, Git commands keep track of the path > from the superproject to the submodule in order to print a "display path" to > the user, e.g. > > Submodule path '../super/sub/nested-sub': checked out 'abcdef' > > For historical reasons, "git submodule--helper update" uses > "--recursive-prefix" for this purpose, but it should use "--super-prefix" > instead because: > > * That's what every other command uses (not just submodule--helper > subcommands). > * Using the "--super-prefix" helper functions makes the "git > submodule--helper update" code simpler > > = Patch organization > > * Patch 1/6 makes sure that display paths are only computed using display > path helper functions ([do_]get_submodule_displaypath()) and fixes some > display paths that we never realized were broken. > * Patches 2-3/6 simplify and deduplicate some display path computations. > * Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not > needed. > * This doesn't affect correctness, but we want to do this eventually, so > do it now to make 5/6 a bit cleaner. > * Patch 5/6 replaces "--recursive-prefix" with "--super-prefix", making > do_get_submodule_displaypath() obsolete. > * GGG outputs an odd diff; I recommend viewing it with "--histogram" > * Patch 6/6 removes do_get_submodule_displaypath(). > > = Series history > > Changes in v2: > > * Rebase onto ab/submodule-cleanup (previously master) > * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6 > * Style fixes in .c and tests Thanks for the update, Looks like something happened with GGG to send the base series + yours, I confirmed that 1-13/18 are the same as my series in gitster/ab/submodule-cleanup. I played around with this a bit, and pushed some amends to https://github.com/avar/git/tree/avar/pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2; which you should take more as the results of poking around, I think this is fine as-is if Junio would like to pick it up, sans the possible bug mentioned below. A range-diff of the changes I made follows below, changes: * Split up the "add tests" to a new commit :) You mentioned "test_expect_failure", I just assumed we'd use test_expect_succes, and then the 2nd commit has this test change: diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 9a076e025f3..6cc07460dd2 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1163 +1163 @@ test_expect_success 'submodule update should skip unmerged submodules' ' - Skipping unmerged submodule middle//bottom + Skipping unmerged submodule middle/bottom @@ -1176 +1176 @@ test_expect_success 'submodule update --recursive skip submodules with strategy= - Skipping submodule '\''../middle/'\'' + Skipping submodule '\''middle/bottom'\'' Which I think makes it much easier to explain the change itself, the first commit basically just says "tests for existing behavior, see the next commit for details". * Used test_cmp instead of "grep -F", makes for easier to grok output :) * I had to squint a bit at the change from strvec_pushl() to strvec_pushf(). I.e. we're changing both that we're passing "--foo bar" v.s. "--foo=bar" *and* "bar" v.s. "bar/", but as the commit message notes it's just the latter that matters. Just a nit, but I think it's a bit easier to reason about just changing the one thing we need to change there, although that means giving the "if" braces. This also leaves the "--super-prefix" code consistent with all the other places where we "strvec.*--super-prefix" at the end. * We can just declare some of the variables and initialize them at the beginning, but couldn't when they were a strbuf. * Your 18/18 has some odd code formatting of "var =\n\t\tvalue(..." when we usually do "var = value(...\n...". * You appear to have (and I haven't "fixed" it here) in 13/18 mentioned "While we're fixing the display names, also fix inconsistent quoting of the submodule name", but as the test_cmp shows we appear not to have done that part at all? Is this referring to a forgotten change where we should be saying (note the added '-quotes): Skipping unmerged submodule 'middle/bottom' Either that's what you mean & you forgot, or we're missing a test, or I'm misunderstanding it... I hope this helps, at least some of it... 1: 64c138df196 ! 1: 3d9977006d3 submodule--helper update: use display path helper @@ Metadata Author: Glen Choo <chooglen@google.com> ## Commit message ## - submodule--helper update: use display path helper + submodule--helper tests: add missing "display path" coverage There are two locations in prepare_to_clone_next_submodule() that - manually calculate the submodule display path, but should just use - do_get_submodule_displaypath() for consistency. - - Do this replacement and reorder the code slightly to avoid computing - the display path twice. - - This code was never tested, and adding tests shows that both these sites - have been computing the display path incorrectly ever since they were - introduced in 48308681b0 (git submodule update: have a dedicated helper - for cloning, 2016-02-29) [1]: - - - The first hunk puts a "/" between recursive_prefix and ce->name, but - recursive_prefix already ends with "/". - - The second hunk calls relative_path() on recursive_prefix and - ce->name, but relative_path() only makes sense when both paths share - the same base directory. This is never the case here: - - recursive_prefix is the path from the topmost superproject to the - current submodule - - ce->name is the path from the root of the current submodule to its - submodule. - so, e.g. recursive_prefix="super" and ce->name="submodule" produces - displayname="../super" instead of "super/submodule". - - While we're fixing the display names, also fix inconsistent quoting of - the submodule name. - - [1] I verified this by applying the tests to 48308681b0. + manually calculate the submodule display path, as discussed in the + next commit the "Skipping" output isn't exactly what we want, but + let's test how we behave now, before changing the existing behavior. Signed-off-by: Glen Choo <chooglen@google.com> - - ## builtin/submodule--helper.c ## -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, - const char *update_string; - enum submodule_update_type update_type; - char *key; -- struct strbuf displaypath_sb = STRBUF_INIT; - struct strbuf sb = STRBUF_INIT; -- const char *displaypath = NULL; -+ char *displaypath; - int needs_cloning = 0; - int need_free_url = 0; - -+ displaypath = do_get_submodule_displaypath(ce->name, -+ suc->update_data->prefix, -+ suc->update_data->recursive_prefix); -+ - if (ce_stage(ce)) { -- if (suc->update_data->recursive_prefix) -- strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name); -- else -- strbuf_addstr(&sb, ce->name); -- strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf); -+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); - strbuf_addch(out, '\n'); - goto cleanup; - } - - sub = submodule_from_path(the_repository, null_oid(), ce->name); - -- if (suc->update_data->recursive_prefix) -- displaypath = relative_path(suc->update_data->recursive_prefix, -- ce->name, &displaypath_sb); -- else -- displaypath = ce->name; -- - if (!sub) { - next_submodule_warn_missing(suc, out, displaypath); - goto cleanup; -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, - "--no-single-branch"); - - cleanup: -- strbuf_release(&displaypath_sb); -+ free(displaypath); - strbuf_release(&sb); - if (need_free_url) - free((void*)url); + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## t/t7406-submodule-update.sh ## @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' ' @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets + # because of its unmerged state + test_config -C top-cloned submodule.middle.update !true && + git -C top-cloned submodule update --recursive 2>actual.err && -+ grep -F "Skipping unmerged submodule middle/bottom" actual.err ++ cat >expect.err <<-\EOF && ++ Skipping unmerged submodule middle//bottom ++ EOF ++ test_cmp expect.err actual.err +' + +test_expect_success 'submodule update --recursive skip submodules with strategy=none' ' @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets + test_commit -C top-cloned/middle/bottom downstream_commit && + git -C top-cloned/middle config submodule.bottom.update none && + git -C top-cloned submodule update --recursive 2>actual.err && -+ grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err ++ cat >expect.err <<-\EOF && ++ Skipping submodule '\''../middle/'\'' ++ EOF ++ test_cmp expect.err actual.err +' + test_done -: ----------- > 2: d1a47d302ee submodule--helper update: use display path helper 2: d3e803a4630 ! 3: a5b30ac94e6 submodule--helper: don't recreate recursive prefix @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data - if (update_data->recursive_prefix) - strvec_pushl(args, "--recursive-prefix", - update_data->recursive_prefix, NULL); -+ if (update_data->displaypath) -+ strvec_pushf(args, "--recursive-prefix=%s/", -+ update_data->displaypath); ++ if (update_data->displaypath) { ++ strvec_push(args, "--recursive-prefix"); ++ strvec_pushf(args, "%s/", update_data->displaypath); ++ } if (update_data->quiet) strvec_push(args, "--quiet"); if (update_data->force) 3: 1f7cf6ffaf1 = 4: 0ea102882a7 submodule--helper: use correct display path helper 4: 85e65f143b6 = 5: 8085bc83a0c submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags 5: 1d8b6b244dc ! 6: 4c00a1b496a submodule--helper update: use --super-prefix @@ builtin/submodule--helper.c: struct submodule_update_clone { enum submodule_update_type update_default; struct object_id suboid; @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, - - displaypath = do_get_submodule_displaypath(ce->name, - suc->update_data->prefix, -- suc->update_data->recursive_prefix); -+ get_super_prefix()); - - if (ce_stage(ce)) { - strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); + char *key; + struct update_data *ud = suc->update_data; + char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix, +- ud->recursive_prefix); ++ get_super_prefix()); + struct strbuf sb = STRBUF_INIT; + int needs_cloning = 0; + int need_free_url = 0; @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec * { enum submodule_update_type update_type = update_data->update_default; - strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL); - strvec_pushf(args, "--jobs=%d", update_data->max_jobs); - if (update_data->displaypath) -- strvec_pushf(args, "--recursive-prefix=%s/", -+ strvec_pushf(args, "--super-prefix=%s/", - update_data->displaypath); + if (update_data->displaypath) { +- strvec_push(args, "--recursive-prefix"); ++ strvec_push(args, "--super-prefix"); + strvec_pushf(args, "%s/", update_data->displaypath); + } + strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL); + strvec_pushf(args, "--jobs=%d", update_data->max_jobs); if (update_data->quiet) 6: a21e8cd174d ! 7: 639f07e4b84 submodule--helper: remove display path helper @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const sub = submodule_from_path(the_repository, null_oid(), path); @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, + enum submodule_update_type update_type; + char *key; + struct update_data *ud = suc->update_data; +- char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix, +- get_super_prefix()); ++ char *displaypath = get_submodule_displaypath(ce->name, ud->prefix); + struct strbuf sb = STRBUF_INIT; int needs_cloning = 0; int need_free_url = 0; - -- displaypath = do_get_submodule_displaypath(ce->name, -- suc->update_data->prefix, -- get_super_prefix()); -+ displaypath = -+ get_submodule_displaypath(ce->name, suc->update_data->prefix); - - if (ce_stage(ce)) { - strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) { ensure_core_worktree(update_data->sm_path); q
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote: > >> = Series history >> >> Changes in v2: >> >> * Rebase onto ab/submodule-cleanup (previously master) >> * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6 >> * Style fixes in .c and tests > > Thanks for the update, Looks like something happened with GGG to send > the base series + yours, I confirmed that 1-13/18 are the same as my > series in gitster/ab/submodule-cleanup. Yes, oops. I resent this as v3 (sans your base), but you can ignore that :P > I played around with this a bit, and pushed some amends to > https://github.com/avar/git/tree/avar/pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2; > which you should take more as the results of poking around, I think this > is fine as-is if Junio would like to pick it up, sans the possible bug > mentioned below. > > A range-diff of the changes I made follows below, changes: > > * Split up the "add tests" to a new commit :) You mentioned > "test_expect_failure", I just assumed we'd use test_expect_succes, > and then the 2nd commit has this test change: > > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > index 9a076e025f3..6cc07460dd2 100755 > --- a/t/t7406-submodule-update.sh > +++ b/t/t7406-submodule-update.sh > @@ -1163 +1163 @@ test_expect_success 'submodule update should skip unmerged submodules' ' > - Skipping unmerged submodule middle//bottom > + Skipping unmerged submodule middle/bottom > @@ -1176 +1176 @@ test_expect_success 'submodule update --recursive skip submodules with strategy= > - Skipping submodule '\''../middle/'\'' > + Skipping submodule '\''middle/bottom'\'' > > Which I think makes it much easier to explain the change itself, the > first commit basically just says "tests for existing behavior, see > the next commit for details". Ah, I see what you meant now. Makes sense. > * Used test_cmp instead of "grep -F", makes for easier to grok output :) > > * I had to squint a bit at the change from strvec_pushl() to > strvec_pushf(). > > I.e. we're changing both that we're passing "--foo bar" > v.s. "--foo=bar" *and* "bar" v.s. "bar/", but as the commit message > notes it's just the latter that matters. > > Just a nit, but I think it's a bit easier to reason about just > changing the one thing we need to change there, although that means > giving the "if" braces. This also leaves the "--super-prefix" code > consistent with all the other places where we > "strvec.*--super-prefix" at the end. Both sound good. > * We can just declare some of the variables and initialize them at the > beginning, but couldn't when they were a strbuf. I think this is just "char *displaypath" in 2/7 aka d1a47d302e (submodule--helper update: use display path helper, 2022-06-28)? That change makes sense. > * Your 18/18 has some odd code formatting of "var =\n\t\tvalue(..." > when we usually do "var = value(...\n...". Thanks for catching that. I could have sworn that came from "make style". > * You appear to have (and I haven't "fixed" it here) in 13/18 mentioned > "While we're fixing the display names, also fix inconsistent quoting > of the submodule name", but as the test_cmp shows we appear not to > have done that part at all? Is this referring to a forgotten change > where we should be saying (note the added '-quotes): > > Skipping unmerged submodule 'middle/bottom' > > Either that's what you mean & you forgot, or we're missing a test, or > I'm misunderstanding it... Yes, this is a genuine error. In v2, I dropped this change (as you suggested) and I neglected to amend the comit message.. > I hope this helps, at least some of it... Thanks! Yes it helps a lot. If you don't mind, I'll roll your commits + the commit message fix as v4. > 1: 64c138df196 ! 1: 3d9977006d3 submodule--helper update: use display path helper > @@ Metadata > Author: Glen Choo <chooglen@google.com> > > ## Commit message ## > - submodule--helper update: use display path helper > + submodule--helper tests: add missing "display path" coverage > > There are two locations in prepare_to_clone_next_submodule() that > - manually calculate the submodule display path, but should just use > - do_get_submodule_displaypath() for consistency. > - > - Do this replacement and reorder the code slightly to avoid computing > - the display path twice. > - > - This code was never tested, and adding tests shows that both these sites > - have been computing the display path incorrectly ever since they were > - introduced in 48308681b0 (git submodule update: have a dedicated helper > - for cloning, 2016-02-29) [1]: > - > - - The first hunk puts a "/" between recursive_prefix and ce->name, but > - recursive_prefix already ends with "/". > - - The second hunk calls relative_path() on recursive_prefix and > - ce->name, but relative_path() only makes sense when both paths share > - the same base directory. This is never the case here: > - - recursive_prefix is the path from the topmost superproject to the > - current submodule > - - ce->name is the path from the root of the current submodule to its > - submodule. > - so, e.g. recursive_prefix="super" and ce->name="submodule" produces > - displayname="../super" instead of "super/submodule". > - > - While we're fixing the display names, also fix inconsistent quoting of > - the submodule name. > - > - [1] I verified this by applying the tests to 48308681b0. > + manually calculate the submodule display path, as discussed in the > + next commit the "Skipping" output isn't exactly what we want, but > + let's test how we behave now, before changing the existing behavior. > > Signed-off-by: Glen Choo <chooglen@google.com> > - > - ## builtin/submodule--helper.c ## > -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > - const char *update_string; > - enum submodule_update_type update_type; > - char *key; > -- struct strbuf displaypath_sb = STRBUF_INIT; > - struct strbuf sb = STRBUF_INIT; > -- const char *displaypath = NULL; > -+ char *displaypath; > - int needs_cloning = 0; > - int need_free_url = 0; > - > -+ displaypath = do_get_submodule_displaypath(ce->name, > -+ suc->update_data->prefix, > -+ suc->update_data->recursive_prefix); > -+ > - if (ce_stage(ce)) { > -- if (suc->update_data->recursive_prefix) > -- strbuf_addf(&sb, "%s/%s", suc->update_data->recursive_prefix, ce->name); > -- else > -- strbuf_addstr(&sb, ce->name); > -- strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf); > -+ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); > - strbuf_addch(out, '\n'); > - goto cleanup; > - } > - > - sub = submodule_from_path(the_repository, null_oid(), ce->name); > - > -- if (suc->update_data->recursive_prefix) > -- displaypath = relative_path(suc->update_data->recursive_prefix, > -- ce->name, &displaypath_sb); > -- else > -- displaypath = ce->name; > -- > - if (!sub) { > - next_submodule_warn_missing(suc, out, displaypath); > - goto cleanup; > -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > - "--no-single-branch"); > - > - cleanup: > -- strbuf_release(&displaypath_sb); > -+ free(displaypath); > - strbuf_release(&sb); > - if (need_free_url) > - free((void*)url); > + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## t/t7406-submodule-update.sh ## > @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' ' > @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets > + # because of its unmerged state > + test_config -C top-cloned submodule.middle.update !true && > + git -C top-cloned submodule update --recursive 2>actual.err && > -+ grep -F "Skipping unmerged submodule middle/bottom" actual.err > ++ cat >expect.err <<-\EOF && > ++ Skipping unmerged submodule middle//bottom > ++ EOF > ++ test_cmp expect.err actual.err > +' > + > +test_expect_success 'submodule update --recursive skip submodules with strategy=none' ' > @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets > + test_commit -C top-cloned/middle/bottom downstream_commit && > + git -C top-cloned/middle config submodule.bottom.update none && > + git -C top-cloned submodule update --recursive 2>actual.err && > -+ grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err > ++ cat >expect.err <<-\EOF && > ++ Skipping submodule '\''../middle/'\'' > ++ EOF > ++ test_cmp expect.err actual.err > +' > + > test_done > -: ----------- > 2: d1a47d302ee submodule--helper update: use display path helper > 2: d3e803a4630 ! 3: a5b30ac94e6 submodule--helper: don't recreate recursive prefix > @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data > - if (update_data->recursive_prefix) > - strvec_pushl(args, "--recursive-prefix", > - update_data->recursive_prefix, NULL); > -+ if (update_data->displaypath) > -+ strvec_pushf(args, "--recursive-prefix=%s/", > -+ update_data->displaypath); > ++ if (update_data->displaypath) { > ++ strvec_push(args, "--recursive-prefix"); > ++ strvec_pushf(args, "%s/", update_data->displaypath); > ++ } > if (update_data->quiet) > strvec_push(args, "--quiet"); > if (update_data->force) > 3: 1f7cf6ffaf1 = 4: 0ea102882a7 submodule--helper: use correct display path helper > 4: 85e65f143b6 = 5: 8085bc83a0c submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags > 5: 1d8b6b244dc ! 6: 4c00a1b496a submodule--helper update: use --super-prefix > @@ builtin/submodule--helper.c: struct submodule_update_clone { > enum submodule_update_type update_default; > struct object_id suboid; > @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > - > - displaypath = do_get_submodule_displaypath(ce->name, > - suc->update_data->prefix, > -- suc->update_data->recursive_prefix); > -+ get_super_prefix()); > - > - if (ce_stage(ce)) { > - strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); > + char *key; > + struct update_data *ud = suc->update_data; > + char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix, > +- ud->recursive_prefix); > ++ get_super_prefix()); > + struct strbuf sb = STRBUF_INIT; > + int needs_cloning = 0; > + int need_free_url = 0; > @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec * > { > enum submodule_update_type update_type = update_data->update_default; > > - strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL); > - strvec_pushf(args, "--jobs=%d", update_data->max_jobs); > - if (update_data->displaypath) > -- strvec_pushf(args, "--recursive-prefix=%s/", > -+ strvec_pushf(args, "--super-prefix=%s/", > - update_data->displaypath); > + if (update_data->displaypath) { > +- strvec_push(args, "--recursive-prefix"); > ++ strvec_push(args, "--super-prefix"); > + strvec_pushf(args, "%s/", update_data->displaypath); > + } > + strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL); > + strvec_pushf(args, "--jobs=%d", update_data->max_jobs); > if (update_data->quiet) > 6: a21e8cd174d ! 7: 639f07e4b84 submodule--helper: remove display path helper > @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const > sub = submodule_from_path(the_repository, null_oid(), path); > > @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > + enum submodule_update_type update_type; > + char *key; > + struct update_data *ud = suc->update_data; > +- char *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix, > +- get_super_prefix()); > ++ char *displaypath = get_submodule_displaypath(ce->name, ud->prefix); > + struct strbuf sb = STRBUF_INIT; > int needs_cloning = 0; > int need_free_url = 0; > - > -- displaypath = do_get_submodule_displaypath(ce->name, > -- suc->update_data->prefix, > -- get_super_prefix()); > -+ displaypath = > -+ get_submodule_displaypath(ce->name, suc->update_data->prefix); > - > - if (ce_stage(ce)) { > - strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); > @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) > { > ensure_core_worktree(update_data->sm_path);
Thanks Ævar for the feedback! I've incorporated all of the suggestions, except breaking up the tests in 1/6 into their own patch - I wasn't convinced of the value of having prescriptive tests (with test_expect_failure), and I found it difficult to discuss descriptive tests (with test_expect_success) without also having the code in the same diff. FWIW, this version (and the previous one) "git rebase -x 'make test'" cleanly :) Note for Junio: this version is based on ab/submodule-cleanup (and so will future versions). = Description This series is a refactor of "git submodule--helper update" that replaces "--recursive-prefix" with "--super-prefix" (see Background). This was initially motivated by: * Junio's suggestion to simplify the code [1] (in response to a memory leak found by Johannes Schindelin [2]). * A desire to use the module_list API + get_submodule_displaypath() outside of builtin/submodule--helper.c (I expect to use this to check out branches in each submodule). But it also happens to remove some overly complicated/duplicated code that was literally converted from shell :) = Background When recursing into nested submodules, Git commands keep track of the path from the superproject to the submodule in order to print a "display path" to the user, e.g. Submodule path '../super/sub/nested-sub': checked out 'abcdef' For historical reasons, "git submodule--helper update" uses "--recursive-prefix" for this purpose, but it should use "--super-prefix" instead because: * That's what every other command uses (not just submodule--helper subcommands). * Using the "--super-prefix" helper functions makes the "git submodule--helper update" code simpler = Patch organization * Patch 1/6 makes sure that display paths are only computed using display path helper functions ([do_]get_submodule_displaypath()) and fixes some display paths that we never realized were broken. * Patches 2-3/6 simplify and deduplicate some display path computations. * Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not needed. * This doesn't affect correctness, but we want to do this eventually, so do it now to make 5/6 a bit cleaner. * Patch 5/6 replaces "--recursive-prefix" with "--super-prefix", making do_get_submodule_displaypath() obsolete. * GGG outputs an odd diff; I recommend viewing it with "--histogram" * Patch 6/6 removes do_get_submodule_displaypath(). = Series history Changes in v2: * Rebase onto ab/submodule-cleanup (previously master) * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6 * Style fixes in .c and tests = Future work At the end of this series, get_submodule_displaypath() can be moved to submodule.h, which would make submodule.c:get_super_prefix_or_empty() obsolete. I have a patch that demonstrates this (CI run: [4]), but I decided to keep this series more focused on "git submodule--helper update" so that it's easier to review. [1] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g [2] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com [3] https://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com [4] https://github.com/chooglen/git/actions/runs/2572557584 Glen Choo (6): submodule--helper: eliminate internal "--update" option submodule--helper update: use display path helper submodule--helper: don't recreate recursive prefix submodule--helper: use correct display path helper submodule--helper update: use --super-prefix submodule--helper: remove display path helper Ævar Arnfjörð Bjarmason (12): git-submodule.sh: remove unused sanitize_submodule_env() git-submodule.sh: remove unused $prefix variable git-submodule.sh: make the "$cached" variable a boolean git-submodule.sh: remove unused top-level "--branch" argument submodule--helper: have --require-init imply --init submodule update: remove "-v" option submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs" submodule--helper: report "submodule" as our name in some "-h" output submodule--helper: understand --checkout, --merge and --rebase synonyms git-submodule.sh: use "$quiet", not "$GIT_QUIET" git-sh-setup.sh: remove "say" function, change last users submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags builtin/submodule--helper.c | 161 +++++++++++++++------------------ contrib/subtree/git-subtree.sh | 15 ++- git-instaweb.sh | 2 +- git-sh-setup.sh | 16 ---- git-submodule.sh | 88 +++++++----------- submodule.c | 2 +- t/t7406-submodule-update.sh | 58 +++++++++++- 7 files changed, 175 insertions(+), 167 deletions(-) base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1282%2Fchooglen%2Fsubmodule%2Fremove-recursive-prefix-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2 Pull-Request: https://github.com/git/git/pull/1282 Range-diff vs v1: -: ----------- > 1: 85775255f18 git-submodule.sh: remove unused sanitize_submodule_env() -: ----------- > 2: 960fad98e8a git-submodule.sh: remove unused $prefix variable -: ----------- > 3: 757d0927976 git-submodule.sh: make the "$cached" variable a boolean -: ----------- > 4: da3aae9e847 git-submodule.sh: remove unused top-level "--branch" argument -: ----------- > 5: d9c7f69aaa6 submodule--helper: have --require-init imply --init -: ----------- > 6: 0d68ee723e5 submodule update: remove "-v" option -: ----------- > 7: 6e556c412e9 submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs" -: ----------- > 8: 36d45163b6d submodule--helper: report "submodule" as our name in some "-h" output -: ----------- > 9: 8f12108c295 submodule--helper: understand --checkout, --merge and --rebase synonyms -: ----------- > 10: b788fc671bf submodule--helper: eliminate internal "--update" option -: ----------- > 11: 2eec4637397 git-submodule.sh: use "$quiet", not "$GIT_QUIET" -: ----------- > 12: 5b893f7d81e git-sh-setup.sh: remove "say" function, change last users 1: 473548f2fa4 ! 13: 64c138df196 submodule--helper update: use display path helper @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st - struct strbuf displaypath_sb = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; - const char *displaypath = NULL; -+ char *displaypath = NULL; ++ char *displaypath; int needs_cloning = 0; int need_free_url = 0; @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st - else - strbuf_addstr(&sb, ce->name); - strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf); -- strbuf_addch(out, '\n'); -+ strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath); ++ strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); + strbuf_addch(out, '\n'); goto cleanup; } @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets + test_when_finished "rm -fr top-cloned" && + cp -r top-clean top-cloned && + -+ # Create an upstream commit in each repo ++ # Create an upstream commit in each repo, starting with bottom + test_commit -C bottom upstream_commit && -+ (cd middle && -+ git -C bottom fetch && -+ git -C bottom checkout -f FETCH_HEAD && -+ git add bottom && -+ git commit -m "upstream_commit" -+ ) && -+ (cd top && -+ git -C middle fetch && -+ git -C middle checkout -f FETCH_HEAD && -+ git add middle && -+ git commit -m "upstream_commit" -+ ) && ++ # Create middle commit ++ git -C middle/bottom fetch && ++ git -C middle/bottom checkout -f FETCH_HEAD && ++ git -C middle add bottom && ++ git -C middle commit -m "upstream_commit" && ++ # Create top commit ++ git -C top/middle fetch && ++ git -C top/middle checkout -f FETCH_HEAD && ++ git -C top add middle && ++ git -C top commit -m "upstream_commit" && + + # Create a downstream conflict -+ (cd top-cloned/middle && -+ test_commit -C bottom downstream_commit && -+ git add bottom && -+ git commit -m "downstream_commit" && -+ git fetch --recurse-submodules origin && -+ test_must_fail git merge origin/main -+ ) && ++ test_commit -C top-cloned/middle/bottom downstream_commit && ++ git -C top-cloned/middle add bottom && ++ git -C top-cloned/middle commit -m "downstream_commit" && ++ git -C top-cloned/middle fetch --recurse-submodules origin && ++ test_must_fail git -C top-cloned/middle merge origin/main && ++ + # Make the update of "middle" a no-op, otherwise we error out + # because of its unmerged state + test_config -C top-cloned submodule.middle.update !true && + git -C top-cloned submodule update --recursive 2>actual.err && -+ grep "Skipping unmerged submodule .middle/bottom." actual.err ++ grep -F "Skipping unmerged submodule middle/bottom" actual.err +' + +test_expect_success 'submodule update --recursive skip submodules with strategy=none' ' @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets + test_commit -C top-cloned/middle/bottom downstream_commit && + git -C top-cloned/middle config submodule.bottom.update none && + git -C top-cloned submodule update --recursive 2>actual.err && -+ grep "Skipping submodule .middle/bottom." actual.err ++ grep -F "Skipping submodule ${SQ}middle/bottom${SQ}" actual.err +' + test_done 2: 618053730e1 ! 14: d3e803a4630 submodule--helper: don't recreate recursive prefix @@ Commit message ## builtin/submodule--helper.c ## @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec * - { + strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL); strvec_pushf(args, "--jobs=%d", update_data->max_jobs); - if (update_data->recursive_prefix) 3: 7cd1c46f350 = 15: 1f7cf6ffaf1 submodule--helper: use correct display path helper -: ----------- > 16: 85e65f143b6 submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags 4: 57988287fc0 ! 17: 1d8b6b244dc submodule--helper update: use --super-prefix @@ builtin/submodule--helper.c: struct submodule_update_clone { const char *prefix; - const char *recursive_prefix; const char *displaypath; - const char *update_default; + enum submodule_update_type update_default; struct object_id suboid; @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st + get_super_prefix()); if (ce_stage(ce)) { - strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath); -@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path) - - static void update_data_to_args(struct update_data *update_data, struct strvec *args) + strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); +@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec * { + enum submodule_update_type update_type = update_data->update_default; + - strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL); - strvec_pushf(args, "--jobs=%d", update_data->max_jobs); if (update_data->displaypath) @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg - N_("path"), - N_("path into the working tree, across nested " - "submodule boundaries")), - OPT_STRING(0, "update", &opt.update_default, - N_("string"), - N_("rebase, merge, checkout or none")), + OPT_SET_INT(0, "checkout", &opt.update_default, + N_("use the 'checkout' update strategy (default)"), + SM_UPDATE_CHECKOUT), @@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix) module_list_active(&list); @@ builtin/submodule--helper.c: struct cmd_struct { {"name", module_name, 0}, - {"clone", module_clone, 0}, + {"clone", module_clone, SUPPORT_SUPER_PREFIX}, - {"add", module_add, SUPPORT_SUPER_PREFIX}, + {"add", module_add, 0}, - {"update", module_update, 0}, + {"update", module_update, SUPPORT_SUPER_PREFIX}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, - {"init", module_init, SUPPORT_SUPER_PREFIX}, + {"init", module_init, 0}, 5: 9fa13380b02 ! 18: a21e8cd174d submodule--helper: remove display path helper @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st + get_submodule_displaypath(ce->name, suc->update_data->prefix); if (ce_stage(ce)) { - strbuf_addf(out, _("Skipping unmerged submodule '%s'\n"), displaypath); + strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) { ensure_core_worktree(update_data->sm_path);