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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2017, 23:42 UTC
Message-ID: <xmqqfukmedca.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170113194326.13950-1-sbeller@google.com>

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.