[PATCH v2 0/3] git-submodule: New subcommand 'summary'

[PATCH v2 0/3] git-submodule: New subcommand 'summary'

[Ping Yin]

This patch series introduces summary support for submodule.

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

[Ping Yin]

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(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index a6aaf40..8009a8d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -4,7 +4,7 @@
 #
 # Copyright (c) 2007 Lars Hjemli
 
-USAGE='[--quiet] [--cached] [add <repo> [-b branch]|status|init|update] [--] [<path>...]'
+USAGE='[--quiet] [--cached] [add <repo> [-b branch]|status|init|update|summary [<commit>]] [--] [<path>...]'
 OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
@@ -320,7 +320,69 @@ set_name_rev () {
 	) )
 	test -z "$revname" || revname=" ($revname)"
 }
+#
+# Show commit summary for submodules in index or working tree relative to the given commit
+#
+# If '--cached' is given, show summary between index and given commit,
+# or between working tree and given commit
+#
+# @ = [head counting commits from (default 'HEAD'),] requested paths (default to all)
+#
+cmd_summary()
+{
+	# parse $args after "submodule ... summary".
+	while test $# -ne 0
+	do
+		case "$1" in
+		--cached)
+			cached=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
 
+	cache_option=${cached:+--cached}
+
+	if rev=$(git rev-parse --verify "$1^0" 2>/dev/null)
+	then
+		head=$rev
+		shift
+	else
+		head=HEAD
+	fi
+
+	cwd=$(pwd)
+	cd_to_toplevel
+
+	# Get modified modules cared by user
+	modules=$(git diff $cache_option --raw $head -- "$@" |
+		grep '^:160000\|:[0-9]\+ 160000' |
+		while read mod_src mod_dst sha1_src sha1_dst status name
+		do
+			# Always show modules deleted or type-changed (blob<->module)
+			test $status = D -o $status = T && echo "$name" && continue
+			# Also show added or modified modules which are checked out
+			GIT_DIR="$name/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
+			echo "$name"
+		done
+	)
+
+	# TODO: quote module names containing space or tab
+	test -n "$modules" &&
+	echo "# Submodules modified: "$modules &&
+	echo "#"
+	cd "$cwd"
+}
 #
 # List all submodules, prefixed with:
 #  - submodule not initialized
@@ -391,7 +453,7 @@ cmd_status()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | init | update | status)
+	add | init | update | status | summary)
 		command=$1
 		;;
 	-q|--quiet)
@@ -430,8 +492,8 @@ then
 	usage
 fi
 
-# "--cached" is accepted only by "status"
-if test -n "$cached" && test "$command" != status
+# "--cached" is accepted only by "status" and "summary"
+if test -n "$cached" && test "$command" != status -a "$command" != summary
 then
 	usage
 fi
-- 
1.5.4.3.347.g5314c

[PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Ping Yin]

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(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 8009a8d..8322771 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -380,7 +380,75 @@ cmd_summary()
 	# TODO: quote module names containing space or tab
 	test -n "$modules" &&
 	echo "# Submodules modified: "$modules &&
-	echo "#"
+	echo "#" &&
+	git diff $cache_option --raw $head -- $modules |
+	grep '^:160000\|:[0-9]\+ 160000' |
+	cut -c2- |
+	while read mod_src mod_dst sha1_src sha1_dst status name
+	do
+		sha1_src=$(echo $sha1_src | sed -e 's/\.\.\.//')
+		sha1_dst=$(echo $sha1_dst | sed -e 's/\.\.\.//')
+		check_src=$(echo $mod_src | grep 160000)
+		check_dst=$(echo $mod_dst | grep 160000)
+		errmsg=
+		unfound_src=
+		unfound_dst=
+
+		test -z "$check_src" ||
+		GIT_DIR="$name/.git" git-rev-parse --verify $sha1_src^0 >/dev/null 2>&1 ||
+		unfound_src=t
+
+		test -z "$check_dst" ||
+		GIT_DIR="$name/.git" git-rev-parse --verify $sha1_dst^0 >/dev/null 2>&1 ||
+		unfound_dst=t
+
+		case "$unfound_src,$unfound_dst" in
+		t,)
+			errmsg="  Warn: $name doesn't contain commit $sha1_src"
+			;;
+		,t)
+			errmsg="  Warn: $name doesn't contain commit $sha1_dst"
+			;;
+		t,t)
+			errmsg="  Warn: $name doesn't contain commits $sha1_src and $sha1_dst"
+			;;
+		*)
+			left=
+			right=
+			test -n "$check_src" &&
+			left=$(GIT_DIR="$name/.git" git log --pretty=format:"  <%s" \
+			${check_dst:+$sha1_dst..}$sha1_src 2>/dev/null)
+
+			test -n "$check_dst" &&
+			right=$(GIT_DIR="$name/.git" git log --reverse --pretty=format:"  >%s" \
+			${check_src:+$sha1_src..}$sha1_dst 2>/dev/null)
+			;;
+		esac
+
+		sha1_src=$(echo $sha1_src | cut -c1-7)
+		sha1_dst=$(echo $sha1_dst | cut -c1-7)
+		if test $status = T
+		then
+			if test $mod_dst = 160000
+			then
+				echo "* $name $sha1_src(blob)->$sha1_dst(submodule):"
+			else
+				echo "* $name $sha1_src(submodule)->$sha1_dst(blob):"
+			fi
+		else
+			echo "* $name $sha1_src...$sha1_dst:"
+		fi
+		if test -n "$errmsg"
+		then
+			# Don't give error msg for modification whose dst is not submodule, i.e. deleted or changed to blob
+			test $mod_dst = 160000 && echo "$errmsg"
+		else
+			test -n "$left" && echo "$left"
+			test -n "$right" && echo "$right"
+		fi
+		echo
+	done | sed -e 's/^/# /'
+
 	cd "$cwd"
 }
 #
