[PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[Francesco Pretto]

According to "Documentation/gitmodules.txt", 'checkout' is a valid
'submodule.<name>.update' command. Also "git-submodule.sh" refers to
it and processes it correctly. Reflecting commit 'ac1fbb' to support
this syntax and also validate property values during 'update' command,
issuing an error if the value found is unknown.

Signed-off-by: Francesco Pretto <ceztko@gmail.com>
---
 git-submodule.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2677f2e..4a30087 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -622,7 +622,7 @@ cmd_init()
 		   test -z "$(git config submodule."$name".update)"
 		then
 			case "$upd" in
-			rebase | merge | none)
+			checkout | rebase | merge | none)
 				;; # known modes of updating
 			*)
 				echo >&2 "warning: unknown update mode '$upd' suggested for submodule '$name'"
@@ -805,6 +805,17 @@ cmd_update()
 			update_module=$update
 		else
 			update_module=$(git config submodule."$name".update)
+			case "$update_module" in
+			'')
+				;; # Unset update mode
+			checkout | rebase | merge | none)
+				;; # Known update modes
+			!*)
+				;; # Custom update command
+			*)
+				die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
+				;;
+			esac
 		fi
 
 		displaypath=$(relative_path "$prefix$sm_path")
-- 
1.8.5.2.229.g4448466.dirty

Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[Junio C Hamano]

Francesco Pretto <ceztko@gmail.com> writes:

> According to "Documentation/gitmodules.txt", 'checkout' is a valid
> 'submodule.<name>.update' command.

As you can see in the surrounding text, we call the value of
submodule.*.update a "mode", not a command.

> Also "git-submodule.sh" refers to
> it and processes it correctly.

This present tense puzzles me.  If it already refers to checkout and
handles it correctly is there anything that needs to be done?  Or
did you mean "it should refer to and process it but it doesn't, so
make it so?"

> Reflecting commit 'ac1fbb' to support
> this syntax and also validate property values during 'update' command,
> issuing an error if the value found is unknown.

Sorry, but -ECANNOTPARSE.

Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[Francesco Pretto]

2014/1/7 Junio C Hamano <gitster@pobox.com>:
> Francesco Pretto <ceztko@gmail.com> writes:
>
>> According to "Documentation/gitmodules.txt", 'checkout' is a valid
>> 'submodule.<name>.update' command.
>
> As you can see in the surrounding text, we call the value of
> submodule.*.update a "mode", not a command.
>

Ok.

>> Also "git-submodule.sh" refers to
>> it and processes it correctly.
>
> This present tense puzzles me.  If it already refers to checkout and
> handles it correctly is there anything that needs to be done?  Or
> did you mean "it should refer to and process it but it doesn't, so
> make it so?"
>

Like you said, "it already refers to checkout and handles it
correctly". I think the use of the simple present tense here is
correct: it's a fact. Feel free to advice another wording if you
prefer.

>> Reflecting commit 'ac1fbb' to support
>> this syntax and also validate property values during 'update' command,
>> issuing an error if the value found is unknown.
>
> Sorry, but -ECANNOTPARSE.

Not sure what's wrong here, can you explain why it's failing? I'm
using git-format-patch/git-send-email with default settings. Also, if
you can edit and keep the sign-off (I'm not familiar with the
mailing-list maintainer workflow, sorry), feel free to do it.

Thanks

Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[Junio C Hamano]

Francesco Pretto <ceztko@gmail.com> writes:

> Like you said, "it already refers to checkout and handles it
> correctly". I think the use of the simple present tense here is
> correct: it's a fact. Feel free to advice another wording if you
> prefer.

It is not about preference but what we want to convey to the
readers.  When you start the sentence with "Oh, it already works
correctly", the readers need to see this sentence finished: "It
already works, it is handled correctly, but we change the code
nevertheless because ...?".

Here is my attempt to fill that "because ..." part:

	Subject: git-submodule.sh: 'checkout' is a valid update mode

	'checkout' is documented as one of the valid values for
	'submodule.<name>.update' variable, and in a repository with
	the variable set to 'checkout', "git submodule update"
	command do update using the 'checkout' mode.

	However, it has been an accident that the implementation
	works this way; any unknown value would trigger the same
	codepath and update using the 'checkout' mode.

        Tighten the codepath and explicitly list 'checkout' as one
	of the known update modes, and error out when an unknown
	update mode is used.

	Also, teach the codepath that initializes the configuration
	variable from in-tree .gitmodules that 'checkout' is one of
	the valid values---the code since ac1fbbda (submodule: do
	not copy unknown update mode from .gitmodules, 2013-12-02)
	used to treat the value 'checkout' as unknown and mapped it
	to 'none', which made little sense.

Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[Francesco Pretto]

2014/1/7 Junio C Hamano <gitster@pobox.com>:
> It is not about preference but what we want to convey to the
> readers.  When you start the sentence with "Oh, it already works
> correctly", the readers need to see this sentence finished: "It
> already works, it is handled correctly, but we change the code
> nevertheless because ...?".
>
> Here is my attempt to fill that "because ..." part:
>
>         Subject: git-submodule.sh: 'checkout' is a valid update mode
>
>         'checkout' is documented as one of the valid values for
>         'submodule.<name>.update' variable, and in a repository with
>         the variable set to 'checkout', "git submodule update"
>         command do update using the 'checkout' mode.
>
>         However, it has been an accident that the implementation
>         works this way; any unknown value would trigger the same
>         codepath and update using the 'checkout' mode.
>
>         Tighten the codepath and explicitly list 'checkout' as one
>         of the known update modes, and error out when an unknown
>         update mode is used.
>
>         Also, teach the codepath that initializes the configuration
>         variable from in-tree .gitmodules that 'checkout' is one of
>         the valid values---the code since ac1fbbda (submodule: do
>         not copy unknown update mode from .gitmodules, 2013-12-02)
>         used to treat the value 'checkout' as unknown and mapped it
>         to 'none', which made little sense.
>

I wouldn't be able to explain the change better than your description.
Also, I was under the improper assumption that the change was obvious.
Thank you very much for the amended patch description.

Cheers,
Francesco

Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command

[Francesco Pretto]

2014/1/7 Junio C Hamano <gitster@pobox.com>:
> Sorry, but -ECANNOTPARSE.
>

A bird told me what -ECANNOTPARSE means. Tell me if this comment sounds better:

According to "Documentation/gitmodules.txt", 'checkout' is a valid
'submodule.<name>.update' mode. Also "git-submodule.sh" already refers
to it and handles it correctly. Fix cmd_init() to also accept 'checkout' as
valid update mode and add a similar validation in cmd_update(), issuing
an error if the value read is unknown.