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

[Junio C Hamano <gitster@pobox.com>]

Glen Choo <chooglen@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Emily Shaffer <emilyshaffer@google.com> writes:
>>
>>> @@ -2617,6 +2622,12 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
>>>  
>>>  	free(prefixed_path);
>>>  
>>> +	/*
>>> +	 * This entry point is always called from a submodule, so this is a
>>> +	 * good place to set a hint that this repo is a submodule.
>>> +	 */
>>> +	git_config_set("submodule.hasSuperproject", "true");
>>> +
>>>  	if (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)
>>>  		return do_run_update_procedure(&update_data);
>>
>> In Glen's update to rewrite "submodule update" in C, this part is
>> replaced with a call to update_submodule2().  I am not sure what the
>> current repository is at this point of the code with and without
>> Glen's topic, but are we sure we are in a submodule we discovered?
>
> Rereading this, I realize you probably meant that this conflicts with
> part1, not part2...
>
> At the end of part1, update_submodule2() is called from inside the
> submodule (specifically from run_update_procedure()). So a good merge
> conflict resolution would be to set the config _before_ calling
> update_submodule2(). e.g.
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index bef9ab22d4..f53808d995 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2672,6 +2677,11 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
>                                             &update_data.update_strategy);
>
>         free(prefixed_path);
> +       /*
> +        * This entry point is always called from a submodule, so this is a
> +        * good place to set a hint that this repo is a submodule.
> +        */
> +       git_config_set("submodule.hasSuperproject", "true");
>         return update_submodule2(&update_data);
>  }

That matched my tentative resolution I made last night, but what do
you think about this part of the test added by the patch?

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 11cccbb333..ec2397fc69 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 &&
+	 test_unconfig submodule.hasSuperproject &&
+	 git submodule update &&
+	 test_cmp_config -C submodule true --type=bool submodule.hasSuperproject
+	)
+'
+
 test_done

We go to "super", make sure that superproject does not have
submodule.hasSuperproject set, run "git submodule update", and see
if the configuration file in "submodule" subdirectory has the
variable set.  It does not clear the variable from the submodule
before starting, so the variable given to the submodule when it was
cloned would be there, even if "git submodule update" failed to set
it.

I am wondering if it should do something like the attached instead.

We

 * clear the variable from "super" and "super/submodule"
   repositories;

 * run "git submodule update";

 * ensure that "git submodule update" did not touch "super/.git/config";

 * ensure that "git submodule update" added the variable to
   "super/submodule/.git/config".

Clearing the variable from "super" is technically wrong because the
repository is set up as a submodule of "recursivesuper" and if we
had further tests, we should restore it in "super", but the point is
that we are makng sure "git submodule update" sets the variable in
the configuration file of the submodule, and not in the superproject's. 

With the conflict resolution above, this "corrected" test fails and
shows that superproject's configuration file is updated after "git
submodule update".

This series alone, without your topic, this "corrected" test fails,
and that is where my "are we sure we are mucking with the
configuration file in the submodule"? comes from.

diff --git c/t/t7406-submodule-update.sh w/t/t7406-submodule-update.sh
index 000e055811..c9912bb242 100755
--- c/t/t7406-submodule-update.sh
+++ w/t/t7406-submodule-update.sh
@@ -1083,4 +1083,16 @@ test_expect_success 'submodule update --filter sets partial clone settings' '
 	test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter
 '
 
+test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' '
+	(cd super &&
+	 test_unconfig submodule.hasSuperproject &&
+	 test_unconfig -C submodule submodule.hasSuperproject &&
+	 git submodule update &&
+	 echo in super &&
+	 test_cmp_config false --type=bool submodule.hasSuperproject &&
+	 echo in submodule &&
+	 test_cmp_config -C submodule true --type=bool submodule.hasSuperproject
+	)
+'
+
 test_done