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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/01/2016 01:52 AM, David Turner wrote:
> 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 <dturner@twopensource.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  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.

> [...]

Michael