git-p4: auto-delete named temporary file

PATCH Superseded Philip McGraw via GitGitGadget 5 participants 12 messages Aug 1, 2019 – Oct 6, 2019 1 message not in the archive

lore

Share & tools

lore

Philip McGraw via GitGitGadget

author

2019-08-01 21:39 UTC #0 lore raw

cover git-p4: auto-delete named temporary file

Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.

Original code produced double-open problems on Windows platform from using already open NamedTemporaryFile() generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!

Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."

Signed-off-by Philip.McGraw [email protected]

[[email protected]]

Philip.McGraw (1):
git-p4: auto-delete named temporary file

 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-303%2Fphilip-mcgraw%2Fgit-p4-auto-delete-named-temporary-file-v3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-303/philip-mcgraw/git-p4-auto-delete-named-temporary-file-v3-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/303

Signature · 1 line

gitgitgadget

Andrey

Reviewed-by Andrey Mazo on the v1 series

2019-08-02 03:48 UTC #1 lore raw

01.08.2019, 17:39, "Philip McGraw via GitGitGadget" <[email protected]>:

Quoted 20 lines

Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.
Original code produced double-open problems on Windows platform from using already open NamedTemporaryFile() generated filename instead of object.
Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!
Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."
Signed-off-by Philip.McGraw [email protected]
[[email protected]]
Philip.McGraw (1):
git-p4: auto-delete named temporary file
git-p4.py | 13 ++++++-------

Quoted 8 lines

 1 file changed, 6 insertions(+), 7 deletions(-)
base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-303%2Fphilip-mcgraw%2Fgit-p4-auto-delete-named-temporary-file-v3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-303/philip-mcgraw/git-p4-auto-delete-named-temporary-file-v3-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/303
--
gitgitgadget

Looks good to me!

Reviewed-by Andrey Mazo <[email protected]>

Signature · 1 line

Andrey.

Philip McGraw via GitGitGadget

author

2019-08-02 19:43 UTC #2 lore raw

v2 cover git-p4: auto-delete named temporary file

In reply to Philip McGraw via GitGitGadget

Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.

Original code produced double-open problems on Windows platform from using already open NamedTemporaryFile() generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!

Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."

Signed-off-by Philip.McGraw [email protected]

[[email protected]]

Philip.McGraw (1):
git-p4: auto-delete named temporary file

 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-303%2Fphilip-mcgraw%2Fgit-p4-auto-delete-named-temporary-file-v3-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-303/philip-mcgraw/git-p4-auto-delete-named-temporary-file-v3-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/303

Range-diff vs v1:

1: 1b270ef9a6 ! 1: 7e59b5cec2 git-p4: auto-delete named temporary file
@@ -2,13 +2,17 @@

git-p4: auto-delete named temporary file

+ Avoid double-open exceptions on Windows platform when
+ calculating for lfs compressed size threshold
+ (git-p4.largeFileCompressedThreshold) comparisons.
+
Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.

- Original code produced double-open problems on Windows
- platform from using already open NamedTemporaryFile()
- generated filename instead of object.
+ Original code had double-open exception on Windows
+ platform because file still open from NamedTemporaryFile()
+ using generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!
@@ -18,6 +22,7 @@
nothing to do with "git version recent enough."

Signed-off-by Philip.McGraw <[email protected]>

+ Reviewed-by: Andrey Mazo <[email protected]>

diff --git a/git-p4.py b/git-p4.py
--- a/git-p4.py

Signature · 1 line

gitgitgadget

Git Gadget

author

2019-08-26 13:51 UTC #8 lore raw

PATCH 1/1 git-p4: auto-delete named temporary file

In reply to Philip McGraw via GitGitGadget

From: "Philip.McGraw" <[email protected]>

Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.

Original code produced double-open problems on Windows platform from using already open NamedTemporaryFile() generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!

Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."

Signed-off-by Philip.McGraw <[email protected]>

---
 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

git-p4.py +0 -0

index c71a6832e2..33bdb14fd1 100755
@@ -1160,13 +1160,11 @@ def exceedsLargeFileThreshold(self, relPath, contents):
             if contentsSize <=

