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

[Jens Lehmann <Jens.Lehmann@web.de>]

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