git/list[1] front-page[2] threads[3] people[4] search[5] about
 

RE: [PATCH] git-p4: close temporary file before removing

From
Philip McGraw <philip.mcgraw@bentley.com>
Date
Jul 31, 2019, 21:51 UTC
Message-ID
<BL0PR1901MB209790A0A8F5F9C8EFB8B3F0FFDF0@BL0PR1901MB2097.namprd19.prod.outlook.com>
In-Reply-To
<2835251564582156@myt6-4218ece6190d.qloud-c.yandex.net>
2019.07.31 10:09 Andrey <ahippo@yandex.ru> 
Show 49 quoted lines
>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

Previous: AndreyNext: Andrey
Message 5 of 8 in “git-p4: close temporary file before removing”
  1. git-p4: close temporary file before removingPhilip McGraw, Jul 30, 2019
  2. AndreyJul 31, 2019
  3. Philip McGrawJul 31, 2019
  4. AndreyJul 31, 2019
  5. Philip McGrawJul 31, 2019
  6. AndreyAug 1, 2019
  7. Philip McGrawAug 1, 2019
  8. AndreyAug 2, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.