gitConfigInt('git-p4.largeFileCompressedThreshold'):
return False
contentTempFile = self.generateTempFile(contents)
- compressedContentFile =
tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)

-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
-            zf.close()
-            compressedContentsSize = zf.infolist()[0].compress_size
+            compressedContentFile =

tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
+ with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
+ zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
+ compressedContentsSize = zf.infolist()[0].compress_size
os.remove(contentTempFile)
- os.remove(compressedContentFile.name)
if compressedContentsSize >
gitConfigInt('git-p4.largeFileCompressedThreshold'):
return True
return False
@@ -3514,8 +3512,9 @@ def importHeadRevision(self, revision):
self.updateOptionDict(details)
try:
self.commit(details,
self.extractFilesFromCommit(details), self.branch)
- except IOError:
+ except IOError as err:
print("IO error with git fast-import. Is your git version
recent enough?")
+ print("IO error details: {}".format(err))
print(self.gitError.read())

def openStreams(self):

--
gitgitgadget

Junio C Hamano

2019-08-26 16:39 UTC #9 lore raw

Funny that the patch is line-wrapped, which I do not recall ever seeing in GGG-generated e-mails. Dscho, do you know if anything funny is going on?

Quoted 16 lines · Git Gadget

From: "Philip.McGraw" <[email protected]>
...
git-p4.py +0 -0
index c71a6832e2..33bdb14fd1 100755
@@ -1160,13 +1160,11 @@ def exceedsLargeFileThreshold(self, relPath, contents):
             if contentsSize <=
gitConfigInt('git-p4.largeFileCompressedThreshold'):
return False
contentTempFile = self.generateTempFile(contents)
- compressedContentFile =
tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
git-p4.py +0 -2
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
...

Andrey Mazo

2019-08-27 03:44 UTC #3 lore raw

PATCH v2 1/1 git-p4: auto-delete named temporary file

In reply to Philip McGraw via GitGitGadget

From: "Philip.McGraw" <[email protected]>

Avoid double-open exceptions on Windows platform when calculating for lfs compressed size threshold (git-p4.largeFileCompressedThreshold) comparisons.

Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.

Original code had double-open exception on Windows platform because file still open from NamedTemporaryFile() using generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!

Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."

Signed-off-by Philip.McGraw <[email protected]>

Reviewed-by Andrey Mazo <[email protected]>

---
 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

git-p4.py +6 -7

index c71a6832e2..33bdb14fd1 100755
@@ -1158,17 +1158,15 @@ def exceedsLargeFileThreshold(self, relPath, contents):
         if gitConfigInt('git-p4.largeFileCompressedThreshold'):
             contentsSize = sum(len(d) for d in contents)
             if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return False
             contentTempFile = self.generateTempFile(contents)
-            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
-            zf.close()
-            compressedContentsSize = zf.infolist()[0].compress_size
+            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
+            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
+                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
+                compressedContentsSize = zf.infolist()[0].compress_size
             os.remove(contentTempFile)
-            os.remove(compressedContentFile.name)
             if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return True
         return False
 
     def addLargeFile(self, relPath):

@@ -3512,12 +3510,13 @@ def importHeadRevision(self, revision):
         details["time"] = res["time"]
 
         self.updateOptionDict(details)
         try:
             self.commit(details, self.extractFilesFromCommit(details), self.branch)
-        except IOError:
+        except IOError as err:
             print("IO error with git fast-import. Is your git version recent enough?")
+            print("IO error details: {}".format(err))
             print(self.gitError.read())
 
     def openStreams(self):
         self.importProcess = subprocess.Popen(["git", "fast-import"],
                                               stdin=subprocess.PIPE,

base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Pull-Request: https://github.com/gitgitgadget/git/pull/303

Signature · 1 line

2.21.0

Junio C Hamano

2019-08-27 22:31 UTC #4 lore raw

Quoted 17 lines · Andrey Mazo

From: "Philip.McGraw" <[email protected]>
Avoid double-open exceptions on Windows platform when calculating for lfs compressed size threshold (git-p4.largeFileCompressedThreshold) comparisons.
Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.
Original code had double-open exception on Windows platform because file still open from NamedTemporaryFile() using generated filename instead of object.
Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!

Quoted 7 lines

Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."
Signed-off-by Philip.McGraw <[email protected]>
Reviewed-by Andrey Mazo <[email protected]>
---

Luke, does this look good?

I know Mazo is the only other contributor who has multiple commits to git-p4.py in the past 2 years, to make Reviewed-by carry some weight ;-) but as we have so small number of people touching this script anyway, I'd rather see what the main contributor in the past 2 years thinks.

Thanks.

Quoted 47 lines

 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

git-p4.py +6 -7

index c71a6832e2..33bdb14fd1 100755
@@ -1158,17 +1158,15 @@ def exceedsLargeFileThreshold(self, relPath, contents):
         if gitConfigInt('git-p4.largeFileCompressedThreshold'):
             contentsSize = sum(len(d) for d in contents)
             if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return False
             contentTempFile = self.generateTempFile(contents)
-            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
-            zf.close()
-            compressedContentsSize = zf.infolist()[0].compress_size
+            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
+            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
+                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
+                compressedContentsSize = zf.infolist()[0].compress_size
             os.remove(contentTempFile)
-            os.remove(compressedContentFile.name)
             if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return True
         return False
 
     def addLargeFile(self, relPath):

@@ -3512,12 +3510,13 @@ def importHeadRevision(self, revision):
         details["time"] = res["time"]
 
         self.updateOptionDict(details)
         try:
             self.commit(details, self.extractFilesFromCommit(details), self.branch)
-        except IOError:
+        except IOError as err:
             print("IO error with git fast-import. Is your git version recent enough?")
+            print("IO error details: {}".format(err))
             print(self.gitError.read())
 
     def openStreams(self):
         self.importProcess = subprocess.Popen(["git", "fast-import"],
                                               stdin=subprocess.PIPE,

base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Pull-Request: https://github.com/gitgitgadget/git/pull/303

Luke Diamand

2019-08-28 08:34 UTC #5 lore raw

Quoted 28 lines · Junio C Hamano

Quoted 17 lines · Andrey Mazo
From: "Philip.McGraw" <[email protected]>
Avoid double-open exceptions on Windows platform when calculating for lfs compressed size threshold (git-p4.largeFileCompressedThreshold) comparisons.
Take new approach using the NamedTemporaryFile() file-like object as input to the ZipFile() which auto-deletes after implicit close leaving with scope.
Original code had double-open exception on Windows platform because file still open from NamedTemporaryFile() using generated filename instead of object.
Thanks to Andrey for patiently suggesting several iterations on this change for avoiding exceptions!
Quoted 7 lines
Also print error details after resulting IOError to make debugging cause of exception less mysterious when it has nothing to do with "git version recent enough."
Signed-off-by Philip.McGraw <[email protected]>
Reviewed-by Andrey Mazo <[email protected]>
---

Quoted 7 lines

Luke, does this look good?
I know Mazo is the only other contributor who has multiple commits to git-p4.py in the past 2 years, to make Reviewed-by carry some weight ;-) but as we have so small number of people touching this script anyway, I'd rather see what the main contributor in the past 2 years thinks.

I think it looks reasonable.

Ack.

Quoted 50 lines

Thanks.

Quoted 47 lines

 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

git-p4.py +6 -7

index c71a6832e2..33bdb14fd1 100755
@@ -1158,17 +1158,15 @@ def exceedsLargeFileThreshold(self, relPath, contents):
         if gitConfigInt('git-p4.largeFileCompressedThreshold'):
             contentsSize = sum(len(d) for d in contents)
             if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return False
             contentTempFile = self.generateTempFile(contents)
-            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
-            zf.close()
-            compressedContentsSize = zf.infolist()[0].compress_size
+            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
+            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
+                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
+                compressedContentsSize = zf.infolist()[0].compress_size
             os.remove(contentTempFile)
-            os.remove(compressedContentFile.name)
             if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return True
         return False
 
     def addLargeFile(self, relPath):

@@ -3512,12 +3510,13 @@ def importHeadRevision(self, revision):
         details["time"] = res["time"]
 
         self.updateOptionDict(details)
         try:
             self.commit(details, self.extractFilesFromCommit(details), self.branch)
-        except IOError:
+        except IOError as err:
             print("IO error with git fast-import. Is your git version recent enough?")
+            print("IO error details: {}".format(err))
             print(self.gitError.read())
 
     def openStreams(self):
         self.importProcess = subprocess.Popen(["git", "fast-import"],
                                               stdin=subprocess.PIPE,

base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Pull-Request: https://github.com/gitgitgadget/git/pull/303

Johannes Schindelin

2019-08-28 12:25 UTC #10 lore raw

In reply to Junio C Hamano

Hi Junio,

Quoted 3 lines · Junio C Hamano

Funny that the patch is line-wrapped, which I do not recall ever seeing in GGG-generated e-mails. Dscho, do you know if anything funny is going on?

Yes, this was me trying to re-send the patch via GMail's web UI because the first time GitGitGadget sent it, it did not get through (only the cover letter did).

So I tried to fix the screw-up by sending manually, and screwed it up even more.

Sorry about that.
Dscho

Quoted 20 lines

Quoted 16 lines · Git Gadget
From: "Philip.McGraw" <[email protected]>
...
git-p4.py +0 -0
index c71a6832e2..33bdb14fd1 100755
@@ -1160,13 +1160,11 @@ def exceedsLargeFileThreshold(self, relPath, contents):
             if contentsSize <=
gitConfigInt('git-p4.largeFileCompressedThreshold'):
return False
contentTempFile = self.generateTempFile(contents)
- compressedContentFile =
tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
git-p4.py +0 -2
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
...

Junio C Hamano

2019-08-29 03:57 UTC #11 lore raw

Quoted 3 lines · Johannes Schindelin

Yes, this was me trying to re-send the patch via GMail's web UI because the first time GitGitGadget sent it, it did not get through (only the cover letter did).

As long as that was manual screw-up, while fixing some glitches in the machinery, that is fine. I care about automation running smoothly.

Thanks.

Johannes Schindelin

2019-08-29 11:45 UTC #12 lore raw

Hi Junio,

Quoted 9 lines · Junio C Hamano

Quoted 3 lines · Johannes Schindelin
Yes, this was me trying to re-send the patch via GMail's web UI because the first time GitGitGadget sent it, it did not get through (only the cover letter did).
As long as that was manual screw-up, while fixing some glitches in the machinery, that is fine. I care about automation running smoothly.

Indeed, I also care about automation, as that keeps me sane (I would not be able to do everything I do without offloading substantial parts to Azure Pipelines, like GitGitGadget).

I am quite a bit worried about this, as it seemed to happen at least three times. I'll see if it happens again, and when it does, I will try to save the mbox files as build artifacts, and then add another job that I can run manually to re-send the mbox files that did not make it to the mailing list (identified by the absence from public-inbox).

At the moment, I am still rather busy elsewhere, trying to catch up after three weeks that I spent (or tried to spend) mostly offline.

And I am really sorry for not writing a reply to the thread when I tried to re-send the patch manually, describing what I was doing, and why. I will try to do better next time!

Ciao,
Dscho

Junio C Hamano

2019-10-06 02:43 UTC #7 lore raw

In reply to a message not in the archive

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
...
Luke, does this look good?
I know Mazo is the only other contributor who has multiple commits to git-p4.py in the past 2 years, to make Reviewed-by carry some weight ;-) but as we have so small number of people touching this script anyway, I'd rather see what the main contributor in the past 2 years thinks.

Quoted 6 lines

Quoted 3 lines
I think it looks reasonable.
Ack.
Junio, does Philip need to resend a v3 with Luke's ack or will you just add it yourself while queuing the patch? (sorry, if you already picked up the patch -- I just didn't see it in the last What's cooking update)

Sorry, I do not recall the details of this old a thread but if I recall correctly there was only a corrupt patch that I cannot use, so somebody would need to send a patch that actually applies. With a patch that applies, I can tweak the log message
with acked-by etc. just fine.

Thanks.