Re: [PATCH 1/2] git-submodule.sh: Support 'checkout' as a valid update command
Ok, applying the suggested modifications and resending shortly.
Thank you,
Francesco
2014/1/6 Junio C Hamano :
> Junio C Hamano writes:
>
>> "W. Trevor King" writes:
>>
>>> On Sun, Jan 05, 2014 at 03:50:48AM +0100, Francesco Pretto wrote:
+ case "$update_module" in
+ '')
+ ;; # Unset update mode
+ checkout | rebase | merge | none)
+ ;; # Known update modes
+ !*)
+ ;; # Custom update command
+ *)
+ update_module=
+ echo >&2 "warning: invalid update mode for
submodule '$name'"
+ ;;
+ esac
>>>
>>> I'd prefer `die "…"` to `echo >&2 "…"`. It's hard to know if mapping
>>> the user's preferred (unknown) update mechanism to 'checkout' is
>>> serious or not.
>>>
>>> This commit also makes me think that --rebase, --merge, and --checkout
>>> should be replaced with a single --update={rebase|merge|checkout|!…}
>>> option, but that's probably food for another commit (and a long
>>> finger-breaking deprecation period).
>>
>> All of the above points sound sensible to me.
>
> I'll tentatively queue this on 'pu' (with the suggested "die"
> update), with some rewording of the log message. The patch needs to
> be signed-off, though.
>
> Thanks.
--
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 1/2] git-submodule.sh: Support 'checkout' as a valid update command
Junio C Hamano writes:
> "W. Trevor King" writes:
>
>> On Sun, Jan 05, 2014 at 03:50:48AM +0100, Francesco Pretto wrote:
>>> + case "$update_module" in
>>> + '')
>>> + ;; # Unset update mode
>>> + checkout | rebase | merge | none)
>>> + ;; # Known update modes
>>> + !*)
>>> + ;; # Custom update command
>>> + *)
>>> + update_module=
>>> + echo >&2 "warning: invalid update mode for
>>> submodule '$name'"
>>> + ;;
>>> + esac
>>
>> I'd prefer `die "…"` to `echo >&2 "…"`. It's hard to know if mapping
>> the user's preferred (unknown) update mechanism to 'checkout' is
>> serious or not.
>>
>> This commit also makes me think that --rebase, --merge, and --checkout
>> should be replaced with a single --update={rebase|merge|checkout|!…}
>> option, but that's probably food for another commit (and a long
>> finger-breaking deprecation period).
>
> All of the above points sound sensible to me.
I'll tentatively queue this on 'pu' (with the suggested "die"
update), with some rewording of the log message. The patch needs to
be signed-off, though.
Thanks.
--
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 1/2] git-submodule.sh: Support 'checkout' as a valid update command
"W. Trevor King" writes:
> On Sun, Jan 05, 2014 at 03:50:48AM +0100, Francesco Pretto wrote:
>> +case "$update_module" in
>> +'')
>> +;; # Unset update mode
>> +checkout | rebase | merge | none)
>> +;; # Known update modes
>> +!*)
>> +;; # Custom update command
>> +*)
>> +update_module=
>> +echo >&2 "warning: invalid update mode for
>> submodule '$name'"
>> +;;
>> +esac
>
> I'd prefer `die "…"` to `echo >&2 "…"`. It's hard to know if mapping
> the user's preferred (unknown) update mechanism to 'checkout' is
> serious or not.
>
> This commit also makes me think that --rebase, --merge, and --checkout
> should be replaced with a single --update={rebase|merge|checkout|!…}
> option, but that's probably food for another commit (and a long
> finger-breaking deprecation period).
All of the above points sound sensible to me.
--
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 1/2] git-submodule.sh: Support 'checkout' as a valid update command
On Sun, Jan 05, 2014 at 03:50:48AM +0100, Francesco Pretto wrote:
> + case "$update_module" in
> + '')
> + ;; # Unset update mode
> + checkout | rebase | merge | none)
> + ;; # Known update modes
> + !*)
> + ;; # Custom update command
> + *)
> + update_module=
> + echo >&2 "warning: invalid update mode for
> submodule '$name'"
> + ;;
> + esac
I'd prefer `die "…"` to `echo >&2 "…"`. It's hard to know if mapping
the user's preferred (unknown) update mechanism to 'checkout' is
serious or not.
This commit also makes me think that --rebase, --merge, and --checkout
should be replaced with a single --update={rebase|merge|checkout|!…}
option, but that's probably food for another commit (and a long
finger-breaking deprecation period).
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 1/2] git-submodule.sh: Support 'checkout' as a valid update command
On Sun, Jan 05, 2014 at 03:50:48AM +0100, Francesco Pretto wrote: > According to "Documentation/gitmodules.txt", 'checkout' is a valid > 'submodule..update' command. Also "git-submodule.sh" refers to > it and processes it correctly. Reflect commit 'ac1fbb' to support this > syntax and also validates property values during 'update' command, > issuing a warning if the value found is unknwon. s/unknwon/unknown/ > --- > git-submodule.sh | 14 +- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index 2677f2e..1d041a7 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,18 @@ 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 > + *) > + update_module= > + echo >&2 "warning: invalid update mode for > submodule '$name'" How about additionally telling the user the current value that is wrong like this: echo >&2 "warning: invalid update mode '$update_module' for submodule '$name'" ? But apart from those minor nits the patch looks good to me. Cheers Heiko -- 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
