[PATCH 0/5] submodule summary support

[PATCH 0/5] submodule summary support

[Ping Yin]

This patch series introduces summary support for submodule.

The first three patches teach git-submodule subcommand  'summary', and the last two patches teach git-status/git-commit 'status.submodulesummary' configuration to enable/disable submodule summary.

[PATCH 1/5] 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 submodules sm1 to sm3.
--------------------------------------------
 $ git submodule summary
 # Submodules modifiled: sm1 sm2 sm3
 #
 # * 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
 #
--------------------------------------------

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.

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

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

diff --git a/git-submodule.sh b/git-submodule.sh
index ad9fe62..0e81484 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
@@ -255,7 +255,46 @@ 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)
+#
+modules_summary()
+{
+	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 which have been checked out (i.e. cared by user)
+	modules=$(git diff $cache_option --raw $head -- "$@" |
+		grep '^:160000\|:000000 160000' |
+		while read mod_src mod_dst sha1_src sha1_dst status name
+		do
+			test $status=D && echo "$name" && continue
+			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
@@ -305,6 +344,9 @@ do
 	update)
 		update=1
 		;;
+	summary)
+		summary=1
+		;;
 	status)
 		status=1
 		;;
@@ -345,17 +387,20 @@ case "$add,$branch" in
 	;;
 esac
 
-case "$add,$init,$update,$status,$cached" in
-1,,,,)
+case "$add,$init,$update,$summary,$status,$cached" in
+1,,,,,)
 	module_add "$@"
 	;;
-,1,,,)
+,1,,,,)
 	modules_init "$@"
 	;;
-,,1,,)
+,,1,,,)
 	modules_update "$@"
 	;;
-,,,*,*)
+,,,1,,*)
+	modules_summary "$@"
+	;;
+,,,,*,*)
 	modules_list "$@"
 	;;
 *)
-- 
1.5.4.rc2.9.gf5146-dirty

[PATCH 2/5] 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 |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 67 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 0e81484..cccb539 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -292,7 +292,73 @@ modules_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\|:000000 160000' |
+	cut -c2- |
+	while read mod_src mod_dst sha1_src sha1_dst status name
+	do
+		sha1_dst=$(echo $sha1_dst | cut -c1-7)
+		sha1_src=$(echo $sha1_src | cut -c1-7)
+		check_dst=t
+		check_src=t
+		case $status in
+		D)
+			check_dst=
+			;;
+		A)
+			check_src=
+			;;
+		esac
+
+		(
+			errmsg=
+			unfound_src=
+			unfound_dst=
+
+			test -z "$check_src" ||
+			GIT_DIR="$name/.git" git-rev-parse $sha1_src >&/dev/null ||
+			unfound_src=t
+
+			test -z "$check_dst" ||
+			GIT_DIR="$name/.git" git-rev-parse $sha1_dst >&/dev/null ||
+			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
+
+			echo "* $name $sha1_src...$sha1_dst:"
+			if test -n "$errmsg"
+			then
+				echo "$errmsg"
+			else
+				test -n "$left" && echo "$left"
+				test -n "$right" && echo "$right"
+			fi
+			echo
+		) | sed 's/^/# /'
+	done
+
 	cd "$cwd"
 }
 #
-- 
1.5.4.rc2.9.gf5146-dirty

[PATCH 3/5] 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 |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index cccb539..58d0243 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -4,7 +4,8 @@
 #
 # Copyright (c) 2007 Lars Hjemli
 
-USAGE='[--quiet] [--cached] [add <repo> [-b branch]|status|init|update|summary <commit>] [--] [<path>...]'
+USAGE="[--quiet] [--cached] [-n|--summary-limit <number>] \
+[add <repo> [-b branch]|status|init|update|summary <commit>] [--] [<path>...]"
 OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
@@ -16,6 +17,7 @@ update=
 status=
 quiet=
 cached=
+summary_limit=
 
 #
 # print stuff on stdout unless -q was specified
@@ -265,6 +267,10 @@ set_name_rev () {
 #
 modules_summary()
 {
+	summary_limit=${summary_limit:-1000000}
+	summary_limit=$((summary_limit<0?1000000:summary_limit))
+	test $summary_limit = 0 && return
+
 	cache_option=${cached:+--cached}
 
 	if rev=$(git rev-parse --verify "$1^0" 2>/dev/null)
@@ -352,8 +358,29 @@ modules_summary()
 			then
 				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)
+				lc=$((lc0<summary_limit?lc0:summary_limit))
+				rc=$((summary_limit>lc?summary_limit-lc:1))
+				rc=$((rc<rc0?rc:rc0))
+
+				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
 		) | sed 's/^/# /'
