Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Taylor Blau <me@ttaylorr.com>]

On Fri, Apr 06, 2018 at 03:40:56AM -0400, Eric Sunshine wrote:
> On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
> >> +test_expect_success 'uses entry when available' '
> >> +       echo bar >expect &&
> >> +       git config --add core.foo bar &&
> >> +       git config --default baz core.foo >actual &&
> >> +       git config --unset core.foo &&
> >> +       test_cmp expect actual
> >> +'
> >
> > If you happen to re-roll, can we move this test so it immediately
> > follows the "uses --default when missing entry" test?
>
> One other issue. If "git config --default ..." fails, the --unset line
> will never be invoked, thus cleanup won't happen.
>
> To ensure cleanup, either use:
>
>     test_when_finished "git config --unset core.foo" &&
>
> earlier in the test (just after the --add line) or use the
> test_config() function to both set the value and ensure its cleanup.

Neat. I wasn't aware of 'test_when_finished'. I think that I prefer the
explicitness of 'test_when_finished "git config ..."' over
'test_config()', but I am happy to use whichever is preferred. Since
t1310 is new, there's no historical precedent to follow. Please let me
know if you have a preference one way or another.

Thanks,
Taylor