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

From: Junio C Hamano <[email-masked]>
Date: Fri, 17 Feb 2017 09:50:54 -0800

Jeff King <[email-masked]> writes:

> On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:
>
>> When the current branch is renamed, the deletion of the old ref is
>> recorded in HEAD's log with an empty message.  Now that delete_refs()
>> accepts a reflog message, provide a more descriptive message.  This
>> message will not be included in the reflog of the renamed branch, but
>> its log already covers the renaming event with a message of "Branch:
>> renamed ...".
>
> Right, makes sense. The code overall looks fine, though I have one
> nit:
>
>> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
>>  		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
>>  			oldrefname, strerror(errno));
>>  
>> -	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
>> +	strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);
>
> We've been anything but consistent with our reflog messages in the past,
> but I think the general guideline for new ones is to use "command:
> action". Of course we don't _know_ the action here, because we're deep
> in rename_ref().
>
> Should we have the caller pass it in for us, as we do with delete_ref()
> and update_ref()?
>
> 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?

[Footnote]

*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?