Re: [GSoC PATCH v2 1/2] repo: add the flag -z as an alias for --format=nul
Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> writes:
> Other Git commands that have nul-terminated output (e.g. git-config,
> git-status, git-ls-files) have a flag `-z` for using the null character
> as the record separator.
Putting the devil's advocate hat on, "--format=<plain,nul>" was an attempt to avoid needless proliferation of options (e.g. presense of "-z" would tempt people into add "--json" when they introduce "--format=json"), so it may not be unconditionally a good idea to mimic these older commands where there are only two output formats.
But assuming that the short-and-sweet "-z" is something we want to add, the patch itself looks pretty well done, but not quite.
> Add the `-z` flag to git-repo-info as an alias for `--format=nul`,
> making it consistent with the behavior of the other commands.
Show 24 quoted lines
> diff --git a/builtin/repo.c b/builtin/repo.c
> index 8c6e7f42ab..5df33de42e 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -115,20 +115,27 @@ static int print_fields(int argc, const char **argv,
> static int repo_info(int argc, const char **argv, const char *prefix,
> struct repository *repo)
> {
> - const char *format_str = "keyvalue";
> + const char *format_str = NULL;
> enum output_format format;
> + int format_nul = 0;
> struct option options[] = {
> OPT_STRING(0, "format", &format_str, N_("format"),
> N_("output format")),
> + OPT_BOOL('z', NULL, &format_nul, N_("alias for --format=nul")),
> OPT_END()
> };
>
> argc = parse_options(argc, argv, prefix, options, repo_usage, 0);
>
> - if (!strcmp(format_str, "keyvalue"))
> - format = FORMAT_KEYVALUE;
> - else if (!strcmp(format_str, "nul"))> + die_for_incompatible_opt2(!!format_nul, "-z",
> + !!format_str, "--format");
Hmph, so "git repo info --format=nul -z" is now forbidden? That does not make much sense to me.
> + format_str = format_str ? format_str : "keyvalue";
if (!format_str)
format_str = "keyvalue";
is probably easier to follow, but I suspect this becomes a bit of moot point as the general structure of this command line parsing may have to change when you fix the "-z is --format=nul so why are they incompatible?" problem.
Show 6 quoted lines
> + if (format_nul || !strcmp(format_str, "nul"))
> format = FORMAT_NUL_TERMINATED;
> + else if (!strcmp(format_str, "keyvalue"))
> + format = FORMAT_KEYVALUE;
> else
> die(_("invalid format '%s'"), format_str);You'd probably need to define a parseopt callback function for "format" and "-z", and remember the one that you saw the last. So giving "-z --format=nul --format=text" would first set an internal "format" to FORMAT_NUL_TERMINATED (due to "-z"), and then to the same FORMAT_NUL_TERMINATED again (due to "--format=nul"), and then finally to FORMAT_TEXT (due to "--format=text"), or something like that, which would give the familiar "the last one wins" semantics.
Something like (not even compile tested):
static int parse_format_cb(const struct option *opt,
const char *arg, int unset)
{
enum otuput_format *format = opt->value; if (opt->short_name == 'z')
*format = FORMAT_NUL_TERMINATED;
else if (!strcmp(arg, "nul"))
*format = FORMAT_NUL_TERMINATED;
else if (!strcmp(arg, "keyvalue"))
*format = FORMAT_KEYVALUE;
else
die(_("invalid format '--format=%s'", arg));
return 0;
}with
enum output_format format = FORMAT_KEYVALUE;
struct option opt[] = {
OPT_CALLBACK_F(0, "format", &format, N_("format"),
N_("output format"),
PARSE_OPT_NONEG, parse_format_cb),
OPT_CALLBACK_F('z', NULL, &format, NULL,
N_("synonym for --format=nul"),
PARSE_OPT_NONEG|PARSE_OPT_NOARG,
parse_format_cb),
};perhaps?
Lucas Seiki Oshiro
