Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/bug489290_targetdir_rel_pwd

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Aleksey,

> > Jenkins: http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-trunk-kewpie/21/
> > (it may be a bit hard to see if it breaks anything as everything isn't passing everywhere yet)
> >
>
> Um, it's insanity. I suggest we don't use kewpie until we have a clean
> baseline. Can you please rewrite that test case to the old testing
> framework?

That is probably so but it wasn't my initiative to use kewpie for this test.

> Would calling expand_target_dir_name() or cleanup_dirname() or
> unpack_dirname() from get_one_option() be a better place to do the
> conversion to avoid code duplication?

Yes. It will help us to avoid code duplication, but in this case we will do extra work when using xtrabackup with --prepare option. Besides it wouldn't help us if we're not specifying --target-dir option.

> You don't have to do that. With the MY_WME flag my_getwd() will print
> the following message on error:
>
> Can't get working directory (Errcode: %d)

I thought that it is the good practice to start error message with "xtrabackup:".
It could help when doing output parsing.

> Wouldn't cleanup_dirname(xtrabackup_target_dir, xtrabackup_target_dir)
> be sufficient? It just looks like you're duplicating a part of
> cleanup_dirname().

You wouldn't beleive it, but after call to normalize_dirname, cleanup_dirname, etc string "./xtrabackup_backupfiles/" will stay the same. I'm using cleanup_dirname in order to "cleanup" dirname, for example to remove extra "/./", "//", etc.

« Back to merge proposal