Re: [PATCH 15/16] refs: add LMDB refs backend

[David Turner <dturner@twopensource.com>]

On Wed, 2015-12-23 at 15:32 +0100, Michael Haggerty wrote:
> +core.refsBackendType::
> > +	Type of refs backend. Default is to use the original files
> > +	based backend. Set to 'lmdb' to activate the lmdb database
> > +	backend.  If you use the lmdb backend,
> > +	core.repositoryFormatVersion must be set to 1, and
> > +	extensions.refBackend must be set to 'lmdb'.
> 
> This phrasing makes it sound like I can go into an existing
> repository,
> change this repository setting, and *presto* I will have a LMDB
> -backed
> repository. I suggest rewording it more along the lines of "this
> setting
> reflects the refs backend that is currently in use".
> 
> Also please see my earlier question about whether users should see
> the
> term "backend" or whether other terms would be easier to understand.

Adjusted, thanks.

> > +All per-worktree refs (refs/bisect/* and HEAD) are store using
> 
> s/store/stored/

Fixed.

> > +the traditional files-based backend.
> > +
> > +Reflogs are stored as a series of database entries.
> > +
> > +For non-empty reflogs, there is one entry per logged ref
> > +update.  The key format is logs/[refname]\0[timestamp].  The
> > timestamp
> > +is a 64-bit unsigned integer number of nanoseconds since 1/1/1970.
> > +This means that reflog entries are chronologically ordered. 
> >  Because
> > +LMDB is a btree database, we can efficiently iterate over these
> > keys.
> 
> Is there a guarantee that the reflog entries for all references
> updated
> in a single transaction have the same timestamp? Is there a guarantee
> that updates that happened in *different* transactions have different
> timestamps? These might be useful properties if they are easy to
> implement, because then one could deduce the scope of transactions
> from
> the reflog. But of course, they admittedly go beyond what the files
> backend offers and so are not a requirement.

All of this stuff is handled outside of the storage backend, so I don't
want to address it here.  That is, this offers the same guarantees as
the files-based backend (neither of those, I think).

> Is the timestamp in ASCII base 10, binary format, or or something
> else?

I've added the words "network byte order" to make it clear that this is
binary.

> > +For an empty reflog, there is a "header" entry to show that a
> > reflog
> > +exists.  The header has the same format as an ordinary reflog, but
> > with
> > +a timeztamp of all zeros and an empty value.
> > +
> > +Reflog values are in the same format as the original files-based
> > +reflog.
> 
> I assume that means that each entry has the same contents as one line
> from a reflog file. Does the value include a trailing LF? (From above
> I
> guess it does have a trailing NUL.)

Trailing LF, yes; will fix.

> Is the timestamp within the reflog entry guaranteed to agree with the
> timestamp in the key? This might be a convenient property for
> debugging.

Yes; edited.
diff --git a/Makefile b/Makefile

> > index 5bd68e0..77b96d9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1037,6 +1037,17 @@ ifdef USE_LIBPCRE
> >  	EXTLIBS += -lpcre
> >  endif
> >  
> > +ifdef USE_LIBLMDB
> > +	BASIC_CFLAGS += -DUSE_LIBLMDB
> > +	ifdef LIBLMDBDIR
> > +		BASIC_CFLAGS += -I$(LIBLMDBDIR)/include
> > +		EXTLIBS += -L$(LIBLMDBDIR)/$(lib)
> > $(CC_LD_DYNPATH)$(LIBLMDBDIR)/$(lib)
> > +	endif
> > +	EXTLIBS += -llmdb
> > +	LIB_OBJS += refs/lmdb-backend.o
> > +	TEST_PROGRAMS_NEED_X += test-refs-lmdb-backend
> > +endif
> > +
> >  ifdef HAVE_ALLOCA_H
> >  	BASIC_CFLAGS += -DHAVE_ALLOCA_H
> >  endif
> > @@ -2124,6 +2135,7 @@ GIT-BUILD-OPTIONS: FORCE
> >  	@echo NO_CURL=\''$(subst ','\'',$(subst
> > ','\'',$(NO_CURL)))'\' >>$@+
> >  	@echo NO_EXPAT=\''$(subst ','\'',$(subst
> > ','\'',$(NO_EXPAT)))'\' >>$@+
> >  	@echo USE_LIBPCRE=\''$(subst ','\'',$(subst
> > ','\'',$(USE_LIBPCRE)))'\' >>$@+
> > +	@echo USE_LIBLMDB=\''$(subst ','\'',$(subst
> > ','\'',$(USE_LIBLMDB)))'\' >>$@+
> >  	@echo NO_PERL=\''$(subst ','\'',$(subst
> > ','\'',$(NO_PERL)))'\' >>$@+
> >  	@echo NO_PYTHON=\''$(subst ','\'',$(subst
> > ','\'',$(NO_PYTHON)))'\' >>$@+
> >  	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
> > ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
> > diff --git a/builtin/init-db.c b/builtin/init-db.c
> > index 44db591..1eb2feb 100644
> > --- a/builtin/init-db.c
> > +++ b/builtin/init-db.c
> > @@ -179,6 +179,7 @@ static int create_default_files(const char
> > *template_path)
> >  	int reinit;
> >  	int filemode;
> >  	struct strbuf err = STRBUF_INIT;
> > +	int repo_version = 0;
> >  
> >  	/* Just look for `init.templatedir` */
> >  	git_config(git_init_db_config, NULL);
> > @@ -209,7 +210,14 @@ static int create_default_files(const char
> > *template_path)
> >  		git_config_set("core.refsBackendType",
> > refs_backend_type);
> >  		config_data.refs_backend_type = refs_backend_type;
> >  		config_data.refs_base = get_git_dir();
> > +#ifdef USE_LIBLMDB
> > +		register_refs_backend(&refs_be_lmdb);
> > +#endif
> >  		set_refs_backend(refs_backend_type, &config_data);
> > +		if (!strcmp(refs_backend_type, "lmdb")) {
> 
> Wouldn't expressing this condition as
> 
>     if (strcmp(refs_backend_type, "files"))
> 
> better reflect the reason that we might need to set
> "extensions.refBackend" and use repo_version "1"?

Yeah.

>  (You might also need a
> "refs_backend_type &&" and/or a "*refs_backend_type &&" in there.)

I've just decided to set this var to be "files" by default, to simplify
a lot of this code.

> >   *  - either an objects/ directory _or_ the proper
> >   *    GIT_OBJECT_DIRECTORY environment variable
> > - *  - a refs/ directory
> > - *  - either a HEAD symlink or a HEAD file that is formatted as
> > - *    a proper "ref:", or a regular file HEAD that has a properly
> > - *    formatted sha1 object name.
> > + *  - a refdb/ directory or
> > + *    - a refs/ directory
> > + *    - either a HEAD symlink or a HEAD file that is formatted as
> > + *      a proper "ref:", or a regular file HEAD that has a
> > properly
> > + *      formatted sha1 object name.
> 
> The indentation looks wrong here. I think the requirement is
> *  - either a refdb/ directory or a refs/ directory
> *  - either a HEAD symlink or a HEAD file that is formatted as
> *    a proper "ref:", or a regular file HEAD that has a properly
> *    formatted sha1 object name.
> 
> though the test of the *contents* of HEAD is currently skipped for
> non-files backends.

Yes.

Thanks for the review. Hopefully another version by EOD, but we'll see.