Re: [PATCH v2] t4150: fix broken test for am --scissors

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 6, 2018, 15:04 UTC
Message-ID: <xmqqpnyvr0yn.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CACRoPnTWxYvGfqgGt2r5ETOhfJZnqr1fTO9Fzwp-u25XbMGPPQ@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 25 quoted lines

> I've taken a look at the original test, and it is pretty broken. My
> ...
> So, there are 3 problems that will need to be fixed.
> ...
> This fixes problem (3) by using an in-body header.
> ...
> This fixes the first half of problem (2) by making the naming of the
> files the same.
> ...
> Nit: I'm not quite sure about naming the tag "scissors-used" though,
> since this commit was not created from the output of "git am
> --scissors". Maybe it should be named `commit-without-scissors-line`
> or something?
>
> This hunk removes the line:
>
>     git format-patch --stdout scissors^ >scissors-patch.eml &&
>
> without a corresponding replacement, but that is fine because the test
> should not be using a patch without a scissors line.
> ...
> This fixes the other half of problem (2) by making the naming of the
> files the same.
> ...
> So, this patch fixes the 3 problems with the tests, and so looks correct to me.

Beautifully explained. There are many styles of patch review, and I'd personally call this "think aloud to follow the patch author's flow of thought." style. Your review is probably one of the best examples of reviews in the style. Very readable, helping readers to reach the same degree of understanding of what problem the patch tries to address and how, and demonostrates the reviewer thought through the problem just as deeply as the patch author, all of which gives weight to the final "looks correct to me".

Thanks, both.  Will queue.