Re: [PATCH v5 25/27] refs: add LMDB refs storage backend

[David Turner <dturner@twopensource.com>]

On Thu, 2016-02-18 at 15:50 +0700, Duy Nguyen wrote:

[snip]

Thanks; applied the above

> This permission makes me wonder if we need adjust_shared_perm() here
> and some other places.

So just add this after every mkdir?

	if (shared_repository)
		adjust_shared_perm(db_path);

> > +	if (ret)
> > +		die("BUG: mdb_env_open (%s) failed: %s", path,
> > +		    mdb_strerror(ret));
> > +}
> > +
> > +static int lmdb_init_db(int shared, struct strbuf *err)
> > +{
> > +	/*
> > +	 * 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(git_path("refs.lmdb")));
> > +
> > +	if (mkdir(db_path, 0775) && errno != EEXIST) {
> > +		strbuf_addf(err, "%s", strerror(errno));
> 
> maybe strbuf_addstr, unless want to add something more, "mkdir
> failed"?

added more

> > +static int read_per_worktree_ref(const char *submodule, const char
> > *refname,
> > +				 struct MDB_val *val, int
> > *needs_free)
> 
> From what I read, I suspect these _per_worktree functions will be
> identical for the next backend. Should we just hand over the job for
> files backend? For all entry points that may deal with per-worktree
> refs, e.g. lmdb_resolve_ref_unsafe, can we check ref_type() first
> thing, if it's per-worktree we call
> refs_be_files.resolve_ref_unsafe()
> instead?  It could even be done at frontend level,
> e.g. refs.c:resolve_ref_unsafe().
> 
> Though I may be talking rubbish here because I don't know how whether
> it has anything to do with transactions.

The reason I did it this way is that some ref chains cross backend
boundaries (e.g. HEAD -> refs/heads/master).  But if we have other
backends later, we could generalize.

> > +{
> > +	struct strbuf sb = STRBUF_INIT;
> > +	struct strbuf path = STRBUF_INIT;
> > +	struct stat st;
> > +	int ret = -1;
> > +
> > +	submodule_path(&path, submodule, refname);
> > +
> > +#ifndef NO_SYMLINK_HEAD
> 
> It started with the compiler warns about unused "st" when this macro
> is defined. Which makes me wonder, should we do something like this
> to
> make sure this code compiles unconditionally?
> 
> +#ifndef NO_SYMLINK_HEAD
> +       int no_symlink_head = 0;
> +#else
> +       int no_symlink_head = 1;
> +#endif
> ...
> +       if (!no_symlink_head) {
> ...

OK.

> > +int lmdb_transaction_begin_flags(struct strbuf *err, unsigned int
> > flags)
> 
> static?

yep

> > +static const char *parse_ref_data(struct lmdb_transaction
> > *transaction,
> > +				  const char *refname, const char
> > *ref_data,
> > +				  unsigned char *sha1, int
> > resolve_flags,
> > +				  int *flags, int bad_name)
> > +{
>[snip]
> This code looks a lot like near the end of resolve_ref_1(). Maybe we
> could share the code in refs/backend-common.c or something and call
> here instead?

When I wrote this, I couldn't find a straightforward way to factor out
the commonalities, but I'll try again now that I understand the refs
code better.

> > +static int show_one_reflog_ent(struct strbuf *sb,
> > each_reflog_ent_fn fn, void *cb_data)
> > +{
> > +	unsigned char osha1[20], nsha1[20];
> > +	char *email_end, *message;
> > +	unsigned long timestamp;
> > +	int tz;
> > +
> > +	/* old (raw sha) new (raw sha) name <email> SP time TAB
> > msg LF */
> 
> Hmm.. since you're going with raw sha-1, this is clearly not a text
> string anymore, any reason to still keep LF at the end?

IIRC, some of the common funcs depend on this.

> > +static int lmdb_delete_reflog(const char *refname)
> > +{
> > +	MDB_val key, val;
> > +	char *log_path;
> > +	int len;
> > +	MDB_cursor *cursor;
> > +	int ret = 0;
> > +	int mdb_ret;
> > +	struct strbuf err = STRBUF_INIT;
> > +	int in_transaction;
> > +
> > +	if (ref_type(refname) != REF_TYPE_NORMAL)
> > +		return refs_be_files.delete_reflog(refname);
> 
> Yay.. delegating work to files backend. I still think doing this in
> refs.c:delete_reflog() may be a good idea.

Yes, I agree.

> > +int lmdb_reflog_expire(const char *refname, const unsigned char
> > *sha1,
> 
> static?

Yep.

> > +static int lmdb_create_symref(const char *ref_target,
> > +			      const char *refs_heads_master,
> > +			      const char *logmsg)
> > +{
> > +
> ...
> > +	mdb_put_or_die(&transaction, &key, &val, 0);
> > +
> > +	/* TODO: Don't create ref d/f conflicts */
> 
> I'm not sure I get this comment. D/F conflicts are no longer a thing
> for lmdb backend, right?

I'm trying to avoid the lmdb backend creating a set of refs that the
files backend can't handle.  This would make collaboration with other
versions of git more difficult.

> > +MDB_env *submodule_txn_begin(struct lmdb_transaction *transaction)
> 
> static?

Yes.

> > +{
> > +	int ret;
> > +	MDB_env *submodule_env = NULL;
> > +	struct strbuf path = STRBUF_INIT;
> > +
> > +	strbuf_git_path_submodule(&path, transaction->submodule,
> > "refs.lmdb");
> > +
> > +	if (!is_directory(path.buf))
> > +		goto done;
> > +
> > +	mkdir(path.buf, 0775);
> 
> A few other places where mkdir() is called, we may need to
> adjust_shared_perm().

OK.

> > diff --git a/setup.c b/setup.c
> > index 1a62277..00625ab 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -279,7 +279,7 @@ int ref_storage_backend_config(const char *var,
> > const char *value, void *ptr)
> >   *
> >   *  - either an objects/ directory _or_ the proper
> >   *    GIT_OBJECT_DIRECTORY environment variable
> > - *  - a refs/ directory
> > + *  - a refs.lmdb/ 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.
> > @@ -313,8 +313,13 @@ int is_git_directory(const char *suspect)
> >  
> >  	strbuf_setlen(&path, len);
> >  	strbuf_addstr(&path, "/refs");
> > -	if (access(path.buf, X_OK))
> > -		goto done;
> > +
> > +	if (access(path.buf, X_OK)) {
> > +		strbuf_setlen(&path, len);
> > +		strbuf_addstr(&path, "/refs.lmdb");
> > +		if (access(path.buf, X_OK))
> > +			goto done;
> > +	}
> > 
> 
> I think it's ok leaving this function unmodified, which means "refs"
> directory will always be there and "refs.lmdb" does not matter. If
> somehow "refs" is deleted, old binaries get confused anyway so we
> can't delete it.

OK.

> > @@ -1089,8 +1089,11 @@ static int refs_from_alternate_cb(struct
> > alternate_object_database *e,
> >  		goto out;
> >  	/* Is this a git repository with refs? */
> >  	memcpy(other + len - 8, "/refs", 6);
> > -	if (!is_directory(other))
> > -		goto out;
> > +	if (!is_directory(other)) {
> > +		memcpy(other + len - 8, "/refs.lmdb", 11);
> > +		if (!is_directory(other))
> > +			goto out;
> > +	}
> 
> and probably the same here. I have no idea what this code does
> though,
> but if it's about detecting git directory, it should call
> is_git_directory() instead.

I'll back out my change, but not do the the is_git_directory thing
since I don't have a strong sense of why this code is the way it is.