Re: [PATCH] add -p: fix counting empty context lines in edited patches

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 4, 2018, 17:21 UTC
Message-ID: <CAPig+cSc49B4Mn7m9XOp+QF3qcT06ncGGXJpssFJMNjCQ8e7Mw@mail.gmail.com>
In-Reply-To: <36f2d9e0-ba79-64d3-ffb5-d0772cafa153@talktalk.net>

On Mon, Jun 4, 2018 at 6:08 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 13 quoted lines

> On 01/06/18 21:03, Eric Sunshine wrote:
>> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood <phillip.wood@talktalk.net> wrote:
>>> +               } elsif ($mode eq ' ' or $_ eq "\n") {
>>
>> Based upon a very cursory read of parts of git-add-interactive.perl,
>> do I understand correctly that we don't have to worry about $_ ever
>> being "\r\n" on Windows?
>
> Good question, I think the short answer no. If my understanding of the
> newline section of perlport [1] is correct then on Windows "\n" eq
> "\012" and the io layer replaces "\015\012" with "\n" when reading in
> 'text' mode (which I think is the default if you don't specify one when
> opening the file/process or with binmode()).

That was my interpretation, as well (though I didn't audit the code closely).

> As "\n" is only one
> character it would perhaps be better to test '$mode' rather than '$_'
> above - what do you think.

That could be clearer. As a reviewer, I had to spend extra brain cycles wondering why $mode was used everywhere else but $_ in just this one place. (Not that it was difficult to figure out.)

Show 25 quoted lines

>>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>>> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
>>> +test_expect_success 'setup file' '
>>> +       [...]
>>> +'
>>> +test_expect_success 'setup patch' '
>>> +       [...]
>>> +'
>>> +test_expect_success 'setup expected' '
>>> +       [...]
>>> +'
>>> +test_expect_success 'edit can strip spaces from empty context lines' '
>>> +       test_write_lines e n q | git add -p 2>error &&
>>> +       test_must_be_empty error &&
>>> +       git diff >output &&
>>> +       diff_cmp expected output
>>> +'
>>
>> I would have expected all the setup work to be contained directly in
>> the sole test which needs it rather than spread over three tests (two
>> of which are composed of a single command). Not a big deal, and not
>> worth a re-roll.
>
> Good point I was torn between that and matching the existing style in
> that file seems to be to create a million ancillary tests to do the set-up.

I see what you mean. Following existing practice in the file makes sense, though breaking from that practice by bundling all the setup into the single test which uses it wouldn't hurt either. It's a judgment call (and not worrying about too much).