{"thread":{"id":"55980","subject":"[PATCH 0/2] Some submodule related code cleanup","startedAt":"2021-06-21T19:08:58Z","lastAt":"2021-06-22T18:15:15Z","messageCount":7,"participants":["Kaartic Sivaraam","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"428090","messageId":"20210621190837.9487-1-kaartic.sivaraam@gmail.com","threadId":"55980","inReplyTo":null,"subject":"[PATCH 0/2] Some submodule related code cleanup","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-06-21T19:08:35Z","receivedAt":"2021-06-21T19:08:58Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"When taking a look at various changes related to the submodule\nbuilting conversion effort[1], I noticed a couple of minor changes\nthat are independent of the builtin conversion effort.\n\nSo, I'm sending this series with those suggested changes.\n\n[1]: https://public-inbox.org/git/D32894F5-FC76-4DD2-A2F6-E69AAE88C645@gmail.com/\n\n--\nSivaraam\n\n\nKaartic Sivaraam (2):\n submodule--helper: remove an unreachable call to usage_with_options\n submodule: remove unnecessary `prefix` based option logic\n\n builtin/submodule--helper.c | 2 --\n git-submodule.sh | 14 +++++++-------\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\n-- \n2.32.0.9.g81a5432dce.dirty\n\n"},{"id":"428091","messageId":"20210621190837.9487-2-kaartic.sivaraam@gmail.com","threadId":"55980","inReplyTo":"20210621190837.9487-1-kaartic.sivaraam@gmail.com","subject":"[PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-06-21T19:08:36Z","receivedAt":"2021-06-21T19:09:01Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"The code path in question calls `error` in a particular case.\nBut, `error` never returns as it exits directly. This makes\nthe call to `usage_with_options` that follows the `error` call\nunreachable.\n\nSo, remove the unreachable `usage_with_options` call.\n\nSigned-off-by: Kaartic Sivaraam \n---\n builtin/submodule--helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ae6174ab05..c9aa838083 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1637,8 +1637,6 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n \n \tif (all && argc) {\n \t\terror(\"pathspec and --all are incompatible\");\n-\t\tusage_with_options(git_submodule_helper_usage,\n-\t\t\t\t module_deinit_options);\n \t}\n \n \tif (!argc && !all)\n-- \n2.32.0.9.g81a5432dce.dirty\n\n"},{"id":"428092","messageId":"20210621190837.9487-3-kaartic.sivaraam@gmail.com","threadId":"55980","inReplyTo":"20210621190837.9487-1-kaartic.sivaraam@gmail.com","subject":"[PATCH 2/2] submodule: remove unnecessary `prefix` based option logic","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-06-21T19:08:37Z","receivedAt":"2021-06-21T19:09:02Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Over time when parts of submodule have been ported from shell to\nbuiltin, many instances of the submodule helper have been added.\nAlso added with them are some unnecessary option passing\nlogic that are based on the `prefix` shell variable which never\ngets set in their code flows.\n\nOn analysis, the only shell functions which have a valid usage\nfor the `prefix` shell variable are:\n\n - cmd_update: which is the only function which sets the variable\n and thus uses it properly\n\n - cmd_init: which uses the variable via a call from cmd_update\n\nSo, remove the unnecessary option parsing logic based on the `prefix`\nshell variable.\n\nSigned-off-by: Kaartic Sivaraam \n---\n git-submodule.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..cb06aa02c8 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -335,7 +335,7 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n #\n@@ -402,7 +402,7 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n is_tip_reachable () (\n@@ -726,7 +726,7 @@ cmd_set_branch() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n }\n \n #\n@@ -755,7 +755,7 @@ cmd_set_url() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n }\n \n #\n@@ -807,7 +807,7 @@ cmd_summary() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${prefix:+--prefix \"$prefix\"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n }\n #\n # List all submodules, prefixed with:\n@@ -848,7 +848,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n }\n #\n # Sync remote urls for submodules\n@@ -881,7 +881,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n cmd_absorbgitdirs()\n-- \n2.32.0.9.g81a5432dce.dirty\n\n"},{"id":"428096","messageId":"CAPig+cT66BT7fCfHBJM25D1SBVKAwRpSh+SAaR8YmqZX+7epvA@mail.gmail.com","threadId":"55980","inReplyTo":"20210621190837.9487-2-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-06-21T19:58:00Z","receivedAt":"2021-06-21T19:58:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 21, 2021 at 3:09 PM Kaartic Sivaraam\n wrote:\n> The code path in question calls `error` in a particular case.\n> But, `error` never returns as it exits directly. This makes\n> the call to `usage_with_options` that follows the `error` call\n> unreachable.\n\nerror() returns -1; you will commonly see:\n\n if (check_something())\n return error(...);\n\n> So, remove the unreachable `usage_with_options` call.\n>\n> Signed-off-by: Kaartic Sivaraam \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -1637,8 +1637,6 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n> if (all && argc) {\n> error(\"pathspec and --all are incompatible\");\n> - usage_with_options(git_submodule_helper_usage,\n> - module_deinit_options);\n> }\n\nusage_with_options(), on the other hand, exits directly.\n"},{"id":"428220","messageId":"7c1522ab-26b8-66b2-46e2-7b974c763b3b@gmail.com","threadId":"55980","inReplyTo":"CAPig+cT66BT7fCfHBJM25D1SBVKAwRpSh+SAaR8YmqZX+7epvA@mail.gmail.com","subject":"Re: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-06-22T18:02:02Z","receivedAt":"2021-06-22T18:08:04Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 22/06/21 1:28 am, Eric Sunshine wrote:\n> On Mon, Jun 21, 2021 at 3:09 PM Kaartic Sivaraam\n> wrote:\n>> The code path in question calls `error` in a particular case.\n>> But, `error` never returns as it exits directly. This makes\n>> the call to `usage_with_options` that follows the `error` call\n>> unreachable.\n> \n> error() returns -1; you will commonly see:\n> \n> if (check_something())\n> return error(...);\n>\n\nYou're right. I guess I was drowsy when I was looking at this\npart for the code. The passing tests didn't help either.\n\n>> So, remove the unreachable `usage_with_options` call.\n>>\n>> Signed-off-by: Kaartic Sivaraam \n>> ---\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> @@ -1637,8 +1637,6 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n>> if (all && argc) {\n>> error(\"pathspec and --all are incompatible\");\n>> - usage_with_options(git_submodule_helper_usage,\n>> - module_deinit_options);\n>> }\n> \n> usage_with_options(), on the other hand, exits directly.\n> \n\nGot it. Will drop this patch and re-roll.\n\nThanks,\nSivaraam\n"},{"id":"428222","messageId":"20210622181452.2974-1-kaartic.sivaraam@gmail.com","threadId":"55980","inReplyTo":"20210621190837.9487-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v2 0/1] Some submodule related code cleanup","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-06-22T18:14:51Z","receivedAt":"2021-06-22T18:15:10Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"This is v2 of the series on submodule related code cleanup.\n\nChanges since v1:\n\nBased on review feedback from Eric, I dropped the first patch as it\nwas an incorrect change.\n\nThe second patch is included as-is.\n\nThanks,\nSivaraam\n\n\nKaartic Sivaraam (1):\n submodule: remove unnecessary `prefix` based option logic\n\n git-submodule.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\n-- \n2.32.0.9.g81a5432dce.dirty\n\n"},{"id":"428223","messageId":"20210622181452.2974-2-kaartic.sivaraam@gmail.com","threadId":"55980","inReplyTo":"20210622181452.2974-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v2 1/1] submodule: remove unnecessary `prefix` based option logic","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-06-22T18:14:52Z","receivedAt":"2021-06-22T18:15:15Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Over time when parts of submodule have been ported from shell to\nbuiltin, many instances of the submodule helper have been added.\nAlso added with them are some unnecessary option passing\nlogic that are based on the `prefix` shell variable which never\ngets set in their code flows.\n\nOn analysis, the only shell functions which have a valid usage\nfor the `prefix` shell variable are:\n\n - cmd_update: which is the only function which sets the variable\n and thus uses it properly\n\n - cmd_init: which uses the variable via a call from cmd_update\n\nSo, remove the unnecessary option parsing logic based on the `prefix`\nshell variable.\n\nSigned-off-by: Kaartic Sivaraam \n---\n git-submodule.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..cb06aa02c8 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -335,7 +335,7 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n #\n@@ -402,7 +402,7 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n is_tip_reachable () (\n@@ -726,7 +726,7 @@ cmd_set_branch() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n }\n \n #\n@@ -755,7 +755,7 @@ cmd_set_url() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n }\n \n #\n@@ -807,7 +807,7 @@ cmd_summary() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${prefix:+--prefix \"$prefix\"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n }\n #\n # List all submodules, prefixed with:\n@@ -848,7 +848,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n }\n #\n # Sync remote urls for submodules\n@@ -881,7 +881,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n cmd_absorbgitdirs()\n-- \n2.32.0.9.g81a5432dce.dirty\n\n"}]}