Improved support for navigating trees: #903

rcoup · 2019-04-26T13:51:23Z

Navigating tree objects is pretty crude via Pygit2 at the moment, requiring a lot of boilerplate lookups.

This change incorporates the same / operator approach as Pathlib and GitPython:

my_tree / 'path' resolves to a TreeEntry (same as my_tree['path'] currently)
If a TreeEntry is of type tree, then:
- my_tree_entry / 'path' resolves to a child TreeEntry
- my_tree_entry['path'] and my_tree_entry[0] similarly
- 'path' in my_tree_entry returns a boolean.
this enables my_tree / 'path' / 'path' navigation

Ideally I'd suggest that maybe Tree should probably be a subclass of TreeEntry so it kinda magically works how the user expects, but that would require quite a few changes.

jdavid · 2019-04-27T06:20:45Z

Looks good! Could you fix the tests in Travis? Don't worry about AppVeyor, they're broken for a different reason.
And update the documentation please. Thanks!

rcoup · 2019-04-30T11:09:00Z

Been using this the last couple of days... it's much better to navigate the tree, but still a bit awkward to actually get to blobs.

Before:

meta_tree = repo[commit.tree['meta'].id]
features_tree = repo[meta_tree['features'].id]
info_blob = repo[features_tree['info.json'].id]
info = json.loads(info_blob.data)

Now:

feature_info = commit.tree / 'meta' / 'features' / 'info.json'
info = json.loads(repo[feature_info.id].data)

With latest commit incorporating .blob and .tree properties.

feature_info = commit.tree / 'meta' / 'features' / 'info.json'
info = json.loads(feature_info.blob.data)

Thoughts?

Submodules end up with a TreeEntry.type=commit, but I can't find a way to lookup the submodule from the oid, only from the absolute path (which we don't have in the TreeEntry, only the relative name) — so I've left that alone.

rcoup · 2019-04-30T11:27:29Z

@jdavid can you give the ref-counting/free'ing a review?

I'm Incrementing the repo refcount when it's associated with a TreeEntry
and clearing it when the TreeEntry is dealloc'd.
AFAICT using the various wrap_ methods means the libgit2-alloc'd items will get cleaned up
I'm freeing the temporary git_tree subtrees I use in getitem/truediv

src/tree.c

jdavid · 2019-05-04T08:17:12Z

Good work! The ref-counting/free'ing looks correct. Just made a couple of tiny comments.

About the user interface:

Would it be simpler for the user to have treeentry.obj instead of treentry.blob and treeentry.tree?
Why not to support treeentry / 0? That would make _getitem and _truediv to behave the same.

I think these changes would be nice for the user, and they would simplify a bit the code as well.

dsully · 2019-05-07T21:24:56Z

These changes look fantastic!

rcoup · 2019-05-08T09:31:50Z

I considered both of these, I'll add my thoughts below. Happy to be challenged.

Would it be simpler for the user to have treeentry.obj instead of treentry.blob and treeentry.tree?

I like the elegance, but I can't think of an actual use-case for it? Seems like either I'd expect a blob or another tree for any particular case? "Explicit is better than implicit" and all.

Commits are another type possibility wrt submodules, but I've left that for someone else when they need it.

Why not to support treeentry / 0? That would make _getitem and _truediv to behave the same.

If I'm iterating through everything in a directory then I'd always use an iterator or for/getitem. I know it makes it simpler for the implementation, but can't see how it helps the user?

jdavid · 2019-05-10T21:16:43Z

The fact that these two changes make the code simpler is a big advantage. It's difficult to stress it enough, 150+ developers have contributed changes to pygit2, and today I maintain those changes/code (and when I leave someone else will do).

On the other side, the fact that we cannot imagine a use case, doesn't mean it doesn't exist, and more importantly it doesn't hurt, it's a feature superset anyway.

rcoup · 2019-06-06T14:54:41Z

@jdavid sorry, been distracted with other work. I've made both the changes you suggested. 👍

- `my_tree / 'path'` resolves to a TreeEntry (same as `my_tree['path']`) - If `my_tree_entry` is of type `tree`: `my_tree_entry / 'path'`, `my_tree_entry['path']`, and `my_tree_entry[0]` all resolve to another TreeEntry (same as `repo[my_tree_entry.id]['path']`) This enables `my_tree / 'path' / 'path'` navigation ala Pathlib & GitPython.

