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...
> 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.
Created attachment 95683 [details] [diff] [review] v2 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+
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.