-- 
1.5.4.3.347.g5314c

[PATCH v2 3/3] git-submodule: New subcommand 'summary' (3) - limit summary size

[Ping Yin]

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(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 8322771..64f1f5c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -4,7 +4,9 @@
 #
 # Copyright (c) 2007 Lars Hjemli
 
-USAGE='[--quiet] [--cached] [add <repo> [-b branch]|status|init|update|summary [<commit>]] [--] [<path>...]'
+USAGE="[--quiet] [--cached] \
+[add <repo> [-b branch]|status|init|update|summary [-n|--summary-limit <number>] [<commit>]] \
+[--] [<path>...]"
 OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
@@ -330,6 +332,8 @@ set_name_rev () {
 #
 cmd_summary()
 {
+	summary_limit=-1
+
 	# parse $args after "submodule ... summary".
 	while test $# -ne 0
 	do
@@ -337,6 +341,14 @@ cmd_summary()
 		--cached)
 			cached=1
 			;;
+		-n|--summary-limit)
+			if test -z "$2" || echo "$2" | grep --quiet -v '^-\?[0-9]\+$'
+			then
+				usage
+			fi
+			summary_limit="$2"
+			shift
+			;;
 		--)
 			shift
 			break
@@ -351,6 +363,8 @@ cmd_summary()
 		shift
 	done
 
+	test $summary_limit = 0 && return
+
 	cache_option=${cached:+--cached}
 
 	if rev=$(git rev-parse --verify "$1^0" 2>/dev/null)
@@ -443,8 +457,44 @@ cmd_summary()
 			# Don't give error msg for modification whose dst is not submodule, i.e. deleted or changed to blob
 			test $mod_dst = 160000 && echo "$errmsg"
 		else
-			test -n "$left" && echo "$left"
-			test -n "$right" && echo "$right"
+			lc0=0
+			rc0=0
+			test -n "$left" && lc0=$(echo "$left" | wc -l)
+			test -n "$right" && rc0=$(echo "$right" | wc -l)
+
+			if (( $summary_limit < 0 ))
+			then
+				lc=$lc0
+				rc=$rc0
+			elif (( $lc0 < $summary_limit ))
+			then
+				lc=$lc0
+				rc=$(($summary_limit-$lc))
+			else
+				lc=$summary_limit
+				rc=1
+			fi
+
+			if (( $rc > $rc0 ))
+			then
+				rc=$rc0
+			fi
+
+			if test -n "$left"
+			then
+				skip=$(($lc0-$lc))
+				echo "$left" | head -$(($lc-1))
+				test $skip != 0 && echo "  <... ($skip more)"
+				echo "$left" | tail -1
+			fi
+
+			if test -n "$right"
+			then
+				skip=$(($rc0-$rc))
+				echo "$right" | head -$(($rc-1))
+				test $skip != 0 && echo "  >... ($skip more)"
+				echo "$right" | tail -1
+			fi
 		fi
 		echo
 	done | sed -e 's/^/# /'
