* [PATCH 0/3] git-submodule.sh: improve parsing of options
@ 2024-12-07 13:51 Roy Eldar
2024-12-07 13:51 ` [PATCH 1/3] git-submodule.sh: make some variables boolean Roy Eldar
` (3 more replies)
0 siblings, 4 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-07 13:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Roy Eldar
When we run "git submodule", the script parses the various options and
then invokes "git-submodule--helper". Unlike most builtin git commands
which parse short/long options using parse-options.c, the parsing of
arguments is completely done within git-submodule.sh; therefore, there
are some inconsistencies with the rest of the commands, in particular
the parsing of option arguments given to various options.
Improve the handling of option arguments for both long & short options;
for example, passing flags such as "--branch=master" or "-j8" now works.
Roy Eldar (3):
git-submodule.sh: make some variables boolean
git-submodule.sh: improve parsing of some long options
git-submodule.sh: improve parsing of short options
git-submodule.sh | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 1/3] git-submodule.sh: make some variables boolean
2024-12-07 13:51 [PATCH 0/3] git-submodule.sh: improve parsing of options Roy Eldar
@ 2024-12-07 13:51 ` Roy Eldar
2024-12-07 23:43 ` Junio C Hamano
2024-12-08 0:06 ` Eric Sunshine
2024-12-07 13:52 ` [PATCH 2/3] git-submodule.sh: improve parsing of some long options Roy Eldar
` (2 subsequent siblings)
3 siblings, 2 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-07 13:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Roy Eldar
When git-submodule.sh parses various options and switchs, it sets some
variables to values; in particular, every switch that is passed causes a
corresponding variable to be set to 1, which then affects the options
given to git-submodule--helper.
There are some variables are assigned "$1", although there is no reason
for it; this was actually noticed in 757d092 for the "$cached" variable.
Make some variables boolean, in order to increase consistency throught
the script and reduce possible confusion.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 03c5a220a2..107011f613 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -78,7 +78,7 @@ cmd_add()
shift
;;
-f | --force)
- force=$1
+ force=1
;;
-q|--quiet)
quiet=1
@@ -231,7 +231,7 @@ cmd_deinit()
do
case "$1" in
-f|--force)
- force=$1
+ force=1
;;
-q|--quiet)
quiet=1
@@ -294,7 +294,7 @@ cmd_update()
nofetch=1
;;
-f|--force)
- force=$1
+ force=1
;;
-r|--rebase)
rebase=1
@@ -500,10 +500,10 @@ cmd_summary() {
cached=1
;;
--files)
- files="$1"
+ files=1
;;
--for-status)
- for_status="$1"
+ for_status=1
;;
-n|--summary-limit)
summary_limit="$2"
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 2/3] git-submodule.sh: improve parsing of some long options
2024-12-07 13:51 [PATCH 0/3] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-07 13:51 ` [PATCH 1/3] git-submodule.sh: make some variables boolean Roy Eldar
@ 2024-12-07 13:52 ` Roy Eldar
2024-12-07 13:52 ` [PATCH 3/3] git-submodule.sh: improve parsing of short options Roy Eldar
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
3 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-07 13:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Roy Eldar
Some command-line options have a long form which takes an argument. In
this case, the argument can be given right after `='; for example,
"--depth" takes a numerical argument, which can be given as "--depth=X".
Support the case where the argument is given right after `=' for all
long options, in order to improve consistency throughout the script.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 107011f613..a47d2a89f3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
-f | --force)
force=1
;;
@@ -110,6 +113,9 @@ cmd_add()
custom_name=$2
shift
;;
+ --name=*)
+ custom_name="${1#--name=}"
+ ;;
--depth)
case "$2" in '') usage ;; esac
depth="--depth=$2"
@@ -425,6 +431,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
--)
shift
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 3/3] git-submodule.sh: improve parsing of short options
2024-12-07 13:51 [PATCH 0/3] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-07 13:51 ` [PATCH 1/3] git-submodule.sh: make some variables boolean Roy Eldar
2024-12-07 13:52 ` [PATCH 2/3] git-submodule.sh: improve parsing of some long options Roy Eldar
@ 2024-12-07 13:52 ` Roy Eldar
2024-12-08 0:02 ` Junio C Hamano
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
3 siblings, 1 reply; 43+ messages in thread
From: Roy Eldar @ 2024-12-07 13:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Roy Eldar
Some command-line options have a short form which takes an argument; for
example, "--jobs" has the form "-j", and it takes a numerical argument.
When parsing short options, support the case where there is no space
between the flag and the option argument, in order to improve
consistency with the rest of the builtin git commands.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index a47d2a89f3..fc85458fb1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -352,6 +355,9 @@ cmd_update()
jobs="--jobs=$2"
shift
;;
+ -j*)
+ jobs="--jobs=${1#-j}"
+ ;;
--jobs=*)
jobs=$1
;;
@@ -431,6 +437,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -519,6 +528,10 @@ cmd_summary() {
isnumber "$summary_limit" || usage
shift
;;
+ -n*)
+ summary_limit="${1#-n}"
+ isnumber "$summary_limit" || usage
+ ;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
isnumber "$summary_limit" || usage
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] git-submodule.sh: make some variables boolean
2024-12-07 13:51 ` [PATCH 1/3] git-submodule.sh: make some variables boolean Roy Eldar
@ 2024-12-07 23:43 ` Junio C Hamano
2024-12-08 0:06 ` Eric Sunshine
1 sibling, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2024-12-07 23:43 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin
Roy Eldar <royeldar0@gmail.com> writes:
> When git-submodule.sh parses various options and switchs, it sets some
> variables to values; in particular, every switch that is passed causes a
> corresponding variable to be set to 1, which then affects the options
> given to git-submodule--helper.
>
> There are some variables are assigned "$1", although there is no reason
> for it; this was actually noticed in 757d092 for the "$cached" variable.
Wearing devil's advocate hat on.
This can cut both ways. If you are doing a thin wrapper front-end,
when calling into the underlying machinery that has its own option
parser, it is often easier to write and read
submodule--helper $force $cached
instead of
submodule--helper ${force:+--force} ${cached:+--cached}
In addition, when debugging such a script by running it under "sh -x",
a typical construct like
if test "$force" = 1
then
... do the force thing ...
would appear in the "-x" trace as
+test 1 = 1
+... do the force thing ...
if you force yourself to use "1", but
if test "$force" = "--force"
then
... do the force thing ...
would show either
+test --force = --force
or
+test "" = --force
and the latter is especially useful when you are wondering why the
"--force" you gave from the command line is not taking effect.
Having said all that, as long as the use of these switch variables
are done consistently (e.g., consistently set effective ones to "1",
and consistently set ones that are off to ""), we are OK.
Thanks.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/3] git-submodule.sh: improve parsing of short options
2024-12-07 13:52 ` [PATCH 3/3] git-submodule.sh: improve parsing of short options Roy Eldar
@ 2024-12-08 0:02 ` Junio C Hamano
2024-12-09 16:21 ` Roy E
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2024-12-08 0:02 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin
Roy Eldar <royeldar0@gmail.com> writes:
> Some command-line options have a short form which takes an argument; for
> example, "--jobs" has the form "-j", and it takes a numerical argument.
>
> When parsing short options, support the case where there is no space
> between the flag and the option argument, in order to improve
> consistency with the rest of the builtin git commands.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>
> ---
> git-submodule.sh | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index a47d2a89f3..fc85458fb1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -77,6 +77,9 @@ cmd_add()
> branch=$2
> shift
> ;;
> + -b*)
> + branch="${1#-b}"
> + ;;
> --branch=*)
> branch="${1#--branch=}"
> ;;
OK, so we used to take "--branch foo" and assign 'foo' to $branch,
"--branch=foo" now does the same, and then we have "-b foo" doing
the same with this step.
> @@ -352,6 +355,9 @@ cmd_update()
> jobs="--jobs=$2"
> shift
> ;;
> + -j*)
> + jobs="--jobs=${1#-j}"
> + ;;
> --jobs=*)
> jobs=$1
> ;;
This is a bit curious. If you stick the principle in 1/3, this
should assign "4", not "--jobs=4", to variable $jobs upon seeing
"-j4", and that would be consistent with how the $branch gets its
value above.
As I said in the devil's advocate section in my review for 1/3, I
often find the value of the variable spelling out the option name
as well as the option value (i.e., force="--force", not force=1;
branch="--branch=foo", not branch=foo; jobs=--jobs=4, not jobs=4)
easier to debug and drive other programs using these variables, so I
do not mind jobs=--jobs=4 at all, but if we want to be consistent in
the other direction, this would probably want to be modified in the
name of consistency?
Other than that, all three patches looked sane to me.
Thanks.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] git-submodule.sh: make some variables boolean
2024-12-07 13:51 ` [PATCH 1/3] git-submodule.sh: make some variables boolean Roy Eldar
2024-12-07 23:43 ` Junio C Hamano
@ 2024-12-08 0:06 ` Eric Sunshine
1 sibling, 0 replies; 43+ messages in thread
From: Eric Sunshine @ 2024-12-08 0:06 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin
On Sat, Dec 7, 2024 at 8:53 AM Roy Eldar <royeldar0@gmail.com> wrote:
> When git-submodule.sh parses various options and switchs, it sets some
s/switchs/switches/
> variables to values; in particular, every switch that is passed causes a
> corresponding variable to be set to 1, which then affects the options
> given to git-submodule--helper.
>
> There are some variables are assigned "$1", although there is no reason
s/are assigned/assigned/
> for it; this was actually noticed in 757d092 for the "$cached" variable.
>
> Make some variables boolean, in order to increase consistency throught
s/throught/throughout/
> the script and reduce possible confusion.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/3] git-submodule.sh: improve parsing of short options
2024-12-08 0:02 ` Junio C Hamano
@ 2024-12-09 16:21 ` Roy E
0 siblings, 0 replies; 43+ messages in thread
From: Roy E @ 2024-12-09 16:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin
Hi,
Junio C Hamano <gitster@pobox.com> wrote:
> As I said in the devil's advocate section in my review for 1/3, I
> often find the value of the variable spelling out the option name
> as well as the option value (i.e., force="--force", not force=1;
> branch="--branch=foo", not branch=foo; jobs=--jobs=4, not jobs=4)
> easier to debug and drive other programs using these variables, so I
> do not mind jobs=--jobs=4 at all, but if we want to be consistent in
> the other direction, this would probably want to be modified in the
> name of consistency?
I find this approach better as well, and I agree this is easier to read,
and has some other advantages like better ability to debug the script.
I modified the script so that all of the variables are assigned values
which contain the option name as well as the option value, and I will
post a v2 patch series soon.
> Other than that, all three patches looked sane to me.
Thanks a lot!
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-07 13:51 [PATCH 0/3] git-submodule.sh: improve parsing of options Roy Eldar
` (2 preceding siblings ...)
2024-12-07 13:52 ` [PATCH 3/3] git-submodule.sh: improve parsing of short options Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 1/8] git-submodule.sh: make some variables boolean Roy Eldar
` (9 more replies)
3 siblings, 10 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
When we run "git submodule", the script parses the various options and
then invokes "git-submodule--helper". Unlike most builtin git commands
which parse short/long options using parse-options.c, the parsing of
arguments is completely done within git-submodule.sh; therefore, there
are some inconsistencies with the rest of the commands, in particular
the parsing of option arguments given to various options.
Improve the handling of option arguments for both long & short options;
for example, passing flags such as "--branch=master" or "-j8" now works.
Changes since v1:
- Make variable values always contain the option name.
- Rename a couple of variables in order to improve consistency.
Link to v1:
https://lore.kernel.org/git/20241207135201.2536-1-royeldar0@gmail.com
Roy Eldar (8):
git-submodule.sh: make some variables boolean
git-submodule.sh: improve parsing of some long options
git-submodule.sh: improve parsing of short options
git-submodule.sh: get rid of isnumber
git-submodule.sh: get rid of unused variable
git-submodule.sh: add some comments
git-submodule.sh: improve variables readability
git-submodule.sh: rename some variables
git-submodule.sh | 214 +++++++++++++++++++++++------------------------
1 file changed, 104 insertions(+), 110 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 1/8] git-submodule.sh: make some variables boolean
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 2/8] git-submodule.sh: improve parsing of some long options Roy Eldar
` (8 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
When git-submodule.sh parses various options and switches, it sets some
variables to values; in particular, every switch that is passed causes a
corresponding variable to be set to 1, which then affects the options
given to git-submodule--helper.
Some variables are assigned "$1", although there is no reason for it;
this was actually noticed in 757d092 for the "$cached" variable.
Make some variables boolean, in order to increase consistency throughout
the script and reduce possible confusion.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 03c5a220a2..107011f613 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -78,7 +78,7 @@ cmd_add()
shift
;;
-f | --force)
- force=$1
+ force=1
;;
-q|--quiet)
quiet=1
@@ -231,7 +231,7 @@ cmd_deinit()
do
case "$1" in
-f|--force)
- force=$1
+ force=1
;;
-q|--quiet)
quiet=1
@@ -294,7 +294,7 @@ cmd_update()
nofetch=1
;;
-f|--force)
- force=$1
+ force=1
;;
-r|--rebase)
rebase=1
@@ -500,10 +500,10 @@ cmd_summary() {
cached=1
;;
--files)
- files="$1"
+ files=1
;;
--for-status)
- for_status="$1"
+ for_status=1
;;
-n|--summary-limit)
summary_limit="$2"
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 2/8] git-submodule.sh: improve parsing of some long options
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-09 16:50 ` [PATCH v2 1/8] git-submodule.sh: make some variables boolean Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 3/8] git-submodule.sh: improve parsing of short options Roy Eldar
` (7 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Some command-line options have a long form which takes an argument. In
this case, the argument can be given right after `='; for example,
"--depth" takes a numerical argument, which can be given as "--depth=X".
Support the case where the argument is given right after `=' for all
long options, in order to improve consistency throughout the script.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 107011f613..a47d2a89f3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
-f | --force)
force=1
;;
@@ -110,6 +113,9 @@ cmd_add()
custom_name=$2
shift
;;
+ --name=*)
+ custom_name="${1#--name=}"
+ ;;
--depth)
case "$2" in '') usage ;; esac
depth="--depth=$2"
@@ -425,6 +431,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
--)
shift
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 3/8] git-submodule.sh: improve parsing of short options
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-09 16:50 ` [PATCH v2 1/8] git-submodule.sh: make some variables boolean Roy Eldar
2024-12-09 16:50 ` [PATCH v2 2/8] git-submodule.sh: improve parsing of some long options Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 4/8] git-submodule.sh: get rid of isnumber Roy Eldar
` (6 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Some command-line options have a short form which takes an argument; for
example, "--jobs" has the form "-j", and it takes a numerical argument.
When parsing short options, support the case where there is no space
between the flag and the option argument, in order to improve
consistency with the rest of the builtin git commands.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index a47d2a89f3..fc85458fb1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -352,6 +355,9 @@ cmd_update()
jobs="--jobs=$2"
shift
;;
+ -j*)
+ jobs="--jobs=${1#-j}"
+ ;;
--jobs=*)
jobs=$1
;;
@@ -431,6 +437,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -519,6 +528,10 @@ cmd_summary() {
isnumber "$summary_limit" || usage
shift
;;
+ -n*)
+ summary_limit="${1#-n}"
+ isnumber "$summary_limit" || usage
+ ;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
isnumber "$summary_limit" || usage
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 4/8] git-submodule.sh: get rid of isnumber
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (2 preceding siblings ...)
2024-12-09 16:50 ` [PATCH v2 3/8] git-submodule.sh: improve parsing of short options Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 5/8] git-submodule.sh: get rid of unused variable Roy Eldar
` (5 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
It's entirely unnecessary to check whether the argument given to an
option (i.e. --summary-limit) is valid in the shell wrapper, since it's
already done when parsing the various options in git-submodule--helper.
Remove this check from the script; this both improves consistency
throughout the script, and the error message shown to the user in case
some invalid non-numeric argument was passed to "--summary-limit" is
more informative as well.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index fc85458fb1..833ac8362b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -53,11 +53,6 @@ jobs=
recommend_shallow=
filter=
-isnumber()
-{
- n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
-}
-
#
# Add a new submodule to the working tree, .gitmodules and the index
#
@@ -524,17 +519,15 @@ cmd_summary() {
for_status=1
;;
-n|--summary-limit)
+ case "$2" in '') usage ;; esac
summary_limit="$2"
- isnumber "$summary_limit" || usage
shift
;;
-n*)
summary_limit="${1#-n}"
- isnumber "$summary_limit" || usage
;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
- isnumber "$summary_limit" || usage
;;
--)
shift
@@ -554,7 +547,7 @@ cmd_summary() {
${files:+--files} \
${cached:+--cached} \
${for_status:+--for-status} \
- ${summary_limit:+-n $summary_limit} \
+ ${summary_limit:+-n "$summary_limit"} \
-- \
"$@"
}
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 5/8] git-submodule.sh: get rid of unused variable
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (3 preceding siblings ...)
2024-12-09 16:50 ` [PATCH v2 4/8] git-submodule.sh: get rid of isnumber Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 6/8] git-submodule.sh: add some comments Roy Eldar
` (4 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Remove the variable "$diff_cmd" which is no longer used.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 833ac8362b..67cdea331b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -503,7 +503,6 @@ cmd_set_url() {
cmd_summary() {
summary_limit=-1
for_status=
- diff_cmd=diff-index
# parse $args after "submodule ... summary".
while test $# -ne 0
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 6/8] git-submodule.sh: add some comments
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (4 preceding siblings ...)
2024-12-09 16:50 ` [PATCH v2 5/8] git-submodule.sh: get rid of unused variable Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 7/8] git-submodule.sh: improve variables readability Roy Eldar
` (3 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Add a couple of comments in a few functions where they were missing.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 67cdea331b..3b44aeddbf 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -418,6 +418,7 @@ cmd_set_branch() {
default=
branch=
+ # parse $args after "submodule ... set-branch".
while test $# -ne 0
do
case "$1" in
@@ -466,6 +467,7 @@ cmd_set_branch() {
# $@ = requested path, requested url
#
cmd_set_url() {
+ # parse $args after "submodule ... set-url".
while test $# -ne 0
do
case "$1" in
@@ -604,6 +606,7 @@ cmd_status()
#
cmd_sync()
{
+ # parse $args after "submodule ... sync".
while test $# -ne 0
do
case "$1" in
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 7/8] git-submodule.sh: improve variables readability
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (5 preceding siblings ...)
2024-12-09 16:50 ` [PATCH v2 6/8] git-submodule.sh: add some comments Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 16:50 ` [PATCH v2 8/8] git-submodule.sh: rename some variables Roy Eldar
` (2 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
When git-submodule.sh parses various options and switches, it sets some
variables to values; the variables in turn affect the options given to
git-submodule--helper.
Currently, variables which correspond to switches have boolean values
(for example, whenever "--force" is passed, force=1), while variables
which correspond to options which take arguments have string values that
sometimes contain the option name and sometimes only the option value.
Set all of the variables to strings which contain the option name (e.g.
force="--force" rather than force=1); this has a couple of advantages:
it improves consistency, readability and debuggability.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 219 +++++++++++++++++++++--------------------------
1 file changed, 98 insertions(+), 121 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 3b44aeddbf..2da4d55d64 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -52,6 +52,10 @@ single_branch=
jobs=
recommend_shallow=
filter=
+deinit_all=
+default=
+summary_limit=
+for_status=
#
# Add a new submodule to the working tree, .gitmodules and the index
@@ -63,37 +67,33 @@ filter=
cmd_add()
{
# parse $args after "submodule ... add".
- reference_path=
while test $# -ne 0
do
case "$1" in
-b | --branch)
case "$2" in '') usage ;; esac
- branch=$2
+ branch="--branch=$2"
shift
;;
- -b*)
- branch="${1#-b}"
- ;;
- --branch=*)
- branch="${1#--branch=}"
+ -b* | --branch*)
+ branch="$1"
;;
-f | --force)
- force=1
+ force=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--progress)
- progress=1
+ progress=$1
;;
--reference)
case "$2" in '') usage ;; esac
- reference_path=$2
+ reference="--reference=$2"
shift
;;
--reference=*)
- reference_path="${1#--reference=}"
+ reference="$1"
;;
--ref-format)
case "$2" in '') usage ;; esac
@@ -104,15 +104,15 @@ cmd_add()
ref_format="$1"
;;
--dissociate)
- dissociate=1
+ dissociate=$1
;;
--name)
case "$2" in '') usage ;; esac
- custom_name=$2
+ custom_name="--name=$2"
shift
;;
--name=*)
- custom_name="${1#--name=}"
+ custom_name="$1"
;;
--depth)
case "$2" in '') usage ;; esac
@@ -120,7 +120,7 @@ cmd_add()
shift
;;
--depth=*)
- depth=$1
+ depth="$1"
;;
--)
shift
@@ -142,14 +142,14 @@ cmd_add()
fi
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${progress:+"--progress"} \
- ${branch:+--branch "$branch"} \
- ${reference_path:+--reference "$reference_path"} \
+ $quiet \
+ $force \
+ $progress \
+ ${branch:+"$branch"} \
+ ${reference:+"$reference"} \
${ref_format:+"$ref_format"} \
- ${dissociate:+--dissociate} \
- ${custom_name:+--name "$custom_name"} \
+ $dissociate \
+ ${custom_name:+"$custom_name"} \
${depth:+"$depth"} \
-- \
"$@"
@@ -168,10 +168,10 @@ cmd_foreach()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
-*)
usage
@@ -184,8 +184,8 @@ cmd_foreach()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach \
- ${quiet:+--quiet} \
- ${recursive:+--recursive} \
+ $quiet \
+ $recursive \
-- \
"$@"
}
@@ -202,7 +202,7 @@ cmd_init()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--)
shift
@@ -219,7 +219,7 @@ cmd_init()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init \
- ${quiet:+--quiet} \
+ $quiet \
-- \
"$@"
}
@@ -230,18 +230,17 @@ cmd_init()
cmd_deinit()
{
# parse $args after "submodule ... deinit".
- deinit_all=
while test $# -ne 0
do
case "$1" in
-f|--force)
- force=1
+ force=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--all)
- deinit_all=t
+ deinit_all=$1
;;
--)
shift
@@ -258,9 +257,9 @@ cmd_deinit()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${deinit_all:+--all} \
+ $quiet \
+ $force \
+ $deinit_all \
-- \
"$@"
}
@@ -277,31 +276,31 @@ cmd_update()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
-v|--verbose)
- quiet=0
+ quiet=
;;
--progress)
- progress=1
+ progress=$1
;;
-i|--init)
- init=1
+ init=$1
;;
--require-init)
- require_init=1
+ require_init=$1
;;
--remote)
- remote=1
+ remote=$1
;;
-N|--no-fetch)
- nofetch=1
+ nofetch=$1
;;
-f|--force)
- force=1
+ force=$1
;;
-r|--rebase)
- rebase=1
+ rebase=$1
;;
--ref-format)
case "$2" in '') usage ;; esac
@@ -320,22 +319,19 @@ cmd_update()
reference="$1"
;;
--dissociate)
- dissociate=1
+ dissociate=$1
;;
-m|--merge)
- merge=1
+ merge=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
--checkout)
- checkout=1
- ;;
- --recommend-shallow)
- recommend_shallow="--recommend-shallow"
+ checkout=$1
;;
- --no-recommend-shallow)
- recommend_shallow="--no-recommend-shallow"
+ --recommend-shallow|--no-recommend-shallow)
+ recommend_shallow=$1
;;
--depth)
case "$2" in '') usage ;; esac
@@ -343,24 +339,18 @@ cmd_update()
shift
;;
--depth=*)
- depth=$1
+ depth="$1"
;;
-j|--jobs)
case "$2" in '') usage ;; esac
jobs="--jobs=$2"
shift
;;
- -j*)
- jobs="--jobs=${1#-j}"
- ;;
- --jobs=*)
- jobs=$1
+ -j*|--jobs*)
+ jobs="$1"
;;
- --single-branch)
- single_branch="--single-branch"
- ;;
- --no-single-branch)
- single_branch="--no-single-branch"
+ --single-branch|--no-single-branch)
+ single_branch=$1
;;
--filter)
case "$2" in '') usage ;; esac
@@ -385,22 +375,21 @@ cmd_update()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${progress:+"--progress"} \
- ${remote:+--remote} \
- ${recursive:+--recursive} \
- ${init:+--init} \
- ${nofetch:+--no-fetch} \
- ${rebase:+--rebase} \
- ${merge:+--merge} \
- ${checkout:+--checkout} \
+ $quiet \
+ $force \
+ $progress \
+ $remote \
+ $recursive \
+ $init \
+ $nofetch \
+ $rebase \
+ $merge \
+ $checkout \
${ref_format:+"$ref_format"} \
${reference:+"$reference"} \
- ${dissociate:+"--dissociate"} \
+ $dissociate \
${depth:+"$depth"} \
- ${require_init:+--require-init} \
- ${dissociate:+"--dissociate"} \
+ $require_init \
$single_branch \
$recommend_shallow \
$jobs \
@@ -415,9 +404,6 @@ cmd_update()
# $@ = requested path
#
cmd_set_branch() {
- default=
- branch=
-
# parse $args after "submodule ... set-branch".
while test $# -ne 0
do
@@ -426,18 +412,15 @@ cmd_set_branch() {
# we don't do anything with this but we need to accept it
;;
-d|--default)
- default=1
+ default=$1
;;
-b|--branch)
case "$2" in '') usage ;; esac
- branch=$2
+ branch="--branch=$2"
shift
;;
- -b*)
- branch="${1#-b}"
- ;;
- --branch=*)
- branch="${1#--branch=}"
+ -b*|--branch=*)
+ branch="$1"
;;
--)
shift
@@ -454,9 +437,9 @@ cmd_set_branch() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch \
- ${quiet:+--quiet} \
- ${branch:+--branch "$branch"} \
- ${default:+--default} \
+ $quiet \
+ ${branch:+"$branch"} \
+ $default \
-- \
"$@"
}
@@ -472,7 +455,7 @@ cmd_set_url() {
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--)
shift
@@ -489,7 +472,7 @@ cmd_set_url() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url \
- ${quiet:+--quiet} \
+ $quiet \
-- \
"$@"
}
@@ -503,32 +486,26 @@ cmd_set_url() {
# $@ = [commit (default 'HEAD'),] requested paths (default all)
#
cmd_summary() {
- summary_limit=-1
- for_status=
-
# parse $args after "submodule ... summary".
while test $# -ne 0
do
case "$1" in
--cached)
- cached=1
+ cached=$1
;;
--files)
- files=1
+ files=$1
;;
--for-status)
- for_status=1
+ for_status=$1
;;
-n|--summary-limit)
case "$2" in '') usage ;; esac
- summary_limit="$2"
+ summary_limit="--summary-limit=$2"
shift
;;
- -n*)
- summary_limit="${1#-n}"
- ;;
- --summary-limit=*)
- summary_limit="${1#--summary-limit=}"
+ -n*|--summary-limit=*)
+ summary_limit="$1"
;;
--)
shift
@@ -545,10 +522,10 @@ cmd_summary() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary \
- ${files:+--files} \
- ${cached:+--cached} \
- ${for_status:+--for-status} \
- ${summary_limit:+-n "$summary_limit"} \
+ $files \
+ $cached \
+ $for_status \
+ ${summary_limit:+"$summary_limit"} \
-- \
"$@"
}
@@ -569,13 +546,13 @@ cmd_status()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--cached)
- cached=1
+ cached=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
--)
shift
@@ -592,9 +569,9 @@ cmd_status()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status \
- ${quiet:+--quiet} \
- ${cached:+--cached} \
- ${recursive:+--recursive} \
+ $quiet \
+ $cached \
+ $recursive \
-- \
"$@"
}
@@ -611,11 +588,11 @@ cmd_sync()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
shift
;;
--recursive)
- recursive=1
+ recursive=$1
shift
;;
--)
@@ -632,8 +609,8 @@ cmd_sync()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync \
- ${quiet:+--quiet} \
- ${recursive:+--recursive} \
+ $quiet \
+ $recursive \
-- \
"$@"
}
@@ -656,10 +633,10 @@ do
command=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--cached)
- cached=1
+ cached=$1
;;
--)
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 8/8] git-submodule.sh: rename some variables
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (6 preceding siblings ...)
2024-12-09 16:50 ` [PATCH v2 7/8] git-submodule.sh: improve variables readability Roy Eldar
@ 2024-12-09 16:50 ` Roy Eldar
2024-12-09 23:26 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Junio C Hamano
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
9 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-09 16:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Every switch and option which is passed to git-submodule.sh has a
corresponding variable which is set accordingly; by convention, the name
of the variable is the option name (for example, "--jobs" and "$jobs").
Rename "$custom_name" and "$deinit_all" in order to improve consistency.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 2da4d55d64..9c10472b5f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -44,7 +44,7 @@ nofetch=
rebase=
merge=
checkout=
-custom_name=
+name=
depth=
progress=
dissociate=
@@ -52,7 +52,7 @@ single_branch=
jobs=
recommend_shallow=
filter=
-deinit_all=
+all=
default=
summary_limit=
for_status=
@@ -108,11 +108,11 @@ cmd_add()
;;
--name)
case "$2" in '') usage ;; esac
- custom_name="--name=$2"
+ name="--name=$2"
shift
;;
--name=*)
- custom_name="$1"
+ name="$1"
;;
--depth)
case "$2" in '') usage ;; esac
@@ -149,7 +149,7 @@ cmd_add()
${reference:+"$reference"} \
${ref_format:+"$ref_format"} \
$dissociate \
- ${custom_name:+"$custom_name"} \
+ ${name:+"$name"} \
${depth:+"$depth"} \
-- \
"$@"
@@ -240,7 +240,7 @@ cmd_deinit()
quiet=$1
;;
--all)
- deinit_all=$1
+ all=$1
;;
--)
shift
@@ -259,7 +259,7 @@ cmd_deinit()
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
$quiet \
$force \
- $deinit_all \
+ $all \
-- \
"$@"
}
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (7 preceding siblings ...)
2024-12-09 16:50 ` [PATCH v2 8/8] git-submodule.sh: rename some variables Roy Eldar
@ 2024-12-09 23:26 ` Junio C Hamano
2024-12-10 0:50 ` Junio C Hamano
2024-12-10 18:11 ` Roy E
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
9 siblings, 2 replies; 43+ messages in thread
From: Junio C Hamano @ 2024-12-09 23:26 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine
Roy Eldar <royeldar0@gmail.com> writes:
> When we run "git submodule", the script parses the various options and
> then invokes "git-submodule--helper". Unlike most builtin git commands
> which parse short/long options using parse-options.c, the parsing of
> arguments is completely done within git-submodule.sh; therefore, there
> are some inconsistencies with the rest of the commands, in particular
> the parsing of option arguments given to various options.
>
> Improve the handling of option arguments for both long & short options;
> for example, passing flags such as "--branch=master" or "-j8" now works.
>
> Changes since v1:
>
> - Make variable values always contain the option name.
> - Rename a couple of variables in order to improve consistency.
After reading this, it was confusing to see [1/8] still doing "1 or
empty" boolean, only to be further modified in [7/8]. We prefer to
see a single series stumbling in the middle and changing the course.
Just a tangent.
While a simple wrapper script is generally easier to debug and read
if $verbose variable's value is "--verbose" or "", there is a case
where following this pattern is not a good idea. If an option we
are eventually going to give to the underlying command takes a
value, the value can contain whitespace, and the option and its
value need to be passed as two separate arguments, it is less error
prone to use the "variable only contains the value" approach.
Imagine that submodule--helper takes a "--foo" option with a greeting
message like "hello world" in such a way. We'd want to trigger it
this way:
git submodule--helper --foo "hello world"
as we are assuming that for some reason we need to pass them as two
words, and
git submodule--helper --foo="hello world"
is not an option. In such a case, a wrapper script that takes such
an optional parameter in $foo is easier to write like so
# parse
foo=
while ...
do
case "$opt" in
--foo=*) foo="${1#--foo=}" ;;
--foo) foo=${2?"--foo without value"}; shift ;;
...
esac
shift
done
# interpolate
git submodule--helper ${foo:+--foo "$foo"}
in order to avoid the value given to the option split at $IFS
whitespace. With foo='--foo="hello world"', passing it to the
underlying command would involve use of eval and becomes error
prone.
I am assuming (but I don't use "git submodule" very often, so my
assumption may be way off) that there is no such variable we need to
pass, but if not, we may need to reconsider and use the "variable has
only value of the option" for at least some of them.
> Link to v1:
>
> https://lore.kernel.org/git/20241207135201.2536-1-royeldar0@gmail.com
>
> Roy Eldar (8):
> git-submodule.sh: make some variables boolean
> git-submodule.sh: improve parsing of some long options
> git-submodule.sh: improve parsing of short options
> git-submodule.sh: get rid of isnumber
> git-submodule.sh: get rid of unused variable
> git-submodule.sh: add some comments
> git-submodule.sh: improve variables readability
> git-submodule.sh: rename some variables
>
> git-submodule.sh | 214 +++++++++++++++++++++++------------------------
> 1 file changed, 104 insertions(+), 110 deletions(-)
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-09 23:26 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Junio C Hamano
@ 2024-12-10 0:50 ` Junio C Hamano
2024-12-10 18:11 ` Roy E
1 sibling, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2024-12-10 0:50 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine
Junio C Hamano <gitster@pobox.com> writes:
> After reading this, it was confusing to see [1/8] still doing "1 or
> empty" boolean, only to be further modified in [7/8]. We prefer to
> see a single series stumbling in the middle and changing the course.
Facepalm. Of course, we prefer _not_ to see such a change of course
in the middle.
Thanks.
> I am assuming (but I don't use "git submodule" very often, so my
> assumption may be way off) that there is no such variable we need to
> pass, but if not, we may need to reconsider and use the "variable has
> only value of the option" for at least some of them.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-09 23:26 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Junio C Hamano
2024-12-10 0:50 ` Junio C Hamano
@ 2024-12-10 18:11 ` Roy E
2024-12-11 0:02 ` Junio C Hamano
1 sibling, 1 reply; 43+ messages in thread
From: Roy E @ 2024-12-10 18:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine
Junio C Hamano <gitster@pobox.com> wrote:
> After reading this, it was confusing to see [1/8] still doing "1 or
> empty" boolean, only to be further modified in [7/8]. We prefer to
> see a single series stumbling in the middle and changing the course.
OK, I will remove this patch from the series and post a v3 series soon.
> While a simple wrapper script is generally easier to debug and read
> if $verbose variable's value is "--verbose" or "", there is a case
> where following this pattern is not a good idea. If an option we
> are eventually going to give to the underlying command takes a
> value, the value can contain whitespace, and the option and its
> value need to be passed as two separate arguments, it is less error
> prone to use the "variable only contains the value" approach.
>
> Imagine that submodule--helper takes a "--foo" option with a greeting
> message like "hello world" in such a way. We'd want to trigger it
> this way:
>
> git submodule--helper --foo "hello world"
>
> as we are assuming that for some reason we need to pass them as two
> words, and
>
> git submodule--helper --foo="hello world"
>
> is not an option. In such a case, a wrapper script that takes such
> an optional parameter in $foo is easier to write like so
>
> # parse
> foo=
> while ...
> do
> case "$opt" in
> --foo=*) foo="${1#--foo=}" ;;
> --foo) foo=${2?"--foo without value"}; shift ;;
> ...
> esac
> shift
> done
>
> # interpolate
> git submodule--helper ${foo:+--foo "$foo"}
>
> in order to avoid the value given to the option split at $IFS
> whitespace. With foo='--foo="hello world"', passing it to the
> underlying command would involve use of eval and becomes error
> prone.
>
> I am assuming (but I don't use "git submodule" very often, so my
> assumption may be way off) that there is no such variable we need to
> pass, but if not, we may need to reconsider and use the "variable has
> only value of the option" for at least some of them.
Indeed, there aren't such variables; all of the options which take
arguments have exactly one argument.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3 0/7] git-submodule.sh: improve parsing of options
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
` (8 preceding siblings ...)
2024-12-09 23:26 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Junio C Hamano
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-10 18:44 ` [PATCH v3 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
` (7 more replies)
9 siblings, 8 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
When we run "git submodule", the script parses the various options and
then invokes "git-submodule--helper". Unlike most builtin git commands
which parse short/long options using parse-options.c, the parsing of
arguments is completely done within git-submodule.sh; therefore, there
are some inconsistencies with the rest of the commands, in particular
the parsing of option arguments given to various options.
Improve the handling of option arguments for both long & short options;
for example, passing flags such as "--branch=master" or "-j8" now works.
Changes since v2:
- Remove redundant patch from the series.
- Rename another variable for the sake of consistency.
Link to v2:
https://lore.kernel.org/git/20241209165009.40653-1-royeldar0@gmail.com
Roy Eldar (7):
git-submodule.sh: improve parsing of some long options
git-submodule.sh: improve parsing of short options
git-submodule.sh: get rid of isnumber
git-submodule.sh: get rid of unused variable
git-submodule.sh: add some comments
git-submodule.sh: improve variables readability
git-submodule.sh: rename some variables
git-submodule.sh | 216 +++++++++++++++++++++++------------------------
1 file changed, 105 insertions(+), 111 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3 1/7] git-submodule.sh: improve parsing of some long options
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-10 18:44 ` [PATCH v3 2/7] git-submodule.sh: improve parsing of short options Roy Eldar
` (6 subsequent siblings)
7 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Some command-line options have a long form which takes an argument. In
this case, the argument can be given right after `='; for example,
"--depth" takes a numerical argument, which can be given as "--depth=X".
Support the case where the argument is given right after `=' for all
long options, in order to improve consistency throughout the script.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 03c5a220a2..d3e3669fde 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
-f | --force)
force=$1
;;
@@ -110,6 +113,9 @@ cmd_add()
custom_name=$2
shift
;;
+ --name=*)
+ custom_name="${1#--name=}"
+ ;;
--depth)
case "$2" in '') usage ;; esac
depth="--depth=$2"
@@ -425,6 +431,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
--)
shift
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 2/7] git-submodule.sh: improve parsing of short options
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
2024-12-10 18:44 ` [PATCH v3 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-10 18:44 ` [PATCH v3 3/7] git-submodule.sh: get rid of isnumber Roy Eldar
` (5 subsequent siblings)
7 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Some command-line options have a short form which takes an argument; for
example, "--jobs" has the form "-j", and it takes a numerical argument.
When parsing short options, support the case where there is no space
between the flag and the option argument, in order to improve
consistency with the rest of the builtin git commands.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index d3e3669fde..fd54cb8fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -352,6 +355,9 @@ cmd_update()
jobs="--jobs=$2"
shift
;;
+ -j*)
+ jobs="--jobs=${1#-j}"
+ ;;
--jobs=*)
jobs=$1
;;
@@ -431,6 +437,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -519,6 +528,10 @@ cmd_summary() {
isnumber "$summary_limit" || usage
shift
;;
+ -n*)
+ summary_limit="${1#-n}"
+ isnumber "$summary_limit" || usage
+ ;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
isnumber "$summary_limit" || usage
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 3/7] git-submodule.sh: get rid of isnumber
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
2024-12-10 18:44 ` [PATCH v3 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
2024-12-10 18:44 ` [PATCH v3 2/7] git-submodule.sh: improve parsing of short options Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-10 18:44 ` [PATCH v3 4/7] git-submodule.sh: get rid of unused variable Roy Eldar
` (4 subsequent siblings)
7 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
It's entirely unnecessary to check whether the argument given to an
option (i.e. --summary-limit) is valid in the shell wrapper, since it's
already done when parsing the various options in git-submodule--helper.
Remove this check from the script; this both improves consistency
throughout the script, and the error message shown to the user in case
some invalid non-numeric argument was passed to "--summary-limit" is
more informative as well.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index fd54cb8fa6..3adaa8d9a3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -53,11 +53,6 @@ jobs=
recommend_shallow=
filter=
-isnumber()
-{
- n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
-}
-
#
# Add a new submodule to the working tree, .gitmodules and the index
#
@@ -524,17 +519,15 @@ cmd_summary() {
for_status="$1"
;;
-n|--summary-limit)
+ case "$2" in '') usage ;; esac
summary_limit="$2"
- isnumber "$summary_limit" || usage
shift
;;
-n*)
summary_limit="${1#-n}"
- isnumber "$summary_limit" || usage
;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
- isnumber "$summary_limit" || usage
;;
--)
shift
@@ -554,7 +547,7 @@ cmd_summary() {
${files:+--files} \
${cached:+--cached} \
${for_status:+--for-status} \
- ${summary_limit:+-n $summary_limit} \
+ ${summary_limit:+-n "$summary_limit"} \
-- \
"$@"
}
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 4/7] git-submodule.sh: get rid of unused variable
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
` (2 preceding siblings ...)
2024-12-10 18:44 ` [PATCH v3 3/7] git-submodule.sh: get rid of isnumber Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-10 18:44 ` [PATCH v3 5/7] git-submodule.sh: add some comments Roy Eldar
` (3 subsequent siblings)
7 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Remove the variable "$diff_cmd" which is no longer used.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 3adaa8d9a3..ba3bef8821 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -503,7 +503,6 @@ cmd_set_url() {
cmd_summary() {
summary_limit=-1
for_status=
- diff_cmd=diff-index
# parse $args after "submodule ... summary".
while test $# -ne 0
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 5/7] git-submodule.sh: add some comments
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
` (3 preceding siblings ...)
2024-12-10 18:44 ` [PATCH v3 4/7] git-submodule.sh: get rid of unused variable Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-10 18:44 ` [PATCH v3 6/7] git-submodule.sh: improve variables readability Roy Eldar
` (2 subsequent siblings)
7 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Add a couple of comments in a few functions where they were missing.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index ba3bef8821..ee86e4de46 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -418,6 +418,7 @@ cmd_set_branch() {
default=
branch=
+ # parse $args after "submodule ... set-branch".
while test $# -ne 0
do
case "$1" in
@@ -466,6 +467,7 @@ cmd_set_branch() {
# $@ = requested path, requested url
#
cmd_set_url() {
+ # parse $args after "submodule ... set-url".
while test $# -ne 0
do
case "$1" in
@@ -604,6 +606,7 @@ cmd_status()
#
cmd_sync()
{
+ # parse $args after "submodule ... sync".
while test $# -ne 0
do
case "$1" in
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 6/7] git-submodule.sh: improve variables readability
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
` (4 preceding siblings ...)
2024-12-10 18:44 ` [PATCH v3 5/7] git-submodule.sh: add some comments Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-11 0:14 ` Junio C Hamano
2024-12-11 1:56 ` Đoàn Trần Công Danh
2024-12-10 18:44 ` [PATCH v3 7/7] git-submodule.sh: rename some variables Roy Eldar
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
7 siblings, 2 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
When git-submodule.sh parses various options and switches, it sets some
variables to values; the variables in turn affect the options given to
git-submodule--helper.
Currently, variables which correspond to switches have boolean values
(for example, whenever "--force" is passed, force=1), while variables
which correspond to options which take arguments have string values that
sometimes contain the option name and sometimes only the option value.
Set all of the variables to strings which contain the option name (e.g.
force="--force" rather than force=1); this has a couple of advantages:
it improves consistency, readability and debuggability.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 213 +++++++++++++++++++++--------------------------
1 file changed, 95 insertions(+), 118 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index ee86e4de46..2da4d55d64 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -52,6 +52,10 @@ single_branch=
jobs=
recommend_shallow=
filter=
+deinit_all=
+default=
+summary_limit=
+for_status=
#
# Add a new submodule to the working tree, .gitmodules and the index
@@ -63,37 +67,33 @@ filter=
cmd_add()
{
# parse $args after "submodule ... add".
- reference_path=
while test $# -ne 0
do
case "$1" in
-b | --branch)
case "$2" in '') usage ;; esac
- branch=$2
+ branch="--branch=$2"
shift
;;
- -b*)
- branch="${1#-b}"
- ;;
- --branch=*)
- branch="${1#--branch=}"
+ -b* | --branch*)
+ branch="$1"
;;
-f | --force)
force=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--progress)
- progress=1
+ progress=$1
;;
--reference)
case "$2" in '') usage ;; esac
- reference_path=$2
+ reference="--reference=$2"
shift
;;
--reference=*)
- reference_path="${1#--reference=}"
+ reference="$1"
;;
--ref-format)
case "$2" in '') usage ;; esac
@@ -104,15 +104,15 @@ cmd_add()
ref_format="$1"
;;
--dissociate)
- dissociate=1
+ dissociate=$1
;;
--name)
case "$2" in '') usage ;; esac
- custom_name=$2
+ custom_name="--name=$2"
shift
;;
--name=*)
- custom_name="${1#--name=}"
+ custom_name="$1"
;;
--depth)
case "$2" in '') usage ;; esac
@@ -120,7 +120,7 @@ cmd_add()
shift
;;
--depth=*)
- depth=$1
+ depth="$1"
;;
--)
shift
@@ -142,14 +142,14 @@ cmd_add()
fi
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${progress:+"--progress"} \
- ${branch:+--branch "$branch"} \
- ${reference_path:+--reference "$reference_path"} \
+ $quiet \
+ $force \
+ $progress \
+ ${branch:+"$branch"} \
+ ${reference:+"$reference"} \
${ref_format:+"$ref_format"} \
- ${dissociate:+--dissociate} \
- ${custom_name:+--name "$custom_name"} \
+ $dissociate \
+ ${custom_name:+"$custom_name"} \
${depth:+"$depth"} \
-- \
"$@"
@@ -168,10 +168,10 @@ cmd_foreach()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
-*)
usage
@@ -184,8 +184,8 @@ cmd_foreach()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach \
- ${quiet:+--quiet} \
- ${recursive:+--recursive} \
+ $quiet \
+ $recursive \
-- \
"$@"
}
@@ -202,7 +202,7 @@ cmd_init()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--)
shift
@@ -219,7 +219,7 @@ cmd_init()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init \
- ${quiet:+--quiet} \
+ $quiet \
-- \
"$@"
}
@@ -230,7 +230,6 @@ cmd_init()
cmd_deinit()
{
# parse $args after "submodule ... deinit".
- deinit_all=
while test $# -ne 0
do
case "$1" in
@@ -238,10 +237,10 @@ cmd_deinit()
force=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--all)
- deinit_all=t
+ deinit_all=$1
;;
--)
shift
@@ -258,9 +257,9 @@ cmd_deinit()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${deinit_all:+--all} \
+ $quiet \
+ $force \
+ $deinit_all \
-- \
"$@"
}
@@ -277,31 +276,31 @@ cmd_update()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
-v|--verbose)
- quiet=0
+ quiet=
;;
--progress)
- progress=1
+ progress=$1
;;
-i|--init)
- init=1
+ init=$1
;;
--require-init)
- require_init=1
+ require_init=$1
;;
--remote)
- remote=1
+ remote=$1
;;
-N|--no-fetch)
- nofetch=1
+ nofetch=$1
;;
-f|--force)
force=$1
;;
-r|--rebase)
- rebase=1
+ rebase=$1
;;
--ref-format)
case "$2" in '') usage ;; esac
@@ -320,22 +319,19 @@ cmd_update()
reference="$1"
;;
--dissociate)
- dissociate=1
+ dissociate=$1
;;
-m|--merge)
- merge=1
+ merge=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
--checkout)
- checkout=1
- ;;
- --recommend-shallow)
- recommend_shallow="--recommend-shallow"
+ checkout=$1
;;
- --no-recommend-shallow)
- recommend_shallow="--no-recommend-shallow"
+ --recommend-shallow|--no-recommend-shallow)
+ recommend_shallow=$1
;;
--depth)
case "$2" in '') usage ;; esac
@@ -343,24 +339,18 @@ cmd_update()
shift
;;
--depth=*)
- depth=$1
+ depth="$1"
;;
-j|--jobs)
case "$2" in '') usage ;; esac
jobs="--jobs=$2"
shift
;;
- -j*)
- jobs="--jobs=${1#-j}"
- ;;
- --jobs=*)
- jobs=$1
+ -j*|--jobs*)
+ jobs="$1"
;;
- --single-branch)
- single_branch="--single-branch"
- ;;
- --no-single-branch)
- single_branch="--no-single-branch"
+ --single-branch|--no-single-branch)
+ single_branch=$1
;;
--filter)
case "$2" in '') usage ;; esac
@@ -385,22 +375,21 @@ cmd_update()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${progress:+"--progress"} \
- ${remote:+--remote} \
- ${recursive:+--recursive} \
- ${init:+--init} \
- ${nofetch:+--no-fetch} \
- ${rebase:+--rebase} \
- ${merge:+--merge} \
- ${checkout:+--checkout} \
+ $quiet \
+ $force \
+ $progress \
+ $remote \
+ $recursive \
+ $init \
+ $nofetch \
+ $rebase \
+ $merge \
+ $checkout \
${ref_format:+"$ref_format"} \
${reference:+"$reference"} \
- ${dissociate:+"--dissociate"} \
+ $dissociate \
${depth:+"$depth"} \
- ${require_init:+--require-init} \
- ${dissociate:+"--dissociate"} \
+ $require_init \
$single_branch \
$recommend_shallow \
$jobs \
@@ -415,9 +404,6 @@ cmd_update()
# $@ = requested path
#
cmd_set_branch() {
- default=
- branch=
-
# parse $args after "submodule ... set-branch".
while test $# -ne 0
do
@@ -426,18 +412,15 @@ cmd_set_branch() {
# we don't do anything with this but we need to accept it
;;
-d|--default)
- default=1
+ default=$1
;;
-b|--branch)
case "$2" in '') usage ;; esac
- branch=$2
+ branch="--branch=$2"
shift
;;
- -b*)
- branch="${1#-b}"
- ;;
- --branch=*)
- branch="${1#--branch=}"
+ -b*|--branch=*)
+ branch="$1"
;;
--)
shift
@@ -454,9 +437,9 @@ cmd_set_branch() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch \
- ${quiet:+--quiet} \
- ${branch:+--branch "$branch"} \
- ${default:+--default} \
+ $quiet \
+ ${branch:+"$branch"} \
+ $default \
-- \
"$@"
}
@@ -472,7 +455,7 @@ cmd_set_url() {
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--)
shift
@@ -489,7 +472,7 @@ cmd_set_url() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url \
- ${quiet:+--quiet} \
+ $quiet \
-- \
"$@"
}
@@ -503,32 +486,26 @@ cmd_set_url() {
# $@ = [commit (default 'HEAD'),] requested paths (default all)
#
cmd_summary() {
- summary_limit=-1
- for_status=
-
# parse $args after "submodule ... summary".
while test $# -ne 0
do
case "$1" in
--cached)
- cached=1
+ cached=$1
;;
--files)
- files="$1"
+ files=$1
;;
--for-status)
- for_status="$1"
+ for_status=$1
;;
-n|--summary-limit)
case "$2" in '') usage ;; esac
- summary_limit="$2"
+ summary_limit="--summary-limit=$2"
shift
;;
- -n*)
- summary_limit="${1#-n}"
- ;;
- --summary-limit=*)
- summary_limit="${1#--summary-limit=}"
+ -n*|--summary-limit=*)
+ summary_limit="$1"
;;
--)
shift
@@ -545,10 +522,10 @@ cmd_summary() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary \
- ${files:+--files} \
- ${cached:+--cached} \
- ${for_status:+--for-status} \
- ${summary_limit:+-n "$summary_limit"} \
+ $files \
+ $cached \
+ $for_status \
+ ${summary_limit:+"$summary_limit"} \
-- \
"$@"
}
@@ -569,13 +546,13 @@ cmd_status()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--cached)
- cached=1
+ cached=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
--)
shift
@@ -592,9 +569,9 @@ cmd_status()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status \
- ${quiet:+--quiet} \
- ${cached:+--cached} \
- ${recursive:+--recursive} \
+ $quiet \
+ $cached \
+ $recursive \
-- \
"$@"
}
@@ -611,11 +588,11 @@ cmd_sync()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
shift
;;
--recursive)
- recursive=1
+ recursive=$1
shift
;;
--)
@@ -632,8 +609,8 @@ cmd_sync()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync \
- ${quiet:+--quiet} \
- ${recursive:+--recursive} \
+ $quiet \
+ $recursive \
-- \
"$@"
}
@@ -656,10 +633,10 @@ do
command=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--cached)
- cached=1
+ cached=$1
;;
--)
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 7/7] git-submodule.sh: rename some variables
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
` (5 preceding siblings ...)
2024-12-10 18:44 ` [PATCH v3 6/7] git-submodule.sh: improve variables readability Roy Eldar
@ 2024-12-10 18:44 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
7 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-10 18:44 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine, Roy Eldar
Every switch and option which is passed to git-submodule.sh has a
corresponding variable which is set accordingly; by convention, the name
of the variable is the option name (for example, "--jobs" and "$jobs").
Rename "$custom_name", "$deinit_all" and "$nofetch", for consistency.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 2da4d55d64..24276847ad 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -40,11 +40,11 @@ init=
require_init=
files=
remote=
-nofetch=
+no_fetch=
rebase=
merge=
checkout=
-custom_name=
+name=
depth=
progress=
dissociate=
@@ -52,7 +52,7 @@ single_branch=
jobs=
recommend_shallow=
filter=
-deinit_all=
+all=
default=
summary_limit=
for_status=
@@ -108,11 +108,11 @@ cmd_add()
;;
--name)
case "$2" in '') usage ;; esac
- custom_name="--name=$2"
+ name="--name=$2"
shift
;;
--name=*)
- custom_name="$1"
+ name="$1"
;;
--depth)
case "$2" in '') usage ;; esac
@@ -149,7 +149,7 @@ cmd_add()
${reference:+"$reference"} \
${ref_format:+"$ref_format"} \
$dissociate \
- ${custom_name:+"$custom_name"} \
+ ${name:+"$name"} \
${depth:+"$depth"} \
-- \
"$@"
@@ -240,7 +240,7 @@ cmd_deinit()
quiet=$1
;;
--all)
- deinit_all=$1
+ all=$1
;;
--)
shift
@@ -259,7 +259,7 @@ cmd_deinit()
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
$quiet \
$force \
- $deinit_all \
+ $all \
-- \
"$@"
}
@@ -294,7 +294,7 @@ cmd_update()
remote=$1
;;
-N|--no-fetch)
- nofetch=$1
+ no_fetch=$1
;;
-f|--force)
force=$1
@@ -381,7 +381,7 @@ cmd_update()
$remote \
$recursive \
$init \
- $nofetch \
+ $no_fetch \
$rebase \
$merge \
$checkout \
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-10 18:11 ` Roy E
@ 2024-12-11 0:02 ` Junio C Hamano
2024-12-11 6:13 ` Roy E
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2024-12-11 0:02 UTC (permalink / raw)
To: Roy E
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine
Roy E <royeldar0@gmail.com> writes:
>> I am assuming (but I don't use "git submodule" very often, so my
>> assumption may be way off) that there is no such variable we need to
>> pass, but if not, we may need to reconsider and use the "variable has
>> only value of the option" for at least some of them.
>
> Indeed, there aren't such variables; all of the options which take
> arguments have exactly one argument.
Just to make sure we are on the same page,
--foo "hello world"
is an example of an option "foo" that takes exactly one argument, a
string which happens to have a whitespace in it, and is an example
for which "variable has the dash-dash option, equals, and its value"
pattern would not work well.
If we pass an argument that is an end-user provided message or is a
project controlled pathname, we are likely to be in the same
situation.
If we can pass it as
--foo="hello world"
then we are safe, as we can do
foo="--foo=hello world"
... later ...
git cmd ${foo:+"$foo"}
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 6/7] git-submodule.sh: improve variables readability
2024-12-10 18:44 ` [PATCH v3 6/7] git-submodule.sh: improve variables readability Roy Eldar
@ 2024-12-11 0:14 ` Junio C Hamano
2024-12-11 6:21 ` Roy E
2024-12-11 1:56 ` Đoàn Trần Công Danh
1 sibling, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2024-12-11 0:14 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine
Roy Eldar <royeldar0@gmail.com> writes:
> + -b* | --branch*)
> + branch="$1"
Once another option like "--branches-only" will be caught by this
case arm. Do not omit "=" in the pattern.
> @@ -142,14 +142,14 @@ cmd_add()
> fi
>
> git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add \
> - ${quiet:+--quiet} \
> - ${force:+--force} \
> - ${progress:+"--progress"} \
> - ${branch:+--branch "$branch"} \
> - ${reference_path:+--reference "$reference_path"} \
> + $quiet \
> + $force \
> + $progress \
> + ${branch:+"$branch"} \
> + ${reference:+"$reference"} \
> ${ref_format:+"$ref_format"} \
> - ${dissociate:+--dissociate} \
> - ${custom_name:+--name "$custom_name"} \
> + $dissociate \
> + ${custom_name:+"$custom_name"} \
> ${depth:+"$depth"} \
> -- \
> "$@"
Looking good.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 6/7] git-submodule.sh: improve variables readability
2024-12-10 18:44 ` [PATCH v3 6/7] git-submodule.sh: improve variables readability Roy Eldar
2024-12-11 0:14 ` Junio C Hamano
@ 2024-12-11 1:56 ` Đoàn Trần Công Danh
2024-12-11 6:09 ` Junio C Hamano
1 sibling, 1 reply; 43+ messages in thread
From: Đoàn Trần Công Danh @ 2024-12-11 1:56 UTC (permalink / raw)
To: Roy Eldar
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine
On 2024-12-10 20:44:41+0200, Roy Eldar <royeldar0@gmail.com> wrote:
> ;;
> --reference)
> case "$2" in '') usage ;; esac
> - reference_path=$2
> + reference="--reference=$2"
> shift
> ;;
> --reference=*)
> - reference_path="${1#--reference=}"
> + reference="$1"
--reference takes a path to some repository,
(see also git-clone --reference),
thus it can have any characters, including but not limit to whitespace.
I think we need to discard this hunk!
--
Danh
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 6/7] git-submodule.sh: improve variables readability
2024-12-11 1:56 ` Đoàn Trần Công Danh
@ 2024-12-11 6:09 ` Junio C Hamano
0 siblings, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2024-12-11 6:09 UTC (permalink / raw)
To: Đoàn Trần Công Danh
Cc: Roy Eldar, git, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes:
>> --reference=*)
>> - reference_path="${1#--reference=}"
>> + reference="$1"
>
> --reference takes a path to some repository,
> (see also git-clone --reference),
> thus it can have any characters, including but not limit to whitespace.
> I think we need to discard this hunk!
I didn't double check the code that uses the variable, but as long
as the code that uses $reference writs it correctly, e.g.
git subcmd ${reference:+"$reference"} ...
it should correctly pass what it received in the above assignment
from the user in $1 just fine, even if its value can contain any
arbitrary byte, I would think.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-11 0:02 ` Junio C Hamano
@ 2024-12-11 6:13 ` Roy E
2024-12-11 6:16 ` Junio C Hamano
0 siblings, 1 reply; 43+ messages in thread
From: Roy E @ 2024-12-11 6:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine, Đoàn Trần Công Danh
Junio C Hamano <gitster@pobox.com> wrote:
> Just to make sure we are on the same page,
>
> --foo "hello world"
>
> is an example of an option "foo" that takes exactly one argument, a
> string which happens to have a whitespace in it, and is an example
> for which "variable has the dash-dash option, equals, and its value"
> pattern would not work well.
I'm not sure why then this pattern would not work; when the argument is
passed to the option in this case, we set the variable to "--foo=$2",
so it should be fine (like you've written below).
> If we can pass it as
>
> --foo="hello world"
>
> then we are safe, as we can do
>
> foo="--foo=hello world"
> ... later ...
> git cmd ${foo:+"$foo"}
All of the options with arguments of git-submodule--helper can be
passed as "--foo=...", and spaces (or other misc characters) that
appear in the option value shouldn't pose any problem whatsoever;
the logic in parse-options.c::parse_long_opt confirms that.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/8] git-submodule.sh: improve parsing of options
2024-12-11 6:13 ` Roy E
@ 2024-12-11 6:16 ` Junio C Hamano
0 siblings, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2024-12-11 6:16 UTC (permalink / raw)
To: Roy E
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine, Đoàn Trần Công Danh
Roy E <royeldar0@gmail.com> writes:
> All of the options with arguments of git-submodule--helper can be
> passed as "--foo=...",
That part was what I was cautioning about. I suspect it is true as
it should be using parse-options API but didn't double check myself.
> and spaces (or other misc characters) that
> appear in the option value shouldn't pose any problem whatsoever;
The latter is consistent with what I gave you.
Thanks.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 6/7] git-submodule.sh: improve variables readability
2024-12-11 0:14 ` Junio C Hamano
@ 2024-12-11 6:21 ` Roy E
0 siblings, 0 replies; 43+ messages in thread
From: Roy E @ 2024-12-11 6:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Eric Sunshine
Junio C Hamano <gitster@pobox.com> wrote:
> Roy Eldar <royeldar0@gmail.com> writes:
>
> > + -b* | --branch*)
> > + branch="$1"
>
> Once another option like "--branches-only" will be caught by this
> case arm. Do not omit "=" in the pattern.
Thanks. I noticed that I did so in a couple of other places as well, so
I'll fix that and send a v4 patch series.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 0/7] git-submodule.sh: improve parsing of options
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
` (6 preceding siblings ...)
2024-12-10 18:44 ` [PATCH v3 7/7] git-submodule.sh: rename some variables Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
` (6 more replies)
7 siblings, 7 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
When we run "git submodule", the script parses the various options and
then invokes "git-submodule--helper". Unlike most builtin git commands
which parse short/long options using parse-options.c, the parsing of
arguments is completely done within git-submodule.sh; therefore, there
are some inconsistencies with the rest of the commands, in particular
the parsing of option arguments given to various options.
Improve the handling of option arguments for both long & short options;
for example, passing flags such as "--branch=master" or "-j8" now works.
Changes since v3:
- Minor typos in some switch case patterns.
Link to v3:
https://lore.kernel.org/git/20241210184442.10723-1-royeldar0@gmail.com
Roy Eldar (7):
git-submodule.sh: improve parsing of some long options
git-submodule.sh: improve parsing of short options
git-submodule.sh: get rid of isnumber
git-submodule.sh: get rid of unused variable
git-submodule.sh: add some comments
git-submodule.sh: improve variables readability
git-submodule.sh: rename some variables
git-submodule.sh | 216 +++++++++++++++++++++++------------------------
1 file changed, 105 insertions(+), 111 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 1/7] git-submodule.sh: improve parsing of some long options
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 2/7] git-submodule.sh: improve parsing of short options Roy Eldar
` (5 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
Some command-line options have a long form which takes an argument. In
this case, the argument can be given right after `='; for example,
"--depth" takes a numerical argument, which can be given as "--depth=X".
Support the case where the argument is given right after `=' for all
long options, in order to improve consistency throughout the script.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 03c5a220a2..d3e3669fde 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
-f | --force)
force=$1
;;
@@ -110,6 +113,9 @@ cmd_add()
custom_name=$2
shift
;;
+ --name=*)
+ custom_name="${1#--name=}"
+ ;;
--depth)
case "$2" in '') usage ;; esac
depth="--depth=$2"
@@ -425,6 +431,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ --branch=*)
+ branch="${1#--branch=}"
+ ;;
--)
shift
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 2/7] git-submodule.sh: improve parsing of short options
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-11 6:32 ` [PATCH v4 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 3/7] git-submodule.sh: get rid of isnumber Roy Eldar
` (4 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
Some command-line options have a short form which takes an argument; for
example, "--jobs" has the form "-j", and it takes a numerical argument.
When parsing short options, support the case where there is no space
between the flag and the option argument, in order to improve
consistency with the rest of the builtin git commands.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index d3e3669fde..fd54cb8fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -77,6 +77,9 @@ cmd_add()
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -352,6 +355,9 @@ cmd_update()
jobs="--jobs=$2"
shift
;;
+ -j*)
+ jobs="--jobs=${1#-j}"
+ ;;
--jobs=*)
jobs=$1
;;
@@ -431,6 +437,9 @@ cmd_set_branch() {
branch=$2
shift
;;
+ -b*)
+ branch="${1#-b}"
+ ;;
--branch=*)
branch="${1#--branch=}"
;;
@@ -519,6 +528,10 @@ cmd_summary() {
isnumber "$summary_limit" || usage
shift
;;
+ -n*)
+ summary_limit="${1#-n}"
+ isnumber "$summary_limit" || usage
+ ;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
isnumber "$summary_limit" || usage
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 3/7] git-submodule.sh: get rid of isnumber
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-11 6:32 ` [PATCH v4 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
2024-12-11 6:32 ` [PATCH v4 2/7] git-submodule.sh: improve parsing of short options Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 4/7] git-submodule.sh: get rid of unused variable Roy Eldar
` (3 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
It's entirely unnecessary to check whether the argument given to an
option (i.e. --summary-limit) is valid in the shell wrapper, since it's
already done when parsing the various options in git-submodule--helper.
Remove this check from the script; this both improves consistency
throughout the script, and the error message shown to the user in case
some invalid non-numeric argument was passed to "--summary-limit" is
more informative as well.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index fd54cb8fa6..3adaa8d9a3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -53,11 +53,6 @@ jobs=
recommend_shallow=
filter=
-isnumber()
-{
- n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
-}
-
#
# Add a new submodule to the working tree, .gitmodules and the index
#
@@ -524,17 +519,15 @@ cmd_summary() {
for_status="$1"
;;
-n|--summary-limit)
+ case "$2" in '') usage ;; esac
summary_limit="$2"
- isnumber "$summary_limit" || usage
shift
;;
-n*)
summary_limit="${1#-n}"
- isnumber "$summary_limit" || usage
;;
--summary-limit=*)
summary_limit="${1#--summary-limit=}"
- isnumber "$summary_limit" || usage
;;
--)
shift
@@ -554,7 +547,7 @@ cmd_summary() {
${files:+--files} \
${cached:+--cached} \
${for_status:+--for-status} \
- ${summary_limit:+-n $summary_limit} \
+ ${summary_limit:+-n "$summary_limit"} \
-- \
"$@"
}
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 4/7] git-submodule.sh: get rid of unused variable
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
` (2 preceding siblings ...)
2024-12-11 6:32 ` [PATCH v4 3/7] git-submodule.sh: get rid of isnumber Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 5/7] git-submodule.sh: add some comments Roy Eldar
` (2 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
Remove the variable "$diff_cmd" which is no longer used.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 3adaa8d9a3..ba3bef8821 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -503,7 +503,6 @@ cmd_set_url() {
cmd_summary() {
summary_limit=-1
for_status=
- diff_cmd=diff-index
# parse $args after "submodule ... summary".
while test $# -ne 0
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 5/7] git-submodule.sh: add some comments
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
` (3 preceding siblings ...)
2024-12-11 6:32 ` [PATCH v4 4/7] git-submodule.sh: get rid of unused variable Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 6/7] git-submodule.sh: improve variables readability Roy Eldar
2024-12-11 6:32 ` [PATCH v4 7/7] git-submodule.sh: rename some variables Roy Eldar
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
Add a couple of comments in a few functions where they were missing.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index ba3bef8821..ee86e4de46 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -418,6 +418,7 @@ cmd_set_branch() {
default=
branch=
+ # parse $args after "submodule ... set-branch".
while test $# -ne 0
do
case "$1" in
@@ -466,6 +467,7 @@ cmd_set_branch() {
# $@ = requested path, requested url
#
cmd_set_url() {
+ # parse $args after "submodule ... set-url".
while test $# -ne 0
do
case "$1" in
@@ -604,6 +606,7 @@ cmd_status()
#
cmd_sync()
{
+ # parse $args after "submodule ... sync".
while test $# -ne 0
do
case "$1" in
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 6/7] git-submodule.sh: improve variables readability
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
` (4 preceding siblings ...)
2024-12-11 6:32 ` [PATCH v4 5/7] git-submodule.sh: add some comments Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
2024-12-11 6:32 ` [PATCH v4 7/7] git-submodule.sh: rename some variables Roy Eldar
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
When git-submodule.sh parses various options and switches, it sets some
variables to values; the variables in turn affect the options given to
git-submodule--helper.
Currently, variables which correspond to switches have boolean values
(for example, whenever "--force" is passed, force=1), while variables
which correspond to options which take arguments have string values that
sometimes contain the option name and sometimes only the option value.
Set all of the variables to strings which contain the option name (e.g.
force="--force" rather than force=1); this has a couple of advantages:
it improves consistency, readability and debuggability.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 213 +++++++++++++++++++++--------------------------
1 file changed, 95 insertions(+), 118 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index ee86e4de46..6df25efc48 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -52,6 +52,10 @@ single_branch=
jobs=
recommend_shallow=
filter=
+deinit_all=
+default=
+summary_limit=
+for_status=
#
# Add a new submodule to the working tree, .gitmodules and the index
@@ -63,37 +67,33 @@ filter=
cmd_add()
{
# parse $args after "submodule ... add".
- reference_path=
while test $# -ne 0
do
case "$1" in
-b | --branch)
case "$2" in '') usage ;; esac
- branch=$2
+ branch="--branch=$2"
shift
;;
- -b*)
- branch="${1#-b}"
- ;;
- --branch=*)
- branch="${1#--branch=}"
+ -b* | --branch=*)
+ branch="$1"
;;
-f | --force)
force=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--progress)
- progress=1
+ progress=$1
;;
--reference)
case "$2" in '') usage ;; esac
- reference_path=$2
+ reference="--reference=$2"
shift
;;
--reference=*)
- reference_path="${1#--reference=}"
+ reference="$1"
;;
--ref-format)
case "$2" in '') usage ;; esac
@@ -104,15 +104,15 @@ cmd_add()
ref_format="$1"
;;
--dissociate)
- dissociate=1
+ dissociate=$1
;;
--name)
case "$2" in '') usage ;; esac
- custom_name=$2
+ custom_name="--name=$2"
shift
;;
--name=*)
- custom_name="${1#--name=}"
+ custom_name="$1"
;;
--depth)
case "$2" in '') usage ;; esac
@@ -120,7 +120,7 @@ cmd_add()
shift
;;
--depth=*)
- depth=$1
+ depth="$1"
;;
--)
shift
@@ -142,14 +142,14 @@ cmd_add()
fi
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${progress:+"--progress"} \
- ${branch:+--branch "$branch"} \
- ${reference_path:+--reference "$reference_path"} \
+ $quiet \
+ $force \
+ $progress \
+ ${branch:+"$branch"} \
+ ${reference:+"$reference"} \
${ref_format:+"$ref_format"} \
- ${dissociate:+--dissociate} \
- ${custom_name:+--name "$custom_name"} \
+ $dissociate \
+ ${custom_name:+"$custom_name"} \
${depth:+"$depth"} \
-- \
"$@"
@@ -168,10 +168,10 @@ cmd_foreach()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
-*)
usage
@@ -184,8 +184,8 @@ cmd_foreach()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach \
- ${quiet:+--quiet} \
- ${recursive:+--recursive} \
+ $quiet \
+ $recursive \
-- \
"$@"
}
@@ -202,7 +202,7 @@ cmd_init()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--)
shift
@@ -219,7 +219,7 @@ cmd_init()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init \
- ${quiet:+--quiet} \
+ $quiet \
-- \
"$@"
}
@@ -230,7 +230,6 @@ cmd_init()
cmd_deinit()
{
# parse $args after "submodule ... deinit".
- deinit_all=
while test $# -ne 0
do
case "$1" in
@@ -238,10 +237,10 @@ cmd_deinit()
force=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--all)
- deinit_all=t
+ deinit_all=$1
;;
--)
shift
@@ -258,9 +257,9 @@ cmd_deinit()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${deinit_all:+--all} \
+ $quiet \
+ $force \
+ $deinit_all \
-- \
"$@"
}
@@ -277,31 +276,31 @@ cmd_update()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
-v|--verbose)
- quiet=0
+ quiet=
;;
--progress)
- progress=1
+ progress=$1
;;
-i|--init)
- init=1
+ init=$1
;;
--require-init)
- require_init=1
+ require_init=$1
;;
--remote)
- remote=1
+ remote=$1
;;
-N|--no-fetch)
- nofetch=1
+ nofetch=$1
;;
-f|--force)
force=$1
;;
-r|--rebase)
- rebase=1
+ rebase=$1
;;
--ref-format)
case "$2" in '') usage ;; esac
@@ -320,22 +319,19 @@ cmd_update()
reference="$1"
;;
--dissociate)
- dissociate=1
+ dissociate=$1
;;
-m|--merge)
- merge=1
+ merge=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
--checkout)
- checkout=1
- ;;
- --recommend-shallow)
- recommend_shallow="--recommend-shallow"
+ checkout=$1
;;
- --no-recommend-shallow)
- recommend_shallow="--no-recommend-shallow"
+ --recommend-shallow|--no-recommend-shallow)
+ recommend_shallow=$1
;;
--depth)
case "$2" in '') usage ;; esac
@@ -343,24 +339,18 @@ cmd_update()
shift
;;
--depth=*)
- depth=$1
+ depth="$1"
;;
-j|--jobs)
case "$2" in '') usage ;; esac
jobs="--jobs=$2"
shift
;;
- -j*)
- jobs="--jobs=${1#-j}"
- ;;
- --jobs=*)
- jobs=$1
+ -j*|--jobs=*)
+ jobs="$1"
;;
- --single-branch)
- single_branch="--single-branch"
- ;;
- --no-single-branch)
- single_branch="--no-single-branch"
+ --single-branch|--no-single-branch)
+ single_branch=$1
;;
--filter)
case "$2" in '') usage ;; esac
@@ -385,22 +375,21 @@ cmd_update()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
- ${quiet:+--quiet} \
- ${force:+--force} \
- ${progress:+"--progress"} \
- ${remote:+--remote} \
- ${recursive:+--recursive} \
- ${init:+--init} \
- ${nofetch:+--no-fetch} \
- ${rebase:+--rebase} \
- ${merge:+--merge} \
- ${checkout:+--checkout} \
+ $quiet \
+ $force \
+ $progress \
+ $remote \
+ $recursive \
+ $init \
+ $nofetch \
+ $rebase \
+ $merge \
+ $checkout \
${ref_format:+"$ref_format"} \
${reference:+"$reference"} \
- ${dissociate:+"--dissociate"} \
+ $dissociate \
${depth:+"$depth"} \
- ${require_init:+--require-init} \
- ${dissociate:+"--dissociate"} \
+ $require_init \
$single_branch \
$recommend_shallow \
$jobs \
@@ -415,9 +404,6 @@ cmd_update()
# $@ = requested path
#
cmd_set_branch() {
- default=
- branch=
-
# parse $args after "submodule ... set-branch".
while test $# -ne 0
do
@@ -426,18 +412,15 @@ cmd_set_branch() {
# we don't do anything with this but we need to accept it
;;
-d|--default)
- default=1
+ default=$1
;;
-b|--branch)
case "$2" in '') usage ;; esac
- branch=$2
+ branch="--branch=$2"
shift
;;
- -b*)
- branch="${1#-b}"
- ;;
- --branch=*)
- branch="${1#--branch=}"
+ -b*|--branch=*)
+ branch="$1"
;;
--)
shift
@@ -454,9 +437,9 @@ cmd_set_branch() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch \
- ${quiet:+--quiet} \
- ${branch:+--branch "$branch"} \
- ${default:+--default} \
+ $quiet \
+ ${branch:+"$branch"} \
+ $default \
-- \
"$@"
}
@@ -472,7 +455,7 @@ cmd_set_url() {
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--)
shift
@@ -489,7 +472,7 @@ cmd_set_url() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url \
- ${quiet:+--quiet} \
+ $quiet \
-- \
"$@"
}
@@ -503,32 +486,26 @@ cmd_set_url() {
# $@ = [commit (default 'HEAD'),] requested paths (default all)
#
cmd_summary() {
- summary_limit=-1
- for_status=
-
# parse $args after "submodule ... summary".
while test $# -ne 0
do
case "$1" in
--cached)
- cached=1
+ cached=$1
;;
--files)
- files="$1"
+ files=$1
;;
--for-status)
- for_status="$1"
+ for_status=$1
;;
-n|--summary-limit)
case "$2" in '') usage ;; esac
- summary_limit="$2"
+ summary_limit="--summary-limit=$2"
shift
;;
- -n*)
- summary_limit="${1#-n}"
- ;;
- --summary-limit=*)
- summary_limit="${1#--summary-limit=}"
+ -n*|--summary-limit=*)
+ summary_limit="$1"
;;
--)
shift
@@ -545,10 +522,10 @@ cmd_summary() {
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary \
- ${files:+--files} \
- ${cached:+--cached} \
- ${for_status:+--for-status} \
- ${summary_limit:+-n "$summary_limit"} \
+ $files \
+ $cached \
+ $for_status \
+ ${summary_limit:+"$summary_limit"} \
-- \
"$@"
}
@@ -569,13 +546,13 @@ cmd_status()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--cached)
- cached=1
+ cached=$1
;;
--recursive)
- recursive=1
+ recursive=$1
;;
--)
shift
@@ -592,9 +569,9 @@ cmd_status()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status \
- ${quiet:+--quiet} \
- ${cached:+--cached} \
- ${recursive:+--recursive} \
+ $quiet \
+ $cached \
+ $recursive \
-- \
"$@"
}
@@ -611,11 +588,11 @@ cmd_sync()
do
case "$1" in
-q|--quiet)
- quiet=1
+ quiet=$1
shift
;;
--recursive)
- recursive=1
+ recursive=$1
shift
;;
--)
@@ -632,8 +609,8 @@ cmd_sync()
done
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync \
- ${quiet:+--quiet} \
- ${recursive:+--recursive} \
+ $quiet \
+ $recursive \
-- \
"$@"
}
@@ -656,10 +633,10 @@ do
command=$1
;;
-q|--quiet)
- quiet=1
+ quiet=$1
;;
--cached)
- cached=1
+ cached=$1
;;
--)
break
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 7/7] git-submodule.sh: rename some variables
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
` (5 preceding siblings ...)
2024-12-11 6:32 ` [PATCH v4 6/7] git-submodule.sh: improve variables readability Roy Eldar
@ 2024-12-11 6:32 ` Roy Eldar
6 siblings, 0 replies; 43+ messages in thread
From: Roy Eldar @ 2024-12-11 6:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Eric Sunshine,
Đoàn Trần Công Danh, Roy Eldar
Every switch and option which is passed to git-submodule.sh has a
corresponding variable which is set accordingly; by convention, the name
of the variable is the option name (for example, "--jobs" and "$jobs").
Rename "$custom_name", "$deinit_all" and "$nofetch", for consistency.
Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
git-submodule.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 6df25efc48..2999b31fad 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -40,11 +40,11 @@ init=
require_init=
files=
remote=
-nofetch=
+no_fetch=
rebase=
merge=
checkout=
-custom_name=
+name=
depth=
progress=
dissociate=
@@ -52,7 +52,7 @@ single_branch=
jobs=
recommend_shallow=
filter=
-deinit_all=
+all=
default=
summary_limit=
for_status=
@@ -108,11 +108,11 @@ cmd_add()
;;
--name)
case "$2" in '') usage ;; esac
- custom_name="--name=$2"
+ name="--name=$2"
shift
;;
--name=*)
- custom_name="$1"
+ name="$1"
;;
--depth)
case "$2" in '') usage ;; esac
@@ -149,7 +149,7 @@ cmd_add()
${reference:+"$reference"} \
${ref_format:+"$ref_format"} \
$dissociate \
- ${custom_name:+"$custom_name"} \
+ ${name:+"$name"} \
${depth:+"$depth"} \
-- \
"$@"
@@ -240,7 +240,7 @@ cmd_deinit()
quiet=$1
;;
--all)
- deinit_all=$1
+ all=$1
;;
--)
shift
@@ -259,7 +259,7 @@ cmd_deinit()
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
$quiet \
$force \
- $deinit_all \
+ $all \
-- \
"$@"
}
@@ -294,7 +294,7 @@ cmd_update()
remote=$1
;;
-N|--no-fetch)
- nofetch=$1
+ no_fetch=$1
;;
-f|--force)
force=$1
@@ -381,7 +381,7 @@ cmd_update()
$remote \
$recursive \
$init \
- $nofetch \
+ $no_fetch \
$rebase \
$merge \
$checkout \
--
2.30.2
^ permalink raw reply related [flat|nested] 43+ messages in thread
end of thread, other threads:[~2024-12-11 6:34 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-07 13:51 [PATCH 0/3] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-07 13:51 ` [PATCH 1/3] git-submodule.sh: make some variables boolean Roy Eldar
2024-12-07 23:43 ` Junio C Hamano
2024-12-08 0:06 ` Eric Sunshine
2024-12-07 13:52 ` [PATCH 2/3] git-submodule.sh: improve parsing of some long options Roy Eldar
2024-12-07 13:52 ` [PATCH 3/3] git-submodule.sh: improve parsing of short options Roy Eldar
2024-12-08 0:02 ` Junio C Hamano
2024-12-09 16:21 ` Roy E
2024-12-09 16:50 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-09 16:50 ` [PATCH v2 1/8] git-submodule.sh: make some variables boolean Roy Eldar
2024-12-09 16:50 ` [PATCH v2 2/8] git-submodule.sh: improve parsing of some long options Roy Eldar
2024-12-09 16:50 ` [PATCH v2 3/8] git-submodule.sh: improve parsing of short options Roy Eldar
2024-12-09 16:50 ` [PATCH v2 4/8] git-submodule.sh: get rid of isnumber Roy Eldar
2024-12-09 16:50 ` [PATCH v2 5/8] git-submodule.sh: get rid of unused variable Roy Eldar
2024-12-09 16:50 ` [PATCH v2 6/8] git-submodule.sh: add some comments Roy Eldar
2024-12-09 16:50 ` [PATCH v2 7/8] git-submodule.sh: improve variables readability Roy Eldar
2024-12-09 16:50 ` [PATCH v2 8/8] git-submodule.sh: rename some variables Roy Eldar
2024-12-09 23:26 ` [PATCH v2 0/8] git-submodule.sh: improve parsing of options Junio C Hamano
2024-12-10 0:50 ` Junio C Hamano
2024-12-10 18:11 ` Roy E
2024-12-11 0:02 ` Junio C Hamano
2024-12-11 6:13 ` Roy E
2024-12-11 6:16 ` Junio C Hamano
2024-12-10 18:44 ` [PATCH v3 0/7] " Roy Eldar
2024-12-10 18:44 ` [PATCH v3 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
2024-12-10 18:44 ` [PATCH v3 2/7] git-submodule.sh: improve parsing of short options Roy Eldar
2024-12-10 18:44 ` [PATCH v3 3/7] git-submodule.sh: get rid of isnumber Roy Eldar
2024-12-10 18:44 ` [PATCH v3 4/7] git-submodule.sh: get rid of unused variable Roy Eldar
2024-12-10 18:44 ` [PATCH v3 5/7] git-submodule.sh: add some comments Roy Eldar
2024-12-10 18:44 ` [PATCH v3 6/7] git-submodule.sh: improve variables readability Roy Eldar
2024-12-11 0:14 ` Junio C Hamano
2024-12-11 6:21 ` Roy E
2024-12-11 1:56 ` Đoàn Trần Công Danh
2024-12-11 6:09 ` Junio C Hamano
2024-12-10 18:44 ` [PATCH v3 7/7] git-submodule.sh: rename some variables Roy Eldar
2024-12-11 6:32 ` [PATCH v4 0/7] git-submodule.sh: improve parsing of options Roy Eldar
2024-12-11 6:32 ` [PATCH v4 1/7] git-submodule.sh: improve parsing of some long options Roy Eldar
2024-12-11 6:32 ` [PATCH v4 2/7] git-submodule.sh: improve parsing of short options Roy Eldar
2024-12-11 6:32 ` [PATCH v4 3/7] git-submodule.sh: get rid of isnumber Roy Eldar
2024-12-11 6:32 ` [PATCH v4 4/7] git-submodule.sh: get rid of unused variable Roy Eldar
2024-12-11 6:32 ` [PATCH v4 5/7] git-submodule.sh: add some comments Roy Eldar
2024-12-11 6:32 ` [PATCH v4 6/7] git-submodule.sh: improve variables readability Roy Eldar
2024-12-11 6:32 ` [PATCH v4 7/7] git-submodule.sh: rename some variables Roy Eldar
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 NNTP newsgroup(s).