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

[Glen Choo <chooglen@google.com>]

Glen Choo <chooglen@google.com> writes:

> 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);

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