From 48bfcb44ee8cfdd1989faad8b39018d4059be483 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Dec 16 2016 11:12:57 +0000 Subject: Issue 1382 - Improve patch viewing in Issues There are two issues here: [1] The current diff produced by html_diff() is lacking some important details. Tabs and trailing white spaces are not correctly highlighted. This commit creates a new pygment Style that is based off the current style "tango" and modifies it to use background color for changed lines for the html formatter which allows for the highlighting of trailing spaces. Then a filter is added to the diff lexer that shows tabs, and allows trailing white spaces to be identified by the html formatter. [2] When opening an attachment in an "issue" check the file extension to see if its a patch file. If it is then render the file in html to get our nice "diff" output. This makes reviewing patches that are attached to Issues much easier to evaluate. Updated tests to account for the new html rendering of patches. Also fixed pep8 errors in filters.py & issues.py https://pagure.io/pagure/issue/1382 --- diff --git a/pagure/templates/patchfile.html b/pagure/templates/patchfile.html new file mode 100644 index 0000000..44dea01 --- /dev/null +++ b/pagure/templates/patchfile.html @@ -0,0 +1,18 @@ +{% extends "repo_master.html" %} + +{% block title %}Patch File - {{ + repo.namespace + '/' if repo.namespace }}{{ repo.name }} {% endblock %} + +{% block repo %} +
+

{{ patchfile }}

