[1/1] git-p4: auto-delete named temporary file

Commit Message

Linus Arver via GitGitGadget Aug. 26, 2019, 1:51 p.m. UTC

From: "Philip.McGraw" <[e-mail]>

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 <[e-mail]>
---
 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

             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

Comments

Junio C Hamano Aug. 26, 2019, 4:39 p.m. UTC | #1

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?

Git Gadget <[e-mail]> writes:

> From: "Philip.McGraw" <[e-mail]>
> ...
> diff --git a/git-p4.py b/git-p4.py
> index c71a6832e2..33bdb14fd1 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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)
> ...

Johannes Schindelin Aug. 28, 2019, 12:25 p.m. UTC | #2

Hi Junio,

On Mon, 26 Aug 2019, Junio C Hamano wrote:

> 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

>
> Git Gadget <[e-mail]> writes:
>
> > From: "Philip.McGraw" <[e-mail]>
> > ...
> > diff --git a/git-p4.py b/git-p4.py
> > index c71a6832e2..33bdb14fd1 100755
> > --- a/git-p4.py
> > +++ b/git-p4.py
> > @@ -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)
> > ...
>

Junio C Hamano Aug. 29, 2019, 3:57 a.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> 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 Aug. 29, 2019, 11:45 a.m. UTC | #4

Hi Junio,

On Wed, 28 Aug 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > 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

Patch

diff --git a/git-p4.py b/git-p4.py
index c71a6832e2..33bdb14fd1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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)