@ryanlerch Can you rebase this on current git master?

Also, does this mean that syntax highlighting can be made better so that we don't need to limit what constitutes "highlighted" code?

Pretty please pagure-ci rebuild

Also, does this mean that syntax highlighting can be made better so that we don't need to limit what constitutes "highlighted" code?

AFAIK, Diffs were never syntax highlighted, so not sure what you mean here...

Well, since you added a way to do syntax highlighting client-side, couldn't you change it to prefer that for all syntax highlighting of code when JS is available (like in the file view)?

Pretty please pagure-ci rebuild

Also, the syntax highlight style is different from the syntax highlighting used everywhere else (it's not solarized light)...

@pingou it seems the "fix" i used for the unicode filenames issue here doesnt work on jenkins. It worked locally FWIW.

any thoughts on how to fix this better?

We already have a method to do this, I think we could re-use it with little changes if we want to return JSON instead of plain text, cf pagure.ui.repo.view_commit_diff

Which version is this?

I don't think we need the README in the sources :)

Do we want to keep all the styles?

Not sure you want to keep this change :)

Or this one (and the ones below) :)

This passes, but see the comment above, I think we're being a little too short in what we test thus this doesn't catch the extras d added in the templates

@pingou it seems the "fix" i used for the unicode filenames issue here doesnt work on jenkins. It worked locally FWIW.
any thoughts on how to fix this better?

Let's keep this to a different PR, this one is already quite large as it is :)

