Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Jeff King <hidden>
Date: 2017-02-17 08:23:00
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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