git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 7/7] diff.c: add --color-moved-ignore-space-delta option

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Apr 24, 2018, 22:35 UTC
Message-ID
<20180424153513.dc2404cd111c44ac78bd8ed8@google.com>
In-Reply-To
<20180424210330.87861-8-sbeller@google.com>

On Tue, 24 Apr 2018 14:03:30 -0700 Stefan Beller <sbeller@google.com> wrote:

> +--color-moved-[no-]ignore-space-prefix-delta::
> +	Ignores whitespace when comparing lines when performing the move
> +	detection for --color-moved. This ignores uniform differences
> +	of white space at the beginning lines in moved blocks.

Setting this option means that moved lines may be indented or dedented, and if they have been indented or dedented by the same amount, they are still considered to be the same block. Maybe call this --color-moved-allow-indentation-change.

> +struct ws_delta {
> +	char *string; /* The prefix delta, which is the same in the block */

Probably better described as "the difference between the '-' line and the '+' line". This may be the empty string if there is no difference.

> +	int direction; /* adding or removing the line? */

What is the value when "added" and what when "removed"? Also, it is not truly "added" or "removed", so a better way might be: 1 if the '-' line is longer than the '+' line, and 0 otherwise. (And make sure that the documentation is correct with respect to equal lines.)

> +	int missmatch; /* in the remainder */

s/missmatch/mismatch/ Also, what is this used for?

Show 12 quoted lines
> +	if (diffopt->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)
> +		/*
> +		 * As there is not specific white space config given,
> +		 * we'd need to check for a new block, so ignore all
> +		 * white space. The setup of the white space
> +		 * configuration for the next block is done else where
> +		 */
> +		flags |= XDF_IGNORE_WHITESPACE;
> +
>  	return !xdiff_compare_lines(a->es->line, a->es->len,
>  				    b->es->line, b->es->len,
>  				    flags);
I wrote in [1]:
  I think we should just prohibit combining this with any of the
  whitespace ignoring flags except for the space-at-eol one. They seem
  to contradict anyway.

To elaborate, adding a specific flag that may interfere with other user-provided flags sounds like we're unnecessarily adding combinations that we must keep track of, and that it's both logical (from a user's point of view) and clearer (as for the code) to just forbid such combinations.

[1] https://public-inbox.org/git/20180402174118.d204ec0d4b9d2fa7ebd77739@google.com/
Show 8 quoted lines
>  	struct moved_entry **pmb = NULL; /* potentially moved blocks */
>  	int pmb_nr = 0, pmb_alloc = 0;
> -	int n, flipped_block = 1, block_length = 0;
>  
> +	struct ws_delta *wsd = NULL; /* white space deltas between pmb */
> +	int wsd_alloc = 0;
> +
> +	int n, flipped_block = 1, block_length = 0;

It seems like pmb and wsd are parallel arrays - could each wsd be embedded into the corresponding entry of pmb instead?

Show 10 quoted lines
> --- a/diff.h
> +++ b/diff.h
> @@ -214,6 +214,8 @@ struct diff_options {
>  	} color_moved;
>  	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
>  	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
> +	/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */
> +	#define COLOR_MOVED_DELTA_WHITESPACES	(1 << 22)
>  	int color_moved_ws_handling;
>  };

Setting of DELTA_WHITESPACES should be a separate field, not as part of ws_handling. It's fine for the ws_handling to be a bitset, since that's how it's passed to xdiff_compare_lines(), but we don't need to do the same for DELTA_WHITESPACES.

Show 53 quoted lines
> +test_expect_success 'compare whitespace delta across moved blocks' '
> +
> +	git reset --hard &&
> +	q_to_tab <<-\EOF >text.txt &&
> +	QIndented
> +	QText across
> +	Qthree lines
> +	QBut! <- this stands out
> +	Qthis one
> +	QQline did
> +	Qnot adjust
> +	EOF
> +
> +	git add text.txt &&
> +	git commit -m "add text.txt" &&
> +
> +	q_to_tab <<-\EOF >text.txt &&
> +	QQIndented
> +	QQText across
> +	QQthree lines
> +	QQQBut! <- this stands out
> +	this one
> +	Qline did
> +	not adjust
> +	EOF
> +
> +	git diff --color --color-moved-ignore-space-prefix-delta |
> +		grep -v "index" |
> +		test_decode_color >actual &&
> +
> +	q_to_tab <<-\EOF >expected &&
> +		<BOLD>diff --git a/text.txt b/text.txt<RESET>
> +		<BOLD>--- a/text.txt<RESET>
> +		<BOLD>+++ b/text.txt<RESET>
> +		<CYAN>@@ -1,7 +1,7 @@<RESET>
> +		<RED>-QIndented<RESET>
> +		<RED>-QText across<RESET>
> +		<RED>-Qthree lines<RESET>
> +		<RED>-QBut! <- this stands out<RESET>
> +		<RED>-Qthis one<RESET>
> +		<RED>-QQline did<RESET>
> +		<RED>-Qnot adjust<RESET>
> +		<GREEN>+<RESET>QQ<GREEN>Indented<RESET>
> +		<GREEN>+<RESET>QQ<GREEN>Text across<RESET>
> +		<GREEN>+<RESET>QQ<GREEN>three lines<RESET>
> +		<GREEN>+<RESET>QQQ<GREEN>But! <- this stands out<RESET>
> +		<GREEN>+<RESET><GREEN>this one<RESET>
> +		<GREEN>+<RESET>Q<GREEN>line did<RESET>
> +		<GREEN>+<RESET><GREEN>not adjust<RESET>
> +	EOF
> +
> +	test_cmp expected actual
> +'

I would have expected every line except the "this stands out" line to be colored differently than the usual RED and GREEN. Is this test output expected?

Previous: Stefan BellerNext: Stefan Beller
Message 6 of 18 in “[PATCHv2 0/7] Moved code detection: ignore space on uniform indentation”
  1. Stefan BellerApr 24, 2018
  2. 1/7 xdiff/xdiff.h: remove unused flagsStefan Beller, Apr 24, 2018
  3. 2/7 xdiff/xdiffi.c: remove unneeded function declarationsStefan Beller, Apr 24, 2018
  4. 3/7 diff.c: do not pass diff options as keydata to hashmapStefan Beller, Apr 24, 2018
  5. 7/7 diff.c: add --color-moved-ignore-space-delta optionStefan Beller, Apr 24, 2018
  6. Jonathan TanApr 24, 2018
  7. 4/7 diff.c: adjust hash function signature to match hashmap expectationStefan Beller, Apr 24, 2018
  8. 5/7 diff.c: add a blocks mode for moved code detectionStefan Beller, Apr 24, 2018
  9. Jonathan TanApr 24, 2018
  10. Stefan BellerApr 24, 2018
  11. Jonathan TanApr 24, 2018
  12. 6/7 diff.c: decouple white space treatment from move detection algorithmStefan Beller, Apr 24, 2018
  13. Jonathan TanApr 24, 2018
  14. Stefan BellerApr 24, 2018
  15. Jonathan TanApr 24, 2018
  16. Stefan BellerApr 24, 2018
  17. Stefan BellerApr 24, 2018
  18. Jonathan TanApr 25, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.