On Thu, Feb 16, 2017 at 10:57:59PM -0500, Kyle Meyer wrote:
> 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.
> 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.
> 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
