Re: [PATCH v4 1/6] submodule: Make 'checkout' update_module explicit
On Thu, Jan 16, 2014 at 09:07:22PM +0100, Francesco Pretto wrote: > 2014/1/16 W. Trevor King : > > 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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/6] submodule: Make 'checkout' update_module explicit
2014/1/16 W. Trevor King : > 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: > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/6] submodule: Make 'checkout' update_module explicit
On Thu, Jan 16, 2014 at 10:46:36AM -0800, Junio C Hamano wrote: > "W. Trevor King" writes: > > @@ -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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/6] submodule: Make 'checkout' update_module explicit
"W. Trevor King" 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 > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
