[RFC/PATCH v3 0/4] Disable "git status" comment prefix

[RFC/PATCH v3 0/4] Disable "git status" comment prefix

[Matthieu Moy]

So, here's a reroll that makes the code cleaner.

First patches are cleanups without behavioral changes, only the last
one does something new.

I'm waiting for more comments to decide what to do with the
configuration option. Right now, my preference would be to call it
status.oldStyle and default it to false (i.e. change the behavior, but
allow old-timers to get back the old one).

Matthieu Moy (4):
  builtin/stripspace.c: fix broken indentation
  wt-status: use argv_array API
  get rid of "git submodule summary --for-status"
  status: introduce status.displayCommentChar to disable display of #

 Documentation/config.txt     |  5 +++
 builtin/commit.c             |  9 +++++
 builtin/stripspace.c         |  8 ++---
 git-submodule.sh             | 17 +--------
 t/t7401-submodule-summary.sh | 13 -------
 t/t7508-status.sh            | 43 +++++++++++++++++++++++
 wt-status.c                  | 82 +++++++++++++++++++++++++++++++++-----------
 wt-status.h                  |  1 +
 8 files changed, 125 insertions(+), 53 deletions(-)

-- 
1.8.4.12.gf9d53a3.dirty

[PATCH v3 1/4] builtin/stripspace.c: fix broken indentation

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin/stripspace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index e981dfb..1259ed7 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -89,11 +89,11 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 
 	if (argc == 2) {
 		if (!strcmp(argv[1], "-s") ||
-			!strcmp(argv[1], "--strip-comments")) {
-			 strip_comments = 1;
+		    !strcmp(argv[1], "--strip-comments")) {
+			strip_comments = 1;
 		} else if (!strcmp(argv[1], "-c") ||
-					 !strcmp(argv[1], "--comment-lines")) {
-			 mode = COMMENT_LINES;
+			   !strcmp(argv[1], "--comment-lines")) {
+			mode = COMMENT_LINES;
 		} else {
 			mode = INVAL;
 		}
-- 
1.8.4.12.gf9d53a3.dirty

[PATCH v3 2/4] wt-status: use argv_array API

[Matthieu Moy]

No behavior change, but two slight code reorganization: argv_array_push
doesn't accept NULL strings, and duplicates its argument hence
summary_limit must be written to before being inserted into argv.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 wt-status.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index cb24f1f..958a53c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -8,6 +8,7 @@
 #include "diffcore.h"
 #include "quote.h"
 #include "run-command.h"
+#include "argv-array.h"
 #include "remote.h"
 #include "refs.h"
 #include "submodule.h"
@@ -663,29 +664,30 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	char summary_limit[64];
 	char index[PATH_MAX];
 	const char *env[] = { NULL, NULL };
-	const char *argv[8];
-
-	env[0] =	index;
-	argv[0] =	"submodule";
-	argv[1] =	"summary";
-	argv[2] =	uncommitted ? "--files" : "--cached";
-	argv[3] =	"--for-status";
-	argv[4] =	"--summary-limit";
-	argv[5] =	summary_limit;
-	argv[6] =	uncommitted ? NULL : (s->amend ? "HEAD^" : "HEAD");
-	argv[7] =	NULL;
+	struct argv_array argv = ARGV_ARRAY_INIT;
 
 	sprintf(summary_limit, "%d", s->submodule_summary);
 	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
 
+	env[0] = index;
+	argv_array_push(&argv, "submodule");
+	argv_array_push(&argv, "summary");
+	argv_array_push(&argv, uncommitted ? "--files" : "--cached");
+	argv_array_push(&argv, "--for-status");
+	argv_array_push(&argv, "--summary-limit");
+	argv_array_push(&argv, summary_limit);
+	if (!uncommitted)
+		argv_array_push(&argv, s->amend ? "HEAD^" : "HEAD");
+
 	memset(&sm_summary, 0, sizeof(sm_summary));
-	sm_summary.argv = argv;
+	sm_summary.argv = 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);
+	argv_array_clear(&argv);
 }
 
 static void wt_status_print_other(struct wt_status *s,
-- 
1.8.4.12.gf9d53a3.dirty

[PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Matthieu Moy]

The --for-status option was an undocumented option used only by
wt-status.c, which inserted a header and commented out the output. We can
achieve the same result within wt-status.c, without polluting the
submodule command-line options.

This will make it easier to disable the comments from wt-status.c later.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 git-submodule.sh             | 17 +----------------
 t/t7401-submodule-summary.sh | 13 -------------
 wt-status.c                  | 29 +++++++++++++++++++++++++++--
 3 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2979197..fccdec9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -965,7 +965,6 @@ set_name_rev () {
 #
 cmd_summary() {
 	summary_limit=-1
-	for_status=
 	diff_cmd=diff-index
 
 	# parse $args after "submodule ... summary".
@@ -978,9 +977,6 @@ cmd_summary() {
 		--files)
 			files="$1"
 			;;
-		--for-status)
-			for_status="$1"
-			;;
 		-n|--summary-limit)
 			summary_limit="$2"
 			isnumber "$summary_limit" || usage
@@ -1149,18 +1145,7 @@ cmd_summary() {
 			echo
 		fi
 		echo
-	done |
-	if test -n "$for_status"; then
-		if [ -n "$files" ]; then
-			gettextln "Submodules changed but not updated:" | git stripspace -c
-		else
-			gettextln "Submodule changes to be committed:" | git stripspace -c
-		fi
-		printf "\n" | git stripspace -c
-		git stripspace -c
-	else
-		cat
-	fi
+	done
 }
 #
 # List all submodules, prefixed with:
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index ac2434c..b435d03 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -262,19 +262,6 @@ EOF
 	test_cmp expected actual
 "
 
-test_expect_success '--for-status' "
-	git submodule summary --for-status HEAD^ >actual &&
-	test_i18ncmp actual - <<EOF
-# Submodule changes to be committed:
-#
-# * sm1 $head6...0000000:
-#
-# * sm2 0000000...$head7 (2):
-#   > Add foo9
-#
-EOF
-"
-
 test_expect_success 'fail when using --files together with --cached' "
 	test_must_fail git submodule summary --files --cached
 "
diff --git a/wt-status.c b/wt-status.c
index 958a53c..d91661d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	char index[PATH_MAX];
 	const char *env[] = { NULL, NULL };
 	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct strbuf cmd_stdout = STRBUF_INIT;
+	struct strbuf summary = STRBUF_INIT;
+	char *summary_content;
+	size_t len;
 
 	sprintf(summary_limit, "%d", s->submodule_summary);
 	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
@@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	argv_array_push(&argv, "submodule");
 	argv_array_push(&argv, "summary");
 	argv_array_push(&argv, uncommitted ? "--files" : "--cached");
-	argv_array_push(&argv, "--for-status");
 	argv_array_push(&argv, "--summary-limit");
 	argv_array_push(&argv, summary_limit);
 	if (!uncommitted)
@@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	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.out = -1;
+
 	run_command(&sm_summary);
 	argv_array_clear(&argv);
+
+	len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
+
+	/* prepend header, only if there's an actual output */
+	if (len) {
+		if (uncommitted)
+			strbuf_addstr(&summary, _("Submodules changed but not updated:"));
+		else
+			strbuf_addstr(&summary, _("Submodule changes to be committed:"));
+		strbuf_addstr(&summary, "\n\n");
+	}
+	strbuf_addbuf(&summary, &cmd_stdout);
+	strbuf_release(&cmd_stdout);
+
+	summary_content = strbuf_detach(&summary, &len);
+	strbuf_add_commented_lines(&summary, summary_content, len);
+	free(summary_content);
+
+	summary_content = strbuf_detach(&summary, &len);
+	fprintf(s->fp, summary_content);
+	free(summary_content);
 }
 
 static void wt_status_print_other(struct wt_status *s,
-- 
1.8.4.12.gf9d53a3.dirty

[PATCH v3 4/4] status: introduce status.displayCommentChar to disable display of #

[Matthieu Moy]

Historically, "git status" needed to prefix each output line with '#' so
that the output could be added as comment to the commit message. This
prefix comment has no real purpose when "git status" is ran from the
command-line, and this may distract users from the real content.

Allow the user to disable this prefix comment. In the long run, if users
like the non-prefix output, it may make sense to flip the default value
to true.

Obviously, status.displayCommentChar applies to "git status" but is
ignored by "git commit", so the status is still commented in
COMMIT_EDITMSG.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/config.txt |  5 +++++
 builtin/commit.c         |  9 +++++++++
 t/t7508-status.sh        | 43 +++++++++++++++++++++++++++++++++++++++++++
 wt-status.c              | 35 +++++++++++++++++++++++++----------
 wt-status.h              |  1 +
 5 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..0afa531 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2118,6 +2118,11 @@ status.branch::
 	Set to true to enable --branch by default in linkgit:git-status[1].
 	The option --no-branch takes precedence over this variable.
 
+status.displayCommentChar::
+	If set to false, linkgit:git-status[1] will not prefix each
+	line with the comment char (`core.commentChar`, i.e. `#` by
+	default). Defaults to true.
+
 status.showUntrackedFiles::
 	By default, linkgit:git-status[1] and linkgit:git-commit[1] show
 	files which are not currently tracked by Git. Directories which
diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..d4cfced 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1182,6 +1182,10 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		s->use_color = git_config_colorbool(k, v);
 		return 0;
 	}
