Re: [PATCH v4 14/21] refs: always handle non-normal refs in files backend

On 02/18/2016 03:44 AM, David Turner wrote:

On Fri, 2016-02-12 at 16:07 +0100, Michael Haggerty wrote:

quoted

On 02/05/2016 08:44 PM, David Turner wrote:

quoted

Always handle non-normal (per-worktree or pseudo) refs in the files
backend instead of alternate backends.

Sometimes a ref transaction will update both a per-worktree ref and
a
normal ref.  For instance, an ordinary commit might update
refs/heads/master and HEAD (or at least HEAD's reflog).

Updates to normal refs continue to go through the chosen backend.

Updates to non-normal refs are moved to a separate files backend
transaction.

Signed-off-by: David Turner <redacted>
---
 refs.c | 81
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 227c018..18ba356 100644
--- a/refs.c
+++ b/refs.c

@@ -9,6 +9,11 @@
 #include "object.h"
 #include "tag.h"
 
+static const char split_transaction_fail_warning[] = N_(
+	"A ref transaction was split across two refs backends. 
 Part of the "
+	"transaction succeeded, but then the update to the per
-worktree refs "
+	"failed.  Your repository may be in an inconsistent

state.");
+
 /*
  * We always have a files backend and it is the default.
  */

@@ -791,6 +796,13 @@ void ref_transaction_free(struct

ref_transaction *transaction)
 	free(transaction);
 }
 
+static void add_update_obj(struct ref_transaction *transaction,
+			   struct ref_update *update)
+{
+	ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
+	transaction->updates[transaction->nr++] = update;
+}
+
 static struct ref_update *add_update(struct ref_transaction
*transaction,
 				     const char *refname)
 {

@@ -798,8 +810,7 @@ static struct ref_update *add_update(struct

ref_transaction *transaction,
 	struct ref_update *update = xcalloc(1, sizeof(*update) +
len);
 
 	memcpy((char *)update->refname, refname, len); /* includes
NUL */
-	ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
-	transaction->updates[transaction->nr++] = update;
+	add_update_obj(transaction, update);
 	return update;
 }
 

@@ -1217,11 +1228,38 @@ static int dereference_symrefs(struct

ref_transaction *transaction,
 	return 0;
 }
 
+/*
+ * Move all non-normal ref updates into a specially-created
+ * files-backend transaction
+ */
+static int move_abnormal_ref_updates(struct ref_transaction
*transaction,
+				     struct ref_transaction
*files_transaction,
+				     struct strbuf *err)
+{
+	int i;
+
+	for (i = 0; i < transaction->nr; i++) {
+		int last;
+		struct ref_update *update = transaction
->updates[i];
+
+		if (ref_type(update->refname) == REF_TYPE_NORMAL)
+			continue;
+
+		last = --transaction->nr;
+		transaction->updates[i] = transaction
->updates[last];
+		add_update_obj(files_transaction, update);
+	}
+
+	return 0;
+}
+

I think this function is incorrect. The update that was previously at
transaction->updates[i] never gets checked for abnormality. 

Yes it does; that's the "update" variable that we just checked.

Sorry, I meant to say "the update that was previously at
`transaction->updates[transaction->nr - 1]` never gets checked for
abnormality". Because it gets moved to `transaction->updates[i]`, then
`i` is incremented without checking it.

[...]

quoted

Another alternative would be to set

    update->flags |= REF_ABNORMAL
[...]

I'm also interested in this idea. Perhaps it would also be nice to
report *why* they fail (e.g. the conflicting ref name).  I did a
variant of this with for the journal code, but my way of doing it
turned out to be a bad idea (long story).  But I want to stay focused
on the simplest thing possible, for now.

Fair enough.

[...]

quoted

Does initial_ref_transaction_commit() need the same treatment?

We only use that for remote refs -- I'm not sure if those can be
symrefs.  Wouldn't hurt.

Hmmm, good point. I think the only symrefs that can be set during a
clone are `HEAD` and `refs/remotes/origin/HEAD`. But I guess that no
other references are updated *through* these symrefs, so it's probably
OK. I haven't checked carefully, though.

[...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu