git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Paul Tan <pyokagan@gmail.com>
Cc: Andrei Rybak <rybak.a.v@gmail.com>,
	Git List <git@vger.kernel.org>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH v2] t4150: fix broken test for am --scissors
Date: Mon, 06 Aug 2018 08:04:00 -0700	[thread overview]
Message-ID: <xmqqpnyvr0yn.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CACRoPnTWxYvGfqgGt2r5ETOhfJZnqr1fTO9Fzwp-u25XbMGPPQ@mail.gmail.com> (Paul Tan's message of "Mon, 6 Aug 2018 16:58:52 +0800")

Paul Tan <pyokagan@gmail.com> writes:

> 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.


  reply	other threads:[~2018-08-06 15:04 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-03 13:38 [RFC] broken test for git am --scissors Andrei Rybak
2018-08-03 22:39 ` [PATCH] t4150: fix broken test for " Andrei Rybak
2018-08-03 23:04   ` Junio C Hamano
2018-08-04  0:39     ` Andrei Rybak
2018-08-04 18:10   ` [PATCH v2] " Andrei Rybak
2018-08-06  8:58     ` Paul Tan
2018-08-06 15:04       ` Junio C Hamano [this message]
2018-08-06 17:42       ` Andrei Rybak
2018-08-07 10:53         ` Paul Tan
2018-08-06 17:49     ` [PATCH v3] " Andrei Rybak
2018-08-06 20:14       ` Junio C Hamano
2018-08-07 11:24         ` Andrei Rybak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqpnyvr0yn.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=pyokagan@gmail.com \
    --cc=rybak.a.v@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).