builtin-status submodule summary

builtin-status submodule summary

[Ping Yin]

This patch series builds on top of next. It teaches builtin-status/commit
summary for submodules.

It contains
      git-submodule summary: --for-status option
      builtin-status: submodule summary support
      builtin-status: configurable submodule summary size
      buitin-status: Add tests for submodule summary

with diffstat

 git-submodule.sh             |   17 +++++-
 t/t7401-submodule-summary.sh |   13 ++++
 t/t7502-status.sh            |  136 ++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  |   38 ++++++++++++
 4 files changed, 203 insertions(+), 1 deletions(-)

[PATCH v2 1/4] git-submodule summary: --for-status option

[Ping Yin]

The --for-status option is mainly used by builtin-status/commit.
It adds 'Modified submodules:' line at top and  '# ' prefix to all
following lines.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh             |   17 ++++++++++++++++-
 t/t7401-submodule-summary.sh |   13 +++++++++++++
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 5f1d5ef..4baafba 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -342,6 +342,7 @@ set_name_rev () {
 #
 cmd_summary() {
 	summary_limit=-1
+	for_status=
 
 	# parse $args after "submodule ... summary".
 	while test $# -ne 0
@@ -350,6 +351,9 @@ cmd_summary() {
 		--cached)
 			cached="$1"
 			;;
+		--for-status)
+			for_status="$1"
+			;;
 		-n|--summary-limit)
 			if summary_limit=$(($2 + 0)) 2>/dev/null && test "$summary_limit" = "$2"
 			then
@@ -398,6 +402,12 @@ cmd_summary() {
 	)
 
 	test -n "$modules" &&
+	if test -n "$for_status"; then
+		echo "# Modified submodules:"
+		echo "#"
+	else
+		true
+	fi &&
 	git diff-index $cached --raw $head -- $modules |
 	grep -e '^:160000' -e '^:[0-7]* 160000' |
 	cut -c2- |
@@ -499,7 +509,12 @@ cmd_summary() {
 			echo
 		fi
 		echo
-	done
+	done |
+	if test -n "$for_status"; then
+		sed -e "s|^|# |" -e 's|^# $|#|'
+	else
+		cat
+	fi
 }
 #
 # List all submodules, prefixed with:
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 0f3c42a..1dbb39d 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -192,4 +192,17 @@ test_expect_success 'given commit' "
 EOF
 "
 
+test_expect_success '--for-status' "
+    git submodule summary --for-status HEAD^ >actual &&
+    diff actual - <<-EOF
+# Modified submodules:
+#
+# * sm1 $head6...0000000:
+#
+# * sm2 0000000...$head7 (2):
+#   > Add foo9
+#
+EOF
+"
+
 test_done
-- 
1.5.4.3.347.g5314c

[PATCH v2 2/4] builtin-status: submodule summary support

[Ping Yin]

This commit teaches 'git commit/status' show 'Modified submodules'
section given by 'git submodule summary --cached --for-status'
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 701d13d..75ee7ba 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -8,6 +8,7 @@
 #include "revision.h"
 #include "diffcore.h"
 #include "quote.h"
+#include "run-command.h"
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
@@ -219,6 +220,30 @@ static void wt_status_print_changed(struct wt_status *s)
 	rev.diffopt.format_callback_data = 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",
+		"--for-status",
+		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 */
+	run_command(&sm_summary);
+}
 
 static void wt_status_print_untracked(struct wt_status *s)
 {
@@ -321,6 +346,9 @@ void wt_status_print(struct wt_status *s)
 	}
 
 	wt_status_print_changed(s);
+	// must flush s->fp since 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.3.347.g5314c

[PATCH v2 3/4] builtin-status: configurable submodule summary size

[Ping Yin]

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

status.submodulesummary is 0 by default which disables the summary.

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

diff --git a/wt-status.c b/wt-status.c
index 75ee7ba..2f47e36 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -12,6 +12,7 @@
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
+int wt_status_submodule_summary = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
 	"\033[32m", /* WT_STATUS_UPDATED: green */
