Re: [PATCH v8 2/3] introduce submodule.hasSuperproject record

From: Glen Choo <chooglen@google.com>
Date: Mar 8, 2022, 22:29 UTC
Message-ID: <kl6lbkyg5b8z.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <kl6lee3c5bzl.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 20 quoted lines

> Emily Shaffer <emilyshaffer@google.com> writes:
>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 652861aa66..59dffda775 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -449,6 +449,9 @@ cmd_update()
>>  			;;
>>  		esac
>>  
>> +		# Note that the submodule is a submodule.
>> +		git -C "$sm_path" config submodule.hasSuperproject "true"
>> +
>>  		if test -n "$recursive"
>>  		then
>>  			(
>
> This hunk has a textual conflict with 'ar/submodule-update reroll pt
> 2', but the fix is easy - just teach "git submodule--helper update" to
> set the config in C.

From our dicussion (offline), it turns out this statement isn't really correct because we do set the config in C, but we do it in clone_submodule():

   diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
   index c5d3fc3817..92986646bc 100644
   --- a/builtin/submodule--helper.c
   +++ b/builtin/submodule--helper.c
   @@ -1839,6 +1839,11 @@ static int clone_submodule(struct module_clone_data *clone_data)
       git_config_set_in_file(p, "submodule.alternateErrorStrategy",
                  error_strategy);

   +	/*
   +	 * Teach the submodule that it's a submodule.
   +	 */
   +	git_config_set_in_file(p, "submodule.hasSuperproject", "true");
   +
     free(sm_alternate);
     free(error_strategy);

Show 46 quoted lines

>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 11cccbb333..422c3cc343 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s
>>  	)
>>  '
>>  
>> +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' '
>> +	(cd super &&
>> +	 git -C submodule config --unset submodule.hasSuperproject &&
>> +	 git submodule update &&
>> +	 git -C submodule config submodule.hasSuperproject
>> +	)
>> +'
>> +
>>  test_done
>
>
> I think there is a gap in the test coverage. I notice that this doesn't
> test that we set submodule.hasSuperproject when the submodule is cloned
> for the first time with 'git submodule update'. I thought that maybe the
> test for this was here...
>
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index 40cf8d89aa..833fa01961 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -115,6 +115,10 @@ inspect() {
>>  	git -C "$sub_dir" rev-parse HEAD >head-sha1 &&
>>  	git -C "$sub_dir" update-index --refresh &&
>>  	git -C "$sub_dir" diff-files --exit-code &&
>> +
>> +	# Ensure that submodule.hasSuperproject is set.
>> +	git -C "$sub_dir" config "submodule.hasSuperproject"
>> +
>>  	git -C "$sub_dir" clean -n -d -x >untracked
>>  }
>>  
>
> But when I removed the "set submodule.hasSuperproject in submodule"
> line, i.e. 
>
>  		git -C "$sm_path" config submodule.hasSuperproject "true"
>
> t7400 still passes.

So we would expect that newly cloned submodules would pass even without this .sh line.

I don't think we need to do this twice in C and in shell. We can move this line:

   +	git_config_set_in_file(p, "submodule.hasSuperproject", "true");

into run-update-procedure (and out of clone_submodule()). This way it's guaranteed to touch every submodule (newly cloned or not).