[FEATURE] Add walks from cwd and not repo root #638
Conversation
r0mainK
changed the title
|
In general, "dulwich.porcelain.X" should work like the "git X" command, including behaviour around os.getcwd(). Users that don't want to change the cwd can specify absolute paths. Obviously there are some exceptions to this, e.g. allowing richer data types for inputs/outputs. Adding an "all" argument and not defaulting to adding everything seems reasonable to me. |
r0mainK
changed the title
|
Okay, so gonna add this functionality. Now if |
dulwich/tests/test_porcelain.py
Outdated
| @@ -57,7 +57,8 @@ class PorcelainTestCase(TestCase): | |||
|
|
|||
| def setUp(self): | |||
| super(PorcelainTestCase, self).setUp() | |||
| self.test_dir = tempfile.mkdtemp() | |||
| self.test_dir = "/Users/romain/tests_dulwich" | |||
There was a problem hiding this comment.
This is what's breaking the tests on travis. It doesn't have a directory called /Users/romain ;)
There was a problem hiding this comment.
aha my bad, i got my tmp dir symlinked to /private/tmp so I switched to this, or tests failed :p but wan't talking about that, before there was a bug were python2.7 and pypy failed but other succeeded
There was a problem hiding this comment.
@jelmer like this one for the other PR: https://travis-ci.org/dulwich/dulwich/builds/396828073?utm_source=github_status&utm_medium=notification
| get_untracked_paths(os.getcwd(), r.path, r.open_index())) | ||
| relpaths = [] | ||
| if not isinstance(paths, list): | ||
| paths = [paths] | ||
| for p in paths: | ||
| relpath = os.path.relpath(p, r.path) | ||
| if all: |
There was a problem hiding this comment.
What is this trying to do? I don't understand why further manipulation is needed if all is set here.
| @@ -58,6 +58,7 @@ class PorcelainTestCase(TestCase): | |||
| def setUp(self): | |||
| super(PorcelainTestCase, self).setUp() | |||
| self.test_dir = tempfile.mkdtemp() | |||
| os.mkdir(self.test_dir) | |||
There was a problem hiding this comment.
This is unnecessary, tempfile.mkdtemp already creates the directory.
| porcelain.commit(repo=self.repo.path, message=b'test', | ||
| author=b'test <email>', | ||
| committer=b'test <email>') | ||
| finally: |
There was a problem hiding this comment.
Verify that the right files were added to the index?
| with open(os.path.join(self.repo.path, 'foo', 'blie'), 'w') as f: | ||
| f.write("\n") | ||
|
|
||
| porcelain.add(repo=self.repo.path, all=True, |
There was a problem hiding this comment.
Shouldn't this verify that the changes to a file are also added if one of the files existed before?
Signed-off-by: Romain Keramitas <r.keramitas@gmail.com>
|
@r0mainK ping |
This might not be a fix but an implementation choice, but given the change is simple I thought I'd open a PR and see what happens.
CONTEXT
Currently when using add and not passing any paths to add, we walk though the current working directory instead of the repo root. This means that we are basically doing a
git add ., assuming that the current working directory is in the repo, thus requiring to change the working directory if we are not located in it. It also means if we are using dulwich as an API in another git repo, let's say A, to modify B, then we will walk through A instead of our target B, even if therepoargu point toB.CHANGES
I think one of three two can and should be done:
allargument to walk through the repo root would be useful, and better then forcing the user to switch working directory.all, as the current doc implies, then switchingos.getcwd()tor.pathlike I did does the trick.