[PATCH] submodule update: run custom update script for initial populating as well

[PATCH] submodule update: run custom update script for initial populating as well

[Stefan Beller]

In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
2011-02-17), all actions were defaulted to checkout for populating
a submodule initially, because merging or rebasing makes no sense
in that situation.

Other commands however do make sense, such as the custom command
that was added later (6cb5728c43, submodule update: allow custom
command to update submodule working tree, 2013-07-03).

I am unsure about the "none" command, as I can see an initial
checkout there as a useful thing. On the other hand going strictly
by our own documentation, we should do nothing in case of "none"
as well, because the user asked for it.

Reported-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  7 ++++++-
 t/t7406-submodule-update.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 554bd1c494..aeb721ab7e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -606,7 +606,12 @@ cmd_update()
 		if test $just_cloned -eq 1
 		then
 			subsha1=
-			update_module=checkout
+			if test "$update_module" = "merge" ||
+			   test "$update_module" = "rebase" ||
+			   test "$update_module" = "none"
+			then
+				update_module=checkout
+			fi
 		else
 			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c4cc..1407fa6098 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -424,6 +424,19 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 	test_i18ncmp actual expect
 '
 
+test_expect_success 'submodule update - command run for initial population of submodule' '
+	cat <<-\ EOF >expect
+	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
+	EOF
+	(
+		cd super &&
+		rm -rf submodule
+		test_must_fail git submodule update >../actual
+	)
+	test_cmp expect actual
+	git -C super submodule update --checkout
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'
 Failed to recurse into submodule path '../super'
@@ -476,6 +489,7 @@ test_expect_success 'submodule init picks up merge' '
 '
 
 test_expect_success 'submodule update --merge  - ignores --merge  for new submodules' '
+	test_config -C super submodule.submodule.update checkout &&
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
@@ -488,6 +502,7 @@ test_expect_success 'submodule update --merge  - ignores --merge  for new submod
 '
 
 test_expect_success 'submodule update --rebase - ignores --rebase for new submodules' '
+	test_config -C super submodule.submodule.update checkout &&
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
-- 
2.11.0.300.g08194d1431.dirty

Re: [PATCH] submodule update: run custom update script for initial populating as well

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
> 2011-02-17), all actions were defaulted to checkout for populating
> a submodule initially, because merging or rebasing makes no sense
> in that situation.
>
> Other commands however do make sense, such as the custom command
> that was added later (6cb5728c43, submodule update: allow custom
> command to update submodule working tree, 2013-07-03).

Makes sense.

> I am unsure about the "none" command, as I can see an initial
> checkout there as a useful thing. On the other hand going strictly
> by our own documentation, we should do nothing in case of "none"
> as well, because the user asked for it.

I think "none" is "I'll decide which revision of the submodule
should be there---do not decide it for me".  If the user is
explicitly saying with "git submodule init" to have "some" version,
and if the user did not have any (because the user didn't show
interest in any checkout of the submodule before), then I think it
probably makes more sense to checkout the version bound to the
superproject, than leaving the directory empty.

> Reported-by: Han-Wen Nienhuys <hanwen@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh            |  7 ++++++-
>  t/t7406-submodule-update.sh | 15 +++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 554bd1c494..aeb721ab7e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -606,7 +606,12 @@ cmd_update()
>  		if test $just_cloned -eq 1
>  		then
>  			subsha1=
> -			update_module=checkout
> +			if test "$update_module" = "merge" ||
> +			   test "$update_module" = "rebase" ||
> +			   test "$update_module" = "none"
> +			then
> +				update_module=checkout
> +			fi

... which seems to be what you did.  Do we need a documentation
update, or does this just make the behaviour of this corner case
consistent with what is already documented?

Thanks.

Re: [PATCH] submodule update: run custom update script for initial populating as well

[Stefan Beller]

On Fri, Jan 13, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
>> 2011-02-17), all actions were defaulted to checkout for populating
>> a submodule initially, because merging or rebasing makes no sense
>> in that situation.
>>
>> Other commands however do make sense, such as the custom command
>> that was added later (6cb5728c43, submodule update: allow custom
>> command to update submodule working tree, 2013-07-03).
>
> Makes sense.
>
>> I am unsure about the "none" command, as I can see an initial
>> checkout there as a useful thing. On the other hand going strictly
>> by our own documentation, we should do nothing in case of "none"
>> as well, because the user asked for it.
>
> I think "none" is "I'll decide which revision of the submodule
> should be there---do not decide it for me".  If the user is
> explicitly saying with "git submodule init" to have "some" version,
> and if the user did not have any (because the user didn't show
> interest in any checkout of the submodule before), then I think it
> probably makes more sense to checkout the version bound to the
> superproject, than leaving the directory empty.
>
>> Reported-by: Han-Wen Nienhuys <hanwen@google.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  git-submodule.sh            |  7 ++++++-
>>  t/t7406-submodule-update.sh | 15 +++++++++++++++
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 554bd1c494..aeb721ab7e 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -606,7 +606,12 @@ cmd_update()
>>               if test $just_cloned -eq 1
>>               then
>>                       subsha1=
>> -                     update_module=checkout
>> +                     if test "$update_module" = "merge" ||
>> +                        test "$update_module" = "rebase" ||
>> +                        test "$update_module" = "none"
>> +                     then
>> +                             update_module=checkout
>> +                     fi
>
> ... which seems to be what you did.  Do we need a documentation
> update, or does this just make the behaviour of this corner case
> consistent with what is already documented?