+
+ {% autoescape false %} + {{ diff|html_diff}} + {% endautoescape %} +
+{% endblock %} + +{% block jscripts %} + {{ super() }} +{% endblock %} diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index 1cb41b0..10e6760 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -25,6 +25,7 @@ import six from pygments import highlight from pygments.lexers.text import DiffLexer from pygments.formatters import HtmlFormatter +from pygments.filters import VisibleWhitespaceFilter import pagure.exceptions import pagure.lib @@ -153,8 +154,8 @@ def format_loc(loc, commit=None, filename=None, tree_id=None, prequest=None, tpl_delete = '' if cnt - 1 in comments: @@ -166,10 +167,10 @@ def format_loc(loc, commit=None, filename=None, tree_id=None, prequest=None, status = str(comment.parent.status).lower() if authenticated() and ( ( - status in ['true', 'open'] - and comment.user.user == flask.g.fas_user.username - ) - or is_repo_admin(comment.parent.project)): + status in ['true', 'open'] and + comment.user.user == flask.g.fas_user.username + ) or + is_repo_admin(comment.parent.project)): templ_delete = tpl_delete % ({'commentid': comment.id}) templ_edit = tpl_edit % ({ 'edit_url': flask.url_for( @@ -377,12 +378,19 @@ def html_diff(diff): """Display diff as HTML""" if diff is None: return + difflexer = DiffLexer() + # Do not give whitespaces the special Whitespace token type as this + # prevents the html formatter from picking up on trailing whitespaces in + # the diff. + difflexer.add_filter(VisibleWhitespaceFilter(wstokentype=False, tabs=True)) + return highlight( diff, - DiffLexer(), + difflexer, HtmlFormatter( + linenos='inline', noclasses=True, - style="tango",) + style="diffstyle") ) diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 3e0f94d..657a102 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -13,7 +13,6 @@ # pylint: disable=too-many-locals # pylint: disable=too-many-statements - import flask import os from collections import defaultdict @@ -22,6 +21,7 @@ from math import ceil import pygit2 import werkzeug.datastructures from sqlalchemy.exc import SQLAlchemyError +from binaryornot.helpers import is_binary_string import kitchen.text.converters as ktc import mimetypes @@ -82,9 +82,8 @@ urlregex = re.compile( ) urlpattern = re.compile(urlregex) -# URLs - +# URLs @APP.route( '//issue//update/', methods=['GET', 'POST']) @@ -163,9 +162,9 @@ def update_issue(repo, issueid, username=None, namespace=None): if comment is None or comment.issue.project != repo: flask.abort(404, 'Comment not found') - if (flask.g.fas_user.username != comment.user.username - or comment.parent.status != 'Open') \ - and not flask.g.repo_admin: + if (flask.g.fas_user.username != comment.user.username or + comment.parent.status != 'Open') and not \ + flask.g.repo_admin: flask.abort( 403, 'You are not allowed to remove this comment from ' @@ -975,8 +974,8 @@ def edit_issue(repo, issueid, username=None, namespace=None): if issue is None or issue.project != repo: flask.abort(404, 'Issue not found') - if not (flask.g.repo_admin - or flask.g.fas_user.username == issue.user.username): + if not (flask.g.repo_admin or + flask.g.fas_user.username == issue.user.username): flask.abort( 403, 'You are not allowed to edit issues for this project') @@ -1156,6 +1155,20 @@ def view_issue_raw_file( if not data: flask.abort(404, 'No content found') + if (filename.endswith('.patch') or filename.endswith('.diff')) \ + and not is_binary_string(content.data): + # We have a patch file attached to this issue, render the diff in html + orig_filename = filename.split('-', 1)[1] + return flask.render_template( + 'patchfile.html', + select='issues', + repo=repo, + username=username, + diff=data, + patchfile=orig_filename, + form=pagure.forms.ConfirmationForm(), + ) + if not mimetype and data[:2] == '#!': mimetype = 'text/plain' @@ -1186,8 +1199,8 @@ def view_issue_raw_file( @APP.route('//issue//comment//edit', methods=('GET', 'POST')) -@APP.route('///issue//comment//edit', - methods=('GET', 'POST')) +@APP.route('///issue//comment//' + + 'edit', methods=('GET', 'POST')) @APP.route('/fork///issue//comment' '//edit', methods=('GET', 'POST')) @APP.route('/fork////issue//comment' @@ -1215,8 +1228,8 @@ def edit_comment_issue( if comment is None or comment.parent.project != project: flask.abort(404, 'Comment not found') - if (flask.g.fas_user.username != comment.user.username - or comment.parent.status != 'Open') \ + if (flask.g.fas_user.username != comment.user.username or + comment.parent.status != 'Open') \ and not flask.g.repo_admin: flask.abort(403, 'You are not allowed to edit this comment') diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index ba41e46..60d354c 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -33,6 +33,7 @@ from pygments.formatters import HtmlFormatter from pygments.lexers import guess_lexer_for_filename from pygments.lexers.special import TextLexer from pygments.util import ClassNotFound +from pygments.filters import VisibleWhitespaceFilter from sqlalchemy.exc import SQLAlchemyError import mimetypes @@ -548,12 +549,18 @@ def view_file(repo, identifier, filename, username=None, namespace=None): except (ClassNotFound, TypeError): lexer = TextLexer() + style = "tango" + + if ext in ('.diff', '.patch'): + lexer.add_filter(VisibleWhitespaceFilter( + wstokentype=False, tabs=True)) + style = "diffstyle" content = highlight( file_content, lexer, HtmlFormatter( noclasses=True, - style="tango",) + style=style,) ) output_type = 'file' else: diff --git a/setup.py b/setup.py index 215718a..833581e 100644 --- a/setup.py +++ b/setup.py @@ -58,6 +58,8 @@ setup( entry_points=""" [moksha.consumer] integrator = pagureCI.consumer:Integrator + [pygments.styles] + diffstyle = pagure.ui.diff_style:DiffStyle """, classifiers=[ 'License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)', diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index ede1eae..2d570c3 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -1209,11 +1209,8 @@ class PagureFlaskRepotests(tests.Modeltests): '\n 2\n ', output.data) self.assertIn( - '' + - '@@ -1,2 +1,1 @@', - output.data) - self.assertIn( - '- Row 0', output.data) + '- ' + + 'Row 0', output.data) # View inverse commits comparison output = self.app.get('/test/c/%s..%s' % (c1.oid.hex, c2.oid.hex)) self.assertEqual(output.status_code, 200) @@ -1232,12 +1229,8 @@ class PagureFlaskRepotests(tests.Modeltests): (c1.oid.hex, c2.oid.hex), output.data) self.assertIn( - '' + - '@@ -1,1 +1,2 @@', - output.data) - self.assertIn( - '+ Row 0', - output.data) + '' + + '+ Row 0', output.data) def compare_all(c1, c3): # View commits comparison @@ -1251,14 +1244,12 @@ class PagureFlaskRepotests(tests.Modeltests): (c1.oid.hex, c3.oid.hex), output.data) self.assertIn( - '' + - '@@ -1,1 +1,2 @@', - output.data) - self.assertIn( - '+ Row 0', output.data) + '+ Row 0', output.data) self.assertEqual( output.data.count( - '+ Row 0'), 2) + '+ Row 0'), 2) self.assertIn( 'Commits \n ' + '' + - '@@ -1,2 +1,1 @@', - output.data) + '@@ -1,2 +1,1' + + ' @@', output.data) self.assertIn( - '- Row 0', - output.data) - self.assertEqual( - output.data.count( - '- Row 0'), 1) + '- ' + + 'Row 0', output.data) self.assertIn( 'Commits \n ' + '+ Pagure' in output.data) - self.assertTrue( - '+ ======' in output.data) # View first commit - with the old URL scheme disabled - default output = self.app.get( @@ -1768,18 +1751,8 @@ class PagureFlaskRepotests(tests.Modeltests): self.assertTrue(' Authored by Alice Author\n' in output.data) self.assertTrue(' Committed by Cecil Committer\n' in output.data) self.assertTrue( - # new version of pygments - '
' - '
'
-            ''
-            ''
-            '@@ -0,0 +1,3 @@' in output.data
-            or
-            # old version of pygments
-            '
'
-            '
'
-            ''
-            '@@ -0,0 +1,3 @@' in output.data)
+            '+ Pagure' in output.data)
+            ' 2' +
+            ' ' +
+            '+ Pagure' in output.data)
         self.assertTrue(
-            '+ ======' in output.data)
+            ' 3' +
+            ' ' +
+            '+ ======' in output.data)
 
         # Try the old URL scheme with a short hash
         output = self.app.get(
diff --git a/tests/test_zzz_pagure_flask_ui_old_commit.py b/tests/test_zzz_pagure_flask_ui_old_commit.py
index 660058d..ec2f4b6 100644
--- a/tests/test_zzz_pagure_flask_ui_old_commit.py
+++ b/tests/test_zzz_pagure_flask_ui_old_commit.py
@@ -96,10 +96,15 @@ class PagureFlaskRepoOldUrltests(tests.Modeltests):
             in output.data)
         self.assertTrue('  Authored by Alice Author\n' in output.data)
         self.assertTrue('  Committed by Cecil Committer\n' in output.data)