@@ -430,6 +457,13 @@ do
 	--cached)
 		cached=1
 		;;
+	-n|--summary-limit)
+		if [ -z "$2" ]; then
+			usage
+		fi
+		summary_limit="$2"
+		shift
+		;;
 	--)
 		break
 		;;
-- 
1.5.4.rc2.9.gf5146-dirty

[PATCH 4/5] git-status: submodule summary support

[Ping Yin]

By introducing this commit, 'git commit/status' will additionally show
'Submodules modified' section given by 'git --cached submodule summary' just
before 'Untracked files' section.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 wt-status.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index c0c2472..df5be80 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "diffcore.h"
+#include "run-command.h"
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = 0;
@@ -271,6 +272,30 @@ static void wt_status_print_changed(struct wt_status *s)
 	wt_read_cache(s);
 	run_diff_files(&rev, 0);
 }
+static void wt_status_print_submodule_summary(struct wt_status *s)
+{
+	struct child_process sm_summary;
+	const char *argv[] = {
+		"submodule",
+		"summary",
+		"--cached",
+		s->amend ? "HEAD^" : "HEAD",
+		NULL
+	};
+	char index[PATH_MAX];
+	const char *env[2] = { index, NULL };
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
+
+	memset(&sm_summary, 0, sizeof(sm_summary));
+	sm_summary.argv = argv;
+	sm_summary.env = env;
+	sm_summary.git_cmd = 1;
+	sm_summary.no_stdin = 1;
+	fflush(s->fp);
+	sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
+	sm_summary.no_stderr = 1;
+	run_command(&sm_summary);
+}
 
 static void wt_status_print_untracked(struct wt_status *s)
 {
@@ -374,6 +399,9 @@ void wt_status_print(struct wt_status *s)
 	}
 
 	wt_status_print_changed(s);
+	// we must flush s->fp here since the following call will write to s->fp in a child process
+	fflush(s->fp);
+	wt_status_print_submodule_summary(s);
 	wt_status_print_untracked(s);
 
 	if (s->verbose && !s->is_initial)
-- 
1.5.4.rc2.9.gf5146-dirty

[PATCH 5/5] git-status: configurable submodule summary size

[Ping Yin]

Add config variable status.submodulesummary which is passed as
the arg for '--summary-limit' of 'git submodule' to limit the
submodule summary size.

The summary function is disabled by default by this config
(say status.submodulesummary is 0 by default).

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 wt-status.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index df5be80..25ce58b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -11,6 +11,7 @@
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = 0;
+int wt_status_submodule_summary = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
 	"\033[32m", /* WT_STATUS_UPDATED: green */
