Re: [PATCH] git-svn: workaround for a bug in svn serf backend

2013-12-30 Thread Thomas Rast 
Roman Kagan rka...@mail.ru writes:

 + # workaround for a bug in svn serf backend (v1.8.5 and below):
 + # store 3d argument to -add_file() in a local variable, to make it
 + # have the same lifetime as $fbat
 + my $upa = $self-url_path($m-{file_a});
   my $fbat = $self-add_file($self-repo_path($m-{file_b}), $pbat,
 - $self-url_path($m-{file_a}), $self-{r});
 + $upa, $self-{r});

Hmm, now that you put it that way, I wonder if the patch is correct.

Let me first rephrase the problem to verify that I understand the issue:

  $fbat keeps a pointer to the $upa string, without maintaining a
  reference to it.  When $fbat is destroyed, it needs this string, so we
  must ensure that the lifetime of $upa is at least as long as that of
  $fbat.

However, does Perl make any guarantees as to the order in which local
variables are unreferenced and then destroyed?  I can't find any such
guarantee.

In the absence of such, wouldn't we have to keep $upa in an outer,
separate scope to ensure that $fbat is destroyed first?

-- 
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

2013-12-30 Thread Roman Kagan 
2013/12/30 Thomas Rast t...@thomasrast.ch:
 Roman Kagan rka...@mail.ru writes:

 + # workaround for a bug in svn serf backend (v1.8.5 and below):
 + # store 3d argument to -add_file() in a local variable, to make it
 + # have the same lifetime as $fbat
 + my $upa = $self-url_path($m-{file_a});
   my $fbat = $self-add_file($self-repo_path($m-{file_b}), $pbat,
 - $self-url_path($m-{file_a}), $self-{r});
 + $upa, $self-{r});

 Hmm, now that you put it that way, I wonder if the patch is correct.

 Let me first rephrase the problem to verify that I understand the issue:

   $fbat keeps a pointer to the $upa string, without maintaining a
   reference to it.  When $fbat is destroyed, it needs this string, so we
   must ensure that the lifetime of $upa is at least as long as that of
   $fbat.

No.  The string is needed in subversion's close_file(), so we want to
keep it alive until close_file() returns. Surviving till the end of
the current function scope is sufficient for that.

 However, does Perl make any guarantees as to the order in which local
 variables are unreferenced and then destroyed?

We don't care about the order they are destroyed WRT each other.

Roman.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

2013-12-26 Thread Jonathan Nieder 
Roman Kagan wrote:

 Subversion serf backend in versions 1.8.5 and below has a bug that the
 function creating the descriptor of a file change -- add_file() --
 doesn't make a copy of its 3d argument when storing it on the returned

3d makes me think of 3-dimensional. ;-)  I think you mean third
(or the abbreviation 3rd).

 descriptor.  As a result, by the time this field is used (in
 transactions of file copying or renaming) it may well be released.

Please describe the symptom so this patch is easy to find when other
people run into it.

Do I remember correctly that ... released and scribbled over with a
new value, causing such-and-such assertion to fire was what happened?

 This patch works around this bug, by storing the value to be passed as
 the 3d argument to add_file() in a local variable with the same scope as
 the file change descriptor, making sure their lifetime is the same.

Could this be reproduced with a test script to make sure we don't
reintroduce the bug again later?  (It's okay if the test only fails on
machines with the problematic svn version.)

Modulo the confusing 3-dimensional arguments in comments, the code
change looks good.

Thanks and hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

2013-12-26 Thread Roman Kagan 
2013/12/27 Jonathan Nieder jrnie...@gmail.com:
 Roman Kagan wrote:

 Subversion serf backend in versions 1.8.5 and below has a bug that the
 function creating the descriptor of a file change -- add_file() --
 doesn't make a copy of its 3d argument when storing it on the returned

 3d makes me think of 3-dimensional. ;-)  I think you mean third
 (or the abbreviation 3rd).

Indeed.

 descriptor.  As a result, by the time this field is used (in
 transactions of file copying or renaming) it may well be released.

 Please describe the symptom so this patch is easy to find when other
 people run into it.

OK

 Do I remember correctly that ... released and scribbled over with a
 new value, causing such-and-such assertion to fire was what happened?

Exactly.

 This patch works around this bug, by storing the value to be passed as
 the 3d argument to add_file() in a local variable with the same scope as
 the file change descriptor, making sure their lifetime is the same.

 Could this be reproduced with a test script to make sure we don't
 reintroduce the bug again later?  (It's okay if the test only fails on
 machines with the problematic svn version.)

That would need a fairly fancy setup phase, as the bug triggers only
on http(s)-accessed svn repositories.  I'll take a look if there's
something already available in the existing test scripts; writing one
from scratch for this specific case is IMO beyond the reasonable
effort.

 Modulo the confusing 3-dimensional arguments in comments, the code
 change looks good.

Thanks, I'll adjust the wording and resubmit.

Roman.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

2013-12-26 Thread Roman Kagan 
2013/12/27 Roman Kagan rka...@mail.ru:
 2013/12/27 Jonathan Nieder jrnie...@gmail.com:
 Could this be reproduced with a test script to make sure we don't
 reintroduce the bug again later?  (It's okay if the test only fails on
 machines with the problematic svn version.)

 That would need a fairly fancy setup phase, as the bug triggers only
 on http(s)-accessed svn repositories.  I'll take a look if there's
 something already available in the existing test scripts

Turns out the stuff is all there, and the tests doing file renames and
dcomit-ting them do exist (t9115-git-svn-dcommit-funky-renames.sh for
one).

However, the httpd setup is seriously broken; I haven't managed to get
it to run on my Fedora 20 with apache 2.4.6.  Apparently git-svn tests
(almost) never get executed against an http-based repository; even
those who don't set NO_SVN_TESTS get them run against file-based
repository and thus don't trigger the error.

Someone with better apache-foo needs to take a look into that.  Once
that is sorted out I believe the tests will start triggering the bug.

Meanwhile I assume that the patch doesn't need to include an extra testcase.

Roman.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html