[PATCH 1/2] hash-object: support --stdin-paths with --no-filters

[PATCH 1/2] hash-object: support --stdin-paths with --no-filters

[Erik Faye-Lund]

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 builtin-hash-object.c  |    8 ++++----
 t/t1007-hash-object.sh |    4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/builtin-hash-object.c b/builtin-hash-object.c
index 6a5f5b5..080af1a 100644
--- a/builtin-hash-object.c
+++ b/builtin-hash-object.c
@@ -33,6 +33,8 @@ static void hash_object(const char *path, const char *type, int write_object,
 	hash_fd(fd, type, write_object, vpath);
 }
 
+static int no_filters;
+
 static void hash_stdin_paths(const char *type, int write_objects)
 {
 	struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
@@ -44,7 +46,8 @@ static void hash_stdin_paths(const char *type, int write_objects)
 				die("line is badly quoted");
 			strbuf_swap(&buf, &nbuf);
 		}
-		hash_object(buf.buf, type, write_objects, buf.buf);
+		hash_object(buf.buf, type, write_objects,
+		    no_filters ? NULL : buf.buf);
 	}
 	strbuf_release(&buf);
 	strbuf_release(&nbuf);
@@ -60,7 +63,6 @@ static const char *type;
 static int write_object;
 static int hashstdin;
 static int stdin_paths;
-static int no_filters;
 static const char *vpath;
 
 static const struct option hash_object_options[] = {
@@ -100,8 +102,6 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 			errstr = "Can't specify files with --stdin-paths";
 		else if (vpath)
 			errstr = "Can't use --stdin-paths with --path";
-		else if (no_filters)
-			errstr = "Can't use --stdin-paths with --no-filters";
 	}
 	else {
 		if (hashstdin > 1)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index fd98e44..1509fe3 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -65,10 +65,6 @@ test_expect_success "Can't use --path with --stdin-paths" '
 	echo example | test_must_fail git hash-object --stdin-paths --path=foo
 '
 
-test_expect_success "Can't use --stdin-paths with --no-filters" '
-	echo example | test_must_fail git hash-object --stdin-paths --no-filters
-'
-
 test_expect_success "Can't use --path with --no-filters" '
 	test_must_fail git hash-object --no-filters --path=foo
 '
-- 
1.7.0.rc2

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

[Erik Faye-Lund]

If I enable core.autocrlf and perform a "git svn rebase" that fetches
a change containing CRLFs, the git-svn meta-data gets corrupted.

Commit d3c9634e worked around this by setting core.autocrlf to "false"
in the per-repo config when initing the clone. However if the config
variable was changed, the breakage would still occur. This made it
painful to work with git-svn on repos with mostly checked in LFs on
Windows.

This patch tries to fix the same problem while allowing core.autocrlf
to be enabled, by disabling filters when when hashing.

git-svn is currently the only call-site for hash_and_insert_object
(apart from the test-suite), so changing it should be safe.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

With this patch applied, I guess we can also revert d3c9634e. I didn't
do this in this series, because I'm lazy and selfish and thus only
changed the code I needed to get what I wanted to work ;)

I've been running git-svn with these patches with core.autocrlf enabled
since December, and never seen the breakage that I saw before d3c9634e.

 perl/Git.pm |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index e8df55d..2378da4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -842,7 +842,7 @@ sub _open_hash_and_insert_object_if_needed {
 
 	($self->{hash_object_pid}, $self->{hash_object_in},
 	 $self->{hash_object_out}, $self->{hash_object_ctx}) =
-		command_bidi_pipe(qw(hash-object -w --stdin-paths));
+		command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));
 }
 
 sub _close_hash_and_insert_object {
-- 
1.7.0.rc2

Re: [PATCH 1/2] hash-object: support --stdin-paths with --no-filters

[Dmitry Potapov]

Hi Erik,

I don't remember why I made --no-filters incompatible with --stdin-paths
when I added it. So your patch makes perfect sense to me. However, I
think you should correct synopsis in Documentation/git-hash-object.txt,
so it will be clear that these two options can be used together now.
Also, it would be nice if you add a test case to make sure that it works
as expected and will not broken in the future..


Thanks,
Dmitry

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

[Eric Wong]

Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> If I enable core.autocrlf and perform a "git svn rebase" that fetches
> a change containing CRLFs, the git-svn meta-data gets corrupted.
> 
> Commit d3c9634e worked around this by setting core.autocrlf to "false"
> in the per-repo config when initing the clone. However if the config
> variable was changed, the breakage would still occur. This made it
> painful to work with git-svn on repos with mostly checked in LFs on
> Windows.
> 
> This patch tries to fix the same problem while allowing core.autocrlf
> to be enabled, by disabling filters when when hashing.
> 
> git-svn is currently the only call-site for hash_and_insert_object
> (apart from the test-suite), so changing it should be safe.
> 
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> 
> With this patch applied, I guess we can also revert d3c9634e. I didn't
> do this in this series, because I'm lazy and selfish and thus only
> changed the code I needed to get what I wanted to work ;)
> 
> I've been running git-svn with these patches with core.autocrlf enabled
> since December, and never seen the breakage that I saw before d3c9634e.