+	if (!strcmp(k, "status.displaycommentchar")) {
+		s->display_comment_char = git_config_bool(k, v);
+		return 0;
+	}
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
 		int slot = parse_status_slot(k, 13);
 		if (slot < 0)
@@ -1496,6 +1500,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	git_config(git_commit_config, &s);
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
+	/*
+	 * Ignore status.displayCommentChar: we do need comments in
+	 * COMMIT_EDITMSG.
+	 */
+	s.display_comment_char = 1;
 	determine_whence(&s);
 	s.colopts = 0;
 
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index ac3d0fe..d0d7c4a 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -113,6 +113,39 @@ test_expect_success 'status (2)' '
 	test_i18ncmp expect output
 '
 
+strip_comments () {
+	sed "s/^\# //; s/^\#$//; s/^#\t/\t/" <"$1" >"$1".tmp &&
+	rm "$1" && mv "$1".tmp "$1"
+}
+
+test_expect_success 'status with status.displayCommentChar=false' '
+	strip_comments expect &&
+	git -c status.displayCommentChar=false status >output &&
+	test_i18ncmp expect output
+'
+
+test_expect_success 'setup fake editor' '
+	cat >.git/editor <<-\EOF &&
+	#! /bin/sh
+	cp "$1" output
+EOF
+	chmod 755 .git/editor
+'
+
+commit_template_commented () {
+	(
+		EDITOR=.git/editor &&
+		export EDITOR &&
+		# Fails due to empty message
+		test_must_fail git commit
+	) &&
+	! grep '^[^#]' output
+}
+
+test_expect_success 'commit ignores status.displayCommentChar=false in COMMIT_EDITMSG' '
+	commit_template_commented
+'
+
 cat >expect <<\EOF
 # On branch master
 # Changes to be committed:
