From: Junio C Hamano <hidden> Date: 2016-06-15 22:59:40
"W. Trevor King" [off-list ref] writes:
quoted hunk
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 <redacted>
---
git-submodule.sh | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
@@ -803,17 +803,10 @@ cmd_update()update_module=$updateelseupdate_module=$(gitconfigsubmodule."$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+iftest-z"$update_module"+then+update_module="checkout"+fifi
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.
quoted hunk
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.
From: W. Trevor King <hidden> Date: 2016-06-15 22:59:40
On Thu, Jan 16, 2014 at 10:46:36AM -0800, Junio C Hamano wrote:
"W. Trevor King" [off-list ref] writes:
quoted
@@ -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.
Avoiding useless clones is probably more important than avoiding
duplicate "Invalid update mode" messages. I'll reinstate the case
statement in v5, but I think it should live outside of the “load from
config” block, in case someone adds the ability to set arbitrary
update modes from the command line (`--update merge`, `--update
'!command'`, etc.).
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
From: Francesco Pretto <hidden> Date: 2016-06-15 22:59:40
2014/1/16 W. Trevor King [off-list ref]:
Avoiding useless clones is probably more important than avoiding
duplicate "Invalid update mode" messages.
No, it's not duplicate code. I'll explain, please follow me:
quoted hunk
@@ -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
This is a *validation*. It's done before going more through the code
and die early.
*)
+ die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
This should be an *assert* -> it means if you reach this case
statement you (programmer) have messed the code something in the code
before. In fact in my original patch I wrote something like "invalid
update_module at this flow".
Please keep both as Junio said.
Thanks,
Francesco
From: W. Trevor King <hidden> Date: 2016-06-15 22:59:40
On Thu, Jan 16, 2014 at 09:07:22PM +0100, Francesco Pretto wrote:
2014/1/16 W. Trevor King [off-list ref]:
quoted
Avoiding useless clones is probably more important than avoiding
duplicate "Invalid update mode" messages.
No, it's not duplicate code.
I meant “duplicating the "Invalid update mode" error message”. I
missed the die-early distinction, but I understand now. I think its
non-DRY to have an early case statement to validate the update_module,
and a later case statement to use it. Still, keeping those separate
statements in sync shouldn't be too hard ;).
Please keep both as Junio said.
That's what I said I'd do in the email you're quoting ;). Are you ok
with the die-early validation checking all update_module settings
instead of just checking the loaded-from config branch?
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy