Posted by kbahey on October 18, 2008 at 4:19pm
2 followers
| Project: | CVS integration |
| Version: | 6.x-1.x-dev |
| Component: | X-CVS scripts |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
This is a followup from here http://drupal.org/node/322938
Patch attached with better error checking.
Note that it now writes explicitly to the password file so, running this script as blah > CVSROOT/passwd is no longer possible.
In fact, the errors will be displayed to standard output. Hopefully this is captured somewhere useful for review.
Comments
#1
#2
Thanks, that's a good start. A few problems:
A) You need a full path to the passwd file, which is site-specific. That's why it should be a command-line arg (as I mentioned in the original issue), not a PHP define.
B) Why manually come up with the DB error messages instead of just using mysql_error() and mysql_errno() directly for the messages?
C) It doesn't look like file_put_contents() is atomic. I guess given how crappy the current one works, this would already be an improvement, but if we're going to be improving it, why not fix it for real? I'd rather we write the new data into $filename.new and then call
rename("$filename.new", $filename);to atomically replace the file so there's never a moment in time when the file doesn't exist and isn't complete. This would also allow us to test and report errors writing the .new file, too.D) All errors should be written to STDERR, not STDOUT.
Thanks!
-Derek
#3
Check this.
#4
Syntax error. Check this.
#5
Excellent, much better. ;)
Now it just needs to check the return values from file_put_contents() and rename() and report errors + abort if there are any.
Thanks!
#6
Oh, and we can lose the define('PASSWD') code and comment, since that's not used any more.
And, for extra credit, the help text that reports bad usage could mention what the arg is supposed to be. ;)
#7
#8
Thanks for all the rerolls. ;) I'm currently out of town, and will be traveling home tomorrow, so I should have more time to actually test this and deploy it in the next few days. Visual inspection reveals only the most minor of pedantic style gripes, but I'm not going to ask you to reroll just for those. Meanwhile, it wouldn't hurt if anyone else wanted to take a look and/or test this themselves.
#9
Cool.
Two other thoughts:
- Do not forget to change the cron command line/script to NOT do "> CVS/passwd" anymore. We don't use stdout anymore in this script.
- Any reason why we don't bootstrap Drupal and use db_query and friends? Is it because we are on a Drupal-less server?
#10
re: fixing the cron invocation -- yup, that was the plan. since you had forked this issue from #322938: generate-cvs-passwd.php is stupid and can leave CVSROOT/passwd file broken i figured the cron stuff was all that was left for that other issue.
re: bootstrapping Drupal -- right, this runs on cvs.d.o which doesn't have a Drupal checkout. We could use one there for this and for #136866: Bootstrap Drupal so xcvs scripts can use Drupal database functions, but it still hasn't happened and it's not really a priority.
Thanks for being thoughtful,
-Derek
#11
I had a chance to test this out on cvs.d.o. A few minor problems:
A) $argc was used incorrectly (fence-post bug). $argc == 2 when you have a command-line arg, since argv[0] is always set to the name of the script.
B) inside error_message(), $argv needs to be declared global.
C) fixed some pedantic spacing issues in the generated error messages.
After that, I tested all possible failure cases, and each was correctly caught and reported. And, the success case was indeed successful. ;) So, I committed to DRUPAL-5 and HEAD.
Now, let's return to #322938: generate-cvs-passwd.php is stupid and can leave CVSROOT/passwd file broken for the part about fixing how this is invoked via cron on cvs.d.o itself.
#12
Automatically closed -- issue fixed for two weeks with no activity.