102487 - product-change page appears even if i don't write a comment

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P3)

Description

[Niels Aufbau]

I tried to move bug 101258 to the evangelism product without giving a comment. 
(My previous comment had been "Reopening, will send to evangelism.")  I got a
page asking me to make sure the target milestone and version fields were
correct, and *then* I was told that the change couldn't go through because I
didn't include a comment.

I'm leaving the severity at "normal" because even though this doesn't cause a
lot of problems, it does make Bugzilla look stupid.

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

I think this is the fix.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Taking.

Gerv

Comment 3

[Christian Reis]

Comment on attachment 57690 [details] [diff] [review]
Patch v.1

r=kiko, even though i hate that if statement. maybe add a tiny comment saying
what it does?

Comment 4

[David D. Kilzer (ddk)]

r=ddk, but note violation of coding standard with '{' on a line by
itself. (I can't believe I'm complaining about that because I tend
to code with '{' on a separate line. :^)

Comment 5

[David D. Kilzer (ddk)]

Comment on attachment 57690 [details] [diff] [review]
Patch v.1

But see my previous comment.

Comment 6

[Dave Miller [:justdave]]

checked in.

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.107; previous revision: 1.106

Comment 7

[Dave Miller [:justdave]]

Backed out...  this checking broke the tree.

[Sat Nov 17 13:19:06 2001] process_bug.cgi: main::CheckonComment() called too
early to check prototype at process_bug.cgi line 139.
process_bug.cgi syntax OK
not ok 36 - process_bug.cgi--WARNING
#     Failed test (t/001compile.t at line 69)

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 57690 [details] [diff] [review]
Patch v.1

shame on the reviewers...  this doesn't even pass the tests...	:-)

Comment 9

[David D. Kilzer (ddk)]

Attachment: Patch v.2

- Moved errant '{' from its own line to the previous line.
- Fixed warning about CheckonComment() subroutine by declaring all
  local subroutines at the top of the file before any of them are
  used.

Comment 10

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 11

[Gervase Markham [:gerv]]

> Fixed warning about CheckonComment() subroutine by declaring all
> local subroutines at the top of the file before any of them are
> used.

This seems overkill, and we don't do it anywhere else. Can't you just move the
subroutine in the file?

We can do this for 2.16 - it's not hard.

Gerv

Comment 12

[David D. Kilzer (ddk)]

Yes, the subroutine could be moved to fix the used-before-declaration
warning.  However, am I the only one concerned with code readability?

Interlacing subroutines through a path of code execution (especially
in a CGI script) doesn't enhance readability for me.  Having all the
subroutines in one place in the file, either at the beginning or (my
preference) at the end, makes it easier to read the code IMO.

For Patch v.2, I decided to pre-declare all the subs in this file
since it seemed silly to do only one.  (I thought about moving ALL
the subroutines to the end of the file after doing this, but then the
patch would have gotten much longer.)

Personally, I think pre-declaring local subroutines in each file, then
moving them all to the end of the file should be a part of the code
style guidelines, but I'll abide by whatever the majority decides to
do.

Feel free to post a different patch. :)

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 58248 [details] [diff] [review]
Patch v.2

While I am not opposed to the idea of declaring subroutines, 
I do agree with Gerv when he says that we don't do it anywhere
else in Bugzilla and shouldn't do it in one file and not 
the rest.  Please open a new bug for that.

Also, you were concerned with readability, and so am I:

>+if ((($::FORM{'id'} && $::FORM{'product'} ne $::oldproduct) 
>+     || (!$::FORM{'id'} && $::FORM{'product'} ne $::dontchange))
>+    && CheckonComment( "reassignbycomponent" )) {

Please move the end brace on the multiline if statement to a
new line which will enhance readability and conform to both
Perl and Bugzilla coding guidelines.

Comment 14

[David D. Kilzer (ddk)]

Attachment: Patch v.3

- Moves CheckonComment() subroutine within the file to a location
  before it is used, rather than predeclaring the subroutine.
- Moved the curly brace back to its own line in the patch to the
  if() statement (as in Patch v.1).  Will be filing a documentation
  bug for this since it does violate the current Bugzilla coding
  standards (docs/sgml/patches.sgml), or at least the coding
  standards do not clarify what to do in a situation with a
  multi-line if() statement.

Comment 15

[David D. Kilzer (ddk)]

Bug 107918 talks about using the perl style guide in the perlstyle(1)
manpage.  This includes the following paragraph (covering curly brace
placement):

       Regarding aesthetics of code lay out, about the only thing
       Larry cares strongly about is that the closing curly
       bracket of a multi-line BLOCK should line up with the key­
       word that started the construct.  Beyond that, he has
       other preferences that aren't so strong:

       ·   4-column indent.

       ·   Opening curly on same line as keyword, if possible,
           otherwise line up.

       [...]

Therefore, no need to open a new bug for the disputed curly brace.

Comment 16

[David D. Kilzer (ddk)]

NOTE: Bug 112191 opened for subroutine predeclaration issue raised
by Patch v.2 (attachment 58248 [details] [diff] [review]).

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 59337 [details] [diff] [review]
Patch v.3

r=caillon

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 59337 [details] [diff] [review]
Patch v.3

r=gerv. Looks OK to me.

Gerv

Comment 19

[Dave Miller [:justdave]]

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.111; previous revision: 1.110