Changeset - abaf8e1033a6
[Not reviewed]
default
0 2 0
Thomas De Schampheleire - 8 years ago 2018-05-19 22:00:41
thomas.de_schampheleire@nokia.com
pullrequests: don't show empty "additional changesets" (issue #280)

When opening a pullrequest on a revision range including the tipmost
revision, and then pushing a new revision on top of that, the PR page shows:
This pullrequest can be updated with changes on ...:
and then nothing.

This is because the available revisions looped over are in 'avail_cs' but
the guard around the loop is checking on 'avail_revs'. The former, while
derived from avail_revs, can become empty under circumstances like this one.

Fix the problem by changing the guard checking avail_cs rather than
avail_revs, and making sure the printed message is aligned to 'No additional
changes found'.
2 files changed with 4 insertions and 1 deletions:
0 comments (0 inline, 0 general)
kallithea/controllers/pullrequests.py
Show inline comments
 
@@ -448,194 +448,197 @@ class PullrequestsController(BaseRepoCon
 
        raise HTTPForbidden()
 

	
 
    @LoginRequired(allow_default_user=True)
 
    @HasRepoPermissionLevelDecorator('read')
 
    def show(self, repo_name, pull_request_id, extra=None):
 
        repo_model = RepoModel()
 
        c.users_array = repo_model.get_users_js()
 
        c.pull_request = PullRequest.get_or_404(pull_request_id)
 
        c.allowed_to_change_status = self._get_is_allowed_change_status(c.pull_request)
 
        cc_model = ChangesetCommentsModel()
 
        cs_model = ChangesetStatusModel()
 

	
 
        # pull_requests repo_name we opened it against
 
        # ie. other_repo must match
 
        if repo_name != c.pull_request.other_repo.repo_name:
 
            raise HTTPNotFound
 

	
 
        # load compare data into template context
 
        c.cs_repo = c.pull_request.org_repo
 
        (c.cs_ref_type,
 
         c.cs_ref_name,
 
         c.cs_rev) = c.pull_request.org_ref.split(':')
 

	
 
        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(':') # a_rev is ancestor
 

	
 
        org_scm_instance = c.cs_repo.scm_instance # property with expensive cache invalidation check!!!
 
        try:
 
            c.cs_ranges = []
 
            for x in c.pull_request.revisions:
 
                c.cs_ranges.append(org_scm_instance.get_changeset(x))
 
        except ChangesetDoesNotExistError:
 
            c.cs_ranges = []
 
            h.flash(_('Revision %s not found in %s') % (x, c.cs_repo.repo_name),
 
                'error')
 
        c.cs_ranges_org = None # not stored and not important and moving target - could be calculated ...
 
        revs = [ctx.revision for ctx in reversed(c.cs_ranges)]
 
        c.jsdata = graph_data(org_scm_instance, revs)
 

	
 
        c.is_range = False
 
        try:
 
            if c.a_ref_type == 'rev': # this looks like a free range where target is ancestor
 
                cs_a = org_scm_instance.get_changeset(c.a_rev)
 
                root_parents = c.cs_ranges[0].parents
 
                c.is_range = cs_a in root_parents
 
                #c.merge_root = len(root_parents) > 1 # a range starting with a merge might deserve a warning
 
        except ChangesetDoesNotExistError: # probably because c.a_rev not found
 
            pass
 
        except IndexError: # probably because c.cs_ranges is empty, probably because revisions are missing
 
            pass
 

	
 
        avail_revs = set()
 
        avail_show = []
 
        c.cs_branch_name = c.cs_ref_name
 
        c.a_branch_name = None
 
        other_scm_instance = c.a_repo.scm_instance
 
        c.update_msg = ""
 
        c.update_msg_other = ""
 
        try:
 
            if not c.cs_ranges:
 
                c.update_msg = _('Error: changesets not found when displaying pull request from %s.') % c.cs_rev
 
            elif org_scm_instance.alias == 'hg' and c.a_ref_name != 'ancestor':
 
                if c.cs_ref_type != 'branch':
 
                    c.cs_branch_name = org_scm_instance.get_changeset(c.cs_ref_name).branch # use ref_type ?
 
                c.a_branch_name = c.a_ref_name
 
                if c.a_ref_type != 'branch':
 
                    try:
 
                        c.a_branch_name = other_scm_instance.get_changeset(c.a_ref_name).branch # use ref_type ?
 
                    except EmptyRepositoryError:
 
                        c.a_branch_name = 'null' # not a branch name ... but close enough
 
                # candidates: descendants of old head that are on the right branch
 
                #             and not are the old head itself ...
 
                #             and nothing at all if old head is a descendant of target ref name
 
                if not c.is_range and other_scm_instance._repo.revs('present(%s)::&%s', c.cs_ranges[-1].raw_id, c.a_branch_name):
 
                    c.update_msg = _('This pull request has already been merged to %s.') % c.a_branch_name
 
                elif c.pull_request.is_closed():
 
                    c.update_msg = _('This pull request has been closed and can not be updated.')
 
                else: # look for descendants of PR head on source branch in org repo
 
                    avail_revs = org_scm_instance._repo.revs('%s:: & branch(%s)',
 
                                                             revs[0], c.cs_branch_name)
 
                    if len(avail_revs) > 1: # more than just revs[0]
 
                        # also show changesets that not are descendants but would be merged in
 
                        targethead = other_scm_instance.get_changeset(c.a_branch_name).raw_id
 
                        if org_scm_instance.path != other_scm_instance.path:
 
                            # Note: org_scm_instance.path must come first so all
 
                            # valid revision numbers are 100% org_scm compatible
 
                            # - both for avail_revs and for revset results
 
                            hgrepo = unionrepo.unionrepository(org_scm_instance.baseui,
 
                                                               org_scm_instance.path,
 
                                                               other_scm_instance.path)
 
                        else:
 
                            hgrepo = org_scm_instance._repo
 
                        show = set(hgrepo.revs('::%ld & !::parents(%s) & !::%s',
 
                                               avail_revs, revs[0], targethead))
 
                        if show:
 
                        c.update_msg = _('The following additional changes are available on %s:') % c.cs_branch_name
 
                    else:
 
                            c.update_msg = _('No additional changesets found for iterating on this pull request.')
 
                    else:
 
                        show = set()
 
                        avail_revs = set() # drop revs[0]
 
                        c.update_msg = _('No additional changesets found for iterating on this pull request.')
 

	
 
                    # TODO: handle branch heads that not are tip-most
 
                    brevs = org_scm_instance._repo.revs('%s - %ld - %s', c.cs_branch_name, avail_revs, revs[0])
 
                    if brevs:
 
                        # also show changesets that are on branch but neither ancestors nor descendants
 
                        show.update(org_scm_instance._repo.revs('::%ld - ::%ld - ::%s', brevs, avail_revs, c.a_branch_name))
 
                        show.add(revs[0]) # make sure graph shows this so we can see how they relate
 
                        c.update_msg_other = _('Note: Branch %s has another head: %s.') % (c.cs_branch_name,
 
                            h.short_id(org_scm_instance.get_changeset((max(brevs))).raw_id))
 

	
 
                    avail_show = sorted(show, reverse=True)
 

	
 
            elif org_scm_instance.alias == 'git':
 
                c.cs_repo.scm_instance.get_changeset(c.cs_rev) # check it exists - raise ChangesetDoesNotExistError if not
 
                c.update_msg = _("Git pull requests don't support iterating yet.")
 
        except ChangesetDoesNotExistError:
 
            c.update_msg = _('Error: some changesets not found when displaying pull request from %s.') % c.cs_rev
 

	
 
        c.avail_revs = avail_revs
 
        c.avail_cs = [org_scm_instance.get_changeset(r) for r in avail_show]
 
        c.avail_jsdata = graph_data(org_scm_instance, avail_show)
 

	
 
        raw_ids = [x.raw_id for x in c.cs_ranges]
 
        c.cs_comments = c.cs_repo.get_comments(raw_ids)
 
        c.cs_statuses = c.cs_repo.statuses(raw_ids)
 

	
 
        ignore_whitespace = request.GET.get('ignorews') == '1'
 
        line_context = safe_int(request.GET.get('context'), 3)
 
        c.ignorews_url = _ignorews_url
 
        c.context_url = _context_url
 
        fulldiff = request.GET.get('fulldiff')
 
        diff_limit = None if fulldiff else self.cut_off_limit
 

	
 
        # we swap org/other ref since we run a simple diff on one repo
 
        log.debug('running diff between %s and %s in %s',
 
                  c.a_rev, c.cs_rev, org_scm_instance.path)
 
        try:
 
            raw_diff = diffs.get_diff(org_scm_instance, rev1=safe_str(c.a_rev), rev2=safe_str(c.cs_rev),
 
                                      ignore_whitespace=ignore_whitespace, context=line_context)
 
        except ChangesetDoesNotExistError:
 
            raw_diff = _("The diff can't be shown - the PR revisions could not be found.")
 
        diff_processor = diffs.DiffProcessor(raw_diff or '', diff_limit=diff_limit)
 
        c.limited_diff = diff_processor.limited_diff
 
        c.file_diff_data = []
 
        c.lines_added = 0
 
        c.lines_deleted = 0
 

	
 
        for f in diff_processor.parsed:
 
            st = f['stats']
 
            c.lines_added += st['added']
 
            c.lines_deleted += st['deleted']
 
            filename = f['filename']
 
            fid = h.FID('', filename)
 
            html_diff = diffs.as_html(enable_comments=True, parsed_lines=[f])
 
            c.file_diff_data.append((fid, None, f['operation'], f['old_filename'], filename, html_diff, st))
 

	
 
        # inline comments
 
        c.inline_cnt = 0
 
        c.inline_comments = cc_model.get_inline_comments(
 
                                c.db_repo.repo_id,
 
                                pull_request=pull_request_id)
 
        # count inline comments
 
        for __, lines in c.inline_comments:
 
            for comments in lines.values():
 
                c.inline_cnt += len(comments)
 
        # comments
 
        c.comments = cc_model.get_comments(c.db_repo.repo_id, pull_request=pull_request_id)
 

	
 
        # (badly named) pull-request status calculation based on reviewer votes
 
        (c.pull_request_reviewers,
 
         c.pull_request_pending_reviewers,
 
         c.current_voting_result,
 
         ) = cs_model.calculate_pull_request_result(c.pull_request)
 
        c.changeset_statuses = ChangesetStatus.STATUSES
 

	
 
        c.is_ajax_preview = False
 
        c.ancestors = None # [c.a_rev] ... but that is shown in an other way
 
        return render('/pullrequests/pullrequest_show.html')
 

	
 
    @LoginRequired()
 
    @HasRepoPermissionLevelDecorator('read')
 
    @jsonify
 
    def comment(self, repo_name, pull_request_id):
 
        pull_request = PullRequest.get_or_404(pull_request_id)
 

	
 
        status = request.POST.get('changeset_status')
 
        close_pr = request.POST.get('save_close')
 
        delete = request.POST.get('save_delete')
 
        f_path = request.POST.get('f_path')
 
        line_no = request.POST.get('line')
 

	
 
        if (status or close_pr or delete) and (f_path or line_no):
 
            # status votes and closing is only possible in general comments
kallithea/templates/pullrequests/pullrequest_show.html
Show inline comments
 
@@ -43,193 +43,193 @@ ${self.repo_context_bar('showpullrequest
 
            %endif
 
            </label>
 
            <div>
 
              <div class="formatted-fixed">${h.urlify_text(c.pull_request.description, c.pull_request.org_repo.repo_name)}</div>
 
            </div>
 
        </div>
 

	
 
        %if editable:
 
        <div class="pr-do-edit form-group" style="display:none">
 
              <label for="pullrequest_title">${_('Title')}:</label>
 
              <div>
 
                  ${h.text('pullrequest_title',class_='form-control',value=c.pull_request.title,placeholder=_('Summarize the changes'))}
 
              </div>
 
        </div>
 

	
 
        <div class="pr-do-edit form-group" style="display:none">
 
              <label for="pullrequest_desc">${_('Description')}:</label>
 
              <div>
 
                  ${h.textarea('pullrequest_desc',content=c.pull_request.description,placeholder=_('Write a short description on this pull request'),class_='form-control')}
 
              </div>
 
        </div>
 
        %endif
 

	
 
        <div class="form-group">
 
          <label>${_('Voting Result')}:</label>
 
          <div>
 
            %if c.current_voting_result:
 
              <i class="icon-circle changeset-status-${c.current_voting_result}" title="${_('Pull request status calculated from votes')}"></i>
 
              <span class="changeset-status-lbl" data-toggle="tooltip" title="${_('Pull request status calculated from votes')}">
 
                %if c.pull_request.is_closed():
 
                    ${_('Closed')},
 
                %endif
 
                ${h.changeset_status_lbl(c.current_voting_result)}
 
              </span>
 
            %endif
 
          </div>
 
        </div>
 
        <div class="form-group">
 
          <label>${_('Origin')}:</label>
 
          <div>
 
            <div>
 
              ${h.link_to_ref(c.pull_request.org_repo.repo_name, c.cs_ref_type, c.cs_ref_name, c.cs_rev)}
 
              %if c.cs_ref_type != 'branch':
 
                ${_('on')} ${h.link_to_ref(c.pull_request.org_repo.repo_name, 'branch', c.cs_branch_name)}
 
              %endif
 
            </div>
 
          </div>
 
        </div>
 
        <div class="form-group">
 
          <label>${_('Target')}:</label>
 
          <div>
 
            %if c.is_range:
 
              ${_("This is just a range of changesets and doesn't have a target or a real merge ancestor.")}
 
            %else:
 
              ${h.link_to_ref(c.pull_request.other_repo.repo_name, c.a_ref_type, c.a_ref_name)}
 
              ## we don't know other rev - c.a_rev is ancestor and not necessarily on other_name_branch branch
 
            %endif
 
          </div>
 
        </div>
 
        <div class="form-group">
 
          <label>${_('Pull changes')}:</label>
 
          <div>
 
            %if c.cs_ranges:
 
              <div>
 
               ## TODO: use cs_ranges[-1] or org_ref_parts[1] in both cases?
 
               %if h.is_hg(c.pull_request.org_repo):
 
                 <span>hg pull ${c.pull_request.org_repo.clone_url()} -r ${h.short_id(c.cs_ranges[-1].raw_id)}</span>
 
               %elif h.is_git(c.pull_request.org_repo):
 
                 <span>git pull ${c.pull_request.org_repo.clone_url()} ${c.pull_request.org_ref_parts[1]}</span>
 
               %endif
 
              </div>
 
            %endif
 
          </div>
 
        </div>
 
        <div class="form-group">
 
          <label>${_('Created on')}:</label>
 
          <div>
 
              <div>${h.fmt_date(c.pull_request.created_on)}</div>
 
          </div>
 
        </div>
 
        <div class="form-group">
 
          <label>${_('Owner')}:</label>
 
          <div class="pr-not-edit">
 
                  ${h.gravatar_div(c.pull_request.owner.email, size=20)}
 
                  <span>${c.pull_request.owner.full_name_and_username}</span><br/>
 
                  <span><a href="mailto:${c.pull_request.owner.email}">${c.pull_request.owner.email}</a></span><br/>
 
          </div>
 
          <div class="pr-do-edit" style="display:none">
 
               ${h.text('owner', class_='form-control', value=c.pull_request.owner.username, placeholder=_('Type name of user'))}
 
          </div>
 
        </div>
 

	
 
        <div class="form-group">
 
          <label>${_('Next iteration')}:</label>
 
            <div>
 
              <p>${c.update_msg}</p>
 
              %if c.avail_revs:
 
              %if c.avail_cs:
 
              <div id="updaterevs" class="clearfix">
 
                <div id="updaterevs-graph">
 
                  <canvas id="avail_graph_canvas"></canvas>
 
                </div>
 
                <table class="table" id="updaterevs-table">
 
                  %for cnt, cs in enumerate(c.avail_cs):
 
                    <tr id="chg_available_${cnt+1}" class="${'mergerow' if len(cs.parents) > 1 and not (editable and cs.revision in c.avail_revs) else ''}">
 
                      %if c.cs_ranges and cs.revision == c.cs_ranges[-1].revision:
 
                        %if editable:
 
                        <td>
 
                            ${h.radio(name='updaterev', value='', checked=True)}
 
                        </td>
 
                        %endif
 
                        <td colspan="4"><span>${_("Current revision - no change")}</span></td>
 
                      %else:
 
                        %if editable:
 
                        <td>
 
                          ${h.radio(name='updaterev', value=cs.raw_id, style=None if cs.revision in c.avail_revs else 'visibility: hidden')}
 
                        </td>
 
                        %endif
 
                        <td><span data-toggle="tooltip" title="${h.age(cs.date)}">${cs.date}</span></td>
 
                        <td>${h.link_to(h.show_id(cs),h.url('changeset_home',repo_name=c.cs_repo.repo_name,revision=cs.raw_id), class_='changeset_hash')}</td>
 
                        <td>
 
                          <div class="pull-right">
 
                            %for tag in cs.tags:
 
                              <span class="label label-tag" title="${_('Tag %s') % tag}">
 
                                ${h.link_to(tag,h.url('changeset_home',repo_name=c.repo_name,revision=cs.raw_id))}
 
                              </span>
 
                            %endfor
 
                          </div>
 
                          <div class="message">${h.urlify_text(cs.message, c.repo_name)}</div>
 
                        </td>
 
                      %endif
 
                    </tr>
 
                  %endfor
 
                </table>
 
              </div>
 
              <div class="alert alert-info">${_("Pull request iterations do not change content once created. Select a revision to create a new iteration.")}</div>
 
              %endif
 
              %if c.update_msg_other:
 
                <div class="alert alert-info">${c.update_msg_other}</div>
 
              %endif
 
            </div>
 
        </div>
 
        %if editable:
 
        <div class="form-group">
 
          <div class="buttons">
 
            ${h.submit('pr-form-save',_('Save Changes'),class_="btn btn-default btn-sm")}
 
            ${h.submit('pr-form-clone',_('Create New Iteration with Changes'),class_="btn btn-default btn-sm",disabled='disabled')}
 
            ${h.reset('pr-form-reset',_('Cancel Changes'),class_="btn btn-default btn-sm")}
 
          </div>
 
        </div>
 
        %endif
 
      </div>
 
    </div>
 
    ## REVIEWERS
 
    <div class="pr-reviewers-box pull-left">
 
        <h4 class="pr-details-title">${_('Reviewers')}</h4>
 
        <div id="reviewers">
 
          ## members goes here !
 
          <div>
 
            %for member,status in c.pull_request_reviewers:
 
              <input type="hidden" value="${member.user_id}" name="org_review_members" />
 
            %endfor
 
            <ul id="review_members" class="list-unstyled">
 
            %for member,status in c.pull_request_reviewers:
 
              ## WARNING: the HTML below is duplicate with
 
              ## kallithea/public/js/base.js
 
              ## If you change something here it should be reflected in the template too.
 
              <li id="reviewer_${member.user_id}">
 
                <span class="reviewers_member">
 
                  <input type="hidden" value="${member.user_id}" name="review_members" />
 
                  <span class="reviewer_status" data-toggle="tooltip" title="${h.changeset_status_lbl(status)}">
 
                      <i class="icon-circle changeset-status-${status}"></i>
 
                  </span>
 
                  ${h.gravatar(member.email, size=14)}
 
                  <span>
 
                    ${member.full_name_and_username}
 
                    %if c.pull_request.owner_id == member.user_id:
 
                      (${_('Owner')})
 
                    %endif
 
                  </span>
 
                  %if editable:
 
                  <a href="#" class="reviewer_member_remove" onclick="removeReviewMember(${member.user_id})" title="${_('Remove reviewer')}">
 
                      <i class="icon-minus-circled"></i>
 
                  </a>
 
                  %endif
 
                </span>
 
              </li>
 
            %endfor
 
            </ul>
 
          </div>
 
          %if editable:
 
          <div>
 
             ${h.text('user', class_='form-control',placeholder=_('Type name of reviewer to add'))}
 
          </div>
0 comments (0 inline, 0 general)