1169499 - Reviewers reset unless r? syntax is present

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Gregory Szorc [:gps]]

I just pushed a new review series to https://reviewboard.mozilla.org/r/7951/. I had reviewers set on all the old published review requests. After pushing new, rebased commits, reviewers have disappeared from the draft review requests (I'm not using r? syntax to define reviewers via commit messages). I reckon the clearing of reviewers is related to supporting the r? syntax.

If reviewers are specified in the review request, I don't think a new push w/o reviewers should clear the existing reviewers.

Comment 1

[Dan Minor [:dminor]]

This is essentially the same problem as Bug 1164132 but I think it is worth solving separately as it is clearly undesirable behaviour that is easy to avoid.

Comment 2

[Dan Minor [:dminor]]

Attachment: MozReview Request: reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r?gps

reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r?gps

This is a symptom of a larger problem where specifying reviewers in the commit
summary causes any reviewers specified in the UI to be lost. Solving that
larger problem requires us to have a good means of identifying reviewers which
were specified in the UI and which were specified in the commit summary.

In the mean time, if no reviewers are specified in the commit summary we can
skip setting the reviewers on the draft, which will preserve reviewers that
were set in the UI if none are specified in the commit.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8612805 [details]
MozReview Request: reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r?gps

https://reviewboard.mozilla.org/r/9635/#review8467

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:347
(Diff revision 1)
> +        else:
> +            draft = rr.get_or_create_draft(
> +                    summary=commit['message'].splitlines()[0],
> +                    description=commit['message'])

I liked your `props` dict approach from above to avoid code duplication. I think you should change this code while you are here.

::: hgext/reviewboard/tests/test-specify-reviewers.t:332
(Diff revision 1)
> -  $ hg commit --amend -m 'Bug 1 - Amended stuff; r?romulus, r?remus'
> +  $ hg commit --amend -m 'Bug 1 - Amended more stuff; r?romulus, r?remus'

I don't think you needed to change the commit message here. But it doesn't matter too much.

Comment 4

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools

Comment 5

[Dan Minor [:dminor]]

(In reply to Dan Minor [:dminor] from comment #4)
> Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools

Well, obviously :)

The revision is: https://hg.mozilla.org/hgcustom/version-control-tools/rev/665dfb80447a