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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
