Re: [PATCH 12/16] refs: always handle non-normal refs in files backend

On 12/03/2015 01:35 AM, David Turner wrote:

Extra word? s/with//?

+static int move_abnormal_ref_updates(struct ref_transaction *transaction,
+				     struct ref_transaction *files_transaction,
+				     struct string_list *symrefs)
+{
+	int i;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = transaction->updates[i];
+		const char *resolved;
+		int flags = 0;
+		unsigned char sha1[20];
+
+		if (ref_type(update->refname) == REF_TYPE_NORMAL)
+			continue;
+
+		resolved = resolve_ref_unsafe(update->refname, 0, sha1, &flags);
+
+		if (update->flags & REF_NODEREF || !(flags & REF_ISSYMREF)) {
+			int last;
+
+			add_update_obj(files_transaction, update);
+			/*
+			 * Replace this transaction with the
+			 * last transaction, removing it from
+			 * the list of backend transactions
+			 */
+			last = --transaction->nr;
+			transaction->updates[i] = transaction->updates[last];

The "last" temporary variable could be trivially inlined.

+			continue;
+		}
+
+		if (resolved) {
+			struct ref_update *new_update;
+			struct string_list_item *item;
+
+			if (ref_type(resolved) != REF_TYPE_NORMAL)
+				die("Non-normal symbolic ref `%s` points to non-normal ref `%s`", update->refname, resolved);

We don't usually use backticks in error messages. Please use "'" instead.

Also, please store this error message into a "strbuf *err" and report it
via the usual mechanism.

+			new_update = xmalloc(sizeof(*new_update) +
+					     strlen(resolved) + 1);
+			memcpy(new_update, update, sizeof(*update));

Wouldn't it be preferable to replace this messy replacement code
(including the memcpy(), which can't be checked by the type system) with
a call to ref_transaction_update() followed by moving the new update to
this position in the list and possibly tweaking some of its fields?

+			if (update->flags & REF_HAVE_OLD &&
+			    hashcmp(sha1, update->old_sha1)) {
+				/* consistency check failed */
+				free(new_update);
+				return -1;

We need an error message to be reported in this case; i.e., via a
"struct strbuf *err" argument.

But actually, I don't understand why this check is needed here at all.
Isn't it redundant with a similar check that will be done later (and
properly, under lock) as part of the main ref_transaction_commit()?

I think dying here is too abrupt. Some callers try to recover from a
failed ref_transaction_commit(). Couldn't you "goto done" and let the
caller deal with err?

+		ret = move_abnormal_ref_updates(transaction, files_transaction,
+						&symrefs);
+		if (ret)
+			goto done;
+
+		/* files backend commit */
+		if (ref_update_reject_duplicates(files_transaction,
+						 &files_affected_refnames,
+						 err)) {
+			ret = TRANSACTION_GENERIC_ERROR;
+			goto done;
+		}

Is it correct to reject_duplicates among "abnormal" references and
"normal" references separately? ***

This conditional would perhaps be more to the point if expressed as "if
(files_transaction)".

+		ret = refs_be_files.transaction_commit(files_transaction,
+						       &files_affected_refnames,
+						       err);
+		if (ret) {
+			warning(split_transaction_fail_warning);
+			goto done;
+		}
+
+		/* reflogging for dereferenced symbolic refs */
+		for_each_string_list_item(item, &symrefs) {
+			struct ref_update *update = item->util;
+			if (files_log_ref_write(item->string, update->old_sha1,
+						update->new_sha1,
+						update->msg, update->flags, err))
+				warning("failed to log ref update for symref %s",
+					item->string);
+		}

I think this code is incorrect because it doesn't lock the symbolic
reference before modifying its reflog (though I seem to recall that the
old code was buggy in this respect, too).

I wonder whether it would be simpler overall to leave the ref_update for
the symbolic ref in the files_transaction, but set a special internal
internal flag like REF_LOG_ONLY which tells the usual transaction_commit
code to add a reflog entry for update->old_sha1 to update->new_sha1,
without actually changing the reference.

I very much like the idea of introducing special handling for symbolic
reference updates within a transaction. In fact, I think I would go even
farther:

Let's take the example of an update to HEAD, which currently points at
refs/heads/master. I think it would *always* be a good idea (i.e., even
when only the files backend is in use) to split that ref_update into two
ref_updates:

1. One to update refs/heads/master and add a reflog entry for that
   reference.

2. One to add a reflog entry for HEAD (i.e. using the new REF_LOG_ONLY
   flag suggested above).

Why?

* It ensures that both references are locked correctly while their
  reflogs are updated. (I believe the current code gets this wrong.)

* It improves the reject_duplicates coverage, which (I think) currently
  wouldn't detect the conflict between a direct update of
  refs/heads/master with a simultaneous update of the same reference
  via HEAD.

* It could later be generalized to an update that goes through multiple
  layers of symref indirection (though this would be a very low
  priority).

This might benefit the split-backend situation that you are implementing
here. You could first do the symref-splitting step I just described, and
*then* separate the non-normal from the normal refs. I think the net
result would be simpler.

This patch is a lot to digest. I'm not yet confident that I have thought
through all of the ramifications of this patch. I guess a few iterations
will be needed in any case.

By the way, all of the patches preceding this one that I haven't
commented on look OK to me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu