User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040820 Debian/1.7.2-4 Build Identifier: As discussed in bug 156834, the timestamp SQL type should be replaced by datetime. This bug is against delta_ts column in bugs table. Patch will follow. Reproducible: Always Steps to Reproduce:

>> +# 2004-08-29 - Tomas.Kopal@altap.cz, bug ??? I assume you want to put the bug number in the patch :-)

Ooops :-). Thanks.

Minor change in checksetup.pl to avoid annoying message on every run, even when the type is not actually changed.

Comment on attachment 157558 [details] [diff] [review] V3 of the patch Sorry to take so long to get here. This is now bitrotten and I get several failures when trying to apply to the tip. However they aren't many and I expect they are pretty easy to fix. I reviewed it and it looks good so probably it will be good to go in the next step from my point of view. Since you'll repost it, the identation issues that I noticed are: +if (($fielddef = GetFieldDef("bugs", "delta_ts")) && + $fielddef->[1] =~ /^timestamp/) { +ChangeFieldType ('bugs', 'delta_ts', 'DATETIME NOT NULL'); +} + - . " WHERE bug_id = $bugid"); + . "$diffed WHERE bug_id = $bugid"); - . " WHERE bug_id = $bugid"); + . "$diffed WHERE bug_id = $bugid"); (in fact this sounds like duplication, maybe we could avoid that, of course in another bug if it's complicated)

Actually ignore the last 2 identation issues, it seems they were "broken" in a way and you were trying to fix those :-) The first one still applies though.

Un-bitrotted, checked, polished, glazed, gold plated, simply ready for next review round :-). I have also added updates to two files in the contrib directory, don't know what is the policy - should I try to keep them up-to-date or just ignore? If b) is correct, then just ignore the two parts of the patch :-).

Comment on attachment 171615 [details] [diff] [review] V4 of the patch >RCS file: /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v >-$sql .= "$::userid, now(), "; >+$sql .= "$::userid, now(), now(), "; >+ SendSQL("UPDATE bugs SET delta_ts = NOW()," . I think we should avoid using now() here. $timestamp has already been defined and I would prefer something like (assuming you put the definition of $timestamp higher in the code): $sql .="$::userid, $timestamp, $timestamp"; UPDATE bugs SET delta_ts = $timestamp vladd and I disagree on this point, that the "cleanup" process should be part of another bug. But as you are doing stuff on it, it seems fair to me to do it as well. >RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v At line 1100, we still have: SendSQL("UPDATE bugs SET delta_ts=NOW() WHERE bug_id=$i"); This line is not part of your code, but as you are considering delta_ts, my opinion is that it should also be converted to $timestamp. If justdave says it's ok like this, then remove my r- and open a bug on that.

Well, here is the proposed cleanup incorporated into the patch. The changes are not so intrusive, so I think it's ok to add them here... Feel free to ignore it and check in V4 of you do not agree.

Comment on attachment 171733 [details] [diff] [review] V5 of the patch >Index: importxml.pl >=================================================================== >+ push (@query, "delta_ts"); >+ if ( (defined $bug_fields{'delta_ts'}) && ($bug_fields{'delta_ts'}) ){ >+ push (@values, SqlQuote($bug_fields{$field})); >+ } Sorry, I have to deny review again. ./runtests.pl is complaining about importxml.pl, line 351 (test 001). The reason is that $field is undefined. It should be $bug_fields{'delta_ts'}. Nit: I would also be *very* happy if you could change globals.pl, line 141 from delta_ts = now() to delta_ts = $timestamp as this variable is already defined and used in the insert just above it. :) With these two comments fixed, I'm ok for a r+. Note, also to Vlad: Did you check that all existing "UPDATE bugs" in checksetup.pl are correct? This is the single file which I did not investigate seriously.

(In reply to comment #10) > (From update of attachment 171733 [details] [diff] [review] [edit]) > >Index: importxml.pl > >=================================================================== > >+ push (@query, "delta_ts"); > >+ if ( (defined $bug_fields{'delta_ts'}) && ($bug_fields{'delta_ts'}) ){ > >+ push (@values, SqlQuote($bug_fields{$field})); > >+ } > > > Sorry, I have to deny review again. ./runtests.pl is complaining about > importxml.pl, line 351 (test 001). The reason is that $field is undefined. It > should be $bug_fields{'delta_ts'}. Good one, thanks. Fixed. > > Nit: I would also be *very* happy if you could change globals.pl, line 141 from > delta_ts = now() to delta_ts = $timestamp as this variable is already defined > and used in the insert just above it. :) > > With these two comments fixed, I'm ok for a r+. > If it comes to make other people happy, I am always willing to do my best ;-) - fixed. While verifying that the callers are really passing usable value here, I have also added the timestamp parameter to the call on marking a bug diplicate and confirming a bug by popular vote to make all the DB changes with the same timestamp. I understand it's getting a bit further from the subject of this bug, but it's a minor change and obviously correct. Feel free to disagree and remove it from the patch :-). > > Note, also to Vlad: Did you check that all existing "UPDATE bugs" in > checksetup.pl are correct? This is the single file which I did not investigate > seriously. > In the first versions of the patch, I added the column change to the top of the DB updates in checksetup.pl, and changed all subsequent usage. But that seems to be a bit too intrusive and error prone, so in the recent version, the column change is done as a last change in checksetup.pl and no other queries are modified. This ensures that if someone have older database, it will be updated first (using still timestamp type) and then the type is changed. No changes are needed for new DBs, so it should always work. Does it make sense?

Bugger, wrong file, sorry :-).

Comment on attachment 172158 [details] [diff] [review] V7 of the patch OK, I'm fully satisfied with this patch! :) About checksetup.pl, I have applied your patch, used Bugzilla a bit and nothing bad happened. *If* there is any problem about that, we will open a new bug about checksetup.pl only. r=LpSolit

Comment on attachment 172158 [details] [diff] [review] V7 of the patch I've taken a look at the checksetup.pl part and it looks ok to me.

Checking in CGI.pl; /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v <-- CGI.pl new revision: 1.224; previous revision: 1.223 done Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.65; previous revision: 1.64 done Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.331; previous revision: 1.330 done Checking in editmilestones.cgi; /cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v <-- editmilestones.cgi new revision: 1.27; previous revision: 1.26 done Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.64; previous revision: 1.63 done Checking in editversions.cgi; /cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v <-- editversions.cgi new revision: 1.26; previous revision: 1.25 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.288; previous revision: 1.287 done Checking in importxml.pl; /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v <-- importxml.pl new revision: 1.38; previous revision: 1.37 done Checking in move.pl; /cvsroot/mozilla/webtools/bugzilla/move.pl,v <-- move.pl new revision: 1.29; previous revision: 1.28 done Checking in post_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v <-- post_bug.cgi new revision: 1.97; previous revision: 1.96 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.228; previous revision: 1.227 done Checking in sanitycheck.cgi; /cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v <-- sanitycheck.cgi new revision: 1.74; previous revision: 1.73 done Checking in votes.cgi; /cvsroot/mozilla/webtools/bugzilla/votes.cgi,v <-- votes.cgi new revision: 1.23; previous revision: 1.22 done Checking in Bugzilla/BugMail.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm new revision: 1.23; previous revision: 1.22 done Checking in contrib/bug_email.pl; /cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v <-- bug_email.pl new revision: 1.21; previous revision: 1.20 done Checking in contrib/jb2bz.py; /cvsroot/mozilla/webtools/bugzilla/contrib/jb2bz.py,v <-- jb2bz.py new revision: 1.2; previous revision: 1.1 done

This has been backed out. It critically breaks things in its current form (see bug 280265)

*** Bug 280265 has been marked as a duplicate of this bug. ***

Comment on attachment 172158 [details] [diff] [review] V7 of the patch see previous comment

