git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: 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 13:21:04 -0400	[thread overview]
Message-ID: <CAPig+cSc49B4Mn7m9XOp+QF3qcT06ncGGXJpssFJMNjCQ8e7Mw@mail.gmail.com> (raw)
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:
> 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.)

>>> 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).

  reply	other threads:[~2018-06-04 17:21 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
2018-06-04 17:21       ` Eric Sunshine [this message]
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=CAPig+cSc49B4Mn7m9XOp+QF3qcT06ncGGXJpssFJMNjCQ8e7Mw@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --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 \
    /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).