Web lists-archives.com

Re: [GSoC][PATCH v4 4/5] t0000: use test_cmp instead of "test" builtin




On 03/31, jonathan chang wrote:
> On Sun, Mar 31, 2019 at 3:38 AM Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
> >
> > On 03/30, Jonathan Chang wrote:
> > > Signed-off-by: Jonathan Chang <ttjtftx@xxxxxxxxx>
> > > ---
> > >  t/t0000-basic.sh | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> > > index 3de13daabe..49923c5ff1 100755
> > > --- a/t/t0000-basic.sh
> > > +++ b/t/t0000-basic.sh
> > > @@ -1118,16 +1118,18 @@ P=$(test_oid root)
> > >
> > >  test_expect_success 'git commit-tree records the correct tree in a commit' '
> > >       commit0=$(echo NO | git commit-tree $P) &&
> > > -     git show --pretty=raw $commit0 >actual &&
> >
> > This line has been introduced in a previous commit.  If the file was
> > called 'output' there already, I think that patch would be just as
> > understandable, but this diff would be a little less noisy.
> 
> Make sense. I tried to make patches from last iteration untouched,
> so I don't break anything.

This is what running tests, and reviewing your own code are good for :)

>                             And I was wondering since I'm only
> appending patches, if I should also append the PATCH number as
> [PATCH v3 4/3], to reduce the number of emails. Now I realize that
> by making it a new iteration, we can also make some improvement
> to reduce the patch noise.

Right, appending patches using 4/3 for example can be done in some
rare cases, but since you already modified patch 2/2, a new new
iteration is required anyway.

So yeah improving the overall series is always a good thing.
Especially since I see you already included the range-diff, it's not
very hard for reviewers to see what changed in the last iteration.