-- 
1.5.4.3.347.g5314c

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

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

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

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

>  # * sm2 5c8bfb5...000000:
>  #   <one line message for F
>  #
>  # * sm3 354cd45...3f751e5:
>  #   Warn: sm3 doesn't contain commit 354cd45

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.

>  # * sm4 354cd34(submodule)->235efa(blob):
>  #   <one line message for G
>  #
>  # * sm5 354cd34(blob)->235efa(submodule):
>  #   >one line message for H
>  #
> ...
> Illustration for output of deleted sm2 is similar.
>
> sm3 and sm4 are submodules with typechanging (blob<->submodule).

Are they?  I think you meant 4 and 5.

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

> @@ -320,7 +320,69 @@ set_name_rev () {
>  	) )
>  	test -z "$revname" || revname=" ($revname)"
>  }
> +#
> +# Show commit summary for submodules in index or working tree relative to the given commit
> +#
> +# If '--cached' is given, show summary between index and given commit,
> +# or between working tree and given commit

Overlong lines again.

> +# @ = [head counting commits from (default 'HEAD'),] requested paths (default to all)
> +#

What's "@ =" convention?

> +cmd_summary()
> +{

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

> +	# parse $args after "submodule ... summary".
> +	while test $# -ne 0
> +	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.

> +
> +	if rev=$(git rev-parse --verify "$1^0" 2>/dev/null)
> +	then
> +		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?

> +
> +	cwd=$(pwd)
> +	cd_to_toplevel
> +
> +	# 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.

> +		grep '^:160000\|:[0-9]\+ 160000' |

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 '

> +	# TODO: quote module names containing space or tab

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.

> +	test -n "$modules" &&
> +	echo "# Submodules modified: "$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...

Re: [PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> 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(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8009a8d..8322771 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -380,7 +380,75 @@ cmd_summary()
>  	# TODO: quote module names containing space or tab
>  	test -n "$modules" &&
>  	echo "# Submodules modified: "$modules &&
> -	echo "#"
> +	echo "#" &&
> +	git diff $cache_option --raw $head -- $modules |
> +	grep '^:160000\|:[0-9]\+ 160000' |
> +	cut -c2- |

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

> +	while read mod_src mod_dst sha1_src sha1_dst status name
> +	do
> +		sha1_src=$(echo $sha1_src | sed -e 's/\.\.\.//')
> +		sha1_dst=$(echo $sha1_dst | sed -e 's/\.\.\.//')

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

> +		check_src=$(echo $mod_src | grep 160000)
> +		check_dst=$(echo $mod_dst | grep 160000)

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

> +		errmsg=
> +		unfound_src=
> +		unfound_dst=

Perhaps "missing" is a better word.

> +		test -z "$check_src" ||
> +		GIT_DIR="$name/.git" git-rev-parse --verify $sha1_src^0 >/dev/null 2>&1 ||
> +		unfound_src=t

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

Re: [PATCH v2 3/3] git-submodule: New subcommand 'summary' (3) - limit summary size

[Junio C Hamano]

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?
>
> 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"?

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

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.

> +		-n|--summary-limit)
> +			if test -z "$2" || echo "$2" | grep --quiet -v '^-\?[0-9]\+$'

\?\+?????

	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.

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

[Ping Yin]

>
>
>  > +             grep '^:160000\|:[0-9]\+ 160000' |
>
>  This looks troublesome.
>
>   - [0-9] is obviously wrong and [0-7] is what you meant;
>   - \| and \+ are not BRE but GNU.
>
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

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

[Ping Yin]

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


> > .Example: a super project with modified/deleted/typechanged submodules sm1 to sm5.
 >
 > 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".

> > +     while test $# -ne 0
 > > +     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)

> > +     then
 > > +             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)

> > +     cd_to_toplevel
 > > +
 > > +     # 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 "# Submodules modified: "$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

Re: [PATCH v2 3/3] git-submodule: New subcommand 'summary' (3) - limit summary size

[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)

>  > +                     if test -z "$2" || echo "$2" | grep --quiet -v '^-\?[0-9]\+$'
 >
 >  \?\+?????
 >
 >         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

Re: [PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Ping Yin]

On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>
>  > +             check_src=$(echo $mod_src | grep 160000)
>  > +             check_dst=$(echo $mod_dst | grep 160000)
>
>  Huh?  Do you mean "test $mod_src = ':160000'" and such?
>
Yeah. There are 2 places needing test this. I don't find a better way


