Re: [PATCH v4 1/6] submodule: Make 'checkout' update_module explicit

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

"W. Trevor King" <wking@tremily.us> writes:

> This avoids the current awkwardness of having either '' or 'checkout'
> for checkout-mode updates, which makes testing for checkout-mode
> updates (or non-checkout-mode updates) easier.
>
> Signed-off-by: W. Trevor King <wking@tremily.us>
> ---
>  git-submodule.sh | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 5247f78..5e8776c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -803,17 +803,10 @@ 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
> +			if test -z "$update_module"
> +			then
> +				update_module="checkout"
> +			fi
>  		fi

Is this a good change?

It removes the code to prevent a broken configuration value from
slipping through.  The code used to stop early to give the user a
chance to fix it before actually letting "submodule update" to go
into the time consuming part, e.g. a call to module_clone, but that
code is now lost.

>  		displaypath=$(relative_path "$prefix$sm_path")
> @@ -882,11 +875,16 @@ Maybe you want to use 'update --init'?")"
>  			case ";$cloned_modules;" in
>  			*";$name;"*)
>  				# then there is no local change to integrate
> -				update_module= ;;
> +				update_module=checkout ;;
>  			esac
>  
>  			must_die_on_failure=
>  			case "$update_module" in
> +			checkout)
> +				command="git checkout $subforce -q"
> +				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
> +				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
> +				;;
>  ...
>  			*)
> -				command="git checkout $subforce -q"
> -				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
> -				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
> -				;;
> +				die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
>  			esac

These two hunks make sense.  It is clear in the updated code that
"checkout" is what is implemented with that "git checkout $subforce
-q", and not any other random update mode.