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

[Junio C Hamano <junio@pobox.com>]

Ping Yin <pkufranky@gmail.com> writes:

> This commit teaches 'git commit/status' show 'Modified submodules'
> section given by
>
> git submodule summary --cached --for-status --summary-limit <limit>
>
> just before 'Untracked files' section.
>
> The <limit> is given by the config variable status.submodulesummary
> to limit the submodule summary size. status.submodulesummary is 0 by
> default which disables the summary.

This begs an obvious question "What if I want to have an unlimited summary
in the status output?"

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 3ea269a..32b6660 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -52,6 +52,10 @@ If the config variable `status.relativePaths` is set to false, then all
>  paths shown are relative to the repository root, not to the current
>  directory.
>  
> +If 'status.submodulesummary' is set to a non zero number, the submodule
> +summary will be enabled and a summary of commits for modified submodules
> +will be shown (see --summary-limit option of linkgit:git-submodule[1]).
> +

The docs quote configuration variables in typewriter face by using
backticks (pay attention to how status.relativePaths is quoted in your
hunk header comment).

> diff --git a/wt-status.c b/wt-status.c
> index b3fd57b..369c519 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -8,9 +8,11 @@
>  #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;
> +int wt_status_submodule_summary = 0;

Don't initialize to 0.  BSS takes care of it.

>  static char wt_status_colors[][COLOR_MAXLEN] = {
>  	"",         /* WT_STATUS_HEADER: normal */
>  	"\033[32m", /* WT_STATUS_UPDATED: green */
> @@ -219,6 +221,38 @@ 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)
> +{
> ...
> +	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);

I recall we had some clean-up on how file descriptors are inherited and
duped around when run_command() runs a subprocess.  Hannes, is this dup()
consistent with how the things ought to be these days?

> @@ -330,6 +365,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);

With this code, you would feed a NULL to atoi() with:

        [status]
                submoduleSummary

How about making this a bool-or-number, and boolean true gives whatever
the default limit (may be unlimited) when no --summary-limit is given?

We may want to have git_config_bool_or_int() in config.c so that the
caller can tell if the value is boolean true or integer 1, which would be
a reasonable improvement independent from this submodule summary
codepath.

int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
{
        *is_bool = 1;
	if (!value)
		return 1;
	if (!*value)
		return 0;
	if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
		return 1;
	if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))
		return 0;
	*is_bool = 0;
	return git_config_int(name, value) != 0;
}

With that, your configuration parser can do something like:

	if (!strcmp(k, "status.submodulesummary")) {
		int is_bool, val;
		val = git_config_bool_or_int(k, v, &b);
		if (is_bool || val <= 0) {
                	wt_status_submodule_summary_enabled = val;
		} else {
                	wt_status_submodule_summary = val;
                	wt_status_submodule_summary_enabled = 1;
		}
	}

and skip the call to wt_status_print_submodule_summary() when not
enabled.

>  int wt_status_relative_paths = 1;
>  int wt_status_use_color = -1;
> +int wt_status_submodule_summary = -1; /* unspecified */
  +int wt_status_submodule_summary_enabled;

The call site in wt_status_print() would look like: 

	...
 	wt_status_print_changed(s);
	if (wt_status_submodule_summary_enabled > 0)
		wt_status_print_submodule_summary(s);
 	wt_status_print_untracked(s);
	...

and the wt_status_print_submodule_summary() would not add --summary-limit
parameter if wt_status_submodule_summary is left unspecified (i.e. -1).