Re: [PATCH 2/2] git-svn: support fetch with autocrlf on

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Sun, Feb 14, 2010 at 1:46 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 14 Feb 2010, Erik Faye-Lund wrote:
>
>> On Sun, Feb 14, 2010 at 12:59 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > On Sat, 13 Feb 2010, Erik Faye-Lund wrote:
>> >
>> >> I don't think it affects svn dcommit in any way, except from the
>> >> implicit svn rebase that svn dcommit performs. d3c9634e sets
>> >> core.autocrlf to "false" on init, but re-enabling it hasn't shown any
>> >> problems in my end. I'm using git-svn with these patches and
>> >> core.autocrlf enabled every day at my day-job.
>> >
>> > To elicit a warm and fuzzy feeling about your patch, you will have to
>> > analyze the code paaths of dcommit, and how crlf affects them. Then
>> > you will have to describe why dcommit does not have a problem with
>> > crlf with your patches anymore.
>> >
>> > Remember, the idea of a commit message is to optimize the overall time
>> > balance, i.e. avoid the many to perform what the one can do for them.
>> > And since you have to do that analysis for yourself anyway, it makes
>> > sense to write up the result in the commit message.
>> >
>>
>> I'm sorry, but I'm confused. What missed from my commit message?
>
> I had the impression that you sent a mail asking to revert the commit that
> hardcoded autocrlf to false for git svn. For that commit, you would have
> to provide the information I requested.
>

No. I sent a patch series that fixes this issue properly instead of
just disabling core.autocrlf. I added a comment that mentioned that
the hack possibly could be reverted. I didn't ask anyone to revert it.

>> The question of dcommit was a question that Eric asked, and I'm not
>> really sure why he did. I tried to explain why in my reply. d3c9634e
>> never was about dcommit the way I understand it, but about clone:
>> http://code.google.com/p/msysgit/issues/detail?id=232
>
> Well, technically, you are right, it is only about clone.
>
> But.
>
> If you set autocrlf to false in every git svn clone, then of course,
> dcommit is very much affected by the setting. Along with all other git svn
> operations.
>

Wouldn't any such changes be bugs that d3c9634e introduced?

> And since your patches aimed at undoing that patch, i.e. no longer setting
> autocrlf to false upon git svn clone, you have to show that git svn in
> general can handle autocrlf = true (or = input) just fine.
>

My patch aimed at allowing me to use core.autocrlf. I don't care what
the default for core.autocrlf in a git-svn clone is, I'm just sick of
accidentally checking in CRLFs into my company's SVN repo because
enabling core.autocrlf corrupts my local repo.

> And by "to show" I do not mean just test it. That is not good enough,
> because your workflow is more than just likely to miss out on ways other
> people use git svn. You have the source code, and you can look all git
> calls and analyze them for potential autocrlf problems.
>

What makes you think that I haven't properly analyzed the issue? I'm
wondering especially about this, considering that you yourself
described your fix as "so obvious it did not even need testing"...
What's the required amount of analysis here?

I'm sorry if I misunderstand you here, but I'm still not understanding
what's lacking...

I guess "This patch tries to fix the same problem while allowing
core.autocrlf to be enabled, by disabling filters when when hashing"
could be rewritten into something like this:

"This patch tries to fix the same problem while allowing core.autocrlf
to be enabled, by making sure that filters aren't applied while
importing files into the git-repo"

Perhaps I should add a paragraph explaining why the issue triggered in
the first place (before d3c9634e)?

-- 
Erik "kusma" Faye-Lund