{"thread":{"id":"58985","subject":"[PATCH] Revert \"builtin/bundle.c: let parse-options parse subcommands\"","startedAt":"2022-12-20T12:32:08Z","lastAt":"2022-12-27T23:42:51Z","messageCount":11,"participants":["Hubert Jasudowicz","Derrick Stolee","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469362","messageId":"20221220123142.812965-1-hubertj@stmcyber.pl","threadId":"58985","inReplyTo":null,"subject":"[PATCH] Revert \"builtin/bundle.c: let parse-options parse subcommands\"","fromName":"Hubert Jasudowicz","fromEmail":"hubertj@stmcyber.pl","sentAt":"2022-12-20T12:31:42Z","receivedAt":"2022-12-20T12:32:08Z","isPatch":true,"sender":{"key":"hubertj@stmcyber.pl","avatar":null},"body":"This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.\n\nThe change breaks git bundle command. Running any subcommand\nresults with:\n\n$ git bundle create\nSegmentation fault (core dumped)\n\nAfter reverting the change, everything works correctly.\n---\n builtin/bundle.c | 25 ++++++++++++++++++-------\n 1 file changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex c12c09f854..d9b46f8e03 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -206,19 +206,30 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \n int cmd_bundle(int argc, const char **argv, const char *prefix)\n {\n-\tparse_opt_subcommand_fn *fn = NULL;\n \tstruct option options[] = {\n-\t\tOPT_SUBCOMMAND(\"create\", &fn, cmd_bundle_create),\n-\t\tOPT_SUBCOMMAND(\"verify\", &fn, cmd_bundle_verify),\n-\t\tOPT_SUBCOMMAND(\"list-heads\", &fn, cmd_bundle_list_heads),\n-\t\tOPT_SUBCOMMAND(\"unbundle\", &fn, cmd_bundle_unbundle),\n \t\tOPT_END()\n \t};\n+\tint result;\n \n \targc = parse_options(argc, argv, prefix, options, builtin_bundle_usage,\n-\t\t\t     0);\n+\t\tPARSE_OPT_STOP_AT_NON_OPTION);\n \n \tpacket_trace_identity(\"bundle\");\n \n-\treturn !!fn(argc, argv, prefix);\n+\tif (argc < 2)\n+\t\tusage_with_options(builtin_bundle_usage, options);\n+\n+\telse if (!strcmp(argv[0], \"create\"))\n+\t\tresult = cmd_bundle_create(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"verify\"))\n+\t\tresult = cmd_bundle_verify(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"list-heads\"))\n+\t\tresult = cmd_bundle_list_heads(argc, argv, prefix);\n+\telse if (!strcmp(argv[0], \"unbundle\"))\n+\t\tresult = cmd_bundle_unbundle(argc, argv, prefix);\n+\telse {\n+\t\terror(_(\"Unknown subcommand: %s\"), argv[0]);\n+\t\tusage_with_options(builtin_bundle_usage, options);\n+\t}\n+\treturn result ? 1 : 0;\n }\n-- \n2.38.1\n\n"},{"id":"469374","messageId":"0609575c-68ad-5392-0631-3563c179f177@github.com","threadId":"58985","inReplyTo":"20221220123142.812965-1-hubertj@stmcyber.pl","subject":"Re: [PATCH] Revert \"builtin/bundle.c: let parse-options parse subcommands\"","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-20T13:30:11Z","receivedAt":"2022-12-20T13:30:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/20/22 7:31 AM, Hubert Jasudowicz wrote:\n> This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.\n> \n> The change breaks git bundle command. Running any subcommand\n> results with:\n> \n> $ git bundle create\n> Segmentation fault (core dumped)\n\nCould you be more specific? We have tests that verify that\nthese commands work without a segfault. There must be something\ndifferent about your environment that makes the segfault occur.\n\nOne thing that I could believe is that you are running 'git\nbundle create' outside of a Git repository, and I doubt we have\ntests covering that scenario.\n\nBefore reverting this change that has been out in two releases,\nI recommend adding a test that demonstrates your failure and\nthen doing a specific update to fix that scenario.\n\nThanks,\n-Stolee\n\n"},{"id":"469376","messageId":"patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com","threadId":"58985","inReplyTo":"20221220123142.812965-1-hubertj@stmcyber.pl","subject":"[PATCH] bundle: don't segfault on \"git bundle <subcmd>\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T13:40:18Z","receivedAt":"2022-12-20T13:40:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since aef7d75e580 (builtin/bundle.c: let parse-options parse\nsubcommands, 2022-08-19) we've been segfaulting if no argument was\nprovided.\n\nThe fix is easy, as all of the \"git bundle\" subcommands require a\nnon-option argument we can check that we have arguments left after\ncalling parse-options().\n\nThis makes use of code added in 73c3253d75e (bundle: framework for\noptions before bundle file, 2019-11-10), before this change that code\nhas always been unreachable. In 73c3253d75e we'd never reach it as we\nalready checked \"argc < 2\" in cmd_bundle() itself.\n\nThen when aef7d75e580 (whose segfault we're fixing here) migrated this\ncode to the subcommand API it removed that \"argc < 2\" check, but we\nwere still checking the wrong \"argc\" in parse_options_cmd_bundle(), we\nneed to check the \"newargc\". The \"argc\" will always be >= 1, as it\nwill necessarily contain at least the subcommand name\nitself (e.g. \"create\").\n\nAs an aside, this could be safely squashed into this, but let's not do\nthat for this minimal segfault fix, as it's an unrelated refactoring:\n\n\t--- a/builtin/bundle.c\n\t+++ b/builtin/bundle.c\n\t@@ -55,13 +55,12 @@ static int parse_options_cmd_bundle(int argc,\n\t \t\tconst char * const usagestr[],\n\t \t\tconst struct option options[],\n\t \t\tchar **bundle_file) {\n\t-\tint newargc;\n\t-\tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n\t+\targc = parse_options(argc, argv, NULL, options, usagestr,\n\t \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n\t-\tif (!newargc)\n\t+\tif (!argc)\n\t \t\tusage_with_options(usagestr, options);\n\t \t*bundle_file = prefix_filename(prefix, argv[0]);\n\t-\treturn newargc;\n\t+\treturn argc;\n\t }\n\n\t static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n\nReported-by: Hubert Jasudowicz <hubertj@stmcyber.pl>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nA proposed replacement for\nhttps://lore.kernel.org/git/20221220123142.812965-1-hubertj@stmcyber.pl/;\nlet's move forward & add a test rather than reverting away from the\nsubcommand APIe\n\n builtin/bundle.c       | 2 +-\n t/t6020-bundle-misc.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex c12c09f8549..61c76284768 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -58,7 +58,7 @@ static int parse_options_cmd_bundle(int argc,\n \tint newargc;\n \tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n-\tif (argc < 1)\n+\tif (!newargc)\n \t\tusage_with_options(usagestr, options);\n \t*bundle_file = prefix_filename(prefix, argv[0]);\n \treturn newargc;\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 833205125ab..3a1cf30b1d7 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bundle.sh\n \n+for cmd in create verify list-heads unbundle\n+do\n+\ttest_expect_success \"usage: git bundle $cmd needs an argument\" '\n+\t\ttest_expect_code 129 git bundle $cmd\n+\t'\n+done\n+\n # Create a commit or tag and set the variable with the object ID.\n test_commit_setvar () {\n \tnotick=\n-- \n2.39.0.1106.g08bce9674be\n\n"},{"id":"469377","messageId":"221220.861qouxk79.gmgdl@evledraar.gmail.com","threadId":"58985","inReplyTo":"0609575c-68ad-5392-0631-3563c179f177@github.com","subject":"Re: [PATCH] Revert \"builtin/bundle.c: let parse-options parse subcommands\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T13:42:46Z","receivedAt":"2022-12-20T13:45:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 20 2022, Derrick Stolee wrote:\n\n> On 12/20/22 7:31 AM, Hubert Jasudowicz wrote:\n>> This reverts commit aef7d75e5809eda765bbe407c7f8e0f8617f0fd0.\n>> \n>> The change breaks git bundle command. Running any subcommand\n>> results with:\n>> \n>> $ git bundle create\n>> Segmentation fault (core dumped)\n>\n> Could you be more specific?\n\nI don't think the report can get more specific than (quoting it):\n\n\t$ git bundle create\n\tSegmentation fault (core dumped)\n\n:) Did you try running it?\n\n> We have tests that verify that\n> these commands work without a segfault. There must be something\n> different about your environment that makes the segfault occur.\n\nWe don't have those tests, I submitted an alternate smaller fix in\nhttps://lore.kernel.org/git/patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com/\nthat adds some.\n\nI think what you're misrecalling here is probably that we have general\ntests for running \"git <cmd> -h\" for all built-in <cmd>, but we don't\nhave any such tests for running sub-commands.\n\nAnd even then, that wouldn't catch this, as it's a bespoke segfault in\nthe bundle code, as it can't handle not getting at least one non-option\nargument.\n"},{"id":"469378","messageId":"12f06dc2-8bfe-8cbe-c6b8-659f5bf92ea4@stmcyber.pl","threadId":"58985","inReplyTo":"patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com","subject":"Re: [PATCH] bundle: don't segfault on \"git bundle <subcmd>\"","fromName":"Hubert Jasudowicz","fromEmail":"hubert.jasudowicz@stmcyber.pl","sentAt":"2022-12-20T13:50:35Z","receivedAt":"2022-12-20T13:50:46Z","isPatch":true,"sender":{"key":"hubert.jasudowicz@stmcyber.pl","avatar":null},"body":"On 12/20/22 14:40, Ævar Arnfjörð Bjarmason wrote:> A proposed replacement for\n> https://lore.kernel.org/git/20221220123142.812965-1-hubertj@stmcyber.pl/;\n> let's move forward & add a test rather than reverting away from the\n> subcommand APIe\n> \n>  builtin/bundle.c       | 2 +-\n>  t/t6020-bundle-misc.sh | 7 +++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/bundle.c b/builtin/bundle.c\n> index c12c09f8549..61c76284768 100644\n> --- a/builtin/bundle.c\n> +++ b/builtin/bundle.c\n> @@ -58,7 +58,7 @@ static int parse_options_cmd_bundle(int argc,\n>  \tint newargc;\n>  \tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n>  \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n> -\tif (argc < 1)\n> +\tif (!newargc)\n>  \t\tusage_with_options(usagestr, options);\n>  \t*bundle_file = prefix_filename(prefix, argv[0]);\n>  \treturn newargc;\n> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\n> index 833205125ab..3a1cf30b1d7 100755\n> --- a/t/t6020-bundle-misc.sh\n> +++ b/t/t6020-bundle-misc.sh\n> @@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>  . ./test-lib.sh\n>  . \"$TEST_DIRECTORY\"/lib-bundle.sh\n>  \n> +for cmd in create verify list-heads unbundle\n> +do\n> +\ttest_expect_success \"usage: git bundle $cmd needs an argument\" '\n> +\t\ttest_expect_code 129 git bundle $cmd\n> +\t'\n> +done\n> +\n>  # Create a commit or tag and set the variable with the object ID.\n>  test_commit_setvar () {\n>  \tnotick=\n\nSeems to work. Thanks!\n\nTested-by: Hubert Jasudowicz <hubertj@stmcyber.pl>\n\nHubert Jasudowicz\n"},{"id":"469518","messageId":"xmqq5ydzpwtk.fsf@gitster.g","threadId":"58985","inReplyTo":"patch-1.1-2319eb2ddbd-20221220T133941Z-avarab@gmail.com","subject":"Re: [PATCH] bundle: don't segfault on \"git bundle <subcmd>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-25T11:05:59Z","receivedAt":"2022-12-25T11:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> As an aside, this could be safely squashed into this, but let's not do\n> that for this minimal segfault fix, as it's an unrelated refactoring:\n>\n> \t--- a/builtin/bundle.c\n> \t+++ b/builtin/bundle.c\n> \t@@ -55,13 +55,12 @@ static int parse_options_cmd_bundle(int argc,\n> \t \t\tconst char * const usagestr[],\n> \t \t\tconst struct option options[],\n> \t \t\tchar **bundle_file) {\n> \t-\tint newargc;\n> \t-\tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n> \t+\targc = parse_options(argc, argv, NULL, options, usagestr,\n> \t \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n> \t-\tif (!newargc)\n> \t+\tif (!argc)\n> \t \t\tusage_with_options(usagestr, options);\n> \t \t*bundle_file = prefix_filename(prefix, argv[0]);\n> \t-\treturn newargc;\n> \t+\treturn argc;\n> \t }\n\nThat would actually make the intent much clearer and if the code\nwere written to update argc instead of introducing a separate\nvarilable, this bug would not have happened.\n\nThanks, will queue (without the clean-up at least for now).\n"},{"id":"469519","messageId":"xmqqzgbboi92.fsf@gitster.g","threadId":"58985","inReplyTo":"12f06dc2-8bfe-8cbe-c6b8-659f5bf92ea4@stmcyber.pl","subject":"Re: [PATCH] bundle: don't segfault on \"git bundle <subcmd>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-25T11:06:01Z","receivedAt":"2022-12-25T11:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hubert Jasudowicz <hubert.jasudowicz@stmcyber.pl> writes:\n\n> Seems to work. Thanks!\n>\n> Tested-by: Hubert Jasudowicz <hubertj@stmcyber.pl>\n>\n> Hubert Jasudowicz\n\nThanks for reporting and testing.\n\n"},{"id":"469559","messageId":"patch-1.2-357cea183d6-20221227T183708Z-avarab@gmail.com","threadId":"58985","inReplyTo":"cover-0.2-00000000000-20221227T183708Z-avarab@gmail.com","subject":"[PATCH 1/2] builtin/bundle.c: remove superfluous \"newargc\" variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-27T18:39:09Z","receivedAt":"2022-12-27T18:40:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in 891cb09db6c (bundle: don't segfault on \"git bundle\n<subcmd>\", 2022-12-20) the \"newargc\" in this function is redundant to\nusing our own \"argc\". Let's refactor the function to avoid needlessly\nintroducing another variable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/bundle.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 61c76284768..3d1ad220ff8 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -55,13 +55,12 @@ static int parse_options_cmd_bundle(int argc,\n \t\tconst char * const usagestr[],\n \t\tconst struct option options[],\n \t\tchar **bundle_file) {\n-\tint newargc;\n-\tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n+\targc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n-\tif (!newargc)\n+\tif (!argc)\n \t\tusage_with_options(usagestr, options);\n \t*bundle_file = prefix_filename(prefix, argv[0]);\n-\treturn newargc;\n+\treturn argc;\n }\n \n static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n-- \n2.39.0.1106.gf45ba805d1a\n\n"},{"id":"469560","messageId":"cover-0.2-00000000000-20221227T183708Z-avarab@gmail.com","threadId":"58985","inReplyTo":"xmqq5ydzpwtk.fsf@gitster.g","subject":"[PATCH 0/2] builtin/bundle.c: segfault fix style & error reporting follow-up","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-27T18:39:08Z","receivedAt":"2022-12-27T18:40:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Dec 25 2022, Junio C Hamano wrote:\n\n> That would actually make the intent much clearer and if the code\n> were written to update argc instead of introducing a separate\n> varilable, this bug would not have happened.\n\nHere's a couple of general improvements on top of ab/bundle-wo-args,\nwe should also report to the user that they were missing the <file>\nargument, as 2/2 here fixes.\n\nÆvar Arnfjörð Bjarmason (2):\n  builtin/bundle.c: remove superfluous \"newargc\" variable\n  bundle <cmd>: have usage_msg_opt() note the missing \"<file>\"\n\n builtin/bundle.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\n-- \n2.39.0.1106.gf45ba805d1a\n\n"},{"id":"469561","messageId":"patch-2.2-00f74250fd0-20221227T183708Z-avarab@gmail.com","threadId":"58985","inReplyTo":"cover-0.2-00000000000-20221227T183708Z-avarab@gmail.com","subject":"[PATCH 2/2] bundle <cmd>: have usage_msg_opt() note the missing \"<file>\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-27T18:39:10Z","receivedAt":"2022-12-27T18:40:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Improve the usage we emit on e.g. \"git bundle create\" to note why\nwe're showing the usage, it's because the \"<file>\" argument is\nmissing.\n\nWe know that'll be the case for all parse_options_cmd_bundle() users,\nas they're passing the \"char **bundle_file\" parameter, which as the\ncontext shows we're expected to populate.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/bundle.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 3d1ad220ff8..acceef62001 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -58,7 +58,7 @@ static int parse_options_cmd_bundle(int argc,\n \targc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \tif (!argc)\n-\t\tusage_with_options(usagestr, options);\n+\t\tusage_msg_opt(_(\"need a <file> argument\"), usagestr, options);\n \t*bundle_file = prefix_filename(prefix, argv[0]);\n \treturn argc;\n }\n-- \n2.39.0.1106.gf45ba805d1a\n\n"},{"id":"469572","messageId":"xmqq7cycqv73.fsf@gitster.g","threadId":"58985","inReplyTo":"cover-0.2-00000000000-20221227T183708Z-avarab@gmail.com","subject":"Re: [PATCH 0/2] builtin/bundle.c: segfault fix style & error reporting follow-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-27T23:32:32Z","receivedAt":"2022-12-27T23:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Here's a couple of general improvements on top of ab/bundle-wo-args,\n> we should also report to the user that they were missing the <file>\n> argument, as 2/2 here fixes.\n>\n> Ævar Arnfjörð Bjarmason (2):\n>   builtin/bundle.c: remove superfluous \"newargc\" variable\n>   bundle <cmd>: have usage_msg_opt() note the missing \"<file>\"\n>\n>  builtin/bundle.c | 9 ++++-----\n>  1 file changed, 4 insertions(+), 5 deletions(-)\n\nI have no strong opinions on the second one, but the first one is a\ndefinite improvement.  Will queue both.  Thanks. \n"}]}