On Mon, May 12, 2008 at 7:20 AM, Junio C Hamano <junio@xxxxxxxxx> wrote:
> imyousuf@xxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxxxx
Mobile: +880-1711402557
--
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
