160112 - Clean up quip table conversion code

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

The badly-indented error messages are annoying. What we should do is convert
data/comments iff its there and there aren't any comments in the db.

We especially don't want to tell people that we created this backup file and
they need to get rid of it - if we can tell them that we could remove it
ourselves...

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: v1

Comment 2

[Gervase Markham [:gerv]]

> The badly-indented error messages are annoying. 

What's wrong with that indentation style?

> What we should do is convert
> data/comments iff its there and there aren't any comments in the db.

No; some admins may have other ways of adding to data/comments.

If data comments exists, we should append values to the DB (even if it already
contains some comments), and rename the file. I wanted to just delete it, but
the original patch author objected and I didn't feel strongly enough to argue.
If you feel it should be deleted, let's just delete it.

Gerv

Comment 3

[davef]

The indenting style, I take no credit for, as I was happy to continue changing
it to whatever it's supposed to be.

The reason I preferred just renaming (but leaving the file) was to not cause
data loss if someone's upgrading to a temporary database first - my upgrade
process for b.g.o has been load a clean copy of the db, run
checksetup/test/develop, then dump the db and reload. If we were silently nuking
the data/comments file, I'd be pretty annoyed if I don't have a backup. This
gets even worse if you're appending the comments... If you want it changed, rock
on, but that's my reasoning as to why it's a bad idea.

Comment 4

[Bradley Baetz (:bbaetz)]

I didn't mean indenting in the source code, but rather in the output:

The data/comments.bak file can be removed, as it's no longer
           used.

doesn't look very nice.

Its also annoying - it either can be removed, in which case bz should do it, or
its a harmless backup file, in which case we shouldn't touch it.

> No; some admins may have other ways of adding to data/comments.

I don't follow that - it doesn't matter how stuff was in data/comments; if its
there, then we convert it. Or do you mean stuff added after the db was
converted? I don't think we want to care about that.

Comment 5

[Gervase Markham [:gerv]]

OK, I get the indent stuff now :-)

Re: the backup file. Fine. Let's move it to .bak, mention it in the message,
then ignore it for evermore. I'm fine with that.

Gerv

Comment 6

[Dave Miller [:justdave]]

just a kink to throw in the loop here...

Got two landfill installs...  one was created by cloning the other.  Both use
the same database because neither is testing any schema changes.

Since one was created by cloning the other one, they both had the same quips. 
Since the quips got appended, now we have duplicate quips.

Not sure if we care, but thought I'd mention it.

Comment 7

[Gervase Markham [:gerv]]

Also, a fresh install runs the conversion code - data/comments is checked in
with a 0 size. Firstly, we shouldn't be running the code on a 0-sized file, and
secondly, we should CVS-remove it.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

data/comments isn't checked in; checksetup.pl creates it when it creates teh
data dir - my patch removes that along with the other fu.

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: v2

OK, try this, instead

Comment 10

[Joel Peshkin]

Comment on attachment 95683 [details] [diff] [review]
v2

r=, 2xr by joel

Comment 11

[Bradley Baetz (:bbaetz)]

Checked in