git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
David Turner <dturner@twopensource.com>
Date
Feb 18, 2016, 20:23 UTC
Message-ID
<1455827001.7528.87.camel@twopensource.com>
In-Reply-To
<20160218085023.GA30049@lanh>
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);
Show 22 quoted lines
> > +	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
Show 16 quoted lines
> > +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.

Show 23 quoted lines
> > +{
> > +	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
Show 12 quoted lines
> > +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.

Show 13 quoted lines
> > +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.
Show 16 quoted lines
> > +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.
Show 12 quoted lines
> > +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.
Show 15 quoted lines
> > +{
> > +	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.
Show 33 quoted lines
> > 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.
Show 17 quoted lines
> > @@ -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.

Previous: Duy NguyenNext: Junio C Hamano
Message 32 of 52 in “refs backends”
  1. 00/27 refs backendsDavid Turner, Feb 18, 2016
  2. 01/27 refs: Move head_ref{,_submodule} to the common codeDavid Turner, Feb 18, 2016
  3. 02/27 refs: move for_each_*ref* functions into common codeDavid Turner, Feb 18, 2016
  4. 03/27 refs: add a backend method structure with transaction functionsDavid Turner, Feb 18, 2016
  5. 04/27 refs: add methods for misc ref operationsDavid Turner, Feb 18, 2016
  6. 05/27 refs: add method for do_for_each_refDavid Turner, Feb 18, 2016
  7. 06/27 refs: add do_for_each_per_worktree_refDavid Turner, Feb 18, 2016
  8. 07/27 refs: add methods for reflogDavid Turner, Feb 18, 2016
  9. 08/27 refs: add method for initial ref transaction commitDavid Turner, Feb 18, 2016
  10. 09/27 refs: add method for delete_refsDavid Turner, Feb 18, 2016
  11. 10/27 refs: add methods to init refs dbDavid Turner, Feb 18, 2016
  12. 11/27 refs: add method to rename refsDavid Turner, Feb 18, 2016
  13. 12/27 refs: forbid cross-backend ref renamesDavid Turner, Feb 18, 2016
  14. Duy NguyenFeb 20, 2016
  15. David TurnerFeb 24, 2016
  16. 13/27 refs: make lock genericDavid Turner, Feb 18, 2016
  17. 14/27 refs: move duplicate check to common codeDavid Turner, Feb 18, 2016
  18. 15/27 refs: allow log-only updatesDavid Turner, Feb 18, 2016
  19. 16/27 refs: don't dereference on renameDavid Turner, Feb 18, 2016
  20. 17/27 refs: on symref reflog expire, lock symref not referrentDavid Turner, Feb 18, 2016
  21. 18/27 refs: resolve symbolic refs firstDavid Turner, Feb 18, 2016
  22. 19/27 refs: always handle non-normal refs in files backendDavid Turner, Feb 18, 2016
  23. 20/27 init: allow alternate ref strorage to be set for new reposDavid Turner, Feb 18, 2016
  24. 21/27 refs: check submodules' ref storage configDavid Turner, Feb 18, 2016
  25. 22/27 clone: allow ref storage backend to be set for cloneDavid Turner, Feb 18, 2016
  26. 23/27 svn: learn ref-storage argumentDavid Turner, Feb 18, 2016
  27. Eric WongFeb 20, 2016
  28. David TurnerFeb 23, 2016
  29. 24/27 refs: add register_ref_storage_backends()David Turner, Feb 18, 2016
  30. 25/27 refs: add LMDB refs storage backendDavid Turner, Feb 18, 2016
  31. Duy NguyenFeb 18, 2016
  32. David TurnerFeb 18, 2016
  33. Junio C HamanoFeb 18, 2016
  34. Duy NguyenFeb 19, 2016
  35. David TurnerFeb 19, 2016
  36. Duy NguyenFeb 20, 2016
  37. David TurnerFeb 24, 2016
  38. Junio C HamanoFeb 20, 2016
  39. David TurnerFeb 19, 2016
  40. Junio C HamanoFeb 19, 2016
  41. Duy NguyenFeb 20, 2016
  42. David TurnerFeb 24, 2016
  43. Duy NguyenFeb 25, 2016
  44. Duy NguyenFeb 20, 2016
  45. David TurnerFeb 24, 2016
  46. Duy NguyenFeb 25, 2016
  47. refs: document transaction semanticsDavid Turner, Feb 25, 2016
  48. David TurnerFeb 25, 2016
  49. Junio C HamanoFeb 25, 2016
  50. David TurnerFeb 25, 2016
  51. 26/27 refs: tests for lmdb backendDavid Turner, Feb 18, 2016
  52. 27/27 tests: add ref-storage argumentDavid Turner, Feb 18, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.