Re: [PATCH v2 2/5] git-submodule.sh: Add recurse subcommand with basic options

["Imran M Yousuf" <imyousuf@gmail.com>]

On Mon, May 12, 2008 at 7:20 AM, Junio C Hamano <junio@pobox.com> wrote:
> imyousuf@gmail.com writes:
>
>  > The recurse commands behavior can be customized with several arguments
>  > that it accepts. The synopsis for the recurse command is:
>  >
>  >       git-submodule recurse [-q|--quiet] [-e|--exit-after-error]
>  >       [-d|--depth <recursion depth>] [-b|--breadth-first]
>  >       <git command> [<arguments> ...]
>
>  Is there a reason to limit the command that can be run per submodule to
>  only "git" commands?  To me, this "recurse" looks like a glorified "find"
>  command that can trigger its action only to submodule directories, but
>  limits what can be given to its -exec option to "git" commands.  While it
>  would not make sense to give certain git command to recurse (e.g. neither
>  "git show 65ea3b8" nor "git clone $there" would make any sense), it would
>  be handy if we can give certain non-git commands to it (e.g. "du -sh").

I do agree how the recurse command looks, but considering that it is a
'git submodule' subcommand I thought having a general command might
have faced a greater criticism from the community. Similarly about not
allowing certain git commands is also in my list for the later version
as it would require a bigger discussion in the community.

>
>
>  > @@ -580,6 +585,129 @@ cmd_status()
>  >       done
>  >  }
>  >
>  > +# Check whether the submodule is initialized or not
>  > +initialize_sub_module()
>
>  Everybody else seems to spell "<do-something>_submodule"; should this be
>  any different?
>
>
>  > +{
>  > +     if test ! -d "$1"/.git
>  > +     then
>  > +             say "Submodule $1 is not initialized and skipped"
>  > +             return 1
>  > +     # Returns true if submodule is already initialized
>
>  Micronit; s/Returns/Return/.  A sentence that begins with a capitalized
>  verb in comments is almost always in imperative mood, not third-person
>  singular present.
>

Got it, thanks for the correction.

>
>  > +     elif test -d "$1"/.git
>  > +     then
>  > +             return 0
>  > +     fi
>  > +}
>
>  Otherwise, what does it return?  Do you need elif there, or just "else"?
>

Yup, else would be sufficient. Sorry for the mistake

>
>  > +# This function simply checks whether the depth is traverseable in terms of
>  > +# depth and if so then it sequentially traverses its submodules
>  > +traverse_submodules()
>  > +{
>  > +     # If current depth is the range specified than it will continue
>  > +     # else return with success
>  > +     if test "$depth" -gt 0 &&
>  > +             test "$current_depth" -ge "$depth"
>  > +     then
>  > +             return 0;
>  > +     fi
>  > +     # If submodules exists than it will traverse over them
>  > +     if test -f .gitmodules
>  > +     then
>  > +             # Incrementing the depth for the next level of submodules
>  > +             current_depth=$(($current_depth + 1))
>  > +                for mod_path in `sed -n -e 's/path = //p' .gitmodules`; do
>  > +                        traverse_module "$mod_path" "$@"
>  > +                done
>  > +             # Decremented the depth to bring it back to the depth of
>  > +             # the current submodule
>  > +             current_depth=$(($current_depth - 1))
>  > +     fi
>  > +}
>
>  This makes me wonder if you should be iterating over .gitmodules, or
>  perhaps you may want to iterate over output of git-ls-files (picking
>  entries of gitlink type).  How should a local change that adds a new
>  submodule or removes an existing submodule, or moves an existing submodule
>  interact with "submodule recurse"?

Actually once I am done with the recurse command I was planning to add
submodule mv and rm subcommands :). About the git-ls-files command yes
that is also an option, but in case of move it would require editing
.gitmodules and .git/config. AFAIK user need to currently manually
edit them for updating, hoping to write a shell script to get it done.

About it interacting with these changes, as long as the .gitmodules
file is updated correctly it should not be a problem, but if it
becomes inconsistent then it will chokes. In this regard, I checked
how 'git submodule update' works and it uses git-ls-fiiles --stage
with grep to find the gitlinks path and then search them through
.git/config, but it also faces the same problem if move is done
manually without changing the files. Also to be noted is the status
command also uses git-ls-files.

The reason why I did .gitsubmodule is I want to introduce auto-init
and update as an option, and plan to do it once the basic recurse
patches are accepted :). Then reading the .gitmodules would have been
necessary.

About the sed script another option would be to use -

git config -f ./.gitmodules --get-regexp '^submodule\..*\.path$' |
sed -n -e 's|^submodule\.\(.*\)\.path \(.*\)$|\2|p'

Will using this be preferable? I think so :).