New version of the fix. Adds quotes and SqlQuote calls as neccesary, does not remove any SqlQuote calls. I still think that we should remove the SqlQuote calls where they are not neccessary, as they are part of the deprecated Bugzilla::DB functionality we want to get rid of, but I agree this shoud be done in a separate bug and not here. Note that this is not complete patch for delta_ts, it needs to be applied on top of the previous one (backed out). I think it will make review much easier than if I merge them.

Is it not easier to change $timestamp to something like: # get current time SendSQL("SELECT NOW()"); my $timestamp = SqlQuote(FetchOneColumn()); or: # get current time SendSQL("SELECT NOW()"); my $timestamp = FetchOneColumn(); my $sql_timestamp = SqlQuote($timestamp); So that you don't need to write '$timestamp' or SqlQuote($timestamp) everywhere?

Comment on attachment 172859 [details] [diff] [review] V3 Fix for the previous patch By inspection, it looks good. You catched all the missing quotes. I missed this '$timestamp' stuff (with quotes) when reviewing previous patches; sorry for that. I need to test it a bit before granting review. But my previous comment still applies: having a sql_timestamp instead of writing '$now' or 'timestamp' may be better, IMO.

(In reply to comment #20) > Is it not easier to change $timestamp to something like: > > # get current time > SendSQL("SELECT NOW()"); > my $timestamp = SqlQuote(FetchOneColumn()); > > or: > # get current time > SendSQL("SELECT NOW()"); > my $timestamp = FetchOneColumn(); > my $sql_timestamp = SqlQuote($timestamp); > > So that you don't need to write '$timestamp' or SqlQuote($timestamp) everywhere? Well, maybe. As I said, I would prefer not to use SqlQuote as it's deprecated function which should go away eventually, and it's not neccessary for values returned directly from DB anyway. But if you insist, I will fix it :-).

Tomas, adding quotes in a single place is better. This prevent us from forgetting them! ;) I discussed that point with mkanat in IRC and he agrees with me.

