maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Sergei Golubchik <serg@xxxxxxxxxxxx> writes:

> Just one question and couple of suggestions below.
> If you agree with them - please change accordingly and push!

Thanks for review! I believe I addressed all your suggestions.
Comments below.

 - Kristian.

>> +  $base_dir= dirname($file)
>> +    unless defined($base_dir);
>
> why do you pass base_dir as an argument, instead of always using
> basedir($file) ?

It was a mistake. I was trying to preserve old behaviour, but misunderstood
what that behaviour was. Changed to use basedir($file) always.

>> +      for my $sourced_file ($base_dir . '/' . $include,
>> +                            $::glob_mysql_test_dir . '/' . $include)
>
> Can you also look in the suite dir ? In the order of
>   basedir(file), suite_dir, glob_mysql_test_dir.

Yes, done.

>> +          my ($sub_tags, $sub_master_opts, $sub_slave_opts)=
>> +            get_tags_from_file($sourced_file, $base_dir);
>> +          push @$tags, @$sub_tags;
>> +          # Tests (rpl.rpl_ddl at least) rely on options being in reverse order
>> +          # of include :-/ Hence the unshift().
>> +          unshift @$master_opts, @$sub_master_opts;
>> +          unshift @$slave_opts, @$sub_slave_opts;
>
> I think having options in the order of includes is more logical.
> Better to sort includes in the rpl_ddl.test, and use push here.
> It is more intuitive and less error-prone.

Agree.

It turns out what is really needed is that options from includes are added
before options from main file, so main file can override options from includes
(this is what rpl_ddl needs, include/have_innodb adds --innodb, then
rpl_ddl.test adds --skip-innodb for the slave only).

So I changed it so that options come in order of includes, but first from
included files, last from including file. More logical indeed.

 - Kristian.