kallithea Changeset - 84eb5b7b1bac

Changeset - 84eb5b7b1bac

Parent rev.

Child rev.

[Not reviewed]

default

0 5 0

Mads Kiilerich - 11 years ago 2015-02-11 03:05:00
madski@unity3d.com

pullrequests: prevent creation of invalid pull requests, empty or unrelated or criss cross

5 files changed with 86 insertions and 55 deletions:

kallithea/controllers/compare.py

kallithea/controllers/pullrequests.py

kallithea/templates/compare/compare_cs.html

kallithea/tests/functional/test_compare.py

kallithea/tests/functional/test_compare_local.py

0 comments (0 inline, 0 general)

kallithea/controllers/compare.py

➞

Show inline comments

@@ @@ -32,7 +32,7 @@ import re @@
 from pylons import request, tmpl_context as c
 from pylons.i18n.translation import _
 from webob.exc import HTTPFound, HTTPBadRequest
+from webob.exc import HTTPFound, HTTPBadRequest, HTTPNotFound
 from kallithea.config.routing import url
 from kallithea.lib.utils2 import safe_str, safe_int
@@ @@ -81,7 +81,7 @@ class CompareController(BaseRepoControll @@
         Returns lists of changesets that can be merged from org_repo@org_rev
         to other_repo@other_rev
         ... and the other way
-        ... and the ancestor that would be used for merge
+        ... and the ancestors that would be used for merge
         :param org_repo: repo object, that is most likely the original repo we forked from
         :param org_rev: the revision we want our compare to be made
@@ @@ -89,11 +89,10 @@ class CompareController(BaseRepoControll @@
             all changesets that we need to obtain
         :param other_rev: revision we want out compare to be made on other_repo
         """
-        ancestor = None
+        ancestors = None
         if org_rev == other_rev:
             org_changesets = []
             other_changesets = []
             ancestor = org_rev
         elif alias == 'hg':
             #case two independent repos
@@ @@ -108,25 +107,20 @@ class CompareController(BaseRepoControll @@
             else:
                 hgrepo = other_repo._repo
             if org_repo.EMPTY_CHANGESET in (org_rev, other_rev):
                 # work around unexpected behaviour in Mercurial < 3.4
                 ancestor = org_repo.EMPTY_CHANGESET
             ancestors = [hgrepo[ancestor].hex() for ancestor in
                          hgrepo.revs("id(%s) & ::id(%s)", other_rev, org_rev)]
             if ancestors:
                 log.debug("shortcut found: %s is already an ancestor of %s", other_rev, org_rev)
             else:
                 ancestors = hgrepo.revs("ancestor(id(%s), id(%s))", org_rev, other_rev)
                 if ancestors:
                     # FIXME: picks arbitrary ancestor - but there is usually only one
                     try:
                         ancestor = hgrepo[ancestors.first()].hex()
                     except AttributeError:
                         # removed in hg 3.2
                         ancestor = hgrepo[ancestors[0]].hex()
                 log.debug("no shortcut found: %s is not an ancestor of %s", other_rev, org_rev)
                 ancestors = [hgrepo[ancestor].hex() for ancestor in
                              hgrepo.revs("heads(::id(%s) & ::id(%s))", org_rev, other_rev)] # FIXME: expensive!
             other_revs = hgrepo.revs("ancestors(id(%s)) and not ancestors(id(%s)) and not id(%s)",
                                      other_rev, org_rev, org_rev)
             other_changesets = [other_repo.get_changeset(rev) for rev in other_revs]
             org_revs = hgrepo.revs("ancestors(id(%s)) and not ancestors(id(%s)) and not id(%s)",
                                    org_rev, other_rev, other_rev)
             org_changesets = [org_repo.get_changeset(hgrepo[rev].hex()) for rev in org_revs]
         elif alias == 'git':
@@ @@ -147,10 +141,10 @@ class CompareController(BaseRepoControll @@
                 other_changesets = [other_repo.get_changeset(rev) for rev in reversed(revs)]
                 if other_changesets:
-                    ancestor = other_changesets[0].parents[0].raw_id
+                    ancestors = [other_changesets[0].parents[0].raw_id]
                 else:
                     # no changesets from other repo, ancestor is the other_rev
-                    ancestor = other_rev
+                    ancestors = [other_rev]
                 # dulwich 0.9.9 doesn't have a Repo.close() so we have to mess with internals:
                 gitrepo.object_store.close()
@@ @@ -166,13 +160,13 @@ class CompareController(BaseRepoControll @@
                 so, se = org_repo.run_git_command(
                     ['merge-base', org_rev, other_rev]
+                )
-                ancestor = re.findall(r'[0-9a-fA-F]{40}', so)[0]
+                ancestors = [re.findall(r'[0-9a-fA-F]{40}', so)[0]]
             org_changesets = []
         else:
             raise Exception('Bad alias only git and hg is allowed')
-        return other_changesets, org_changesets, ancestor
+        return other_changesets, org_changesets, ancestors
     @LoginRequired()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
@@ @@ -228,7 +222,7 @@ class CompareController(BaseRepoControll @@
         c.cs_ref_name = other_ref_name
         c.cs_ref_type = other_ref_type
-        c.cs_ranges, c.cs_ranges_org, c.ancestor = self._get_changesets(
+        c.cs_ranges, c.cs_ranges_org, c.ancestors = self._get_changesets(
             c.a_repo.scm_instance.alias, c.a_repo.scm_instance, c.a_rev,
             c.cs_repo.scm_instance, c.cs_rev)
         raw_ids = [x.raw_id for x in c.cs_ranges]
@@ @@ -244,17 +238,28 @@ class CompareController(BaseRepoControll @@
         org_repo = c.a_repo
         other_repo = c.cs_repo
         if merge and c.ancestor:
         if merge:
             rev1 = msg = None
             if not c.cs_ranges:
                 msg = _('Cannot show empty diff')
             elif not c.ancestors:
                 msg = _('No ancestor found for merge diff')
             elif len(c.ancestors) == 1:
                 rev1 = c.ancestors[0]
             else:
                 msg = _('Multiple merge ancestors found for merge compare')
             if rev1 is None:
                 h.flash(msg, category='error')
                 log.error(msg)
                 raise HTTPNotFound
             # case we want a simple diff without incoming changesets,
             # previewing what will be merged.
             # Make the diff on the other repo (which is known to have other_rev)
             log.debug('Using ancestor %s as rev1 instead of %s',
                       c.ancestor, c.a_rev)
             rev1 = c.ancestor
                       rev1, c.a_rev)
             org_repo = other_repo
         else: # comparing tips, not necessarily linearly related
             if merge:
                 log.error('Unable to find ancestor revision')
             if org_repo != other_repo:
                 # TODO: we could do this by using hg unionrepo
                 log.error('cannot compare across repos %s and %s', org_repo, other_repo)

kallithea/controllers/pullrequests.py

➞

Show inline comments

@@ @@ -343,13 +343,26 @@ class PullrequestsController(BaseRepoCon @@
             other_ref_name = cs.raw_id[:12]
             other_ref = '%s:%s:%s' % (other_ref_type, other_ref_name, cs.raw_id)
-        cs_ranges, _cs_ranges_not, ancestor_rev = \
+        cs_ranges, _cs_ranges_not, ancestor_revs = \
             CompareController._get_changesets(org_repo.scm_instance.alias,
                                               other_repo.scm_instance, other_rev, # org and other "swapped"
                                               org_repo.scm_instance, org_rev,
+                                              )
         ancestor_rev = msg = None
         if not cs_ranges:
             msg = _('Cannot create empty pull request')
         elif not ancestor_revs:
             ancestor_rev = org_repo.scm_instance.EMPTY_CHANGESET
         elif len(ancestor_revs) == 1:
             ancestor_rev = ancestor_revs[0]
         else:
             msg = _('Cannot create pull request - criss cross merge detected, please merge a later %s revision to %s'
                     ) % (other_ref_name, org_ref_name)
         if ancestor_rev is None:
             ancestor_rev = org_repo.scm_instance.EMPTY_CHANGESET
             h.flash(msg, category='error')
             log.error(msg)
             raise HTTPNotFound
         revisions = [cs_.raw_id for cs_ in cs_ranges]
         # hack: ancestor_rev is not an other_rev but we want to show the
@@ @@ -396,9 +409,23 @@ class PullrequestsController(BaseRepoCon @@
         #assert other_ref_type == 'branch', other_ref_type # TODO: what if not?
         new_other_rev = self._get_ref_rev(other_repo, other_ref_type, other_ref_name)
-        cs_ranges, _cs_ranges_not, ancestor_rev = CompareController._get_changesets(org_repo.scm_instance.alias,
+        cs_ranges, _cs_ranges_not, ancestor_revs = CompareController._get_changesets(org_repo.scm_instance.alias,
             other_repo.scm_instance, new_other_rev, # org and other "swapped"
             org_repo.scm_instance, new_org_rev)
         ancestor_rev = msg = None
         if not cs_ranges:
             msg = _('Cannot create empty pull request update') # cannot happen!
         elif not ancestor_revs:
             msg = _('Cannot create pull request update - no common ancestor found') # cannot happen
         elif len(ancestor_revs) == 1:
             ancestor_rev = ancestor_revs[0]
         else:
             msg = _('Cannot create pull request update - criss cross merge detected, please merge a later %s revision to %s'
                     ) % (other_ref_name, org_ref_name)
         if ancestor_rev is None:
             h.flash(msg, category='error')
             log.error(msg)
             raise HTTPNotFound
         old_revisions = set(old_pull_request.revisions)
         revisions = [cs.raw_id for cs in cs_ranges]
@@ @@ -586,7 +613,7 @@ class PullrequestsController(BaseRepoCon @@
         c.a_repo = c.pull_request.other_repo
         (c.a_ref_type,
          c.a_ref_name,
-         c.a_rev) = c.pull_request.other_ref.split(':') # other_rev is ancestor
+         c.a_rev) = c.pull_request.other_ref.split(':') # a_rev is ancestor
         org_scm_instance = c.cs_repo.scm_instance # property with expensive cache invalidation check!!!
         c.cs_repo = c.cs_repo
@@ @@ -746,7 +773,7 @@ class PullrequestsController(BaseRepoCon @@
         c.changeset_statuses = ChangesetStatus.STATUSES
         c.as_form = False
-        c.ancestor = None # there is one - but right here we don't know which
+        c.ancestors = None # [c.a_rev] ... but that is shown in an other way
         return render('/pullrequests/pullrequest_show.html')
     @LoginRequired()

kallithea/templates/compare/compare_cs.html

➞

Show inline comments

@@ @@ -4,10 +4,23 @@ @@
     <span class="empty_data">${_('No changesets')}</span>
   %else:
     %if c.ancestor:
     <div class="ancestor">${_('Ancestor')}:
       ${h.link_to(h.short_id(c.ancestor),h.url('changeset_home',repo_name=c.repo_name,revision=c.ancestor), class_="changeset_hash")}
     </div>
     %if c.ancestors:
       <div class="ancestor">
         %if len(c.ancestors) > 1:
         <div style="color:red; font-weight: bold">
           ${_('Criss cross merge situation with multiple merge ancestors detected!')}
         </div>
         <div style="color:red">
           ${_('Please merge the target branch to your branch before creating a pull request.')}
         </div>
         %endif
         <div>
           ${_('Merge Ancestor')}:
           %for ancestor in c.ancestors:
             ${h.link_to(h.short_id(ancestor),h.url('changeset_home',repo_name=c.repo_name,revision=ancestor), class_="changeset_hash")}
           %endfor
         </div>
       </div>
     %endif
     <div id="graph_nodes">
@@ @@ -93,14 +106,6 @@ @@
           merge='1')
         )}
       </div>
       <div style="font-size:1.1em;font-weight: bold;clear:both;padding-top:10px">
         ${_('Common ancestor')}:
         %if c.ancestor:
         ${h.link_to(h.short_id(c.ancestor),h.url('changeset_home',repo_name=c.repo_name,revision=c.ancestor), class_="changeset_hash")}
         %else:
         ${_('No common ancestor found - repositories are unrelated')}
         %endif
       </div>
     %endif
     %if c.cs_ranges_org is not None:
       ## TODO: list actual changesets?

kallithea/tests/functional/test_compare.py

➞

Show inline comments

@@ @@ -489,12 +489,9 @@ class TestCompareController(TestControll @@
                                     other_ref_type="branch",
                                     other_ref_name=rev1,
                                     other_repo=r1_name,
                                     merge='1',))
+                                    merge='1',), status=404)
         response.mustcontain('%s@%s' % (r2_name, rev1))
         response.mustcontain('%s@%s' % (r1_name, rev2))
         response.mustcontain('No files')
         response.mustcontain('No changesets')
         response.mustcontain('Cannot show empty diff')
         cs0 = fixture.commit_change(repo=r1_name, filename='file2',
                 content='line1-added-after-fork', message='commit2-parent',
@@ @@ -569,12 +566,9 @@ class TestCompareController(TestControll @@
                                     other_ref_type="branch",
                                     other_ref_name=rev2,
                                     other_repo=r1_name,
                                     merge='1',))
+                                    merge='1',), status=404)
         response.mustcontain('%s@%s' % (r2_name, rev1))
         response.mustcontain('%s@%s' % (r1_name, rev2))
         response.mustcontain('No files')
         response.mustcontain('No changesets')
         response.mustcontain('Cannot show empty diff')
         cs0 = fixture.commit_change(repo=r1_name, filename='file2',
                 content='line1-added-after-fork', message='commit2-parent',

kallithea/tests/functional/test_compare_local.py

➞

Show inline comments

@@ @@ -207,7 +207,7 @@ class TestCompareController(TestControll @@
         ## outgoing changesets between those revisions
         response.mustcontain("""<a class="changeset_hash" href="/%s/changeset/3d8f361e72ab303da48d799ff1ac40d5ac37c67e">r1:%s</a>""" % (HG_REPO, rev2))
-        response.mustcontain('Common ancestor')
+        response.mustcontain('Merge Ancestor')
         response.mustcontain("""<a class="changeset_hash" href="/%s/changeset/b986218ba1c9b0d6a259fac9b050b1724ed8e545">%s</a>""" % (HG_REPO, rev1))
     def test_compare_revisions_git_as_form(self):
@@ @@ -227,5 +227,5 @@ class TestCompareController(TestControll @@
         ## outgoing changesets between those revisions
         response.mustcontain("""<a class="changeset_hash" href="/%s/changeset/38b5fe81f109cb111f549bfe9bb6b267e10bc557">r1:%s</a>""" % (GIT_REPO, rev2[:12]))
-        response.mustcontain('Common ancestor')
+        response.mustcontain('Merge Ancestor')
         response.mustcontain("""<a class="changeset_hash" href="/%s/changeset/c1214f7e79e02fc37156ff215cd71275450cffc3">%s</a>""" % (GIT_REPO, rev1[:12]))

0 comments (0 inline, 0 general)