Fix path_to_tree_path #635

r0mainK · 2018-06-14T09:12:49Z

A couple entangled issues here:

Incheck_ignore:

path_to_tree_path was not being used appropriately, basically path = os.path.relpath(path, r.path) was done twice
paths in index are always bytes, but paths passed to ignore_manager should always be strings: had to add some encoding/decoding

In path_to_tree_path:

not checking if the tree_path did indeed start by repo_path
types again, after all calls to path_to_tree_path the returned path is used to check if it's in the index: might as well always return bytes. also small bug possible when replacing the separator, if the input path was in fact bytes.

jelmer

Please add tests for behaviour you're modifying.

jelmer · 2018-06-14T09:58:35Z

dulwich/porcelain.py

@@ -170,17 +170,21 @@ def open_repo_closing(path_or_repo):
    return closing(Repo(path_or_repo))


-def path_to_tree_path(repopath, path):
+def path_to_tree_path(repo_path, tree_path):


See comments in other PR, please don't rename arguments.

jelmer · 2018-06-14T09:59:08Z

dulwich/porcelain.py

-    os.path.relpath(path, repopath)
+    if tree_path.startswith(repo_path):
+        tree_path = os.path.relpath(tree_path, repo_path)
+    if isinstance(tree_path, str):


This is invalid on Python2.

which part ? startswith is defined, as is isinstance, no ?

the isinstance bit; if something is a bytestring on python2 and it has non-ascii characters, then encoding it will lead to mayhem since it tries to convert from ascii:

"ĳ".encode('utf-8')
Traceback (most recent call last):
File "", line 1, in
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc4 in position 0: ordinal not in range(128)

ah right, then we can just do the opposite: not isinstance(f(path), bytes). just checked it seems to work in python2

jelmer · 2018-06-14T10:01:36Z

dulwich/porcelain.py

@@ -1189,9 +1193,12 @@ def check_ignore(repo, paths, no_index=False):
        ignore_manager = IgnoreFilterManager.from_repo(r)
        for path in paths:
            if os.path.isabs(path):
-                path = os.path.relpath(path, r.path)


This is intentional. paths specified here are relative to the current path, not to the repository root (similar to the behaviour of "git check-ignore").

hmm let me get this straight.

if at the start both path and r.path are already relative to the current path then you do not need this

if at the start you know for sure that r.path is relative to the current path, and path might be absolute, then the right code should be: path = os.path.relpath(path, os.getcwd()) no ?

Either path or r.path can be absolute paths, so we do need the call to os.path.relpath.

os.path.relpath will determine the absolute paths for path and r.path relative to os.getcwd() and then determine the relative path between them. E.g.:

