Skip to content

Conversation

Matth--
Copy link
Contributor

@Matth-- Matth-- commented Sep 2, 2020

The from and to in diffs did not include the full path name when
spaces are being used. This change allows spaces in the file paths
as well (no tabs).

@SpacePossum
Copy link
Contributor

Hi and nice find!

Just wondering if this can be tweaked even more to something like; (^---\h+(?P<file>[^\v]+)) as after --- (or +++) there can be any horizontal space (not 100% this must a single space in all the diff formats) and than whatever follows up a vertical line break or end of the string makes up the file name I think.

@Matth-- Matth-- force-pushed the fix-parse-with-spaces branch from fab05ce to 68f1ae7 Compare September 2, 2020 08:55
@Matth--
Copy link
Contributor Author

Matth-- commented Sep 2, 2020

Hi @SpacePossum

Good suggestions! I changed the commit.

I also added tabs to the exclusion list. This test also has timestamps after the filenames (with a leading tab).

If tabs are being used for filenames (why? 😄 ) it gets represented as \t and the filenames are surrounded with double-quotes.

diff --git "a/\ttabbed.txt" "b/\ttabbed2.txt"
index 9daeafb..e69de29 100644
--- "a/\ttabbed.txt"
+++ "b/\ttabbed2.txt"
@@ -1 +0,0 @@
-test

Would this cover everything?

@SpacePossum
Copy link
Contributor

Good catch on the timestamps! I really wished "they" formalized the diff file format in some standard :D
It looks good me!

@Matth--
Copy link
Contributor Author

Matth-- commented Sep 7, 2020

Any idea why the checks failed?

@Matth-- Matth-- force-pushed the fix-parse-with-spaces branch from 444bf51 to 68f1ae7 Compare September 7, 2020 12:54
@SpacePossum
Copy link
Contributor

diff --git a/src/Parser.php b/src/Parser.php
index 6090e81..cc9e388 100644
--- a/src/Parser.php
+++ b/src/Parser.php
@@ -37,8 +37,8 @@ public function parse(string $string): array
         $collected = [];
 
         for ($i = 0; $i < $lineCount; ++$i) {
-            if (preg_match('(^---\\s+(?P<file>\\S+))', $lines[$i], $fromMatch) &&
-                preg_match('(^\\+\\+\\+\\s+(?P<file>\\S+))', $lines[$i + 1], $toMatch)) {
+            if (preg_match('#^---\h+"?(?P<file>[^\\v\\t"]+)#', $lines[$i], $fromMatch) &&
+                preg_match('#^\\+\\+\\+\\h+"?(?P<file>[^\\v\\t"]+)#', $lines[$i + 1], $toMatch)) {
                 if ($diff !== null) {
                     $this->parseFileDiff($diff, $collected);
 
diff --git a/tests/ParserTest.php b/tests/ParserTest.php
index 7623970..30d9fe2 100644
--- a/tests/ParserTest.php
+++ b/tests/ParserTest.php
@@ -166,4 +166,45 @@ public function diffProvider(): array
             ],
         ];
     }
+
+    public function testParseWithSpacesInFileNames(): void
+    {
+        $content =
+<<<PATCH
+diff --git a/Foo Bar.txt b/Foo Bar.txt
+index abcdefg..abcdefh 100644
+--- a/Foo Bar.txt
++++ b/Foo Bar.txt
+@@ -20,4 +20,5 @@ class Foo
+     const ONE = 1;
+     const TWO = 2;
++    const THREE = 3;
+     const FOUR = 4;
+PATCH;
+
+        $diffs = $this->parser->parse($content);
+
+        $this->assertEquals('a/Foo Bar.txt', $diffs[0]->getFrom());
+        $this->assertEquals('b/Foo Bar.txt', $diffs[0]->getTo());
+    }
+
+    public function testParseWithSpacesInFileNamesAndTimestamp(): void
+    {
+        $content =
+            <<<PATCH
+diff --git a/Foo Bar.txt b/Foo Bar.txt
+index abcdefg..abcdefh 100644
+--- "a b.txt"  2020-10-02 13:31:52.938811371 +0200
++++ "a c.txt"  2020-10-02 13:31:50.022792064 +0200
+@@ -20,4 +20,5 @@ class Foo
+     const ONE = 1;
+     const TWO = 2;
++    const THREE = 3;
+     const FOUR = 4;
+PATCH;
+        $diffs = $this->parser->parse($content);
+
+        $this->assertEquals('a b.txt', $diffs[0]->getFrom());
+        $this->assertEquals('a c.txt', $diffs[0]->getTo());
+    }
 }

Hi again!

Above will allow for --- "a b.txt" 2020-10-02 13:31:52.938811371 +0200 as well (currently incorrectly returns the file name with the quotes attached). It also makes your fixture content inline so we don't need the file itself.
Let me know what you think!

About the test failing, not sure as it doesn't seem related to your changes. Maybe try to kick them again by committing something to the PR as the composer dependencies might be stuck in the cache.

The from and to in diffs did not include the full path name when
spaces are being used. This change allows spaces in the file paths
as well (no tabs).
@Matth-- Matth-- force-pushed the fix-parse-with-spaces branch from 68f1ae7 to 3c9aefe Compare October 7, 2020 07:05
@Matth--
Copy link
Contributor Author

Matth-- commented Oct 7, 2020

@SpacePossum Good catch!

I implemented your changes and the tests pass.

Do you know how I get that diff output (with timestamps and surrounding quotes)?

@SpacePossum
Copy link
Contributor

SpacePossum commented Oct 7, 2020

I made the diff examples using git diff on my setup.

If you are generating diffs using the project you can use this output builder and pass the file name with quotes:
https://github.com/sebastianbergmann/diff/blob/4.0.3/src/Output/StrictUnifiedDiffOutputBuilder.php
(we do this for example here as well https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/3.0/src/Differ/UnifiedDiffer.php#L37)
It also has options for timestamps.

The test failures are confusing to me, maybe Sebastian can help out with the setup when he has some time.

@SpacePossum
Copy link
Contributor

Ping @sebastianbergmann whenever you've got time, please let us know how we can help out, thanks! :)
This PR looks good to me, but the test setup seems broken.

@sebastianbergmann
Copy link
Owner

This PR looks good to me, but the test setup seems broken.

Maybe @localheinz has some insight why GitHub Actions (sometimes) fail for pull requests on my projects.

@SpacePossum If this is looks okay to you, then it is okay for me as well. I'll merge this which should (fingers crossed) give us a useful GitHub Actions build.

@sebastianbergmann sebastianbergmann merged commit e3ec605 into sebastianbergmann:master Oct 13, 2020
@SpacePossum
Copy link
Contributor

thanks so much :)
and 👋 to Andreas

thanks for the PR @Matth-- 👍

@localheinz
Copy link
Contributor

@sebastianbergmann

Could this be the composer root version problem again?

@sebastianbergmann
Copy link
Owner

Could be, don't know. Sorry!

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

Successfully merging this pull request may close these issues.

4 participants