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

[Jeff King]

On Fri, Feb 17, 2017 at 06:40:28PM -0500, Kyle Meyer wrote:

> > 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).
> 
> Would moving cleanup like "rm -f .git/$m" within the test's body using
> test_when_finished also be preferred?

Yeah, that too.  I forgot to mention it.

-Peff

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

[Kyle Meyer]

Jeff King  writes:

[...]

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

Seems reasonable to cover this case as well.

> 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).

Right.  I'd be happy to follow up with a patch updating the style in
t1400-update-ref.sh.

> 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).

Would moving cleanup like "rm -f .git/$m" within the test's body using
test_when_finished also be preferred?

-- 
Kyle

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

[Junio C Hamano]

Jeff King  writes:

> ...

All good review comments.

I briefly wondered if recording the deletion of the current branch
in HEAD's reflog has much practical values (Porcelain "git branch"
would not even allow deletion in the first place), but because it is
a rare and unusual event, it probably makes more sense to have a
record of it.

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

[Jeff King]

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

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

[Kyle Meyer]

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.

Signed-off-by: Kyle Meyer 
---
I'm not sure if the test here (or in the next patch) is worth including.

 builtin/update-ref.c  | 2 +-
 t/t1400-update-ref.sh | 9 +
 2 files changed, 10 insertions(+), 1 deletion(-)

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);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
  flags | create_reflog_flag,
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
+
 test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
git update-ref $outside $A &&
-- 
2.11.1