From: Andrey Date: Fri, 02 Aug 2019 03:50:31 GMT Subject: Re: [PATCH] git-p4: close temporary file before removing Message-ID: <28612161564717831@iva4-ba508a90b0c0.qloud-c.yandex.net> In-Reply-To: 01.08.2019, 11:30, "Philip McGraw" : >>  From: Andrey >>  Sent: Wednesday, 31 July, 2019 21:35 >>  To: Philip McGraw >>  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" : >>  > 2019.07.31 10:09 Andrey >>  >> 31.07.2019, 09:53, "Philip McGraw" : >>  >>>>   30.07.2019, 13:37, "Philip McGraw" : >>  >>>>   > 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.