+
         self.assertTrue(
-            '+ Pagure' in output.data)
+            '' +
+            ' 2 + Pagure' in output.data)
         self.assertTrue(
-            '+ ======' in output.data)
+            '' +
+            ' 3 + ======' in output.data)
 
         self.app = pagure.APP.test_client()
         # View first commit - with the old URL scheme
@@ -111,10 +116,15 @@ class PagureFlaskRepoOldUrltests(tests.Modeltests):
             in output.data)
         self.assertTrue('  Authored by Alice Author\n' in output.data)
         self.assertTrue('  Committed by Cecil Committer\n' in output.data)
+
         self.assertTrue(
-            '+ Pagure' in output.data)
+            ' 2' +
+            ' ' +
+            '+ Pagure' in output.data)
         self.assertTrue(
-            '+ ======' in output.data)
+            '' +
+            ' 3 + ======' in output.data)
 
         # Add some content to the git repo
         tests.add_content_git_repo(os.path.join(self.path, 'test.git'))
@@ -133,16 +143,16 @@ class PagureFlaskRepoOldUrltests(tests.Modeltests):
         self.assertTrue('  Committed by Cecil Committer\n' in output.data)
         self.assertTrue(
             # new version of pygments
-            '
'
-            '
'
-            ''
-            ''
-            '@@ -0,0 +1,3 @@' in output.data
+            '
1 @@ -0,0 +1,3 @@' in
+            output.data
             or
             # old version of pygments
-            '
'
-            '
'
-            ''
+            '
' +
+            '
' +
+            '' +
             '@@ -0,0 +1,3 @@' in output.data)
 
         # Add a fork of a fork
@@ -180,9 +190,13 @@ class PagureFlaskRepoOldUrltests(tests.Modeltests):
         self.assertTrue('  Authored by Alice Author\n' in output.data)
         self.assertTrue('  Committed by Cecil Committer\n' in output.data)
         self.assertTrue(
-            '+ Pagure' in output.data)
+            ' 2' +
+            ' ' +
+            '+ Pagure' in output.data)
         self.assertTrue(
-            '+ ======' in output.data)
+            ' 3' +
+            ' ' +
+            '+ ======' in output.data)
 
         # View commit of fork - With the old URL scheme
         output = self.app.get(
@@ -194,9 +208,13 @@ class PagureFlaskRepoOldUrltests(tests.Modeltests):
         self.assertTrue('  Authored by Alice Author\n' in output.data)
         self.assertTrue('  Committed by Cecil Committer\n' in output.data)
         self.assertTrue(
-            '+ Pagure' in output.data)
+            ' 2' +
+            ' ' +
+            '+ Pagure' in output.data)
         self.assertTrue(
-            '+ ======' in output.data)
+            ' 3' +
+            ' ' +
+            '+ ======' in output.data)
 
         # Try the old URL scheme with a short hash
         output = self.app.get(