Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Kyle Meyer <kyle@kyleam.com>, git@vger.kernel.org
Subject: Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
Date: Fri, 17 Feb 2017 14:43:50 -0500	[thread overview]
Message-ID: <20170217194350.prhp5joh33cbvwsd@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqk28ou2o1.fsf@gitster.mtv.corp.google.com>

On Fri, Feb 17, 2017 at 09:50:54AM -0800, Junio C Hamano wrote:

> > I see we actually already have a "logmsg" parameter. It already says
> > "Branch: renamed %s to %s". Could we just reuse that? I know that this
> > step is technically just the deletion, but I think it more accurately
> > describes the whole operation that the deletion is part of.
> 
> True, but stepping back a bit,...
> 
> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog?  I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.  So I am not sure if it is a good
> thing to describe the deletion more readably with a message (which
> is what this patch does) in the first place.  If we can just say
> "don't log this deletion event in HEAD's reflog", wouldn't that be
> more desirable?

Yes. I think the options are basically (in order of decreasing
preference in my opinion):

  1. Log a rename entry (same sha1, but note the rename in the free-form
     text).

  2. Log a delete (sha1 goes to null) followed by a creation (from null
     back to the original sha1).

  3. Log nothing at all for HEAD.

This does half of (2). If we do the second half, then I'd prefer it to
(3). But if we can do (1), that is better still (IMHO).

> *1* Is the reason why the code in files_rename_ref() we are looking
>     at does not adjust HEAD to point at the new ref is because it is
>     just handing one ref-store and obviouvious to symrefs in other
>     backends?

I'm actually confused about which bit of code is updating HEAD. I do not
see it either in files_rename_ref() or in the caller. Yet it clearly
happens. But that is the code that would know enough to do (1) or the
second half of (2) above.

-Peff