Re: [PATCH] Allow combined diff to ignore white-spaces

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2013, 07:45 UTC
Message-ID: <7v38wdc4ei.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1362236658-17200-1-git-send-email-apelisse@gmail.com>

Antoine Pelisse <apelisse@gmail.com> writes:

Show 20 quoted lines

> Currently, it's not possible to use the space-ignoring options (-b, -w,
> --ignore-space-at-eol) with combined diff. It makes it pretty impossible
> to read a merge between a branch that changed all tabs to spaces, and a
> branch with functional changes.
>
> Pass diff flags to diff engine, so that combined diff behaves as normal
> diff does with spaces.
>
> It also means that a conflict-less merge done using a ignore-* strategy
> option will not show any conflict if shown in combined-diff using the
> same option.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> That should be reviewed carefully as I'm not exactly sure that does make
> sense with the way combined-diff works.
> Still it seems natural to me to be able to remove the space in combined
> diff as we do with normal diff. Especially as I unfortunately have to
> deal with many space + feature merges that are very hard to analyze/handle
> if space differences can't be hidden.

Assuming "That" at the beginning of the off-line comment refers to "this patch", I actually I do not think this patch needs to be reviewed carefully at all.

The change in your patch to make the internal pairwise diff to ignore whitespaces is an obvious and a sensible first step for your stated goal. With it, a block of lines where the postimage makes no change other than whitespace changes from one parent and matches with the other parent will see no hunks in either of the pair-wise diff, and such a hunk will not appear in the final output, which is exactly what you want.

What needs to be audited carefully is the part of combine diff logic that this patch does *not* touch.

An example. After collecting pairwise diffs between the shared postimage and individual parents, we align the hunks and coalesce "identical" preimage lines to form shared "-" (removed) lines. I think that step is done with byte-for-byte comparison. If the postimage removes a line from one parent and a corresponding line from another parent, and if these corresponding lines in the parents differ only by whitespaces in a way the user told us to ignore with -b/-w/etc., you would need to tweak the logic to coalesce corresponding "lost" lines in the append_lost() function. Otherwise, you will see two " -" and "- " lines that remove these corresponding lines from the parents that are identical when you ignore whitespaces.

You should add some tests; it would help you think about these issues through.

For example, in this history:

       git init
       seq 11 >ten
       git add ten
       git commit -m initial

       seq 10 | sed -e 's/6/6 six/' -e 's/2/2 two/' >ten
       echo 11 >>ten
       git commit -m ten -a

       git checkout -b side HEAD^
       seq 10 | sed -e 's/^/  /' | sed -e 's/2/2 dos/' >ten
       echo 11 >>ten
       git commit -m indent -a

       git merge master

       seq 10 | sed -e 's/5/5 go/' -e 's/2/2 dois/' >ten
       git commit -m merged -a

one side indents the original and tweaks some lines, while the other side tweaks some other lines without reindenting. Most notably, the fifth line on one side is "5" while on the other side is " 5", and this line is rewritten to "5 go" in the final. The lost lines are not coalesced to a single "-- 5" (nor "-- 5") but shows that two preimages were different only by whitespaces. Compare that with what happens to the final line "11" that gets lost in the result.

Thanks.

Show 39 quoted lines

> Cheers,
> Antoine
>
>  combine-diff.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 35d41cd..7ca0a72 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -215,7 +215,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
>  			 struct sline *sline, unsigned int cnt, int n,
>  			 int num_parent, int result_deleted,
>  			 struct userdiff_driver *textconv,
> -			 const char *path)
> +			 const char *path, long flags)
>  {
>  	unsigned int p_lno, lno;
>  	unsigned long nmask = (1UL << n);
> @@ -231,7 +231,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
>  	parent_file.ptr = grab_blob(parent, mode, &sz, textconv, path);
>  	parent_file.size = sz;
>  	memset(&xpp, 0, sizeof(xpp));
> -	xpp.flags = 0;
> +	xpp.flags = flags;
>  	memset(&xecfg, 0, sizeof(xecfg));
>  	memset(&state, 0, sizeof(state));
>  	state.nmask = nmask;
> @@ -962,7 +962,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
>  				     elem->parent[i].mode,
>  				     &result_file, sline,
>  				     cnt, i, num_parent, result_deleted,
> -				     textconv, elem->path);
> +				     textconv, elem->path, opt->xdl_opts);
>  	}
>
>  	show_hunks = make_hunks(sline, cnt, num_parent, dense);
> --
> 1.7.9.5