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

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