Re: [PATCH 1/5] submodule--helper update: use display path helper

From: Glen Choo <chooglen@google.com>
Date: Jun 28, 2022, 17:34 UTC
Message-ID: <kl6ly1xghetp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <220628.861qv9gpg5.gmgdl@evledraar.gmail.com>

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@gmail.com> writes:

Show 19 quoted lines

> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@google.com>
>>
>> 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.

Show 50 quoted lines

>> 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.