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@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.
>
Thanks for your suggestion, but I think I'll stick to what I have for now :)
Philippe.
