Subject: Re: [PATCH v2 12/12] test-lib: split up and deprecate test_create_repo()

From: SZEDER Gábor <[email-masked]>
Date: Tue, 20 Apr 2021 23:27:31 +0200

On Sat, Apr 17, 2021 at 11:45:41PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >>  5. Since we don't need to move the .git/hooks directory
> >
> > Since we don't change directory anymore...
> >
> >> we don't need
> >>     the subshell here either.
> >> 
> >>     That wasn't really needed for the .git/hooks either, but was being
> >>     done for the convenience of not having to quote the path to the
> >>     repository as we moved the hooks.
> >
> > And then this dubious explanation will not be necessary.
> 
> Why dubious? That's why we had the subshell-ing. See 0d314ce834
> (test-lib: use subshell instead of cd $new && .. && cd $old,
> 2010-08-30).

That commit doesn't mention hooks at all, so I'm not sure what I
should see there.  OTOH, you did specifically write "use a subshell
instead of keeping track of the old working directory and cd-ing back
when it's done"...  which is in line with our best practices on how to
change directories in our tests in order to reliably change back even
in case of an error.

And that cd-ing back and forth was not added because of the hooks
either, but because back in the day when 'test_create_repo' was
created 'git init-db' could only create a new repository in the
current directory.

> I don't mind rewording or not including some of this verbosity per-se,
> but I wonder why you're honing in on the subshell part in
> particular. Maybe there's something I'm missing...
> 
> The goal of the commit messsage is to point-by-point tear apart facets
> of the previous behavior,

Agreed.

> and assure the reader that e.g. the
> subshelling isn't needed for some other subtle reason.

The role of moving the hooks with respect to the subshell is not a
subtlety, it is simply irrelevant.  And while you are so focused on
this irrevelancy you keep forgetting to mention the factor that
actually does matter: that we won't change directories anymore.