From: Jeff King <peff@peff.net>
Date: Fri, 17 Feb 2017 08:31:12 GMT
Subject: Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
Message-ID: <20170217083112.vn7m4udsopmlvnn5@sigill.intra.peff.net>
In-Reply-To: <20170217035800.13214-4-kyle@kyleam.com>

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.

-Peff