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

From: Jeff King <peff@peff.net>
Date: Nov 12, 2012, 21:17 UTC
Message-ID: <20121112211742.GJ4623@sigill.intra.peff.net>
In-Reply-To: <1352674383-23654-2-git-send-email-thomas@xteddy.org>

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

Show 5 quoted lines

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

Show 20 quoted lines

> 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