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@xxxxxxxxx>
No, not encouraged.
But parseoptification does have some nice benefits, so let's see how
the patch looks...
[...]
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> ---
> 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 @@
[...]
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html