Re: [PATCH v1 1/6] Allow --porcelain[=<n>] in status and commit commands
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Jul 20, 2016, 15:38 UTC
- Message-ID
- <578F9AE8.4000407@jeffhostetler.com>
- In-Reply-To
- <alpine.DEB.2.20.1607201600050.14111@virtualbox>
On 07/20/2016 11:08 AM, Johannes Schindelin wrote:
Show 18 quoted lines
> On Tue, 19 Jul 2016, Jeff Hostetler wrote:
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> + } else if (arg) {
>> + int n = strtol(arg, NULL, 10);
>> + if (n == 1)
>> + *value = STATUS_FORMAT_PORCELAIN;
>> + else
>> + die("unsupported porcelain version");
>> + } else {
>> + *value = STATUS_FORMAT_PORCELAIN;
>
> This could be folded into the previous conditional:
>
> }
> else {
> int n = arg ? strtol(arg, NULL, 10) : 1;
> ...
>I did it this way because I didn't want to make any assumptions here on the numeric value of the enum values. Or rather, I didn't want to add specific assignments to the enum type.
This also helps make it easier to see my later commit:
else if (n == 2) *value = STATUS_FORMAT_PORCELAIN_V2;Also, I didn't want to alter the order of assignments to the global
status_format variable. That is, if I capture the value of <n> in
a porcelain_version variable and assign that to status_format
afterwards, there is opportunity for mistakes if they type:
git status --short --porcelain=1 --long
where the status_format variable is assigned 3 times.Show 25 quoted lines
> Having said this, ...
>
>> @@ -1336,9 +1347,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>> N_("show status concisely"), STATUS_FORMAT_SHORT),
>> OPT_BOOL('b', "branch", &s.show_branch,
>> N_("show branch information")),
>> - OPT_SET_INT(0, "porcelain", &status_format,
>> - N_("machine-readable output"),
>> - STATUS_FORMAT_PORCELAIN),
>> + { OPTION_CALLBACK, 0, "porcelain", &status_format,
>> + N_("version"), N_("machine-readable output"),
>> + PARSE_OPT_OPTARG, opt_parse_porcelain },
>
> How about using a COUNTUP here instead? We could then set the status
> format afterwards, like this:
>
> if (porcelain == 0)
> status_format = STATUS_FORMAT_UNSPECIFIED;
> else {
> status_format = STATUS_FORMAT_PORCELAIN;
> if (porcelain > 1)
> warning("No porcelain v%d; falling back to v1",
> porcelain);
> }
>Maybe I misread the COUNTUP docs, but it looked like it would allow "--porcelain --porcelain", but not "--porcelain=2".
Jeff