Re: [PATCH] git-svn: introduce --parents parameter for commands branch and tag

From: Tobias Schulte <tobias.schulte@gliderpilot.de>
Date: May 15, 2013, 19:51 UTC
Message-ID: <5193E74C.6000705@gliderpilot.de>
In-Reply-To: <20130515023547.GA21987@dcvr.yhbt.net>

On 15.05.2013 04:35, Eric Wong wrote:

Show 9 quoted lines

>> +	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;

Show 7 quoted lines

> 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.

Show 25 quoted lines

> 
>> +++ 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