Closed
Bug 1097213
Opened 10 years ago
Closed 10 years ago
|hg push -r tip::tip review| leads to "submitting 2 changesets for review"
Categories
(MozReview Graveyard :: General, defect)
MozReview Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: MattN, Assigned: gps)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
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.
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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 ?
Assignee | ||
Comment 4•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
/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 | ||
Updated•10 years ago
|
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
You can now use `hg push -c <rev>` to specify a single changeset for review.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 12•10 years ago
|
||
docs were written as part of the patch. https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Comment 13•10 years ago
|
||
Weird, I swear I couldn't find "-c" with find-in-page before I added dev-doc-needed.
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8563750 -
Attachment is obsolete: true
Attachment #8618600 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
Updated•9 years ago
|
Product: Developer Services → MozReview
You need to log in
before you can comment on or make changes to this bug.
Description
•