Also, the syntax highlight style is different from the syntax highlighting used everywhere else (it's not solarized light)...

Well since the PR page hasn't been restyled, it's likely the opposite, everywhere else isn't using the same highlighting as this one :)

Well, since you added a way to do syntax highlighting client-side, couldn't you change it to prefer that for all syntax highlighting of code when JS is available (like in the file view)?

One PR at a time, but I think it's a good idea :)

@pingou it seems the "fix" i used for the unicode filenames issue here doesnt work on jenkins. It worked locally FWIW.
any thoughts on how to fix this better?

Let's keep this to a different PR, this one is already quite large as it is :)

The issue here is that the tests already there test with a repo with a unicode char in a filename. So the tests failed on this PR :(

The issue here is that the tests already there test with a repo with a unicode char in a filename. So the tests failed on this PR :(

Hm, ok, this is passing now but doesn't with this refactorring. I'll try to look at it :)

The issue here is that the tests already there test with a repo with a unicode char in a filename. So the tests failed on this PR :(

Hm, ok, this is passing now but doesn't with this refactorring. I'll try to look at it :)

okies, will pull out my attempt to fix this, and it should fail on the original issue :)

Pretty please pagure-ci rebuild

Okay, removed the debugging extra 'd's, and fixed the tests from them.

Also, pulled the extra files from the added JS libries -- only the highlight css we are using, and no more readmes and all that. Added a VERSION file to diff2html, as it doesnt have it as a comment in the source itself.

Also pulled out my attempts to fixed the unicode issues, so jenkins should fail on the acutal issue, rather than my attempts to fix it :D

Pretty please pagure-ci rebuild

Pretty please pagure-ci rebuild

So I was testing this PR against current git master, and I noticed some very weird issues.

First, while it appears that #3299 is sort of fixed now, the way it indicates a rename is not intuitive:

I would expect it to render text something along the lines of "oldpath -> newpath"...

Secondly, no commits render diffs at all:

This probably explains the sole test failure, too, since it's on the function that renders commits.

So I was testing this PR against current git master, and I noticed some very weird issues.
First, while it appears that #3299 is sort of fixed now, the way it indicates a rename is not intuitive:

I would expect it to render text something along the lines of "oldpath -> newpath"...

IHMO, it really doent matter the order they are in, the strike is something we have used to signify a rename in commits for ages. The newname is first, because that is really the thing the user wants to know here IMHO, what it is called now.

Secondly, no commits render diffs at all:

This probably explains the sole test failure, too, since it's on the function that renders commits.

Are there any JS errors in the console of the webbrowser when these diffs do not render? does it happen on all commits? There may just be something in this commit that is causing issues. This JS works for me in testing, so need some more information.

So I was testing this PR against current git master, and I noticed some very weird issues.
First, while it appears that #3299 is sort of fixed now, the way it indicates a rename is not intuitive:
https://i.imgur.com/GPukVN6.png"
I would expect it to render text something along the lines of "oldpath -> newpath"...

IHMO, it really doent matter the order they are in, the strike is something we have used to signify a rename in commits for ages. The newname is first, because that is really the thing the user wants to know here IMHO, what it is called now.

OK, I guess...

Secondly, no commits render diffs at all:
https://i.imgur.com/1nlNrfJ.png" />
This probably explains the sole test failure, too, since it's on the function that renders commits.

Are there any JS errors in the console of the webbrowser when these diffs do not render? does it happen on all commits? There may just be something in this commit that is causing issues. This JS works for me in testing, so need some more information.

There are a few console errors, 404s withjquery.min.js and fedora-bootstrap stuff...

Source map error: request failed with status 404
Resource URL: http://localhost.localdomain/static/vendor/jquery/jquery.min.js
Source Map URL: jquery.min.map
Source map error: request failed with status 404 Resource URL: https://apps.fedoraproject.org/global/fedora-bootstrap-1.1.1/fedora-bootstrap.js Source Map URL: popper.js.map

However, the jQuery min js file exists on the computer:

ngompa@opensuse-lp150-skuld-vm:~> ls /usr/lib/python2.7/site-packages/pagure/static/vendor/jquery
jquery-1.10.2.js  jquery-1.10.2.min.js  jquery.js  jquery.min.js

And Fedora Bootstrap URL is valid and accessible from here...

And this issue is happening on every repo on my system on the view of any commit.

added some debugging to the ajax call to see if we can see what is happening here... the debugging will show in the console now. if you pull down the new commit i just added.

The source map issues are really nothing to worry about here.

Arg. this must be a pygit2 api change. What version of pygit2 are you running?

I'm using pygit2 0.26.0 on openSUSE Leap 15.0.

Let's rename this INFO or README and include both the version and info on where it's from and any instruction to retrieve the correct set of files again (if there is a need for this)

It turns out the pygit Patch.patch was added fairly recently ( https://github.com/libgit2/pygit2/pull/741/commits/302e40dce60291a5567602cbd053a6d5a1080a47 )

In the past, we rendered this with our own patch_to_diff filter, but this does not render a standalone, patch that diff2html recognizes.

This is the output from the patch_to_diff filter:

@@ -0,0 +1,3 @@
+ # endoculator
+ 
+ a document creation engine 
\ No newline at end of file

Compared to the output from Patch.patch , which does get recognised by diff2html:

diff --git a/README.md b/README.md
new file mode 100644
index 0000000..45e1be5
 --- /dev/null
+++ b/README.md
@@ -0,0 +1,3 @@
+# endoculator
+
+a document creation engine
\ No newline at end of file

If you want to raise the minimum requirement for libgit2 and pygit2, that's fine, but you should document this.

That change was introduced in pygit 0.26.2...

@ryanlerch look at: pagure.lib.git.commit_to_patch it may do what you want in a backward compatible way

There's a bad conflict merge here...

@pingou okies, have moved the code out of internal, into the view_commit_diff endpoint, and added support for versions of pygit that do not have Patch.patch.

The issue here is that we want the patch to be split up into individual files in the patch, which is why I was using Patch.patch. Now, i check with (hasattr() ) to see if the patch can be generated that way, and use that. Otherwise, we use commit_to_patch, and then manually split out the infivifual diffs.

The split() i use to do this there is pretty simplistic, so please check that and see how it can be improved.

@pingou also added some additional information in INFO files for the two new JS libraries added

Pretty please pagure-ci rebuild

Pretty please pagure-ci rebuild

Pretty please pagure-ci rebuild

This looks good to me, if people want to give it another try I'd welcome it, otherwise if jenkins is also happy, I'll merge :)

Pretty please pagure-ci rebuild

Jenkins is happy, so am I :)