Re: [GSOC][PATCH] t4121: modernize test style

From: Vivan Garg <v.garg.work@gmail.com>
Date: Feb 21, 2023, 19:48 UTC
Message-ID: <CADupsJNPVm13iSv84SXpqtP8Dhm_-htPTr59yPs_J=aQzs2FwQ@mail.gmail.com>
In-Reply-To: <38cb184d-e47a-2129-a93e-16ffd2afe67a@github.com>

On Tue, Feb 21, 2023 at 10:27 AM Victoria Dye <vdye@github.com> wrote:

Show 6 quoted lines

>
> nit: period at the end of the sentence (s/lines/lines.)
>
> Also, this commit message explains *why* you're making the change, but not
> what the commit actually does (that is, update the tests to adhere to the
> new style). Would you mind adding a note about that to the message?

For sure! Is it correct that I will need to amend the commit message and send out a v2 of the patch?

Show 13 quoted lines

> >
> > -test_expect_success \
> > -     'check if contextually independent diffs for the same file apply' \
> > -     '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply'
> > +test_expect_success 'check if contextually independent diffs for the same file apply' '
> > +     ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply
> > +'
>
> As for this one, the test is correctly updated to the new style (per the
> microproject prompt). However, the spacing around the '|' is a little weird
> - I think there should be a space after ')'. On your next re-roll, could you
> fix that spacing (in this patch is fine - it's not a substantial enough
> change to warrant its own commit)?

I'm not sure what you mean by "next re-roll," are you referring to v2? But then you said it was fine in this patch, so I'm confused. If I am going to be sending a v2, couldn't I just revert the last commit and add this change to the same commit?