@@ -872,6 +905,16 @@ test_expect_success 'status submodule summary' '
 	test_i18ncmp expect output
 '
 
+test_expect_success 'status submodule summary with status.displayCommentChar=false' '
+	strip_comments expect &&
+	git -c status.displayCommentChar=false status >output &&
+	test_i18ncmp expect output
+'
+
+test_expect_success 'commit with submodule summary ignores displayCommentChar' '
+	commit_template_commented
+'
+
 cat >expect <<EOF
  M dir1/modified
 A  dir2/added
diff --git a/wt-status.c b/wt-status.c
index d91661d..cdbcd6f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -46,9 +46,11 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 
 	strbuf_vaddf(&sb, fmt, ap);
 	if (!sb.len) {
-		strbuf_addch(&sb, comment_line_char);
-		if (!trail)
-			strbuf_addch(&sb, ' ');
+		if (s->display_comment_char) {
+			strbuf_addch(&sb, comment_line_char);
+			if (!trail)
+				strbuf_addch(&sb, ' ');
+		}
 		color_print_strbuf(s->fp, color, &sb);
 		if (trail)
 			fprintf(s->fp, "%s", trail);
@@ -59,7 +61,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 		eol = strchr(line, '\n');
 
 		strbuf_reset(&linebuf);
-		if (at_bol) {
+		if (at_bol && s->display_comment_char) {
 			strbuf_addch(&linebuf, comment_line_char);
 			if (*line != '\n' && *line != '\t')
 				strbuf_addch(&linebuf, ' ');
@@ -129,6 +131,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->untracked.strdup_strings = 1;
 	s->ignored.strdup_strings = 1;
 	s->show_branch = -1;  /* unspecified */
+	s->display_comment_char = 1;
 }
 
 static void wt_status_print_unmerged_header(struct wt_status *s)
@@ -706,9 +709,11 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	strbuf_addbuf(&summary, &cmd_stdout);
 	strbuf_release(&cmd_stdout);
 
-	summary_content = strbuf_detach(&summary, &len);
-	strbuf_add_commented_lines(&summary, summary_content, len);
-	free(summary_content);
+	if (s->display_comment_char) {
+		summary_content = strbuf_detach(&summary, &len);
+		strbuf_add_commented_lines(&summary, summary_content, len);
+		free(summary_content);
+	}
 
 	summary_content = strbuf_detach(&summary, &len);
 	fprintf(s->fp, summary_content);
@@ -793,6 +798,8 @@ static void wt_status_print_tracking(struct wt_status *s)
 	struct strbuf sb = STRBUF_INIT;
 	const char *cp, *ep;
 	struct branch *branch;
+	char comment_line_string[3];
+	int i;
 
 	assert(s->branch && !s->is_initial);
 	if (prefixcmp(s->branch, "refs/heads/"))
@@ -801,12 +808,20 @@ static void wt_status_print_tracking(struct wt_status *s)
 	if (!format_tracking_info(branch, &sb))
 		return;
 
+	i = 0;
+	if (s->display_comment_char) {
+		comment_line_string[i++] = comment_line_char;
+		comment_line_string[i++] = ' ';
+	}
+	comment_line_string[i] = '\0';
+
 	for (cp = sb.buf; (ep = strchr(cp, '\n')) != NULL; cp = ep + 1)
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s),
-				 "%c %.*s", comment_line_char,
+				 "%s%.*s", comment_line_string,
 				 (int)(ep - cp), cp);
