Subject: Re: [PATCH v2 07/10] t4205: cover `git log --reflog -z` blindspot

From: Denton Liu <[email-masked]>
Date: Fri, 8 Nov 2019 13:47:11 -0800

Hi Eric,

On Fri, Nov 08, 2019 at 03:36:25PM -0500, Eric Sunshine wrote:

[...]

>     for r in $(git rev-list --reflog)
>     do
>         git show -s --pretty="$p" "$r" &&
>         printf "\0" || return 1
>     done >expect &&
> 
> For completeness, the above example also drops the unnecessary 'revs'
> variable, uses double quotes rather than single when interpolating $p,
> and makes the loop early-exit a bit more idiomatic.

The reason why I pulled out `revs` was because putting the rev-list
within the for-loop would cause its exit code to be lost if it ever
failed. Since I've been cleaning up test scripts to not lose exit codes
from git commands, I figured it'd be a bad idea to introduce an instance
here ;)

Thanks for the other two suggestions, though. I didn't know that you
could write it like that.

> > +       git log -z --pretty=oneline --reflog >actual &&
> > +       # no trailing NUL
> 
> To what is this comment referring?

I wanted to point out how in the oneline case, the trailing NUL is
already included, unlike in the multiple cases, so we don't need to
output another one.

I'll rewrite this as

	# the trailing NUL is already produced so we don't need to
	# output another one

Thanks for your feedback,

Denton