[PATCH v7 00/11] refs backend pre-vtable

[PATCH v7 00/11] refs backend pre-vtable

[Michael Haggerty]

This is another reroll of the pre-vtable part of the refs-backend
patch series dt/refs-backend-pre-vtable. v6 [1] proved cumbersome
because it conflicted messily with lf/ref-is-hidden-namespace [2]. The
conflicts were partly due to the motion of code across files but, even
worse, due to the change of order of function definitions between old
and new files.

So I have heavily "optimized" this reroll for reviewability and to
minimize conflicts with other work in the area. The only such work
that I know of is lf/ref-is-hidden-namespace, which can now be merged
with this series *without conflicts*.

Changes since v6:

* It doesn't move refs.c to the refs/ subdirectory, as v6 did.
  Instead, leave the shared code in the existing file, refs.c.

* Don't rename refs.c to refs/files-backend.c and then selectively
  move content back to refs.c. Instead, move content only once and
  only in one direction, namely from refs.c -> refs/files-backend.c
  (in patch 08/11). This is a giant commit but *it makes no other
  changes* so it should be easy to review. (The "--patience" option of
  "git diff" is quite helpful here. It was turned on when this patch
  series was generated.)

* Preserve the order of code during the move. Aside from a few lines
  of boilerplate, each of the following commands, when applied to
  commit 08, shows only lines being deleted:

    git diff --patience $commit^:refs.c $commit:refs.c
    git diff --patience $commit^:refs.c $commit:refs/files-backend.c

* To make all of the above possible, patches 01 through 06 do a little
  bit of preparatory code untangling. These commits have themselves
  been split up a bit to make them as "obviously correct" as possible.
  Patch 07 creates the new header file, refs/refs-internal.h, thereby
  increasing the visibility of some declarations.

* The final patches 09, 10, and 11 are not quite as "obviously
  correct" as the first eight, but I left them in to keep the logical
  contents of this patch series the same as v6. But these last three
  commits could just as well be postponed until the next tranche of
  patches if that helps speed the way of the first eight patches into
  master.

* I also fixed a commit message and fixed the implementation of the
  new verify_refname_available() function to return a negative number
  on error as documented (previously it returned 1 on error).

I've tried to attribute authorship of these changes as fairly as
possible based on who initiated the corresponding changes. If anybody
feels that I have appropriated his work or, conversely, put words into
his mouth, just let me know and I would be happy to adjust the
authorship.

It would be great to get this patch series (at least the first eight
patches) reviewed and merged as soon as possible. Even though it no
longer conflicts with lf/ref-is-hidden-namespace, it is still very
prone to conflicting with any other work in the references code.

This patch series is also available on my GitHub fork [3] as branch
"refs-backend-pre-vtable".

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/280325/focus=280754
[2] http://article.gmane.org/gmane.comp.version-control.git/281004
[3] https://github.com/mhagger/git

David Turner (5):
  refs: make is_branch public
  copy_msg(): rename to copy_reflog_msg()
  initdb: make safe_create_dir public
  files_log_ref_write: new function
  refs: break out ref conflict checks

Michael Haggerty (4):
  pack_if_possible_fn(): use ref_type() instead of is_per_worktree_ref()
  refname_is_safe(): improve docstring
  refs/refs-internal.h: new header file
  refs: split filesystem-based refs code into a new file

Ronnie Sahlberg (2):
  verify_refname_available(): rename function
  verify_refname_available(): new function

 Makefile                       |    3 +-
 builtin/init-db.c              |   12 -
 cache.h                        |    8 +
 path.c                         |   12 +
 refs.c                         | 3709 +---------------------------------------
 refs.h                         |    2 +
 refs.c => refs/files-backend.c | 1287 +-------------
 refs/refs-internal.h           |  202 +++
 8 files changed, 315 insertions(+), 4920 deletions(-)
 copy refs.c => refs/files-backend.c (75%)
 create mode 100644 refs/refs-internal.h

-- 
2.6.2

[PATCH v7 01/11] refs: make is_branch public

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

is_branch was already non-static, but this patch declares it in the
header.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.h b/refs.h
index 6d30c98..39b8edc 100644
--- a/refs.h
+++ b/refs.h
@@ -217,6 +217,8 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
  */
 int pack_refs(unsigned int flags);
 
+int is_branch(const char *refname);
+
 /*
  * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
-- 
2.6.2

[PATCH v7 02/11] verify_refname_available(): rename function

[Michael Haggerty]

From: Ronnie Sahlberg <sahlberg@google.com>

Rename verify_refname_available() to verify_refname_available_dir() to
make the old name available for a more general purpose.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/refs.c b/refs.c
index 132eff5..0617e0c 100644
--- a/refs.c
+++ b/refs.c
@@ -279,7 +279,7 @@ struct ref_dir {
  * presence of an empty subdirectory does not block the creation of a
  * similarly-named reference.  (The fact that reference names with the
  * same leading components can conflict *with each other* is a
- * separate issue that is regulated by verify_refname_available().)
+ * separate issue that is regulated by verify_refname_available_dir().)
  *
  * Please note that the name field contains the fully-qualified
  * reference (or subdirectory) name.  Space could be saved by only
@@ -911,11 +911,11 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
  *
  * extras and skip must be sorted.
  */
