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

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);

quoted

+	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

quoted

+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.

quoted

+{
+	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.

quoted

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

static?

yep

quoted

+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.

quoted

+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.

quoted

+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.

quoted

+int lmdb_reflog_expire(const char *refname, const unsigned char
*sha1,

static?

Yep.

quoted

+static int lmdb_create_symref(const char *ref_target,
+			      const char *refs_heads_master,
+			      const char *logmsg)
+{
+

...

quoted

+	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.

quoted

+MDB_env *submodule_txn_begin(struct lmdb_transaction *transaction)

static?

Yes.

quoted

+{
+	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.

quoted

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.

quoted

@@ -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.