v2, 3 partsgit-submodule: New subcommand 'summary'

This patch series introduces summary support for submodule.

Following patches teach git-submodule a new subcommand 'summary' to show commit summary of user-cared (i.e. checked out) submodules since a given commit (default HEAD) of super project.

For a submodule in question, a series of commits will be shown as the path from the src commit to the dst commit, where the src commit is from the given super project commit, and the dst commit is from the index or working tree (switched by --cached).

.Example: a super project with modified/deleted/typechanged submodules sm1 to sm5.
--------------------------------------------
 $ git submodule summary
 # Submodules modifiled: sm1 sm2 sm3 sm4 sm5
 #
 # * sm1 354cd45...3f751e5:
 #   <one line message for C
 #   <one line message for B
 #   >one line message for D
 #   >one line message for E
 #
 # * sm2 5c8bfb5...000000:
 #   <one line message for F
 #
 # * sm3 354cd45...3f751e5:
 #   Warn: sm3 doesn't contain commit 354cd45
 #
 # * sm4 354cd34(submodule)->235efa(blob):
 #   <one line message for G
 #
 # * sm5 354cd34(blob)->235efa(submodule):
 #   >one line message for H
 #
--------------------------------------------

sm1 has commit C as src (given commit or HEAD) and commit E as dst (index
or working tree) as following picture shows.
	--A-->B-->C (in src:354cd45)
	  \
	   -->D-->E (in dst:3f751e5)

The 'Submodules modified' section for sm1 shows how to change sm1 from src commit C to dst commit E: firstly backward (<) to commit A from commit C via commit B, and then forward (>) to commit E via commit D.

Illustration for output of deleted sm2 is similar.

sm3 and sm4 are submodules with typechanging (blob<->submodule).

If the src/dst commit for a submodule is not found in the work tree (say the submodule respository in the work tree), a warning will be issued. sm3 falls into this case.

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
 #
--------------------------------------------

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 66 insertions(+), 4 deletions(-)

This patch does the hard work of submodule summary and finally gives output shown in last patch.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 69 insertions(+), 1 deletions(-)

This patches teaches git-submodule an option '--summary-limit|-n <number>' to limit number of commits for the summary. Number 0 will disable summary and minus number will not limit the summary size.

For beauty and clarification, the last commit for each section (backward and forward) will always be shown disregarding the given limit. So actual summary size may be greater than the given limit.

In the same super project of these patch series, 'git submodule -n 2 summary sm1' and 'git submodule -n 3 summary sm1' will show the same.

---------------------------------------
 $ git submodule -n 2 summary sm1
 # Submodules modifiled: sm1
 #
 # * sm1 354cd45...3f751e5:
 #   <one line message for C
 #   <one line message for B
 #   >... (1 more)
 #   >one line message for E
 #
---------------------------------------

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 53 insertions(+), 3 deletions(-)

Ping Yin <pkufranky@gmail.com> writes:

"Negative means unlimited" feels unnecessary. Didn't you make "unlimited" the default anyway?

This description is unclear. Does "-n 2" tell "show 2 commits from both side", or "show 2 in total"?

When you have room only for N lines, you might have to say (X more), but you never need to say (1 more). You can fit that omitted one item on that line instead of wasting that line to say (1 more).

It is unclear from the above illustration as the (1 more) is pointing at right without having anything newer than that, but I think you meant to do something like this:

    <top
    <second
    <... (N more)
    <bottom
    >top
    >second
    >... (M more)
    >bottom

I guess fork-point may be interesting enough that showing the bottom one like you did might make sense. I am not convinced but we'll see.

\?\+?????

	summary_limit=$(expr "$2" : '[0-9][0-9]*$')

or even

	if summary_limit=$(( $2 + 0 )) 2>/dev/null ||
           test "$2" != "$summary_limit"
	then
		usage
	fi

perhaps.

Don't.  The first line of this script says "#!/bin/sh", not bash.

On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

expr is portable?

summary_limit=$(( $2 + 0 )) will always has return status 0
So i use
            summary_limit=$(($2 + 0))
            if test $summary_limit = 0 -a "$2" != 0
            then
                usage
            fi

"Ping Yin" <pkufranky@gmail.com> writes:

It's not just portable but is a very old fashioned and time-proven way to do things like this.

Ah, there's a typo there.  The intention was to reject non numbers

	for two in 43 -32 'deadbeef' ' -27' HEAD ''
        do
		if sl=$(( $two + 0 )) 2>/dev/null && test "$two" == "$sl"
                then
                	echo Ah, "$two", that is a number.
		else
                	echo You gave me an un-number "'$two' (vs '$sl')".
		fi
	done

Ping Yin <pkufranky@gmail.com> writes:

Same issues as before. It might be worthwhile to write this part all in Perl, I suspect...

This is unneeded if you used the right tool, i.e. diff-index.

Huh?  Do you mean "test $mod_src = ':160000'" and such?

Perhaps "missing" is a better word.

You do not want to do ^0; you will not be bind a non-commit in gitlink entry anyway.

On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah. There are 2 places needing test this. I don't find a better way

On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

Actually, i need ^0. "git-rev-parse --verify sha1" will always succeed if sha1 is an valid name with lenght 40 even if the sha1 doesn't belong to the repository. But what i want to verify is that the sha1 is not just valid/unique but also belongs to the submodule repository.

