launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20368
[Merge] lp:~cjwatson/launchpad/git-mp-commits-diff into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-mp-commits-diff into lp:launchpad with lp:~cjwatson/launchpad/git-mp-commits as a prerequisite.
Commit message:
Implement AJAX revision diffs for Git.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mp-commits-diff/+merge/294704
Implement AJAX revision diffs for Git.
I had to shuffle the code around that subtracts one from the start revno in the Bazaar case, since subtracting one from a SHA-1 commit ID isn't going to work so well. For a similar reason, this depends on https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/294700 being deployed first so that we don't need to do extra work to figure out the previous commit ID. Otherwise it's fairly straightforward: we just need to proxy the diff through the webapp.
I initially expected this to be difficult because diffs could be non-Unicode and might require extra work to transport through JSON. But as it happens, and somewhat to my surprise, pygit2 always encodes diffs to UTF-8 with errors="replace" (see src/diff.c:Diff_patch__get__ and src/utils.h:to_unicode_n). This is possibly a slightly odd design decision on their part, but it's certainly convenient for us, and we were going to have to do something similar in order to display diffs in the webapp anyway.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mp-commits-diff into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2016-05-14 11:04:40 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2016-05-14 11:04:41 +0000
@@ -585,10 +585,11 @@
self.context.source_branch.unique_name),
})
else:
- # XXX cjwatson 2015-04-29: Unimplemented for Git; this would
- # require something in the webapp which proxies diff requests to
- # the code browser, or an integrated code browser.
- pass
+ cache.objects.update({
+ 'branch_diff_link':
+ canonical_url(self.context.source_git_repository) +
+ '/+diff/',
+ })
if getFeatureFlag("longpoll.merge_proposals.enabled"):
cache.objects['merge_proposal_event_key'] = subscribe(
self.context).event_key
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2016-05-07 00:40:18 +0000
+++ lib/lp/code/browser/configure.zcml 2016-05-14 11:04:41 +0000
@@ -838,6 +838,10 @@
permission="launchpad.Edit"
name="+edit"
template="../templates/gitrepository-edit.pt"/>
+ <class class="lp.code.browser.gitrepository.GitRepositoryDiffView">
+ <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher"
+ attributes="__call__"/>
+ </class>
<browser:page
for="lp.code.interfaces.gitrepository.IGitRepository"
class="lp.code.browser.gitrepository.GitRepositoryDeletionView"
=== modified file 'lib/lp/code/browser/gitrepository.py'
--- lib/lp/code/browser/gitrepository.py 2016-01-25 12:21:31 +0000
+++ lib/lp/code/browser/gitrepository.py 2016-05-14 11:04:41 +0000
@@ -34,6 +34,7 @@
Interface,
providedBy,
)
+from zope.publisher.interfaces.browser import IBrowserPublisher
from zope.schema import Choice
from zope.schema.vocabulary import (
SimpleTerm,
@@ -173,6 +174,22 @@
return None
return self.context.getMergeProposalByID(id)
+ @stepto("+diff")
+ def traverse_diff(self):
+ segments = list(self.request.getTraversalStack())
+ if len(segments) == 1:
+ new = segments.pop()
+ old = new + "^"
+ self.request.stepstogo.consume()
+ elif len(segments) == 2:
+ new = segments.pop()
+ old = segments.pop()
+ self.request.stepstogo.consume()
+ self.request.stepstogo.consume()
+ else:
+ return None
+ return GitRepositoryDiffView(self.context, self.request, old, new)
+
class GitRepositoryEditMenu(NavigationMenu):
"""Edit menu for `IGitRepository`."""
@@ -606,6 +623,29 @@
"'%s'." % default_branch)
+@implementer(IBrowserPublisher)
+class GitRepositoryDiffView:
+
+ def __init__(self, context, request, old, new):
+ self.context = context
+ self.request = request
+ self.old = old
+ self.new = new
+
+ def __call__(self):
+ content = self.context.getDiff(self.old, self.new)
+ filename = "%s_%s.diff" % (self.old, self.new)
+ self.request.response.setHeader(
+ "Content-Type", 'text/x-patch; charset="UTF-8"')
+ self.request.response.setHeader("Content-Length", str(len(content)))
+ self.request.response.setHeader(
+ "Content-Disposition", "attachment; filename=%s" % filename)
+ return content
+
+ def browserDefault(self, request):
+ return self, ()
+
+
class GitRepositoryDeletionView(LaunchpadFormView):
schema = IGitRepository
=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py 2015-10-19 10:56:16 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py 2016-05-14 11:04:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for GitRepositoryView."""
@@ -756,6 +756,32 @@
self.assertShownTypes([InformationType.PROPRIETARY], repository)
+class TestGitRepositoryDiffView(BrowserTestCase):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_render(self):
+ hosting_client = FakeMethod()
+ diff = u"A fake diff\n"
+ hosting_client.getDiff = FakeMethod(result={"patch": diff})
+ self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+ person = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=person)
+ browser = self.getUserBrowser(
+ canonical_url(repository) + "/+diff/0123456/0123456^")
+ with person_logged_in(person):
+ self.assertEqual(
+ [((repository.getInternalPath(), "0123456^", "0123456"), {})],
+ hosting_client.getDiff.calls)
+ self.assertEqual(
+ 'text/x-patch;charset=UTF-8', browser.headers["Content-Type"])
+ self.assertEqual(str(len(diff)), browser.headers["Content-Length"])
+ self.assertEqual(
+ "attachment; filename=0123456^_0123456.diff",
+ browser.headers["Content-Disposition"])
+ self.assertEqual("A fake diff\n", browser.contents)
+
+
class TestGitRepositoryDeletionView(BrowserTestCase):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/code/interfaces/githosting.py'
--- lib/lp/code/interfaces/githosting.py 2016-05-14 11:04:40 +0000
+++ lib/lp/code/interfaces/githosting.py 2016-05-14 11:04:41 +0000
@@ -67,6 +67,24 @@
start commit's history.
"""
+ def getDiff(path, old, new, common_ancestor=False, context_lines=None,
+ logger=None):
+ """Get the diff between two commits.
+
+ :param path: Physical path of the repository on the hosting service.
+ :param old: The OID of the old commit.
+ :param new: The OID of the new commit.
+ :param common_ancestor: If True, return the symmetric or common
+ ancestor diff, equivalent to
+ `git diff $(git-merge-base OLD NEW) NEW`.
+ :param context_lines: Include this number of lines of context around
+ each hunk.
+ :param logger: An optional logger.
+ :return: A dict mapping 'commits' to a list of commits between 'old'
+ and 'new' (formatted as with `getCommits`) and 'patch' to the
+ text of the diff between 'old' and 'new'.
+ """
+
def getMergeDiff(path, base, head, logger=None):
"""Get the merge preview diff between two commits.
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2016-05-03 15:12:46 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2016-05-14 11:04:41 +0000
@@ -541,6 +541,14 @@
:return: A binary string with the blob content.
"""
+ def getDiff(old, new):
+ """Get the diff between two commits in this repository.
+
+ :param old: The OID of the old commit.
+ :param new: The OID of the new commit.
+ :return: The diff as a binary string.
+ """
+
class IGitRepositoryModerateAttributes(Interface):
"""IGitRepository attributes that can be edited by more than one community.
=== modified file 'lib/lp/code/javascript/branch.revisionexpander.js'
--- lib/lp/code/javascript/branch.revisionexpander.js 2011-07-19 06:37:54 +0000
+++ lib/lp/code/javascript/branch.revisionexpander.js 2016-05-14 11:04:41 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2011 Canonical Ltd. This software is licensed under the
+/* Copyright 2011-2016 Canonical Ltd. This software is licensed under the
* GNU Affero General Public License version 3 (see the file LICENSE).
*
* Expandable branch revisions.
@@ -89,7 +89,7 @@
}
var rev_no_range = expander.icon_node.get(
'id').replace('expandable-', '').split('-');
- var start_revno = rev_no_range[0]-1;
+ var start_revno = rev_no_range[0];
var end_revno = rev_no_range[1];
lp_client.get(bmp_get_diff_url(start_revno, end_revno),
=== modified file 'lib/lp/code/javascript/tests/test_branchrevisionexpander.js'
--- lib/lp/code/javascript/tests/test_branchrevisionexpander.js 2013-03-20 03:41:40 +0000
+++ lib/lp/code/javascript/tests/test_branchrevisionexpander.js 2016-05-14 11:04:41 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2010-2012 Canonical Ltd. This software is licensed under the
+/* Copyright 2010-2016 Canonical Ltd. This software is licensed under the
* GNU Affero General Public License version 3 (see the file LICENSE). */
YUI.add('lp.code.branch.revisionexpander.test', function (Y) {
@@ -73,7 +73,7 @@
var FakeExpander = function() {};
FakeExpander.prototype = {
'icon_node':
- Y.Node.create('<div id="expandable-23-45"></div>'),
+ Y.Node.create('<div id="expandable-22-45"></div>'),
'receive': function (node) {
this.received_node = node;
}
@@ -117,13 +117,33 @@
},
/*
- * bmp_get_diff_url(0, 2) just returns URL_BASE/1, rather than
- * URL_BASE/1/0 which Loggerhead will reject.
+ * bmp_get_diff_url(0, 2) returns URL_BASE/2/null:, rather than
+ * URL_BASE/2/0 which Loggerhead will reject.
*/
test_bmp_get_diff_url_of_0_to_2: function() {
Y.Assert.areEqual(
'fake-link-base/2/null:',
module.bmp_get_diff_url(0, 2));
+ },
+
+ /*
+ * bmp_get_diff_url(revision) gets the URL for a diff of just that
+ * Git revision.
+ */
+ test_bmp_get_diff_url_git_one_arg: function() {
+ Y.Assert.areEqual(
+ 'fake-link-base/0123456',
+ module.bmp_get_diff_url('0123456'));
+ },
+
+ /*
+ * bmp_get_diff_url(start_revision, end_revision) gets the URL for a
+ * diff of the given Git revision range.
+ */
+ test_bmp_get_diff_url_git_two_args: function() {
+ Y.Assert.areEqual(
+ 'fake-link-base/789abcd/0123456',
+ module.bmp_get_diff_url('0123456', '789abcd'));
}
}));
=== modified file 'lib/lp/code/model/githosting.py'
--- lib/lp/code/model/githosting.py 2016-05-14 11:04:40 +0000
+++ lib/lp/code/model/githosting.py 2016-05-14 11:04:41 +0000
@@ -124,6 +124,21 @@
"Failed to get commit log from Git repository: %s" %
unicode(e))
+ def getDiff(self, path, old, new, common_ancestor=False,
+ context_lines=None, logger=None):
+ """See `IGitHostingClient`."""
+ try:
+ if logger is not None:
+ logger.info(
+ "Requesting diff for %s from %s to %s" % (path, old, new))
+ separator = "..." if common_ancestor else ".."
+ url = "/repo/%s/compare/%s%s%s" % (
+ path, quote(old), separator, quote(new))
+ return self._get(url, params={"context_lines": context_lines})
+ except Exception as e:
+ raise GitRepositoryScanFault(
+ "Failed to get diff from Git repository: %s" % unicode(e))
+
def getMergeDiff(self, path, base, head, prerequisite=None, logger=None):
"""See `IGitHostingClient`."""
try:
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2016-05-14 11:04:40 +0000
+++ lib/lp/code/model/gitrepository.py 2016-05-14 11:04:41 +0000
@@ -1036,6 +1036,12 @@
hosting_client = getUtility(IGitHostingClient)
return hosting_client.getBlob(self.getInternalPath(), filename, rev)
+ def getDiff(self, old, new):
+ """See `IGitRepository`."""
+ hosting_client = getUtility(IGitHostingClient)
+ diff = hosting_client.getDiff(self.getInternalPath(), old, new)
+ return diff["patch"]
+
def canBeDeleted(self):
"""See `IGitRepository`."""
# Can't delete if the repository is associated with anything.
=== modified file 'lib/lp/code/model/tests/test_githosting.py'
--- lib/lp/code/model/tests/test_githosting.py 2016-05-14 11:04:40 +0000
+++ lib/lp/code/model/tests/test_githosting.py 2016-05-14 11:04:41 +0000
@@ -180,6 +180,32 @@
"Failed to get commit log from Git repository: Bad request",
self.client.getLog, "123", "refs/heads/master")
+ def test_getDiff(self):
+ with self.mockRequests(content=b'{"patch": ""}'):
+ diff = self.client.getDiff("123", "a", "b")
+ self.assertEqual({"patch": ""}, diff)
+ self.assertRequest("repo/123/compare/a..b", method="GET")
+
+ def test_getDiff_common_ancestor(self):
+ with self.mockRequests(content=b'{"patch": ""}'):
+ diff = self.client.getDiff("123", "a", "b", common_ancestor=True)
+ self.assertEqual({"patch": ""}, diff)
+ self.assertRequest("repo/123/compare/a...b", method="GET")
+
+ def test_getDiff_context_lines(self):
+ with self.mockRequests(content=b'{"patch": ""}'):
+ diff = self.client.getDiff("123", "a", "b", context_lines=4)
+ self.assertEqual({"patch": ""}, diff)
+ self.assertRequest(
+ "repo/123/compare/a..b?context_lines=4", method="GET")
+
+ def test_getDiff_failure(self):
+ with self.mockRequests(status_code=400, content=b"Bad request"):
+ self.assertRaisesWithContent(
+ GitRepositoryScanFault,
+ "Failed to get diff from Git repository: Bad request",
+ self.client.getDiff, "123", "a", "b")
+
def test_getMergeDiff(self):
with self.mockRequests(content=b'{"patch": ""}'):
diff = self.client.getMergeDiff("123", "a", "b")
=== modified file 'lib/lp/code/templates/branch-macros.pt'
--- lib/lp/code/templates/branch-macros.pt 2016-05-07 00:40:18 +0000
+++ lib/lp/code/templates/branch-macros.pt 2016-05-14 11:04:41 +0000
@@ -204,8 +204,9 @@
<div class="revision-group-diff" tal:condition="revisions | nothing">
<a href="#" class="hidden expander-icon js-action"
tal:define="start_revno python:revisions[0].sequence;
- last_revno python:revisions[-1].sequence"
- tal:attributes="id string:expandable-${start_revno}-${last_revno}"
+ prev_revno python:revisions[0].sequence - 1;
+ last_revno python:revisions[-1].sequence"
+ tal:attributes="id string:expandable-${prev_revno}-${last_revno}"
>Changes added by revision <tal:revno replace="start_revno">1</tal:revno>
<tal:plural condition="python: start_revno!=last_revno">
to revision <tal:revno replace="last_revno">2</tal:revno>
=== modified file 'lib/lp/code/templates/codereviewnewrevisions-footer.pt'
--- lib/lp/code/templates/codereviewnewrevisions-footer.pt 2016-05-14 11:04:40 +0000
+++ lib/lp/code/templates/codereviewnewrevisions-footer.pt 2016-05-14 11:04:41 +0000
@@ -12,7 +12,8 @@
</tal:bzr-revisions>
<tal:git-revisions condition="context/git_ref">
<tal:revisions define="ref context/git_ref;
- commit_infos context/revisions;">
+ commit_infos context/revisions;
+ show_diff_expander python:True;">
<metal:commits use-macro="ref/@@+macros/ref-commits"/>
</tal:revisions>
</tal:git-revisions>
=== modified file 'lib/lp/code/templates/git-macros.pt'
--- lib/lp/code/templates/git-macros.pt 2016-05-14 11:04:40 +0000
+++ lib/lp/code/templates/git-macros.pt 2016-05-14 11:04:41 +0000
@@ -115,6 +115,24 @@
<tal:commit repeat="commit_info commit_infos">
<metal:commit-text use-macro="ref/@@+macros/commit-text"/>
</tal:commit>
+ <tal:ajax-revision-diffs
+ condition="request/features/code.ajax_revision_diffs.enabled">
+ <tal:diff-expander condition="show_diff_expander | nothing">
+ <div class="revision-group-diff">
+ <a href="#" class="hidden expander-icon js-action"
+ tal:define="start_revision python:commit_infos[0]['sha1'];
+ prev_revision python:commit_infos[0]['sha1'] + '^';
+ last_revision python:commit_infos[-1]['sha1']"
+ tal:attributes="id string:expandable-${prev_revision}-${last_revision}"
+ >Changes added by commit
+ <tal:sha1 replace="start_revision/fmt:shorten/10"/>
+ <tal:plural condition="python: start_revision!=last_revision">
+ to commit <tal:sha1 replace="last_revision/fmt:shorten/10"/>
+ </tal:plural></a>
+ <div class="hidden expander-content">Loading diff <img src="/@@/spinner"/></div>
+ </div>
+ </tal:diff-expander>
+ </tal:ajax-revision-diffs>
</dl>
</metal:ref-commits>
Follow ups
