git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] git-p4: close temporary file before removing
@ 2019-07-30 17:37 Philip McGraw
  2019-07-31  1:48 ` Andrey
  0 siblings, 1 reply; 8+ messages in thread
From: Philip McGraw @ 2019-07-30 17:37 UTC (permalink / raw)
  To: git

python os.remove() throws exceptions on Windows platform when attempting
to remove file while it is still open.  Need to grab filename while file open,
close file handle, then remove by name.  Apparently other platforms are more
permissive of removing files while busy.
reference: https://docs.python.org/3/library/os.html#os.remove
---
 git-p4.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index c71a6832e2..6b9d2a8317 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):
                 return False
             contentTempFile = self.generateTempFile(contents)
             compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
+            compressedContentFileName = compressedContentFile.name
             zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
             zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
             zf.close()
             compressedContentsSize = zf.infolist()[0].compress_size
             os.remove(contentTempFile)
-            os.remove(compressedContentFile.name)
+            compressedContentFile.close()
+            os.remove(compressedContentFileName)
             if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return True
         return False
--
2.21.0.windows.1

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

* Re: [PATCH] git-p4: close temporary file before removing
  2019-07-30 17:37 [PATCH] git-p4: close temporary file before removing Philip McGraw
@ 2019-07-31  1:48 ` Andrey
  2019-07-31 13:53   ` Philip McGraw
  0 siblings, 1 reply; 8+ messages in thread
From: Andrey @ 2019-07-31  1:48 UTC (permalink / raw)
  To: Philip McGraw, git, luke



30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
> python os.remove() throws exceptions on Windows platform when attempting
> to remove file while it is still open. Need to grab filename while file open,
> close file handle, then remove by name. Apparently other platforms are more
> permissive of removing files while busy.
> reference: https://docs.python.org/3/library/os.html#os.remove
> ---
>  git-p4.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index c71a6832e2..6b9d2a8317 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):
>                  return False
>              contentTempFile = self.generateTempFile(contents)
>              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> + compressedContentFileName = compressedContentFile.name
>              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
>              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
>              zf.close()
>              compressedContentsSize = zf.infolist()[0].compress_size
>              os.remove(contentTempFile)
> - os.remove(compressedContentFile.name)
> + compressedContentFile.close()
> + os.remove(compressedContentFileName)

I'm not sure why NamedTemporaryFile() is called with delete=False above,
but it appears to me that it can have delete=True instead,
so that there is no need to call os.remove() explicitly
and thus worry about remove vs close ordering at all.

>              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
>                  return True
>          return False
> --
> 2.21.0.windows.1

Thank you,
Andrey.


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

* RE: [PATCH] git-p4: close temporary file before removing
  2019-07-31  1:48 ` Andrey
@ 2019-07-31 13:53   ` Philip McGraw
  2019-07-31 14:09     ` Andrey
  0 siblings, 1 reply; 8+ messages in thread
From: Philip McGraw @ 2019-07-31 13:53 UTC (permalink / raw)
  To: ahippo; +Cc: git, luke

> 30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
> > python os.remove() throws exceptions on Windows platform when attempting
> > to remove file while it is still open. Need to grab filename while file open,
> > close file handle, then remove by name. Apparently other platforms are more
> > permissive of removing files while busy.
> > reference: https://docs.python.org/3/library/os.html#os.remove
> > ---
> >  git-p4.py | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-p4.py b/git-p4.py
> > index c71a6832e2..6b9d2a8317 100755
> > --- a/git-p4.py
> > +++ b/git-p4.py
> > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):
> >                  return False
> >              contentTempFile = self.generateTempFile(contents)
> >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> > + compressedContentFileName = compressedContentFile.name
> >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
> >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> >              zf.close()
> >              compressedContentsSize = zf.infolist()[0].compress_size
> >              os.remove(contentTempFile)
> > - os.remove(compressedContentFile.name)
> > + compressedContentFile.close()
> > + os.remove(compressedContentFileName)
> 
> I'm not sure why NamedTemporaryFile() is called with delete=False above,
> but it appears to me that it can have delete=True instead,
> so that there is no need to call os.remove() explicitly
> and thus worry about remove vs close ordering at all.
> 
> >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
> >                  return True
> >          return False
> > --
> > 2.21.0.windows.1
> 
> Thank you,
> Andrey.

