Re: [RFC PATCH 1/1] status: Allow for short-form via config option

[Jeff King <peff@peff.net>]

On Sun, Nov 11, 2012 at 10:53:03PM +0000, Thomas Adam wrote:

> It is currently not possible to use the short-form output of git status
> without declaring an alias to do so.
> 
> This isn't always desirable therfore, define a git config option which can
> be set to display the short-form:  status.shortwithbranch

We usually try to avoid booleans for selection among options, even if
there is currently only one useful option. That way, it makes it easier
to extend later when another option presents itself.  So having
"status.format" that you could set to "short", "long", or "porcelain"
would make more sense (although "short" is likely to be the only useful
one currently).

And then we can have a separate boolean like "status.branch" to show the
branch by default when showing short format. That would would naturally
extend to more booleans as other options are added (Phil Hord's recent
"show tree state" patches come to mind).

We also need to consider the impact of options on scripts. I think
status.format should be OK, since any script calling "git status" would
have to explicitly pass "--porcelain" already, which would override this
option. But we would want to make sure that "status.branch" would not
affect the porcelain.

> ---
>  builtin/commit.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

It would need documentation, and tests would be good both to make sure
the feature works, but also to demonstrate that it does not break
--porcelain.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index a17a5df..552a9f1 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1142,6 +1142,18 @@ static int git_status_config(const char *k, const char *v, void *cb)
>  			return error(_("Invalid untracked files mode '%s'"), v);
>  		return 0;
>  	}
> +
> +	if (!strcmp(k, "status.shortwithbranch")) {
> +		if (git_config_bool(k, v)) {
> +			if (!v)
> +				return config_error_nonbool(k);
> +			else if(!strcmp(v, "true")) {
> +				status_format = STATUS_FORMAT_SHORT;
> +				s->show_branch = 1;
> +			}
> +			return 0;
> +		}
> +	}

I'm not sure what is going on with the extra nonbool and "true" check.
Shouldn't it just be:

  if (git_config_bool(k, v)) {
          status_format = STATUS_FORMAT_SHORT;
          s->show_branch = 1;
  }
  else {
          /* what do we do when it is false? */
  }
  return 0;

If we follow my suggestions above, then it would be more like:

  if (!strcmp(k, "status.format")) {
          if (!v)
                  return config_error_nonbool(k);
          return parse_status_format(v, &status_format);
  }
  if (!strcmp(k, "status.branch")) {
          s->show_branch = git_config_bool(k, v);
          return 0;
  }

but that would still have to resolve the setting of show_branch when
--porcelain is in use. I think you would need to store the
config-derived value separately, and then only fill it into
s->show_branch if no value was given on the command-line _and_ we are
not showing the porcelain format.

-Peff