1089959 - Reject empty commits

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Bug 1128555 is related.

Comment 2

[Masatoshi Kimura [:emk]]

(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.

Comment 3

[Gregory Szorc [:gps]]

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

Comment 4

[Mark Côté [:mcote]]

Didn't mean to fix this.

Anyway this should be fixed before we get to Review Board, either on the hg server or client.

Comment 5

[Dan Minor [:dminor]]

Attachment: 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

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Dan Minor [:dminor]]

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.

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Dan Minor [:dminor]]

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

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Dan Minor [:dminor]]

Thanks for the review, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/16e7e8b523fa

Comment 13

[Dan Minor [:dminor]]

Attachment: MozReview Request: hgext: add check for empty changesets to hg client (bug 1089959); r=gps