Re: [PATCH v3 19/20] refs: add LMDB refs backend

From: David Turner <dturner@twopensource.com>
Date: Jan 19, 2016, 18:55 UTC
Message-ID: <1453229733.16226.33.camel@twopensource.com>
In-Reply-To: <20160115133317.GJ10612@hank>

On Fri, 2016-01-15 at 14:33 +0100, Thomas Gummerer wrote:

> [I don't know too much about the refs code, but was still interested
> in this patch series, so below are the few things that I noticed
> while
> reading through it.]

Thanks for taking a look.

> On 01/14, David Turner wrote:

Show 5 quoted lines

> > +exists.  The header has the same format as an ordinary reflog, but
> > with
> > +a timeztamp of all zeros and an empty value.
> 
> s/timeztamp/timestamp/

Fixed, thanks.

Show 9 quoted lines

> > +test "$(git config extensions.refstorage)" = "lmdb" && die "git
> > -new-workdir is incompatible with the refs lmdb storage"
> > +
> 
> Is it expected that other potential ref backends are compatible with
> git-new-workdir?  Otherwise I think it would make more sense to
> whitelist the files backend here instead of blacklisting the lmdb
> backend, so we don't risk forgetting about this when adding another
> backend.

I no longer remember what the exact issue was, but we can be safe and restrict git-new-workdir to the files backend (since it looks likely that worktrees will replace it anyway).

Show 25 quoted lines

> >  case "$git_dir" in
> >  .git)
> >  	git_dir="$orig_git/.git"
> 
> [...]
> 
> > +static int lmdb_init_db(struct strbuf *err, int shared)
> > +{
> > +	/*
> > +	 * To create a db, all we need to do is make a directory
> > for
> > +	 * it to live in; lmdb will do the rest.
> > +	 */
> > +
> > +	if (!db_path)
> > +		db_path =
> > xstrdup(real_path(get_refdb_path(get_git_common_dir())));
> 
> I think we're leaking some memory from get_refdb_path() here.
> get_refdb_path() uses strbuf_detach(), which according to its
> docstring
> makes its caller take care of the memory of the returned string.
> real_path() then uses strbuf_addstr() to add the string to its
> internal strbuf, but leaves the string we get from get_refdb_path()
> alone, so it leaks.

That only happens once per run, since db_path is set afterwards, but I'll change get_refdb_path() to return a pointer to a statically allocated string (as many similar funcs do). That will be easier to read.