threads / patch / 44879

patchsubmodule update: run custom update script for initial populating as well

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

## tl;dr

8 messages between Jan 13, 2017 and Jan 25, 2017. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Stefan Beller· Jan 13, 2017, 19:43 UTC · lore

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(-)
Show changes to 2 files +21 −1

git-submodule.sh, t/t7406-submodule-update.sh

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
Junio C Hamano· Jan 13, 2017, 23:42 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 8 quoted lines
> 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.

Show 22 quoted lines
> 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.
Stefan Beller· Jan 13, 2017, 23:52 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 13, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 52 quoted lines
> 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

Junio C Hamano· Jan 13, 2017, 23:58 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 6 quoted lines
> +			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.
Stefan Beller· Jan 14, 2017, 00:00 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 13, 2017 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> 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

Stefan Beller· Jan 25, 2017, 23:37 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +19 −1

git-submodule.sh, t/t7406-submodule-update.sh

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
Brandon Williams· Jan 25, 2017, 23:41 UTC · re: Stefan Beller · lore

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

On 01/25, Stefan Beller wrote:
Show 57 quoted lines
> 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

Show 23 quoted lines
> +
>  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
Stefan Beller· Jan 25, 2017, 23:48 UTC · re: Brandon Williams · lore

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

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(-)
Show changes to 2 files +16 −1

git-submodule.sh, t/t7406-submodule-update.sh

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

← back to recent threads