Commit Graph

78 Commits

Author SHA1 Message Date
Phillip Wood
10acc5f750 diff --color-moved-ws: demonstrate false positives
'diff --color-moved-ws=allow-indentation-change' can highlight lines
that have internal whitespace changes rather than indentation
changes. For example in commit 1a07e59c3e ("Update messages in
preparation for i18n", 2018-07-21) the lines

-               die (_("must end with a color"));
+               die(_("must end with a color"));

are highlighted as moved when they should not be. Modify an existing
test to show the problem that will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-01-10 10:38:03 -08:00
Junio C Hamano
986c518107 Merge branch 'sg/test-must-be-empty'
Test fixes.

* sg/test-must-be-empty:
  tests: use 'test_must_be_empty' instead of 'test_cmp <empty> <out>'
  tests: use 'test_must_be_empty' instead of 'test_cmp /dev/null <out>'
  tests: use 'test_must_be_empty' instead of 'test ! -s'
  tests: use 'test_must_be_empty' instead of '! test -s'
2018-08-27 14:33:43 -07:00
SZEDER Gábor
1c5e94f459 tests: use 'test_must_be_empty' instead of 'test_cmp <empty> <out>'
Using 'test_must_be_empty' is shorter and more idiomatic than

  >empty &&
  test_cmp empty out

as it saves the creation of an empty file.  Furthermore, sometimes the
expected empty file doesn't have such a descriptive name like 'empty',
and its creation is far away from the place where it's finally used
for comparison (e.g. in 't7600-merge.sh', where two expected empty
files are created in the 'setup' test, but are used only about 500
lines later).

These cases were found by instrumenting 'test_cmp' to error out the
test script when it's used to compare empty files, and then converted
manually.

Note that even after this patch there still remain a lot of cases
where we use 'test_cmp' to check empty files:

  - Sometimes the expected output is not hard-coded in the test, but
    'test_cmp' is used to ensure that two similar git commands produce
    the same output, and that output happens to be empty, e.g. the
    test 'submodule update --merge  - ignores --merge  for new
    submodules' in 't7406-submodule-update.sh'.

  - Repetitive common tasks, including preparing the expected results
    and running 'test_cmp', are often extracted into a helper
    function, and some of this helper's callsites expect no output.

  - For the same reason as above, the whole 'test_expect_success'
    block is within a helper function, e.g. in 't3070-wildmatch.sh'.

  - Or 'test_cmp' is invoked in a loop, e.g. the test 'cvs update
    (-p)' in 't9400-git-cvsserver-server.sh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-21 11:48:36 -07:00
Junio C Hamano
14677d25ab Merge branch 'ab/test-must-be-empty-for-master'
Test updates.

* ab/test-must-be-empty-for-master:
  tests: make use of the test_must_be_empty function
2018-08-20 11:33:48 -07:00
Junio C Hamano
706b0b5e8d Merge branch 'es/diff-color-moved-fix'
One of the "diff --color-moved" mode "dimmed_zebra" that was named
in an unusual way has been deprecated and replaced by
"dimmed-zebra".

* es/diff-color-moved-fix:
  diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"
2018-08-15 15:08:22 -07:00
Ævar Arnfjörð Bjarmason
d3c6751b18 tests: make use of the test_must_be_empty function
Change various tests that use an idiom of the form:

    >expect &&
    test_cmp expect actual

To instead use:

    test_must_be_empty actual

The test_must_be_empty() wrapper was introduced in ca8d148daf ("test:
test_must_be_empty helper", 2013-06-09). Many of these tests have been
added after that time. This was mostly found with, and manually pruned
from:

    git grep '^\s+>.*expect.* &&$' t

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-30 11:18:41 -07:00
Eric Sunshine
e3f2f5f9cd diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"
The --color-moved "dimmed_zebra" mode (with an underscore) is an
anachronism. Most options and modes are hyphenated. It is more difficult
to type and somewhat more difficult to read than those which are
hyphenated. Therefore, rename it to "dimmed-zebra", and nominally
deprecate "dimmed_zebra".

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-25 14:23:52 -07:00
Stefan Beller
ca1f4ae4df diff.c: add white space mode to move detection that allows indent changes
The option of --color-moved has proven to be useful as observed on the
mailing list. However when refactoring sometimes the indentation changes,
for example when partitioning a functions into smaller helper functions
the code usually mostly moved around except for a decrease in indentation.

To just review the moved code ignoring the change in indentation, a mode
to ignore spaces in the move detection as implemented in a previous patch
would be enough.  However the whole move coloring as motivated in commit
2e2d5ac (diff.c: color moved lines differently, 2017-06-30), brought
up the notion of the reviewer being able to trust the move of a "block".

As there are languages such as python, which depend on proper relative
indentation for the control flow of the program, ignoring any white space
change in a block would not uphold the promises of 2e2d5ac that allows
reviewers to pay less attention to the inside of a block, as inside
the reviewer wants to assume the same program flow.

This new mode of white space ignorance will take this into account and will
only allow the same white space changes per line in each block. This patch
even allows only for the same change at the beginning of the lines.

As this is a white space mode, it is made exclusive to other white space
modes in the move detection.

This patch brings some challenges, related to the detection of blocks.
We need a wide net to catch the possible moved lines, but then need to
narrow down to check if the blocks are still intact. Consider this
example (ignoring block sizes):

 - A
 - B
 - C
 +    A
 +    B
 +    C

At the beginning of a block when checking if there is a counterpart
for A, we have to ignore all space changes. However at the following
lines we have to check if the indent change stayed the same.

Checking if the indentation change did stay the same, is done by computing
the indentation change by the difference in line length, and then assume
the change is only in the beginning of the longer line, the common tail
is the same. That is why the test contains lines like:

 - <TAB> A
 ...
 + A <TAB>
 ...

As the first line starting a block is caught using a compare function that
ignores white spaces unlike the rest of the block, where the white space
delta is taken into account for the comparison, we also have to think about
the following situation:

 - A
 - B
 -   A
 -   B
 +    A
 +    B
 +      A
 +      B

When checking if the first A (both in the + and - lines) is a start of
a block, we have to check all 'A' and record all the white space deltas
such that we can find the example above to be just one block that is
indented.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-19 12:02:54 -07:00
Stefan Beller
b3095712f9 diff.c: decouple white space treatment from move detection algorithm
In the original implementation of the move detection logic the choice for
ignoring white space changes is the same for the move detection as it is
for the regular diff.  Some cases came up where different treatment would
have been nice.

Allow the user to specify that white space should be ignored differently
during detection of moved lines than during generation of added and removed
lines. This is done by providing analogs to the --ignore-space-at-eol,
-b, and -w options by introducing the option --color-moved-ws=<modes>
with the modes named "ignore-space-at-eol", "ignore-space-change" and
"ignore-all-space", which is used only during the move detection phase.

As we change the default, we'll adjust the tests.

For now we do not infer any options to treat white spaces in the move
detection from the generic white space options given to diff.
This can be tuned later to reasonable default.

As we plan on adding more white space related options in a later patch,
that interferes with the current white space options, use a flag field
and clamp it down to  XDF_WHITESPACE_FLAGS, as that (a) allows to easily
check at parse time if we give invalid combinations and (b) can reuse
parts of this patch.

By having the white space treatment in its own option, we'll also
make it easier for a later patch to have an config option for
spaces in the move detection.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-17 11:25:31 -07:00
Stefan Beller
51da15eb23 diff.c: add a blocks mode for moved code detection
The new "blocks" mode provides a middle ground between plain and zebra.
It is as intuitive (few colors) as plain, but still has the requirement
for a minimum of lines/characters to count a block as moved.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
 (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-17 11:25:31 -07:00
Stefan Beller
74cfa7bed9 t4015: avoid git as a pipe input
In t4015 we have a pattern of

    git diff [<options, related to color>] |
        grep -v "index" |
        test_decode_color >actual &&

to produce output that we want to test against. This pattern was introduced
in 86b452e276 (diff.c: add dimming to moved line detection, 2017-06-30)
as then the focus on getting the colors right. However the pattern used
is not best practice as we do care about the exit code of Git. So let's
not have Git as the upstream of a pipe. Piping the output of grep to
some function is fine as we assume grep to be un-flawed in our test suite.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-17 11:25:31 -07:00
Junio C Hamano
6f3a0b6da5 Merge branch 'bw/submodule-config-cleanup'
Recent update to the submodule configuration code broke "diff-tree"
by accidentally stopping to read from the index upfront.

* bw/submodule-config-cleanup:
  diff-tree: read the index so attribute checks work in bare repositories
2017-12-19 11:33:57 -08:00
Brandon Williams
fd66bcc31f diff-tree: read the index so attribute checks work in bare repositories
A regression was introduced in 557a5998d (submodule: remove
gitmodules_config, 2017-08-03) to how attribute processing was handled
in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute"
files from the working tree and then falls back to reading them from the
index if there isn't a copy checked out in the worktree.  Prior to
557a5998d the index was read as a side effect of the call to
'gitmodules_config()' which ensured that the index was already populated
before entering the attribute subsystem.

Since the call to 'gitmodules_config()' was removed the index is no
longer being read so when the attribute system tries to read from the
in-memory index it doesn't find any ".gitattribute" entries effectively
ignoring any configured attributes.

Fix this by explicitly reading the index during the setup of diff-tree.

Reported-by: Ben Boeckel <ben.boeckel@kitware.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-12-06 14:49:18 -08:00
Junio C Hamano
10f65c239a Merge branch 'jc/ignore-cr-at-eol'
The "diff" family of commands learned to ignore differences in
carriage return at the end of line.

* jc/ignore-cr-at-eol:
  diff: --ignore-cr-at-eol
  xdiff: reassign xpparm_t.flags bits
2017-11-27 11:06:31 +09:00
Junio C Hamano
e9282f02b2 diff: --ignore-cr-at-eol
A new option --ignore-cr-at-eol tells the diff machinery to treat a
carriage-return at the end of a (complete) line as if it does not
exist.

Just like other "--ignore-*" options to ignore various kinds of
whitespace differences, this will help reviewing the real changes
you made without getting distracted by spurious CRLF<->LF conversion
made by your editor program.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
[jch: squashed in command line completion by Dscho]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-11-08 10:05:27 +09:00
Jeff King
da58318e76 diff: fix whitespace-skipping with --color-moved
The code for handling whitespace with --color-moved
represents partial strings as a pair of pointers. There are
two possible conventions for the end pointer:

  1. It points to the byte right after the end of the
     string.

  2. It points to the final byte of the string.

But we seem to use both conventions in the code:

  a. we assign the initial pointers from the NUL-terminated
     string using (1)

  b. we eat trailing whitespace by checking the second
     pointer for isspace(), which needs (2)

  c. the next_byte() function checks for end-of-string with
     "if (cp > endp)", which is (2)

  d. in next_byte() we skip past internal whitespace with
     "while (cp < end)", which is (1)

This creates fewer bugs than you might think, because there
are some subtle interactions. Because of (a) and (c), we
always return the NUL-terminator from next_byte(). But all
of the callers of next_byte() happen to handle that
gracefully.

Because of the mismatch between (d) and (c), next_byte()
could accidentally return a whitespace character right at
endp. But because of the interaction of (a) and (b), we fail
to actually chomp trailing whitespace, meaning our endp
_always_ points to a NUL, canceling out the problem.

But that does leave (b) as a real bug: when ignoring
whitespace only at the end-of-line, we don't correctly trim
it, and fail to match up lines.

We can fix the whole thing by moving consistently to one
convention. Since convention (1) is idiomatic in our code
base, we'll pick that one.

The existing "-w" and "-b" tests continue to pass, and a new
"--ignore-space-at-eol" shows off the breakage we're fixing.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-21 21:12:35 +09:00
Jeff King
d5aae1f7cd t4015: test the output of "diff --color-moved -b"
Commit fa5ba2c1dd (diff: fix infinite loop with
--color-moved --ignore-space-change, 2017-10-12) added a
test to make sure that "--color-moved -b" doesn't run
forever, but the test in question doesn't actually have any
moved lines in it.

Let's scrap that test and add a variant of the existing
"--color-moved -w" test, but this time we'll check that we
find the move with whitespace changes, but not arbitrary
whitespace additions.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-21 21:11:08 +09:00
Jeff King
83de23cfea t4015: check "negative" case for "-w --color-moved"
We test that lines with whitespace changes are not found by
"--color-moved" by default, but are found if "-w" is added.
Let's add one more twist: a line that has non-whitespace
changes should not be marked as a pure move.

This is perhaps an obvious case for us to get right (and we
do), but as we add more whitespace tests, they will form a
pattern of "make sure this case is a move and this other
case is not".

Note that we have to add a line to our moved block, since
having a too-small block doesn't trigger the "moved"
heuristics.  And we also add a line of context to ensure
that there's more context lines than moved lines (so the
diff shows us moving the lines up, rather than moving the
context down).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-21 21:11:04 +09:00
Jeff King
ecd512582c t4015: refactor --color-moved whitespace test
In preparation for testing several different whitespace
options, let's split out the setup and cleanup steps of the
whitespace test.

While we're here, let's also switch to using "<<-" to indent
our here-documents properly, and use q_to_tab to more
explicitly mark where we expect whitespace to appear.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-21 21:10:58 +09:00
Junio C Hamano
91ccfb8517 Merge branch 'sb/diff-color-move'
A recently added "--color-moved" feature of "diff" fell into
infinite loop when ignoring whitespace changes, which has been
fixed.

* sb/diff-color-move:
  diff: fix infinite loop with --color-moved --ignore-space-change
2017-10-17 13:29:19 +09:00
Jeff King
fa5ba2c1dd diff: fix infinite loop with --color-moved --ignore-space-change
The --color-moved code uses next_byte() to advance through
the blob contents. When the user has asked to ignore
whitespace changes, we try to collapse any whitespace change
down to a single space.

However, we enter the conditional block whenever we see the
IGNORE_WHITESPACE_CHANGE flag, even if the next byte isn't
whitespace.

This means that the combination of "--color-moved and
--ignore-space-change" was completely broken. Worse, because
we return from next_byte() without having advanced our
pointer, the function makes no forward progress in the
buffer and loops infinitely.

Fix this by entering the conditional only when we actually
see whitespace. We can apply this also to the
IGNORE_WHITESPACE change. That code path isn't buggy
(because it falls through to returning the next
non-whitespace byte), but it makes the logic more clear if
we only bother to look at whitespace flags after seeing that
the next byte is whitespace.

Reported-by: Orgad Shaneh <orgads@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-16 11:57:45 +09:00
Junio C Hamano
97cb362262 Merge branch 'sb/test-cmp-expect-actual'
Test tweak.

* sb/test-cmp-expect-actual:
  tests: fix diff order arguments in test_cmp
2017-10-11 14:52:23 +09:00
Stefan Beller
9c5b2fab30 tests: fix diff order arguments in test_cmp
Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-07 10:56:08 +09:00
Junio C Hamano
aebd23506e Merge branch 'jk/ui-color-always-to-auto-maint' into jk/ui-color-always-to-auto
* jk/ui-color-always-to-auto-maint:
  color: make "always" the same as "auto" in config
  provide --color option for all ref-filter users
  t3205: use --color instead of color.branch=always
  t3203: drop "always" color test
  t6006: drop "always" color config tests
  t7502: use diff.noprefix for --verbose test
  t7508: use test_terminal for color output
  t3701: use test-terminal to collect color output
  t4015: prefer --color to -c color.diff=always
  test-terminal: set TERM=vt100
2017-10-04 12:04:47 +09:00
Jeff King
269c73e8d3 t4015: use --color with --color-moved
The tests for --color-moved write their output to a file,
but doing so suppresses color output under "auto". Right now
this is solved by running the whole script under
"color.diff=always". In preparation for the behavior of
"always" changing, let's explicitly enable color.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-04 11:48:17 +09:00
Jeff King
a655a59595 t4015: prefer --color to -c color.diff=always
t4015 contains many color-related tests which need to
override the "is stdout a tty" check. They do so by setting
the color.diff config, but we can accomplish the same with
the --color option. Besides being shorter to type, switching
will prepare us for upcoming changes to "always" when see it
in config.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-10-04 11:25:12 +09:00
Jonathan Tan
f0b8fb6e59 diff: define block by number of alphanumeric chars
The existing behavior of diff --color-moved=zebra does not define the
minimum size of a block at all, instead relying on a heuristic applied
later to filter out sets of adjacent moved lines that are shorter than 3
lines long. This can be confusing, because a block could thus be colored
as moved at the source but not at the destination (or vice versa),
depending on its neighbors.

Instead, teach diff that the minimum size of a block is 20 alphanumeric
characters, the same heuristic used by "git blame". This allows diff to
still exclude uninteresting lines appearing on their own (such as those
solely consisting of one or a few closing braces), as was the intention
of the adjacent-moved-line heuristic.

This requires a change in some tests in that some of their lines are no
longer considered to be part of a block, because they are too short.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-08-16 11:44:00 -07:00
Jonathan Tan
09153277f8 diff: respect MIN_BLOCK_LENGTH for last block
Currently, MIN_BLOCK_LENGTH is only checked when diff encounters a line
that does not belong to the current block. In particular, this means
that MIN_BLOCK_LENGTH is not checked after all lines are encountered.

Perform that check.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-08-16 11:44:00 -07:00
Stefan Beller
86b452e276 diff.c: add dimming to moved line detection
Any lines inside a moved block of code are not interesting. Boundaries
of blocks are only interesting if they are next to another block of moved
code.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-30 13:59:42 -07:00
Stefan Beller
176841f0c9 diff.c: color moved lines differently, plain mode
Add the 'plain' mode for move detection of code. This omits the checking
for adjacent blocks, so it is not as useful. If you have a lot of the
same blocks moved in the same patch, the 'Zebra' would end up slow as it
is O(n^2) (n is number of same blocks). So this may be useful there and
is generally easy to add. Instead be very literal at the move detection,
do not skip over short blocks here.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-30 13:59:42 -07:00
Stefan Beller
2e2d5ac184 diff.c: color moved lines differently
When a patch consists mostly of moving blocks of code around, it can
be quite tedious to ensure that the blocks are moved verbatim, and not
undesirably modified in the move. To that end, color blocks that are
moved within the same patch differently. For example (OM, del, add,
and NM are different colors):

    [OM]  -void sensitive_stuff(void)
    [OM]  -{
    [OM]  -        if (!is_authorized_user())
    [OM]  -                die("unauthorized");
    [OM]  -        sensitive_stuff(spanning,
    [OM]  -                        multiple,
    [OM]  -                        lines);
    [OM]  -}

           void another_function()
           {
    [del] -        printf("foo");
    [add] +        printf("bar");
           }

    [NM]  +void sensitive_stuff(void)
    [NM]  +{
    [NM]  +        if (!is_authorized_user())
    [NM]  +                die("unauthorized");
    [NM]  +        sensitive_stuff(spanning,
    [NM]  +                        multiple,
    [NM]  +                        lines);
    [NM]  +}

However adjacent blocks may be problematic. For example, in this
potentially malicious patch, the swapping of blocks can be spotted:

    [OM]  -void sensitive_stuff(void)
    [OM]  -{
    [OMA] -        if (!is_authorized_user())
    [OMA] -                die("unauthorized");
    [OM]  -        sensitive_stuff(spanning,
    [OM]  -                        multiple,
    [OM]  -                        lines);
    [OMA] -}

           void another_function()
           {
    [del] -        printf("foo");
    [add] +        printf("bar");
           }

    [NM]  +void sensitive_stuff(void)
    [NM]  +{
    [NMA] +        sensitive_stuff(spanning,
    [NMA] +                        multiple,
    [NMA] +                        lines);
    [NM]  +        if (!is_authorized_user())
    [NM]  +                die("unauthorized");
    [NMA] +}

If the moved code is larger, it is easier to hide some permutation in the
code, which is why some alternative coloring is needed.

This patch implements the first mode:
* basic alternating 'Zebra' mode
  This conveys all information needed to the user.  Defer customization to
  later patches.

First I implemented an alternative design, which would try to fingerprint
a line by its neighbors to detect if we are in a block or at the boundary.
This idea iss error prone as it inspected each line and its neighboring
lines to determine if the line was (a) moved and (b) if was deep inside
a hunk by having matching neighboring lines. This is unreliable as the
we can construct hunks which have equal neighbors that just exceed the
number of lines inspected. (Think of 'AXYZBXYZCXYZD..' with each letter
as a line, that is permutated to AXYZCXYZBXYZD..').

Instead this provides a dynamic programming greedy algorithm that finds
the largest moved hunk and then has several modes on highlighting bounds.

A note on the options '--submodule=diff' and '--color-words/--word-diff':
In the conversion to use emit_line in the prior patches both submodules
as well as word diff output carefully chose to call emit_line with sign=0.
All output with sign=0 is ignored for move detection purposes in this
patch, such that no weird looking output will be generated for these
cases. This leads to another thought: We could pass on '--color-moved' to
submodules such that they color up moved lines for themselves. If we'd do
so only line moves within a repository boundary are marked up.

It is useful to have moved lines colored, but there are annoying corner
cases, such as a single line moved, that is very common. For example
in a typical patch of C code, we have closing braces that end statement
blocks or functions.

While it is technically true that these lines are moved as they show up
elsewhere, it is harmful for the review as the reviewers attention is
drawn to such a minor side annoyance.

For now let's have a simple solution of hardcoding the number of
moved lines to be at least 3 before coloring them. Note, that the
length is applied across all blocks to find the 'lonely' blocks
that pollute new code, but do not interfere with a permutated
block where each permutation has less lines than 3.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-30 13:59:42 -07:00
Junio C Hamano
a17505f262 diff: introduce diff.wsErrorHighlight option
With the preparatory steps, it has become trivial to teach the
system a new diff.wsErrorHighlight configuration that gives the
default value for --ws-error-highlight command line option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-10-04 15:49:05 -07:00
Junio C Hamano
f3f5c7f520 t4015: split out the "setup" part of ws-error-highlight test
We'd want to run this same set of test twice, once with the option
and another time with an equivalent configuration setting.  Split
out the step that prepares the test data and expected output and
move the test for the command line option into a separate test.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-10-04 15:49:05 -07:00
Junio C Hamano
b8767f791c diff.c: --ws-error-highlight=<kind> option
Traditionally, we only cared about whitespace breakages introduced
in new lines.  Some people want to paint whitespace breakages on old
lines, too.  When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, those breakages are there but they
were inherited from the original, so let's not touch them for now."

Introduce `--ws-error-highlight=<kind>` option, that lets them pass
a comma separated list of `old`, `new`, and `context` to specify
what lines to highlight whitespace errors on.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-05-26 23:00:01 -07:00
Junio C Hamano
0ad782f240 t4015: separate common setup and per-test expectation
The last two tests in the script were to

 - set up color.diff.* slots
 - set up an expectation for a single test
 - run that test and check the result

but split in a wrong way.  It did the first two in the first test
and the third one in the second test.  The latter two belong to each
other.  This matters when you plan to add more of these tests that
share the common coloring.

While at it, make sure we use a color different from old, which is
also red.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-05-26 12:45:20 -07:00
Junio C Hamano
d55ef3e044 t4015: modernise style
Move the preparatory steps that create the expected output inside
the test bodies, remove unnecessary blank lines before and after the
test bodies, and drop SP between redirection operator and its target.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-05-26 12:45:20 -07:00
Junio C Hamano
05e9d907dd t4015: simplify sed command that is not even seen by sed
Noticed by Andreas Schwab; \<LF> inside a double quotes pair is
eaten by the shell to become an empty string and is not doing
anything.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2013-11-04 10:11:15 -08:00
Ben Walton
90a95301d3 Change sed i\ usage to something Solaris' sed can handle
Solaris' sed was choking on the i\ commands used in
t4015-diff-whitespace as it couldn't parse the program properly.
Modify two uses of sed that worked in GNU sed but not Solaris'
(/usr/bin or /usr/xpg4/bin) to an equivalent form that is handled
properly by both.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2013-10-28 09:27:06 -07:00
Antoine Pelisse
36617af7ed diff: add --ignore-blank-lines option
The goal of the patch is to introduce the GNU diff
-B/--ignore-blank-lines as closely as possible. The short option is not
available because it's already used for "break-rewrites".

When this option is used, git-diff will not create hunks that simply
add or remove empty lines, but will still show empty lines
addition/suppression if they are close enough to "valuable" changes.

There are two differences between this option and GNU diff -B option:
- GNU diff doesn't have "--inter-hunk-context", so this must be handled
- The following sequence looks like a bug (context is displayed twice):

    $ seq 5 >file1
    $ cat <<EOF >file2
    change
    1
    2

    3
    4
    5
    change
    EOF
    $ diff -u -B file1 file2
    --- file1	2013-06-08 22:13:04.471517834 +0200
    +++ file2	2013-06-08 22:13:23.275517855 +0200
    @@ -1,5 +1,7 @@
    +change
     1
     2
    +
     3
     4
     5
    @@ -3,3 +5,4 @@
     3
     4
     5
    +change

So here is a more thorough description of the option:
- real changes are interesting
- blank lines that are close enough (less than context size) to
interesting changes are considered interesting (recursive definition)
- "context" lines are used around each hunk of interesting changes
- If two hunks are separated by less than "inter-hunk-context", they
will be merged into one.

The implementation does the "interesting changes selection" in a single
pass.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2013-06-19 15:17:45 -07:00
René Scharfe
baf5aaa333 xdiff: print post-image for common records instead of pre-image
Normally it doesn't matter if we show the pre-image or th post-image
for the common parts of a diff because they are the same.  If
white-space changes are ignored they can differ, though.  The
new text after applying the diff is more interesting in that case,
so show that instead of the old contents.

Note: GNU diff shows the pre-image.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2012-01-06 11:10:05 -08:00
Johannes Sixt
f4b05a4947 Make the tab width used for whitespace checks configurable
A new whitespace "rule" is added that sets the tab width to use for
whitespace checks and fix-ups and replaces the hard-coded constant 8.

Since the setting is part of the rules, it can be set per file using
.gitattributes.

The new configuration is backwards compatible because older git versions
simply ignore unknown whitespace rules.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2010-12-01 14:47:51 -08:00
Junio C Hamano
b3ff808b71 Merge branch 'en/and-cascade-tests'
* en/and-cascade-tests: (25 commits)
  t4124 (apply --whitespace): use test_might_fail
  t3404: do not use 'describe' to implement test_cmp_rev
  t3404 (rebase -i): introduce helper to check position of HEAD
  t3404 (rebase -i): move comment to description
  t3404 (rebase -i): unroll test_commit loops
  t3301 (notes): use test_expect_code for clarity
  t1400 (update-ref): use test_must_fail
  t1502 (rev-parse --parseopt): test exit code from "-h"
  t6022 (renaming merge): chain test commands with &&
  test-lib: introduce test_line_count to measure files
  tests: add missing &&, batch 2
  tests: add missing &&
  Introduce sane_unset and use it to ensure proper && chaining
  t7800 (difftool): add missing &&
  t7601 (merge-pull-config): add missing &&
  t7001 (mv): add missing &&
  t6016 (rev-list-graph-simplify-history): add missing &&
  t5602 (clone-remote-exec): add missing &&
  t4026 (color): remove unneeded and unchained command
  t4019 (diff-wserror): add lots of missing &&
  ...

Conflicts:
	t/t7006-pager.sh
2010-11-24 15:51:49 -08:00
Jonathan Nieder
a48fcd8369 tests: add missing &&
Breaks in a test assertion's && chain can potentially hide
failures from earlier commands in the chain.

Commands intended to fail should be marked with !, test_must_fail, or
test_might_fail.  The examples in this patch do not require that.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2010-11-09 11:59:49 -08:00
Kevin Ballard
cfd1a9849c diff: handle lines containing only whitespace and tabs better
When a line contains nothing but whitespace with at least one tab
and the core.whitespace config option contains blank-at-eol, the
whitespace on the line is being printed twice, once unhighlighted
(unless otherwise matched by one of the other core.whitespace values),
and a second time highlighted for blank-at-eol.

Update the leading indentation check to stop checking when it reaches
the trailing whitespace.

Signed-off-by: Kevin Ballard <kevin@sb.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2010-10-20 16:10:15 -07:00
Junio C Hamano
5977744d04 Merge branch 'cc/maint-diff-CC-binary'
* cc/maint-diff-CC-binary:
  diff: fix "git show -C -C" output when renaming a binary file

Conflicts:
	diff.c
2010-06-18 11:16:57 -07:00
Christian Couder
296c6bb21a diff: fix "git show -C -C" output when renaming a binary file
A bug was introduced in 3e97c7c6af
(No diff -b/-w output for all-whitespace changes, Nov 19 2009)
that made the lines:

  diff --git a/bar b/sub/bar
  similarity index 100%
  rename from bar
  rename to sub/bar

disappear from "git show -C -C" output when file bar is a binary
file.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2010-06-06 15:14:27 -07:00
Chris Webb
b27eb49948 whitespace: tests for git-diff --check with tab-in-indent error class
[jc: with test fixes from J6t]

Signed-off-by: Chris Webb <chris@arachsys.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2010-04-04 14:21:00 -07:00
Greg Bacon
3e97c7c6af No diff -b/-w output for all-whitespace changes
Change git-diff's whitespace-ignoring modes to generate
output only if a non-empty patch results, which git-apply
rejects.

Update the tests to look for the new behavior.

Signed-off-by: Greg Bacon <gbacon@dbresearch.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-11-20 22:00:36 -08:00
Junio C Hamano
afd9db4173 Merge branch 'jc/maint-1.6.0-blank-at-eof' (early part) into jc/maint-blank-at-eof
* 'jc/maint-1.6.0-blank-at-eof' (early part):
  diff --whitespace: fix blank lines at end
  core.whitespace: split trailing-space into blank-at-{eol,eof}
  diff --color: color blank-at-eof
  diff --whitespace=warn/error: fix blank-at-eof check
  diff --whitespace=warn/error: obey blank-at-eof
  diff.c: the builtin_diff() deals with only two-file comparison
  apply --whitespace: warn blank but not necessarily empty lines at EOF
  apply --whitespace=warn/error: diagnose blank at EOF
  apply.c: split check_whitespace() into two
  apply --whitespace=fix: detect new blank lines at eof correctly
  apply --whitespace=fix: fix handling of blank lines at the eof
2009-09-15 03:28:08 -07:00
Junio C Hamano
467babf8d0 diff --whitespace=warn/error: fix blank-at-eof check
The "diff --check" logic used to share the same issue as the one fixed for
"git apply" earlier in this series, in that a patch that adds new blank
lines at end could appear as

    @@ -l,5 +m,7 @@$
    _context$
    _context$
    -deleted$
    +$
    +$
    +$
    _$
    _$

where _ stands for SP and $ shows a end-of-line.  Instead of looking at
each line in the patch in the callback, simply count the blank lines from
the end in two versions, and notice the presence of new ones.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-09-04 11:50:27 -07:00