Saves doing another `repo[tree_entry.id]` lookup Tree navigation: `blob = (tree / 'path' / 'file').blob`

- drop .blob & .tree in favour of .obj - use getitem implementation for truediv: allows `T / 'a' / 0` to correspond to `T['a'][0]`

rcoup · 2019-06-10T10:15:36Z

Rebased on latest master

rcoup · 2019-06-10T12:03:42Z

Don't worry about AppVeyor, they're broken for a different reason.

hmm, not so sure about that?

...
C:\Users\appveyor\AppData\Local\Programs\Common\Microsoft\Visual C++ for Python\9.0\VC\Bin\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -IC:\projects\pygit2\venv\include -IC:\Python27\include -IC:\Python27\PC /Tcsrc\tree.c /Fobuild\temp.win32-2.7\Release\src\tree.obj
tree.c
src\tree.c(199) : error C2143: syntax error : missing ';' before 'type'
src\tree.c(200) : error C2065: 'err' : undeclared identifier
src\tree.c(201) : error C2065: 'err' : undeclared identifier
src\tree.c(208) : error C2143: syntax error : missing '{' before '*'
src\tree.c(220) : error C2143: syntax error : missing ';' before 'type'
src\tree.c(221) : error C2065: 'err' : undeclared identifier
src\tree.c(222) : error C2065: 'err' : undeclared identifier
src\tree.c(226) : warning C4133: 'return' : incompatible types - from 'PyObject *' to 'int *'
src\tree.c(240) : warning C4133: 'return' : incompatible types - from 'int *' to 'PyObject *'
src\tree.c(385) : warning C4028: formal parameter 1 different from declaration
src\tree.c(385) : warning C4133: 'initializing' : incompatible types - from 'TreeEntry *(__cdecl *)(TreeEntry *,PyObject *)' to 'binaryfunc'
src\tree.c(539) : warning C4090: 'function' : different 'const' qualifiers
src\tree.c(836) : warning C4028: formal parameter 1 different from declaration
src\tree.c(836) : warning C4133: 'initializing' : incompatible types - from 'TreeEntry *(__cdecl *)(Tree *,PyObject *)' to 'binaryfunc'
...

rcoup · 2019-06-10T21:57:41Z

@jdavid at least one of the Windows build configs passed now 👍 Current appveyor failure looks intermittent, can you trigger a rebuild?

Config.__init__() does not always set self._config, which can cause trouble during __del__(). This avoids seemingly spurious and not really helpful exceptions. Fixes libgit2#916

jdavid · 2019-06-13T17:10:10Z

Thanks!

Yes I fixed AppVeyor in master. I've re-run the incomplete jobs several times, but there're still failures.

rcoup · 2019-06-13T21:48:53Z

@jdavid aha, there's an actual compile error there now. Will figure it out somehow :)

- `my_tree / 'path'` resolves to a TreeEntry (same as `my_tree['path']`) - If `my_tree_entry` is of type `tree`: `my_tree_entry / 'path'`, `my_tree_entry['path']`, and `my_tree_entry[0]` all resolve to another TreeEntry (same as `repo[my_tree_entry.id]['path']`) This enables `my_tree / 'path' / 'path'` navigation ala Pathlib & GitPython.

Saves doing another `repo[tree_entry.id]` lookup Tree navigation: `blob = (tree / 'path' / 'file').blob`

- drop .blob & .tree in favour of .obj - use getitem implementation for truediv: allows `T / 'a' / 0` to correspond to `T['a'][0]`

jdavid · 2019-06-16T12:10:00Z

fixed in commit f0f8cc2

…e-nav

rcoup · 2019-06-25T15:04:50Z

@jdavid pulled your changes into here. ✅

jdavid · 2019-06-26T07:51:34Z

Merged thanks!

I still want to give a 2nd look. Specifically this adds a reference to the repository from TreeEntry. I wonder whether it would be possible to implement the same feature without that reference, and in a way that is as friendly to the user. But have not yet stopped to think about it, I'll open an issue if I've some idea.

rcoup · 2019-06-26T08:18:42Z

Thanks ❤️

this adds a reference to the repository from TreeEntry. I wonder whether it would be possible to implement the same feature without that reference

Yeah, needs the repo reference to be able to look up the associated objects, but is a small wart implementation-wise.

One alternative is to keep a reference to the parent/root tree, store one repository reference there, and access everything that way — but then TreeEntry's would need a parent reference instead: I decided the outcome wasn't much different, though it's arguably more elegant (I guess that would allow support for mytreeentry / ".." / "foo" too).

