Closed Bug 1097213 Opened 5 years ago Closed 5 years ago

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

Categories

(MozReview Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MattN, Assigned: gps)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

Attached file 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.
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.
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.
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 ?
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.
(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.
Attached file MozReview Request: bz://1097213/gps (obsolete) —
/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
Attachment #8563750 - Flags: review?(smacleod)
Assignee: nobody → gps
Status: NEW → ASSIGNED
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 on attachment 8563750 [details]
MozReview Request: bz://1097213/gps

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

Fix it, then Ship-It!
Attachment #8563750 - Flags: review?(smacleod) → review+
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.
You can now use `hg push -c <rev>` to specify a single changeset for review.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Weird, I swear I couldn't find "-c" with find-in-page before I added dev-doc-needed.
Attachment #8563750 - Attachment is obsolete: true
Attachment #8618600 - Flags: review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.