Re: [PATCH 1/5] Add a generic tree traversal to fetch SVN properties.

[Benoit SIGOURE <tsuna@lrde.epita.fr>]

[-- Attachment #1: Type: text/plain, Size: 3576 bytes --]

On Oct 16, 2007, at 9:43 AM, Eric Wong wrote:

> Benoit Sigoure <tsuna@lrde.epita.fr> wrote:
>> 	* git-svn.perl (&traverse_ignore): Remove.
>> 	(&prop_walk): New.
>> 	(&cmd_show_ignore): Use prop_walk.
>>
>> Signed-off-by: Benoit Sigoure <tsuna@lrde.epita.fr>
>
> Although I myself have never needed this functionality, this series
> looks pretty good in general.

I heavily script Git with my own wrappers and having this sort if  
functionality does enhance the scriptability of git-svn.

>
> Thanks.

You're welcome :)

>
> One comment below about property selection (whitelist vs blacklist).
>
>
> It would be possible to get identical information out of  
> unhandled.log,
> but older repositories may not have complete information...  Maybe  
> some
> local option would be good for people with complete unhandled.log  
> files;
> but it could be really incomplete/insufficient.
>

In order to avoid using SVN::Ra and avoid access to the SVN repo?   
Hmm, clever, I didn't think about this.  Maybe we can provide both,  
the default would check unhandled.log and an option would enable  
direct access to the SVN repo?

>
> I'm not sure about 5/5, it's purely a style issue, however I don't
> really feel strongly about a trailing "\n" either way...   
> Nevertheless,
> it is definitely not part of this series and should be treated
> independently.
>

Indeed.

>
> Coding style
>
> Other than that, I prefer to keep braces on the same line as foreach,
> if, else statements.  I generally follow the git and Linux coding
> style for C in my Perl code.
>
> One exception that I make for Perl (but not C) is that I keep the "{"
> for subs on the same line (since subs can be nested and anonymous ones
> passed as arguments and such); unlike their C counterparts[1]

Indeed, sorry, I started correctly but then completely forgot to  
follow the existing Coding Style.  The CS I use daily is totally  
different, sorry ;)
Shall I resend the patch series with corrected CS?

>
> [1] - well, nesting functions is allowed in C99 or GNU C, I can't
>       remember which or both...
>

GNU C, AFAIR.

>> ---
>>  git-svn.perl |   66 +++++++++++++++++++++++++++++++++++++++ 
>> +-----------------
>>  1 files changed, 46 insertions(+), 20 deletions(-)
>>
>> diff --git a/git-svn.perl b/git-svn.perl
>> index 777e436..abc83ec 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
[...]
>
> How about having a blacklist (for the author, date, log, uuid?)  
> instead
> of a whitelist?  I can't remember all of them that should be  
> blacklisted,
>  but maybe it's just author, date and log)..
>
>> +	my $interesting_props = 0;
>> +	foreach(keys %{$props})
>> +	{
>> +		# If it doesn't start with `svn:', it must be a
>> +		# user-defined property.
>> +		++$interesting_props and next if $_ !~ /^svn:/;
>> +		# FIXME: Fragile, if SVN adds new public properties,
>> +		# this needs to be updated.
>> +		++$interesting_props if /^svn:(?:ignore|keywords|executable
>> +		                                 |eol-style|mime-type
>> +						 |externals|needs-lock)$/x;
>> +	}

Why not.  I thought that the SVN internals were more subject to  
change than the public "interface", hence the check.

>> +	&$sub($self, $p, $props) if $interesting_props;
>> +

PS: For some reason, the introduction message didn't make its way to  
the ML.  I made a mistake when sending it because I first ran git  
send-email --compose, then noticed that it sent only one mail, and  
ran git send-email *.patch afterwards.  Weird.

Cheers,

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]