Re: [PATCH v2 1/3] git-submodule: New subcommand 'summary' (1) - code framework

On Sat, Mar 1, 2008 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
 >
 > Ping Yin <pkufranky@gmail.com> writes:

 >
 > Overlong lines.  What's the dot before "Example"?

 '.' is an asciitext syntax. Maybe i should not use it in a commit message.

 > >
 > > sm3 and sm4 are submodules with typechanging (blob<->submodule).
 >
 > Are they?  I think you meant 4 and 5.
 >

 oh, it's a typo

 >
 > > This patch just gives the framework. It just finds the submodules to be
 > > shown as follows.
 > >
 > > --------------------------------------------
 > >  $ git submodule summary
 > >  # Submodules modifiled: sm1 sm2 sm3 sm4 sm5
 > >  #
 > > --------------------------------------------
 >
 > Probably it would be a better organization to show only this in the commit
 > log message for [1/3] and describe how the output is enhanced in the log
 > message of the commit as the code builds more .
 >

 Nice suggestion. Should i move most text into man page instead of in
 commit message?

 >
 > > +# @ = [head counting commits from (default 'HEAD'),] requested
paths (default to all)
 > > +#
 >
 > What's "@ =" convention?
 typo again, should be $@

 >
 > > +cmd_summary()
 > > +{
 >
 > We seem to have '{' on the same line for shell functions in our scripts.
 >
 ok

 >
 > > +     # parse $args after "submodule ... summary".

 > > +     do
 > > +             case "$1" in
 > > +             --cached)
 > > +                     cached=1
 > > +                     ;;
 >
 > If you do this "cached="$1" instead here, then you do not need to do ...
 >
 > > +     cache_option=${cached:+--cached}
 >
 > ... this.
 >
 --cached may be passed before 'summary' subcommand. So in the outer
 option parsing should i replace cached=1 to cached=$1 just in this
 patch or in another patch?

 >
 > > +
 > > +     if rev=$(git rev-parse --verify "$1^0" 2>/dev/null)

 > > +             head=$rev
 > > +             shift
 > > +     else
 > > +             head=HEAD
 > > +     fi
 >
 > Hmph, is showing diff with anything other than HEAD useful?  What happens
 > if the user says "git submodules status HAED" by mistake?
 >
 s/status/summary ?
 This patch has nothing to do with git submodule status.
 'git submodule summary' and 'git submodule summary HEAD' is equivalent

 Except HEAD, HEAD^ is neccessary. Since my target is to call 'git
 submodule summary' in wt-status.c and teach git-status and git-commit
 show this summary. So when 'git commit --amend', 'git submodule
 summary HEAD^' should be used.
 >
 > > +
 > > +     cwd=$(pwd)

 > > +
 > > +     # Get modified modules cared by user
 > > +     modules=$(git diff $cache_option --raw $head -- "$@" |
 >
 > When scripting, please do not use "git diff" unless absolutely necessary.
 > Its output is not meant for script consumption and can be made more "user
 > friendly" as user request comes in.  Instead, use "git diff-index" here.

 ok

 >
 > > +     test -n "$modules" &&

 > > +     echo "#"
 > > +     cd "$cwd"
 >
 > Hmph, is there any point to try coming back there?  You could have even
 > done the cd-to-toplevel inside of $( ... ) construct which is run inside a
 > subshell, so...
 >
 Hmm,  just to avoid side effect of this function.  I'll put 'cd' into
 the subshell

 --
 Ping Yin