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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 &/

> 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?

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux