threads / patch / 12668

v2, 4 partsbuiltin-status: submodule summary support

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

## tl;dr

15 messages between Mar 13, 2008 and Mar 14, 2008. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Ping Yin· Mar 13, 2008, 13:48 UTC · lore

builtin-status submodule summary

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(-)
Ping Yin· Mar 13, 2008, 13:48 UTC · re: Ping Yin · lore

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

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(-)
Show changes to 2 files +29 −1

git-submodule.sh, t/t7401-submodule-summary.sh

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
Ping Yin· Mar 13, 2008, 13:48 UTC · re: Ping Yin · lore

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(-)
Show changes to wt-status.c +28 −0
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
Ping Yin· Mar 13, 2008, 13:48 UTC · re: Ping Yin · lore

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

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(-)
Show changes to wt-status.c +10 −0
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
Ping Yin· Mar 13, 2008, 13:48 UTC · re: Ping Yin · lore

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

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t7502-status.sh |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 136 insertions(+), 0 deletions(-)
Show changes to t/t7502-status.sh +136 −0
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
Johannes Sixt· Mar 13, 2008, 14:21 UTC · re: Ping Yin · lore

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

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
Junio C Hamano· Mar 13, 2008, 18:04 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 11 quoted lines
> 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".

Ping Yin· Mar 13, 2008, 18:14 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 14, 2008 at 2:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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.
Show changes to t/t7502-status.sh +1 −3
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
Ping Yin· Mar 13, 2008, 18:09 UTC · re: Johannes Sixt · lore

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

On Thu, Mar 13, 2008 at 10:21 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 14 quoted lines
> 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
Johannes Sixt· Mar 13, 2008, 14:15 UTC · re: Ping Yin · lore

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

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
Johannes Sixt· Mar 13, 2008, 14:11 UTC · re: Ping Yin · lore

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

Ping Yin schrieb:
Show 7 quoted lines
> +	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).

Show 5 quoted lines
>  	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
Ping Yin· Mar 13, 2008, 18:01 UTC · re: Johannes Sixt · lore

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

On Thu, Mar 13, 2008 at 10:11 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 10 quoted lines
> 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.
Show changes to wt-status.c +0 −2
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
Ping Yin· Mar 14, 2008, 14:30 UTC · re: Ping Yin · lore

Re: builtin-status submodule summary

On Thu, Mar 13, 2008 at 9:48 PM, Ping Yin <pkufranky@gmail.com> wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Mar 14, 2008, 16:39 UTC · re: Ping Yin · lore

Re: builtin-status submodule summary

"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.
Ping Yin· Mar 14, 2008, 17:45 UTC · re: Junio C Hamano · lore

Re: builtin-status submodule summary

On Sat, Mar 15, 2008 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> "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

← back to recent threads