Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function

[David Rothenberger]

Kyle J. McKay mackyle at gmail.com writes:

 +sub temp_is_locked {
 + my ($self, $name) = _maybe_self( at _);
 + my $temp_fd = \$TEMP_FILEMAP{$name};
 +
 + defined $$temp_fd  $$temp_fd-opened  
 $TEMP_FILES{$$temp_fd}{locked};
 +}
 +
  =item temp_release ( NAME )
 
  =item temp_release ( FILEHANDLE )
  at  at  -1248,7 +1277,7  at  at  sub _temp_cache {
 
   my $temp_fd = \$TEMP_FILEMAP{$name};
   if (defined $$temp_fd and $$temp_fd-opened) {
 - if ($TEMP_FILES{$$temp_fd}{locked}) {
 + if (temp_is_locked($name)) {
   throw Error::Simple(Temp file with moniker ' .
   $name . ' already in use);
   }

There's a problem with this use of temp_is_locked. There is an else
clause right after this:

} else {
if (defined $$temp_fd) {
# then we're here because of a closed handle.

Prior to the patch, the comment is correct, but after the patch, the
if block may also be entered if the file is open but locked. This is
because temp_is_locked checks that the temp file is defined, open,
and locked.

This issue leads to lots of 

  Temp file 'svn_delta_3360_0' was closed. Opening replacement. 

messages for me.

Reverting the change in _temp_cache solves the problem for me.
Adding an   !$$temp_fd-opened clause to the if statement also
works, but this is less efficient.


--
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 v3 1/2] Git.pm: add new temp_is_locked function

[Kyle J. McKay]

On Jul 18, 2013, at 11:34, David Rothenberger wrote:

Kyle J. McKay mackyle at gmail.com writes:


+sub temp_is_locked {
+   my ($self, $name) = _maybe_self( at _);
+   my $temp_fd = \$TEMP_FILEMAP{$name};
+
+	defined $$temp_fd  $$temp_fd-opened  $TEMP_FILES{$$temp_fd} 
{locked};

+}
+
=item temp_release ( NAME )

=item temp_release ( FILEHANDLE )
at  at  -1248,7 +1277,7  at  at  sub _temp_cache {

my $temp_fd = \$TEMP_FILEMAP{$name};
if (defined $$temp_fd and $$temp_fd-opened) {
-   if ($TEMP_FILES{$$temp_fd}{locked}) {
+   if (temp_is_locked($name)) {
throw Error::Simple(Temp file with moniker ' .
$name . ' already in use);
}


There's a problem with this use of temp_is_locked. There is an else
clause right after this:

} else {
if (defined $$temp_fd) {
# then we're here because of a closed handle.

Prior to the patch, the comment is correct, but after the patch, the
if block may also be entered if the file is open but locked. This is
because temp_is_locked checks that the temp file is defined, open,
and locked.

This issue leads to lots of

 Temp file 'svn_delta_3360_0' was closed. Opening replacement.

messages for me.

Reverting the change in _temp_cache solves the problem for me.
Adding an   !$$temp_fd-opened clause to the if statement also
works, but this is less efficient.


That change was made as a result of this feedback:

On Jul 6, 2013, at 17:11, Jonathan Nieder wrote:

Hi,

Kyle McKay wrote:


The temp_is_locked function can be used to determine whether
or not a given name previously passed to temp_acquire is
currently locked.

[...]

+=item temp_is_locked ( NAME )
+
+Returns true if the file mapped to CNAME is currently locked.
+
+If true is returned, an attempt to Ctemp_acquire() the same



[snip]


Looking more closely, it looks like this is factoring out the idiom
for checking if a name is already in use from the _temp_cache
function.  Would it make sense for _temp_cache to call this helper?


So I think the answer is it does not make sense for _temp_cache to  
call this helper.


Will release a v4 in just a moment with that single change reverted.
--
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 v3 1/2] Git.pm: add new temp_is_locked function

[Jonathan Nieder]

Kyle J. McKay wrote:

 That change was made as a result of this feedback:

 On Jul 6, 2013, at 17:11, Jonathan Nieder wrote:
 Kyle McKay wrote:

 The temp_is_locked function can be used to determine whether
 or not a given name previously passed to temp_acquire is
 currently locked.
 [...]
 +=item temp_is_locked ( NAME )
 +
 +Returns true if the file mapped to CNAME is currently locked.
 +
 +If true is returned, an attempt to Ctemp_acquire() the same

 [snip]

 Looking more closely, it looks like this is factoring out the idiom
 for checking if a name is already in use from the _temp_cache
 function.  Would it make sense for _temp_cache to call this helper?

 So I think the answer is it does not make sense for _temp_cache to
 call this helper.

Thanks for looking into it.

Sorry for the confusion.  The point of my question was an example of a
way to make sure the internal API stays easy to understand.  But it
seems to have backfired, and this is a small enough isolated change
that I think it's okay to say let's clean it up later.

 Will release a v4 in just a moment with that single change reverted.

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

[PATCH v3 1/2] Git.pm: add new temp_is_locked function

[Kyle J. McKay]

From: Kyle J. McKay mack...@gmail.com

The temp_is_locked function can be used to determine whether
or not a given name previously passed to temp_acquire is
currently locked.

Signed-off-by: Kyle J. McKay mack...@gmail.com
---
 perl/Git.pm | 33 +++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 7a252ef..0ba15b9 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -61,7 +61,7 @@ require Exporter;
 remote_refs prompt
 get_tz_offset
 credential credential_read credential_write
-temp_acquire temp_release temp_reset temp_path);
+temp_acquire temp_is_locked temp_release temp_reset temp_path);
 
 
 =head1 DESCRIPTION
@@ -1206,6 +1206,35 @@ sub temp_acquire {
$temp_fd;
 }
 
+=item temp_is_locked ( NAME )
+
+Returns true if the internal lock created by a previous Ctemp_acquire()
+call with CNAME is still in effect.
+
+When temp_acquire is called on a CNAME, it internally locks the temporary
+file mapped to CNAME.  That lock will not be released until Ctemp_release()
+is called with either the original CNAME or the LFile::Handle that was
+returned from the original call to temp_acquire.
+
+Subsequent attempts to call Ctemp_acquire() with the same CNAME will fail
+unless there has been an intervening Ctemp_release() call for that CNAME
+(or its corresponding LFile::Handle that was returned by the original
+Ctemp_acquire() call).
+
+If true is returned by Ctemp_is_locked() for a CNAME, an attempt to
+Ctemp_acquire() the same CNAME will cause an error unless
+Ctemp_release is first called on that CNAME (or its corresponding
+LFile::Handle that was returned by the original Ctemp_acquire() call).
+
+=cut
+
+sub temp_is_locked {
+   my ($self, $name) = _maybe_self(@_);
+   my $temp_fd = \$TEMP_FILEMAP{$name};
+
+   defined $$temp_fd  $$temp_fd-opened  
$TEMP_FILES{$$temp_fd}{locked};
+}
+
 =item temp_release ( NAME )
 
 =item temp_release ( FILEHANDLE )
@@ -1248,7 +1277,7 @@ sub _temp_cache {
 
my $temp_fd = \$TEMP_FILEMAP{$name};
if (defined $$temp_fd and $$temp_fd-opened) {
-   if ($TEMP_FILES{$$temp_fd}{locked}) {
+   if (temp_is_locked($name)) {
throw Error::Simple(Temp file with moniker ' .
$name . ' already in use);
}
-- 
1.8.3

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