"Ping Yin" <pkufranky@gmail.com> writes:

Don't you want "git cat-file -e <sha1>" then, unless you are checking more than one sha1?

On Sat, Mar 8, 2008 at 12:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:

Hmm, yeah, the command you give has the same effect with
   "git-rev-parse --verify sha1^0"

"Ping Yin" <pkufranky@gmail.com> writes:

Ah, I missed that.  Thanks for the clarification.

Ping Yin <pkufranky@gmail.com> writes:

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

Nice touch. This can happen even there is no repository corruption. E.g. you can have a checkout of supermodule at a revision newer than your last "git fetch" in sm3.

Are they?  I think you meant 4 and 5.

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 .

Overlong lines again.

What's "@ =" convention?

We seem to have '{' on the same line for shell functions in our scripts.

If you do this "cached="$1" instead here, then you do not need to do ... 

... this.

Hmph, is showing diff with anything other than HEAD useful? What happens if the user says "git submodules status HAED" by mistake?

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.

This looks troublesome.

 - [0-9] is obviously wrong and [0-7] is what you meant;
 - \| and \+ are not BRE but GNU.

It probably is more portable if written like this:

	grep -e '^:160000 ' -e '^:[0-7]* 160000 '

Yeah, the scripted version is bound to have a lot of issues with funny characters, but let's not worry about it too much. It is something we can fix later and more easily when we rewrite it in C (or Perl), but first get the feature right and stable.

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...

man grep says
 In  basic regular expressions the metacharacters ?, +, {, |, (, and )
lose their special meaning; instead use the backslashed versions \?,
\+, \{, \|, \(, and \).

Doen't it mean that '\|' is BRE ?

"Ping Yin" <pkufranky@gmail.com> writes:

It just says unlike in ERE, these characters are not special in BRE; it does not at all say using backslash like \?, \+, and \| makes them so.

And they are not. \(...\), \{m\}, \{m,\} and \{m,n\} are part of BRE, but the two you used (\+ and \|) are not. GNU accept these two as extensions, but other POSIX implementations may have troubles with them.

  http://www.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap09.html

Please be gentle to porters to non GNU systems. Either stay inside BRE (which I think we have managed to do with our usage of grep) or explicitly ask for ERE with "grep -E".

Junio C Hamano <gitster@pobox.com> writes:

I think this would help people new to our codebase.

Also 

    Message-ID: <7v4pdtgjf0.fsf@gitster.siamese.dyndns.org>
    Subject: Re: [PATCH] git-filter-branch could be confused by similar names
    Date: Fri, 04 Jan 2008 17:17:39 -0800

aka

    http://article.gmane.org gmane.comp.version-control.git/69630

---

 Documentation/CodingGuidelines |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

On Sun, Mar 2, 2008 at 4:52 AM, Junio C Hamano <gitster@pobox.com> wrote:

ok, 3x.

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

On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
 > Ping Yin <pkufranky@gmail.com> writes:
 >
 >  > This patches teaches git-submodule an option '--summary-limit|-n <number>'
 >  > to limit number of commits for the summary. Number 0 will disable summary
 >  > and minus number will not limit the summary size.
 >
 >  "Negative means unlimited" feels unnecessary.  Didn't you make "unlimited"
 >  the default anyway?
 'unlimited' is the default, i should clarify this in the message.

 I think 'Negative means unlimited' is neccessary. Someone may override
 --summary-limit in the shell alias, but sometime he may want to  bring
 back the unlimited behavior in the command line.

 > >
 >  > For beauty and clarification, the last commit for each section (backward
 >  > and forward) will always be shown disregarding the given limit. So actual
 >  > summary size may be greater than the given limit.
 >  >
 >  > In the same super project of these patch series, 'git submodule -n 2
 >  > summary sm1' and 'git submodule -n 3 summary sm1' will show the same.
 >
 >  This description is unclear.  Does "-n 2" tell "show 2 commits from both
 >  side", or "show 2 in total"?
 >
 I should make it clear that -n means 'in total'.

 >  > ---------------------------------------
 >  >  $ git submodule -n 2 summary sm1
 >  >  # Submodules modifiled: sm1
 >  >  #
 >  >  # * sm1 354cd45...3f751e5:
 >  >  #   <one line message for C
 >  >  #   <one line message for B
 >  >  #   >... (1 more)
 >  >  #   >one line message for E
 >  >  #
 >
 >  When you have room only for N lines, you might have to say (X more), but
 >  you never need to say (1 more).  You can fit that omitted one item on that
 >  line instead of wasting that line to say (1 more).
 >
 make sense.

 >
 >  > +             -n|--summary-limit)

 >
 >  \?\+?????
 >
 >         summary_limit=$(expr "$2" : '[0-9][0-9]*$')
 >
 >  or even
 >
 >         if summary_limit=$(( $2 + 0 )) 2>/dev/null ||
 >            test "$2" != "$summary_limit"
 >         then
 >                 usage
 >         fi
 >
 >  perhaps.
 >
 >  > +                     if (( $summary_limit < 0 ))
 >
 >  Don't.  The first line of this script says "#!/bin/sh", not bash.
 >
 ok, i'll fix it.

 --
 Ping Yin