Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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

> 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