1097213 - |hg push -r tip::tip review| leads to "submitting 2 changesets for review"

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Matthew N. [:MattN]]

Attachment: Console output

I used the new syntax from bug 1089685 to push one commit (tip as the base and revision to be reviewed) but it ended up trying to make reviews for the mq patch applied before base too.

Comment 1

[Gregory Szorc [:gps]]

To clarify, the use case is selecting a single commit in the middle of a draft sequence that is to be reviewed. i.e. X^ is draft, X is not a head, and children(X) are draft.

Overloading revsets for this is both difficult and arguably not appropriate. We may need to introduce `push -c <node>` to specify a single changeset to push. Alternatively, we could add `push --single` to signal that only a single revision should be reviewed.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ah, I thought I was going nuts, failing to make this work. I'd made the assumption that this would just work with bug 1089685. It seemed like an obvious use-case, so bad communication on my part.

FWIW, I'd vote for `push -c rev`, since -c is commonly used elsewhere.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I just hit this...

Is there a way to check what pushing to the review repository will push before actually doing the push? should I type the whole repo url into hg outgoing -b ?

Comment 4

[Gregory Szorc [:gps]]

Since review requests go into a draft state before being published, there really isn't much risk to accidentally pushing the wrong things: you can just push again and publish the new set of drafts. Nobody but you will know there was a mix-up.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #4)
> Since review requests go into a draft state before being published, there
> really isn't much risk to accidentally pushing the wrong things: you can
> just push again and publish the new set of drafts. Nobody but you will know
> there was a mix-up.

It's frustrating and time consuming to go through to reviewboard multiple times to close review requests when things don't go as you had hoped. Filed bug 1103679 for a way to do a dry-run.

Comment 6

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1097213/gps

/r/3813 - reviewboard: add -c to push to allow single changesets to be reviewed (bug 1097213)

Pull down this commit:

hg pull review -r c268dd4b41326ead1b4b74fcf9517f2afd791e2a

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/3813/#review3071

Looks good for the most part, we've *really* been needing this!

::: hgext/reviewboard/client.py
(Diff revision 1)
> +        for node in repo[reviewnode].ancestors():
> +            ctx = repo[node]
>  
> -        if ctx.phase() == phases.public:
> +            if ctx.phase() == phases.public:

hmm, so this isn't really new due to this patch and I may be totally wrong here, but could we not get into a really strange situation if someone has secret changsets. Take the following state:
```
P=Public
D=Draft
S=Secret

Remote(P) <- C1(D) <- C2(D) <- C3(S) <- C4(S)
```

Then the user runs `hg push -r C4`, hg will transfer `C1`, and `C2` to the remote, but not `C3` or `C4`. After which `doreview()` kicks in with `reviewnode == C4` and we'll try to create a review request including the secret `C3` and `C4` changesets.

This will obviously fail since the server will not have `C3` and `C4`. So, we do need to be adding a guard somewhere in our wrapped push to check for secret changesets? or possibly bail out of review creation if we detect a secret changeset here?

::: hgext/reviewboard/client.py
(Diff revision 1)
> +                    _('Only review this specific')))

"Only review this specific" changeset? Is hg going to automatically append the option name or something?

::: docs/mozreview/review-requests.rst
(Diff revision 1)
> +In this form, the specified commit and all of its unpublished ancestors will

I wonder if we should be saying "draft" rather than "unpublished" to stay consistent?

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8563750 [details]
MozReview Request: bz://1097213/gps

https://reviewboard.mozilla.org/r/3809/#review3073

Fix it, then Ship-It!

Comment 9

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/3813/#review3093

> hmm, so this isn't really new due to this patch and I may be totally wrong here, but could we not get into a really strange situation if someone has secret changsets. Take the following state:
> ```
> P=Public
> D=Draft
> S=Secret
> 
> Remote(P) <- C1(D) <- C2(D) <- C3(S) <- C4(S)
> ```
> 
> Then the user runs `hg push -r C4`, hg will transfer `C1`, and `C2` to the remote, but not `C3` or `C4`. After which `doreview()` kicks in with `reviewnode == C4` and we'll try to create a review request including the secret `C3` and `C4` changesets.
> 
> This will obviously fail since the server will not have `C3` and `C4`. So, we do need to be adding a guard somewhere in our wrapped push to check for secret changesets? or possibly bail out of review creation if we detect a secret changeset here?

Good catch. I think this is follow-up fodder, since it is an outstanding issue.

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/0e63b4afe01b

Comment 11

[Gregory Szorc [:gps]]

You can now use `hg push -c <rev>` to specify a single changeset for review.

Comment 12

[Gregory Szorc [:gps]]

docs were written as part of the patch. https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html

Comment 13

[Matthew N. [:MattN]]

Weird, I swear I couldn't find "-c" with find-in-page before I added dev-doc-needed.

Comment 15

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: add -c to push to allow single changesets to be reviewed (bug 1097213)