Re: [PATCH v2 3/5] tests: only automatically unset matching values from test_config

From: Jacob Keller <jacob.keller@gmail.com>
Date: Jun 16, 2022, 22:07 UTC
Message-ID: <CA+P7+xomvVK4ZvQfepbVCCgE8uXZvueZQudEa07pvWQYrU=hJQ@mail.gmail.com>
In-Reply-To: <xmqq35g4s3xl.fsf@gitster.g>

On Thu, Jun 16, 2022 at 2:18 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > +     # Only enable --fixed-value if we have two parameters
> > +     if test $# < 2
> > +     then
> > +             fixedvalue=
> > +     fi
>
> Two comments:
>
>  * Does "<" do what you expect to do?  Did you mean "-lt"?
>
>  * Using "bug in the test script: $*" and diagnosing missing
>    parameters, instead of silently ignoring the option the developer
>    wrote, would be more preferrable.

I guess we should check that it has exactly 1 or 2 arguments, yea.

Show 18 quoted lines

>
> > +     git ${config_dir:+-C "$config_dir"} config ${global:+--global} ${fixedvalue:+--fixed-value} --unset-all "$1" "$2"
> >       config_status=$?
> >       case "$config_status" in
> >       5) # ok, nothing to unset
> > @@ -575,7 +586,7 @@ test_config () {
> >               esac
> >               shift
> >       done
> > -     test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} ${global:+--global} '$1'" &&
> > +     test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} --fixed-value ${global:+--global} '$1' '$2'" &&
>
> Why are $1 and $2 enclosed in a pair of single quotes?  Is the
> assumption that they do not contain a single quote themselves?
>
> I guess that is true also for config_dir and shares the same
> problem, so you are not introducing a new problem.
>

Yea, I wasn't sure.

> >       git ${config_dir:+-C "$config_dir"} config ${global:+--global} "$1" "$2"
> >  }