Mercurial > kallithea
changeset 6302:84eb5b7b1bac
pullrequests: prevent creation of invalid pull requests, empty or unrelated or criss cross
| author | Mads Kiilerich <madski@unity3d.com> |
|---|---|
| date | Wed, 11 Feb 2015 03:05:00 +0100 |
| parents | 699f432b62fb |
| children | 1cf51cd05e36 |
| files | 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 |
| diffstat | 5 files changed, 86 insertions(+), 55 deletions(-) [+] |
line wrap: on
line diff
--- a/kallithea/controllers/compare.py Thu Nov 10 16:10:41 2016 +0100 +++ b/kallithea/controllers/compare.py Wed Feb 11 03:05:00 2015 +0100 @@ -32,7 +32,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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)
--- a/kallithea/controllers/pullrequests.py Thu Nov 10 16:10:41 2016 +0100 +++ b/kallithea/controllers/pullrequests.py Wed Feb 11 03:05:00 2015 +0100 @@ -343,13 +343,26 @@ 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 @@ #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 @@ 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 @@ 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()
--- a/kallithea/templates/compare/compare_cs.html Thu Nov 10 16:10:41 2016 +0100 +++ b/kallithea/templates/compare/compare_cs.html Wed Feb 11 03:05:00 2015 +0100 @@ -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?
--- a/kallithea/tests/functional/test_compare.py Thu Nov 10 16:10:41 2016 +0100 +++ b/kallithea/tests/functional/test_compare.py Wed Feb 11 03:05:00 2015 +0100 @@ -489,12 +489,9 @@ 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 @@ 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',
--- a/kallithea/tests/functional/test_compare_local.py Thu Nov 10 16:10:41 2016 +0100 +++ b/kallithea/tests/functional/test_compare_local.py Wed Feb 11 03:05:00 2015 +0100 @@ -207,7 +207,7 @@ ## 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 @@ ## 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]))