From 833d1f1c4340e710610e4851b0ac4b5fc4981078 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:19:50 +0000 Subject: [PATCH 1/10] Add a blame endpoint This shows the traditional git blame that we all know and not necessarily love. Fixes https://pagure.io/pagure/issue/1484 --- diff --git a/pagure/templates/blame.html b/pagure/templates/blame.html new file mode 100644 index 0000000..311366a --- /dev/null +++ b/pagure/templates/blame.html @@ -0,0 +1,226 @@ +{% extends "repo_master.html" %} + +{% block title %}Tree - {{ + repo.namespace + '/' if repo.namespace }}{{ repo.name }}{% endblock %} +{% set tag = "home" %} + + +{% block repo %} +
+
+

+ Blame {{ filename }} +

+
+ +
+
+
+ {% if repo.is_fork %} + + {{ repo.user.user }}/{{ repo.name }} + {% else %} + {{ repo.name }} + {% endif %} + +
+ {% if branchname %} +
+ +
+ {% for branch in g.branches %} + {% if origin == 'view_tree' %} + + {{ branch }} + + {% elif origin == 'view_file' %} + + {{ branch }} + + {% endif %} + {% endfor %} +
+
+ {% endif %} +
+
+ +
+
+
+
    +
  1. + + +   {{ branchname }} + +
    2. + {% for file in filename.split('/') %} + {% if loop.first %} + {% set path = file %} + {% else %} + {% set path = path + '/' + file %} + {% endif %} + {% if loop.index != loop.length %}
  2. +   {{ file }} +
    3. + {% elif file %} +
  3. + +   {{ file }} +
    4. + {% endif %} + {% endfor %} +
