Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

From: Jeff King <peff@peff.net>
Date: Feb 17, 2017, 08:22 UTC
Message-ID: <20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net>
In-Reply-To: <20170217035800.13214-3-kyle@kyleam.com>

On Thu, Feb 16, 2017 at 10:57:59PM -0500, Kyle Meyer wrote:

Show 5 quoted lines

> Now that delete_refs() accepts a reflog message, pass the
> user-provided message to delete_refs() rather than silently dropping
> it.  The doesn't matter for the deleted ref's log because the log is
> deleted along with the ref, but this entry will show up in HEAD's
> reflog when deleting a checked out branch.

Sounds good.

Show 10 quoted lines

> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index a41f9adf1..f642acc22 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>  		 */
>  		return delete_ref(refname,
>  				  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
> -				  flags, NULL);
> +				  flags, msg);

This looks obviously correct.

Show 16 quoted lines

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index b0ffc0b57..65918d984 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>  '
>  rm -f .git/$m
>  
> +test_expect_success "deleting current branch adds message to HEAD's log" '
> +	git update-ref $m $A &&
> +	git symbolic-ref HEAD $m &&
> +	git update-ref -mdelmsg -d $m &&
> +	! test -f .git/$m &&
> +	grep "delmsg$" .git/logs/HEAD >/dev/null
> +'
> +rm -f .git/$m

I think covering this with a test is good.

I don't know if it's also worth testing that deleting via HEAD also writes the reflog. I.e.,:

  git update-ref -m delete-by-head -d HEAD

Some of the style here is a bit out-dated, but I think you are just matching the surrounding tests. So that's OK by me (though a patch to modernize the whole thing would be welcome, too).

For reference, the two things I notice are:

  - we prefer test_path_is_missing to "! test -f" these days.

  - we don't redirect the output of grep (it's handled already in
    non-verbose mode, and in verbose mode we try to be...verbose).

-Peff