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

From: Jeff King <peff@peff.net>
Date: Feb 17, 2017, 08:31 UTC
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:

Show 6 quoted lines

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

Show 6 quoted lines

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