Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:39
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 [off-list ref]
No, not encouraged. But parseoptification does have some nice benefits, so let's see how the patch looks... [...]
quoted hunk ↗ jump to hunk
Signed-off-by: Ramkumar Ramachandra <redacted> --- 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
[...]
quoted hunk ↗ jump to hunk
@@ -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