On Wed, Jan 20, 2016 at 1:18 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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.
>
>> 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.
>
>> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html