Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function
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 C is currently locked. >>> + >>> +If true is returned, an attempt to C 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
Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function
On Jul 18, 2013, at 11:34, David Rothenberger wrote:
Kyle J. McKay gmail.com> writes:
+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");
}
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 C is currently locked.
+
+If true is returned, an attempt to C 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
Kyle J. McKay gmail.com> writes:
> +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");
> }
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
