Re: [PATCH v3 15/20] init: allow alternate backends to be set for new repos

[David Turner <dturner@twopensource.com>]

On Fri, 2016-01-15 at 13:51 +0100, Thomas Gummerer wrote:
> On 01/14, David Turner wrote:
> > +	if (requested_ref_storage_backend)
> > +		ref_storage_backend =
> > requested_ref_storage_backend;
> > +	if (strcmp(ref_storage_backend, "files")) {
> > +		git_config_set("extensions.refStorage",
> > ref_storage_backend);
> > +		git_config_set("core.repositoryformatversion",
> > ref_storage_backend);
> > +#ifdef USE_LIBLMDB
> > +		register_ref_storage_backend(&refs_be_lmdb);
> 
> refs_be_lmdb is not yet defined at this point in the patch series. 
>  It
> doesn't break anything, because USE_LIBLMDB doesn't leak through the
> makefile yet, but I still think it would make more sense to have the
> ifdef in 19/20.

Thanks, fixed.

> > +#endif
> > +		set_ref_storage_backend(ref_storage_backend);
> 
> More importantly, there is no check whether setting the ref storage
> backend succeeds.  If a user accidentally sets an invalid value for
> the ref backend, a broken repository will be created without even
> warning the user.
> 
> While the repository will still work fine at this point in the
> series,
> git will die with an invalid ref backend in the config after 19/20.
> It can be fixed by setting extensions.refStorage to files in the
> config, because the backend is initialized to the default files
> backend below when the ref backend cannot be set.
> 
> I think it would be best to die() here, if setting the ref backend
> doesn't succeed, and clean up the files that were already written,
> instead of leaving the user with a broken repository.

I've rearranged this code. 

> > +test_expect_success 'init with bogus storage backend fails' '
> > +
> > +	(
> > +		mkdir again2 &&
> > +		cd again2 &&
> > +		git init --ref-storage=test >out2 2>err2
> > +	)
> > +'
> 
> I noticed the above mainly because of this test, which doesn't seem
> to
> test what it claims to test.  It only seems to test that git init
> succeeds, and writes something to out2 and err2, it's missing the
> checks that the contents are actually what they're supposed to be.

Fixed, thanks.