Jeff King <peff@xxxxxxxx> 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?