Re: cvs: php4 /main main.c

At 10:25 PM 8/27/00 +0200, Sascha Schumann wrote:

On Sun, 27 Aug 2000, Andi Gutmans wrote:

andi          Sun Aug 27 12:51:50 2000 EDT

Modified files:

    /php4/main        main.c

Log: - Reverting Sascha's patch although I don't like doing these kind of things - but as it is Sascha who has reverted a zillion patches I don't feel that - bad about it :) - The patch is morally incorrect and it actually also has a crash bug which - I won't point out because it shouldn't be there.

    Can we please leave this kind of comments out of commit
    messages? First, you are not the moral instance of the church
    of PHP. Second, if you insist on moving this code it would be
    helpful to add where you are seeing a bug.

Relax, don't take it so personally. The bug was that you didn't free the filename in case it had to be freed and you didn't set free_filename in case it had to be set with a copy (i.e. if free_filename was 1 you had to estrdup() filename). Anyway, I thought you said that thttpd gives full path name? What happened that you needed to add this patch there? If this is true (is it? are you looking at the right thttpd variable?) and it really doesn't always give full path name it might make sense to put your patch back (at least until after 4.0.2) to make sure nothing unexpected breaks but I still don't like it. The less magic there is in all of the PHP code the better. Also if you want to put it in then make sure you think about file_handle->free_filename so that your change doesn't crash/leak. This is all assuming that what's being used in the thttpd module is what should be PATH_TRANSLATED and there just is no way to obtain it as full_path name there which sounds weird to me. Don't forget that in a threaded environment that V_GETCWD() call will not give you the correct result. The first time it really is set is during that V_CHDIR_FILE() call (i.e. with help of path_translated). So your path also isn't correct for threaded environments. Andi --- Andi Gutmans <andi@zend.com> http://www.zend.com/