Thanks! Everything not mentioned here seems like an obviously good suggestion. I especially appreciate the translation/compiler flow analysis pieces (I hadn't considered either). Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote: > >> From: Glen Choo <chooglen@xxxxxxxxxx> >> >> 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]: > > I think the tests should really be split up as their own preceding > commit, so we're assured that this "git rebase -x 'make test'"'s > cleanly. Hm, I'm not sure how this changes the result of "git rebase -x 'make test'", since these tests will fail prior to this patch. I could modify the tests to demonstrate the broken behavior though, which would save me some effort in describing it in the commit message. >> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh >> index 43f779d751c..e1dc3b1041b 100755 >> --- a/t/t7406-submodule-update.sh >> +++ b/t/t7406-submodule-update.sh >> @@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' ' >> test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter >> ' >> >> +# NEEDSWORK: Clean up the tests so that we can reuse the test setup. >> +# Don't reuse the existing repos because the earlier tests have >> +# intentionally disruptive configurations. > > Yeah this does seem a bit repetative... > >> +test_expect_success 'setup clean recursive superproject' ' >> + git init bottom && >> + test_commit -C bottom "bottom" && >> + git init middle && >> + git -C middle submodule add ../bottom bottom && >> + git -C middle commit -m "middle" && >> + git init top && >> + git -C top submodule add ../middle middle && >> + git -C top commit -m "top" && >> + git clone --recurse-submodules top top-clean >> +' >> + >> +test_expect_success 'submodule update should skip unmerged submodules' ' >> + test_when_finished "rm -fr top-cloned" && >> + cp -r top-clean top-cloned && >> + >> + # Create an upstream commit in each repo >> + 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" >> + ) && > > E.g. here the mixture of "cd" and "-C" is a bit odd, can't we just use: > > git -C middle/bottom ... > ... > git -C middle add Yeah admittedly this is quite odd. I was hoping that this would be more readable than big chunks of "-C", since I personally find those quite difficult to follow when the directory changes from line to line. But, maybe it's just better to be consistent in just using "-C", and I'll find some way of organizing the lines that keeps them readable. > I also wonder if this can't be a for-loop with the right params I suppose so, though I think there isn't enough repetition here to justify the readability/correctness trade-off.
