Re: [PATCH] Initial manually svn property setting support for git-svn

From: Eric Wong <normalperson@yhbt.net>
Date: Sep 23, 2009, 08:58 UTC
Message-ID: <20090923085812.GA20673@dcvr.yhbt.net>
In-Reply-To: <1927112650.1281253084529659.JavaMail.root@klofta.sjsoft.com>

David Fraser <davidf@sjsoft.com> wrote:

> This basically stores an attribute 'svn-properties' for each file that
> needs them changed, and then sets the properties when committing.

Hi David,

Please wrap your commit messages and emails at 72 columns or less. All git svn code should be wrapped at 80 or less, too.

> Issues remaining:
>  * The way it edits the .gitattributes file is suboptimal - it just
>  appends to the end the latest version

Consider using $GIT_DIR/info/attributes or having an option to use that instead. Keeping a .gitattributes file in the git working tree but _out_ of SVN is important and required, but also difficult to get right.

There are users working on projects that frown upon using unsupported clients like git svn, and accidentally checking .gitattributes into the project would likely annoy non-git users. It's best to keep *.git* stuff outside of SVN projects unless they allow/want it.

But also you should not fail to consider the case that somebody else did intentionally commit .gitattributes into svn and you have little choice but to commit your modifications to .gitattributes. Things like maintaining a mapping between svn:ignore and .gitignore has never happened because of the corner cases that could pop up.

>  * It could use the existing code to get the current svn properties to
>  see if properties need to be changed; but this doesn't work on add

>  * It would be better to cache all the svn properties locally - this
>  could be done automatically in .gitattributes but I'm not sure
>  everyone would want this, etc

It should be possible to infer/rebuild this by parsing unhandled.log files git svn generates by default. There are definitely people who don't want .gitattributes being written to automatically.

>  * No support for deleting properties

What advantage(s) does having this feature in git svn this give over using:

	svn prop(edit|set|del) ARGS $(git svn info --url)

In my experience, explicitly set properties are rarely-used so I need to be convinced it's worth supporting in the future.

> Usage is:
>  git svn propset PROPNAME PROPVALUE PATH
> 
> Added minimal documentation for git-svn propset

We'll also need a test case to ensure it continues working as other changes get made and refactoring gets done.

> +sub check_attr
> +{
> +    my ($attr,$path) = @_;

Please use formatting consistent with the rest of the file. Always use tabs for indent here.

> +    if ( open my $fh, '-|', "git", "check-attr", $attr, "--", $path )

Consider command_output_pipe for better portability/consistency in Git.pm instead of open(my $fh, "-|", @args) which is Perl 5.8+-only.

Thanks for the effort and keep us informed of improvements you make.

-- 
Eric Wong