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

[David Turner <dturner@twopensource.com>]

On Thu, 2016-04-21 at 16:17 +0200, Michael Haggerty wrote:
> 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.

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.