git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Eric Sunshine <sunshine@sunshineco.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Junio C Hamano <gitster@pobox.com>,
	Martin Agren <martin.agren@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Oliver Joseph Ash <oliverjash@gmail.com>,
	Mahmoud Al-Qudsi <mqudsi@neosmart.net>,
	Jeff Felchner <jfelchner1@gmail.com>
Subject: Re: [PATCH] add -p: fix counting empty context lines in edited patches
Date: Mon, 4 Jun 2018 11:08:39 +0100	[thread overview]
Message-ID: <36f2d9e0-ba79-64d3-ffb5-d0772cafa153@talktalk.net> (raw)
In-Reply-To: <CAPig+cSSj2ETXfk8FYUc+=tE6bfoRuqF5Ld4kOgE4+DDpfL+BA@mail.gmail.com>

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



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

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-15 12:21 Regression in patch add? mqudsi
2018-04-15 13:59 ` Martin Ågren
2018-04-16 10:01   ` Phillip Wood
2018-04-16 10:00 ` Phillip Wood
2018-05-10 10:41 ` Oliver Joseph Ash
2018-05-10 12:17   ` Martin Ågren
2018-05-10 13:16     ` Oliver Joseph Ash
2018-05-10 13:54       ` Martin Ågren
2018-05-10 13:49     ` Phillip Wood
2018-05-10 14:11       ` Oliver Joseph Ash
2018-05-10 17:58         ` Phillip Wood
2018-05-11  2:47           ` Junio C Hamano
2018-05-11 18:23             ` Phillip Wood
2018-05-10 13:15 ` Oliver Joseph Ash
2018-06-01 17:46 ` [PATCH] add -p: fix counting empty context lines in edited patches Phillip Wood
2018-06-01 19:07   ` Jacob Keller
2018-06-01 20:03   ` Eric Sunshine
2018-06-04 10:08     ` Phillip Wood [this message]
2018-06-04 17:21       ` Eric Sunshine
2018-06-11  9:46   ` [PATCH v2] " Phillip Wood
2018-07-11 20:27     ` Jeff Felchner
2018-07-11 20:50       ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=36f2d9e0-ba79-64d3-ffb5-d0772cafa153@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jfelchner1@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=mqudsi@neosmart.net \
    --cc=oliverjash@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=sunshine@sunshineco.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).