Right at the start I mentioned that I think ideally Tree should be a subclass of TreeEntry so no matter where you start everything is the same, maybe that's something to change as well. Though right now at least they work effectively the same.

rcoup force-pushed the better-tree-nav branch from 9fe3896 to 0613677 Compare April 30, 2019 13:55

jdavid requested changes May 4, 2019

View reviewed changes

src/tree.c Outdated Show resolved Hide resolved

src/tree.c Outdated Show resolved Hide resolved

rcoup added 8 commits June 10, 2019 11:14

Py2 compatibility fix

2993675

TreeEntry: fix repo refcount

dbcd2be

TreeEntry: add .blob and .tree properties

e13188e

Saves doing another `repo[tree_entry.id]` lookup Tree navigation: `blob = (tree / 'path' / 'file').blob`

Improve docs

7db388e

TreeEntry dealloc fix

17f512a

Review tweaks

a6c6c63

Review changes:

dc3d86e

- drop .blob & .tree in favour of .obj - use getitem implementation for truediv: allows `T / 'a' / 0` to correspond to `T['a'][0]`

rcoup force-pushed the better-tree-nav branch from b7859ce to dc3d86e Compare June 10, 2019 10:15

Fix some compiler warnings and improve compiler compatibility.

18db15c

Do not throw exception when Config.__init__() failed

a4b42ce

Config.__init__() does not always set self._config, which can cause trouble during __del__(). This avoids seemingly spurious and not really helpful exceptions. Fixes libgit2#916

rcoup added 4 commits June 14, 2019 15:56

Py2 compatibility fix

42f14d7

TreeEntry: fix repo refcount

23e603a

TreeEntry: add .blob and .tree properties

8215bc9

Saves doing another `repo[tree_entry.id]` lookup Tree navigation: `blob = (tree / 'path' / 'file').blob`

rcoup and others added 6 commits June 14, 2019 15:56

Improve docs

0771bcf

TreeEntry dealloc fix

3e9b694

Review tweaks

8bcb129

Review changes:

909339d

- drop .blob & .tree in favour of .obj - use getitem implementation for truediv: allows `T / 'a' / 0` to correspond to `T['a'][0]`

Fix some compiler warnings and improve compiler compatibility.

a36d394

Appveyor: fix for Python 3.4

f0f8cc2

Merge remote-tracking branch 'origin/better-tree-nav' into better-tre…

0bbd717

…e-nav

jdavid merged commit 0bbd717 into libgit2:master Jun 26, 2019

rcoup deleted the better-tree-nav branch June 26, 2019 08:18

jdavid mentioned this pull request Jul 7, 2019

Tree navigation: return objects #924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved support for navigating trees: #903

Improved support for navigating trees: #903

rcoup commented Apr 26, 2019

jdavid commented Apr 27, 2019

rcoup commented Apr 30, 2019

rcoup commented Apr 30, 2019

jdavid commented May 4, 2019

dsully commented May 7, 2019

rcoup commented May 8, 2019 •

edited

Loading

jdavid commented May 10, 2019

rcoup commented Jun 6, 2019

rcoup commented Jun 10, 2019

rcoup commented Jun 10, 2019

rcoup commented Jun 10, 2019

jdavid commented Jun 13, 2019

rcoup commented Jun 13, 2019

jdavid commented Jun 16, 2019

rcoup commented Jun 25, 2019

jdavid commented Jun 26, 2019

rcoup commented Jun 26, 2019

Improved support for navigating trees: #903

Improved support for navigating trees: #903

Conversation

rcoup commented Apr 26, 2019

jdavid commented Apr 27, 2019

rcoup commented Apr 30, 2019

rcoup commented Apr 30, 2019

jdavid commented May 4, 2019

dsully commented May 7, 2019

rcoup commented May 8, 2019 • edited Loading

jdavid commented May 10, 2019

rcoup commented Jun 6, 2019

rcoup commented Jun 10, 2019

rcoup commented Jun 10, 2019

rcoup commented Jun 10, 2019

jdavid commented Jun 13, 2019

rcoup commented Jun 13, 2019

jdavid commented Jun 16, 2019

rcoup commented Jun 25, 2019

jdavid commented Jun 26, 2019

rcoup commented Jun 26, 2019

rcoup commented May 8, 2019 •

edited

Loading