@@ -222,12 +223,17 @@ static void wt_status_print_changed(struct wt_status *s)
 }
 static void wt_status_print_submodule_summary(struct wt_status *s)
 {
+	if (wt_status_submodule_summary == 0) return;
 	struct child_process sm_summary;
+	char summary_limit[64];
+	sprintf(summary_limit, "%d", wt_status_submodule_summary);
 	const char *argv[] = {
 		"submodule",
 		"summary",
 		"--cached",
 		"--for-status",
+		"--summary-limit",
+		summary_limit,
 		s->amend ? "HEAD^" : "HEAD",
 		NULL
 	};
@@ -371,6 +377,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.3.347.g5314c

[PATCH v2 4/4] buitin-status: Add tests for submodule summary

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t7502-status.sh |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 136 insertions(+), 0 deletions(-)

diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index e006074..7a9a08f 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -149,4 +149,140 @@ test_expect_success 'status of partial commit excluding new file in index' '
 	diff -u expect output
 '
 
+test_expect_success "setup status submodule summary" '
+	test_create_repo sm &&
+	cd sm &&
+	: >foo &&
+	git add foo &&
+	git commit -m "Add foo" &&
+	cd .. &&
+	git add sm
+'
+
+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#	new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success "status submodule summary is disabled by default" '
+	git status > output &&
+	git diff expect output
+'
+
+cd sm &&
+head=$(git rev-parse --verify HEAD | cut -c1-7) &&
+cd ..
+
+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#	new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Modified submodules:
+#
+# * sm 0000000...$head (1):
+#   > Add foo
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success "status submodule summary" '
+	git config status.submodulesummary 10 &&
+	git status > output &&
+	git diff expect output
+'
+
+
+cat > expect <<EOF
+# On branch master
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+no changes added to commit (use "git add" and/or "git commit -a")
+EOF
+test_expect_success "status submodule summary (clean submodule)" '
+	git commit -m "commit submodule" &&
+	git config status.submodulesummary 10 &&
+	! git status > output &&
+	git diff expect output
+'
+
+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD^1 <file>..." to unstage)
+#
+#	new file:   dir2/added
+#	new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Modified submodules:
+#
+# * sm 0000000...$head (1):
+#   > Add foo
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success "status submodule summary (--amend)" '
+	git config status.submodulesummary 10 &&
+	git status --amend > output &&
+	git diff expect output
+'
+
 test_done
-- 
1.5.4.3.347.g5314c

Re: [PATCH v2 2/4] builtin-status: submodule summary support

[Johannes Sixt]

Ping Yin schrieb:
> +	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 */

The fflush() at this point makes a lot of sense, and doesn't even need a
comment (IMHO).

>  	wt_status_print_changed(s);
> +	// must flush s->fp since 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);

But then we don't need the fflush() here. Right?

-- Hannes

Re: [PATCH v2 3/4] builtin-status: configurable submodule summary size

[Johannes Sixt]

Ping Yin schrieb:
> +	if (wt_status_submodule_summary == 0) return;

Style:
	if (!wt_status_submodule_summary)
		return;

>  	struct child_process sm_summary;
> +	char summary_limit[64];
> +	sprintf(summary_limit, "%d", wt_status_submodule_summary);
>  	const char *argv[] = {

Declaration after statement. That's not portable.

-- Hannes

Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary

[Johannes Sixt]

Ping Yin schrieb:
> +cd sm &&
> +head=$(git rev-parse --verify HEAD | cut -c1-7) &&
> +cd ..

I think you can make these three lines into:

test_expect_success 'get short SHA1 of submodule HEAD' '

	head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7)
'

(not tested, though).

> +test_expect_success "status submodule summary (--amend)" '

Thanks for adding this one.

-- Hannes

Re: [PATCH v2 2/4] builtin-status: submodule summary support

[Ping Yin]

On Thu, Mar 13, 2008 at 10:11 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Ping Yin schrieb:
>
>  >       wt_status_print_changed(s);
>  > +     // must flush s->fp since 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);
>
>  But then we don't need the fflush() here. Right?
>
You are right.

diff --git a/wt-status.c b/wt-status.c
index 2f47e36..468c14c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -352,8 +352,6 @@ void wt_status_print(struct wt_status *s)
        }

        wt_status_print_changed(s);
-       // must flush s->fp since 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)


-- 
Ping Yin

Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Ping Yin schrieb:
>> +cd sm &&
>> +head=$(git rev-parse --verify HEAD | cut -c1-7) &&
>> +cd ..
>
> I think you can make these three lines into:
>
> test_expect_success 'get short SHA1 of submodule HEAD' '
>
> 	head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7)
> '

"git rev-parse --short=12 --verify HEAD" or whatever minimum length you
would want would free you from an ugly "pipe to cut".

Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary

[Ping Yin]