% cd /tmp /tmp% python Python 2.7.15 (default, May 1 2018, 05:55:50) [GCC 7.3.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import os >>> os.path.relpath("foo", "/home/blah/bar") '../../../tmp/foo' >>> os.path.relpath("/tmp/foo", "/home/blah/bar") '../../../tmp/foo'

"foo" gets interpreted as "/tmp/foo" and then the relative path from /home/blah/bar is determined.

Just using path = os.path.relpath(path, os.getcwd()) would not get us a path relative to the repository root.

r0mainK · 2018-06-14T10:56:51Z

So I guess I have two follow up question after seeing the comments, before I add rewrite and add tests:

should path_to_tree_path assume that path and repopath are both relative paths (to the same dir) or not ?
regarding types, should there be assumptions on path and repopath's types, and is there a specific type one should expect as output ?

My personal opinion is that path and repopath should be assumed to be of the same type, either bytes or str, and that the output should always be bytes, given the usage of this function is exclusively to check if paths are in the Index. Furthermore, we should not have to check both paths are relative to the same path, the job should have been done previously.

jelmer · 2018-06-14T11:14:21Z

tree/index paths are supposed to be bytestrings (since they're git paths)

repopath and paths passed into the various porcelain functions are filesystem paths; they can be either bytes or strings, depending on the OS/Python version you're on. The porcelain functions should be able to deal with all this, hence the path_to_tree_path helper.

codecov · 2018-06-14T13:24:52Z

Codecov Report

Merging #635 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #635      +/-   ##
==========================================
+ Coverage   90.66%   90.72%   +0.05%     
==========================================
  Files          75       75              
  Lines       18976    18992      +16     
  Branches     2022     2018       -4     
==========================================
+ Hits        17205    17230      +25     
+ Misses       1353     1348       -5     
+ Partials      418      414       -4

Impacted Files	Coverage Δ
dulwich/tests/test_porcelain.py	`100% <100%> (ø)`	⬆️
dulwich/porcelain.py	`76.24% <100%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4529549...7125a45. Read the comment docs.

r0mainK · 2018-06-14T13:40:40Z

@jelmer ok thks I understand better now. So in the end no need to change the logic of check_ignore, also since path_to_tree_path is only a helper function that makes sure the input path is bytes with the correct separator I just removed the line os.path.relpath(...) that did nothing and took out the repopath from the input, since not used. Also added a test.

jelmer

Please also add a tests that fail without your changes to get_untracked_paths/check_ignore.

jelmer · 2018-06-14T14:37:25Z

dulwich/porcelain.py

    """
-    os.path.relpath(path, repopath)
+    if not isinstance(path, bytes):
+        path = path.encode(DEFAULT_ENCODING)


You'll want to decode from sys.getfilesystemencoding() if original path was a bytestring filesystem path.

E.g. the filesystem path can be iso8859, while the git encoding is 'utf-8'.

jelmer · 2018-06-14T14:38:00Z

dulwich/porcelain.py

@@ -387,7 +388,7 @@ def remove(repo=".", paths=None, cached=False):
        index = r.open_index()
        for p in paths:
            full_path = os.path.abspath(p).encode(sys.getfilesystemencoding())
-            tree_path = path_to_tree_path(r.path, p)
+            tree_path = path_to_tree_path(p)


tree_path needs to be relative to the repository path; where is this enforced now? os.getcwd() is not necessarily r.path.

Well it was never enforced here apparently, as you can see from the diff path_to_tree_path did not enforce it before since the call to os.path.relpath did not change the value of the returned path

Agreed. IMO that was the bug, not that r.path was passed into path_to_tree_path in the first place.

jelmer · 2018-06-15T12:15:43Z

A related bug is #598

r0mainK · 2018-06-25T14:21:37Z

@jelmer hey, sorry for being afk last week ^^" anyway I looked into #598 and it was indeed related. With the changes I brought it worked for me.
So now here is the flow of path_to_tree_path:

Convert both path and repopath to bytes, replace separator if needed.
Test if path is relative to repopath. Can't use os.path.join because it detects common absolute paths, so I do (repopath + b'/' + path).replace(b'//', b'/'). If this exists, then we can return path directly.
If not, at this point we know both path and repopath can be absolute or relative to CWD. If one is absolute and the other relative, we transform the relative to absolute, then return relpath(path, repopath)

I also added tests for path_to_tree_path. Regarding the builds, two test suites are failing (Python:2.7 and Python: pypy) with a lot of errors, but the errors do not seem related to my code, e.g.:

ERROR: test_simple (dulwich.tests.test_porcelain.LsFilesTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dulwich/tests/test_porcelain.py", line 1261, in test_simple
    porcelain.add(repo=self.repo.path, paths=[fullpath])
  File "dulwich/porcelain.py", line 386, in add
    if ignore_manager.is_ignored(relpath):
  File "dulwich/ignore.py", line 336, in is_ignored
    matches = list(self.find_matching(path))
  File "dulwich/ignore.py", line 308, in find_matching
    raise ValueError('%s is an absolute path' % path)
ValueError: /tmp/tmpbM2QIK/repo/foo is an absolute path

In this error path_to_tree_path is not called, and I did not touch anything else. By the way, I saw you just did a release but I would rly appreciate if you could do a new one once this gets merged, be it as is or after changes. I've finished a project that relies on dulwich last week, and it fails because of the #598 right now ^^".

jelmer · 2018-06-25T18:30:44Z

dulwich/porcelain.py

-    :param repo: Repository
-    :param path: A path
+    :param repopath: Repository path, absolute or relative to the cwd
+    :param path: A path, absolute or relative to either the cwd or repopath


Allowing relative paths to either cwd or repopath makes the behaviour of this function unpredictable. This should just be cwd (to be compatible with c git).

I agree, this was to stay compatible with check ignore command which seemed, according to tests, to necessitate this. Should i change tests or logic of that function then ?

dulwich.porcelain.check_ignore should also behave similarly to "git check-ignore". It looks like this will indeed require changing the tests for check_ignore.

jelmer · 2018-06-25T18:31:16Z

dulwich/porcelain.py

+            os.path.sep.encode(sys.getfilesystemencoding()), b'/')
+        repopath = repopath.replace(
+            os.path.sep.encode(sys.getfilesystemencoding()), b'/')
+    if os.path.exists((repopath + b'/' + path).replace(b'//', b'/')):


See above - this makes the behaviour of the function unpredictable, relative paths should only ever be interpreted in one way.

jelmer · 2018-06-25T18:32:09Z

dulwich/tests/test_porcelain.py

+            ".", path))
+        self.assertEqual(b"bar/baz", porcelain.path_to_tree_path(
+            cwd, "bar/baz"))
+        os.mkdir("foo")


Please use TestCaseInTempDir if you're going to create files/directories, otherwise we're leaving these all over the source directory.

My bad, Will change first thing tomoreiw

r0mainK · 2018-06-26T12:51:20Z

@jelmer did the changes you asked, but again tests for Python2.7 and PyPy are failing, do you know what this is about ?

jelmer · 2018-06-26T13:03:08Z

On Tue, Jun 26, 2018 at 05:53:50AM -0700, Romain Keramitas wrote: @jelmer did the changes you asked, but again tests for Python2.7 and PyPy are failing, do you know what this is about ?

I'll take a look this evening.

r0mainK · 2018-07-02T09:20:19Z

@jelmer ping - I know you made comments on the other PR that I will address, but since it is a feature for add which uses path_to_tree_path via get_untracked_pathsI would like to get this merged before in all cases. It fixes, as far as I know, #598 and again tests suites for Python2.7 and Pypy are currently failing for reasons I do not understand, if you could have a look Id really appreciate.

jelmer

This mostly looks good, but I've left two more comments. Can you add a test case for #598 as well?

jelmer · 2018-07-02T20:57:46Z

dulwich/tests/test_porcelain.py

+
+class HelperTests(PorcelainTestCase):
+
+    def test_path_to_tree_path_base(self):


can you perhaps also add a test for the situation where the path is outside of repopath? That should raise an exception of some sort.

Sure, then I'll add a ValueError to be raised when the returned path starts with ..

jelmer · 2018-07-02T20:57:58Z

dulwich/tests/test_porcelain.py

        self.assertEqual(
            ['foo'],
-            list(porcelain.check_ignore(self.repo, ['foo'], no_index=True)))
+            list(porcelain.check_ignore(self.repo, [path], no_index=True)))


Perhaps add a check with a relative path too.

jelmer · 2018-07-02T21:05:12Z

dulwich/porcelain.py

            if not no_index and path_to_tree_path(r.path, path) in index:
                continue
+            if os.path.isabs(path):


I suspect this change is what made the python2.7 tests fail.

thks, gonna check it out

r0mainK · 2018-07-03T10:01:48Z

@jelmer added the 3 tests you wanted, and checked if it was the continue block the problem but it doesnt seem so, I wonder if Travis isnt broken for some reason - like when it seemingly cant find refs Oo

r0mainK · 2018-07-03T16:16:39Z

OK so the error was in the tests I added for path_to_tree_path, more precisely when testing the case where the separator is different from /. This allowed me to see that os.path library of python 2 and 3 is not compatible in this regard:

in python 2, I had to change the separator after I had done relpath:

    if os.path.sep != '/':
        treepath = treepath.replace(
            os.path.sep.encode(sys.getfilesystemencoding()), b'/')

-> or the returned tree_path was an absolute path: b'/Users/romain/dulwich/bar/baz'

in python 3, I had to do it before:

    if os.path.sep != '/':
        path = path.replace(
            os.path.sep.encode(sys.getfilesystemencoding()), b'/')
        repopath = repopath.replace(
            os.path.sep.encode(sys.getfilesystemencoding()), b'/')

-> or a ValueError is raised because tree_path=b'../@sepUsers@sepromain@sepdulwich@sepbar@sepbaz'

So I have taken this test and feature out.

Signed-off-by: Romain Keramitas <[email protected]>

r0mainK · 2018-07-17T10:27:40Z

@jelmer pings, the appveyor build is failing because of change detailed previous comment, tell me what you would rather do.

jelmer · 2018-07-18T10:12:51Z

Thanks, Merged!

r0mainK mentioned this pull request Jun 14, 2018

[WIP] Fix and PEP8 #633

Closed

jelmer reviewed Jun 14, 2018

View reviewed changes

r0mainK changed the title ~~Fix path_to_tree_path and check_ignore~~ Fix path_to_tree_path Jun 14, 2018

alistair-broomhead mentioned this pull request Jun 18, 2018

Prevent clean files from showing up as untracked #601

Open

jelmer reviewed Jun 25, 2018

View reviewed changes

jelmer requested changes Jul 2, 2018

View reviewed changes

Fix path_to_tree_path

7125a45

Signed-off-by: Romain Keramitas <[email protected]>

jelmer merged commit 7125a45 into jelmer:master Jul 18, 2018


		class HelperTests(PorcelainTestCase):

		def test_path_to_tree_path_base(self):

Fix path_to_tree_path #635

Fix path_to_tree_path #635

Conversation

r0mainK commented Jun 14, 2018

jelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelmer Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

r0mainK commented Jun 14, 2018

jelmer commented Jun 14, 2018

codecov bot commented Jun 14, 2018 • edited Loading

Codecov Report

r0mainK commented Jun 14, 2018

jelmer left a comment

Choose a reason for hiding this comment

jelmer Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0mainK Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelmer commented Jun 15, 2018

r0mainK commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0mainK commented Jun 26, 2018

jelmer commented Jun 26, 2018 via email

r0mainK commented Jul 2, 2018

jelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0mainK commented Jul 3, 2018

r0mainK commented Jul 3, 2018 • edited Loading

r0mainK commented Jul 17, 2018

jelmer commented Jul 18, 2018

jelmer Jun 14, 2018 •

edited

Loading

codecov bot commented Jun 14, 2018 •

edited

Loading

jelmer Jun 14, 2018 •

edited

Loading

r0mainK Jun 14, 2018 •

edited

Loading

r0mainK commented Jul 3, 2018 •

edited

Loading