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

[Jeff Hostetler <git@jeffhostetler.com>]

On 07/20/2016 11:58 AM, Jeff King wrote:
> On Tue, Jul 19, 2016 at 06:10:53PM -0400, Jeff Hostetler wrote:
>
>> +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.

Good catch. I'll make it _NONE.

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

I wondered about making it =v1 rather than just =1. It seemed
more aesthetically pleasing, even if it was an extra character
to type.  In a later email in this thread you mention a JSON
option.  If I switched this here to be "=v1" and "=v2", it would
be easy later to have a "=j2" or "=v2j" to do that.


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

I'll look into that.

Jeff