On 01/06/18 21:03, Eric Sunshine wrote:
> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood <phillip.wood@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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
