From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 08 Nov 2019 21:58:40 GMT
Subject: Re: [PATCH v2 07/10] t4205: cover `git log --reflog -z` blindspot
Message-ID: <CAPig+cQ3FrxUGiFXooaiZ+po0qK6tBOda5apCs4=H0mdWxAKOg@mail.gmail.com>
In-Reply-To: <20191108214711.GA27310@generichostname>

On Fri, Nov 8, 2019 at 4:47 PM Denton Liu <liu.denton@gmail.com> wrote:
> On Fri, Nov 08, 2019 at 03:36:25PM -0500, Eric Sunshine wrote:
> > 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 ;)

Make sense. Ignore that suggestion of mine.

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

That will be clearer. It might be even clearer if that comment comes
before the "git log -z" invocation rather than after (though that's
subjective). Thanks.