Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands

From: Jeff King <peff@peff.net>
Date: Jul 20, 2016, 15:58 UTC
Message-ID: <20160720155837.GA24902@sigill.intra.peff.net>
In-Reply-To: <1468966258-11191-2-git-send-email-jeffhost@microsoft.com>

On Tue, Jul 19, 2016 at 06:10:53PM -0400, Jeff Hostetler wrote:

Show 5 quoted lines

> +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
> +{
> +	enum wt_status_format *value = (enum wt_status_format *)opt->value;
> +	if (unset) {
> +		*value = STATUS_FORMAT_UNSPECIFIED;

Nice attention to detail here and below in handling "unset" and "!arg" cases. I think should be STATUS_FORMAT_NONE, though, which is what the old code used to do (since "0" is the usual special value for --no-* options). It only matters if you do:

  git status --no-porcelain

Right now that will switch to the long format, regardless of your config. With your path it defaults to any configured value. It's probably a case that nobody hits ever, but in the absence of a good reason to do otherwise, I'd stick with the current behavior.

Show 6 quoted lines

> +	} else if (arg) {
> +		int n = strtol(arg, NULL, 10);
> +		if (n == 1)
> +			*value = STATUS_FORMAT_PORCELAIN;
> +		else
> +			die("unsupported porcelain version");

This silently allows:

  git status --porcelain="1 for the money"

and later:

  git status --porcelain="2 for the show"

Probably not a big deal in practice, but since the list of formats is constrained, we don't really care about parsing arbitrary numbers. So:

  if (!strcmp(arg, "1"))
	*value = STATUS_FORMAT_PORCELAIN;

is actually simpler, and more robust.

I also wondered if:

  git status --porcelain=v1

is more self-documenting about the meaning of "1". It's purely aesthetics, but it somehow looks better to me. Matching that is also much easier with pure strcmps.

Show 6 quoted lines

> @@ -1381,6 +1392,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  
>  	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
>  	s.ignore_submodule_arg = ignore_submodule_arg;
> +	s.status_format = status_format;
> +

I wonder what happens if you pass a "wt_status" with a format of "SHORT" to the long-formatting code.

I think it is ignored completely, as you are just now introducing the s.status_format field. But I wonder if there is room for further cleanup in pushing the big switch statements from run_status() and cmd_status() into wt-status.c.

-Peff