Re: [PATCH] git-svn: Support for git-svn propset

Alfred Perlstein [off-list ref] 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@dcvr.yhbt.net

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

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.

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

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