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

[Phillip Wood <phillip.wood@talktalk.net>]

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:
>> recount_edited_hunk() introduced in commit 2b8ea7f3c7 ("add -p:
>> calculate offset delta for edited patches", 2018-03-05) required all
>> context lines to start with a space, empty lines are not counted. This
>> was intended to avoid any recounting problems if the user had
>> introduced empty lines at the end when editing the patch. However this
>> introduced a regression into 'git add -p' as it seems it is common for
>> editors to strip the trailing whitespace from empty context lines when
>> patches are edited thereby introducing empty lines that should be
>> counted. 'git apply' knows how to deal with such empty lines and POSIX
>> states that whether or not there is an space on an empty context line
>> is implementation defined [1].
>>
>> Fix the regression by counting lines consist solely of a newline as
> 
> s/consist/&ing/
> --or--
> s/consist/that &/

Thanks, I'd intended to say 'that consist'

>> well as lines starting with a space as context lines and add a test to
>> prevent future regressions.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>  git-add--interactive.perl  |  2 +-
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
>> -               } elsif ($mode eq ' ') {
>> +               } 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()). As "\n" is only one
character it would perhaps be better to test '$mode' rather than '$_'
above - what do you think.

[1] http://perldoc.perl.org/perlport.html#Newlines

>> 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_write_lines a "" b "" c >file &&
>> +       git add file &&
>> +       test_write_lines a "" d "" c >file
>> +'
>> +
>> +test_expect_success 'setup patch' '
>> +       SP=" " &&
>> +       NULL="" &&
>> +       cat >patch <<-EOF
>> +       [...]
>> +       EOF
>> +'
>> +
>> +test_expect_success 'setup expected' '
>> +       cat >expected <<-EOF
>> +       [...]
>> +       EOF
>> +'
>> +
>> +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.

Thanks

Phillip