Re: [PATCH] Convert open("-|") to qx{} calls

From: Alex Riesen <raa.lkml@gmail.com>
Date: Feb 23, 2006, 15:38 UTC
Message-ID: <81b0412b0602230738s3445bd86h2d1d670e0ef5daed@mail.gmail.com>
In-Reply-To: <Pine.LNX.4.63.0602231532470.29635@wbgn013.biozentrum.uni-wuerzburg.de>

On 2/23/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

>       Since of these 4, I only use cvsimport myself, I could only test
>       that. Could someone who uses the others give them a hard beating?

I can't really test them (no svn and cvs, and locked down network), but I took a look at the patches. Hope it helps.

git-cvsimport:

> -               open(F,"git-cat-file commit $ftag |");
> -               while(<F>) {
> +               foreach (qx{git-cat-file commit $ftag}) {
>                         next unless /^author\s.*\s(\d+)\s[-+]\d{4}$/;

Are you sure you don't need quoting/safe pipe here? Or is it a CVS tag?

> +} else {
> +    @input = qx{cvsps --norc opt -u -A --root $opt_d $cvs_tree};
> +    !$? or exit $?;

Same here. $cvs_tree can contain any filesystem-allowed character.

git-svnimport:

> -                       my $sha = <$F>;
> +                       my $sha = qx{git-hash-object -w $tmpname};
> +                       !$? or exit $?;

Is $tmpname safe?

> -       my $sha = <$F>;
> +       my $sha = qx{git-hash-object -w $name};
> +       !$? or exit $?;

Is $name safe?

> -       while(<$f>) {
> +       foreach (qx{git-ls-tree -r -z $gitrev $srcpath}) {
>                 chomp;

Is $srcpath safe?

> -                       while(<$F>) {
> +                       foreach (qx{git-ls-files -z @o1}) {

@o1 must contain filenames. Can be dangerous