add-patch: handle splitting hunks with diff.suppressBlankEmpty

[phillipwood]

This is an alternative to jk/add-patch-with-suppress-blank-empty which was recently discarded from next. I hope that normalizing the context marker will simplify any future changes to the code.

Changes since V1

• Updated merge_hunks() to use normalize_marker() as spotted by Junio
• Updated the test so it checks merge_hunks() as well.

Cc: Jeff King peff@peff.net Cc: Junio C Hamano gitster@pobox.com cc: Phillip Wood phillip.wood123@gmail.com

[phillipwood]

/submit

[gitgitgadget[bot]]

Submitted as pull.1763.git.1721312619822.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1763/phillipwood/add-p-suppress-blank-empty-v1

To fetch this version to local tag pr-1763/phillipwood/add-p-suppress-blank-empty-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1763/phillipwood/add-p-suppress-blank-empty-v1

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When "add -p" parses diffs, it looks for context lines starting with a
> single space. But when diff.suppressBlankEmpty is in effect, an empty
> context line will omit the space, giving us a true empty line. This
> confuses the parser, which is unable to split based on such a line.
>
> It's tempting to say that we should just make sure that we generate a
> diff without that option.  However, although we do not parse hunks that
> the user has manually edited with parse_diff() we do allow the user
> to split such hunks. As POSIX calls the decision of whether to print the
> space here "implementation-defined" we need to handle edited hunks where
> empty context lines omit the space.
>
> So let's handle both cases: a context line either starts with a space or
> consists of a totally empty line by normalizing the first character to a
> space when we parse them. Normalizing the first character rather than
> changing the code to check for a space or newline will hopefully future
> proof against introducing similar bugs if the code is changed.
>
> Reported-by: Ilya Tumaykin <itumaykin@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

Well written.  Thanks for a pleasant read.

> @@ -953,7 +960,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>   context_line_count = 0;
>  
>   while (splittable_into > 1) {
> -     ch = s->plain.buf[current];
> +     ch = normalize_marker(s->plain.buf + current);

I wondered if &s->plain.buf[current] is easier to grok, but what's
written already is good enough ;-)

There is another explicit mention of ' ' in merge_hunks().  Unless
we are normalizing the buffer here (which I do not think we do),
wouldn't we have to make sure that the code over there also knows
that an empty line in a patch is an unmodified empty line?

                if (plain[overlap_end] != ' ')
                        return error(_("expected context line "
                                       "#%d in\n%.*s"),

[gitgitgadget[bot]]

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Junio

On 18/07/2024 17:29, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When "add -p" parses diffs, it looks for context lines starting with a
>> single space. But when diff.suppressBlankEmpty is in effect, an empty
>> context line will omit the space, giving us a true empty line. This
>> confuses the parser, which is unable to split based on such a line.
>>
>> It's tempting to say that we should just make sure that we generate a
>> diff without that option.  However, although we do not parse hunks that
>> the user has manually edited with parse_diff() we do allow the user
>> to split such hunks. As POSIX calls the decision of whether to print the
>> space here "implementation-defined" we need to handle edited hunks where
>> empty context lines omit the space.
>>
>> So let's handle both cases: a context line either starts with a space or
>> consists of a totally empty line by normalizing the first character to a
>> space when we parse them. Normalizing the first character rather than
>> changing the code to check for a space or newline will hopefully future
>> proof against introducing similar bugs if the code is changed.
>>
>> Reported-by: Ilya Tumaykin <itumaykin@gmail.com>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
> > Well written.  Thanks for a pleasant read.

Thanks to Peff for letting me steal his commit message

>> @@ -953,7 +960,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>>      context_line_count = 0;
>>   >>     while (splittable_into > 1) {
>> -        ch = s->plain.buf[current];
>> +        ch = normalize_marker(s->plain.buf + current);
> > I wondered if &s->plain.buf[current] is easier to grok, but what's
> written already is good enough ;-)

Yes I think that would be better

> There is another explicit mention of ' ' in merge_hunks().  Unless
> we are normalizing the buffer here (which I do not think we do),
> wouldn't we have to make sure that the code over there also knows
> that an empty line in a patch is an unmodified empty line?
> >                  if (plain[overlap_end] != ' ')
>                          return error(_("expected context line "
>                                         "#%d in\n%.*s"),

Oh dear, I'm not sure how I missed that. I'll fix that and update the test to make sure it exercises that code path as well.

Thanks for your comments

Phillip

[gitgitgadget[bot]]

User Phillip Wood <phillip.wood123@gmail.com> has been added to the cc: list.

[phillipwood]

/submit

[gitgitgadget[bot]]

Submitted as pull.1763.v2.git.1721491320.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1763/phillipwood/add-p-suppress-blank-empty-v2

To fetch this version to local tag pr-1763/phillipwood/add-p-suppress-blank-empty-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1763/phillipwood/add-p-suppress-blank-empty-v2

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/19a89823f56d762ec4c1cc83ac6e8ece1f9c699d.

[gitgitgadget[bot]]

This branch is now known as pw/add-patch-with-suppress-blank-empty.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/e2c5f0e190ee5d606fdfaaac18ee3b998f580d49.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/cac62ee37c905211c3e25ec62982f3a8f4809faf.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/5437b7dee316b795f5a11610defaf2c097a444a8.

[gitgitgadget[bot]]

There was a status update in the "New Topics" section about the branch pw/add-patch-with-suppress-blank-empty on the Git mailing list:

"git add -p" by users with diff.suppressBlankEmpty set to true
failed to parse the patch that represents an unmodified empty line
with an empty line (not a line with a single space on it), which
has been corrected.

Will cook in 'next'.
source: <pull.1763.v2.git.1721491320.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/762f44ec9db66da7ecd9e8237eb7f56e0d02be48.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/33e57fc29bafc7c42c55f874f4de437fc2bb6070.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/8e5cc97079f50e2459ac3e60f4f805bb18db5b2f.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch pw/add-patch-with-suppress-blank-empty on the Git mailing list:

"git add -p" by users with diff.suppressBlankEmpty set to true
failed to parse the patch that represents an unmodified empty line
with an empty line (not a line with a single space on it), which
has been corrected.

Will cook in 'next'.
source: <pull.1763.v2.git.1721491320.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/8a1826d095381f5d56c52736fde8c8e5fd1a9401.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/3e5288631d244a0f89d51b1ce57f82c93fd3824d.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch pw/add-patch-with-suppress-blank-empty on the Git mailing list:

"git add -p" by users with diff.suppressBlankEmpty set to true
failed to parse the patch that represents an unmodified empty line
with an empty line (not a line with a single space on it), which
has been corrected.

Will merge to 'master'.
source: <pull.1763.v2.git.1721491320.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/32c5cf10396cedac0d078669708ad60c4489fec9.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/d71121c060dc1a76cb5be1674e4f719f5c58136e.

[gitgitgadget[bot]]

This patch series was integrated into master via https://github.com/git/git/commit/d71121c060dc1a76cb5be1674e4f719f5c58136e.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/d71121c060dc1a76cb5be1674e4f719f5c58136e.

[gitgitgadget[bot]]

Closed via d71121c060dc1a76cb5be1674e4f719f5c58136e.