Re: [PATCH 1/4] git submodule: Teach add to accept --group
- From
Stefan Beller <sbeller@google.com>
- Date
- Jan 20, 2016, 23:57 UTC
- Message-ID
- <CAGZ79kaHLkQpaHOY2aqPgdNvWjmpbuMZSogqzPkkVHRUXyibYw@mail.gmail.com>
- In-Reply-To
- <xmqq1t9c54qz.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 20, 2016 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 6fce0dc..ab0f209 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -130,6 +130,7 @@ cmd_add()
>> {
>> # parse $args after "submodule ... add".
>> reference_path=
>> + submodule_groups=
>
> This can just be called $groups in the context of this script. I do
> not foresee we would be planning to deal with other kinds of groups
> here.ok, done.
Show 17 quoted lines
>
>> while test $# -ne 0
>> do
>> case "$1" in
>> @@ -165,6 +166,10 @@ cmd_add()
>> --depth=*)
>> depth=$1
>> ;;
>> + -g|--group)
>> + submodule_groups=${submodule_groups:+${submodule_groups};}"$2"
>> + shift
>> + ;;
>
> You would want to accept "--group=<name>" as well, just like
> existing --reference and --depth do. It won't be much more code,
> and when you move to C (hence parse_options) you'd get it for free
> anyway.I am not sure, if I will to move `add` to C any time soon. Sure I desire less shell and more C[1], but I'd think my time could be spent better than just converting scripts to C. Sometimes I have to though, such as in the case of `init` as the the call out from C to shell is too ugly and the effort to do that is not that much less.
Show 16 quoted lines
> >> @@ -292,6 +297,16 @@ Use -f if you really want to add it." >&2 >> >> git config -f .gitmodules submodule."$sm_name".path "$sm_path" && >> git config -f .gitmodules submodule."$sm_name".url "$repo" && >> + if test -n "$submodule_groups" >> + then >> + OIFS=$IFS >> + IFS=';' > > I do not quite understand the choice of ';' here. If and only if > you _must_ accept multi-word name that has spaces in between as a > group name, the above construct may make sense, but I do not think > we have such requirement. Why not separate with $IFS letters just > like any other normal list managed in shell scripts do? Is there > anything special about names of submodule groups?
Just prior to writing this patch, I spent a good amount of time understanding the error message handling in the parts of the shell code, where the $IFS is used, so my mind was deceived to imitate that.
I agree, we can just use a space as separator here.
Thanks, Stefan
[1] Reason why I have such a disdain to shell is that I do not understand nearly as much as C. I am self-taught in both C and shell, so I do not claim to be perfect. However C is a small but powerful language. After reading "The C Programming Language" by Kernighan and Ritchie I do not think I found a thing that was really unknown to me. However even plain posix-shell is complex enough, that there is no such thing as a compact book to read to understand all its concepts IMHO.