Closed Bug 160112 Opened 19 years ago Closed 18 years ago

Clean up quip table conversion code

Categories

(Bugzilla :: Bugzilla-General, defect)

2.17
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: bbaetz, Assigned: bbaetz)

Details

(Keywords: polish)

Attachments

(1 file, 1 obsolete file)

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...
Attached patch v1 (obsolete) — Splinter Review
> 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

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.
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.
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
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.
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
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.
Attached patch v2Splinter Review
OK, try this, instead
Attachment #93286 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: patch, polish, review
Target Milestone: --- → Bugzilla 2.18
Comment on attachment 95683 [details] [diff] [review]
v2

r=, 2xr by joel
Attachment #95683 - Flags: review+
Checked in
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.