From: Jonathan Tan <jonathantanmy@google.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, simon@ruderich.org, avarab@gmail.com,
jacob.keller@gmail.com
Subject: Re: [PATCH 6/7] diff.c: decouple white space treatment from move detection algorithm
Date: Tue, 24 Apr 2018 15:00:36 -0700 [thread overview]
Message-ID: <20180424150036.8e7ca862525bedb91673ae46@google.com> (raw)
In-Reply-To: <20180424210330.87861-7-sbeller@google.com>
On Tue, 24 Apr 2018 14:03:29 -0700
Stefan Beller <sbeller@google.com> wrote:
> As we change the default, we'll adjust the tests.
This statement is probably better written as:
In some existing tests, options like --ignore-space-at-eol were used
to control the color of the output. They have been updated to use
options like --color-moved-ignore-space-at-eol instead.
> + unsigned flags = diffopt->color_moved_ws_handling
> + & XDF_WHITESPACE_FLAGS;
No need for "& XDF_WHITESPACE_FLAGS".
> + unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
Same here.
> @@ -214,6 +214,7 @@ struct diff_options {
> } color_moved;
> #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
> #define COLOR_MOVED_MIN_ALNUM_COUNT 20
> + int color_moved_ws_handling;
> };
Should the "int" be "unsigned"? I noticed that the flag-like xdl_opts is
signed, but I think it's better for flags to be unsigned. Also, document
what this stores. (And also, I would limit the bits.)
> +test_expect_success 'only move detection ignores white spaces' '
> + git reset --hard &&
> + q_to_tab <<-\EOF >text.txt &&
> + a long line to exceed per-line minimum
> + another long line to exceed per-line minimum
> + original file
> + EOF
> + git add text.txt &&
> + git commit -m "add text" &&
> + q_to_tab <<-\EOF >text.txt &&
> + Qa long line to exceed per-line minimum
> + Qanother long line to exceed per-line minimum
> + new file
> + EOF
> +
> + # Make sure we get a different diff using -w
> + git diff --color --color-moved -w \
> + --color-moved-no-ignore-all-space \
> + --color-moved-no-ignore-space-change \
> + --color-moved-no-ignore-space-at-eol |
> + 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,3 +1,3 @@<RESET>
> + Qa long line to exceed per-line minimum<RESET>
> + Qanother long line to exceed per-line minimum<RESET>
> + <RED>-original file<RESET>
> + <GREEN>+<RESET><GREEN>new file<RESET>
> + EOF
> + test_cmp expected actual &&
> +
> + # And now ignoring white space only in the move detection
> + git diff --color --color-moved \
> + --color-moved-ignore-all-space \
> + --color-moved-ignore-space-change \
> + --color-moved-ignore-space-at-eol |
> + 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,3 +1,3 @@<RESET>
> + <BOLD;MAGENTA>-a long line to exceed per-line minimum<RESET>
> + <BOLD;MAGENTA>-another long line to exceed per-line minimum<RESET>
> + <RED>-original file<RESET>
> + <BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>a long line to exceed per-line minimum<RESET>
> + <BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>another long line to exceed per-line minimum<RESET>
> + <GREEN>+<RESET><GREEN>new file<RESET>
> + EOF
> + test_cmp expected actual
> '
I know I suggested "per-line minimum", but I don't think there is one -
I think we only have a per-block minimum. Maybe delete "per-line" in
each of the lines.
next prev parent reply other threads:[~2018-04-24 22:00 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-24 21:03 [PATCHv2 0/7] Moved code detection: ignore space on uniform indentation Stefan Beller
2018-04-24 21:03 ` [PATCH 1/7] xdiff/xdiff.h: remove unused flags Stefan Beller
2018-04-24 21:03 ` [PATCH 2/7] xdiff/xdiffi.c: remove unneeded function declarations Stefan Beller
2018-04-24 21:03 ` [PATCH 3/7] diff.c: do not pass diff options as keydata to hashmap Stefan Beller
2018-04-24 21:03 ` [PATCH 4/7] diff.c: adjust hash function signature to match hashmap expectation Stefan Beller
2018-04-24 21:03 ` [PATCH 5/7] diff.c: add a blocks mode for moved code detection Stefan Beller
2018-04-24 21:50 ` Jonathan Tan
2018-04-24 22:37 ` Stefan Beller
2018-04-24 22:59 ` Jonathan Tan
2018-04-24 21:03 ` [PATCH 6/7] diff.c: decouple white space treatment from move detection algorithm Stefan Beller
2018-04-24 22:00 ` Jonathan Tan [this message]
2018-04-24 22:19 ` Stefan Beller
2018-04-24 21:03 ` [PATCH 7/7] diff.c: add --color-moved-ignore-space-delta option Stefan Beller
2018-04-24 22:35 ` Jonathan Tan
[not found] ` <CAGZ79kbGkHFSS9K8KKTwNikx3Tw+m+RMLY3RAf8SW_iK9a2OJQ@mail.gmail.com>
2018-04-24 23:23 ` Stefan Beller
2018-04-25 0:11 ` Jonathan Tan
2018-04-24 22:37 ` [PATCHv2 0/7] Moved code detection: ignore space on uniform indentation Jonathan Tan
2018-04-24 22:58 ` Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180424150036.8e7ca862525bedb91673ae46@google.com \
--to=jonathantanmy@google.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=jacob.keller@gmail.com \
--cc=sbeller@google.com \
--cc=simon@ruderich.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).