Status

P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gps, Assigned: dminor)

Tracking

Details

(Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/2529] )

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It is common to accidentally upload empty commits/patches to bugzilla or try. I imagine the practice will continue with review board. We should nip it in the bud much like we do merge commits.

Updated

4 years ago
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/2529]
(Reporter)

Comment 1

4 years ago
Bug 1128555 is related.
(In reply to Gregory Szorc [:gps] from comment #0)
> It is common to accidentally upload empty commits/patches to bugzilla or
> try. I imagine the practice will continue with review board. We should nip
> it in the bud much like we do merge commits.

Regarding try, it's very common to push an empty commit that only includes a trychooser command.
(Reporter)

Comment 3

4 years ago
Yeah, Try is special. Not sure why I typed that in the initial comment. Nobody will take empty commits on Try away from you.

Updated

4 years ago
Assignee: nobody → dminor
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 4

4 years ago
Didn't mean to fix this.

Anyway this should be fixed before we get to Review Board, either on the hg server or client.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

4 years ago
Created attachment 8581686 [details]
MozReview Request: bz://1089959/dminor

/r/5869 - hgext: add check for empty changesets to hg client (bug 1089959); r=gps

Pull down this commit:

hg pull review -r 1fb0fafedf42031e0baa65c7da433b9f52b74dcc
Attachment #8581686 - Flags: review?(gps)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8581686 [details]
MozReview Request: bz://1089959/dminor

https://reviewboard.mozilla.org/r/5867/#review4811

Here's a question: should we prevent the client from pushing empty changesets or should we silently not review empty changesets once they get to the server?

I like silently ignoring them. But that does make autoland a bit more difficult. I think the current approach is fine for now. We can always build in more complexity later if this restriction annoys people enough.

::: hgext/reviewboard/client.py
(Diff revision 1)
> +        ctx = repo[rev]
> +        p1 = ctx.p1().node()
> +        diff = ''.join(patch.diff(repo, node1=p1, node2=ctx.node()))
> +        if len(diff) > 0:
> +            all_empty = False

Computing the diff is potentially expensive. And, sometimes andding or removing empty files are valid commits!

I think the check should instead be `if not ctx.files()`. That is "if no files were changed in this commit."

::: hgext/reviewboard/tests/test-push.t
(Diff revision 1)
> -  searching for changes
> +  abort: empty changeset detected. please include actual content next time.
> -  remote: adding changesets
> -  remote: adding manifests
> -  remote: adding file changes
> -  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
> -  submitting 1 changesets for review
> -  abort: reviewboard error: "105 - One or more fields had errors". please try submitting the review again. if that doesn't work, you've likely encountered a bug.

This test tests the presence of a known, unresolved bug. This test should not be removed.

::: hgext/reviewboard/client.py
(Diff revision 1)
> +        raise util.Abort(_('empty changeset detected. please include actual '
> +            'content next time.'))

We should probably print the changeset that is empty.
Attachment #8581686 - Flags: review?(gps)
(Assignee)

Comment 7

4 years ago
https://reviewboard.mozilla.org/r/5867/#review4815

> This test tests the presence of a known, unresolved bug. This test should not be removed.

I added this test when I fixed Bug 1128555 to catch and show reviewboard errors rather than throwing a 500 inside of the mercurial server. The actual reviewboard error was triggered by pushing an empty changeset, so this test needs to be rewritten if we disallow empty changesets on the client, but provides a nice test of the new functionality.

I agree it would be nice to ensure that we don't break catching reviewboard errors inside of the mercurial server, but I'm not aware of any other causes for these errors that I could use inside of a test case.

edit: Nevermind, if I do ctx.files() like you suggest then that will pass through to reviewboard and this test still applies.
(Reporter)

Comment 8

4 years ago
https://reviewboard.mozilla.org/r/5867/#review4843

> I added this test when I fixed Bug 1128555 to catch and show reviewboard errors rather than throwing a 500 inside of the mercurial server. The actual reviewboard error was triggered by pushing an empty changeset, so this test needs to be rewritten if we disallow empty changesets on the client, but provides a nice test of the new functionality.
> 
> I agree it would be nice to ensure that we don't break catching reviewboard errors inside of the mercurial server, but I'm not aware of any other causes for these errors that I could use inside of a test case.
> 
> edit: Nevermind, if I do ctx.files() like you suggest then that will pass through to reviewboard and this test still applies.

Oh, I thought the bug was reviewboard crashes on any empty file, not just a completely empty diff.
(Assignee)

Comment 9

4 years ago
Comment on attachment 8581686 [details]
MozReview Request: bz://1089959/dminor

/r/5869 - hgext: add check for empty changesets to hg client (bug 1089959); r=gps

Pull down this commit:

hg pull review -r 8dc988b5032f7e574f12532fa4f8bc08aae0639b
Attachment #8581686 - Flags: review?(gps)
(Reporter)

Comment 10

4 years ago
Comment on attachment 8581686 [details]
MozReview Request: bz://1089959/dminor

https://reviewboard.mozilla.org/r/5867/#review4981

::: hgext/reviewboard/client.py
(Diff revision 2)
> +from mercurial import patch

You no longer need this import.

::: hgext/reviewboard/client.py
(Diff revision 2)
> +                               ' content.' % hex(rev)))

We should probably use the short 12 char form here. No sense spamming the user with extra characters.
Attachment #8581686 - Flags: review?(gps) → review+
(Assignee)

Comment 11

4 years ago
Thanks for the review, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/16e7e8b523fa
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

3 years ago
Comment on attachment 8581686 [details]
MozReview Request: bz://1089959/dminor
Attachment #8581686 - Attachment is obsolete: true
Attachment #8618490 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8618490 [details]
MozReview Request: hgext: add check for empty changesets to hg client (bug 1089959); r=gps
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.