-	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
-			 comment_line_char);
+	if (s->display_comment_char)
+		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
+				 comment_line_char);
 }
 
 static int has_unmerged(struct wt_status *s)
diff --git a/wt-status.h b/wt-status.h
index fb7152e..ac02c64 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -50,6 +50,7 @@ struct wt_status {
 	enum commit_whence whence;
 	int nowarn;
 	int use_color;
+	int display_comment_char;
 	int relative_paths;
 	int submodule_summary;
 	int show_ignored_files;
-- 
1.8.4.12.gf9d53a3.dirty

Re: [RFC/PATCH v3 0/4] Disable "git status" comment prefix

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> ... Right now, my preference would be to call it
> status.oldStyle and default it to false (i.e. change the behavior, but
> allow old-timers to get back the old one).

Sounds sensible, at least to me.

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Jens Lehmann]

Am 29.08.2013 15:05, schrieb Matthieu Moy:
> The --for-status option was an undocumented option used only by
> wt-status.c, which inserted a header and commented out the output. We can
> achieve the same result within wt-status.c, without polluting the
> submodule command-line options.
> 
> This will make it easier to disable the comments from wt-status.c later.

Cool, thanks for implementing this!

But unfortunately this change collides with bc/submodule-status-ignored
(I added Brian to the CC) which is currently on its way to next. Your
patch will break the fix in the second commit, because that's only
enabled when the submodule script sees the --for-status option.

A solution for that would be to rebase your patches on top of pu, drop
the first two hunks of the change to git-submodule.sh and still pass
the --for-status option to git-submodule.sh. This would move adding the
comment characters into wt-status.c but will still enable the script to
honor the ignore=all setting when called by status.

