Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 21, 2020, 20:08 UTC
Message-ID: <CAPig+cRqa9Y4mEdktdP3d2+PHWanKZ6q6tXfJXEAW9sqcVwHOw@mail.gmail.com>
In-Reply-To: <xmqqa6u7m1bu.fsf@gitster.c.googlers.com>

On Mon, Dec 21, 2020 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Finally, as this is a one-off case, it might be simpler just to drop
> > this patch altogether and open-code the cleanup in the test itself in
> > patch [2/3] rather than bothering with test_config() in that
> > particular case. For example:
> >
> >     test_when_finished "test_unconfig -C two remote.one.push" &&
> >     git config -C two --add remote.one.push : &&
> >     test_must_fail git -C two push one &&
> >     git config -C two --add remote.one.push ^refs/heads/master &&
> >     git -C two push one
>
> That would be my preference, too.  Thanks for carefully and
> patiently reviewing.

I forgot to mention that it likely would be a good idea to at least mention in the commit message why test_config() is not being used for that particular case. Perhaps saying something along the lines of "one test handles config cleanup manually since test_config() is not prepared to take arbitrary options such as --add" -- or something along those lines -- would be sufficient. Alternatively, an in-code comment within the test explaining the open-coding might be more helpful to people reading the code in the future.