(In reply to comment #23) > Tomas, adding quotes in a single place is better. This prevent us from > forgetting them! ;) > > I discussed that point with mkanat in IRC and he agrees with me. > Point taken, do you want them as plain quotes, or SqlQuote? (or with fries? ;-) ).

(In reply to comment #24) > Point taken, do you want them as plain quotes, or SqlQuote? Personally, I prefer SqlQuote(). Or maybe Bugzilla->dbh->quote() only?

Simple quotes changed to SqlQuote. Again, applies over the previous patch.

Comment on attachment 172919 [details] [diff] [review] V4 Fix for the previous patch r=LpSolit Now please merge this patch with the previous valid one, so that we only have one patch to check in. But, please, do not change anything so that I can easily forward my r+. ;)

Just a merge of V7 patch and V4 fix, nothing else.

Comment on attachment 173149 [details] [diff] [review] V8, merged and ready to go You know, I hate to do this so late in the review cycle, but I noticed something bad last time the patch was checked-in: The checksetup code is in the *wrong place*. It needs to go above the comment about "if you change the --TABLE-- definition", higher up in the file. Just search for the word "--TABLE--".

(In reply to comment #29) > The checksetup code is in the *wrong place*. It needs to go above the comment > about "if you change the --TABLE-- definition", higher up in the file. Correct! I realized that now. But anyway, a *lot* of code is outside these bounds and maybe having it fixed in another patch would be fine too, so that we can fix them all at once... and have this one finally fixed. Do you agree with my suggestion?

Comment on attachment 173149 [details] [diff] [review] V8, merged and ready to go OK, agreed with Tomas. This looks like a correct merge of the two patches, and I'm willing to certify that.

Comment on attachment 173149 [details] [diff] [review] V8, merged and ready to go I just want to make sure once again that there is no problem with this patch... just to be sure. ;)

(In reply to comment #29) > (From update of attachment 173149 [details] [diff] [review] [edit]) > You know, I hate to do this so late in the review cycle, but I noticed > something bad last time the patch was checked-in: > > The checksetup code is in the *wrong place*. It needs to go above the comment > about "if you change the --TABLE-- definition", higher up in the file. Just > search for the word "--TABLE--". > Well, there probably is some rationale behind this rule, but I fail to see it. I see mostly adding of fields above the mentioned line, but modifications after (see e.g. "# 2004/02/15 - Summaries shouldn't be null - see bug 220232"). Even my previous patch for timestamp type has the change AFTER this line (search for "# 2004-08-29 - Tomas.Kopal@altap.cz, bug 257303"). My reason to put it at the end is that I don't have to modify every subsequent piece of code touching bugs table to treat delta_ts in a different way - if it is old DB which needs upgrading, all upgrades are done first on the old timestamp type, and then the type is converted. If it does not need updating, nothing happens. The only limitation is that all further changes to the bugs table in the checksetup.pl must be placed after this change, i.e. in chronological order (which is what I tought is happening). So, can someone explain me where I am wrong?

(In reply to comment #30) > (In reply to comment #29) > > The checksetup code is in the *wrong place*. It needs to go above the comment > > about "if you change the --TABLE-- definition", higher up in the file. > > Correct! I realized that now. But anyway, a *lot* of code is outside these > bounds and maybe having it fixed in another patch would be fine too, so that we > can fix them all at once... and have this one finally fixed. Do you agree with > my suggestion? > Ok, I think understand now, the other changes below that line are wrong too :-). Then I would agree to fix it in another patch, as we should move all changes as a block, to keep them in the same order as they are now. Otherwise we can cause more trouble than we'll fix...

(In reply to comment #30) > Correct! I realized that now. But anyway, a *lot* of code is outside these > bounds and maybe having it fixed in another patch would be fine too, so that we > can fix them all at once... and have this one finally fixed. Do you agree with > my suggestion? > I just opened bug 280856 about that comment.

Comment on attachment 173149 [details] [diff] [review] V8, merged and ready to go r=LpSolit I have tested this patch again on a fresh installation and no problem has been detected. And the new patch is indeed the merge of the two previous ones, as expected. Thanks Tomas! :)

Minor bitrot on the CGI.pl section due to checkin of bug 276838 just an hour or so ago. The fix was intuitive (remove $::unconfirmedstate, add UNCONFIRMED) so I made the change before checking it in. Checking in CGI.pl; /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v <-- CGI.pl new revision: 1.227; previous revision: 1.226 done Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.67; previous revision: 1.66 done Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.336; previous revision: 1.335 done Checking in editmilestones.cgi; /cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v <-- editmilestones.cgi new revision: 1.29; previous revision: 1.28 done Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.67; previous revision: 1.66 done Checking in editversions.cgi; /cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v <-- editversions.cgi new revision: 1.28; previous revision: 1.27 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.298; previous revision: 1.297 done Checking in importxml.pl; /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v <-- importxml.pl new revision: 1.40; previous revision: 1.39 done Checking in move.pl; /cvsroot/mozilla/webtools/bugzilla/move.pl,v <-- move.pl new revision: 1.31; previous revision: 1.30 done Checking in post_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v <-- post_bug.cgi new revision: 1.100; previous revision: 1.99 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.233; previous revision: 1.232 done Checking in sanitycheck.cgi; /cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v <-- sanitycheck.cgi new revision: 1.77; previous revision: 1.76 done Checking in votes.cgi; /cvsroot/mozilla/webtools/bugzilla/votes.cgi,v <-- votes.cgi new revision: 1.26; previous revision: 1.25 done Checking in Bugzilla/BugMail.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm new revision: 1.30; previous revision: 1.29 done Checking in contrib/bug_email.pl; /cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v <-- bug_email.pl new revision: 1.23; previous revision: 1.22 done Checking in contrib/jb2bz.py; /cvsroot/mozilla/webtools/bugzilla/contrib/jb2bz.py,v <-- jb2bz.py new revision: 1.4; previous revision: 1.3 done