@@ -275,10 +276,14 @@ static void wt_status_print_changed(struct wt_status *s)
 static void wt_status_print_submodule_summary(struct wt_status *s)
 {
 	struct child_process sm_summary;
+	char summary_limit[64];
+	sprintf(summary_limit, "%d", wt_status_submodule_summary);
 	const char *argv[] = {
 		"submodule",
 		"summary",
 		"--cached",
+		"--summary-limit",
+		summary_limit,
 		s->amend ? "HEAD^" : "HEAD",
 		NULL
 	};
@@ -424,6 +429,10 @@ void wt_status_print(struct wt_status *s)
 
 int git_status_config(const char *k, const char *v)
 {
+	if (!strcmp(k, "status.submodulesummary")) {
+		wt_status_submodule_summary = atoi(v);
+		return 0;
+	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
 		wt_status_use_color = git_config_colorbool(k, v, -1);
 		return 0;
-- 
1.5.4.rc2.9.gf5146-dirty

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

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> @@ -265,6 +267,10 @@ set_name_rev () {
>  #
>  modules_summary()
>  {
> +	summary_limit=${summary_limit:-1000000}

Why a million?

> +	summary_limit=$((summary_limit<0?1000000:summary_limit))

This is doubly bashism.  Variables must be referenced with $,
and $(( conditional ? iftrue : iffalse )) is not POSIX.

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

[Ping Yin]

On Jan 12, 2008 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
> > @@ -265,6 +267,10 @@ set_name_rev () {
> >  #
> >  modules_summary()
> >  {
> > +     summary_limit=${summary_limit:-1000000}
>
> Why a million?
Because i think a million is big enough. I'd better define a constant
for unlimited number.
>
> > +     summary_limit=$((summary_limit<0?1000000:summary_limit))
>
> This is doubly bashism.  Variables must be referenced with $,
> and $(( conditional ? iftrue : iffalse )) is not POSIX.
>
Ok, i'll fix this.


-- 
Ping Yin

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

[Junio C Hamano]

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

> On Jan 12, 2008 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ping Yin <pkufranky@gmail.com> writes:
>>
>> > @@ -265,6 +267,10 @@ set_name_rev () {
>> >  #
>> >  modules_summary()
>> >  {
>> > +     summary_limit=${summary_limit:-1000000}
>>
>> Why a million?
> Because i think a million is big enough. I'd better define a constant
> for unlimited number.

I think that is a wrong approach to begin with.  You are
assuming that you will always limit and by using improbably
large limit to pretend it is unlimited.  Why not making the
summary list generator truely capable of produce an unlimited
list?

I also think using 100 or so as a sane default, allowing the
user to override to say "I do not want any limitation", is a
much better default.

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

[Ping Yin]

On Jan 13, 2008 3:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Ping Yin" <pkufranky@gmail.com> writes:
>
> > On Jan 12, 2008 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Ping Yin <pkufranky@gmail.com> writes:
> >>
> >> > @@ -265,6 +267,10 @@ set_name_rev () {
> >> >  #
> >> >  modules_summary()
> >> >  {
> >> > +     summary_limit=${summary_limit:-1000000}
> >>
> >> Why a million?
> > Because i think a million is big enough. I'd better define a constant
> > for unlimited number.
>
> I think that is a wrong approach to begin with.  You are
> assuming that you will always limit and by using improbably
> large limit to pretend it is unlimited.  Why not making the
> summary list generator truely capable of produce an unlimited
> list?
>
I used a pseudo unlimited number just to make code shorter.
After considering it again, i find that the code is still brief  by using
a real unlimited number. So i'll correct it.

> I also think using 100 or so as a sane default, allowing the
> user to override to say "I do not want any limitation", is a
> much better default.
>
Reasonable



-- 
Ping Yin

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

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> +	git diff $cache_option --raw $head -- $modules |
> +	grep '^:160000\|:000000 160000' |
> +	cut -c2- |
> +	while read mod_src mod_dst sha1_src sha1_dst status name
> +	do
> +		sha1_dst=$(echo $sha1_dst | cut -c1-7)
> +		sha1_src=$(echo $sha1_src | cut -c1-7)

If you are willing to lose precision forever like this, I think
you can run "git diff" with --abbrev and lose this cut.

> +		check_dst=t
> +		check_src=t
> +		case $status in
> +		D)
> +			check_dst=
> +			;;
> +		A)
> +			check_src=
> +			;;

I'd loosen the above grep (see my comments to your 1/5) and also
add this:

		*)
			continue ;# punt
			;;

so that the rest of the code won't break when seeing a path that
was submodule in the HEAD but is a blob in the index.

> +		esac
> +
> +		(
> +			errmsg=
> +			unfound_src=
> +			unfound_dst=
> +
> +			test -z "$check_src" ||
> +			GIT_DIR="$name/.git" git-rev-parse $sha1_src >&/dev/null ||

And the precision of $sha1_src matter here.  Be it done with
"diff --abbrev" at the toplevel or your "cut", that may not be
unique enough in the submodule.

I think you would want to read full 40-char sha1_src and
sha1_dst with "while read", and keep that full 40-char in these
variables, and use them when calling rev-parse here.

If you are checking if that the object exists in the submodule,
use "rev-parse --verify", which was designed for exactly that
purpose.  If you also want to verify if the object is a commit,
which may be a good idea anyway, "rev-parse --verify $sha1_src^0".

To be portable, use traditional ">/dev/null 2>&1", not ">&/dev/null".

> +			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"
> +				;;

When reporting errors, you would want to give full 40-chars...

> +			*)
> +				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
> +
> +			echo "* $name $sha1_src...$sha1_dst:"

While reporting like this, you would want the shortened form,
perhaps produced your "cut -c1-7".

> +			if test -n "$errmsg"
> +			then
> +				echo "$errmsg"
> +			else
> +				test -n "$left" && echo "$left"
> +				test -n "$right" && echo "$right"
> +			fi
> +			echo
> +		) | sed 's/^/# /'
> +	done

I'd prefer to always have "-e" before the sed expression.

Any reason why you want separate invocation of sed inside the
while loop?  IOW, why isn't it like this?

	git diff --raw |
        while read ...
        do
        	...
	done | sed -e 's/^/# /'

> +
>  	cd "$cwd"

Hmmm.

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

[Ping Yin]

On Jan 12, 2008 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>
> > +             check_dst=t
> > +             check_src=t
> > +             case $status in
> > +             D)
> > +                     check_dst=
> > +                     ;;
> > +             A)
> > +                     check_src=
> > +                     ;;
>
> I'd loosen the above grep (see my comments to your 1/5) and also
> add this:
>
>                 *)
>                         continue ;# punt
>                         ;;
>
> so that the rest of the code won't break when seeing a path that
> was submodule in the HEAD but is a blob in the index.

Right. I thought 'M' should be the default case because I missed the
case 'T' (head blob but index submodule, or the reverse) .

>
> > +             esac
> > +
> > +             (
> > +                     errmsg=
> > +                     unfound_src=
> > +                     unfound_dst=
> > +
> > +                     test -z "$check_src" ||
> > +                     GIT_DIR="$name/.git" git-rev-parse $sha1_src >&/dev/null ||
>
>
> I think you would want to read full 40-char sha1_src and
> sha1_dst with "while read", and keep that full 40-char in these
> variables, and use them when calling rev-parse here.

Hmm, precision is really a problem. However, "git diff --raw" will not
always give full 40-char sha1, instead it will give sha1 with enough
length. So maybe i can use the sha1 from "git diff --raw" ?

>
> If you are checking if that the object exists in the submodule,
> use "rev-parse --verify", which was designed for exactly that
> purpose.  If you also want to verify if the object is a commit,
> which may be a good idea anyway, "rev-parse --verify $sha1_src^0".

Yes, --verify is better.
                   ;;
>
> When reporting errors, you would want to give full 40-chars...
>
As said before, enough is ok?

> > +                     *)
> > +                             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
> > +
> > +                     echo "* $name $sha1_src...$sha1_dst:"
>
> While reporting like this, you would want the shortened form,
> perhaps produced your "cut -c1-7".
>
> > +                     if test -n "$errmsg"
> > +                     then
> > +                             echo "$errmsg"
> > +                     else
> > +                             test -n "$left" && echo "$left"
> > +                             test -n "$right" && echo "$right"
> > +                     fi
> > +                     echo
> > +             ) | sed 's/^/# /'
> > +     done
>
> I'd prefer to always have "-e" before the sed expression.
Is it not portable without "-e"?

>
> Any reason why you want separate invocation of sed inside the
> while loop?  IOW, why isn't it like this?
>
>         git diff --raw |
>         while read ...
>         do
>                 ...
>         done | sed -e 's/^/# /'
>
Just because i'm stupid. :)



-- 
Ping Yin

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

[Ping Yin]

> >
> > I think you would want to read full 40-char sha1_src and
> > sha1_dst with "while read", and keep that full 40-char in these
> > variables, and use them when calling rev-parse here.
>
> Hmm, precision is really a problem. However, "git diff --raw" will not
> always give full 40-char sha1, instead it will give sha1 with enough
> length. So maybe i can use the sha1 from "git diff --raw" ?
>
Oh, I'm wrong. It seems 'git diff --raw' will always give full 40-char
sha1 for submodule entry and abbreviated sha1 for blob entry.

-- 
Ping Yin

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

[Junio C Hamano]

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

>> >
>> > I think you would want to read full 40-char sha1_src and
>> > sha1_dst with "while read", and keep that full 40-char in these
>> > variables, and use them when calling rev-parse here.
>>
>> Hmm, precision is really a problem. However, "git diff --raw" will not
>> always give full 40-char sha1, instead it will give sha1 with enough
>> length. So maybe i can use the sha1 from "git diff --raw" ?
>>
> Oh, I'm wrong. It seems 'git diff --raw' will always give full 40-char
> sha1 for submodule entry and abbreviated sha1 for blob entry.

It is not recommended to use "git diff" in scripts when you can
use one of the "git diff-*" plumbing.  In this case I think you
would want "git-diff-index".  Also see --abbrev option.

You can never determine how many hexdigits are "enough" from the
containing project, as it does not have to have access to the
submodule object store.  That's the reason I suggested to read
full object name from diff-index and use it for error reporting
and object retrieval, and shorten it in the UI for normal status
noise.

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

[Ping Yin]

> +                       echo "* $name $sha1_src...$sha1_dst:"

If it's a type change (head submodule but index blob, or the the
reverse), $sha1_dst or $sha1_src will be the sha1 of the blob. It's
inapprociate to be shown as if it's a commit in the submodule. May
00000000 should be shown instead of the blob sha1? However, any good
way to tell user the type change info? Is it ok to add an additional
line such as ""Add file sm1"  in the summary and also change the
summary description as follows?

-------------------------------------------------------------------------------------
 $ git submodule summary sm1
 # Submodules modifiled: sm1
 #
 # * sm1 354cd45...0000000(3f751e5):
 #   <one line message for C
 #   <one line message for B
 #   >Add file sm1
-------------------------------------------------------------------------------------



> 1.5.4.rc2.9.gf5146-dirty
>
>



-- 
Ping Yin

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

[Junio C Hamano]

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

>> +                       echo "* $name $sha1_src...$sha1_dst:"
>
> If it's a type change (head submodule but index blob, or the the
> reverse), $sha1_dst or $sha1_src will be the sha1 of the blob. It's
> inapprociate to be shown as if it's a commit in the submodule. May
> 00000000 should be shown instead of the blob sha1?

I do not think that adds much value.  When A or B is a
non-commit, you know that A...B notation does not apply, and
because it is probably a rare situation you would want to make
it even more clearer to the reader by using a different
notation.  Like

    echo "* $name have changed from submodule $sha1_src to blob $sha1_dst!!".

perhaps in red bold letter in larger font ;-)

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

