Revert "builtin/bundle.c: let parse-options parse subcommands"

Commit Message

Hubert Jasudowicz Dec. 20, 2022, 12:31 p.m. UTC

This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.

The change breaks git bundle command. Running any subcommand
results with:

$ git bundle create
Segmentation fault (core dumped)

After reverting the change, everything works correctly.
---
 builtin/bundle.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Comments

Derrick Stolee Dec. 20, 2022, 1:30 p.m. UTC | #1

On 12/20/22 7:31 AM, Hubert Jasudowicz wrote:
> This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.
> 
> The change breaks git bundle command. Running any subcommand
> results with:
> 
> $ git bundle create
> Segmentation fault (core dumped)

Could you be more specific? We have tests that verify that
these commands work without a segfault. There must be something
different about your environment that makes the segfault occur.

One thing that I could believe is that you are running 'git
bundle create' outside of a Git repository, and I doubt we have
tests covering that scenario.

Before reverting this change that has been out in two releases,
I recommend adding a test that demonstrates your failure and
then doing a specific update to fix that scenario.

Thanks,
-Stolee

Ævar Arnfjörð Bjarmason Dec. 20, 2022, 1:42 p.m. UTC | #2

On Tue, Dec 20 2022, Derrick Stolee wrote:

> On 12/20/22 7:31 AM, Hubert Jasudowicz wrote:
>> This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.
>> 
>> The change breaks git bundle command. Running any subcommand
>> results with:
>> 
>> $ git bundle create
>> Segmentation fault (core dumped)
>
> Could you be more specific?

I don't think the report can get more specific than (quoting it):

	$ git bundle create
	Segmentation fault (core dumped)

:) Did you try running it?

> We have tests that verify that
> these commands work without a segfault. There must be something
> different about your environment that makes the segfault occur.

We don't have those tests, I submitted an alternate smaller fix in
https://lore.kernel.org/git/[e-mail]/
that adds some.

I think what you're misrecalling here is probably that we have general
tests for running "git <cmd> -h" for all built-in <cmd>, but we don't
have any such tests for running sub-commands.

And even then, that wouldn't catch this, as it's a bespoke segfault in
the bundle code, as it can't handle not getting at least one non-option
argument.

Patch

diff --git a/builtin/bundle.c b/builtin/bundle.c
index c12c09f854..d9b46f8e03 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -206,19 +206,30 @@  static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
-	parse_opt_subcommand_fn *fn = NULL;
 	struct option options[] = {
-		OPT_SUBCOMMAND("create", &fn, cmd_bundle_create),
-		OPT_SUBCOMMAND("verify", &fn, cmd_bundle_verify),
-		OPT_SUBCOMMAND("list-heads", &fn, cmd_bundle_list_heads),
-		OPT_SUBCOMMAND("unbundle", &fn, cmd_bundle_unbundle),
 		OPT_END()
 	};
+	int result;
 
 	argc = parse_options(argc, argv, prefix, options, builtin_bundle_usage,
-			     0);
+		PARSE_OPT_STOP_AT_NON_OPTION);
 
 	packet_trace_identity("bundle");
 
-	return !!fn(argc, argv, prefix);
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
+
+	else if (!strcmp(argv[0], "create"))
+		result = cmd_bundle_create(argc, argv, prefix);
+	else if (!strcmp(argv[0], "verify"))
+		result = cmd_bundle_verify(argc, argv, prefix);
+	else if (!strcmp(argv[0], "list-heads"))
+		result = cmd_bundle_list_heads(argc, argv, prefix);
+	else if (!strcmp(argv[0], "unbundle"))
+		result = cmd_bundle_unbundle(argc, argv, prefix);
+	else {
+		error(_("Unknown subcommand: %s"), argv[0]);
+		usage_with_options(builtin_bundle_usage, options);
+	}
+	return result ? 1 : 0;
 }