+
+ +{% if content %} + {% if output_type in ('file','binary','image','markup', 'blame') %} +
+ {% if content %} + {% if output_type in ('file','binary','image','markup', 'blame') %} +
+ {% if output_type in ('file','markup') and g.repo_admin %} + Edit + {% endif %} + {% if output_type in ('file','markup') %} +
+ + {{ forkbuttonform.csrf_token }} +
+ {% endif %} + {% if output_type == 'markup' %} + Text + {% else %} + Blob + {% endif %} + + Raw +
+ {% endif %} + {% endif %} + + + {% autoescape false %} + {{ content | blame_loc(repo, username, blame) }} + {% endautoescape %} +
+ {% endif %} +{% else %} +No content found in this repository +{% endif %} +
+ + {% if readme %} +
+
+ README{{readme_ext}} +
+
+ {% if safe %} + {{ readme | noJS |safe }} + {% else %} + {{ readme | noJS }} + {% endif %} +
+
+ {% endif %} + +{% endblock %} + +{% block jscripts %} +{{ super() }} + +{% endblock %} diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index 092b77b..cce6543 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -240,6 +240,64 @@ def format_loc(loc, commit=None, filename=None, tree_id=None, prequest=None, return '\n'.join(output) +@APP.template_filter('blame_loc') +def blame_loc(loc, repo, username, blame): + """ Template filter putting the provided lines of code into a table + """ + if loc is None: + return + + output = [ + '
', + '' + ] + + cnt = 1 + for idx, line in enumerate(loc.split('\n')): + if line == '': + break + output.append( + '' + % ( + { + 'cnt': cnt, + 'cnt_lbl': cnt, + } + ) + ) + + cnt += 1 + if not line: + output.append(line) + continue + if line.startswith('')[1] + diff = blame.for_line(idx + 1) + output.append( + '' % author_to_user( + diff.orig_committer, with_name=False) + ) + output.append( + '' % ( + flask.url_for('view_commit', + repo=repo.name, + username=username, + namespace=repo.namespace, + commitid=diff.final_commit_id + ), + shorted_commit(diff.final_commit_id) + ) + ) + output.append('' % line) + output.append('') + + output.append('
' + '%s%s
%s
') + + return '\n'.join(output) + + @APP.template_filter('wraps') def text_wraps(text, size=10): """ Template filter to wrap text at a specified size @@ -331,7 +389,7 @@ def patch_to_diff(patch): @APP.template_filter('author2user') -def author_to_user(author, size=16, cssclass=None): +def author_to_user(author, size=16, cssclass=None, with_name=True): """ Template filter transforming a pygit2 Author object into a text either with just the username or linking to the user in pagure. """ @@ -340,11 +398,20 @@ def author_to_user(author, size=16, cssclass=None): return output user = pagure.lib.search_user(SESSION, email=author.email) if user: - output = "%s %s" % ( - avatar(user.default_email, size), - flask.url_for('view_user', username=user.username), - ('class="%s"' % cssclass) if cssclass else '', - author.name, + output = "%(avatar)s %(username)s" + if not with_name: + output = "%(avatar)s" + + output = output % ( + { + 'avatar': avatar(user.default_email, size), + 'url': flask.url_for('view_user', username=user.username), + 'cssclass': ('class="%s"' % cssclass) if cssclass else '', + 'username': user.username, + 'name': author.name, + } ) return output @@ -379,6 +446,7 @@ def author_to_user_commits(author, link, size=16, cssclass=None): return output + @APP.template_filter('InsertDiv') def insert_div(content): """ Template filter inserting an opening
and closing
diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index fe4592c..b2bbe8a 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -679,6 +679,52 @@ def view_raw_file( return (data, 200, headers) +@APP.route('//blame/') +@APP.route('///blame/') +@APP.route( + '/fork///blame/') +@APP.route( + '/fork////blame/') +def view_blame_file(repo, filename, username=None, namespace=None): + """ Displays the blame of a file or a tree for the specified repo. + """ + repo = flask.g.repo + reponame = flask.g.reponame + repo_obj = flask.g.repo_obj + + branchname = flask.request.args.get('identifier', 'master') + + if repo_obj.is_empty: + flask.abort(404, 'Empty repo cannot have a file') + + commit = repo_obj[repo_obj.head.target] + content = __get_file_in_tree( + repo_obj, commit.tree, filename.split('/'), bail_on_tree=True) + if not content: + flask.abort(404, 'File not found') + + if not isinstance(content, pygit2.Blob): + flask.abort(404, 'File not found') + if is_binary_string(content.data): + flask.abort(400, 'Binary files cannot be blamed') + + content = ktc.to_bytes(content.data) + blame = repo_obj.blame(filename) + + return flask.render_template( + 'blame.html', + select='tree', + repo=repo, + origin='view_file', + username=username, + filename=filename, + branchname=branchname, + content=content, + output_type='blame', + blame=blame, + ) + + @APP.route('//c//') @APP.route('//c/') @APP.route('///c//') From 5328035dc990b670a21b78019915ff481065d6c5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:19:50 +0000 Subject: [PATCH 2/10] Add a link to the blame view on the view_file page --- diff --git a/pagure/templates/file.html b/pagure/templates/file.html index 11eaf18..4dd570f 100644 --- a/pagure/templates/file.html +++ b/pagure/templates/file.html @@ -127,6 +127,7 @@ {{ forkbuttonform.csrf_token }} {% endif %} + {% if output_type == 'markup' %} Blob {% endif %} + + Blame + - {% if content %} - {% if output_type in ('file','binary','image','markup', 'blame') %} -
- {% if output_type in ('file','markup') and g.repo_admin %} - Edit - {% endif %} - {% if output_type in ('file','markup') %} -
- - {{ forkbuttonform.csrf_token }} -
- {% endif %} - {% if output_type == 'markup' %} - Text - {% else %} - Blob - {% endif %} - - Raw -
- {% endif %} - {% endif %} +
+ {% if output_type in ('file','markup') and g.repo_admin %} + Edit + {% endif %} + {% if output_type in ('file','markup') %} +
+ + {{ forkbuttonform.csrf_token }} +
+ {% endif %} + {% if output_type == 'markup' %} + Text + {% else %} + Blob + {% endif %} + Raw +
{% autoescape false %} {{ content | blame_loc(repo, username, blame) }} {% endautoescape %} - {% endif %} {% else %} No content found in this repository {% endif %} From 4de092f378d72e10e06e5b645c39f2250f43419e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:19:50 +0000 Subject: [PATCH 4/10] Make the git blame view a little prettier --- diff --git a/pagure/static/pagure.css b/pagure/static/pagure.css index 402da4b..4dcc17c 100644 --- a/pagure/static/pagure.css +++ b/pagure/static/pagure.css @@ -560,6 +560,10 @@ th[data-sort] { display: none; } +.code_table .cell_commit{ + padding-left:0.5em; +} + /*Our specific responsive overrides*/ @media (min-width:544px) { From 0b68dbd1985b99b2cd6d7813e31a85e40e647f8b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:19:50 +0000 Subject: [PATCH 5/10] Add unit-tests for the git blame endpoint/UI --- diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index 45cf9b3..6d905d6 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -1602,6 +1602,114 @@ class PagureFlaskRepotests(tests.Modeltests): 'text/plain; charset=ascii') self.assertTrue('foo\n bar' in output.data) + def test_view_blame_file(self): + """ Test the view_blame_file endpoint. """ + output = self.app.get('/foo/blame/sources') + # No project registered in the DB + self.assertEqual(output.status_code, 404) + + tests.create_projects(self.session) + + output = self.app.get('/test/blame/sources') + # No git repo associated + self.assertEqual(output.status_code, 404) + + tests.create_projects_git(self.path, bare=True) + + output = self.app.get('/test/blame/sources') + self.assertEqual(output.status_code, 404) + + # Add some content to the git repo + tests.add_content_git_repo(os.path.join(self.path, 'test.git')) + tests.add_readme_git_repo(os.path.join(self.path, 'test.git')) + tests.add_binary_git_repo( + os.path.join(self.path, 'test.git'), 'test.jpg') + tests.add_binary_git_repo( + os.path.join(self.path, 'test.git'), 'test_binary') + + output = self.app.get('/test/blame/foofile') + self.assertEqual(output.status_code, 404) + + # View in a branch + output = self.app.get('/test/blame/sources') + self.assertEqual(output.status_code, 200) + self.assertIn(b'', output.data) + self.assertIn( + b'', output.data) + self.assertIn( + b'', output.data) + + # View what's supposed to be an image + output = self.app.get('/test/blame/test.jpg') + self.assertEqual(output.status_code, 400) + self.assertIn( + b'400 Bad Request', output.data) + self.assertIn( + b'

Binary files cannot be blamed

', output.data) + + # View folder + output = self.app.get('/test/blame/folder1') + self.assertEqual(output.status_code, 404) + self.assertIn("Page not found :'( - Pagure", output.data) + self.assertIn( + '

Page not found (404)

', output.data) + + # View by image name -- with a non-existant file + output = self.app.get('/test/blame/testfoo.jpg') + self.assertEqual(output.status_code, 404) + output = self.app.get('/test/blame/folder1/testfoo.jpg') + self.assertEqual(output.status_code, 404) + + # View file with a non-ascii name + tests.add_commit_git_repo( + os.path.join(self.path, 'test.git'), + ncommits=1, filename='Šource') + output = self.app.get('/test/blame/Šource') + self.assertEqual(output.status_code, 200) + self.assertEqual(output.headers['Content-Type'].lower(), + 'text/html; charset=utf-8') + self.assertIn('  Šource', output.data) + self.assertIn('
 bar
', output.data) + self.assertIn( + '', output.data) + self.assertTrue( + '' + in output.data + or + '' in output.data + ) + + # Add a fork of a fork + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + description='test project #3', + is_fork=True, + parent_id=1, + hook_token='aaabbbppp', + ) + self.session.add(item) + self.session.commit() + + tests.add_content_git_repo( + os.path.join(self.path, 'forks', 'pingou', 'test3.git')) + tests.add_readme_git_repo( + os.path.join(self.path, 'forks', 'pingou', 'test3.git')) + tests.add_commit_git_repo( + os.path.join(self.path, 'forks', 'pingou', 'test3.git'), + ncommits=10) + + output = self.app.get('/fork/pingou/test3/blame/sources') + self.assertEqual(output.status_code, 200) + self.assertIn('
Row 0
Row 0
', output.data) + self.assertIn( + '', output.data) + self.assertIn( + '', output.data) + def test_view_commit(self): """ Test the view_commit endpoint. """ output = self.app.get('/foo/c/bar') From 17526c72e3077fb3cefbb88886637310e5a54a5e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:29:18 +0000 Subject: [PATCH 6/10] Better document what the blame_loc method is and does --- diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index cce6543..6276090 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -243,6 +243,21 @@ def format_loc(loc, commit=None, filename=None, tree_id=None, prequest=None, @APP.template_filter('blame_loc') def blame_loc(loc, repo, username, blame): """ Template filter putting the provided lines of code into a table + + + This method blame lines of code (loc) takes as input a text (lines of + code) concerning a given repo, with its repo and a pygit2.Blame object + and convert it into a html table displayed to the user with the git + blame information (user, commit, commit date). + + :arg loc: a text object of the lines of code to display (in this case, + most likely the content of a file). + :arg repo: the name of the repo in which this file is. + :arg username: the user name of the user whose repo this is, if the repo + is not a *fork*, this value is ``None``. + :arg blame: a pygit2.Blame object allowing us to link a given line of + code to a commit. + """ if loc is None: return From f708f094dc5bf99c189621029407324a42b655ad Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:29:36 +0000 Subject: [PATCH 7/10] Simplify the code in blame_loc, no need to use two variables for the same content --- diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index 6276090..0724cb4 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -274,13 +274,8 @@ def blame_loc(loc, repo, username, blame): output.append( '' - % ( - { - 'cnt': cnt, - 'cnt_lbl': cnt, - } - ) + '"%(cnt)s">' + % ({'cnt': cnt}) ) cnt += 1 From 0959314354678b83ba5b9c391d6278a8c5bcfbb8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:29:53 +0000 Subject: [PATCH 8/10] Empty lines can also be blamed, so do not exclude them --- diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index 0724cb4..0193f68 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -279,9 +279,6 @@ def blame_loc(loc, repo, username, blame): ) cnt += 1 - if not line: - output.append(line) - continue if line.startswith('')[1] diff = blame.for_line(idx + 1) From e635c9e143cd12d05f8788f0e367cf48764ea5b1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 17 2016 16:30:44 +0000 Subject: [PATCH 9/10] Fix the title of the blame button --- diff --git a/pagure/templates/file.html b/pagure/templates/file.html index 4dd570f..b05a074 100644 --- a/pagure/templates/file.html +++ b/pagure/templates/file.html @@ -149,7 +149,7 @@ repo=repo.name, username=username, namespace=repo.namespace, - filename=filename) | unicode }}" title="View as raw">Blame + filename=filename) | unicode }}" title="View git blame">Blame ' ] - cnt = 1 for idx, line in enumerate(loc.split('\n')): if line == '': break + + try: + diff = blame.for_line(idx + 1) + except IndexError: + # Happens at the end of the file, since we are using idx + 1 + continue + + if line.startswith('')[1] + output.append( '' - % ({'cnt': cnt}) + % ({'cnt': idx}) ) - cnt += 1 - if line.startswith('')[1] - diff = blame.for_line(idx + 1) output.append( '' % author_to_user( diff.orig_committer, with_name=False) ) + output.append( '' % ( flask.url_for('view_commit', 
 barRow 0
' '
' '%s%s