Skip to content

Conversation

mojavelinux
Copy link
Contributor

No description provided.

@mojavelinux
Copy link
Contributor Author

I'm looking for someone to review this PR and provide me with some feedback.

@cjhoward92
Copy link
Collaborator

cjhoward92 commented Mar 13, 2018

I think this looks pretty good. Was there some issue you were trying to solve with this? Were you having problems? I am curios as to why this is an issue and what, if anything, could be broken by this change (it seems pretty innocuous).

Thanks for the contribution!

@cjhoward92
Copy link
Collaborator

Was tree_entry.path() returning broken paths on windows? Something like C:/path/to\\file\\location?

@mojavelinux
Copy link
Contributor Author

To start, what nodegit returns differs from what the native git client (git ls-tree) returns, even on Windows. On all platforms, paths in a git repository are reported in posix form (using forward slash as the path separator).

$ git ls-tree --name-only -r HEAD
README.md
lib/README.md
lib/blame.js
...
test/tests/blame.js
...

Where this becomes a problem is when I'm building paths in the TreeWalker. On Windows, I keep having to change backslashes to forward slashes. Otherwise, when I turn around to retrieve a path from the repository, it isn't found. To me, that's where the main inconsistently comes from. If I give a path back to nodegit that nodegit gave to me, it can't find the path in the repository.

There's really no place for backslashes here. Paths in a git repository do not represent real paths (until the part of a checkout). They represent part of the object's identity. Therefore, nodegit shouldn't report different values (and thus break the object's identity) when run on different operating systems.

@implausible implausible merged commit 53e2e66 into nodegit:master Mar 19, 2018
@mojavelinux
Copy link
Contributor Author

Thanks!

@mojavelinux mojavelinux deleted the issue-1433 branch March 19, 2018 19:24
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.

3 participants