I think we do not need to update the documentation, because the
documentation doesn't call out the first/initial call to update to be special.
So for a non existing submodule we can do:

    git submodule update --init --[rebase|merge]

and that falls back to checkout, which *looks* like it was a rebase/merge.
The original bug report was that

    $ git config submodule.<name>.update !echo-script.sh
    $ git submodule update <submodule>
    Submodule path '<submodule>': 'echo-script.sh'
    $ rm -rf <submodule>
    $ git submodule update <submodule>
    .. checked out ..

So while I usually think more verbose documentation is a good idea,
this time it's different, as it merely aligns current documented
behavior with reality.

Thanks,
Stefan

Re: [PATCH] submodule update: run custom update script for initial populating as well

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +			if test "$update_module" = "merge" ||
> +			   test "$update_module" = "rebase" ||
> +			   test "$update_module" = "none"
> +			then
> +				update_module=checkout
> +			fi

	case "$update_module" in
	merge | rebase | none)
		update_module=checkout ;;
	esac

Shorter and probably easier to update.

Re: [PATCH] submodule update: run custom update script for initial populating as well

[Stefan Beller]

On Fri, Jan 13, 2017 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +                     if test "$update_module" = "merge" ||
>> +                        test "$update_module" = "rebase" ||
>> +                        test "$update_module" = "none"
>> +                     then
>> +                             update_module=checkout
>> +                     fi
>
>         case "$update_module" in
>         merge | rebase | none)
>                 update_module=checkout ;;
>         esac
>
> Shorter and probably easier to update.

agreed, want me to reroll or squash locally?

Thanks,
Stefan

[PATCHv2] submodule update: run custom update script for initial populating as well

[Stefan Beller]

In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
2011-02-17), all actions were defaulted to checkout for populating
a submodule initially, because merging or rebasing makes no sense
in that situation.

Other commands however do make sense, such as the custom command
that was added later (6cb5728c43, submodule update: allow custom
command to update submodule working tree, 2013-07-03).

I am unsure about the "none" command, as I can see an initial
checkout there as a useful thing. On the other hand going strictly
by our own documentation, we should do nothing in case of "none"
as well, because the user asked for it.

Reported-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  5 ++++-
 t/t7406-submodule-update.sh | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9788175979..63fc4fe9bc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -607,7 +607,10 @@ cmd_update()
 		if test $just_cloned -eq 1
 		then
 			subsha1=
-			update_module=checkout
+			case "$update_module" in
+			merge | rebase | none)
+				update_module=checkout ;;
+			esac
 		else
 			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 725bbed1f8..2f83243c7d 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -441,6 +441,19 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 	test_i18ncmp actual expect
 '
 
+test_expect_success 'submodule update - command run for initial population of submodule' '
+	cat <<-\ EOF >expect
+	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
+	EOF
+	(
+		cd super &&
+		rm -rf submodule
+		test_must_fail git submodule update >../actual
+	)
+	test_cmp expect actual
+	git -C super submodule update --checkout
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'
 Failed to recurse into submodule path '../super'
@@ -493,6 +506,7 @@ test_expect_success 'submodule init picks up merge' '
 '
 
 test_expect_success 'submodule update --merge  - ignores --merge  for new submodules' '
+	test_config -C super submodule.submodule.update checkout &&
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
@@ -505,6 +519,7 @@ test_expect_success 'submodule update --merge  - ignores --merge  for new submod
 '
 
 test_expect_success 'submodule update --rebase - ignores --rebase for new submodules' '
+	test_config -C super submodule.submodule.update checkout &&
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
-- 
2.11.0.495.g04f60290a0.dirty

Re: [PATCHv2] submodule update: run custom update script for initial populating as well

[Brandon Williams]

