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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html