Alfred Perlstein <alfred@xxxxxxxxxxx> wrote: > This change allows git-svn to support setting subversion properties. > > Very useful for manually setting properties when committing to a > subversion repo that *requires* properties to be set without requiring > moving your changeset to separate subversion checkout in order to > set props. Thanks Alfred, that's a good reason for supporting this feature (something I wasn't convinced of with the original). > This change is initially from David Fraser <davidf () sjsoft ! com> > Appearing here: http://marc.info/?l=git&m=125259772625008&w=2 I've added David to the Cc: (but I never heard back from him regarding comments from his original patch). Alfred: I had some comments on David's original patch here that were never addressed: http://mid.gmane.org/20090923085812.GA20673@xxxxxxxxxxxxx I suspect my concerns about .gitattributes in the source tree from 2009 are less relevant now in 2014 as git-svn seems more socially acceptable in SVN-using projects. Some of my other concerns about mimicking existing Perl style still apply, and we have a Perl section in Documenting/CodingGuidelines nowadays. > They are now forward ported to most recent git along with fixes to > deal with files in subdirectories. > > Developer's Certificate of Origin 1.1 <snip> no need for the whole DCO in the commit message. just the S-o-b: > Signed-off-by: Alfred Perlstein <alfred@xxxxxxxxxxx> Since this was originally written by David, his sign-off from the original email should be here (Cc: for bigger changes) > --- > git-svn.perl | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- > perl/Git/SVN/Editor.pm | 47 +++++++++++++++++++++++++++++++++++++++++++++++ We need a test case written for this new feature. Most folks (including myself) who were ever involved with git-svn rarely use it anymore, and this will likely be rarely-used. > 2 files changed, 96 insertions(+), 1 deletion(-) > > diff --git a/git-svn.perl b/git-svn.perl > index b6e2186..91423a8 100755 > --- a/git-svn.perl > +++ b/git-svn.perl > @@ -115,7 +115,7 @@ my ($_stdin, $_help, $_edit, > $_before, $_after, > $_merge, $_strategy, $_preserve_merges, $_dry_run, $_parents, $_local, > $_prefix, $_no_checkout, $_url, $_verbose, > - $_commit_url, $_tag, $_merge_info, $_interactive); > + $_commit_url, $_tag, $_merge_info, $_interactive, $_set_svn_props); > > # This is a refactoring artifact so Git::SVN can get at this git-svn switch. > sub opt_prefix { return $_prefix || '' } > @@ -193,6 +193,7 @@ my %cmd = ( > 'dry-run|n' => \$_dry_run, > 'fetch-all|all' => \$_fetch_all, > 'commit-url=s' => \$_commit_url, > + 'set-svn-props=s' => \$_set_svn_props, > 'revision|r=i' => \$_revision, > 'no-rebase' => \$_no_rebase, > 'mergeinfo=s' => \$_merge_info, > @@ -228,6 +229,9 @@ my %cmd = ( > 'propget' => [ \&cmd_propget, > 'Print the value of a property on a file or directory', > { 'revision|r=i' => \$_revision } ], > + 'propset' => [ \&cmd_propset, > + 'Set the value of a property on a file or directory - will be set on commit', > + {} ], > 'proplist' => [ \&cmd_proplist, > 'List all properties of a file or directory', > { 'revision|r=i' => \$_revision } ], > @@ -1376,6 +1380,50 @@ sub cmd_propget { > print $props->{$prop} . "\n"; > } > > +# cmd_propset (PROPNAME, PROPVAL, PATH) > +# ------------------------ > +# Adjust the SVN property PROPNAME to PROPVAL for PATH. > +sub cmd_propset { > + my ($propname, $propval, $path) = @_; > + $path = '.' if not defined $path; > + $path = $cmd_dir_prefix . $path; > + usage(1) if not defined $propname; > + usage(1) if not defined $propval; > + my $file = basename($path); > + my $dn = dirname($path); > + # diff has check_attr locally, so just call direct > + #my $current_properties = check_attr( "svn-properties", $path ); I prefer leaving out dead code entirely instead of leaving it commented out. > + my $current_properties = Git::SVN::Editor::check_attr( "svn-properties", $path ); > + my $new_properties = ""; Since some lines are too long, local variables can be shortened to cur_props, new_props without being any less descriptive. > + if ($current_properties eq "unset" || $current_properties eq "" || $current_properties eq "set") { > + $new_properties = "$propname=$propval"; > + } else { > + # TODO: handle combining properties better > + my @props = split(/;/, $current_properties); > + my $replaced_prop = 0; > + foreach my $prop (@props) { > + # Parse 'name=value' syntax and set the property. > + if ($prop =~ /([^=]+)=(.*)/) { > + my ($n,$v) = ($1,$2); > + if ($n eq $propname) > + { > + $v = $propval; > + $replaced_prop = 1; > + } > + if ($new_properties eq "") { $new_properties="$n=$v"; } > + else { $new_properties="$new_properties;$n=$v"; } > + } > + } > + if ($replaced_prop eq 0) { Generally, == is favored for numeric comparisons (but this is boolean) > + $new_properties = "$new_properties;$propname=$propval"; > + } > + } > + my $attrfile = "$dn/.gitattributes"; > + open my $attrfh, '>>', $attrfile or die "Can't open $attrfile: $!\n"; > + # TODO: don't simply append here if $file already has svn-properties > + print $attrfh "$file svn-properties=$new_properties\n"; Would be good to have an explicit close and error checking on it in case of FS errors > +} > + > # cmd_proplist (PATH) > # ------------------- > # Print the list of SVN properties for PATH. > diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm > index 34e8af9..5158c03 100644 > --- a/perl/Git/SVN/Editor.pm > +++ b/perl/Git/SVN/Editor.pm > @@ -288,6 +288,49 @@ sub apply_autoprops { > } > } > > +sub check_attr > +{ > + my ($attr,$path) = @_; > + if ( open my $fh, '-|', "git", "check-attr", $attr, "--", $path ) Use Git.pm (command_output_pipe) for running git commands portably. (And formatting for this sub alone is really off). > + { > + my $val = <$fh>; > + close $fh; > + $val =~ s/^[^:]*:\s*[^:]*:\s*(.*)\s*$/$1/; > + return $val; > + } > + else > + { > + return undef; > + } > +} > + > +sub apply_manualprops { > + my ($self, $file, $fbat) = @_; > + my $pending_properties = check_attr( "svn-properties", $file ); > + if ($pending_properties eq "") { return; } > + # Parse the list of properties to set. > + my @props = split(/;/, $pending_properties); > + # TODO: get existing properties to compare to - this fails for add so currently not done > + # my $existing_props = ::get_svnprops($file); > + my $existing_props = {}; > + # TODO: caching svn properties or storing them in .gitattributes would make that faster > + foreach my $prop (@props) { > + # Parse 'name=value' syntax and set the property. > + if ($prop =~ /([^=]+)=(.*)/) { > + my ($n,$v) = ($1,$2); > + for ($n, $v) { > + s/^\s+//; s/\s+$//; > + } I'd probably rewrite the following hunk: > + # FIXME: clearly I don't know perl and couldn't work out how to evaluate this better > + if (defined $existing_props->{$n} && $existing_props->{$n} eq $v) { > + my $needed = 0; > + } else { > + $self->change_file_prop($fbat, $n, $v); > + } as: my $existing = $existing_props->{$n}; if (!defined($existing) || $existing ne $v) { $self->change_file_prop($fbat, $n, $v); } No need for setting $needed = 0 from what I can tell. Rest of the patch looks fine. Thank you again for bringing this up and I look forward to a reroll of this with my comments addressed (maybe wait a few days for others to comment, holidays and all). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html