On 01/25, Stefan Beller wrote:
> In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
> 2011-02-17), all actions were defaulted to checkout for populating
> a submodule initially, because merging or rebasing makes no sense
> in that situation.
> 
> Other commands however do make sense, such as the custom command
> that was added later (6cb5728c43, submodule update: allow custom
> command to update submodule working tree, 2013-07-03).
> 
> I am unsure about the "none" command, as I can see an initial
> checkout there as a useful thing. On the other hand going strictly
> by our own documentation, we should do nothing in case of "none"
> as well, because the user asked for it.
> 
> Reported-by: Han-Wen Nienhuys <hanwen@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh            |  5 ++++-
>  t/t7406-submodule-update.sh | 15 +++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9788175979..63fc4fe9bc 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -607,7 +607,10 @@ cmd_update()
>  		if test $just_cloned -eq 1
>  		then
>  			subsha1=
> -			update_module=checkout
> +			case "$update_module" in
> +			merge | rebase | none)
> +				update_module=checkout ;;
> +			esac
>  		else
>  			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
>  				git rev-parse --verify HEAD) ||
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 725bbed1f8..2f83243c7d 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -441,6 +441,19 @@ test_expect_success 'submodule update - command in .git/config catches failure -
>  	test_i18ncmp actual expect
>  '
>  
> +test_expect_success 'submodule update - command run for initial population of submodule' '
> +	cat <<-\ EOF >expect
> +	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
> +	EOF
> +	(
> +		cd super &&
> +		rm -rf submodule
> +		test_must_fail git submodule update >../actual
> +	)
> +	test_cmp expect actual
> +	git -C super submodule update --checkout
> +'

You can probably get away without the subshell:

rm -rf super/submodule
test_must_fail git -C super submodule upsate >actual

> +
>  cat << EOF >expect
>  Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'
>  Failed to recurse into submodule path '../super'
> @@ -493,6 +506,7 @@ test_expect_success 'submodule init picks up merge' '
>  '
>  
>  test_expect_success 'submodule update --merge  - ignores --merge  for new submodules' '
> +	test_config -C super submodule.submodule.update checkout &&
>  	(cd super &&
>  	 rm -rf submodule &&
>  	 git submodule update submodule &&
> @@ -505,6 +519,7 @@ test_expect_success 'submodule update --merge  - ignores --merge  for new submod
>  '
>  
>  test_expect_success 'submodule update --rebase - ignores --rebase for new submodules' '
> +	test_config -C super submodule.submodule.update checkout &&
>  	(cd super &&
>  	 rm -rf submodule &&
>  	 git submodule update submodule &&
> -- 
> 2.11.0.495.g04f60290a0.dirty
> 

-- 
Brandon Williams

[PATCHv3] submodule update: run custom update script for initial populating as well

[Stefan Beller]

In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
2011-02-17), all actions were defaulted to checkout for populating
a submodule initially, because merging or rebasing makes no sense
in that situation.

Other commands however do make sense, such as the custom command
that was added later (6cb5728c43, submodule update: allow custom
command to update submodule working tree, 2013-07-03).

I am unsure about the "none" command, as I can see an initial
checkout there as a useful thing. On the other hand going strictly
by our own documentation, we should do nothing in case of "none"
as well, because the user asked for it.

Reported-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Thanks, Brandon, for spotting the unneeded subshell.
 I also fixed the && chaining along the way.
 
 Thanks,
 Stefan

 git-submodule.sh            |  5 ++++-
 t/t7406-submodule-update.sh | 12 ++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9788175979..63fc4fe9bc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -607,7 +607,10 @@ cmd_update()
 		if test $just_cloned -eq 1
 		then
 			subsha1=
-			update_module=checkout
+			case "$update_module" in
+			merge | rebase | none)
+				update_module=checkout ;;
+			esac
 		else
 			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 725bbed1f8..347857fa7c 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -441,6 +441,16 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 	test_i18ncmp actual expect
 '
 
+test_expect_success 'submodule update - command run for initial population of submodule' '
+	cat <<-\ EOF >expect
+	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
+	EOF &&
+	rm -rf super/submodule &&
+	test_must_fail git -C super submodule update >../actual &&
+	test_cmp expect actual &&
+	git -C super submodule update --checkout
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'
 Failed to recurse into submodule path '../super'
@@ -493,6 +503,7 @@ test_expect_success 'submodule init picks up merge' '
 '
 
 test_expect_success 'submodule update --merge  - ignores --merge  for new submodules' '
+	test_config -C super submodule.submodule.update checkout &&
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
@@ -505,6 +516,7 @@ test_expect_success 'submodule update --merge  - ignores --merge  for new submod
 '
 
 test_expect_success 'submodule update --rebase - ignores --rebase for new submodules' '
+	test_config -C super submodule.submodule.update checkout &&
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
-- 
2.11.0.495.g04f60290a0.dirty