Re: [PATCH v7 19/33] refs: allow log-only updates

On Thu, 2016-04-21 at 16:17 +0200, Michael Haggerty wrote:

On 03/01/2016 01:52 AM, David Turner wrote:

quoted

The refs infrastructure learns about log-only ref updates, which
only
update the reflog.  Later, we will use this to separate symbolic
reference resolution from ref updating.

Signed-off-by: David Turner <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 refs/files-backend.c | 15 ++++++++++-----
 refs/refs-internal.h |  7 +++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f565cb..189b86e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -2702,7 +2702,7 @@ static int commit_ref_update(struct ref_lock

*lock,
 			}
 		}
 	}
-	if (commit_ref(lock)) {
+	if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {
 		error("Couldn't set %s", lock->ref_name);
 		unlock_ref(lock);
 		return -1;

@@ -3056,7 +3056,8 @@ static int files_transaction_commit(struct

ref_transaction *transaction,
 			goto cleanup;
 		}
 		if ((update->flags & REF_HAVE_NEW) &&
-		    !(update->flags & REF_DELETING)) {
+		    !(update->flags & REF_DELETING) &&
+		    !(update->flags & REF_LOG_ONLY)) {
 			int overwriting_symref = ((update->type &
REF_ISSYMREF) &&
 						  (update->flags &
REF_NODEREF));
 

@@ -3086,7 +3087,9 @@ static int files_transaction_commit(struct

ref_transaction *transaction,
 				update->flags |= REF_NEEDS_COMMIT;
 			}
 		}
-		if (!(update->flags & REF_NEEDS_COMMIT)) {
+
+		if (!(update->flags & REF_LOG_ONLY) &&
+		    !(update->flags & REF_NEEDS_COMMIT)) {

I was just going over this series again, and I think this hunk is
incorrect. If REF_LOG_ONLY, we created and opened the lockfile. And
we
didn't call write_ref_to_logfile(), so the lockfile is still open.
That
means that we want to call close_ref() here to free up the file
descriptor. (Note that close_ref() closes the lockfile but doesn't
release the lock. That is done further down by unlock_ref().)

So I think this hunk should be omitted.

I realize that this patch series is obsolete, so there is no need to
re-submit. I just wanted to get a sanity check as I implement a new
version of this patch that I'm not misunderstanding something.

I think your logic seems sound, but if you're going to change this,
please make sure tests still pass -- as you know, this area is
something of a minefield.