| Message ID | 20241209165009.40653-1-royeldar0@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | git-submodule.sh: improve parsing of options | expand |
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(-)
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.
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.
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"}
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.
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.