* [PATCH 0/2] Some submodule related code cleanup
@ 2021-06-21 19:08 Kaartic Sivaraam
2021-06-21 19:08 ` [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Kaartic Sivaraam
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Kaartic Sivaraam @ 2021-06-21 19:08 UTC (permalink / raw)
To: Git List; +Cc: Atharva Raykar, Shourya Shukla
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options
2021-06-21 19:08 [PATCH 0/2] Some submodule related code cleanup Kaartic Sivaraam
@ 2021-06-21 19:08 ` Kaartic Sivaraam
2021-06-21 19:58 ` Eric Sunshine
2021-06-21 19:08 ` [PATCH 2/2] submodule: remove unnecessary `prefix` based option logic Kaartic Sivaraam
2021-06-22 18:14 ` [PATCH v2 0/1] Some submodule related code cleanup Kaartic Sivaraam
2 siblings, 1 reply; 7+ messages in thread
From: Kaartic Sivaraam @ 2021-06-21 19:08 UTC (permalink / raw)
To: Git List; +Cc: Atharva Raykar, Shourya Shukla
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 <kaartic.sivaraam@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] submodule: remove unnecessary `prefix` based option logic
2021-06-21 19:08 [PATCH 0/2] Some submodule related code cleanup Kaartic Sivaraam
2021-06-21 19:08 ` [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Kaartic Sivaraam
@ 2021-06-21 19:08 ` Kaartic Sivaraam
2021-06-22 18:14 ` [PATCH v2 0/1] Some submodule related code cleanup Kaartic Sivaraam
2 siblings, 0 replies; 7+ messages in thread
From: Kaartic Sivaraam @ 2021-06-21 19:08 UTC (permalink / raw)
To: Git List; +Cc: Atharva Raykar, Shourya Shukla
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 <kaartic.sivaraam@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options
2021-06-21 19:08 ` [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Kaartic Sivaraam
@ 2021-06-21 19:58 ` Eric Sunshine
2021-06-22 18:02 ` Kaartic Sivaraam
0 siblings, 1 reply; 7+ messages in thread
From: Eric Sunshine @ 2021-06-21 19:58 UTC (permalink / raw)
To: Kaartic Sivaraam; +Cc: Git List, Atharva Raykar, Shourya Shukla
On Mon, Jun 21, 2021 at 3:09 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> 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 <kaartic.sivaraam@gmail.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options
2021-06-21 19:58 ` Eric Sunshine
@ 2021-06-22 18:02 ` Kaartic Sivaraam
0 siblings, 0 replies; 7+ messages in thread
From: Kaartic Sivaraam @ 2021-06-22 18:02 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Atharva Raykar, Shourya Shukla
On 22/06/21 1:28 am, Eric Sunshine wrote:
> On Mon, Jun 21, 2021 at 3:09 PM Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> 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 <kaartic.sivaraam@gmail.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 0/1] Some submodule related code cleanup
2021-06-21 19:08 [PATCH 0/2] Some submodule related code cleanup Kaartic Sivaraam
2021-06-21 19:08 ` [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Kaartic Sivaraam
2021-06-21 19:08 ` [PATCH 2/2] submodule: remove unnecessary `prefix` based option logic Kaartic Sivaraam
@ 2021-06-22 18:14 ` Kaartic Sivaraam
2021-06-22 18:14 ` [PATCH v2 1/1] submodule: remove unnecessary `prefix` based option logic Kaartic Sivaraam
2 siblings, 1 reply; 7+ messages in thread
From: Kaartic Sivaraam @ 2021-06-22 18:14 UTC (permalink / raw)
To: Git Mailing List; +Cc: Eric Sunshine, Atharva Raykar, Emily Shaffer
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] submodule: remove unnecessary `prefix` based option logic
2021-06-22 18:14 ` [PATCH v2 0/1] Some submodule related code cleanup Kaartic Sivaraam
@ 2021-06-22 18:14 ` Kaartic Sivaraam
0 siblings, 0 replies; 7+ messages in thread
From: Kaartic Sivaraam @ 2021-06-22 18:14 UTC (permalink / raw)
To: Git Mailing List; +Cc: Eric Sunshine, Atharva Raykar, Emily Shaffer
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 <kaartic.sivaraam@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-22 18:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 19:08 [PATCH 0/2] Some submodule related code cleanup Kaartic Sivaraam
2021-06-21 19:08 ` [PATCH 1/2] submodule--helper: remove an unreachable call to usage_with_options Kaartic Sivaraam
2021-06-21 19:58 ` Eric Sunshine
2021-06-22 18:02 ` Kaartic Sivaraam
2021-06-21 19:08 ` [PATCH 2/2] submodule: remove unnecessary `prefix` based option logic Kaartic Sivaraam
2021-06-22 18:14 ` [PATCH v2 0/1] Some submodule related code cleanup Kaartic Sivaraam
2021-06-22 18:14 ` [PATCH v2 1/1] submodule: remove unnecessary `prefix` based option logic Kaartic Sivaraam
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).