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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> > ---
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html