From: Eric Wong <normalperson@yhbt.net>
To: Alfred Perlstein <alfred@freebsd.org>
Cc: git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
Junio C Hamano <gitster@pobox.com>,
"Michael G. Schwern" <schwern@pobox.com>,
David Fraser <davidf@sjsoft.com>
Subject: Re: [PATCH] git-svn: Support for git-svn propset
Date: Mon, 1 Dec 2014 09:49:11 +0000 [thread overview]
Message-ID: <20141201094911.GA13931@dcvr.yhbt.net> (raw)
In-Reply-To: <20141201062420.GF99906@elvis.mu.org>
Alfred Perlstein <alfred@freebsd.org> 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 <alfred@freebsd.org>
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).
next prev parent reply other threads:[~2014-12-01 9:49 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-01 6:24 [PATCH] git-svn: Support for git-svn propset Alfred Perlstein
2014-12-01 9:49 ` Eric Wong [this message]
-- strict thread matches above, loose matches on Subject: below --
2014-12-06 22:29 Alfred Perlstein
2014-12-07 5:42 ` Eric Wong
2014-12-07 9:52 ` Alfred Perlstein
2014-12-07 5:45 ` Torsten Bögershausen
2014-12-07 8:00 ` Torsten Bögershausen
2014-12-07 9:23 ` Eric Sunshine
2014-12-07 10:47 Alfred Perlstein
2014-12-08 21:36 ` Eric Wong
2014-12-08 23:43 ` Alfred Perlstein
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141201094911.GA13931@dcvr.yhbt.net \
--to=normalperson@yhbt.net \
--cc=alfred@freebsd.org \
--cc=davidf@sjsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=schwern@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).