>
>  Also the same micronits (s/Incrementing/Increment/; s/Decremented/Decrement/).
>
>  Even if iterating over .gitmodules entries is a good idea, I suspect that
>  sed script is too fragile.  Doesn't .gitmodules use the same format as git
>  configuration files, allowing spaces around values, value quoting and
>  trailing comments on the same line?

I agree on the fragile point and I think I will replace it with the
one I mentioned above.

>
>
>  > +# This actually traverses a submodule; checks whether the its initialized
>  > +# or not, does nothing if not initialized.
>
>  s/the //;?
>
>
>  > +traverse_module()
>  > +{
>  > +     # Will work in the submodule if and only if its initialized
>  > +     initialize_sub_module "$1" &&
>
>  "initialize_sub_module" does not sound like a function that checks if it
>  is initialized, but more like a function to, eh, initialize the submodule.
>  Perhaps the function should be renamed to make it clearer that it is a
>  predicate?

I thought of renaming it but I was a bit lazy as I am writing another
patch for auto initialize :).

>
>
>  > +     (
>  > +             submod_path="$1"
>  > +             shift
>  > +             cd "$submod_path"
>  > +             # If depth-first is specified in that case submodules are
>  > +             # are traversed before executing the command on this submodule
>  > +             test -n "$depth_first" && traverse_submodules "$@"
>  > +             # pwd is mentioned in order to enable the ser to distinguish
>  > +             # between same name modules, e.g. a/lib and b/lib.
>  > +             say "git submodule recurse $submod_path $*"
>  > +             git "$@"
>  > +             # if exit on error is specifed than script will exit if any
>  > +             # command fails. As there is no transaction there will be
>  > +             # no rollback either
>
>  s/than/then/;?
>
>
>  > +             # TODO - If possible facilitate transaction
>  > +             if test "$?" -ne 0 && test -n "$on_error"
>  > +             then
>  > +                     die "FAILED: git submodule $submod_path $*"
>
>  Dying before doing further damage to the repository tree may be a good
>  idea, but I did not see the calling loop in traverse_submodules pay
>  attention to the exit code from here.

Thanks for pointing out this bug, will fix it in the next version.

>
>
>  > +             fi
>  > +             # If depth-first is not specified in that case submodules are
>  > +             # are traversed after executing the command on this submodule
>  > +             test -z "$depth_first" && traverse_submodules "$@"
>  > +     )
>  > +}
>  > +
>  > +# Propagates or recurses over all the submodules at any depth with any
>  > +# git command, e.g. git-clone, git-status, git-commit etc., with the
>  > +# arguments supplied exactly as it would have been supplied to the command
>  > +# otherwise. This actually starts the recursive propagation.
>
>  Is "git-clone" a good example to give here?  What would that mean to
>  recurse into each submodule directories in a superproject to run "clone"?
>

I agree that git-clone is infact a bad example, will remove it :).

>
>  > +cmd_recurse() {
>  > +     while :
>  > +     do
>  > +             case "$1" in
>  > +             -q|--quiet)
>  > +                     quiet=1
>  > +                     ;;
>  > +             -d|--depth)
>  > +                     shift
>  > +                     if test -z "$1"
>  > +                     then
>  > +                             echo "No <recursion depth> specified"
>  > +                             usage
>  > +                     # Arithmatic operation will give an error if depth is not number
>  > +                     # thus chose to check intergerness with regular expression.
>  > +                     # $1 is underquoted becuase the expr is in quotation
>  > +                     elif test "$(expr $1 : '[1-9][0-9]*')" -eq "$(expr $1 : '.*')"
>
>  Huh?
>
>         $ a='1 2 3'
>         $ expr $a : '[1-9]'
>         expr: syntax error
>         $ expr "$a" : '[1-9]'
>         1
>         $ z=$(expr $a : '[1-9]')
>         expr: syntax error
>         $ z=$(expr "$a" : '[1-9]')
>         $ echo $z
>         1
>         $ echo "$(expr $a : '[1-9]')"
>         expr: syntax error
>         $ echo "$(expr "$a" : '[1-9]')"
>         1
>
>  If you want to make sure that $(( ... )) would not choke with given "$1",
>  you can check by attempting to do a simple $(( ... )) to see if it errors
>  out, which would be simpler.
>
>
>         if test -z "$1"
>         then
>                 ...
>         elif ! echo $(( "$1" + 0 )) >/dev/null
>         then
>                 die "$1 is not an integer"
>         ...

This was what I was looking for a simpler and cleaner way :), thanks a
lot Junio.

BTW: its nice to see your emails once again :).

Best regards,

Imran

>
>



-- 
Imran M Yousuf
Email: imran@smartitengineering.com
Mobile: +880-1711402557