On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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.
