373492 - Data in data/mining/* is corrupted

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Frédéric Buclin]

There is a bug in collectstats.pl due to bug 94534 which corrupts all data in data/mining/* files.

When collectstats.pl checks whether it has to recreate files or not, it calls diff_arrays(\@columns, \@new_cols) where @columns is the current list of statuses and resolutions, and @new_cols contains the new list (as you can now custom resolutions). The idea is to check if you had e.g. FIXED|INVALID|WONTFIX and now you have INVALID|WONTFIX|FIXED, in which case you would have to shift columns to match the new order. The problem is that diff_arrays() is insensitive to the order of elements in @columns and @new_cols and so collectstats.pl doesn't see that columns may now be in a different order and will happily appends new stats to data/mining/* files without realizing it puts data out of order (or to be more exact: in an order which doesn't match the old hardcoded one).

2.23.3, 2.23.4 and 3.0 RC1 are all affected. 2.23.2 and older are not.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Any other reviewer can review it too.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 258149 [details] [diff] [review]
patch, v1

>+                for ($[ .. scalar(@columns) -1 + $[) {

  Whoa! What is that crazy syntax? Nobody will have any idea what that means. *I* have no idea what it means. :-) Can we just use a normal for loop?

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
>+                for ($[ .. scalar(@columns) -1 + $[) {

$[ is the index of the first element of arrays (is 0 by default, but could be 1 as well). Said otherwise, I wrote: for (0 .. scalar(@columns) -1). Is that clearer?

Comment 4

[Frédéric Buclin]

Attachment: patch, v1.1

Comment 5

[Gervase Markham [:gerv]]

> $[ is the index of the first element of arrays (is 0 by default, but could be 1
> as well).

I think if the Bugzilla development team had a collective brain seizure and decided to switch to 1-based arrays, this would be only one of hundreds of places where the code would need updating :-) So I wouldn't worry about that possibility.

r=gerv, but I'll leave the request in case mkanat wants to look at it again.

Gerv

Comment 6

[Max Kanat-Alexander]

Comment on attachment 258231 [details] [diff] [review]
patch, v1.1

Okay, looks good to me.

Comment 7

[Frédéric Buclin]

tip:

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.59; previous revision: 1.58
done

3.0 RC1:

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.58.2.1; previous revision: 1.58
done

Comment 8

[Max Kanat-Alexander]

Added to relnotes in bug 379777.