Hi Erik,

How does reverting d3c9634e affect dcommit?  I've never dealt with (or
even looked at) autocrlf, so I'll put my trust in you and Dscho with
anything related to it.

-- 
Eric Wong

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

[Erik Faye-Lund]

On Sat, Feb 13, 2010 at 1:25 PM, Eric Wong <normalperson@yhbt.net> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> wrote:
>> If I enable core.autocrlf and perform a "git svn rebase" that fetches
>> a change containing CRLFs, the git-svn meta-data gets corrupted.
>>
>> Commit d3c9634e worked around this by setting core.autocrlf to "false"
>> in the per-repo config when initing the clone. However if the config
>> variable was changed, the breakage would still occur. This made it
>> painful to work with git-svn on repos with mostly checked in LFs on
>> Windows.
>>
>> This patch tries to fix the same problem while allowing core.autocrlf
>> to be enabled, by disabling filters when when hashing.
>>
>> git-svn is currently the only call-site for hash_and_insert_object
>> (apart from the test-suite), so changing it should be safe.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>>
>> With this patch applied, I guess we can also revert d3c9634e. I didn't
>> do this in this series, because I'm lazy and selfish and thus only
>> changed the code I needed to get what I wanted to work ;)
>>
>> I've been running git-svn with these patches with core.autocrlf enabled
>> since December, and never seen the breakage that I saw before d3c9634e.
>
> Hi Erik,
>
> How does reverting d3c9634e affect dcommit?  I've never dealt with (or
> even looked at) autocrlf, so I'll put my trust in you and Dscho with
> anything related to it.
>

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.

I'd say that reverting d3c9634e would be the Right Thing To Do(tm),
because it makes git svn clone work just as bad as git clone, when
cloning a repo with CRLFs in it ;)

-- 
Erik "kusma" Faye-Lund

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

[Johannes Schindelin]

Hi,

On Sat, 13 Feb 2010, Eric Wong wrote:

> Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> > If I enable core.autocrlf and perform a "git svn rebase" that fetches
> > a change containing CRLFs, the git-svn meta-data gets corrupted.
> > 
> > Commit d3c9634e worked around this by setting core.autocrlf to "false" 
> > in the per-repo config when initing the clone. However if the config 
> > variable was changed, the breakage would still occur. This made it 
> > painful to work with git-svn on repos with mostly checked in LFs on 
> > Windows.
> > 
> > This patch tries to fix the same problem while allowing core.autocrlf 
> > to be enabled, by disabling filters when when hashing.
> > 
> > git-svn is currently the only call-site for hash_and_insert_object 
> > (apart from the test-suite), so changing it should be safe.
> > 
> > Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> > ---
> > 
> > With this patch applied, I guess we can also revert d3c9634e. I didn't
> > do this in this series, because I'm lazy and selfish and thus only
> > changed the code I needed to get what I wanted to work ;)
> > 
> > I've been running git-svn with these patches with core.autocrlf enabled
> > since December, and never seen the breakage that I saw before d3c9634e.
> 
> Hi Erik,
> 
> How does reverting d3c9634e affect dcommit?  I've never dealt with (or
> even looked at) autocrlf, so I'll put my trust in you and Dscho with
> anything related to it.

I have no idea how reverting said commit affects dcommit, and I do not 
have the time to check, sorry!

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

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.

Thanks,
Dscho

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

[Erik Faye-Lund]

On Sun, Feb 14, 2010 at 12:59 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> 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?

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

If there's something that isn't sufficiently explained in the commit
message, I'd like to know so I can improve it for the next round...

-- 
Erik "kusma" Faye-Lund

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

[Johannes Schindelin]

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.

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

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.

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.

Ciao,
Dscho

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

[Erik Faye-Lund]

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

Re: [PATCH 1/2] hash-object: support --stdin-paths with --no-filters

[Erik Faye-Lund]

On Fri, Feb 12, 2010 at 7:37 PM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> Hi Erik,
>
> I don't remember why I made --no-filters incompatible with --stdin-paths
> when I added it. So your patch makes perfect sense to me. However, I
> think you should correct synopsis in Documentation/git-hash-object.txt,
> so it will be clear that these two options can be used together now.
> Also, it would be nice if you add a test case to make sure that it works
> as expected and will not broken in the future..
>

Thanks. I've fixed the synopsis and added a test for the next round.

-- 
Erik "kusma" Faye-Lund