git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] git-svn: Fix termination issues for remote svn connections
@ 2013-09-03  7:35 Uli Heller
  2013-09-05 18:48 ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Uli Heller @ 2013-09-03  7:35 UTC (permalink / raw)
  To: git

When using git-svn in combination with serf-1.2.1 core dumps are
created on termination. This is caused by a bug in serf, a fix for
the bug exists (see https://code.google.com/p/serf/source/detail?r=2146).
Nevertheless, I think it makes sense to fix the issue within the
git perl module Ra.pm, too. The change frees the private copy of
the remote access object on termination which prevents the error
from happening.

Note: Since subversion-1.8.0 and later do require serf-1.2.1 or later,
the core dumps typically do show up when upgrading to a recent version
of subversion.

Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
pointing me to that fix.
---
 perl/Git/SVN/Ra.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 75ecc42..78dd346 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -32,6 +32,11 @@ BEGIN {
 	}
 }

+END {
+	$RA = undef;
+	$ra_invalid = 1;
+}
+
 sub _auth_providers () {
 	my @rv = (
 	  SVN::Client::get_simple_provider(),
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn connections
  2013-09-03  7:35 [PATCH] git-svn: Fix termination issues for remote svn connections Uli Heller
@ 2013-09-05 18:48 ` Junio C Hamano
  2013-09-05 19:02   ` Eric Wong
                     ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Junio C Hamano @ 2013-09-05 18:48 UTC (permalink / raw)
  To: Uli Heller; +Cc: git, Kyle J. McKay, Eric Wong

"Uli Heller" <uli.heller@daemons-point.com> writes:

> When using git-svn in combination with serf-1.2.1 core dumps are
> created on termination. This is caused by a bug in serf, a fix for
> the bug exists (see https://code.google.com/p/serf/source/detail?r=2146).
> Nevertheless, I think it makes sense to fix the issue within the
> git perl module Ra.pm, too. The change frees the private copy of
> the remote access object on termination which prevents the error
> from happening.
>
> Note: Since subversion-1.8.0 and later do require serf-1.2.1 or later,
> the core dumps typically do show up when upgrading to a recent version
> of subversion.
>
> Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
> Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
> pointing me to that fix.
> ---

Thanks.  Please sign-off your patch.

I am Cc'ing Kyle McKay who apparently had some experience working
with git-svn with newer svn that can only use serf, hoping that we
can get an independent opinion/test just to be sure.  Also Cc'ed is
Eric Wong who has been the official git-svn area expert, but I
understand that Eric hasn't needed to use git-svn for quite a while,
so it is perfectly fine if he does not have any comment on this one.

We may want to find a volunteer to move "git svn" forward as a new
area expert (aka subsystem maintainer), by the way.



>  perl/Git/SVN/Ra.pm | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
> index 75ecc42..78dd346 100644
> --- a/perl/Git/SVN/Ra.pm
> +++ b/perl/Git/SVN/Ra.pm
> @@ -32,6 +32,11 @@ BEGIN {
>  	}
>  }
>
> +END {
> +	$RA = undef;
> +	$ra_invalid = 1;
> +}
> +
>  sub _auth_providers () {
>  	my @rv = (
>  	  SVN::Client::get_simple_provider(),

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn connections
  2013-09-05 18:48 ` Junio C Hamano
@ 2013-09-05 19:02   ` Eric Wong
  2013-09-05 23:14     ` Junio C Hamano
  2013-09-06 11:46   ` Kyle J. McKay
  2013-09-06 12:44   ` [PATCH] " Kyle J. McKay
  2 siblings, 1 reply; 12+ messages in thread
From: Eric Wong @ 2013-09-05 19:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Uli Heller, git, Kyle J. McKay

Junio C Hamano <gitster@pobox.com> wrote:
> "Uli Heller" <uli.heller@daemons-point.com> writes:
> > Nevertheless, I think it makes sense to fix the issue within the
> > git perl module Ra.pm, too. The change frees the private copy of
> > the remote access object on termination which prevents the error
> > from happening.

> Thanks.  Please sign-off your patch.
> 
> I am Cc'ing Kyle McKay who apparently had some experience working
> with git-svn with newer svn that can only use serf, hoping that we
> can get an independent opinion/test just to be sure.  Also Cc'ed is
> Eric Wong who has been the official git-svn area expert, but I
> understand that Eric hasn't needed to use git-svn for quite a while,
> so it is perfectly fine if he does not have any comment on this one.
> 
> We may want to find a volunteer to move "git svn" forward as a new
> area expert (aka subsystem maintainer), by the way.

Correct, git-svn has the effect of being self-obsoleting.

I agree with adding a workaround for broken things, however
I suggest a code comment explaining why it is necessary.
The commit message is important, too, but might get harder to track
down if there's code movement/refactoring in the future.

> > +END {
> > +	$RA = undef;
> > +	$ra_invalid = 1;
> > +}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn connections
  2013-09-05 19:02   ` Eric Wong
@ 2013-09-05 23:14     ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2013-09-05 23:14 UTC (permalink / raw)
  To: Eric Wong; +Cc: Uli Heller, git, Kyle J. McKay

Eric Wong <normalperson@yhbt.net> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> "Uli Heller" <uli.heller@daemons-point.com> writes:
>> > Nevertheless, I think it makes sense to fix the issue within the
>> > git perl module Ra.pm, too. The change frees the private copy of
>> > the remote access object on termination which prevents the error
>> > from happening.
>
>> Thanks.  Please sign-off your patch.
>> 
>> I am Cc'ing Kyle McKay who apparently had some experience working
>> with git-svn with newer svn that can only use serf, hoping that we
>> can get an independent opinion/test just to be sure.  Also Cc'ed is
>> Eric Wong who has been the official git-svn area expert, but I
>> understand that Eric hasn't needed to use git-svn for quite a while,
>> so it is perfectly fine if he does not have any comment on this one.
>> 
>> We may want to find a volunteer to move "git svn" forward as a new
>> area expert (aka subsystem maintainer), by the way.
>
> Correct, git-svn has the effect of being self-obsoleting.
>
> I agree with adding a workaround for broken things, however
> I suggest a code comment explaining why it is necessary.
> The commit message is important, too, but might get harder to track
> down if there's code movement/refactoring in the future.

Thanks for a good suggestion.  I agree that this addition is a good
example where in-code comment would really help the future readers.

>
>> > +END {
>> > +	$RA = undef;
>> > +	$ra_invalid = 1;
>> > +}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn connections
  2013-09-05 18:48 ` Junio C Hamano
  2013-09-05 19:02   ` Eric Wong
@ 2013-09-06 11:46   ` Kyle J. McKay
  2013-09-06 12:06     ` Uli Heller
  2013-09-06 12:44   ` [PATCH] " Kyle J. McKay
  2 siblings, 1 reply; 12+ messages in thread
From: Kyle J. McKay @ 2013-09-06 11:46 UTC (permalink / raw)
  To: Junio C Hamano, Uli Heller; +Cc: Git Mailing List, Eric Wong

On Sep 5, 2013, at 11:48, Junio C Hamano wrote:
> "Uli Heller" <uli.heller@daemons-point.com> writes:
>
>> When using git-svn in combination with serf-1.2.1 core dumps are
>> created on termination. This is caused by a bug in serf, a fix for
>> the bug exists (see https://code.google.com/p/serf/source/detail?r=2146) 
>> .
>> Nevertheless, I think it makes sense to fix the issue within the
>> git perl module Ra.pm, too. The change frees the private copy of
>> the remote access object on termination which prevents the error
>> from happening.
>>
>> Note: Since subversion-1.8.0 and later do require serf-1.2.1 or  
>> later,
>> the core dumps typically do show up when upgrading to a recent  
>> version
>> of subversion.
>>
>> Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
>> Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
>> pointing me to that fix.
>> ---
>
> Thanks.  Please sign-off your patch.
>
> I am Cc'ing Kyle McKay who apparently had some experience working
> with git-svn with newer svn that can only use serf, hoping that we
> can get an independent opinion/test just to be sure.  Also Cc'ed is
> Eric Wong who has been the official git-svn area expert, but I
> understand that Eric hasn't needed to use git-svn for quite a while,
> so it is perfectly fine if he does not have any comment on this one.
>
> We may want to find a volunteer to move "git svn" forward as a new
> area expert (aka subsystem maintainer), by the way.
>
>> perl/Git/SVN/Ra.pm | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
>> index 75ecc42..78dd346 100644
>> --- a/perl/Git/SVN/Ra.pm
>> +++ b/perl/Git/SVN/Ra.pm
>> @@ -32,6 +32,11 @@ BEGIN {
>> 	}
>> }
>>
>> +END {
>> +	$RA = undef;
>> +	$ra_invalid = 1;
>> +}
>> +
>> sub _auth_providers () {
>> 	my @rv = (
>> 	  SVN::Client::get_simple_provider(),

I have not, as of yet, been able to reproduce the problem, so I cannot  
verify the solution.  Maybe Uli can provide an example of a git-svn  
command that demonstrates the failure?

I am running a fresh build of subversion 1.8.3 with serf version 1.3.1  
(the most recent serf release).

According to the serf library history, version 1.3.1 of serf was  
tagged at revision 2139 from revision 2138, but the serf fix mentioned  
above was checked in at revision 2146, so it cannot possibly be in the  
serf 1.3.1 release.

I'm using Git built from master (57e4c1783).  I see the same behavior  
both with and without the SVN/Ra.pm patch (and using both bulk updates  
and skelta mode).  Does the problem not happen on a git svn clone?  I  
can force serf back to version 1.2.1 and try that version just to see,  
but I would like to have an example of a known failing git svn command  
for testing purposes.  Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn  connections
  2013-09-06 11:46   ` Kyle J. McKay
@ 2013-09-06 12:06     ` Uli Heller
  2013-09-06 12:44       ` Kyle J. McKay
  0 siblings, 1 reply; 12+ messages in thread
From: Uli Heller @ 2013-09-06 12:06 UTC (permalink / raw)
  To: Kyle J. McKay; +Cc: Junio C Hamano, Git Mailing List, Eric Wong

On Fri, September 6, 2013 1:46 pm, Kyle J. McKay wrote:
> On Sep 5, 2013, at 11:48, Junio C Hamano wrote:
>> "Uli Heller" <uli.heller@daemons-point.com> writes:
>>
>>> When using git-svn in combination with serf-1.2.1 core dumps are
>>> created on termination. This is caused by a bug in serf, a fix for
>>> the bug exists (see
>>> https://code.google.com/p/serf/source/detail?r=2146)
>>> .
>>> Nevertheless, I think it makes sense to fix the issue within the
>>> git perl module Ra.pm, too. The change frees the private copy of
>>> the remote access object on termination which prevents the error
>>> from happening.
>>>
>>> Note: Since subversion-1.8.0 and later do require serf-1.2.1 or
>>> later,
>>> the core dumps typically do show up when upgrading to a recent
>>> version
>>> of subversion.
>>>
>>> Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
>>> Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
>>> pointing me to that fix.
>>> ---
>>
>> Thanks.  Please sign-off your patch.
>>
>> I am Cc'ing Kyle McKay who apparently had some experience working
>> with git-svn with newer svn that can only use serf, hoping that we
>> can get an independent opinion/test just to be sure.  Also Cc'ed is
>> Eric Wong who has been the official git-svn area expert, but I
>> understand that Eric hasn't needed to use git-svn for quite a while,
>> so it is perfectly fine if he does not have any comment on this one.
>>
>> We may want to find a volunteer to move "git svn" forward as a new
>> area expert (aka subsystem maintainer), by the way.
>>
>>> perl/Git/SVN/Ra.pm | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
>>> index 75ecc42..78dd346 100644
>>> --- a/perl/Git/SVN/Ra.pm
>>> +++ b/perl/Git/SVN/Ra.pm
>>> @@ -32,6 +32,11 @@ BEGIN {
>>> 	}
>>> }
>>>
>>> +END {
>>> +	$RA = undef;
>>> +	$ra_invalid = 1;
>>> +}
>>> +
>>> sub _auth_providers () {
>>> 	my @rv = (
>>> 	  SVN::Client::get_simple_provider(),
>
> I have not, as of yet, been able to reproduce the problem, so I cannot
> verify the solution.  Maybe Uli can provide an example of a git-svn
> command that demonstrates the failure?
>
> I am running a fresh build of subversion 1.8.3 with serf version 1.3.1
> (the most recent serf release).
>
> According to the serf library history, version 1.3.1 of serf was
> tagged at revision 2139 from revision 2138, but the serf fix mentioned
> above was checked in at revision 2146, so it cannot possibly be in the
> serf 1.3.1 release.
>
> I'm using Git built from master (57e4c1783).  I see the same behavior
> both with and without the SVN/Ra.pm patch (and using both bulk updates
> and skelta mode).  Does the problem not happen on a git svn clone?  I
> can force serf back to version 1.2.1 and try that version just to see,
> but I would like to have an example of a known failing git svn command
> for testing purposes.  Thanks.

I think this command should produce the error:

  git svn clone --stdlayout https://github.com/uli-heller/uli-javawrapper

You can use any other svn repo as well, you only have to specify an HTTPS
url.

[Yes, I know you typically don't clone github via git svn]

I'll do some more tests using git HEAD and serf 1.3.1 when I'm back home.

Thanks + best regards, Uli

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn  connections
  2013-09-06 12:06     ` Uli Heller
@ 2013-09-06 12:44       ` Kyle J. McKay
  2013-09-06 13:18         ` Uli Heller
  0 siblings, 1 reply; 12+ messages in thread
From: Kyle J. McKay @ 2013-09-06 12:44 UTC (permalink / raw)
  To: Uli Heller; +Cc: Git Mailing List, Eric Wong, Junio C Hamano

On Sep 6, 2013, at 05:06, Uli Heller wrote:
>> I'm using Git built from master (57e4c1783).  I see the same behavior
>> both with and without the SVN/Ra.pm patch (and using both bulk  
>> updates
>> and skelta mode).  Does the problem not happen on a git svn clone?  I
>> can force serf back to version 1.2.1 and try that version just to  
>> see,
>> but I would like to have an example of a known failing git svn  
>> command
>> for testing purposes.  Thanks.
>
> I think this command should produce the error:
>
>  git svn clone --stdlayout https://github.com/uli-heller/uli-javawrapper
>
> You can use any other svn repo as well, you only have to specify an  
> HTTPS
> url.

Yes, that does it.  Interesting that cloning from "https://github.com/uli-heller/uli-javawrapper 
" core dumps while cloning from "http://github.com/uli-heller/uli-javawrapper 
" does not even though the latter redirects to "https://github.com/uli-heller/uli-javawrapper 
".

In any case, I can now reproduce the problem (serf 1.3.1 still breaks  
since it does not yet contain the fix and it is the most recent serf  
release available).

And the Git/SVN/Ra.pm fix does eliminate the problem for me (both with  
bulk updates and with skelta updates -- the crash occurs with either).

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn connections
  2013-09-05 18:48 ` Junio C Hamano
  2013-09-05 19:02   ` Eric Wong
  2013-09-06 11:46   ` Kyle J. McKay
@ 2013-09-06 12:44   ` Kyle J. McKay
  2 siblings, 0 replies; 12+ messages in thread
From: Kyle J. McKay @ 2013-09-06 12:44 UTC (permalink / raw)
  To: Junio C Hamano, Uli Heller; +Cc: Git Mailing List, Eric Wong

On Sep 5, 2013, at 11:48, Junio C Hamano wrote:
> I am Cc'ing Kyle McKay who apparently had some experience working
> with git-svn with newer svn that can only use serf, hoping that we
> can get an independent opinion/test just to be sure.

On Sep 3, 2013, at 00:35, Uli Heller wrote:
> When using git-svn in combination with serf-1.2.1 core dumps are
> created on termination. This is caused by a bug in serf, a fix for
> the bug exists (see https://code.google.com/p/serf/source/detail?r=2146) 
> .
> Nevertheless, I think it makes sense to fix the issue within the
> git perl module Ra.pm, too. The change frees the private copy of
> the remote access object on termination which prevents the error
> from happening.
>
> Note: Since subversion-1.8.0 and later do require serf-1.2.1 or later,
> the core dumps typically do show up when upgrading to a recent version
> of subversion.
>
> Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
> Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
> pointing me to that fix.
> ---

In addition to the needed 'Signed-off-by:' a mention should be made  
here of the need to use 'https:' to reproduce the problem.

> perl/Git/SVN/Ra.pm | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
> index 75ecc42..78dd346 100644
> --- a/perl/Git/SVN/Ra.pm
> +++ b/perl/Git/SVN/Ra.pm
> @@ -32,6 +32,11 @@ BEGIN {
> 	}
> }
>
> +END {
> +	$RA = undef;
> +	$ra_invalid = 1;
> +}
> +
> sub _auth_providers () {
> 	my @rv = (
> 	  SVN::Client::get_simple_provider(),
> -- 
> 1.8.4

Tested-by: Kyle J. McKay <mackyle@gmail.com>

I was able to reproduce the core dumps using subversion 1.8.3 (the  
most recent subversion release) with serf 1.3.1 (the most recent serf  
release) and verify that this patch eliminates the core dump at git- 
svn termination time.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn   connections
  2013-09-06 12:44       ` Kyle J. McKay
@ 2013-09-06 13:18         ` Uli Heller
  2013-09-06 16:41           ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Uli Heller @ 2013-09-06 13:18 UTC (permalink / raw)
  To: Kyle J. McKay; +Cc: Git Mailing List, Eric Wong, Junio C Hamano

On Fri, September 6, 2013 2:44 pm, Kyle J. McKay wrote:
> On Sep 6, 2013, at 05:06, Uli Heller wrote:
>>> I'm using Git built from master (57e4c1783).  I see the same behavior
>>> both with and without the SVN/Ra.pm patch (and using both bulk
>>> updates
>>> and skelta mode).  Does the problem not happen on a git svn clone?  I
>>> can force serf back to version 1.2.1 and try that version just to
>>> see,
>>> but I would like to have an example of a known failing git svn
>>> command
>>> for testing purposes.  Thanks.
>>
>> I think this command should produce the error:
>>
>>  git svn clone --stdlayout https://github.com/uli-heller/uli-javawrapper
>>
>> You can use any other svn repo as well, you only have to specify an
>> HTTPS
>> url.
>
> Yes, that does it.  Interesting that cloning from
> "https://github.com/uli-heller/uli-javawrapper
> " core dumps while cloning from
> "http://github.com/uli-heller/uli-javawrapper
> " does not even though the latter redirects to
> "https://github.com/uli-heller/uli-javawrapper
> ".
>
> In any case, I can now reproduce the problem (serf 1.3.1 still breaks
> since it does not yet contain the fix and it is the most recent serf
> release available).
>
> And the Git/SVN/Ra.pm fix does eliminate the problem for me (both with
> bulk updates and with skelta updates -- the crash occurs with either).

Great. So I don't have to run any more tests ;)

What shall I do next? Add some inline comments to the patch?

---
Best regards, Uli.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] git-svn: Fix termination issues for remote svn   connections
  2013-09-06 13:18         ` Uli Heller
@ 2013-09-06 16:41           ` Junio C Hamano
  2013-09-09  6:01             ` [PATCH v2] " Uli Heller
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-09-06 16:41 UTC (permalink / raw)
  To: Uli Heller; +Cc: Kyle J. McKay, Git Mailing List, Eric Wong

"Uli Heller" <uli.heller@daemons-point.com> writes:

> On Fri, September 6, 2013 2:44 pm, Kyle J. McKay wrote:
> ...
>> In any case, I can now reproduce the problem (serf 1.3.1 still breaks
>> since it does not yet contain the fix and it is the most recent serf
>> release available).
>>
>> And the Git/SVN/Ra.pm fix does eliminate the problem for me (both with
>> bulk updates and with skelta updates -- the crash occurs with either).
>
> Great. So I don't have to run any more tests ;)
>
> What shall I do next? Add some inline comments to the patch?

Something like this?

-- >8 --

From: Uli Heller <uli.heller@daemons-point.com>
Subject: [PATCH] git-svn: fix termination issues for remote svn connections

git-svn used in combination with serf to talk to svn repository
served over HTTPS dumps core on termination.

This is caused by a bug in serf, and the most recent serf release
1.3.1 still exhibits the problem; a fix for the bug exists (see
https://code.google.com/p/serf/source/detail?r=2146).

Until the bug is fixed, work around the issue within the git perl
module Ra.pm by freeing the private copy of the remote access object
on termination, which seems to be sufficient to prevent the error
from happening.

Note: Since subversion-1.8.0 and later do require serf-1.2.1 or
later, this issue typically shows up when upgrading to a recent
version of subversion.

Credits go to Jonathan Lambrechts for proposing a fix to Ra.pm,
Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
pointing me to that fix.

Signed-off-by: Uli Heller <uli.heller@daemons-point.com>
Tested-by: Kyle J. McKay <mackyle@gmail.com>
---
 perl/Git/SVN/Ra.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 75ecc42..78dd346 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -32,6 +32,14 @@ BEGIN {
 	}
 }
 
+# serf has a bug that leads to a coredump upon termination if the
+# remote access object is left around (not fixed yet in serf 1.3.1).
+# Explicitly free it to work around the issue.
+END {
+	$RA = undef;
+	$ra_invalid = 1;
+}
+
 sub _auth_providers () {
 	my @rv = (
 	  SVN::Client::get_simple_provider(),
-- 
1.8.4-431-g94f3a6b

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2] git-svn: Fix termination issues for remote svn   connections
  2013-09-06 16:41           ` Junio C Hamano
@ 2013-09-09  6:01             ` Uli Heller
  2013-09-09 15:42               ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Uli Heller @ 2013-09-09  6:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Kyle J. McKay, Git Mailing List, Eric Wong

 From a763550fc59b756580f9b162839d2737c27f2fe3 Mon Sep 17 00:00:00 2001
From: Uli Heller <uli.heller@daemons-point.com>
Date: Tue, 3 Sep 2013 09:14:03 +0200
Subject: [PATCH] git-svn: Fix termination issues for remote svn connections

When using git-svn in combination with serf-1.2.1 core dumps are
created on termination. This is caused by a bug in serf, a fix for
the bug exists (see https://code.google.com/p/serf/source/detail?r=2146).
Nevertheless, I think it makes sense to fix the issue within the
git perl module Ra.pm, too. The change frees the private copy of
the remote access object on termination which prevents the error
from happening.

Note: Since subversion-1.8.0 and later do require serf-1.2.1 or later,
the core dumps typically do show up when upgrading to a recent version
of subversion.

Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
pointing me to that fix.
Junio C Hamano for proposing a great code comment.
Kyle J. McKay for testing against latest serf (1.3.1).
... and various others - thanks!
---
  perl/Git/SVN/Ra.pm | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 75ecc42..a7b0119 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -32,6 +32,14 @@ BEGIN {
         }
  }

+# serf has a bug that leads to a coredump upon termination if the
+# remote access object is left around (not fixed yet in serf 1.3.1).
+# Explicitly free it to work around the issue.
+END {
+       $RA = undef;
+       $ra_invalid = 1;
+}
+
  sub _auth_providers () {
         my @rv = (
           SVN::Client::get_simple_provider(),
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] git-svn: Fix termination issues for remote svn   connections
  2013-09-09  6:01             ` [PATCH v2] " Uli Heller
@ 2013-09-09 15:42               ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2013-09-09 15:42 UTC (permalink / raw)
  To: Uli Heller; +Cc: Kyle J. McKay, Git Mailing List, Eric Wong

Uli Heller <uli.heller@daemons-point.com> writes:

> From a763550fc59b756580f9b162839d2737c27f2fe3 Mon Sep 17 00:00:00 2001
> From: Uli Heller <uli.heller@daemons-point.com>
> Date: Tue, 3 Sep 2013 09:14:03 +0200
> Subject: [PATCH] git-svn: Fix termination issues for remote svn connections

None of the above need to be in the e-mail body (the first one must
not be in the e-mail body, and the other three are redundant).

> When using git-svn in combination with serf-1.2.1 core dumps are
> created on termination. This is caused by a bug in serf, a fix for
> the bug exists (see https://code.google.com/p/serf/source/detail?r=2146).
> Nevertheless, I think it makes sense to fix the issue within the
> git perl module Ra.pm, too. The change frees the private copy of
> the remote access object on termination which prevents the error
> from happening.
>
> Note: Since subversion-1.8.0 and later do require serf-1.2.1 or later,
> the core dumps typically do show up when upgrading to a recent version
> of subversion.
>
> Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
> Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
> pointing me to that fix.
> Junio C Hamano for proposing a great code comment.
> Kyle J. McKay for testing against latest serf (1.3.1).
> ... and various others - thanks!
> ---

Hmph, the only change I see relative to the version in the message
you are responding to is reversion of the proposed commit log
message rewrite I did, and missing sign-off from you, and the latter
of which matters most.

I'd keep the one queued on 'pu' 73ffac3b (git-svn: fix termination
issues for remote svn connections, 2013-09-03) for now.

Thanks.

>  perl/Git/SVN/Ra.pm | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
> index 75ecc42..a7b0119 100644
> --- a/perl/Git/SVN/Ra.pm
> +++ b/perl/Git/SVN/Ra.pm
> @@ -32,6 +32,14 @@ BEGIN {
>         }
>  }
>
> +# serf has a bug that leads to a coredump upon termination if the
> +# remote access object is left around (not fixed yet in serf 1.3.1).
> +# Explicitly free it to work around the issue.
> +END {
> +       $RA = undef;
> +       $ra_invalid = 1;
> +}
> +
>  sub _auth_providers () {
>         my @rv = (
>           SVN::Client::get_simple_provider(),

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-09-09 22:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03  7:35 [PATCH] git-svn: Fix termination issues for remote svn connections Uli Heller
2013-09-05 18:48 ` Junio C Hamano
2013-09-05 19:02   ` Eric Wong
2013-09-05 23:14     ` Junio C Hamano
2013-09-06 11:46   ` Kyle J. McKay
2013-09-06 12:06     ` Uli Heller
2013-09-06 12:44       ` Kyle J. McKay
2013-09-06 13:18         ` Uli Heller
2013-09-06 16:41           ` Junio C Hamano
2013-09-09  6:01             ` [PATCH v2] " Uli Heller
2013-09-09 15:42               ` Junio C Hamano
2013-09-06 12:44   ` [PATCH] " Kyle J. McKay

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 NNTP newsgroup(s).