Note this is untested.

Could you rebase the patch against the latest master, I like to keep the history linear when possible :)

Here you are :)

Ok after testing it locally, I have adjusted it as follow:

diff --git a/ pagure/ui/repo.py b/ pagure/ui/repo.py
index b949679..9902686 100644
--- a/ pagure/ui/repo.py        
+++ b/ pagure/ui/repo.py        
@@ -89,11 +89,10 @@ def view_repo(repo, username=None):
                     and os.path.normpath(content) == content \
                     and not os.path.isabs(content):
                 try:
-                    traversal_item = tree
-                    for traversal_path in content.split(os.sep):
-                        traversal_item = traversal_item[traversal_path]
-                    if traversal_item.filemode == pygit2.GIT_FILEMODE_BLOB:
-                        content = repo_obj[traversal_item.oid].data
+                    content = __get_file_in_tree(
+                        repo_obj, tree, content.split(os.sep))
+                    if content:
+                        content = content.data
                 except KeyError:
                     pass
             readme, safe = pagure.doc_utils.convert_readme(

It seems to be working for me, how does it sound to you?

1. try-except seems unnecessary then
2. in highly unlikely case content is coerced to False in the comparsion, you'll end up with content being some encapsulated object with rather undefined consequences in the subsequent code as it won't get redefined with its data attribute (i.e., better to use a separate temporary variable)
3. check if the result of __get_file_in_tree is a plain blob would be appropriate (as in the original proposal)
4. content.split(os.sep) vs. content.split('/') as in the rest of the code (applies to the original patch as well): would check what pygit2 uses internally, perhaps it even symbolically defines its own authoritative separator

1. try-except seems unnecessary then

Indeed

1. in highly unlikely case content is coerced to False in the comparsion, you'll end up with content being some encapsulated object with rather undefined consequences in the subsequent code as it won't get redefined with its data attribute (i.e., better to use a separate temporary variable)

content is directly rendered in the output, so there is no later reliance on
.data.
But, using a different variable would solve any potential problem you're right.

1. check if the result of __get_file_in_tree is a plain blob would be appropriate (as in the original proposal)

I believe this is taken care of directly by __get_file_in_tree.

1. content.split(os.sep) vs. content.split('/') as in the rest of the code (applies to the original patch as well): would check what pygit2 uses internally, perhaps it even symbolically defines its own authoritative separator

TBH, I didn't know of the os.sep but I find it much nicer than the approach
I was using, so I'd rather update the code (in a different PR) to use
os.sep.

New version:

diff --git a/ pagure/ui/repo.py b/ pagure/ui/repo.py
index b949679..5cd6c01 100644
--- a/ pagure/ui/repo.py        
+++ b/ pagure/ui/repo.py        
@@ -89,11 +89,10 @@ def view_repo(repo, username=None):
                     and os.path.normpath(content) == content \
                     and not os.path.isabs(content):
                 try:
-                    traversal_item = tree
-                    for traversal_path in content.split(os.sep):
-                        traversal_item = traversal_item[traversal_path]
-                    if traversal_item.filemode == pygit2.GIT_FILEMODE_BLOB:
-                        content = repo_obj[traversal_item.oid].data
+                    tmp_content = __get_file_in_tree(
+                        repo_obj, tree, content.split(os.sep))
+                    if tmp_content:
+                        content = tmp_content.data
                 except KeyError:
                     pass
             readme, safe = pagure.doc_utils.convert_readme(

If you are ok with these changes, feel free to add them to your PR and I'll merge it :)

This is the last PR I'd like to merge before the next release, do you want to approve the changes or are there still changes you would like me to do?

Looking at it right now...

Re 1.: not removed yet, will remove in the fixed commit to come
Re 4.: we should be using plain '/' really, as os.path is platform-specific and we should stick with what pygit2/libgit2 uses:
https://github.com/libgit2/libgit2/blob/v0.23.1/src/tree.c#L831

cool, wfm then :)

Actually now that I looked into the pygit2 + libgit2 sources, we can item-access tree object with composite path directly, which:

• makes code more lightweight, and
• (relying directly on traversal in C), also faster

Note that current implementation of __get_file_in_tree could also make use of a direct access using composite path using the same approach. Also note that I don't believe it currently enforces plain blob only as is expected in case of symlinked README (symlink-to-symlink? strange, IMHO, to executable ditto ... are there even more possibilities?)

Does it work for you like this?

Sounds good to me but there is one thing to be careful with with pygit2 it's its tendency to break ABI/API from release to release.

Currently we support 0.21.4 (F21 and EL7), 0.22.0 (F22) but not 0.22.1 nor 0.23, so if you solution works with all versions great otherwise we should find a way to handle this.

Nice and clean indeed, let me test this

Seems to work great!

Sorry was testing the wrong page:

  File "/home/pingou/repos/gitrepo/pagure/pagure/ui/repo.py", line 92, in view_repo
    dereferenced = tree[content]
TypeError: list indices must be integers, not str

Ouch, looking...

Is it better now? (Confused by tree not being tree object, sorry)

Btw. the requested functionality should be present in pygit2 at least as of v0.18.0 and libgit2 as of v0.19.0

No more luck:

File "/home/pingou/repos/gitrepo/pagure/pagure/ui/repo.py", line 97, in view_repo content = dereferenced.data AttributeError: '_pygit2.TreeEntry' object has no attribute 'data'

If I print dereferenced it indeed show up as a TreeEntry and if I use if isinstance(tree, pygit2.Blob): I skip the error but the content displayed is still not the content of the README.

Last attempt before resorting to your change + my polish...

Working \ó/

Next step, adjust __get_file_in_tree to use this as well, but that can be a different PR :)

Thanks for your work!

Would you mind rebasing it on the top of master? This way we'll keep the history linear :)

Hurray! Repushed.