Junio, what is our take on changing behavior of undocumented internally
used options? Do we have to add a new --for-status2 (which doesn't add
the comment characters) or is it ok to just change the behavior of the
existing option?

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  git-submodule.sh             | 17 +----------------
>  t/t7401-submodule-summary.sh | 13 -------------
>  wt-status.c                  | 29 +++++++++++++++++++++++++++--
>  3 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2979197..fccdec9 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -965,7 +965,6 @@ set_name_rev () {
>  #
>  cmd_summary() {
>  	summary_limit=-1
> -	for_status=
>  	diff_cmd=diff-index
>  
>  	# parse $args after "submodule ... summary".
> @@ -978,9 +977,6 @@ cmd_summary() {
>  		--files)
>  			files="$1"
>  			;;
> -		--for-status)
> -			for_status="$1"
> -			;;
>  		-n|--summary-limit)
>  			summary_limit="$2"
>  			isnumber "$summary_limit" || usage
> @@ -1149,18 +1145,7 @@ cmd_summary() {
>  			echo
>  		fi
>  		echo
> -	done |
> -	if test -n "$for_status"; then
> -		if [ -n "$files" ]; then
> -			gettextln "Submodules changed but not updated:" | git stripspace -c
> -		else
> -			gettextln "Submodule changes to be committed:" | git stripspace -c
> -		fi
> -		printf "\n" | git stripspace -c
> -		git stripspace -c
> -	else
> -		cat
> -	fi
> +	done
>  }
>  #
>  # List all submodules, prefixed with:
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> index ac2434c..b435d03 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -262,19 +262,6 @@ EOF
>  	test_cmp expected actual
>  "
>  
> -test_expect_success '--for-status' "
> -	git submodule summary --for-status HEAD^ >actual &&
> -	test_i18ncmp actual - <<EOF
> -# Submodule changes to be committed:
> -#
> -# * sm1 $head6...0000000:
> -#
> -# * sm2 0000000...$head7 (2):
> -#   > Add foo9
> -#
> -EOF
> -"
> -
>  test_expect_success 'fail when using --files together with --cached' "
>  	test_must_fail git submodule summary --files --cached
>  "
> diff --git a/wt-status.c b/wt-status.c
> index 958a53c..d91661d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>  	char index[PATH_MAX];
>  	const char *env[] = { NULL, NULL };
>  	struct argv_array argv = ARGV_ARRAY_INIT;
> +	struct strbuf cmd_stdout = STRBUF_INIT;
> +	struct strbuf summary = STRBUF_INIT;
> +	char *summary_content;
> +	size_t len;
>  
>  	sprintf(summary_limit, "%d", s->submodule_summary);
>  	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
> @@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>  	argv_array_push(&argv, "submodule");
>  	argv_array_push(&argv, "summary");
>  	argv_array_push(&argv, uncommitted ? "--files" : "--cached");
> -	argv_array_push(&argv, "--for-status");
>  	argv_array_push(&argv, "--summary-limit");
>  	argv_array_push(&argv, summary_limit);
>  	if (!uncommitted)
> @@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>  	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.out = -1;
> +
>  	run_command(&sm_summary);
>  	argv_array_clear(&argv);
> +
> +	len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
> +
> +	/* prepend header, only if there's an actual output */
> +	if (len) {
> +		if (uncommitted)
> +			strbuf_addstr(&summary, _("Submodules changed but not updated:"));
> +		else
> +			strbuf_addstr(&summary, _("Submodule changes to be committed:"));
> +		strbuf_addstr(&summary, "\n\n");
> +	}
> +	strbuf_addbuf(&summary, &cmd_stdout);
> +	strbuf_release(&cmd_stdout);
> +
> +	summary_content = strbuf_detach(&summary, &len);
> +	strbuf_add_commented_lines(&summary, summary_content, len);
> +	free(summary_content);
> +
> +	summary_content = strbuf_detach(&summary, &len);
> +	fprintf(s->fp, summary_content);
> +	free(summary_content);
>  }
>  
>  static void wt_status_print_other(struct wt_status *s,
> 

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> +	/* prepend header, only if there's an actual output */
> +	if (len) {
> +		if (uncommitted)
> +			strbuf_addstr(&summary, _("Submodules changed but not updated:"));
> +		else
> +			strbuf_addstr(&summary, _("Submodule changes to be committed:"));
> +		strbuf_addstr(&summary, "\n\n");
> +	}
> +	strbuf_addbuf(&summary, &cmd_stdout);
> +	strbuf_release(&cmd_stdout);
> +
> +	summary_content = strbuf_detach(&summary, &len);
> +	strbuf_add_commented_lines(&summary, summary_content, len);
> +	free(summary_content);
> +
> +	summary_content = strbuf_detach(&summary, &len);
> +	fprintf(s->fp, summary_content);
> +	free(summary_content);

This "fprintf()" looks bogus to me.  How about adding this on top?

 wt-status.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index d91661d..1f17652 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	strbuf_add_commented_lines(&summary, summary_content, len);
 	free(summary_content);
 
-	summary_content = strbuf_detach(&summary, &len);
-	fprintf(s->fp, summary_content);
-	free(summary_content);
+	fputs(summary.buf, s->fp);
+	strbuf_release(&summary);
 }
 
 static void wt_status_print_other(struct wt_status *s,

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> +	/* prepend header, only if there's an actual output */
>> +	if (len) {
>> +		if (uncommitted)
>> +			strbuf_addstr(&summary, _("Submodules changed but not updated:"));
>> +		else
>> +			strbuf_addstr(&summary, _("Submodule changes to be committed:"));
>> +		strbuf_addstr(&summary, "\n\n");
>> +	}
>> +	strbuf_addbuf(&summary, &cmd_stdout);
>> +	strbuf_release(&cmd_stdout);
>> +
>> +	summary_content = strbuf_detach(&summary, &len);
>> +	strbuf_add_commented_lines(&summary, summary_content, len);
>> +	free(summary_content);
>> +
>> +	summary_content = strbuf_detach(&summary, &len);
>> +	fprintf(s->fp, summary_content);
>> +	free(summary_content);
>
> This "fprintf()" looks bogus to me.

Oops, indeed. I forgot the "%s".

> How about adding this on top?

Your solution is better, yes.

>  wt-status.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index d91661d..1f17652 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>  	strbuf_add_commented_lines(&summary, summary_content, len);
>  	free(summary_content);
>  
> -	summary_content = strbuf_detach(&summary, &len);
> -	fprintf(s->fp, summary_content);
> -	free(summary_content);
> +	fputs(summary.buf, s->fp);
> +	strbuf_release(&summary);
>  }
>  
>  static void wt_status_print_other(struct wt_status *s,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Matthieu Moy]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 29.08.2013 15:05, schrieb Matthieu Moy:
>> The --for-status option was an undocumented option used only by
>> wt-status.c, which inserted a header and commented out the output. We can
>> achieve the same result within wt-status.c, without polluting the
>> submodule command-line options.
>> 
>> This will make it easier to disable the comments from wt-status.c later.
>
> Cool, thanks for implementing this!
>
> But unfortunately this change collides with bc/submodule-status-ignored
> (I added Brian to the CC) which is currently on its way to next.

Thanks for pointing that out. The patch looks buggy:

--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,6 +1036,13 @@ cmd_summary() {
                do
                        # Always show modules deleted or type-changed (blob<->module)
                        test $status = D -o $status = T && echo "$sm_path" && continue
+                       # Respect the ignore setting for --for-status.
+                       if test -n $for_status
+                       then
+                               name=$(module_name "$sm_path")
+                               ignore_config=$(get_submodule_config "$name" ignore none)
+                               test $status != A -a $ignore_config = all && continue
+                       fi

Because of the missing quotes around $for_status, it seems the test is
unconditionnaly true:

$ test -n t ; echo $?
0
$ test -n   ; echo $?
0

This makes me wonder why the ignore configuration should be considered
only with --for-status. Why not turn that into

--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,6 +1036,13 @@ cmd_summary() {
                do
                        # Always show modules deleted or type-changed (blob<->module)
                        test $status = D -o $status = T && echo "$sm_path" && continue
+			# Respect the ignore setting
+			name=$(module_name "$sm_path")
+			ignore_config=$(get_submodule_config "$name" ignore none)
+			test $status != A -a $ignore_config = all && continue

?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Jens Lehmann]

Am 29.08.2013 23:23, schrieb Matthieu Moy:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Am 29.08.2013 15:05, schrieb Matthieu Moy:
>>> The --for-status option was an undocumented option used only by
>>> wt-status.c, which inserted a header and commented out the output. We can
>>> achieve the same result within wt-status.c, without polluting the
>>> submodule command-line options.
>>>
>>> This will make it easier to disable the comments from wt-status.c later.
>>
>> Cool, thanks for implementing this!
>>
>> But unfortunately this change collides with bc/submodule-status-ignored
>> (I added Brian to the CC) which is currently on its way to next.
> 
> Thanks for pointing that out. The patch looks buggy:

Ok, I'll tak

> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1036,6 +1036,13 @@ cmd_summary() {
>                 do
>                         # Always show modules deleted or type-changed (blob<->module)
>                         test $status = D -o $status = T && echo "$sm_path" && continue
> +                       # Respect the ignore setting for --for-status.
> +                       if test -n $for_status
> +                       then
> +                               name=$(module_name "$sm_path")
> +                               ignore_config=$(get_submodule_config "$name" ignore none)
> +                               test $status != A -a $ignore_config = all && continue
> +                       fi
> 
> Because of the missing quotes around $for_status, it seems the test is
> unconditionnaly true:
> 
> $ test -n t ; echo $?
> 0
> $ test -n   ; echo $?
> 0
> 
> This makes me wonder why the ignore configuration should be considered
> only with --for-status. Why not turn that into
> 
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1036,6 +1036,13 @@ cmd_summary() {
>                 do
>                         # Always show modules deleted or type-changed (blob<->module)
>                         test $status = D -o $status = T && echo "$sm_path" && continue
> +			# Respect the ignore setting
> +			name=$(module_name "$sm_path")
> +			ignore_config=$(get_submodule_config "$name" ignore none)
> +			test $status != A -a $ignore_config = all && continue
> 
> ?
> 

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Jens Lehmann]

Am 30.08.2013 21:40, schrieb Jens Lehmann:
> Am 29.08.2013 23:23, schrieb Matthieu Moy:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:
>>>> The --for-status option was an undocumented option used only by
>>>> wt-status.c, which inserted a header and commented out the output. We can
>>>> achieve the same result within wt-status.c, without polluting the
>>>> submodule command-line options.
>>>>
>>>> This will make it easier to disable the comments from wt-status.c later.
>>>
>>> Cool, thanks for implementing this!
>>>
>>> But unfortunately this change collides with bc/submodule-status-ignored
>>> (I added Brian to the CC) which is currently on its way to next.
>>
>> Thanks for pointing that out. The patch looks buggy:
> 
> Ok, I'll tak

Sorry, I accidentally hit "Send" ... :-(

Ok, I'll take a look and will comment on that soon.

>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -1036,6 +1036,13 @@ cmd_summary() {
>>                 do
>>                         # Always show modules deleted or type-changed (blob<->module)
>>                         test $status = D -o $status = T && echo "$sm_path" && continue
>> +                       # Respect the ignore setting for --for-status.
>> +                       if test -n $for_status
>> +                       then
>> +                               name=$(module_name "$sm_path")
>> +                               ignore_config=$(get_submodule_config "$name" ignore none)
>> +                               test $status != A -a $ignore_config = all && continue
>> +                       fi
>>
>> Because of the missing quotes around $for_status, it seems the test is
>> unconditionnaly true:
>>
>> $ test -n t ; echo $?
>> 0
>> $ test -n   ; echo $?
>> 0
>>
>> This makes me wonder why the ignore configuration should be considered
>> only with --for-status. Why not turn that into

Please don't. That changes the default behavior of submodule summary,
which never ignores any submodules. The ignore logic was added to core
git after commands like diff and status learned to check submodules for
modifications too. That was bad for people who used submodules to store
many and/or huge files in a way that wouldn't slow down diff or status,
as it slowed them down again. The ignore option allowed them to continue
using submodules for that purpose. They still need to have the submodule
script ignore the ignore settings, because running them is the point in
time they want to take the extra effort to look into those submodules
they normally ignore. And that's why the submodule totally lacks any
option to control the ignore behavior, which we would also have to add
if we would follow your proposal.

So I think it's either changing the default behavior of --for-status or
adding another option (--for-status-wo-comment or such) which will honor
the ignore setting only when called from status.

>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -1036,6 +1036,13 @@ cmd_summary() {
>>                 do
>>                         # Always show modules deleted or type-changed (blob<->module)
>>                         test $status = D -o $status = T && echo "$sm_path" && continue
>> +			# Respect the ignore setting
>> +			name=$(module_name "$sm_path")
>> +			ignore_config=$(get_submodule_config "$name" ignore none)
>> +			test $status != A -a $ignore_config = all && continue
>>
>> ?
>>
> 

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Jens Lehmann]

Am 30.08.2013 21:51, schrieb Jens Lehmann:
> Am 30.08.2013 21:40, schrieb Jens Lehmann:
>> Am 29.08.2013 23:23, schrieb Matthieu Moy:
>>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>>
>>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:
>>>>> The --for-status option was an undocumented option used only by
>>>>> wt-status.c, which inserted a header and commented out the output. We can
>>>>> achieve the same result within wt-status.c, without polluting the
>>>>> submodule command-line options.
>>>>>
>>>>> This will make it easier to disable the comments from wt-status.c later.
>>>>
>>>> Cool, thanks for implementing this!
>>>>
>>>> But unfortunately this change collides with bc/submodule-status-ignored
>>>> (I added Brian to the CC) which is currently on its way to next.
>>>
>>> Thanks for pointing that out. The patch looks buggy:
>>
>> Ok, I'll tak
> 
> Sorry, I accidentally hit "Send" ... :-(
> 
> Ok, I'll take a look and will comment on that soon.
> 
>>> --- a/git-submodule.sh
>>> +++ b/git-submodule.sh
>>> @@ -1036,6 +1036,13 @@ cmd_summary() {
>>>                 do
>>>                         # Always show modules deleted or type-changed (blob<->module)
>>>                         test $status = D -o $status = T && echo "$sm_path" && continue
>>> +                       # Respect the ignore setting for --for-status.
>>> +                       if test -n $for_status
>>> +                       then
>>> +                               name=$(module_name "$sm_path")
>>> +                               ignore_config=$(get_submodule_config "$name" ignore none)
>>> +                               test $status != A -a $ignore_config = all && continue
>>> +                       fi
>>>
>>> Because of the missing quotes around $for_status, it seems the test is
>>> unconditionnaly true:
>>>
>>> $ test -n t ; echo $?
>>> 0
>>> $ test -n   ; echo $?
>>> 0

Right you are, I did not notice the missing "" in my review. Looks like
we also should add one or more tests making sure that submodule summary
and status never honor the ignore settings.

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote:
> Am 30.08.2013 21:51, schrieb Jens Lehmann:
> > Am 30.08.2013 21:40, schrieb Jens Lehmann:
> >> Am 29.08.2013 23:23, schrieb Matthieu Moy:
> >>> Jens Lehmann <Jens.Lehmann@web.de> writes:
> >>>
> >>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:
> >>> Because of the missing quotes around $for_status, it seems the test is
> >>> unconditionnaly true:
> >>>
> >>> $ test -n t ; echo $?
> >>> 0
> >>> $ test -n   ; echo $?
> >>> 0
> 
> Right you are, I did not notice the missing "" in my review. Looks like
> we also should add one or more tests making sure that submodule summary
> and status never honor the ignore settings.

How do we want to handle this?  I can send a reroll and include some
new tests, but if this code is going away, then there's no point.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Jens Lehmann]

Am 31.08.2013 19:08, schrieb brian m. carlson:
> On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote:
>> Am 30.08.2013 21:51, schrieb Jens Lehmann:
>>> Am 30.08.2013 21:40, schrieb Jens Lehmann:
>>>> Am 29.08.2013 23:23, schrieb Matthieu Moy:
>>>>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>>>>
>>>>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:
>>>>> Because of the missing quotes around $for_status, it seems the test is
>>>>> unconditionnaly true:
>>>>>
>>>>> $ test -n t ; echo $?
>>>>> 0
>>>>> $ test -n   ; echo $?
>>>>> 0
>>
>> Right you are, I did not notice the missing "" in my review. Looks like
>> we also should add one or more tests making sure that submodule summary
>> and status never honor the ignore settings.
> 
> How do we want to handle this?  I can send a reroll and include some
> new tests, but if this code is going away, then there's no point.

A reroll would be great, as I think your patch is a bugfix that should
go in rather soonish no matter how we continue with the comment signs.
Two new tests (one for submodule summary and one for submodule status)
with both the global ignore setting and a submodule specific one set
to "all" showing no impact on the output would suffice (and trigger the
then also fixed missing "" bug ;-).

Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"

[Jens Lehmann]

Am 29.08.2013 21:54, schrieb Jens Lehmann:
> Am 29.08.2013 15:05, schrieb Matthieu Moy:
>> The --for-status option was an undocumented option used only by
>> wt-status.c, which inserted a header and commented out the output. We can
>> achieve the same result within wt-status.c, without polluting the
>> submodule command-line options.
>>
>> This will make it easier to disable the comments from wt-status.c later.
> 
> Cool, thanks for implementing this!
> 
> But unfortunately this change collides with bc/submodule-status-ignored
> (I added Brian to the CC) which is currently on its way to next. Your
> patch will break the fix in the second commit, because that's only
> enabled when the submodule script sees the --for-status option.
> 
> A solution for that would be to rebase your patches on top of pu, drop
> the first two hunks of the change to git-submodule.sh and still pass
> the --for-status option to git-submodule.sh. This would move adding the
> comment characters into wt-status.c but will still enable the script to
> honor the ignore=all setting when called by status.

I think we should go that route, --for-status is an internal option and
nobody should rely on its behavior.

>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>>  git-submodule.sh             | 17 +----------------
>>  t/t7401-submodule-summary.sh | 13 -------------
>>  wt-status.c                  | 29 +++++++++++++++++++++++++++--
>>  3 files changed, 28 insertions(+), 31 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 2979197..fccdec9 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -965,7 +965,6 @@ set_name_rev () {
>>  #
>>  cmd_summary() {
>>  	summary_limit=-1
>> -	for_status=
>>  	diff_cmd=diff-index
>>  
>>  	# parse $args after "submodule ... summary".
>> @@ -978,9 +977,6 @@ cmd_summary() {
>>  		--files)
>>  			files="$1"
>>  			;;
>> -		--for-status)
>> -			for_status="$1"
>> -			;;
>>  		-n|--summary-limit)
>>  			summary_limit="$2"
>>  			isnumber "$summary_limit" || usage

Please drop the two hunks above ...

>> @@ -1149,18 +1145,7 @@ cmd_summary() {
>>  			echo
>>  		fi
>>  		echo
>> -	done |
>> -	if test -n "$for_status"; then
>> -		if [ -n "$files" ]; then
>> -			gettextln "Submodules changed but not updated:" | git stripspace -c
>> -		else
>> -			gettextln "Submodule changes to be committed:" | git stripspace -c
>> -		fi
>> -		printf "\n" | git stripspace -c
>> -		git stripspace -c
>> -	else
>> -		cat
>> -	fi
>> +	done
>>  }
>>  #
>>  # List all submodules, prefixed with:
>> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
>> index ac2434c..b435d03 100755
>> --- a/t/t7401-submodule-summary.sh
>> +++ b/t/t7401-submodule-summary.sh
>> @@ -262,19 +262,6 @@ EOF
>>  	test_cmp expected actual
>>  "
>>  
>> -test_expect_success '--for-status' "
>> -	git submodule summary --for-status HEAD^ >actual &&
>> -	test_i18ncmp actual - <<EOF
>> -# Submodule changes to be committed:
>> -#
>> -# * sm1 $head6...0000000:
>> -#
>> -# * sm2 0000000...$head7 (2):
>> -#   > Add foo9
>> -#
>> -EOF
>> -"
>> -

... and just remove the "# " from the expected output here. This
test can be removed when we use

>>  test_expect_success 'fail when using --files together with --cached' "
>>  	test_must_fail git submodule summary --files --cached
>>  "
>> diff --git a/wt-status.c b/wt-status.c
>> index 958a53c..d91661d 100644
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>>  	char index[PATH_MAX];
>>  	const char *env[] = { NULL, NULL };
>>  	struct argv_array argv = ARGV_ARRAY_INIT;
>> +	struct strbuf cmd_stdout = STRBUF_INIT;
>> +	struct strbuf summary = STRBUF_INIT;
>> +	char *summary_content;
>> +	size_t len;
>>  
>>  	sprintf(summary_limit, "%d", s->submodule_summary);
>>  	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
>> @@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>>  	argv_array_push(&argv, "submodule");
>>  	argv_array_push(&argv, "summary");
>>  	argv_array_push(&argv, uncommitted ? "--files" : "--cached");
>> -	argv_array_push(&argv, "--for-status");

And the line above has to stay.

>>  	argv_array_push(&argv, "--summary-limit");
>>  	argv_array_push(&argv, summary_limit);
>>  	if (!uncommitted)
>> @@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>>  	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.out = -1;
>> +
>>  	run_command(&sm_summary);
>>  	argv_array_clear(&argv);
>> +
>> +	len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
>> +
>> +	/* prepend header, only if there's an actual output */
>> +	if (len) {
>> +		if (uncommitted)
>> +			strbuf_addstr(&summary, _("Submodules changed but not updated:"));
>> +		else
>> +			strbuf_addstr(&summary, _("Submodule changes to be committed:"));
>> +		strbuf_addstr(&summary, "\n\n");
>> +	}
>> +	strbuf_addbuf(&summary, &cmd_stdout);
>> +	strbuf_release(&cmd_stdout);
>> +
>> +	summary_content = strbuf_detach(&summary, &len);
>> +	strbuf_add_commented_lines(&summary, summary_content, len);
>> +	free(summary_content);
>> +
>> +	summary_content = strbuf_detach(&summary, &len);
>> +	fprintf(s->fp, summary_content);
>> +	free(summary_content);
>>  }
>>  
>>  static void wt_status_print_other(struct wt_status *s,

Junio already commented on this part.