Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxxx> > > ## 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@xxxxxxxxxx> > - > - ## 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@xxxxxxxxx> > > ## 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);