Thanks Andrey; simpler is certainly better!  I will test and re-submit v2 of patch with that approach.


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

* Re: [PATCH] git-p4: close temporary file before removing
  2019-07-31 13:53   ` Philip McGraw
@ 2019-07-31 14:09     ` Andrey
  2019-07-31 21:51       ` Philip McGraw
  0 siblings, 1 reply; 8+ messages in thread
From: Andrey @ 2019-07-31 14:09 UTC (permalink / raw)
  To: Philip McGraw; +Cc: git, luke



31.07.2019, 09:53, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>  30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>  > python os.remove() throws exceptions on Windows platform when attempting
>>  > to remove file while it is still open. Need to grab filename while file open,
>>  > close file handle, then remove by name. Apparently other platforms are more
>>  > permissive of removing files while busy.
>>  > reference: https://docs.python.org/3/library/os.html#os.remove
>>  > ---
>>  >  git-p4.py | 4 +++-
>>  >  1 file changed, 3 insertions(+), 1 deletion(-)
>>  >
>>  > diff --git a/git-p4.py b/git-p4.py
>>  > index c71a6832e2..6b9d2a8317 100755
>>  > --- a/git-p4.py
>>  > +++ b/git-p4.py
>>  > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):
>>  >                  return False
>>  >              contentTempFile = self.generateTempFile(contents)
>>  >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
>>  > + compressedContentFileName = compressedContentFile.name
>>  >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
>>  >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
>>  >              zf.close()
>>  >              compressedContentsSize = zf.infolist()[0].compress_size
>>  >              os.remove(contentTempFile)
>>  > - os.remove(compressedContentFile.name)
>>  > + compressedContentFile.close()
>>  > + os.remove(compressedContentFileName)
>>
>>  I'm not sure why NamedTemporaryFile() is called with delete=False above,
>>  but it appears to me that it can have delete=True instead,
>>  so that there is no need to call os.remove() explicitly
>>  and thus worry about remove vs close ordering at all.
>>
>>  >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
>>  >                  return True
>>  >          return False
>>  > --
>>  > 2.21.0.windows.1
>>
>>  Thank you,
>>  Andrey.
>
> Thanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.

Thank you, that would be great!

-- 
Andrey.


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

* RE: [PATCH] git-p4: close temporary file before removing
  2019-07-31 14:09     ` Andrey
@ 2019-07-31 21:51       ` Philip McGraw
  2019-08-01  1:34         ` Andrey
  0 siblings, 1 reply; 8+ messages in thread
From: Philip McGraw @ 2019-07-31 21:51 UTC (permalink / raw)
  To: ahippo; +Cc: git, luke

2019.07.31 10:09 Andrey <ahippo@yandex.ru> 
>31.07.2019, 09:53, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>>  30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>>  > python os.remove() throws exceptions on Windows platform when attempting
>>>  > to remove file while it is still open. Need to grab filename while file open,
>>>  > close file handle, then remove by name. Apparently other platforms are more
>>>  > permissive of removing files while busy.
>>>  > reference: 
>>>  > ---
>>>  >  git-p4.py | 4 +++-
>>>  >  1 file changed, 3 insertions(+), 1 deletion(-)
>>>  >
>>>  > diff --git a/git-p4.py b/git-p4.py
>>>  > index c71a6832e2..6b9d2a8317 100755
>>>  > --- a/git-p4.py
>>>  > +++ b/git-p4.py
>>>  > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):
>>>  >                  return False
>>>  >              contentTempFile = self.generateTempFile(contents)
>>>  >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
>>>  > + compressedContentFileName = compressedContentFile.name
>>>  >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
>>>  >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
>>>  >              zf.close()
>>>  >              compressedContentsSize = zf.infolist()[0].compress_size
>>>  >              os.remove(contentTempFile)
>>>  > - os.remove(compressedContentFile.name)
>>>  > + compressedContentFile.close()
>>>  > + os.remove(compressedContentFileName)
>>>
>>>  I'm not sure why NamedTemporaryFile() is called with delete=False above,
>>>  but it appears to me that it can have delete=True instead,
>>>  so that there is no need to call os.remove() explicitly
>>>  and thus worry about remove vs close ordering at all.
>>>
>>>  >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
>>>  >                  return True
>>>  >          return False
>>>  > --
>>>  > 2.21.0.windows.1
>>>
>>>  Thank you,
>>>  Andrey.
>>
>> Thanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.
>
>Thank you, that would be great!
>
>-- 
>Andrey.

