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

[Eric Wong <normalperson@yhbt.net>]

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