Re: [GSoC PATCH v2 1/2] repo: add the flag -z as an alias for --format=nul

[Junio C Hamano <gitster@pobox.com>]

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.

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

> +	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?