Re: [PATCH v4 14/21] refs: always handle non-normal refs in files backend
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:08:12
On 02/05/2016 08:44 PM, David Turner wrote:
quoted hunk ↗ jump to hunk
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. You could
fix that and also avoid gratuitously changing the order of the updates
like this:
static int move_abnormal_ref_updates(struct ref_transaction *transaction,
struct ref_transaction *files_transaction,
struct strbuf *err)
{
int i, normal_nr = 0;
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
if (ref_type(update->refname) == REF_TYPE_NORMAL)
transaction->updates[normal_nr++] = update;
else
add_update_obj(files_transaction, update);
}
transaction->nr = normal_nr;
return 0;
}
Another alternative would be to set
update->flags |= REF_ABNORMAL
for the abnormal references and *leave them* in the original transaction
while also adding them to files_transactions. Then teach non-files
backends to skip over updates with REF_ABNORMAL.
The reason I thought of this is that long-term I'd like to make it
possible for some reference updates to fail without aborting the
transaction. To implement that, we would want a way for
ref_transaction_commit() to report back to its caller *which* updates
failed, and an obvious way to do that would be to store the
reference-specific errors in struct ref_update. If you leave the
abnormal ref_updates in the main transaction, then my hoped-for change
would be easier.
But that's a separate and hypothetical idea, so you don't have to let it
influence how you implement this patch.
quoted hunk ↗ jump to hunk
int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { int ret = -1; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; + struct string_list files_affected_refnames = STRING_LIST_INIT_NODUP; + struct ref_transaction *files_transaction = NULL; assert(err);@@ -1237,6 +1275,26 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (ret) goto done; + if (the_refs_backend != &refs_be_files) { + files_transaction = ref_transaction_begin(err); + if (!files_transaction) + goto done; + + ret = move_abnormal_ref_updates(transaction, files_transaction, + err); + if (ret) + goto done; + + /* files backend commit */ + if (get_affected_refnames(files_transaction, + &files_affected_refnames, + err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto done; + } + } + + /* main backend commit */ if (get_affected_refnames(transaction, &affected_refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; goto done;@@ -1244,8 +1302,24 @@ int ref_transaction_commit(struct ref_transaction *transaction, ret = the_refs_backend->transaction_commit(transaction, &affected_refnames, err); + if (ret) + goto done; + + if (files_transaction) { + ret = refs_be_files.transaction_commit(files_transaction, + &files_affected_refnames, + err); + if (ret) { + warning(split_transaction_fail_warning); + goto done; + } + } + done: string_list_clear(&affected_refnames, 0); + string_list_clear(&files_affected_refnames, 0); + if (files_transaction) + ref_transaction_free(files_transaction); return ret; }@@ -1285,6 +1359,9 @@ int peel_ref(const char *refname, unsigned char *sha1) int create_symref(const char *ref_target, const char *refs_heads_master, const char *logmsg) { + if (ref_type(ref_target) != REF_TYPE_NORMAL) + return refs_be_files.create_symref(ref_target, refs_heads_master, + logmsg); return the_refs_backend->create_symref(ref_target, refs_heads_master, logmsg); }
Does initial_ref_transaction_commit() need the same treatment? I think files_rename_ref() will break if one of the references is normal and one is abnormal. I think it would be OK to prohibit renaming abnormal refs entirely (can anybody think of an important use case?), but that function should at least do the checks. Michael -- Michael Haggerty mhagger@alum.mit.edu