Mattias Jonsson wrote:
> 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 '&' ?)
Definitely a bug in the mailer. I've been using bleeding edge (alpha
version) of a OSX mailer due to some cool threading capabilities but it
obviously has some other bugs :)
Sorry for the noise.
Regards,
-- Davi
