| Message ID | c890f55f59994231be6114f76f020510eb824453.1666365220.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2e94339fdc3122fd2f92e865105523bc3866a65f |
| Headers | show |
| Series | subtree: fix split and merge after annotated tag was squash-merged | expand |
On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote: > From: Philippe Blain <levraiphilippeblain@gmail.com> > > 9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27) > added validation code to check that options given to 'git subtree <cmd>' > made sense with the command being used. > > Refactor these checks by adding a 'die_incompatible_opt' function to > reduce code duplication. This looks good, but... > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > --- > contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh > index 49ef493ef92..f5eab198c80 100755 > --- a/contrib/subtree/git-subtree.sh > +++ b/contrib/subtree/git-subtree.sh > @@ -102,6 +102,14 @@ assert () { > fi > } > > +# Usage: die_incompatible_opt OPTION COMMAND > +die_incompatible_opt () { > + assert test "$#" = 2 > + opt="$1" > + arg_command="$2" > + die "The '$opt' flag does not make sense with 'git subtree $arg_command'." > +} > + > main () { > if test $# -eq 0 > then > @@ -176,16 +184,16 @@ main () { > arg_debug=1 > ;; > --annotate) > - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." > + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" > arg_split_annotate="$1" > shift > ;; Since this is all in this form I wonder why not (maybe adding some "local" and/or "&&" too while at it): die_if_other_opt { assert test "$#" = 3 other="$1" shift if test -z "$other" then return fi [...the rest of your version] } Then: > --no-annotate) > - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." > + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" Instead of this: die_if_other_opt "$allow_split" "$opt" "$arg_command" Or actually just: die_if_other_opt "$allow_split" Maybe you disagree, but since the function will see the variables & this is purely a helper for this getopts parse loop I think it's fine just to assume we can read "$opt" and "$arg_command" there..., so, urm, maybe just: test -n "$allow_split" || die_incompatible_opt ? :) Anyway, an easy bike shed, you should go for whatever variant you prefer :) Thanks for indulging me.
Hi Ævar, Le 2022-10-21 à 12:22, Ævar Arnfjörð Bjarmason a écrit : > > On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote: > >> From: Philippe Blain <levraiphilippeblain@gmail.com> >> >> 9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27) >> added validation code to check that options given to 'git subtree <cmd>' >> made sense with the command being used. >> >> Refactor these checks by adding a 'die_incompatible_opt' function to >> reduce code duplication. > > This looks good, but... > >> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> >> --- >> contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------ >> 1 file changed, 20 insertions(+), 12 deletions(-) >> >> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh >> index 49ef493ef92..f5eab198c80 100755 >> --- a/contrib/subtree/git-subtree.sh >> +++ b/contrib/subtree/git-subtree.sh >> @@ -102,6 +102,14 @@ assert () { >> fi >> } >> >> +# Usage: die_incompatible_opt OPTION COMMAND >> +die_incompatible_opt () { >> + assert test "$#" = 2 >> + opt="$1" >> + arg_command="$2" >> + die "The '$opt' flag does not make sense with 'git subtree $arg_command'." >> +} >> + >> main () { >> if test $# -eq 0 >> then >> @@ -176,16 +184,16 @@ main () { >> arg_debug=1 >> ;; >> --annotate) >> - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." >> + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" >> arg_split_annotate="$1" >> shift >> ;; > > Since this is all in this form I wonder why not (maybe adding some "local" and/or "&&" too while at it): > > die_if_other_opt { > assert test "$#" = 3 > other="$1" > shift > if test -z "$other" > then > return > fi > [...the rest of your version] > } > > Then: > >> --no-annotate) >> - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." >> + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" > > Instead of this: > > die_if_other_opt "$allow_split" "$opt" "$arg_command" > > Or actually just: > > die_if_other_opt "$allow_split" > > Maybe you disagree, but since the function will see the variables & this > is purely a helper for this getopts parse loop I think it's fine just to > assume we can read "$opt" and "$arg_command" there..., so, urm, maybe just: > > test -n "$allow_split" || die_incompatible_opt > > ? :) Anyway, an easy bike shed, you should go for whatever variant you > prefer :) Thanks for indulging me. > Thanks for your suggestion, but I think I'll stick to what I have for now :) Philippe.
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 49ef493ef92..f5eab198c80 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -102,6 +102,14 @@ assert () { fi } +# Usage: die_incompatible_opt OPTION COMMAND +die_incompatible_opt () { + assert test "$#" = 2 + opt="$1" + arg_command="$2" + die "The '$opt' flag does not make sense with 'git subtree $arg_command'." +} + main () { if test $# -eq 0 then @@ -176,16 +184,16 @@ main () { arg_debug=1 ;; --annotate) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_annotate="$1" shift ;; --no-annotate) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_annotate= ;; -b) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_branch="$1" shift ;; @@ -194,7 +202,7 @@ main () { shift ;; -m) - test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command" arg_addmerge_message="$1" shift ;; @@ -202,34 +210,34 @@ main () { arg_prefix= ;; --onto) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_onto="$1" shift ;; --no-onto) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_onto= ;; --rejoin) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" ;; --no-rejoin) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" ;; --ignore-joins) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_ignore_joins=1 ;; --no-ignore-joins) - test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command" arg_split_ignore_joins= ;; --squash) - test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command" arg_addmerge_squash=1 ;; --no-squash) - test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'." + test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command" arg_addmerge_squash= ;; --)