Re: [PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 21, 2022, 16:22 UTC
Message-ID: <221021.86v8oddtuj.gmgdl@evledraar.gmail.com>
In-Reply-To: <c890f55f59994231be6114f76f020510eb824453.1666365220.git.gitgitgadget@gmail.com>

On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:

Show 8 quoted lines

> 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...

Show 33 quoted lines

> 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.