Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 15, 2011, 21:29 UTC
Message-ID: <20111215212901.GC16160@elie.hsd1.il.comcast.net>
In-Reply-To: <1323967528-10537-3-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> The git-bundle builtin currently parses command-line options by hand;
> this is fragile, and reports cryptic errors on failure.  Use the
> parse-options library to do the parsing instead.

I don't understand how this is fragile. I haven't actually run into error messages from "git bundle" I found to be cryptic, but if they are, they surely can be improved locally. Could you give an example or something?

> Encouraged-by: Jonathan Nieder <jrnieder@gmail.com>

No, not encouraged.

But parseoptification does have some nice benefits, so let's see how the patch looks...

[...]

Show 10 quoted lines

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/bundle.c  |   91 +++++++++++++++++++++++++++++++----------------------
>  t/t5704-bundle.sh |    2 +-
>  2 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 92a8a60..13ed770 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c

[...]

> @@ -9,57 +10,71 @@

[...]

Show 15 quoted lines

>  int cmd_bundle(int argc, const char **argv, const char *prefix)
>  {
> -	struct bundle_header header;
> -	const char *cmd, *bundle_file;
> +	int prefix_length;
>  	int bundle_fd = -1;
> -	char buffer[PATH_MAX];
> -	if (argc < 3)
> -		usage(builtin_bundle_usage);
> +	const char *subcommand, *bundle_file;
> +	struct bundle_header header;
> +	struct option options[] = { OPT_END() };
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);

No, just no. Using parse-options with an empty option table is complete overkill for handling the "-h" option. Without a lot more justification, this doesn't make it seem more sane or readable at all.

Stopping here. I wouldn't mind seeing "git bundle" being parseoptified, but not if the result looks like this.

I _do_ think that a systematic option-parsing library that handles subcommands would be something possible and probably useful for git. Its input might include a table with subcommand names, an option table for each, and a function to call when that subcommand is used:

	struct parseopt_subcommand subcmds[] = {
		{ "list", no_options, notes_list },
		{ "add", add_options, notes_add },
		{ "copy", copy_options, notes_copy },
		{ "append", append_options, notes_append },
		{ "edit", no_options, notes_edit },
		{ "show", no_options, notes_show },
		...
	};

Then "git notes -h" might be able to automatically generate a table of synopses, and "gite notes --help-all" might print help for all subcommands. Something like that.

Hope that helps, Jonathan