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

From: jonathan chang <ttjtftx@gmail.com>
Date: Mar 31, 2019, 05:17 UTC
Message-ID: <CAOAu_YKMuy2vWQqZz+=-7JGhce9BEdMqsr8HOKZOJVq=chpqVw@mail.gmail.com>
In-Reply-To: <20190330193842.GR32487@hank.intra.tgummerer.com>

On Sun, Mar 31, 2019 at 3:38 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:

Show 20 quoted lines

>
> On 03/30, Jonathan Chang wrote:
> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> > ---
> >  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. 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.

Show 15 quoted lines

>
> > -     tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
> > -     test "z$tree" = "z$P"
> > +     git show --pretty=raw $commit0 >output &&
> > +     echo "$P" >expect &&
> > +     sed -n -e "s/^tree //p" -e "/^author /q" output >actual &&
>
> I'd find it a bit more natural to either first create the expect file,
> and then do the 'git show' and 'sed' invocations in two subsequent
> lines, or do them first, and then create the expect files, rather than
> interleaving them.
>
> I'm not sure either of these by itself is worth a new iteration,
> unless there is also something else to fix up.  But it's something
> that you might want to keep in mind for future patches.

I thought about all three options, starting with expect files being the last, but found that most test_cmp seem to have expect file created first, then decided to make the creation of actual and expect be on consecutive lines.

Now I would probably move the creation expect to the last line. Thanks for the review and suggestions.

Show 19 quoted lines

>
> > +     test_cmp expect actual
> >  '
> >
> >  test_expect_success 'git commit-tree records the correct parent in a commit' '
> >       commit1=$(echo NO | git commit-tree $P -p $commit0) &&
> > -     git show --pretty=raw $commit1 >actual &&
> > -     parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) &&
> > -     test "z$commit0" = "z$parent"
> > +     git show --pretty=raw $commit1 >output &&
> > +     echo "$commit0" >expect &&
> > +     sed -n -e "s/^parent //p" -e "/^author /q" output >actual &&
> > +     test_cmp expect actual
> >  '
> >
> >  test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> > --
> > 2.21.0
> >