Hi Junio,
Thanks for the feedback and specially thank you for the coding
standard documentation, I was looking for it. I will make fixes to
both the patches and email them again. Will send this patch again,
probably tomorrow.
Thank you,
Imran
On Jan 9, 2008 2:38 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> imyousuf@xxxxxxxxx writes:
>
> > From: Imran M Yousuf <imyousuf@xxxxxxxxxxxxxxxxxxxxxx>
> >
> >
> > - The purpose of the recurse command in the git submodule is to recurse
> > a command in its submodule at depths specified. For example if one wants
> > to do a diff on its project with submodules at once, one can simply do
> > git-submodule recurse diff HEAD and would see the diff for all the modules
> > it contains. If during recursion it is found that a module has not been
> > initialized or updated than the command also initializes and updates them.
> > It is to be noted that argument specification to the command intended (in
> > above example diff) to recurse will be same as its original command (i.e.
> > git diff). If the original command spec changes it will have no effect on
> > the recurse command. Depth of recursion can be specified simply by
> > mentioning the -d <recursion depth> argument to recurse command. If not
> > specified or specified to 0, it will be comprehended to recurse at all
> > depth; if a positive number is specified than maximum recursion depth will
> > never cross that depth. In order to see some information -v option may be
> > used
>
> The indentation style seems, eh, unique. An overlong single
> paragraph with solid run of sentences is somewhat hard to read.
>
> I am not still convinced that a subcommand other than init,
> which is started recursively, should initialize and update
> submodules that are uninitialized. Currently there is no way,
> other than not having an initialized submodule repository, to
> mark that the user is _not_ interested in a particular
> submodule, and you will lose that if you make recursing into
> uninitialized ones too easy and aggressive.
>
> I suspect that it might be a saner approach to:
>
> - allow "git submodule recurse init [-d depth]" (although I am
> not sure if limit by depth is so useful in practice -- only
> experience will tell us) to auto-initialize the uninitialized
> submodules;
>
> - allow any other "git submodule recurse $cmd" to run
> recursively to _any_ depth, but never auto-initialize the
> uninitialized submodules.
>
> In other words, I think it is a very bad idea to rob the
> existing mechanism from the user to mark uninteresting
> submodules by not initializing them. An alternative would be to
> give them other means to mark "I am not interested in these
> submodules", if you insist on this auto-initialization, but I do
> not offhand think of a mechanism that is easier to use than what
> we already have (i.e. not checking them out).
>
> > @@ -17,6 +17,9 @@ status=
> > quiet=
> > cached=
> > command=
> > +recurse_verbose=0
> > +recursion_depth=0
> > +current_recursion_depth=0
>
> I wonder if we want this "verbose" option to be specific to the
> recurse subcommand, or perhaps other subcommands may want to
> support more verbose output mode, even if they currently don't.
> Perhaps the variable should be just called $verbose?
>
> > +# Initializes the submodule if already not initialized
> > +initialize_sub_module() {
> > + if [ ! -d "$1"/.git ]; then
> > + if [ $recurse_verbose -eq 1 ]; then
> > + echo Initializing and updating "$1"
>
> That's a sensible message for the $verbose mode.
>
> > + fi
> > + git-submodule init "$1"; git-submodule update "$1"
>
> Can init fail for repository "$1"? Do you want to proceed
> updating it if it does?
>
> > +# This actually traverses the module; checks
> > +# whether the module is initialized or not.
> > +# if not initialized, then done so and then the
> > +# intended command is evaluated. Then it
> > +# recursively goes into it modules.
> > +traverse_module() {
> > + if [ $recurse_verbose -eq 1 ]; then
> > + echo Current recursion depth: "$current_recursion_depth"
> > + fi
> > + initialize_sub_module "$1"
> > + (
> > + submod_path="$1"
> > + shift
> > + cd "$submod_path"
> > + if [ $recurse_verbose -eq 1 ]; then
> > + echo Working in mod "$submod_path" @ `pwd` with "$@" \("$#"\)
>
> Is this a sensible message, I have to wonder... Saying `pwd`
> after already saying "$submod_path" feels more like a debugging
> message than just being $verbose.
>
> > + fi
> > + git "$@"
>
> Is the user interested in exit status from this command? Does
> the user want to continue on to other submodules if this one
> fails?
>
> > + # Check whether submodules exists only if recursion to that depth
> > + # was requested by user
> > + if test "$recursion_depth" -eq 0 || test "$current_recursion_depth" -lt "$recursion_depth"
>
> Overly long line. Perhaps...
>
> if test "$depth" -eq 0 ||
> test "$current_depth" -lt "$depth"
> then
> ...
>
> > + then
> > + if [ -f .gitmodules ]; then
> > + for mod_path in `sed -n -e 's/path = //p' .gitmodules`; do
> > + export current_recursion_depth=$(echo "$current_recursion_depth+1" | bc)
>
> (1) I do not think you need to export this variable;
>
> (2) It was reported some shells that we intend to support
> mishandle "export VAR=VAL" construct and we tend to write
> this "VAR=VAL" followed by "export VAR" as two separate
> commands on two separate lines;
>
> (3) We do not use "bc" (and traditionally, shell scripts
> outside git don't, either) in scripts.
>
> So, I think:
>
> current_recursion_depth=$(( $current_recursion_depth + 1 ))
>
> is enough, although the variable name feels overly long.
>
> Probably I would even do:
>
> if test "$depth -ne 0 && test "$current_depth" -ge "$depth"
> then
> exit 0
> fi
> if test -f .gitmodules
> then
> current_recursion_depth=$(( $current_recursion_depth + 1 ))
> for p in $(sed -n -e 's/path = ....)
> do
> traverse_module "$p" "$@"
> done
> fi
>
> which would avoid one level of nesting (and indentation), and
> removes unnecessary increment and decrement inside the loop.
> You are in your own subprocess, so your increment will not
> affect the process that called you, and after leaving the loop
> the only thing you do is just to exit the subprocess.
>
> > +# 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
> > +modules_recurse() {
> > + project_home=`pwd`
> > + echo Project Home: "$project_home"
>
> Doesn't this message belong to $verbose mode?
>
> > + if [ $recurse_verbose -eq 1 ]; then
> > + if [ $recursion_depth = 0 ]; then
> > + echo Recursion will go to all depths
> > + else
> > + echo Recursion depth specified to "$recursion_depth"
> > + fi
>
> These sounds more like debugging message than $verbose.
>
> > + fi
> > + if [ -d "$project_home"/.git/ ]; then
> > + if [ $recurse_verbose -eq 1 ]; then
> > + echo Command to recurse: git "$@"
>
> Likewise -- you will repeat that inside traverse_module anyway.
>
> > + fi
> > + git "$@"
> > + if [ -f .gitmodules ]; then
> > + for mod_path in `sed -n -e 's/path = //p' .gitmodules`; do
> > + export current_recursion_depth=1
> > + traverse_module $mod_path "$@"
> > + done
> > + fi
>
> Do I see a code duplication here? Why isn't this done as the
> first level recursion inside traverse_module? _Even_ _if_ you
> insist auto-initializing submodules, by moving the
> initialize_sub_module call in traverse_module a bit down
> (namely, before it recursively invoke traverse_module itself and
> make it auto initialize $submod_path, not "$1"), I think you can
> remove this duplication.
>
> > + else
> > + echo "$project_home" not a git repo thus exiting
> > + exit
>
> And its exit code is 0 which tells the caller that there is no
> error?
>
> > + fi
> > + unset current_recursion_depth
>
> The reason for this unset is...?
>
> > @@ -326,6 +405,37 @@ do
> > --cached)
> > command="modules_list"
> > ;;
> > + recurse)
> > + command="modules_$1"
> > + repeat=1
> > + while test $repeat = 1
> > + do
>
> You do not need that $repeat thing. Just use "break", like this.
>
> while :
> do
> case ... in
> ...
> *)
> break ;;
> esac
> done
>
> > + -d)
> > + if [ -z $3 ]; then
>
> (minor style)
>
> if test -z "$3"
> then
> ...
>
> > + echo No \<recursion depth\> specified
> > + usage
> > + elif [ `expr "$3" : '[1-9][0-9]*.*'` -eq `expr "$3" : '.*'` ]; then
> > + recursion_depth="$3"
> > + shift
> > + shift
> > + else
> > + echo \<recursion depth\> not an integer
> > + usage
> > + fi
> > + ;;
>
> Instead of checking integerness by hand, it would probably be
> much simpler if you did something like this:
>
> depth="$3"
> depth=$(( $depth + 0 ))
> if test "$depth" != "$3"
> then
> die "what's that '$3' for?"
> else
> : happy
> fi
>
> For a rough guideline of shell constructs we use (and do not
> use), please see Documentation/CodingGuidelines.
>
--
Imran M Yousuf
Entrepreneur & Software Engineer
Smart IT Engineering
Dhaka, Bangladesh
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
