On 2021-03-08 at 16:43:58, Eric Sunshine wrote:
> On Mon, Mar 8, 2021 at 8:18 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > In 552955ed7f ("clone: use more conventional config/option layering",
> > 2020-10-01), clone learned to read configuration options earlier in its
> > execution, before creating the new repository.  However, that led to a
> > problem: if the core.bare setting is set to false in the global config,
> > cloning a bare repository segfaults.  This happens because the
> > repository is falsely thought to be non-bare, but clone has set the work
> > tree to NULL, which is then dereferenced.
> > [...]
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> 
> Perhaps this deserves a:
> 
>     Reported-by: Joseph Vusich <jvusich@amazon.com>

Good point.  Will fix.

> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > @@ -104,6 +104,13 @@ test_expect_success 'redirected clone -v does show progress' '
> > +test_expect_success 'clone does not segfault with --bare and core.bare=false' '
> > +       test_config_global core.bare false &&
> > +       git clone --bare "file://$(pwd)/parent" clone-bare &&
> 
> Can this be done more simply as:
> 
>     git clone --bare parent clone-bare &&
> 
> or even:
> 
>     git clone --bare . clone-bare &&
> 
> without mucking about with $(pwd)?

Sure.  I pulled the line from the test above, but I agree that's nicer.

> > +       git -C clone-bare rev-parse --is-bare-repository >is-bare &&
> > +       test "$(cat is-bare)" = true
> 
> These days, we'd probably say:
> 
>     echo true >expect &&
>     git -C clone-bare rev-parse --is-bare-repository >actual &&
>     test_cmp expect actual
> 
> but it's subjective and minor; not at all worth a re-roll.

There's enough nits to warrant a v2, so I can do one.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US