{"thread":{"id":"51558","subject":"[PATCH] git-p4: close temporary file before removing","startedAt":"2019-07-30T17:37:27Z","lastAt":"2019-08-02T03:50:38Z","messageCount":8,"participants":["Philip McGraw","Andrey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"379587","messageId":"BL0PR1901MB209738ADDF9D931253E8C317FFDC0@BL0PR1901MB2097.namprd19.prod.outlook.com","threadId":"51558","inReplyTo":null,"subject":"[PATCH] git-p4: close temporary file before removing","fromName":"Philip McGraw","fromEmail":"philip.mcgraw@bentley.com","sentAt":"2019-07-30T17:37:22Z","receivedAt":"2019-07-30T17:37:27Z","isPatch":true,"sender":{"key":"philip.mcgraw@bentley.com","avatar":"https://avatars.githubusercontent.com/u/18532989?v=4"},"body":"python os.remove() throws exceptions on Windows platform when attempting\nto remove file while it is still open. Need to grab filename while file open,\nclose file handle, then remove by name. Apparently other platforms are more\npermissive of removing files while busy.\nreference: https://docs.python.org/3/library/os.html#os.remove\n---\n git-p4.py | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex c71a6832e2..6b9d2a8317 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):\n return False\n contentTempFile = self.generateTempFile(contents)\n compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n+ compressedContentFileName = compressedContentFile.name\n zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)\n zf.close()\n compressedContentsSize = zf.infolist()[0].compress_size\n os.remove(contentTempFile)\n- os.remove(compressedContentFile.name)\n+ compressedContentFile.close()\n+ os.remove(compressedContentFileName)\n if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n return True\n return False\n--\n2.21.0.windows.1\n"},{"id":"379632","messageId":"1955471564537683@vla1-53bffb0b04ed.qloud-c.yandex.net","threadId":"51558","inReplyTo":"BL0PR1901MB209738ADDF9D931253E8C317FFDC0@BL0PR1901MB2097.namprd19.prod.outlook.com","subject":"Re: [PATCH] git-p4: close temporary file before removing","fromName":"Andrey","fromEmail":"ahippo@yandex.ru","sentAt":"2019-07-31T01:48:03Z","receivedAt":"2019-07-31T01:48:09Z","isPatch":true,"sender":{"key":"ahippo@yandex.ru","avatar":null},"body":"\n\n30.07.2019, 13:37, \"Philip McGraw\" :\n> python os.remove() throws exceptions on Windows platform when attempting\n> to remove file while it is still open. Need to grab filename while file open,\n> close file handle, then remove by name. Apparently other platforms are more\n> permissive of removing files while busy.\n> reference: https://docs.python.org/3/library/os.html#os.remove\n> ---\n>  git-p4.py | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index c71a6832e2..6b9d2a8317 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):\n>                  return False\n>              contentTempFile = self.generateTempFile(contents)\n>              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n> + compressedContentFileName = compressedContentFile.name\n>              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n>              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)\n>              zf.close()\n>              compressedContentsSize = zf.infolist()[0].compress_size\n>              os.remove(contentTempFile)\n> - os.remove(compressedContentFile.name)\n> + compressedContentFile.close()\n> + os.remove(compressedContentFileName)\n\nI'm not sure why NamedTemporaryFile() is called with delete=False above,\nbut it appears to me that it can have delete=True instead,\nso that there is no need to call os.remove() explicitly\nand thus worry about remove vs close ordering at all.\n\n>              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n>                  return True\n>          return False\n> --\n> 2.21.0.windows.1\n\nThank you,\nAndrey.\n\n"},{"id":"379665","messageId":"BL0PR1901MB2097EA8851C2743D46210D38FFDF0@BL0PR1901MB2097.namprd19.prod.outlook.com","threadId":"51558","inReplyTo":"1955471564537683@vla1-53bffb0b04ed.qloud-c.yandex.net","subject":"RE: [PATCH] git-p4: close temporary file before removing","fromName":"Philip McGraw","fromEmail":"philip.mcgraw@bentley.com","sentAt":"2019-07-31T13:53:35Z","receivedAt":"2019-07-31T13:53:39Z","isPatch":true,"sender":{"key":"philip.mcgraw@bentley.com","avatar":"https://avatars.githubusercontent.com/u/18532989?v=4"},"body":"> 30.07.2019, 13:37, \"Philip McGraw\" :\n> > python os.remove() throws exceptions on Windows platform when attempting\n> > to remove file while it is still open. Need to grab filename while file open,\n> > close file handle, then remove by name. Apparently other platforms are more\n> > permissive of removing files while busy.\n> > reference: https://docs.python.org/3/library/os.html#os.remove\n> > ---\n> >  git-p4.py | 4 +++-\n> >  1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/git-p4.py b/git-p4.py\n> > index c71a6832e2..6b9d2a8317 100755\n> > --- a/git-p4.py\n> > +++ b/git-p4.py\n> > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):\n> >                  return False\n> >              contentTempFile = self.generateTempFile(contents)\n> >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n> > + compressedContentFileName = compressedContentFile.name\n> >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n> >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)\n> >              zf.close()\n> >              compressedContentsSize = zf.infolist()[0].compress_size\n> >              os.remove(contentTempFile)\n> > - os.remove(compressedContentFile.name)\n> > + compressedContentFile.close()\n> > + os.remove(compressedContentFileName)\n> \n> I'm not sure why NamedTemporaryFile() is called with delete=False above,\n> but it appears to me that it can have delete=True instead,\n> so that there is no need to call os.remove() explicitly\n> and thus worry about remove vs close ordering at all.\n> \n> >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n> >                  return True\n> >          return False\n> > --\n> > 2.21.0.windows.1\n> \n> Thank you,\n> Andrey.\n\nThanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.\n\n"},{"id":"379666","messageId":"2835251564582156@myt6-4218ece6190d.qloud-c.yandex.net","threadId":"51558","inReplyTo":"BL0PR1901MB2097EA8851C2743D46210D38FFDF0@BL0PR1901MB2097.namprd19.prod.outlook.com","subject":"Re: [PATCH] git-p4: close temporary file before removing","fromName":"Andrey","fromEmail":"ahippo@yandex.ru","sentAt":"2019-07-31T14:09:16Z","receivedAt":"2019-07-31T14:09:22Z","isPatch":true,"sender":{"key":"ahippo@yandex.ru","avatar":null},"body":"\n\n31.07.2019, 09:53, \"Philip McGraw\" :\n>>  30.07.2019, 13:37, \"Philip McGraw\" :\n>>  > python os.remove() throws exceptions on Windows platform when attempting\n>>  > to remove file while it is still open. Need to grab filename while file open,\n>>  > close file handle, then remove by name. Apparently other platforms are more\n>>  > permissive of removing files while busy.\n>>  > reference: https://docs.python.org/3/library/os.html#os.remove\n>>  > ---\n>>  >  git-p4.py | 4 +++-\n>>  >  1 file changed, 3 insertions(+), 1 deletion(-)\n>>  >\n>>  > diff --git a/git-p4.py b/git-p4.py\n>>  > index c71a6832e2..6b9d2a8317 100755\n>>  > --- a/git-p4.py\n>>  > +++ b/git-p4.py\n>>  > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):\n>>  >                  return False\n>>  >              contentTempFile = self.generateTempFile(contents)\n>>  >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n>>  > + compressedContentFileName = compressedContentFile.name\n>>  >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n>>  >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)\n>>  >              zf.close()\n>>  >              compressedContentsSize = zf.infolist()[0].compress_size\n>>  >              os.remove(contentTempFile)\n>>  > - os.remove(compressedContentFile.name)\n>>  > + compressedContentFile.close()\n>>  > + os.remove(compressedContentFileName)\n>>\n>>  I'm not sure why NamedTemporaryFile() is called with delete=False above,\n>>  but it appears to me that it can have delete=True instead,\n>>  so that there is no need to call os.remove() explicitly\n>>  and thus worry about remove vs close ordering at all.\n>>\n>>  >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n>>  >                  return True\n>>  >          return False\n>>  > --\n>>  > 2.21.0.windows.1\n>>\n>>  Thank you,\n>>  Andrey.\n>\n> Thanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.\n\nThank you, that would be great!\n\n-- \nAndrey.\n\n"},{"id":"379718","messageId":"BL0PR1901MB209790A0A8F5F9C8EFB8B3F0FFDF0@BL0PR1901MB2097.namprd19.prod.outlook.com","threadId":"51558","inReplyTo":"2835251564582156@myt6-4218ece6190d.qloud-c.yandex.net","subject":"RE: [PATCH] git-p4: close temporary file before removing","fromName":"Philip McGraw","fromEmail":"philip.mcgraw@bentley.com","sentAt":"2019-07-31T21:51:22Z","receivedAt":"2019-07-31T21:51:28Z","isPatch":true,"sender":{"key":"philip.mcgraw@bentley.com","avatar":"https://avatars.githubusercontent.com/u/18532989?v=4"},"body":"2019.07.31 10:09 Andrey \n>31.07.2019, 09:53, \"Philip McGraw\" :\n>>>  30.07.2019, 13:37, \"Philip McGraw\" :\n>>>  > python os.remove() throws exceptions on Windows platform when attempting\n>>>  > to remove file while it is still open. Need to grab filename while file open,\n>>>  > close file handle, then remove by name. Apparently other platforms are more\n>>>  > permissive of removing files while busy.\n>>>  > reference: \n>>>  > ---\n>>>  >  git-p4.py | 4 +++-\n>>>  >  1 file changed, 3 insertions(+), 1 deletion(-)\n>>>  >\n>>>  > diff --git a/git-p4.py b/git-p4.py\n>>>  > index c71a6832e2..6b9d2a8317 100755\n>>>  > --- a/git-p4.py\n>>>  > +++ b/git-p4.py\n>>>  > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):\n>>>  >                  return False\n>>>  >              contentTempFile = self.generateTempFile(contents)\n>>>  >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n>>>  > + compressedContentFileName = compressedContentFile.name\n>>>  >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n>>>  >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)\n>>>  >              zf.close()\n>>>  >              compressedContentsSize = zf.infolist()[0].compress_size\n>>>  >              os.remove(contentTempFile)\n>>>  > - os.remove(compressedContentFile.name)\n>>>  > + compressedContentFile.close()\n>>>  > + os.remove(compressedContentFileName)\n>>>\n>>>  I'm not sure why NamedTemporaryFile() is called with delete=False above,\n>>>  but it appears to me that it can have delete=True instead,\n>>>  so that there is no need to call os.remove() explicitly\n>>>  and thus worry about remove vs close ordering at all.\n>>>\n>>>  >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n>>>  >                  return True\n>>>  >          return False\n>>>  > --\n>>>  > 2.21.0.windows.1\n>>>\n>>>  Thank you,\n>>>  Andrey.\n>>\n>> Thanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.\n>\n>Thank you, that would be great!\n>\n>-- \n>Andrey.\n\nUnfortunately it wasn't as simple it seemed: upon testing with only changing delete=True, \nfound that the problem was not solved. Upon further debugging, recoded/refactored slightly adding \nallocateTempFileName() locally scoped function to try to clarify how the NamedTemporaryFile()\nwas actually being used.\n\nWe can't depend on the delete-on-close because the NamedTemporaryFile() is merely allocating \na temporary name for real use by the zipfile open-for-write which fails (on Windows) if file\nwas not explicitly closed first. \n\nHopefully the new patch (https://github.com/gitgitgadget/git/pull/301) will make this more clear.\n\nOpen to other suggestions if still not clear.\n\nThanks again,\nPhilip\n\n"},{"id":"379731","messageId":"2717551564623283@vla1-822b1b47a947.qloud-c.yandex.net","threadId":"51558","inReplyTo":"BL0PR1901MB209790A0A8F5F9C8EFB8B3F0FFDF0@BL0PR1901MB2097.namprd19.prod.outlook.com","subject":"Re: [PATCH] git-p4: close temporary file before removing","fromName":"Andrey","fromEmail":"ahippo@yandex.ru","sentAt":"2019-08-01T01:34:43Z","receivedAt":"2019-08-01T01:34:48Z","isPatch":true,"sender":{"key":"ahippo@yandex.ru","avatar":null},"body":"\n\n31.07.2019, 17:52, \"Philip McGraw\" :\n> 2019.07.31 10:09 Andrey \n>> 31.07.2019, 09:53, \"Philip McGraw\" :\n>>>>   30.07.2019, 13:37, \"Philip McGraw\" :\n>>>>   > python os.remove() throws exceptions on Windows platform when attempting\n>>>>   > to remove file while it is still open. Need to grab filename while file open,\n>>>>   > close file handle, then remove by name. Apparently other platforms are more\n>>>>   > permissive of removing files while busy.\n>>>>   > reference:\n>>>>   > ---\n>>>>   >  git-p4.py | 4 +++-\n>>>>   >  1 file changed, 3 insertions(+), 1 deletion(-)\n>>>>   >\n>>>>   > diff --git a/git-p4.py b/git-p4.py\n>>>>   > index c71a6832e2..6b9d2a8317 100755\n>>>>   > --- a/git-p4.py\n>>>>   > +++ b/git-p4.py\n>>>>   > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self, relPath, contents):\n>>>>   >                  return False\n>>>>   >              contentTempFile = self.generateTempFile(contents)\n>>>>   >              compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n>>>>   > + compressedContentFileName = compressedContentFile.name\n>>>>   >              zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n>>>>   >              zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)\n>>>>   >              zf.close()\n>>>>   >              compressedContentsSize = zf.infolist()[0].compress_size\n>>>>   >              os.remove(contentTempFile)\n>>>>   > - os.remove(compressedContentFile.name)\n>>>>   > + compressedContentFile.close()\n>>>>   > + os.remove(compressedContentFileName)\n>>>>\n>>>>   I'm not sure why NamedTemporaryFile() is called with delete=False above,\n>>>>   but it appears to me that it can have delete=True instead,\n>>>>   so that there is no need to call os.remove() explicitly\n>>>>   and thus worry about remove vs close ordering at all.\n>>>>\n>>>>   >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n>>>>   >                  return True\n>>>>   >          return False\n>>>>   > --\n>>>>   > 2.21.0.windows.1\n>>>>\n>>>>   Thank you,\n>>>>   Andrey.\n>>>\n>>>  Thanks Andrey; simpler is certainly better! I will test and re-submit v2 of patch with that approach.\n>>\n>> Thank you, that would be great!\n>>\n>> --\n>> Andrey.\n>\n> Unfortunately it wasn't as simple it seemed: upon testing with only changing delete=True,\n> found that the problem was not solved. Upon further debugging, recoded/refactored slightly adding\n> allocateTempFileName() locally scoped function to try to clarify how the NamedTemporaryFile()\n> was actually being used.\n>\n> We can't depend on the delete-on-close because the NamedTemporaryFile() is merely allocating\n> a temporary name for real use by the zipfile open-for-write which fails (on Windows) if file\n> was not explicitly closed first.\n\nOh, sorry for misguiding you!\nI didn't think of this aspect.\n\n> Hopefully the new patch (https://github.com/gitgitgadget/git/pull/301) will make this more clear.\n\nThe new changeset looks good to me.\n(I'll post a reply in that thread too)\n\n> Open to other suggestions if still not clear.\n\nJust as a thought, ZipFile() can take a file-like object instead of a file name,\nso can be passed the NamedTemporaryFile() object directly instead of its file name.\nThis should hopefully avoid double-open issue on Windows.\n\nHowever, I'm good with your allocateTempFileName() changeset,\nso it's up to you to give it a try or not.\n\n> Thanks again,\n> Philip\n\nThank you,\nAndrey.\n\n"},{"id":"379745","messageId":"BL0PR1901MB2097141B412696422CE957F4FFDE0@BL0PR1901MB2097.namprd19.prod.outlook.com","threadId":"51558","inReplyTo":"2717551564623283@vla1-822b1b47a947.qloud-c.yandex.net","subject":"RE: [PATCH] git-p4: close temporary file before removing","fromName":"Philip McGraw","fromEmail":"philip.mcgraw@bentley.com","sentAt":"2019-08-01T15:30:18Z","receivedAt":"2019-08-01T15:33:02Z","isPatch":true,"sender":{"key":"philip.mcgraw@bentley.com","avatar":"https://avatars.githubusercontent.com/u/18532989?v=4"},"body":"> From: Andrey \n> Sent: Wednesday, 31 July, 2019 21:35\n> To: Philip McGraw \n> Cc: git@vger.kernel.org; luke@diamand.org\n> Subject: Re: [PATCH] git-p4: close temporary file before removing\n> \n> 31.07.2019, 17:52, \"Philip McGraw\" :\n> > 2019.07.31 10:09 Andrey \n> >> 31.07.2019, 09:53, \"Philip McGraw\" :\n> >>>>   30.07.2019, 13:37, \"Philip McGraw\" :\n> >>>>   > python os.remove() throws exceptions on Windows platform when\n> attempting\n> >>>>   > to remove file while it is still open. Need to grab filename\n> while file open,\n> >>>>   > close file handle, then remove by name. Apparently other\n> platforms are more\n> >>>>   > permissive of removing files while busy.\n> >>>>   > reference:\n> >>>>   > ---\n> >>>>   >  git-p4.py | 4 +++-\n> >>>>   >  1 file changed, 3 insertions(+), 1 deletion(-)\n> >>>>   >\n> >>>>   > diff --git a/git-p4.py b/git-p4.py\n> >>>>   > index c71a6832e2..6b9d2a8317 100755\n> >>>>   > --- a/git-p4.py\n> >>>>   > +++ b/git-p4.py\n> >>>>   > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self,\n> relPath, contents):\n> >>>>   >                  return False\n> >>>>   >              contentTempFile = self.generateTempFile(contents)\n> >>>>   >              compressedContentFile =\n> tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n> >>>>   > + compressedContentFileName = compressedContentFile.name\n> >>>>   >              zf = zipfile.ZipFile(compressedContentFile.name,\n> mode='w')\n> >>>>   >              zf.write(contentTempFile,\n> compress_type=zipfile.ZIP_DEFLATED)\n> >>>>   >              zf.close()\n> >>>>   >              compressedContentsSize =\n> zf.infolist()[0].compress_size\n> >>>>   >              os.remove(contentTempFile)\n> >>>>   > - os.remove(compressedContentFile.name)\n> >>>>   > + compressedContentFile.close()\n> >>>>   > + os.remove(compressedContentFileName)\n> >>>>\n> >>>>   I'm not sure why NamedTemporaryFile() is called with delete=False\n> above,\n> >>>>   but it appears to me that it can have delete=True instead,\n> >>>>   so that there is no need to call os.remove() explicitly\n> >>>>   and thus worry about remove vs close ordering at all.\n> >>>>\n> >>>>   >              if compressedContentsSize > gitConfigInt('git-\n> p4.largeFileCompressedThreshold'):\n> >>>>   >                  return True\n> >>>>   >          return False\n> >>>>   > --\n> >>>>   > 2.21.0.windows.1\n> >>>>\n> >>>>   Thank you,\n> >>>>   Andrey.\n> >>>\n> >>>  Thanks Andrey; simpler is certainly better! I will test and re-submit\n> v2 of patch with that approach.\n> >>\n> >> Thank you, that would be great!\n> >>\n> >> --\n> >> Andrey.\n> >\n> > Unfortunately it wasn't as simple it seemed: upon testing with only\n> changing delete=True,\n> > found that the problem was not solved. Upon further debugging,\n> recoded/refactored slightly adding\n> > allocateTempFileName() locally scoped function to try to clarify how the\n> NamedTemporaryFile()\n> > was actually being used.\n> >\n> > We can't depend on the delete-on-close because the NamedTemporaryFile()\n> is merely allocating\n> > a temporary name for real use by the zipfile open-for-write which fails\n> (on Windows) if file\n> > was not explicitly closed first.\n> \n> Oh, sorry for misguiding you!\n> I didn't think of this aspect.\n\nNo worries! I probably just misunderstood the implementation of your idea.\n> \n> > Hopefully the new patch\n> (https://urldefense.proofpoint.com/v2/url?u=https-\n> 3A__github.com_gitgitgadget_git_pull_301&d=DwIDaQ&c=hmGTLOph1qd_VnCqj81HzE\n> WkDaxmYdIWRBdoFggzhj8&r=b0ikFMJGw7xxhF3yjexiWJpLuNxlAh1SvUDuUJ-\n> pHmE&m=1jGOrV_I1Mg5ajkJ7yFEcNlyLnD6zYNXqXB9Z5SIPyE&s=TdT4WHyQCk5WZty_CvajH\n> XgrZJbmIOl1gbMcngmjmAs&e= ) will make this more clear.\n> \n> The new changeset looks good to me.\n> (I'll post a reply in that thread too)\n> \n> > Open to other suggestions if still not clear.\n> \n> Just as a thought, ZipFile() can take a file-like object instead of a file\n> name,\n> so can be passed the NamedTemporaryFile() object directly instead of its\n> file name.\n> This should hopefully avoid double-open issue on Windows.\n\nAnother excellent idea that minimizes changes. I am testing this approach\nnow and will submit v3 of the patch soon.\n> \n> However, I'm good with your allocateTempFileName() changeset,\n> so it's up to you to give it a try or not.\n> \n> > Thanks again,\n> > Philip\n> \n> Thank you,\n> Andrey.\n\nThanks,\nPhilip\n"},{"id":"379803","messageId":"28612161564717831@iva4-ba508a90b0c0.qloud-c.yandex.net","threadId":"51558","inReplyTo":"BL0PR1901MB2097141B412696422CE957F4FFDE0@BL0PR1901MB2097.namprd19.prod.outlook.com","subject":"Re: [PATCH] git-p4: close temporary file before removing","fromName":"Andrey","fromEmail":"ahippo@yandex.ru","sentAt":"2019-08-02T03:50:31Z","receivedAt":"2019-08-02T03:50:38Z","isPatch":true,"sender":{"key":"ahippo@yandex.ru","avatar":null},"body":"\n\n01.08.2019, 11:30, \"Philip McGraw\" :\n>>  From: Andrey \n>>  Sent: Wednesday, 31 July, 2019 21:35\n>>  To: Philip McGraw \n>>  Cc: git@vger.kernel.org; luke@diamand.org\n>>  Subject: Re: [PATCH] git-p4: close temporary file before removing\n>>\n>>  31.07.2019, 17:52, \"Philip McGraw\" :\n>>  > 2019.07.31 10:09 Andrey \n>>  >> 31.07.2019, 09:53, \"Philip McGraw\" :\n>>  >>>>   30.07.2019, 13:37, \"Philip McGraw\" :\n>>  >>>>   > python os.remove() throws exceptions on Windows platform when\n>>  attempting\n>>  >>>>   > to remove file while it is still open. Need to grab filename\n>>  while file open,\n>>  >>>>   > close file handle, then remove by name. Apparently other\n>>  platforms are more\n>>  >>>>   > permissive of removing files while busy.\n>>  >>>>   > reference:\n>>  >>>>   > ---\n>>  >>>>   >  git-p4.py | 4 +++-\n>>  >>>>   >  1 file changed, 3 insertions(+), 1 deletion(-)\n>>  >>>>   >\n>>  >>>>   > diff --git a/git-p4.py b/git-p4.py\n>>  >>>>   > index c71a6832e2..6b9d2a8317 100755\n>>  >>>>   > --- a/git-p4.py\n>>  >>>>   > +++ b/git-p4.py\n>>  >>>>   > @@ -1161,12 +1161,14 @@ def exceedsLargeFileThreshold(self,\n>>  relPath, contents):\n>>  >>>>   >                  return False\n>>  >>>>   >              contentTempFile = self.generateTempFile(contents)\n>>  >>>>   >              compressedContentFile =\n>>  tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n>>  >>>>   > + compressedContentFileName = compressedContentFile.name\n>>  >>>>   >              zf = zipfile.ZipFile(compressedContentFile.name,\n>>  mode='w')\n>>  >>>>   >              zf.write(contentTempFile,\n>>  compress_type=zipfile.ZIP_DEFLATED)\n>>  >>>>   >              zf.close()\n>>  >>>>   >              compressedContentsSize =\n>>  zf.infolist()[0].compress_size\n>>  >>>>   >              os.remove(contentTempFile)\n>>  >>>>   > - os.remove(compressedContentFile.name)\n>>  >>>>   > + compressedContentFile.close()\n>>  >>>>   > + os.remove(compressedContentFileName)\n>>  >>>>\n>>  >>>>   I'm not sure why NamedTemporaryFile() is called with delete=False\n>>  above,\n>>  >>>>   but it appears to me that it can have delete=True instead,\n>>  >>>>   so that there is no need to call os.remove() explicitly\n>>  >>>>   and thus worry about remove vs close ordering at all.\n>>  >>>>\n>>  >>>>   >              if compressedContentsSize > gitConfigInt('git-\n>>  p4.largeFileCompressedThreshold'):\n>>  >>>>   >                  return True\n>>  >>>>   >          return False\n>>  >>>>   > --\n>>  >>>>   > 2.21.0.windows.1\n>>  >>>>\n>>  >>>>   Thank you,\n>>  >>>>   Andrey.\n>>  >>>\n>>  >>>  Thanks Andrey; simpler is certainly better! I will test and re-submit\n>>  v2 of patch with that approach.\n>>  >>\n>>  >> Thank you, that would be great!\n>>  >>\n>>  >> --\n>>  >> Andrey.\n>>  >\n>>  > Unfortunately it wasn't as simple it seemed: upon testing with only\n>>  changing delete=True,\n>>  > found that the problem was not solved. Upon further debugging,\n>>  recoded/refactored slightly adding\n>>  > allocateTempFileName() locally scoped function to try to clarify how the\n>>  NamedTemporaryFile()\n>>  > was actually being used.\n>>  >\n>>  > We can't depend on the delete-on-close because the NamedTemporaryFile()\n>>  is merely allocating\n>>  > a temporary name for real use by the zipfile open-for-write which fails\n>>  (on Windows) if file\n>>  > was not explicitly closed first.\n>>\n>>  Oh, sorry for misguiding you!\n>>  I didn't think of this aspect.\n>\n> No worries! I probably just misunderstood the implementation of your idea.\n\nNo, you understood what I was saying correctly.\nIt's just that I didn't think of opening the file twice.\n(or rather that it would be a problem)\n\n>>  > Hopefully the new patch\n>>  (https://urldefense.proofpoint.com/v2/url?u=https-\n>>  3A__github.com_gitgitgadget_git_pull_301&d=DwIDaQ&c=hmGTLOph1qd_VnCqj81HzE\n>>  WkDaxmYdIWRBdoFggzhj8&r=b0ikFMJGw7xxhF3yjexiWJpLuNxlAh1SvUDuUJ-\n>>  pHmE&m=1jGOrV_I1Mg5ajkJ7yFEcNlyLnD6zYNXqXB9Z5SIPyE&s=TdT4WHyQCk5WZty_CvajH\n>>  XgrZJbmIOl1gbMcngmjmAs&e= ) will make this more clear.\n>>\n>>  The new changeset looks good to me.\n>>  (I'll post a reply in that thread too)\n>>\n>>  > Open to other suggestions if still not clear.\n>>\n>>  Just as a thought, ZipFile() can take a file-like object instead of a file\n>>  name,\n>>  so can be passed the NamedTemporaryFile() object directly instead of its\n>>  file name.\n>>  This should hopefully avoid double-open issue on Windows.\n>\n> Another excellent idea that minimizes changes. I am testing this approach\n> now and will submit v3 of the patch soon.\n\nThank you for willing to try the new approach!\n\n>>  However, I'm good with your allocateTempFileName() changeset,\n>>  so it's up to you to give it a try or not.\n>>\n>>  > Thanks again,\n>>  > Philip\n>>\n>>  Thank you,\n>>  Andrey.\n>\n> Thanks,\n> Philip\n\n-- \nAndrey.\n\n"}]}