Re: [PATCH] add -p: fix counting empty context lines in edited patches
On Mon, Jun 4, 2018 at 6:08 AM, Phillip Wood wrote:
> On 01/06/18 21:03, Eric Sunshine wrote:
>> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood
>> 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.)
>>> 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).
Re: [PATCH] add -p: fix counting empty context lines in edited patches
On 01/06/18 21:03, Eric Sunshine wrote:
> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood
> 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//
> --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
>> ---
>> 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
Re: [PATCH] add -p: fix counting empty context lines in edited patches
On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood 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//
--or--
s/consist/that &/
> well as lines starting with a space as context lines and add a test to
> prevent future regressions.
>
> Signed-off-by: Phillip Wood
> ---
> 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?
> 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.
Re: [PATCH] add -p: fix counting empty context lines in edited patches
On Fri, Jun 1, 2018 at 10:46 AM, Phillip Wood wrote:
> From: Phillip Wood
>
> 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
> well as lines starting with a space as context lines and add a test to
> prevent future regressions.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
>
> Reported-by: Mahmoud Al-Qudsi
> Reported-by: Oliver Joseph Ash
> Reported-by: Jeff Felchner
> Signed-off-by: Phillip Wood
> ---
> My apologies to everyone who was affected by this regression.
>
Ahhh I suspect this is why my edited code in add -p was sometimes
failing to apply!
Thanks,
Jake
> git-add--interactive.perl | 2 +-
> t/t3701-add-interactive.sh | 43 ++
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index ab022ec073..bb6f249f03 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
> $o_cnt++;
> } elsif ($mode eq '+') {
> $n_cnt++;
> - } elsif ($mode eq ' ') {
> + } elsif ($mode eq ' ' or $_ eq "\n") {
> $o_cnt++;
> $n_cnt++;
> }
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index e5c66f7500..f1bb879ea4 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
> diff_cmp expected output
> '
>
> +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
> + @@ -1,4 +1,4 @@
> +a
> + $NULL
> + -b
> + +f
> + $SP
> + c
> + EOF
> +'
> +
> +test_expect_success 'setup expected' '
> + cat >expected <<-EOF
> + diff --git a/file b/file
> + index b5dd6c9..f910ae9 100644
> + --- a/file
> + +++ b/file
> + @@ -1,5 +1,5 @@
> +a
> + $SP
> + -f
> + +d
> + $SP
> +c
> + 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
> +'
> +
> test_expect_success 'skip files similarly as commit -a' '
> git reset &&
> echo file >.gitignore &&
> --
> 2.17.0
>
[PATCH] add -p: fix counting empty context lines in edited patches
From: Phillip Wood
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
well as lines starting with a space as context lines and add a test to
prevent future regressions.
[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
Reported-by: Mahmoud Al-Qudsi
Reported-by: Oliver Joseph Ash
Reported-by: Jeff Felchner
Signed-off-by: Phillip Wood
---
My apologies to everyone who was affected by this regression.
git-add--interactive.perl | 2 +-
t/t3701-add-interactive.sh | 43 ++
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ab022ec073..bb6f249f03 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
$o_cnt++;
} elsif ($mode eq '+') {
$n_cnt++;
- } elsif ($mode eq ' ') {
+ } elsif ($mode eq ' ' or $_ eq "\n") {
$o_cnt++;
$n_cnt++;
}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index e5c66f7500..f1bb879ea4 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
diff_cmp expected output
'
+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
+ @@ -1,4 +1,4 @@
+a
+ $NULL
+ -b
+ +f
+ $SP
+ c
+ EOF
+'
+
+test_expect_success 'setup expected' '
+ cat >expected <<-EOF
+ diff --git a/file b/file
+ index b5dd6c9..f910ae9 100644
+ --- a/file
+ +++ b/file
+ @@ -1,5 +1,5 @@
+a
+ $SP
+ -f
+ +d
+ $SP
+c
+ 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
+'
+
test_expect_success 'skip files similarly as commit -a' '
git reset &&
echo file >.gitignore &&
--
2.17.0