Unfortunately it wasn't as simple it seemed: upon testing with only changing delete=True, 
found that the problem was not solved.  Upon further debugging, recoded/refactored slightly adding 
allocateTempFileName() locally scoped function to try to clarify how the NamedTemporaryFile()
was actually being used.

We can't depend on the delete-on-close because the NamedTemporaryFile() is merely allocating 
a temporary name for real use by the zipfile open-for-write which fails (on Windows) if file
was not explicitly closed first.  

Hopefully the new patch (https://github.com/gitgitgadget/git/pull/301) will make this more clear.

Open to other suggestions if still not clear.

Thanks again,
Philip


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

* Re: [PATCH] git-p4: close temporary file before removing
  2019-07-31 21:51       ` Philip McGraw
@ 2019-08-01  1:34         ` Andrey
  2019-08-01 15:30           ` Philip McGraw
  0 siblings, 1 reply; 8+ messages in thread
From: Andrey @ 2019-08-01  1:34 UTC (permalink / raw)
  To: Philip McGraw; +Cc: git, luke



31.07.2019, 17:52, "Philip McGraw" <philip.mcgraw@bentley.com>:
> 2019.07.31 10:09 Andrey <ahippo@yandex.ru>
>> 31.07.2019, 09:53, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>>>   30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>>>   > python os.remove() throws exceptions on Windows platform when attempting
>>>>   > to remove file while it is still open. Need to grab filename while file open,
>>>>   > close file handle, then remove by name. Apparently other platforms are more
>>>>   > permissive of removing files while busy.
>>>>   > reference:
>>>>   > ---
>>>>   >  git-p4.py | 4 +++-
>>>>   >  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>   >
>>>>   > diff --git a/git-p4.py b/git-p4.py
>>>>   > index c71a6832e2..6b9d2a8317 100755
>>>>   > --- a/git-p4.py
>>>>   > +++ b/git-p4.py
>>>>   > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):
>>>>   >                  return False
>>>>   >              contentTempFile = self.generateTempFile(contents)
>>>>   >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
>>>>   > + compressedContentFileName = compressedContentFile.name
>>>>   >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
>>>>   >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
>>>>   >              zf.close()
>>>>   >              compressedContentsSize = zf.infolist()[0].compress_size
>>>>   >              os.remove(contentTempFile)
>>>>   > - os.remove(compressedContentFile.name)
>>>>   > + compressedContentFile.close()
>>>>   > + os.remove(compressedContentFileName)
>>>>
>>>>   I'm not sure why NamedTemporaryFile() is called with delete=False above,
>>>>   but it appears to me that it can have delete=True instead,
>>>>   so that there is no need to call os.remove() explicitly
>>>>   and thus worry about remove vs close ordering at all.
>>>>
>>>>   >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
>>>>   >                  return True
>>>>   >          return False
>>>>   > --
>>>>   > 2.21.0.windows.1
>>>>
>>>>   Thank you,
>>>>   Andrey.
>>>
>>>  Thanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.
>>
>> Thank you, that would be great!
>>
>> --
>> Andrey.
>
> Unfortunately it wasn't as simple it seemed: upon testing with only changing delete=True,
> found that the problem was not solved. Upon further debugging, recoded/refactored slightly adding
> allocateTempFileName() locally scoped function to try to clarify how the NamedTemporaryFile()
> was actually being used.
>
> We can't depend on the delete-on-close because the NamedTemporaryFile() is merely allocating
> a temporary name for real use by the zipfile open-for-write which fails (on Windows) if file
> was not explicitly closed first.

Oh, sorry for misguiding you!
I didn't think of this aspect.

> Hopefully the new patch (https://github.com/gitgitgadget/git/pull/301) will make this more clear.

The new changeset looks good to me.
(I'll post a reply in that thread too)

> Open to other suggestions if still not clear.

Just as a thought, ZipFile() can take a file-like object instead of a file name,
so can be passed the NamedTemporaryFile() object directly instead of its file name.
This should hopefully avoid double-open issue on Windows.

However, I'm good with your allocateTempFileName() changeset,
so it's up to you to give it a try or not.

> Thanks again,
> Philip

Thank you,
Andrey.


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

* RE: [PATCH] git-p4: close temporary file before removing
  2019-08-01  1:34         ` Andrey
@ 2019-08-01 15:30           ` Philip McGraw
  2019-08-02  3:50             ` Andrey
  0 siblings, 1 reply; 8+ messages in thread
From: Philip McGraw @ 2019-08-01 15:30 UTC (permalink / raw)
  To: ahippo; +Cc: git, luke

> From: Andrey <ahippo@yandex.ru>
> Sent: Wednesday, 31 July, 2019 21:35
> To: Philip McGraw <Philip.McGraw@bentley.com>
> Cc: git@vger.kernel.org; luke@diamand.org
> Subject: Re: [PATCH] git-p4: close temporary file before removing
> 
> 31.07.2019, 17:52, "Philip McGraw" <philip.mcgraw@bentley.com>:
> > 2019.07.31 10:09 Andrey <ahippo@yandex.ru>
> >> 31.07.2019, 09:53, "Philip McGraw" <philip.mcgraw@bentley.com>:
> >>>>   30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
> >>>>   > python os.remove() throws exceptions on Windows platform when
> attempting
> >>>>   > to remove file while it is still open. Need to grab filename
> while file open,
> >>>>   > close file handle, then remove by name. Apparently other
> platforms are more
> >>>>   > permissive of removing files while busy.
> >>>>   > reference:
> >>>>   > ---
> >>>>   >  git-p4.py | 4 +++-
> >>>>   >  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>   >
> >>>>   > diff --git a/git-p4.py b/git-p4.py
> >>>>   > index c71a6832e2..6b9d2a8317 100755
> >>>>   > --- a/git-p4.py
> >>>>   > +++ b/git-p4.py
> >>>>   > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self,
> relPath, contents):
> >>>>   >                  return False
> >>>>   >              contentTempFile = self.generateTempFile(contents)
> >>>>   >              compressedContentFile =
> tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> >>>>   > + compressedContentFileName = compressedContentFile.name
> >>>>   >              zf = zipfile.ZipFile(compressedContentFile.name,
> mode='w')
> >>>>   >              zf.write(contentTempFile,
> compress_type=zipfile.ZIP_DEFLATED)
> >>>>   >              zf.close()
> >>>>   >              compressedContentsSize =
> zf.infolist()[0].compress_size
> >>>>   >              os.remove(contentTempFile)
> >>>>   > - os.remove(compressedContentFile.name)
> >>>>   > + compressedContentFile.close()
> >>>>   > + os.remove(compressedContentFileName)
> >>>>
> >>>>   I'm not sure why NamedTemporaryFile() is called with delete=False
> above,
> >>>>   but it appears to me that it can have delete=True instead,
> >>>>   so that there is no need to call os.remove() explicitly
> >>>>   and thus worry about remove vs close ordering at all.
> >>>>
> >>>>   >              if compressedContentsSize > gitConfigInt('git-
> p4.largeFileCompressedThreshold'):
> >>>>   >                  return True
> >>>>   >          return False
> >>>>   > --
> >>>>   > 2.21.0.windows.1
> >>>>
> >>>>   Thank you,
> >>>>   Andrey.
> >>>
> >>>  Thanks Andrey; simpler is certainly better! I will test and re-submit
> v2 of patch with that approach.
> >>
> >> Thank you, that would be great!
> >>
> >> --
> >> Andrey.
> >
> > Unfortunately it wasn't as simple it seemed: upon testing with only
> changing delete=True,
> > found that the problem was not solved. Upon further debugging,
> recoded/refactored slightly adding
> > allocateTempFileName() locally scoped function to try to clarify how the
> NamedTemporaryFile()
> > was actually being used.
> >
> > We can't depend on the delete-on-close because the NamedTemporaryFile()
> is merely allocating
> > a temporary name for real use by the zipfile open-for-write which fails
> (on Windows) if file
> > was not explicitly closed first.
> 
> Oh, sorry for misguiding you!
> I didn't think of this aspect.

No worries! I probably just misunderstood the implementation of your idea.
> 
> > Hopefully the new patch
> (https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_gitgitgadget_git_pull_301&d=DwIDaQ&c=hmGTLOph1qd_VnCqj81HzE
> WkDaxmYdIWRBdoFggzhj8&r=b0ikFMJGw7xxhF3yjexiWJpLuNxlAh1SvUDuUJ-
> pHmE&m=1jGOrV_I1Mg5ajkJ7yFEcNlyLnD6zYNXqXB9Z5SIPyE&s=TdT4WHyQCk5WZty_CvajH
> XgrZJbmIOl1gbMcngmjmAs&e= ) will make this more clear.
> 
> The new changeset looks good to me.
> (I'll post a reply in that thread too)
> 
> > Open to other suggestions if still not clear.
> 
> Just as a thought, ZipFile() can take a file-like object instead of a file
> name,
> so can be passed the NamedTemporaryFile() object directly instead of its
> file name.
> This should hopefully avoid double-open issue on Windows.

Another excellent idea that minimizes changes.  I am testing this approach
now and will submit v3 of the patch soon.
> 
> However, I'm good with your allocateTempFileName() changeset,
> so it's up to you to give it a try or not.
> 
> > Thanks again,
> > Philip
> 
> Thank you,
> Andrey.

Thanks,
Philip

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

* Re: [PATCH] git-p4: close temporary file before removing
  2019-08-01 15:30           ` Philip McGraw
@ 2019-08-02  3:50             ` Andrey
  0 siblings, 0 replies; 8+ messages in thread
From: Andrey @ 2019-08-02  3:50 UTC (permalink / raw)
  To: Philip McGraw; +Cc: git, luke



01.08.2019, 11:30, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>  From: Andrey <ahippo@yandex.ru>
>>  Sent: Wednesday, 31 July, 2019 21:35
>>  To: Philip McGraw <Philip.McGraw@bentley.com>
>>  Cc: git@vger.kernel.org; luke@diamand.org
>>  Subject: Re: [PATCH] git-p4: close temporary file before removing
>>
>>  31.07.2019, 17:52, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>  > 2019.07.31 10:09 Andrey <ahippo@yandex.ru>
>>  >> 31.07.2019, 09:53, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>  >>>>   30.07.2019, 13:37, "Philip McGraw" <philip.mcgraw@bentley.com>:
>>  >>>>   > python os.remove() throws exceptions on Windows platform when
>>  attempting
>>  >>>>   > to remove file while it is still open. Need to grab filename
>>  while file open,
>>  >>>>   > close file handle, then remove by name. Apparently other
>>  platforms are more
>>  >>>>   > permissive of removing files while busy.
>>  >>>>   > reference:
>>  >>>>   > ---
>>  >>>>   >  git-p4.py | 4 +++-
>>  >>>>   >  1 file changed, 3 insertions(+), 1 deletion(-)
>>  >>>>   >
>>  >>>>   > diff --git a/git-p4.py b/git-p4.py
>>  >>>>   > index c71a6832e2..6b9d2a8317 100755
>>  >>>>   > --- a/git-p4.py
>>  >>>>   > +++ b/git-p4.py
>>  >>>>   > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self,
>>  relPath, contents):
>>  >>>>   >                  return False
>>  >>>>   >              contentTempFile = self.generateTempFile(contents)
>>  >>>>   >              compressedContentFile =
>>  tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
>>  >>>>   > + compressedContentFileName = compressedContentFile.name
>>  >>>>   >              zf = zipfile.ZipFile(compressedContentFile.name,
>>  mode='w')
>>  >>>>   >              zf.write(contentTempFile,
>>  compress_type=zipfile.ZIP_DEFLATED)
>>  >>>>   >              zf.close()
>>  >>>>   >              compressedContentsSize =
>>  zf.infolist()[0].compress_size
>>  >>>>   >              os.remove(contentTempFile)
>>  >>>>   > - os.remove(compressedContentFile.name)
>>  >>>>   > + compressedContentFile.close()
>>  >>>>   > + os.remove(compressedContentFileName)
>>  >>>>
>>  >>>>   I'm not sure why NamedTemporaryFile() is called with delete=False
>>  above,
>>  >>>>   but it appears to me that it can have delete=True instead,
>>  >>>>   so that there is no need to call os.remove() explicitly
>>  >>>>   and thus worry about remove vs close ordering at all.
>>  >>>>
>>  >>>>   >              if compressedContentsSize > gitConfigInt('git-
>>  p4.largeFileCompressedThreshold'):
>>  >>>>   >                  return True
>>  >>>>   >          return False
>>  >>>>   > --
>>  >>>>   > 2.21.0.windows.1
>>  >>>>
>>  >>>>   Thank you,
>>  >>>>   Andrey.
>>  >>>
>>  >>>  Thanks Andrey; simpler is certainly better! I will test and re-submit
>>  v2 of patch with that approach.
>>  >>
>>  >> Thank you, that would be great!
>>  >>
>>  >> --
>>  >> Andrey.
>>  >
>>  > Unfortunately it wasn't as simple it seemed: upon testing with only
>>  changing delete=True,
>>  > found that the problem was not solved. Upon further debugging,
>>  recoded/refactored slightly adding
>>  > allocateTempFileName() locally scoped function to try to clarify how the
>>  NamedTemporaryFile()
>>  > was actually being used.
>>  >
>>  > We can't depend on the delete-on-close because the NamedTemporaryFile()
>>  is merely allocating
>>  > a temporary name for real use by the zipfile open-for-write which fails
>>  (on Windows) if file
>>  > was not explicitly closed first.
>>
>>  Oh, sorry for misguiding you!
>>  I didn't think of this aspect.
>
> No worries! I probably just misunderstood the implementation of your idea.

No, you understood what I was saying correctly.
It's just that I didn't think of opening the file twice.
(or rather that it would be a problem)

>>  > Hopefully the new patch
>>  (https://urldefense.proofpoint.com/v2/url?u=https-
>>  3A__github.com_gitgitgadget_git_pull_301&d=DwIDaQ&c=hmGTLOph1qd_VnCqj81HzE
>>  WkDaxmYdIWRBdoFggzhj8&r=b0ikFMJGw7xxhF3yjexiWJpLuNxlAh1SvUDuUJ-
>>  pHmE&m=1jGOrV_I1Mg5ajkJ7yFEcNlyLnD6zYNXqXB9Z5SIPyE&s=TdT4WHyQCk5WZty_CvajH
>>  XgrZJbmIOl1gbMcngmjmAs&e= ) will make this more clear.
>>
>>  The new changeset looks good to me.
>>  (I'll post a reply in that thread too)
>>
>>  > Open to other suggestions if still not clear.
>>
>>  Just as a thought, ZipFile() can take a file-like object instead of a file
>>  name,
>>  so can be passed the NamedTemporaryFile() object directly instead of its
>>  file name.
>>  This should hopefully avoid double-open issue on Windows.
>
> Another excellent idea that minimizes changes. I am testing this approach
> now and will submit v3 of the patch soon.

Thank you for willing to try the new approach!

>>  However, I'm good with your allocateTempFileName() changeset,
>>  so it's up to you to give it a try or not.
>>
>>  > Thanks again,
>>  > Philip
>>
>>  Thank you,
>>  Andrey.
>
> Thanks,
> Philip

-- 
Andrey.


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

end of thread, other threads:[~2019-08-02  3:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 17:37 [PATCH] git-p4: close temporary file before removing Philip McGraw
2019-07-31  1:48 ` Andrey
2019-07-31 13:53   ` Philip McGraw
2019-07-31 14:09     ` Andrey
2019-07-31 21:51       ` Philip McGraw
2019-08-01  1:34         ` Andrey
2019-08-01 15:30           ` Philip McGraw
2019-08-02  3:50             ` Andrey

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git