-static int verify_refname_available(const char *refname,
-				    const struct string_list *extras,
-				    const struct string_list *skip,
-				    struct ref_dir *dir,
-				    struct strbuf *err)
+static int verify_refname_available_dir(const char *refname,
+					const struct string_list *extras,
+					const struct string_list *skip,
+					struct ref_dir *dir,
+					struct strbuf *err)
 {
 	const char *slash;
 	int pos;
@@ -2465,8 +2465,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		strbuf_git_path(&orig_ref_file, "%s", orig_refname);
 		if (remove_empty_directories(&orig_ref_file)) {
 			last_errno = errno;
-			if (!verify_refname_available(orig_refname, extras, skip,
-						      get_loose_refs(&ref_cache), err))
+			if (!verify_refname_available_dir(orig_refname, extras, skip,
+							  get_loose_refs(&ref_cache), err))
 				strbuf_addf(err, "there are still refs under '%s'",
 					    orig_refname);
 			goto error_return;
@@ -2479,8 +2479,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (!refname) {
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
-		    !verify_refname_available(orig_refname, extras, skip,
-					      get_loose_refs(&ref_cache), err))
+		    !verify_refname_available_dir(orig_refname, extras, skip,
+						  get_loose_refs(&ref_cache), err))
 			strbuf_addf(err, "unable to resolve reference %s: %s",
 				    orig_refname, strerror(last_errno));
 
@@ -2493,8 +2493,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * our refname.
 	 */
 	if (is_null_oid(&lock->old_oid) &&
-	    verify_refname_available(refname, extras, skip,
-				     get_packed_refs(&ref_cache), err)) {
+	    verify_refname_available_dir(refname, extras, skip,
+					 get_packed_refs(&ref_cache), err)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -3127,10 +3127,10 @@ static int rename_ref_available(const char *oldname, const char *newname)
 	int ret;
 
 	string_list_insert(&skip, oldname);
-	ret = !verify_refname_available(newname, NULL, &skip,
-					get_packed_refs(&ref_cache), &err)
-		&& !verify_refname_available(newname, NULL, &skip,
-					     get_loose_refs(&ref_cache), &err);
+	ret = !verify_refname_available_dir(newname, NULL, &skip,
+					    get_packed_refs(&ref_cache), &err)
+		&& !verify_refname_available_dir(newname, NULL, &skip,
+						 get_loose_refs(&ref_cache), &err);
 	if (!ret)
 		error("%s", err.buf);
 
@@ -4376,12 +4376,12 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 		if ((update->flags & REF_HAVE_OLD) &&
 		    !is_null_sha1(update->old_sha1))
 			die("BUG: initial ref transaction with old_sha1 set");
-		if (verify_refname_available(update->refname,
-					     &affected_refnames, NULL,
-					     loose_refs, err) ||
-		    verify_refname_available(update->refname,
-					     &affected_refnames, NULL,
-					     packed_refs, err)) {
+		if (verify_refname_available_dir(update->refname,
+						 &affected_refnames, NULL,
+						 loose_refs, err) ||
+		    verify_refname_available_dir(update->refname,
+						 &affected_refnames, NULL,
+						 packed_refs, err)) {
 			ret = TRANSACTION_NAME_CONFLICT;
 			goto cleanup;
 		}
-- 
2.6.2

[PATCH v7 03/11] verify_refname_available(): new function

[Michael Haggerty]

From: Ronnie Sahlberg <sahlberg@google.com>

Add a new verify_refname_available() function, which checks whether the
refname is available for use, taking all references (both packed and
loose) into account. This function, unlike the old
verify_refname_available(), has semantics independent of the choice of
reference storage, and can therefore be implemented by alternative
reference backends.

Use the new function in a couple of places.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 66 ++++++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/refs.c b/refs.c
index 0617e0c..ddcdf81 100644
--- a/refs.c
+++ b/refs.c
@@ -279,7 +279,7 @@ struct ref_dir {
  * presence of an empty subdirectory does not block the creation of a
  * similarly-named reference.  (The fact that reference names with the
  * same leading components can conflict *with each other* is a
- * separate issue that is regulated by verify_refname_available_dir().)
+ * separate issue that is regulated by verify_refname_available().)
  *
  * Please note that the name field contains the fully-qualified
  * reference (or subdirectory) name.  Space could be saved by only
@@ -897,19 +897,7 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 /*
  * Return 0 if a reference named refname could be created without
  * conflicting with the name of an existing reference in dir.
- * Otherwise, return a negative value and write an explanation to err.
- * If extras is non-NULL, it is a list of additional refnames with
- * which refname is not allowed to conflict. If skip is non-NULL,
- * ignore potential conflicts with refs in skip (e.g., because they
- * are scheduled for deletion in the same operation). Behavior is
- * undefined if the same name is listed in both extras and skip.
- *
- * Two reference names conflict if one of them exactly matches the
- * leading components of the other; e.g., "refs/foo/bar" conflicts
- * with both "refs/foo" and with "refs/foo/bar/baz" but not with
- * "refs/foo/bar" or "refs/foo/barbados".
- *
- * extras and skip must be sorted.
+ * See verify_refname_available for more information.
  */
 static int verify_refname_available_dir(const char *refname,
 					const struct string_list *extras,
@@ -3120,6 +3108,40 @@ out:
 	return ret;
 }
 
+/*
+ * Return 0 if a reference named refname could be created without
+ * conflicting with the name of an existing reference. Otherwise,
+ * return a negative value and write an explanation to err. If extras
+ * is non-NULL, it is a list of additional refnames with which refname
+ * is not allowed to conflict. If skip is non-NULL, ignore potential
+ * conflicts with refs in skip (e.g., because they are scheduled for
+ * deletion in the same operation). Behavior is undefined if the same
+ * name is listed in both extras and skip.
+ *
+ * Two reference names conflict if one of them exactly matches the
+ * leading components of the other; e.g., "foo/bar" conflicts with
+ * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
+ * "foo/barbados".
+ *
+ * extras and skip must be sorted.
+ */
+static int verify_refname_available(const char *newname,
+				    struct string_list *extras,
+				    struct string_list *skip,
+				    struct strbuf *err)
+{
+	struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
+	struct ref_dir *loose_refs = get_loose_refs(&ref_cache);
+
+	if (verify_refname_available_dir(newname, extras, skip,
+					 packed_refs, err) ||
+	    verify_refname_available_dir(newname, extras, skip,
+					 loose_refs, err))
+		return -1;
+
+	return 0;
+}
+
 static int rename_ref_available(const char *oldname, const char *newname)
 {
 	struct string_list skip = STRING_LIST_INIT_NODUP;
@@ -3127,10 +3149,7 @@ static int rename_ref_available(const char *oldname, const char *newname)
 	int ret;
 
 	string_list_insert(&skip, oldname);
-	ret = !verify_refname_available_dir(newname, NULL, &skip,
-					    get_packed_refs(&ref_cache), &err)
-		&& !verify_refname_available_dir(newname, NULL, &skip,
-						 get_loose_refs(&ref_cache), &err);
+	ret = !verify_refname_available(newname, NULL, &skip, &err);
 	if (!ret)
 		error("%s", err.buf);
 
@@ -4334,8 +4353,6 @@ static int ref_present(const char *refname,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err)
 {
-	struct ref_dir *loose_refs = get_loose_refs(&ref_cache);
-	struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
 	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
@@ -4376,12 +4393,9 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 		if ((update->flags & REF_HAVE_OLD) &&
 		    !is_null_sha1(update->old_sha1))
 			die("BUG: initial ref transaction with old_sha1 set");
-		if (verify_refname_available_dir(update->refname,
-						 &affected_refnames, NULL,
-						 loose_refs, err) ||
-		    verify_refname_available_dir(update->refname,
-						 &affected_refnames, NULL,
-						 packed_refs, err)) {
+		if (verify_refname_available(update->refname,
+					     &affected_refnames, NULL,
+					     err)) {
 			ret = TRANSACTION_NAME_CONFLICT;
 			goto cleanup;
 		}
-- 
2.6.2

[PATCH v7 04/11] copy_msg(): rename to copy_reflog_msg()

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

We will soon increase the visibility of this function, so make its name
more distinctive.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index ddcdf81..480de9a 100644
--- a/refs.c
+++ b/refs.c
@@ -3287,7 +3287,7 @@ static int commit_ref(struct ref_lock *lock)
  * large, while cleaning up the whitespaces.  Especially, convert LF to space,
  * because reflog file is one line per entry.
  */
-static int copy_msg(char *buf, const char *msg)
+static int copy_reflog_msg(char *buf, const char *msg)
 {
 	char *cp = buf;
 	char c;
@@ -3391,7 +3391,7 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 			sha1_to_hex(new_sha1),
 			committer);
 	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
+		len += copy_reflog_msg(logrec + len - 1, msg) - 1;
 
 	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
 	free(logrec);
-- 
2.6.2

[PATCH v7 05/11] pack_if_possible_fn(): use ref_type() instead of is_per_worktree_ref()

[Michael Haggerty]

is_per_worktree_ref() will soon be made private, so use the public
interface, ref_type(), in its place. And now that we're using
ref_type(), we can make it clear that we won't pack pseudorefs. This was
the case before, but due to the not-so-obvious reason that this function
is applied to references via the loose reference cache, which only
includes references that live inside "refs/".

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 480de9a..82129f0 100644
--- a/refs.c
+++ b/refs.c
@@ -2671,8 +2671,6 @@ struct pack_refs_cb_data {
 	struct ref_to_prune *ref_to_prune;
 };
 
-static int is_per_worktree_ref(const char *refname);
-
 /*
  * An each_ref_entry_fn that is run over loose references only.  If
  * the loose reference can be packed, add an entry in the packed ref
@@ -2687,7 +2685,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
 	int is_tag_ref = starts_with(entry->name, "refs/tags/");
 
 	/* Do not pack per-worktree refs: */
-	if (is_per_worktree_ref(entry->name))
+	if (ref_type(entry->name) != REF_TYPE_NORMAL)
 		return 0;
 
 	/* ALWAYS pack tags */
-- 
2.6.2

[PATCH v7 06/11] refname_is_safe(): improve docstring

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 82129f0..f48c58a 100644
--- a/refs.c
+++ b/refs.c
@@ -341,13 +341,17 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 }
 
 /*
- * Check if a refname is safe.
- * For refs that start with "refs/" we consider it safe as long they do
- * not try to resolve to outside of refs/.
+ * Return true iff refname is minimally safe. "Safe" here means that
+ * deleting a loose reference by this name will not do any damage, for
+ * example by causing a file that is not a reference to be deleted.
+ * This function does not check that the reference name is legal; for
+ * that, use check_refname_format().
  *
- * For all other refs we only consider them safe iff they only contain
- * upper case characters and '_' (like "HEAD" AND "MERGE_HEAD", and not like
- * "config").
+ * We consider a refname that starts with "refs/" to be safe as long
+ * as any ".." components that it might contain do not escape "refs/".
+ * Names that do not start with "refs/" are considered safe iff they
+ * consist entirely of upper case characters and '_' (like "HEAD" and
+ * "MERGE_HEAD" but not "config" or "FOO/BAR").
  */
 static int refname_is_safe(const char *refname)
 {
-- 
2.6.2

[PATCH v7 07/11] refs/refs-internal.h: new header file

[Michael Haggerty]

There are a number of constants, structs, and static functions defined
in refs.c and treated as private to the references module. But we want
to support multiple reference backends within the reference module,
and those backends will need access to some heretofore private
declarations.

We don't want those declarations to be visible to non-refs code, so we
don't want to move them to refs.h. Instead, add a new header file,
refs/refs-internal.h, that is intended to be included only from within
the refs module. Make some functions non-static and move some
declarations (and their corresponding docstrings) from refs.c to this
file.

In a moment we will add more content to the "refs" subdirectory.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 175 +++----------------------------------------------
 refs/refs-internal.h | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 191 insertions(+), 166 deletions(-)
 create mode 100644 refs/refs-internal.h

diff --git a/refs.c b/refs.c
index f48c58a..9aff0c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1,6 +1,6 @@
 #include "cache.h"
+#include "refs/refs-internal.h"
 #include "lockfile.h"
-#include "refs.h"
 #include "object.h"
 #include "tag.h"
 #include "dir.h"
@@ -35,41 +35,6 @@ static unsigned char refname_disposition[256] = {
 };
 
 /*
- * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
- * refs (i.e., because the reference is about to be deleted anyway).
- */
-#define REF_DELETING	0x02
-
-/*
- * Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
- */
-#define REF_ISPRUNING	0x04
-
-/*
- * Used as a flag in ref_update::flags when the reference should be
- * updated to new_sha1.
- */
-#define REF_HAVE_NEW	0x08
-
-/*
- * Used as a flag in ref_update::flags when old_sha1 should be
- * checked.
- */
-#define REF_HAVE_OLD	0x10
-
-/*
- * Used as a flag in ref_update::flags when the lockfile needs to be
- * committed.
- */
-#define REF_NEEDS_COMMIT 0x20
-
-/*
- * 0x40 is REF_FORCE_CREATE_REFLOG, so skip it if you're adding a
- * value to ref_update::flags
- */
-
-/*
  * Try to read one refname component from the front of refname.
  * Return the length of the component found, or -1 if the component is
  * not legal.  It is legal if it is something reasonable to have under
@@ -340,20 +305,7 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 	return dir;
 }
 
-/*
- * Return true iff refname is minimally safe. "Safe" here means that
- * deleting a loose reference by this name will not do any damage, for
- * example by causing a file that is not a reference to be deleted.
- * This function does not check that the reference name is legal; for
- * that, use check_refname_format().
- *
- * We consider a refname that starts with "refs/" to be safe as long
- * as any ".." components that it might contain do not escape "refs/".
- * Names that do not start with "refs/" are considered safe iff they
- * consist entirely of upper case characters and '_' (like "HEAD" and
- * "MERGE_HEAD" but not "config" or "FOO/BAR").
- */
-static int refname_is_safe(const char *refname)
+int refname_is_safe(const char *refname)
 {
 	if (starts_with(refname, "refs/")) {
 		char *buf;
@@ -1823,39 +1775,7 @@ static int filter_refs(const char *refname, const struct object_id *oid,
 	return filter->fn(refname, oid, flags, filter->cb_data);
 }
 
-enum peel_status {
-	/* object was peeled successfully: */
-	PEEL_PEELED = 0,
-
-	/*
-	 * object cannot be peeled because the named object (or an
-	 * object referred to by a tag in the peel chain), does not
-	 * exist.
-	 */
-	PEEL_INVALID = -1,
-
-	/* object cannot be peeled because it is not a tag: */
-	PEEL_NON_TAG = -2,
-
-	/* ref_entry contains no peeled value because it is a symref: */
-	PEEL_IS_SYMREF = -3,
-
-	/*
-	 * ref_entry cannot be peeled because it is broken (i.e., the
-	 * symbolic reference cannot even be resolved to an object
-	 * name):
-	 */
-	PEEL_BROKEN = -4
-};
-
-/*
- * Peel the named object; i.e., if the object is a tag, resolve the
- * tag recursively until a non-tag is found.  If successful, store the
- * result to sha1 and return PEEL_PEELED.  If the object is not a tag
- * or is not valid, return PEEL_NON_TAG or PEEL_INVALID, respectively,
- * and leave sha1 unchanged.
- */
-static enum peel_status peel_object(const unsigned char *name, unsigned char *sha1)
+enum peel_status peel_object(const unsigned char *name, unsigned char *sha1)
 {
 	struct object *o = lookup_unknown_object(name);
 
@@ -3110,27 +3030,10 @@ out:
 	return ret;
 }
 
-/*
- * Return 0 if a reference named refname could be created without
- * conflicting with the name of an existing reference. Otherwise,
- * return a negative value and write an explanation to err. If extras
- * is non-NULL, it is a list of additional refnames with which refname
- * is not allowed to conflict. If skip is non-NULL, ignore potential
- * conflicts with refs in skip (e.g., because they are scheduled for
- * deletion in the same operation). Behavior is undefined if the same
- * name is listed in both extras and skip.
- *
- * Two reference names conflict if one of them exactly matches the
- * leading components of the other; e.g., "foo/bar" conflicts with
- * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
- * "foo/barbados".
- *
- * extras and skip must be sorted.
- */
-static int verify_refname_available(const char *newname,
-				    struct string_list *extras,
-				    struct string_list *skip,
-				    struct strbuf *err)
+int verify_refname_available(const char *newname,
+			     struct string_list *extras,
+			     struct string_list *skip,
+			     struct strbuf *err)
 {
 	struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
 	struct ref_dir *loose_refs = get_loose_refs(&ref_cache);
@@ -3284,12 +3187,7 @@ static int commit_ref(struct ref_lock *lock)
 	return 0;
 }
 
-/*
- * copy the reflog message msg to buf, which has been allocated sufficiently
- * large, while cleaning up the whitespaces.  Especially, convert LF to space,
- * because reflog file is one line per entry.
- */
-static int copy_reflog_msg(char *buf, const char *msg)
+int copy_reflog_msg(char *buf, const char *msg)
 {
 	char *cp = buf;
 	char c;
@@ -3310,7 +3208,7 @@ static int copy_reflog_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
-static int should_autocreate_reflog(const char *refname)
+int should_autocreate_reflog(const char *refname)
 {
 	if (!log_all_ref_updates)
 		return 0;
@@ -3963,61 +3861,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }
 
-/**
- * Information needed for a single ref update. Set new_sha1 to the new
- * value or to null_sha1 to delete the ref. To check the old value
- * while the ref is locked, set (flags & REF_HAVE_OLD) and set
- * old_sha1 to the old value, or to null_sha1 to ensure the ref does
- * not exist before update.
- */
-struct ref_update {
-	/*
-	 * If (flags & REF_HAVE_NEW), set the reference to this value:
-	 */
-	unsigned char new_sha1[20];
-	/*
-	 * If (flags & REF_HAVE_OLD), check that the reference
-	 * previously had this value:
-	 */
-	unsigned char old_sha1[20];
-	/*
-	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
-	 * REF_DELETING, and REF_ISPRUNING:
-	 */
-	unsigned int flags;
-	struct ref_lock *lock;
-	int type;
-	char *msg;
-	const char refname[FLEX_ARRAY];
-};
-
-/*
- * Transaction states.
- * OPEN:   The transaction is in a valid state and can accept new updates.
- *         An OPEN transaction can be committed.
- * CLOSED: A closed transaction is no longer active and no other operations
- *         than free can be used on it in this state.
- *         A transaction can either become closed by successfully committing
- *         an active transaction or if there is a failure while building
- *         the transaction thus rendering it failed/inactive.
- */
-enum ref_transaction_state {
-	REF_TRANSACTION_OPEN   = 0,
-	REF_TRANSACTION_CLOSED = 1
-};
-
-/*
- * Data structure for holding a reference transaction, which can
- * consist of checks and updates to multiple references, carried out
- * as atomically as possible.  This structure is opaque to callers.
- */
-struct ref_transaction {
-	struct ref_update **updates;
-	size_t alloc;
-	size_t nr;
-	enum ref_transaction_state state;
-};
-
 struct ref_transaction *ref_transaction_begin(struct strbuf *err)
 {
 	assert(err);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
new file mode 100644
index 0000000..8f5cfb4
--- /dev/null
+++ b/refs/refs-internal.h
@@ -0,0 +1,182 @@
+#ifndef REFS_REFS_INTERNAL_H
+#define REFS_REFS_INTERNAL_H
+
+/*
+ * Data structures and functions for the internal use of the refs
+ * module. Code outside of the refs module should use only the public
+ * functions defined in "refs.h", and should *not* include this file.
+ */
+
+#include "../refs.h"
+
+/*
+ * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
+ * refs (i.e., because the reference is about to be deleted anyway).
+ */
+#define REF_DELETING	0x02
+
+/*
+ * Used as a flag in ref_update::flags when a loose ref is being
+ * pruned.
+ */
+#define REF_ISPRUNING	0x04
+
+/*
+ * Used as a flag in ref_update::flags when the reference should be
+ * updated to new_sha1.
+ */
+#define REF_HAVE_NEW	0x08
+
+/*
+ * Used as a flag in ref_update::flags when old_sha1 should be
+ * checked.
+ */
+#define REF_HAVE_OLD	0x10
+
+/*
+ * Used as a flag in ref_update::flags when the lockfile needs to be
+ * committed.
+ */
+#define REF_NEEDS_COMMIT 0x20
+
+/*
+ * 0x40 is REF_FORCE_CREATE_REFLOG, so skip it if you're adding a
+ * value to ref_update::flags
+ */
+
+/*
+ * Return true iff refname is minimally safe. "Safe" here means that
+ * deleting a loose reference by this name will not do any damage, for
+ * example by causing a file that is not a reference to be deleted.
+ * This function does not check that the reference name is legal; for
+ * that, use check_refname_format().
+ *
+ * We consider a refname that starts with "refs/" to be safe as long
+ * as any ".." components that it might contain do not escape "refs/".
+ * Names that do not start with "refs/" are considered safe iff they
+ * consist entirely of upper case characters and '_' (like "HEAD" and
+ * "MERGE_HEAD" but not "config" or "FOO/BAR").
+ */
+int refname_is_safe(const char *refname);
+
+enum peel_status {
+	/* object was peeled successfully: */
+	PEEL_PEELED = 0,
+
+	/*
+	 * object cannot be peeled because the named object (or an
+	 * object referred to by a tag in the peel chain), does not
+	 * exist.
+	 */
+	PEEL_INVALID = -1,
+
+	/* object cannot be peeled because it is not a tag: */
+	PEEL_NON_TAG = -2,
+
+	/* ref_entry contains no peeled value because it is a symref: */
+	PEEL_IS_SYMREF = -3,
+
+	/*
+	 * ref_entry cannot be peeled because it is broken (i.e., the
+	 * symbolic reference cannot even be resolved to an object
+	 * name):
+	 */
+	PEEL_BROKEN = -4
+};
+
+/*
+ * Peel the named object; i.e., if the object is a tag, resolve the
+ * tag recursively until a non-tag is found.  If successful, store the
+ * result to sha1 and return PEEL_PEELED.  If the object is not a tag
+ * or is not valid, return PEEL_NON_TAG or PEEL_INVALID, respectively,
+ * and leave sha1 unchanged.
+ */
+enum peel_status peel_object(const unsigned char *name, unsigned char *sha1);
+
+/*
+ * Return 0 if a reference named refname could be created without
+ * conflicting with the name of an existing reference. Otherwise,
+ * return a negative value and write an explanation to err. If extras
+ * is non-NULL, it is a list of additional refnames with which refname
+ * is not allowed to conflict. If skip is non-NULL, ignore potential
+ * conflicts with refs in skip (e.g., because they are scheduled for
+ * deletion in the same operation). Behavior is undefined if the same
+ * name is listed in both extras and skip.
+ *
+ * Two reference names conflict if one of them exactly matches the
+ * leading components of the other; e.g., "foo/bar" conflicts with
+ * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
+ * "foo/barbados".
+ *
+ * extras and skip must be sorted.
+ */
+int verify_refname_available(const char *newname,
+			     struct string_list *extras,
+			     struct string_list *skip,
+			     struct strbuf *err);
+
+/*
+ * Copy the reflog message msg to buf, which has been allocated sufficiently
+ * large, while cleaning up the whitespaces.  Especially, convert LF to space,
+ * because reflog file is one line per entry.
+ */
+int copy_reflog_msg(char *buf, const char *msg);
+
+int should_autocreate_reflog(const char *refname);
+
+/**
+ * Information needed for a single ref update. Set new_sha1 to the new
+ * value or to null_sha1 to delete the ref. To check the old value
+ * while the ref is locked, set (flags & REF_HAVE_OLD) and set
+ * old_sha1 to the old value, or to null_sha1 to ensure the ref does
+ * not exist before update.
+ */
+struct ref_update {
+	/*
+	 * If (flags & REF_HAVE_NEW), set the reference to this value:
+	 */
+	unsigned char new_sha1[20];
+	/*
+	 * If (flags & REF_HAVE_OLD), check that the reference
+	 * previously had this value:
+	 */
+	unsigned char old_sha1[20];
+	/*
+	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
+	 * REF_DELETING, and REF_ISPRUNING:
+	 */
+	unsigned int flags;
+	struct ref_lock *lock;
+	int type;
+	char *msg;
+	const char refname[FLEX_ARRAY];
+};
+
+/*
+ * Transaction states.
+ * OPEN:   The transaction is in a valid state and can accept new updates.
+ *         An OPEN transaction can be committed.
+ * CLOSED: A closed transaction is no longer active and no other operations
+ *         than free can be used on it in this state.
+ *         A transaction can either become closed by successfully committing
+ *         an active transaction or if there is a failure while building
+ *         the transaction thus rendering it failed/inactive.
+ */
+enum ref_transaction_state {
+	REF_TRANSACTION_OPEN   = 0,
+	REF_TRANSACTION_CLOSED = 1
+};
+
+/*
+ * Data structure for holding a reference transaction, which can
+ * consist of checks and updates to multiple references, carried out
+ * as atomically as possible.  This structure is opaque to callers.
+ */
+struct ref_transaction {
+	struct ref_update **updates;
+	size_t alloc;
+	size_t nr;
+	enum ref_transaction_state state;
+};
+
+#endif /* REFS_REFS_INTERNAL_H */
-- 
2.6.2

[PATCH v7 09/11] initdb: make safe_create_dir public

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

Soon we will want to create initdb functions for ref backends, and
code from initdb that calls this function needs to move into the files
backend. So this function needs to be public.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/init-db.c | 12 ------------
 cache.h           |  8 ++++++++
 path.c            | 12 ++++++++++++
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index f59f407..07229d6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -24,18 +24,6 @@ static int init_shared_repository = -1;
 static const char *init_db_template_dir;
 static const char *git_link;
 
-static void safe_create_dir(const char *dir, int share)
-{
-	if (mkdir(dir, 0777) < 0) {
-		if (errno != EEXIST) {
-			perror(dir);
-			exit(1);
-		}
-	}
-	else if (share && adjust_shared_perm(dir))
-		die(_("Could not make %s writable by group"), dir);
-}
-
 static void copy_templates_1(struct strbuf *path, struct strbuf *template,
 			     DIR *dir)
 {
diff --git a/cache.h b/cache.h
index 9a905a8..0f9808c 100644
--- a/cache.h
+++ b/cache.h
@@ -1737,4 +1737,12 @@ void stat_validity_update(struct stat_validity *sv, int fd);
 int versioncmp(const char *s1, const char *s2);
 void sleep_millisec(int millisec);
 
+/*
+ * Create a directory and (if share is nonzero) adjust its permissions
+ * according to the shared_repository setting. Only use this for
+ * directories under $GIT_DIR.  Don't use it for working tree
+ * directories.
+ */
+void safe_create_dir(const char *dir, int share);
+
 #endif /* CACHE_H */
diff --git a/path.c b/path.c
index 212695a..9e0283c 100644
--- a/path.c
+++ b/path.c
@@ -723,6 +723,18 @@ int adjust_shared_perm(const char *path)
 	return 0;
 }
 
+void safe_create_dir(const char *dir, int share)
+{
+	if (mkdir(dir, 0777) < 0) {
+		if (errno != EEXIST) {
+			perror(dir);
+			exit(1);
+		}
+	}
+	else if (share && adjust_shared_perm(dir))
+		die(_("Could not make %s writable by group"), dir);
+}
+
 static int have_same_root(const char *path1, const char *path2)
 {
 	int is_abs1, is_abs2;
-- 
2.6.2

[PATCH v7 10/11] files_log_ref_write: new function

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

Because HEAD and stash are per-worktree, every refs backend needs to
go through the files backend to write these refs.

So create a new function, files_log_ref_write, and add it to
refs/refs-internal.h. Later, we will use this to handle reflog updates
for per-worktree symbolic refs (HEAD).

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 8 ++++++++
 refs/refs-internal.h | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a2e5a56..1094348 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2735,6 +2735,14 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg,
 			 int flags, struct strbuf *err)
 {
+	return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
+				   err);
+}
+
+int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+			const unsigned char *new_sha1, const char *msg,
+			int flags, struct strbuf *err)
+{
 	struct strbuf sb = STRBUF_INIT;
 	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags,
 				  err);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8f5cfb4..88a5be0 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -179,4 +179,8 @@ struct ref_transaction {
 	enum ref_transaction_state state;
 };
 
+int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+			const unsigned char *new_sha1, const char *msg,
+			int flags, struct strbuf *err);
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.6.2

[PATCH v7 11/11] refs: break out ref conflict checks

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

Create new function find_descendant_ref, to hold one of the ref
conflict checks used in verify_refname_available. Multiple backends
will need this function, so move it to the common code.

Also move rename_ref_available to the common code, because alternate
backends might need it and it has no files-backend-specific code.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 44 ++++++++++++++++++++++++++++++++++++++++++++
 refs/files-backend.c | 49 +++++++------------------------------------------
 refs/refs-internal.h | 16 ++++++++++++++++
 3 files changed, 67 insertions(+), 42 deletions(-)

diff --git a/refs.c b/refs.c
index 175b3e9..0e2eaf8 100644
--- a/refs.c
+++ b/refs.c
@@ -1027,3 +1027,47 @@ int ref_is_hidden(const char *refname)
 	}
 	return 0;
 }
+
+const char *find_descendant_ref(const char *dirname,
+				const struct string_list *extras,
+				const struct string_list *skip)
+{
+	int pos;
+
+	if (!extras)
+		return NULL;
+
+	/*
+	 * Look at the place where dirname would be inserted into
+	 * extras. If there is an entry at that position that starts
+	 * with dirname (remember, dirname includes the trailing
+	 * slash) and is not in skip, then we have a conflict.
+	 */
+	for (pos = string_list_find_insert_index(extras, dirname, 0);
+	     pos < extras->nr; pos++) {
+		const char *extra_refname = extras->items[pos].string;
+
+		if (!starts_with(extra_refname, dirname))
+			break;
+
+		if (!skip || !string_list_has_string(skip, extra_refname))
+			return extra_refname;
+	}
+	return NULL;
+}
+
+int rename_ref_available(const char *oldname, const char *newname)
+{
+	struct string_list skip = STRING_LIST_INIT_NODUP;
+	struct strbuf err = STRBUF_INIT;
+	int ret;
+
+	string_list_insert(&skip, oldname);
+	ret = !verify_refname_available(newname, NULL, &skip, &err);
+	if (!ret)
+		error("%s", err.buf);
+
+	string_list_clear(&skip, 0);
+	strbuf_release(&err);
+	return ret;
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1094348..8bb3728 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -729,6 +729,7 @@ static int verify_refname_available_dir(const char *refname,
 					struct strbuf *err)
 {
 	const char *slash;
+	const char *extra_refname;
 	int pos;
 	struct strbuf dirname = STRBUF_INIT;
 	int ret = -1;
@@ -834,32 +835,12 @@ static int verify_refname_available_dir(const char *refname,
 		}
 	}
 
-	if (extras) {
-		/*
-		 * Check for entries in extras that start with
-		 * "$refname/". We do that by looking for the place
-		 * where "$refname/" would be inserted in extras. If
-		 * there is an entry at that position that starts with
-		 * "$refname/" and is not in skip, then we have a
-		 * conflict.
-		 */
-		for (pos = string_list_find_insert_index(extras, dirname.buf, 0);
-		     pos < extras->nr; pos++) {
-			const char *extra_refname = extras->items[pos].string;
-
-			if (!starts_with(extra_refname, dirname.buf))
-				break;
-
-			if (!skip || !string_list_has_string(skip, extra_refname)) {
-				strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
-					    refname, extra_refname);
-				goto cleanup;
-			}
-		}
-	}
-
-	/* No conflicts were found */
-	ret = 0;
+	extra_refname = find_descendant_ref(dirname.buf, extras, skip);
+	if (extra_refname)
+		strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
+			    refname, extra_refname);
+	else
+		ret = 0;
 
 cleanup:
 	strbuf_release(&dirname);
@@ -2474,22 +2455,6 @@ int verify_refname_available(const char *newname,
 	return 0;
 }
 
-static int rename_ref_available(const char *oldname, const char *newname)
-{
-	struct string_list skip = STRING_LIST_INIT_NODUP;
-	struct strbuf err = STRBUF_INIT;
-	int ret;
-
-	string_list_insert(&skip, oldname);
-	ret = !verify_refname_available(newname, NULL, &skip, &err);
-	if (!ret)
-		error("%s", err.buf);
-
-	string_list_clear(&skip, 0);
-	strbuf_release(&err);
-	return ret;
-}
-
 static int write_ref_to_lockfile(struct ref_lock *lock,
 				 const unsigned char *sha1, struct strbuf *err);
 static int commit_ref_update(struct ref_lock *lock,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 88a5be0..6c29f45 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -183,4 +183,20 @@ int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
 			const unsigned char *new_sha1, const char *msg,
 			int flags, struct strbuf *err);
 
+/*
+ * Check for entries in extras that are within the specified
+ * directory, where dirname is a reference directory name including
+ * the trailing slash (e.g., "refs/heads/foo/"). Ignore any
+ * conflicting references that are found in skip. If there is a
+ * conflicting reference, return its name.
+ *
+ * extras and skip must be sorted lists of reference names. Either one
+ * can be NULL, signifying the empty list.
+ */
+const char *find_descendant_ref(const char *dirname,
+				const struct string_list *extras,
+				const struct string_list *skip);
+
+int rename_ref_available(const char *oldname, const char *newname);
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.6.2

Re: [PATCH v7 01/11] refs: make is_branch public

[Ramsay Jones]

On 09/11/15 17:03, Michael Haggerty wrote:
> From: David Turner <dturner@twopensource.com>
> 
> is_branch was already non-static, but this patch declares it in the
> header.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/refs.h b/refs.h
> index 6d30c98..39b8edc 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -217,6 +217,8 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
>   */
>  int pack_refs(unsigned int flags);
>  
> +int is_branch(const char *refname);
> +
>  /*
>   * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
>   * REF_NODEREF: act on the ref directly, instead of dereferencing
> 

I don't understand, is_branch() is already declared in refs.h, see line 67.

This is true in master, next and pu now appears to have two declarations.

ATB,
Ramsay Jones

Re: [PATCH v7 07/11] refs/refs-internal.h: new header file

[Ramsay Jones]

On 09/11/15 17:03, Michael Haggerty wrote:
> There are a number of constants, structs, and static functions defined
> in refs.c and treated as private to the references module. But we want
> to support multiple reference backends within the reference module,
> and those backends will need access to some heretofore private
> declarations.
> 
> We don't want those declarations to be visible to non-refs code, so we
> don't want to move them to refs.h. Instead, add a new header file,
> refs/refs-internal.h, that is intended to be included only from within
> the refs module. Make some functions non-static and move some
> declarations (and their corresponding docstrings) from refs.c to this
> file.
> 
> In a moment we will add more content to the "refs" subdirectory.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c               | 175 +++----------------------------------------------
>  refs/refs-internal.h | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 191 insertions(+), 166 deletions(-)
>  create mode 100644 refs/refs-internal.h
> 
> diff --git a/refs.c b/refs.c
> index f48c58a..9aff0c8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1,6 +1,6 @@
>  #include "cache.h"
> +#include "refs/refs-internal.h"
>  #include "lockfile.h"
> -#include "refs.h"

This looked wrong to me, until I had read the remainder of the
patch and noticed that the 'internal' header #included the
'public' interface header.

Unfortunately, this still feels wrong to me! I would rather that
the internal header _not_ include the public header (so, include
them _both_ when necessary). Just my opinion, which you can simply
ignore. :-D

ATB,
Ramsay Jones

Re: [PATCH v7 01/11] refs: make is_branch public

[David Turner]

On Mon, 2015-11-09 at 18:54 +0000, Ramsay Jones wrote:
> 
> On 09/11/15 17:03, Michael Haggerty wrote:
> > From: David Turner <dturner@twopensource.com>
> > 
> > is_branch was already non-static, but this patch declares it in the
> > header.
> > 
> > Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  refs.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/refs.h b/refs.h
> > index 6d30c98..39b8edc 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -217,6 +217,8 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
> >   */
> >  int pack_refs(unsigned int flags);
> >  
> > +int is_branch(const char *refname);
> > +
> >  /*
> >   * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
> >   * REF_NODEREF: act on the ref directly, instead of dereferencing
> > 
> 
> I don't understand, is_branch() is already declared in refs.h, see line 67.
> 
> This is true in master, next and pu now appears to have two declarations.

Agreed.  This was necessary at the time I started on this work, but this
patch apparently stuck around past the time it was needed.   Please
drop.

Re: [PATCH v7 07/11] refs/refs-internal.h: new header file

[David Turner]

On Mon, 2015-11-09 at 19:46 +0000, Ramsay Jones wrote:
> 
> On 09/11/15 17:03, Michael Haggerty wrote:
> > There are a number of constants, structs, and static functions defined
> > in refs.c and treated as private to the references module. But we want
> > to support multiple reference backends within the reference module,
> > and those backends will need access to some heretofore private
> > declarations.
> > 
> > We don't want those declarations to be visible to non-refs code, so we
> > don't want to move them to refs.h. Instead, add a new header file,
> > refs/refs-internal.h, that is intended to be included only from within
> > the refs module. Make some functions non-static and move some
> > declarations (and their corresponding docstrings) from refs.c to this
> > file.
> > 
> > In a moment we will add more content to the "refs" subdirectory.
> > 
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  refs.c               | 175 +++----------------------------------------------
> >  refs/refs-internal.h | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 191 insertions(+), 166 deletions(-)
> >  create mode 100644 refs/refs-internal.h
> > 
> > diff --git a/refs.c b/refs.c
> > index f48c58a..9aff0c8 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1,6 +1,6 @@
> >  #include "cache.h"
> > +#include "refs/refs-internal.h"
> >  #include "lockfile.h"
> > -#include "refs.h"
> 
> This looked wrong to me, until I had read the remainder of the
> patch and noticed that the 'internal' header #included the
> 'public' interface header.
> 
> Unfortunately, this still feels wrong to me! I would rather that
> the internal header _not_ include the public header (so, include
> them _both_ when necessary). Just my opinion, which you can simply
> ignore. :-D

+1 on this.

Re: [PATCH v7 00/11] refs backend pre-vtable

[David Turner]

On Mon, 2015-11-09 at 18:03 +0100, Michael Haggerty wrote:
> This is another reroll of the pre-vtable part of the refs-backend
> patch series dt/refs-backend-pre-vtable. v6 [1] proved cumbersome
> because it conflicted messily with lf/ref-is-hidden-namespace [2]. The
> conflicts were partly due to the motion of code across files but, even
> worse, due to the change of order of function definitions between old
> and new files.
>
> So I have heavily "optimized" this reroll for reviewability and to
> minimize conflicts with other work in the area. The only such work
> that I know of is lf/ref-is-hidden-namespace, which can now be merged
> with this series *without conflicts*.

Modulo Ramsey's comments, this works for me.  AIUI, Ronnie's original
patchset was written the way it was to avoid merge conflicts.  Because
of the changes in the interim, it turns out not to quite work that way.
So it's totally OK with me to write it this new way.

Re: [PATCH v7 01/11] refs: make is_branch public

[Michael Haggerty]

On 11/09/2015 07:54 PM, Ramsay Jones wrote:
> On 09/11/15 17:03, Michael Haggerty wrote:
>> [...]
>> diff --git a/refs.h b/refs.h
>> index 6d30c98..39b8edc 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -217,6 +217,8 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
>>   */
>>  int pack_refs(unsigned int flags);
>>  
>> +int is_branch(const char *refname);
>> +
>>  /*
>>   * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
>>   * REF_NODEREF: act on the ref directly, instead of dereferencing
>>
> 
> I don't understand, is_branch() is already declared in refs.h, see line 67.
> 
> This is true in master, next and pu now appears to have two declarations.

Thanks for noticing! I'll drop that patch from the next round.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v7 07/11] refs/refs-internal.h: new header file

[Michael Haggerty]

On 11/09/2015 08:46 PM, Ramsay Jones wrote:
> On 09/11/15 17:03, Michael Haggerty wrote:
>> [...]
>> diff --git a/refs.c b/refs.c
>> index f48c58a..9aff0c8 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1,6 +1,6 @@
>>  #include "cache.h"
>> +#include "refs/refs-internal.h"
>>  #include "lockfile.h"
>> -#include "refs.h"
> 
> This looked wrong to me, until I had read the remainder of the
> patch and noticed that the 'internal' header #included the
> 'public' interface header.
> 
> Unfortunately, this still feels wrong to me! I would rather that
> the internal header _not_ include the public header (so, include
> them _both_ when necessary). Just my opinion, which you can simply
> ignore. :-D

Yeah, I was of two minds about this. I will change this in the next
round. Thanks for your review!

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu