On 15.05.2013 04:35, Eric Wong wrote:
>> +    if (!eval{$ctx->ls($parent, 'HEAD', 0)}) {
>> +            mk_parent_dirs($ctx, $parent);
>> +            print "Creating parent folder ${parent} ...\n";
>> +            $ctx->mkdir($parent)
>> +                    unless $_dry_run;
> 
> The newline is confusing, I prefer:
> 
>               $ctx->mkdir($parent) unless $_dry_run;

In fact, this was a copy/paste from a few lines above

        print "Copying ${src} at r${rev} to ${dst}...\n";
        $ctx->copy($src, $rev, $dst)
                unless $_dry_run;

> Howeve :
> 
>       if (!$_dry_run) {
>               $ctx->mkdir($parent);
>       }
> 
> May be preferred, too (especially for the non-Perl-fluent)

I am a non-Perl-fluent, as I come from the Java world with some
knowledge of C and C++. But to be able to create the patch I had to
gather some Perl knowledge, and by doing this, I learned enough to
understand, that there is more than one way, ... Especially the
constructs if (condition) foo(); vs foo() if (condition); and the same
for unless. And since the dry_run is the exception in this case, I think
unless is a valid choice -- and is used often in git-svn.perl. So I will
stick to it, but remove the newline.

> 
>> +++ b/t/t9167-git-svn-cmd-branch-subproject.sh
> 
>> +test_expect_success 'initialize svnrepo' '
>> +    mkdir import &&
>> +    (
>> +        (cd import &&
>> +        mkdir -p trunk/project branches tags &&
>> +        (cd trunk/project &&
>> +        echo foo > foo
>> +        ) &&
> 
> Tabs for all indentation, and indent consistently for subshells:
> 
>       mkdir import &&
>       (
>               cd import &&
>               mkdir .. &&
>               (
>                       cd .. &&
>                       ..
>               )
>       )
> 
> We use subshells + cd like this so it's easier to read/maintain.

Again, this was a copy/paste from t9128-git-svn-cmd-branch.sh. So this
file needs some cosmetics, too. And t9127... as well...

> 
> Thanks again, looking forward to applying v2.
> 

I will send v2 soon.

Tobias

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to