On Thu, Mar 13, 2008 at 10:21 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Ping Yin schrieb:
>
> > +cd sm &&
>  > +head=$(git rev-parse --verify HEAD | cut -c1-7) &&
>  > +cd ..
>
>  I think you can make these three lines into:
>
>  test_expect_success 'get short SHA1 of submodule HEAD' '
>
>         head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7)
>  '
>
>  (not tested, though).

Hmm, It doesn't work. Since i need $head1 outputed to file expect as follows

+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD^1 <file>..." to unstage)
+#
+#      new file:   dir2/added
+#      new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#      modified:   dir1/modified
+#
+# Modified submodules:
+#
+# * sm 0000000...$head (1):

Here we need $head.



-- 
Ping Yin

Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary

[Ping Yin]

On Fri, Mar 14, 2008 at 2:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>  > Ping Yin schrieb:
>  >> +cd sm &&
>  >> +head=$(git rev-parse --verify HEAD | cut -c1-7) &&
>  >> +cd ..
>  >
>  > I think you can make these three lines into:
>  >
>  > test_expect_success 'get short SHA1 of submodule HEAD' '
>  >
>  >       head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7)
>  > '
>
>  "git rev-parse --short=12 --verify HEAD" or whatever minimum length you
>  would want would free you from an ugly "pipe to cut".
>
Thanks.

diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index 7a9a08f..80e2a7b 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -187,9 +187,7 @@ test_expect_success
        git diff expect output
 '

-cd sm &&
-head=$(git rev-parse --verify HEAD | cut -c1-7) &&
-cd ..
+head=$(cd sm && git rev-parse --short=7 --verify HEAD)

 cat > expect <<EOF
 # On branch master



-- 
Ping Yin

Re: builtin-status submodule summary

[Ping Yin]

On Thu, Mar 13, 2008 at 9:48 PM, Ping Yin <pkufranky@gmail.com> wrote:
> This patch series builds on top of next. It teaches builtin-status/commit
>  summary for submodules.
>
>  It contains
>       git-submodule summary: --for-status option
>       builtin-status: submodule summary support
>       builtin-status: configurable submodule summary size
>       buitin-status: Add tests for submodule summary
>
>  with diffstat
>
>   git-submodule.sh             |   17 +++++-
>   t/t7401-submodule-summary.sh |   13 ++++
>   t/t7502-status.sh            |  136 ++++++++++++++++++++++++++++++++++++++++++
>   wt-status.c                  |   38 ++++++++++++
>   4 files changed, 203 insertions(+), 1 deletions(-)
>

IMO, git submodule summary is not so useful for me if it's not
integrated into git-status. In fact i never use "git submodule
summary" directly. git-status with submodule summary support is very
useful to help user figure out what is going on in a more global level
when cooking the commit message.

So i think this series should go along with the submodule summary series.



-- 
Ping Yin

Re: builtin-status submodule summary

[Junio C Hamano]

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

> So i think this series should go along with the submodule summary series.

Perhaps eventually, but definitely not this round, I am afraid.

The usefulness of the output in the current implementation of the
"summary" itself is not even proven at this point.  Nobody other than you
has jumped up-and-down and said "submodule summary is great and should be
in status, I did not know what I was missing!" yet.

I hope we verified the code well enough to make sure that people who use
"git submodule" command but do not type "git submodule summary" would not
get hurt by the addition.  For a new feature, that is more important than
how well the new feature works and how useful the new feature is.  And by
shipping a release with it will give it a wider exposure and hopefully a
chance for it to mature to get more useful.  The series just got into a
"harmless to others and is releasable" shape.

Recall how many rounds "submodule summary" took to get into that state,
and how much time and effort were spent on it.  Use it as a yardstick to
guesstimate how much further effort and time will be needed to get the
changes to git-status into a reasonable shape.

It may happen eventually, but not before 1.5.5.

Re: builtin-status submodule summary

[Ping Yin]

On Sat, Mar 15, 2008 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Ping Yin" <pkufranky@gmail.com> writes:
>
>  > So i think this series should go along with the submodule summary series.
>
>
>  Recall how many rounds "submodule summary" took to get into that state,
>  and how much time and effort were spent on it.  Use it as a yardstick to
>  guesstimate how much further effort and time will be needed to get the
>  changes to git-status into a reasonable shape.
>
>  It may happen eventually, but not before 1.5.5.
>
So i'll resend my patches considering suggestions from Johannes Sixt
after 1.5.5.



-- 
Ping Yin