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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2017, 17:50 UTC
Message-ID: <xmqqk28ou2o1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170217083112.vn7m4udsopmlvnn5@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 31 quoted lines

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