-- 
Ping Yin

Re: [PATCH v2 3/3] git-submodule: New subcommand 'summary' (3) - limit summary size

[Ping Yin]

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

>
>  > +             -n|--summary-limit)
>  > +                     if test -z "$2" || echo "$2" | grep --quiet -v '^-\?[0-9]\+$'
>
>  \?\+?????
>
>         summary_limit=$(expr "$2" : '[0-9][0-9]*$')
>
expr is portable?

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

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

Re: [PATCH v2 3/3] git-submodule: New subcommand 'summary' (3) - limit summary size

[Junio C Hamano]

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

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

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

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

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

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

[Junio C Hamano]

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

>>  > +             grep '^:160000\|:[0-9]\+ 160000' |
>>
>>  This looks troublesome.
>>
>>   - [0-9] is obviously wrong and [0-7] is what you meant;
>>   - \| and \+ are not BRE but GNU.
>>
> 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 ?

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

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

[Junio C Hamano]

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

> "Ping Yin" <pkufranky@gmail.com> writes:
> ...
>> 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 ?
>
> 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".

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(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 3b042db..994eb91 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -53,6 +53,18 @@ For shell scripts specifically (not exhaustive):
  - We do not write the noiseword "function" in front of shell
    functions.
 
+ - As to use of grep, stick to a subset of BRE (namely, no \{m,n\},
+   [::], [==], nor [..]) for portability.
+
+   - We do not use \{m,n\};
+
+   - We do not use -E;
+
+   - We do not use ? nor + (which are \{0,1\} and \{1,\}
+     respectively in BRE) but that goes without saying as these
+     are ERE elements not BRE (note that \? and \+ are not even part
+     of BRE -- making them accessible from BRE is a GNU extension).
+
 For C programs:
 
  - We use tabs to indent, and interpret tabs as taking up to

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

[Ping Yin]

On Sun, Mar 2, 2008 at 4:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Ping Yin" <pkufranky@gmail.com> writes:
>
>
>
> >>  > +             grep '^:160000\|:[0-9]\+ 160000' |
>  >>
>  >>  This looks troublesome.
>  >>
>  >>   - [0-9] is obviously wrong and [0-7] is what you meant;
>  >>   - \| and \+ are not BRE but GNU.
>  >>
>  > 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 ?
>
>  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".
>
ok, 3x.


-- 
Ping Yin

Re: [PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Ping Yin]

On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>  > +             test -z "$check_src" ||
>  > +             GIT_DIR="$name/.git" git-rev-parse --verify $sha1_src^0 >/dev/null 2>&1 ||
>  > +             unfound_src=t
>
>  You do not want to do ^0; you will not be bind a non-commit in gitlink
>  entry anyway.
>
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

Re: [PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Jakub Narebski]

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

> On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ping Yin <pkufranky@gmail.com> writes:
>>
>>> +             test -z "$check_src" ||
>>> +             GIT_DIR="$name/.git" git-rev-parse --verify $sha1_src^0 >/dev/null 2>&1 ||
>>> +             unfound_src=t
>>
>>  You do not want to do ^0; you will not be bind a non-commit in gitlink
>>  entry anyway.
>>
> 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.

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Ping Yin]

On Sat, Mar 8, 2008 at 12:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> "Ping Yin" <pkufranky@gmail.com> writes:
>
>  > On Sat, Mar 1, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>  >> Ping Yin <pkufranky@gmail.com> writes:
>  >>
>  >>> +             test -z "$check_src" ||
>  >>> +             GIT_DIR="$name/.git" git-rev-parse --verify $sha1_src^0 >/dev/null 2>&1 ||
>  >>> +             unfound_src=t
>  >>
>  >>  You do not want to do ^0; you will not be bind a non-commit in gitlink
>  >>  entry anyway.
>  >>
>  > 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.
>
>  Don't you want "git cat-file -e <sha1>" then, unless you are checking
>  more than one sha1?
>
Hmm, yeah, the command you give has the same effect with
   "git-rev-parse --verify sha1^0"
>  --
>  Jakub Narebski
>  Poland
>  ShadeHawk on #git
>



-- 
Ping Yin

Re: [PATCH v2 2/3] git-submodule: New subcommand 'summary' (2) - hard work

[Junio C Hamano]

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

> Actually, i need ^0....
> But what i want > to verify is that the sha1 is not just valid/unique
> but also belongs to the submodule repository.

Ah, I missed that.  Thanks for the clarification.