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.