[Ping Yin]

On Jan 13, 2008 3:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Ping Yin" <pkufranky@gmail.com> writes:
>
> >> +                       echo "* $name $sha1_src...$sha1_dst:"
> >
> > If it's a type change (head submodule but index blob, or the the
> > reverse), $sha1_dst or $sha1_src will be the sha1 of the blob. It's
> > inapprociate to be shown as if it's a commit in the submodule. May
> > 00000000 should be shown instead of the blob sha1?
>
> I do not think that adds much value.  When A or B is a
> non-commit, you know that A...B notation does not apply, and
> because it is probably a rare situation you would want to make
> it even more clearer to the reader by using a different
> notation.  Like
>
>     echo "* $name have changed from submodule $sha1_src to blob $sha1_dst!!".
>
> perhaps in red bold letter in larger font ;-)
>
OK, i will think over.



-- 
Ping Yin

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

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> +	# get modified modules which have been checked out (i.e. cared by user)
> +	modules=$(git diff $cache_option --raw $head -- "$@" |
> +		grep '^:160000\|:000000 160000' |
> +		while read mod_src mod_dst sha1_src sha1_dst status name
> +		do

You are listing paths that were already submodule in HEAD, or
newly added submodule.  What about a path that used to be a blob
but is being made into submodule with the next commit (i.e. RHS
is 160000 but LHS is not 000000)?

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

Yes, you would need to worry about *un*quoting them.

> +	test -n "$modules" &&
> +	echo "# Submodules modified: "$modules &&
> +	echo "#"
> +	cd "$cwd"

Hmmmmmm.... 

> -case "$add,$init,$update,$status,$cached" in
> -1,,,,)
> +case "$add,$init,$update,$summary,$status,$cached" in
> +1,,,,,)
>  	module_add "$@"
>  	;;

This is simply unsustainable.

Please see the other thread with Imran M Yousuf regarding the
command dispatcher.  I think that should be the first thing to
fix before doing any change.

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

[Ping Yin]

On Jan 12, 2008 4:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

>
> Ping Yin <pkufranky@gmail.com> writes:
>
> > +     # get modified modules which have been checked out (i.e. cared by user)
> > +     modules=$(git diff $cache_option --raw $head -- "$@" |
> > +             grep '^:160000\|:000000 160000' |
> > +             while read mod_src mod_dst sha1_src sha1_dst status name
> > +             do
>
> You are listing paths that were already submodule in HEAD, or
> newly added submodule.  What about a path that used to be a blob
> but is being made into submodule with the next commit (i.e. RHS
> is 160000 but LHS is not 000000)?

Hmm, i had ignored such a case.
>
>
>
> > -case "$add,$init,$update,$status,$cached" in
> > -1,,,,)
> > +case "$add,$init,$update,$summary,$status,$cached" in
> > +1,,,,,)
> >       module_add "$@"
> >       ;;
>
> This is simply unsustainable.
>
> Please see the other thread with Imran M Yousuf regarding the
> command dispatcher.  I think that should be the first thing to
> fix before doing any change.
>
Ok, i will resend my patches after this fix


-- 
Ping Yin