Hi,
Thanks for the review, my comments below.
Davi Arnaut wrote:
> Hi Mattias,
>
> Mattias Jonsson wrote:
>> #At file:///Users/mattiasj/clones/bzrroot/b35736-51-bugteam/
>>
>> 2681 Mattias Jonsson 2008-06-30
>> Bug#35736 Test 'parts.partition_basic_symlink_myisam' depends
>> on output of 'ls'
>>
>> 'ls' have portable problems (not exact the same behavior on all
>> systems,
>> does not exits on windows).
>
> Rewrite suggestion: The problem is that relying on the output of the
> 'ls' command is not portable as its behavior is not the same between
> systems and it might even not be available at all in (Windows).
>
OK, I'll use your suggestion.
>> So I added list_files that relies on the portable mysys library
>> instead.
>> (and also list_files_write_file and list_files_append_file,
<snip>
>> mysql-test/r/mysqltest.result
>> Bug#35736 Test 'parts.partition_basic_symlink_myisam' depends on
>> output of 'ls'
>>
>> result file change, due to added test of the new list_files command
>> mysql-test/suite/parts/inc/partition_check_drop.inc
>> Bug#35736 Test 'parts.partition_basic_symlink_myisam' depends on
>> output of 'ls'
>>
>> Using the new list_files instead of 'ls'
>>
>> Changed the use of local variables (ls_file, file_list)
>> and server variable (@aux).
>> mysql-test/suite/parts/inc/partition_layout.inc
>> Bug#35736 Test 'parts.partition_basic_symlink_myisam' depends on
>> output of 'ls'
>
> The bug title is not required in the per-file comments.
>
OK, (there might be some disagreement from Ingo there :)
>> Using the new list_files instead of 'ls'
<snip>
>> mysql-test/suite/parts/r/partition_basic_innodb.result
>> Bug#35736 Test 'parts.partition_basic_symlink_myisam' depends on
>> output of 'ls'
>>
>> Replaces '--exec ls' with list_files.
>> Resulting in removal of directory part of file listing.
>>
>> NOTE: bzr diff is making a bigger diff than neccesary,
>
> neccesary -> necessary. Comment is not really necessary, please drop it.
>
>> only the removal of directory part of file listings are changed!
>
> Suggestion: Removal of the directory part of ....
>
Thanks for finding all my typos!
>> mysql-test/suite/parts/r/partition_basic_myisam.result
>> Bug#35736 Test 'parts.partition_basic_symlink_myisam' depends on
>> output of 'ls'
>>
>> Replaces '--exec ls' with list_files.
>> Resulting in removal of directory part of file listing.
<snip>
>> === modified file 'client/mysqltest.c'
>> --- a/client/mysqltest.c 2008-06-19 14:02:32 +0000
>> +++ b/client/mysqltest.c 2008-06-30 16:49:54 +0000
<snip>
>> @@ -2837,6 +2841,161 @@ void do_rmdir(struct st_command *command
>>
>>
>> /*
>> + SYNOPSIS
>> + get_list_files
>> + ds output
>> + ds_dirname dir to list
>> + ds_wild wild-card file pattern (can be empty)
>> +
>> + DESCRIPTION
>> + list all entries in directory (matching ds_wild if given)
>> +*/
>> +
>> +int get_list_files(DYNAMIC_STRING *ds, const DYNAMIC_STRING *ds_dirname,
>> + const DYNAMIC_STRING *ds_wild)
>
> static int get_list_files
>
This is why it is so good to have reviews!
>> +{
>> + uint i;
>> + MY_DIR *dir_info;
>> + FILEINFO *file;
>> + DBUG_ENTER("get_list_files");
>> +
>> + DBUG_PRINT("info", ("listing directory: %s", ds_dirname->str));
>> + if (!(dir_info= my_dir(ds_dirname->str, MYF(0))))
>> + DBUG_RETURN(1);
>> + for (i=0 ; i< (uint) dir_info->number_off_files ; i++)
Hmm it was not 'i< (uint)' it was 'i < (uint)'...
>
> Please add a comment indicating that my_dir sorts the files :)
>
> Coding style:
>
> for (i= 0; i < (uint) dir_info->number_off_files; i++)
>
OK
>> + {
>> + file= dir_info->dir_entry + i;
>> + if (file->name[0] == '.'&&
>> + (file->name[1] == '\0' ||
>> + (file->name[1] == '.'&& file->name[2] == '\0')))
>> + continue; /* . or .. */
>> + if (ds_wild&& ds_wild->length&&
>
> AFAICS, ds_wild is never null...
>
> Coding style: if (ds_wild && ds_wild->length && ..
>
No currently ds_wild cannot be Null, but if the function gets reused
again... I will leave it there if OK with you.
And the code was already 'if (ds_wild && ds_wild->length &&' !?!?
>> + wild_compare(file->name, ds_wild->str, 0))
>> + continue;
<snip>
>> +
>> +void do_list_files(struct st_command *command)
>
> static void do_list_files
>
>> +{
>> + int error;
>> + static DYNAMIC_STRING ds_dirname;
>> + static DYNAMIC_STRING ds_wild;
>> + const struct command_arg list_files_args[] = {
>> + {"dirname", ARG_STRING, TRUE,&ds_dirname, "Directory to list"},
> ^ space
>
>> + {"file", ARG_STRING, FALSE,&ds_wild, "Filename (incl. wildcard)"}
> ^ space
Hmm, there seem to be some error in the mail, because I have the spaces
in the commit mail in my mailbox... (can it be that your mail client
removes spaces before '&' ?)
>> + };
>> + DBUG_ENTER("do_list_files");
>> +
>> + check_command_args(command, command->first_argument,
>> + list_files_args,
>> + sizeof(list_files_args)/sizeof(struct
>> command_arg), ' ');
>> +
>> + error= get_list_files(&ds_res,&ds_dirname,&ds_wild);
>
> Coding style: get_list_files(&ds_res, &ds_dirname, &ds_wild);
> Same for all ocurrances...
>
Hmm, the spaces are already there...
>> + handle_command_error(command, error);
>> + dynstr_free(&ds_dirname);
>> + dynstr_free(&ds_wild);
>> + DBUG_VOID_RETURN;
>> +}
>> +
>> +
>> +/*
>> + SYNOPSIS
>> + do_list_files_write_file_command
>> + command called command
>> + append append file, or create new
>> +
>> + DESCRIPTION
>> + list_files_write_file<filename> <dir_name> [<match_file>]
>> + List files and directories in directory<dir_name> (like `ls`)
>> + [Matching<match_file>, where wild-cards are allowed]
>> +*/
>> +
>> +void do_list_files_write_file_command(struct st_command *command,
>> + my_bool append)
>
> static void ..
>
Yep!
>> +{
>> + int error;
>> + static DYNAMIC_STRING ds_content;
>> + static DYNAMIC_STRING ds_filename;
>> + static DYNAMIC_STRING ds_dirname;
>> + static DYNAMIC_STRING ds_wild;
>> + const struct command_arg list_files_args[] = {
>> + {"filename", ARG_STRING, TRUE,&ds_filename, "Filename for write"},
>
> Condig style, same as above.
>
Defenitly some mail handling error... both in my version of the commit
mail and in the code there are spaces there...
>> + {"dirname", ARG_STRING, TRUE,&ds_dirname, "Directory to list"},
>> + {"file", ARG_STRING, FALSE,&ds_wild, "Filename (incl. wildcard)"}
>> + };
>> + DBUG_ENTER("do_list_files_write_file");
>> +
>> + check_command_args(command, command->first_argument,
>> + list_files_args,
>> + sizeof(list_files_args)/sizeof(struct
>> command_arg), ' ');
>> +
>> + init_dynamic_string(&ds_content, "", 1024, 1024);
>> + error= get_list_files(&ds_content,&ds_dirname,&ds_wild);
>> + handle_command_error(command, error);
>> + str_to_file2(ds_filename.str, ds_content.str, ds_content.length,
>> append);
>> + dynstr_free(&ds_content);
>> + dynstr_free(&ds_filename);
>> + dynstr_free(&ds_dirname);
>> + dynstr_free(&ds_wild);
>> + DBUG_VOID_RETURN;
>> +}
>> +
>> +
>> +/*
>> + SYNOPSIS
>> + do_list_files_write_file
>> + command called command
>> +
>> + DESCRIPTION
>> + list_files_write_file<filename> <dir_name> [<match_file>]
>> + List files and directories in directory<dir_name> (like `ls`)
>> + [Matching<match_file>, where wild-cards are allowed]
>> + and writes it to filename.
>> +
>> + Note: File will be truncated if exists.
>> +*/
>> +
>> +void do_list_files_write_file(struct st_command *command)
>> +{
>> + do_list_files_write_file_command(command, FALSE);
>> +}
>> +
>> +
>> +/*
>> + SYNOPSIS
>> + do_list_files_append_file
>> + command called command
>> +
>> + DESCRIPTION
>> + list_files_append_file<filename> <dir_name>
> [<match_file>]
>> + List files and directories in directory<dir_name> (like `ls`)
>> + [Matching<match_file>, where wild-cards are allowed]
>> + and appends it to filename.
>> +*/
>> +
>> +void do_list_files_append_file(struct st_command *command)
>> +{
>> + do_list_files_write_file_command(command, TRUE);
>
> Couldn't both functions be just arguments to a unique function? What do
> you think?
>
Yep, that would remove some lines.
>> +}
>> +
>> +
>> +/*
>> Read characters from line buffer or file. This is needed to allow
>> my_ungetc() to buffer MAX_DELIMITER_LENGTH characters for a file
>>
>> @@ -7147,6 +7306,9 @@ int main(int argc, char **argv)
>> case Q_REMOVE_FILE: do_remove_file(command); break;
>> case Q_MKDIR: do_mkdir(command); break;
>> case Q_RMDIR: do_rmdir(command); break;
>> + case Q_LIST_FILES: do_list_files(command); break;
>> + case Q_LIST_FILES_WRITE_FILE:
>> do_list_files_write_file(command); break;
>> + case Q_LIST_FILES_APPEND_FILE:
>> do_list_files_append_file(command); break;
>> case Q_FILE_EXIST: do_file_exist(command); break;
>> case Q_WRITE_FILE: do_write_file(command); break;
>> case Q_APPEND_FILE: do_append_file(command); break;
>>
>> === modified file 'mysql-test/r/mysqltest.result'
>> --- a/mysql-test/r/mysqltest.result 2008-04-29 16:08:52 +0000
>> +++ b/mysql-test/r/mysqltest.result 2008-06-30 16:49:54 +0000
>> @@ -725,6 +725,9 @@ drop table t1;
>> mysqltest: At line 1: change user failed: Unknown database 'inexistent'
>> mysqltest: At line 1: change user failed: Access denied for user
>> 'inexistent'@'localhost' (using password: NO)
>> mysqltest: At line 1: change user failed: Access denied for user
>> 'root'@'localhost' (using password: YES)
>> +file1.txt
>> +file1.txt
>> +file2.txt
>> SELECT 'c:\\a.txt' AS col;
>> col
>> z
>>
>> === modified file 'mysql-test/suite/parts/inc/partition_check_drop.inc'
>> --- a/mysql-test/suite/parts/inc/partition_check_drop.inc
>> 2008-02-29 08:52:50 +0000
>> +++ b/mysql-test/suite/parts/inc/partition_check_drop.inc
>> 2008-06-30 16:49:54 +0000
>> @@ -11,9 +11,9 @@
>>
>> #------------------------------------------------------------------------------#
>
>>
>> # Original Author:
>> mleich #
>> # Original Date:
>> 2006-05-12 #
>> -# Change
>> Author: #
>> -# Change
>> Date: #
>> -#
>> Change:
>> #
>> +# Change Author:
>> mattiasj #
>> +# Change Date:
>> 2008-06-30 #
>> +# Change: replaced '--exec ls' with
>> list_files[_write_file/_append_file] #
>>
>> ################################################################################
>
>>
>
> Please drop this kind of in-file changelog.. we already have file
> history in the branch.
>
OK
>>
>> if ($no_debug)
>> @@ -23,25 +23,26 @@ if ($no_debug)
>>
>> if ($do_file_tests)
<snip>
>> +# Change Author:
>> mattiasj #
>> +# Change Date:
>> 2008-06-30 #
>> +# Change: replaced '--exec ls' with
>> list_files[_write_file/_append_file] #
>>
>> ###############################################################################
>>
>
> Same as above.
>
> Otherwise looks great.. good job!
OK, Thanks a lot, I will update the patch and recommit!
>
> Regards,
>
> -- Davi
--
Mattias Jonsson
MySQL Software Engineer
Sun Microsystems
www.sun.com, www.mysql.com
Phone: +46 70 245 91 51
