# [PATCH 0/2] Some submodule related code cleanup 7 messages from 2021-06-21 to 2021-06-22. Participants: Kaartic Sivaraam, Eric Sunshine. Thread: https://gitlist.dev/t/55980 ## Kaartic Sivaraam, 2021-06-21 19:08 Subject: [PATCH 0/2] Some submodule related code cleanup Message-ID: <20210621190837.9487-1-kaartic.sivaraam@gmail.com> URL: https://gitlist.dev/e/20210621190837.9487-1-kaartic.sivaraam%40gmail.com ``` When taking a look at various changes related to the submodule builting conversion effort[1], I noticed a couple of minor changes that are independent of the builtin conversion effort. So, I'm sending this series with those suggested changes. [1]: https://public-inbox.org/git/D32894F5-FC76-4DD2-A2F6-E69AAE88C645@gmail.com/ -- Sivaraam Kaartic Sivaraam (2): submodule--helper: remove an unreachable call to usage_with_options submodule: remove unnecessary `prefix` based option logic builtin/submodule--helper.c | 2 -- git-submodule.sh | 14 +++++++------- 2 files changed, 7 insertions(+), 9 deletions(-) -- 2.32.0.9.g81a5432dce.dirty ``` ## Kaartic Sivaraam, 2021-06-21 19:08 Subject: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Message-ID: <20210621190837.9487-2-kaartic.sivaraam@gmail.com> URL: https://gitlist.dev/e/20210621190837.9487-2-kaartic.sivaraam%40gmail.com In-Reply-To: <20210621190837.9487-1-kaartic.sivaraam@gmail.com> ``` The code path in question calls `error` in a particular case. But, `error` never returns as it exits directly. This makes the call to `usage_with_options` that follows the `error` call unreachable. So, remove the unreachable `usage_with_options` call. Signed-off-by: Kaartic Sivaraam --- builtin/submodule--helper.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index ae6174ab05..c9aa838083 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1637,8 +1637,6 @@ static int module_deinit(int argc, const char **argv, const char *prefix) if (all && argc) { error("pathspec and --all are incompatible"); - usage_with_options(git_submodule_helper_usage, - module_deinit_options); } if (!argc && !all) -- 2.32.0.9.g81a5432dce.dirty ``` ## Kaartic Sivaraam, 2021-06-21 19:08 Subject: [PATCH 2/2] submodule: remove unnecessary `prefix` based option logic Message-ID: <20210621190837.9487-3-kaartic.sivaraam@gmail.com> URL: https://gitlist.dev/e/20210621190837.9487-3-kaartic.sivaraam%40gmail.com In-Reply-To: <20210621190837.9487-1-kaartic.sivaraam@gmail.com> ``` Over time when parts of submodule have been ported from shell to builtin, many instances of the submodule helper have been added. Also added with them are some unnecessary option passing logic that are based on the `prefix` shell variable which never gets set in their code flows. On analysis, the only shell functions which have a valid usage for the `prefix` shell variable are: - cmd_update: which is the only function which sets the variable and thus uses it properly - cmd_init: which uses the variable via a call from cmd_update So, remove the unnecessary option parsing logic based on the `prefix` shell variable. Signed-off-by: Kaartic Sivaraam --- git-submodule.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 4678378424..cb06aa02c8 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -335,7 +335,7 @@ cmd_foreach() shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" } # @@ -402,7 +402,7 @@ cmd_deinit() shift done - git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@" } is_tip_reachable () ( @@ -726,7 +726,7 @@ cmd_set_branch() { shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@" } # @@ -755,7 +755,7 @@ cmd_set_url() { shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@" } # @@ -807,7 +807,7 @@ cmd_summary() { shift done - git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary ${prefix:+--prefix "$prefix"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- "$@" } # # List all submodules, prefixed with: @@ -848,7 +848,7 @@ cmd_status() shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@" } # # Sync remote urls for submodules @@ -881,7 +881,7 @@ cmd_sync() esac done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" } cmd_absorbgitdirs() -- 2.32.0.9.g81a5432dce.dirty ``` ## Eric Sunshine, 2021-06-21 19:58 Subject: Re: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT66BT7fCfHBJM25D1SBVKAwRpSh%2BSAaR8YmqZX%2B7epvA%40mail.gmail.com In-Reply-To: <20210621190837.9487-2-kaartic.sivaraam@gmail.com> ``` On Mon, Jun 21, 2021 at 3:09 PM Kaartic Sivaraam wrote: > The code path in question calls `error` in a particular case. > But, `error` never returns as it exits directly. This makes > the call to `usage_with_options` that follows the `error` call > unreachable. error() returns -1; you will commonly see: if (check_something()) return error(...); > So, remove the unreachable `usage_with_options` call. > > Signed-off-by: Kaartic Sivaraam > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > @@ -1637,8 +1637,6 @@ static int module_deinit(int argc, const char **argv, const char *prefix) > if (all && argc) { > error("pathspec and --all are incompatible"); > - usage_with_options(git_submodule_helper_usage, > - module_deinit_options); > } usage_with_options(), on the other hand, exits directly. ``` ## Kaartic Sivaraam, 2021-06-22 18:02 Subject: Re: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Message-ID: <7c1522ab-26b8-66b2-46e2-7b974c763b3b@gmail.com> URL: https://gitlist.dev/e/7c1522ab-26b8-66b2-46e2-7b974c763b3b%40gmail.com In-Reply-To: ``` On 22/06/21 1:28 am, Eric Sunshine wrote: > On Mon, Jun 21, 2021 at 3:09 PM Kaartic Sivaraam > wrote: >> The code path in question calls `error` in a particular case. >> But, `error` never returns as it exits directly. This makes >> the call to `usage_with_options` that follows the `error` call >> unreachable. > > error() returns -1; you will commonly see: > > if (check_something()) > return error(...); > You're right. I guess I was drowsy when I was looking at this part for the code. The passing tests didn't help either. >> So, remove the unreachable `usage_with_options` call. >> >> Signed-off-by: Kaartic Sivaraam >> --- >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> @@ -1637,8 +1637,6 @@ static int module_deinit(int argc, const char **argv, const char *prefix) >> if (all && argc) { >> error("pathspec and --all are incompatible"); >> - usage_with_options(git_submodule_helper_usage, >> - module_deinit_options); >> } > > usage_with_options(), on the other hand, exits directly. > Got it. Will drop this patch and re-roll. Thanks, Sivaraam ``` ## Kaartic Sivaraam, 2021-06-22 18:14 Subject: [PATCH v2 0/1] Some submodule related code cleanup Message-ID: <20210622181452.2974-1-kaartic.sivaraam@gmail.com> URL: https://gitlist.dev/e/20210622181452.2974-1-kaartic.sivaraam%40gmail.com In-Reply-To: <20210621190837.9487-1-kaartic.sivaraam@gmail.com> ``` This is v2 of the series on submodule related code cleanup. Changes since v1: Based on review feedback from Eric, I dropped the first patch as it was an incorrect change. The second patch is included as-is. Thanks, Sivaraam Kaartic Sivaraam (1): submodule: remove unnecessary `prefix` based option logic git-submodule.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) -- 2.32.0.9.g81a5432dce.dirty ``` ## Kaartic Sivaraam, 2021-06-22 18:14 Subject: [PATCH v2 1/1] submodule: remove unnecessary `prefix` based option logic Message-ID: <20210622181452.2974-2-kaartic.sivaraam@gmail.com> URL: https://gitlist.dev/e/20210622181452.2974-2-kaartic.sivaraam%40gmail.com In-Reply-To: <20210622181452.2974-1-kaartic.sivaraam@gmail.com> ``` Over time when parts of submodule have been ported from shell to builtin, many instances of the submodule helper have been added. Also added with them are some unnecessary option passing logic that are based on the `prefix` shell variable which never gets set in their code flows. On analysis, the only shell functions which have a valid usage for the `prefix` shell variable are: - cmd_update: which is the only function which sets the variable and thus uses it properly - cmd_init: which uses the variable via a call from cmd_update So, remove the unnecessary option parsing logic based on the `prefix` shell variable. Signed-off-by: Kaartic Sivaraam --- git-submodule.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 4678378424..cb06aa02c8 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -335,7 +335,7 @@ cmd_foreach() shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" } # @@ -402,7 +402,7 @@ cmd_deinit() shift done - git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@" } is_tip_reachable () ( @@ -726,7 +726,7 @@ cmd_set_branch() { shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@" } # @@ -755,7 +755,7 @@ cmd_set_url() { shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@" } # @@ -807,7 +807,7 @@ cmd_summary() { shift done - git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary ${prefix:+--prefix "$prefix"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- "$@" } # # List all submodules, prefixed with: @@ -848,7 +848,7 @@ cmd_status() shift done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@" } # # Sync remote urls for submodules @@ -881,7 +881,7 @@ cmd_sync() esac done - git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@" } cmd_absorbgitdirs() -- 2.32.0.9.g81a5432dce.dirty ```