Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File moves are ignored by filterdiff #22

Open
Tracked by #266556
jdm opened this issue Feb 7, 2018 · 20 comments
Open
Tracked by #266556

File moves are ignored by filterdiff #22

jdm opened this issue Feb 7, 2018 · 20 comments

Comments

@jdm
Copy link

jdm commented Feb 7, 2018

A diff like:

diff --git a/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order.html b/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order-new.html
similarity index 100%
rename from tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order.html
rename to tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order-new.html

gets excluded when I run filterdiff -p 1 -i tests/wpt/web-platform-tests.

@jdm
Copy link
Author

jdm commented Feb 7, 2018

@sergiomb2
Copy link
Contributor

can you post the diff file or a kind of sample please ?

@jdm
Copy link
Author

jdm commented Feb 7, 2018

More specifically, with this diff:

diff --git a/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order.html b/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order-new.html
similarity index 100%
rename from tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order.html
rename to tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order-new.html
diff --git a/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.scale.large.html b/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.scale.large.html
deleted file mode 100644
index 926530d1f7e6..000000000000
--- a/tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.scale.large.html
+++ /dev/null
@@ -1,33 +0,0 @@
-<!DOCTYPE html>
-<!-- DO NOT EDIT! This test has been generated by tools/gentest.py. -->
-<title>Canvas test: 2d.transformation.scale.large</title>
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<script src="/common/canvas-tests.js"></script>
-<link rel="stylesheet" href="/common/canvas-tests.css">
-<body class="show_output">
-
-<h1>2d.transformation.scale.large</h1>
-<p class="desc">scale() with large scale factors works</p>
-
-<p class="notes">Not really that large at all, but it hits the limits in Firefox.
-<p class="output">Actual output:</p>
-<canvas id="c" class="output" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
-<p class="output expectedtext">Expected output:<p><img src="/images/green-100x50.png" class="output expected" id="expected" alt="">
-<ul id="d"></ul>
-<script>
-var t = async_test("scale() with large scale factors works");
-_addTest(function(canvas, ctx) {
-
-ctx.fillStyle = '#f00';
-ctx.fillRect(0, 0, 100, 50);
-
-ctx.scale(1e5, 1e5);
-ctx.fillStyle = '#0f0';
-ctx.fillRect(0, 0, 1, 1);
-_assertPixel(canvas, 50,25, 0,255,0,255, "50,25", "0,255,0,255");
-
-
-});
-</script>
-

@jdm
Copy link
Author

jdm commented Feb 7, 2018

filterdiff collects the first five lines, so there are two diff headers, then throws them all out because pat_exclude and verbose are both false at https://github.com/twaugh/patchutils/blob/master/src/filterdiff.c#L1075-L1083.

@sergiomb2
Copy link
Contributor

filterdiff [[-i PATTERN] | [--include=PATTERN]] [[-I FILE] | [--include-from-file=FILE]] [[-p n]

what you want with -p -i ? is a syntax error

filterdiff: option requires an argument -- 'p'

cat yourfile | filterdiff works

@jdm
Copy link
Author

jdm commented Feb 7, 2018

Sorry, I meant -p 1.

@jdm
Copy link
Author

jdm commented Feb 7, 2018

If I use -v it is a workaround for this particular diff, but it incorrectly includes moves that do not match the pattern.

@sergiomb2
Copy link
Contributor

I don't understood the problem , is the -i ?

@jdm
Copy link
Author

jdm commented Feb 7, 2018

The problem is that the diff contains a change to a file (tests/wpt/web-platform-tests/2dcontext/transformations/2d.transformation.order.html) that matches the pattern provided to -i (tests/wpt/web-platform-tests), but the change is not included in the filtered diff.

@sergiomb2
Copy link
Contributor

sergiomb2 commented Feb 8, 2018

+++ /dev/null

as I understand -i just includes files in diff that match if you just have the delete file , what you except ?
Anyway you fail to provide the complete example , I still have to deduce what you mean and what example you use .

@jdm
Copy link
Author

jdm commented Mar 18, 2018

I posted the complete example in #22 (comment). When I filter that diff using filterdiff -p 1 -i tests/wpt/web-platform-tests it only shows the file that is removed, and does not include the file that is renamed.

@sergiomb2
Copy link
Contributor

sergiomb2 commented Mar 21, 2018

I think you expect something that filterdiff does not do , what you are asking to just show the diffs where file name have "tests/wpt/web-platform-tests"
So you need your git diff not detect similarity and show the rename files , but the real patch

@jdm
Copy link
Author

jdm commented Mar 21, 2018

Why is that not the expected behaviour of filterdiff?

@sergiomb2
Copy link
Contributor

you need your git diff not detect similarity and don't try detect renamed files and show the complete patch

@jdm
Copy link
Author

jdm commented Mar 21, 2018

My question is why filterdiff should not be modified to accommodate this diff?

@sergiomb2
Copy link
Contributor

man filterdiff - extract or exclude diffs from a diff file
if file doesn't not have the diff but have rename from / to , what you expect from filterdiff ?

@jdm
Copy link
Author

jdm commented Mar 21, 2018

I think "diff" is generally used to describe more than just the - and + parts of the diff file. I would consider the renaming information an important part of the diff.

@carlfriedrich
Copy link

I just encountered the same problem. @jdm Did you manage to find a workaround for this?

@jdm
Copy link
Author

jdm commented Feb 26, 2022

I no longer remember any of the context for this problem.

@sergiomb2
Copy link
Contributor

I'm pretty sure the cause is https://github.com/twaugh/patchutils/blob/master/src/filterdiff.c#L1070-L1073.

I see, we may try not exclude removes and files permissions as mention in #59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants