| Message ID | 20241207135201.2536-4-royeldar0@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-submodule.sh: improve parsing of options | expand |
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.
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!
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
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(+)