Re: [Outreachy][PATCH v6] t6050: avoid pipes with upstream Git commands

From: Chizoba ODINAKA <chizobajames21@gmail.com>
Date: Oct 15, 2024, 11:07 UTC
Message-ID: <CACwP9aqecUKmN37JYW2qYRhe6nw-Owp5Cp-6Xjb1k7=_zaYmbQ@mail.gmail.com>
In-Reply-To: <Zw2T3PmdbgRGrIdF@nand.local>

Hi Taylor

On Mon, 14 Oct 2024 at 22:57, Taylor Blau <me@ttaylorr.com> wrote:

Show 11 quoted lines

>
> On Mon, Oct 14, 2024 at 04:24:11PM +0100, chizobajames21@gmail.com wrote:
> >  test_expect_success 'test GIT_NO_REPLACE_OBJECTS env variable' '
> > -     GIT_NO_REPLACE_OBJECTS=1 git cat-file commit $HASH2 | grep "author A U Thor" &&
> > -     GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 | grep "A U Thor"
> > +     GIT_NO_REPLACE_OBJECTS=1 git cat-file commit $HASH2 >actual &&
> > +     test_grep "author A U Thor"  actual &&
>
> Nit: it looks like there is an extra space between the closing '"' quote
> character and the filename 'actual'.
>

Thanks for spotting this out. I will fix this up in the next patch.

Show 15 quoted lines

> > @@ -284,8 +313,8 @@ test_expect_success 'bisect and replacements' '
> >  '
> >
> >  test_expect_success 'index-pack and replacements' '
> > -     git --no-replace-objects rev-list --objects HEAD |
> > -     git --no-replace-objects pack-objects test- &&
> > +     git --no-replace-objects rev-list --objects HEAD >actual &&
> > +     git --no-replace-objects pack-objects test- <actual &&
> >       git index-pack test-*.pack
> >  '
>
> Hmm. In other instances, actual seems like an OK name choice, but here I
> wonder if 'in' would be more appropriate, since we're feeding it as
> input to another Git command.
>

Given that it's actually (puns intended) an output from the Git command in the preceding line, isn't a neutral name, like 'actual' a better choice than 'in'.

Show 5 quoted lines

> Other than those couple of comments, these all look pretty reasonable to
> me.
>
> Thanks,
> Taylor

Thanks for the comments. Chizoba