Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 10, 2024, 17:06 UTC
- Message-ID
- <xmqqed81xjl7.fsf@gitster.g>
- In-Reply-To
- <20240710093610.GA2076910@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> I'm a little worried that this creates ambiguities, since I don't think > we are careful about following the hunk header's line counts. Imagine > you had an input like this: > > @@ -1,2 +1,2 @@ > -old > +new > stuff > > some garbage
True. Especially if you allow editing of hunks, the only thing you could make available to you are the version you gave the user and the version you got back from the user, but comparing them would not help resolve the ambiguity to correct the hunk header all that much. "diff" edit mode various editors may have _could_ help as they know each and every keystroke by the user and how the modified patch was constructed to guess the intention better than a mere comparison of before- and after- shape of the patch (but the last time I checked, diff edit mode of GNU Emacs did not adjust the hunk header correctly in some corner cases).
Show 6 quoted lines
> I don't think we'd ever generate this ourselves, but could somebody > manually edit a hunk into this shape? When I tried it in practice, it > looks like we fail to apply the result even before my patch, though. I'm > not sure why that is. If I put "some garbage" without the blank line, we > correctly realize it should be discarded. It's possible I'm just holding > it wrong.
I've given up on the "hunk edit" doing wrong things to a patch already.
The "edit" codepath used to be a lot less careless before Phillip whipped it into a much better shape with the series that ended at 436d18f2 (Merge branch 'pw/add-p-recount', 2018-03-14), that introduced recount_edited_hunk() that special cases "+/-/ ". It already knows that a completely empty line in a patch is an empty and unmodified line (which was ported from f4d35a6b (add -p: fix counting empty context lines in edited patches, 2018-06-11)), so this patch does not have to do anything new there.
But "recounting" will be fooled by garbage left in the edited result, so I think we have done all we can do to resolve possible ambiguities. The patch under discussion is not adding anything new and making it any worse, I would say.
Show 80 quoted lines
> diff --git a/add-patch.c b/add-patch.c
> index 6e176cd21a..7beead1d0a 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -588,7 +588,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> (int)(eol - (plain->buf + file_diff->head.start)),
> plain->buf + file_diff->head.start);
>
> - if ((marker == '-' || marker == '+') && *p == ' ')
> + if ((marker == '-' || marker == '+') && (*p == ' ' || *p == '\n'))
> hunk->splittable_into++;
> if (marker && *p != '\\')
> marker = *p;
> @@ -964,7 +964,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
> * Is this the first context line after a chain of +/- lines?
> * Then record the start of the next split hunk.
> */
> - if ((marker == '-' || marker == '+') && ch == ' ') {
> + if ((marker == '-' || marker == '+') && (ch == ' ' || ch == '\n')) {
> first = 0;
> hunk[1].start = current;
> if (colored)
> @@ -979,14 +979,14 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
> * Then just increment the appropriate counter and continue
> * with the next line.
> */
> - if (marker != ' ' || (ch != '-' && ch != '+')) {
> + if ((marker != ' ' && marker != '\n') || (ch != '-' && ch != '+')) {
> next_hunk_line:
> /* Comment lines are attached to the previous line */
> if (ch == '\\')
> ch = marker ? marker : ' ';
>
> /* current hunk not done yet */
> - if (ch == ' ')
> + if (ch == ' ' || ch == '\n')
> context_line_count++;
> else if (ch == '-')
> header->old_count++;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 5d78868ac1..92c8e6dc8c 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1164,4 +1164,36 @@ test_expect_success 'reset -p with unmerged files' '
> test_must_be_empty staged
> '
>
> +test_expect_success 'splitting handles diff.suppressBlankEmpty' '
> + test_when_finished "git reset --hard" &&
> + cat >file <<-\EOF &&
> + 1
> + 2
> +
> + 3
> + 4
> + EOF
> + git add file &&
> +
> + cat >file <<-\EOF &&
> + one
> + two
> +
> + three
> + four
> + EOF
> + test_write_lines s n y |
> + git -c diff.suppressBlankEmpty=true add -p &&
> +
> + git cat-file blob :file >actual &&
> + cat >expect <<-\EOF &&
> + 1
> + 2
> +
> + three
> + four
> + EOF
> + test_cmp expect actual
> +'
> +
> test_done