[PATCH] git-p4: Fix race between p4_edit and p4_change

[PATCH] git-p4: Fix race between p4_edit and p4_change

[Kevin Green]

Hi,

Ran into a nasty race today with git-p4.  The changelist Files: section was
showing up empty and it turned out to be a race between the p4_edit and
p4_change -o, e.g.

$ p4 edit $file && p4 change -o

will show no files in the Files: section.

I attach a patch after my .sig as a suggested fix.  It simply loops over the
p4_changes -o as long as we're not finding any files (and we always should
since we just did a p4_edit!); sleeping for 3 secs in between to allow
Perforce to catch up with itself.

Thanks

--Kevin


>From 9b3b151f46dc30b9087010bb06defad9d06dfc72 Mon Sep 17 00:00:00 2001
From: Kevin Green <Kevin.Green@morganstanley.com>
Date: Tue, 1 Apr 2008 18:11:32 -0400
Subject: [PATCH] git-p4: Fix race between p4_edit and p4_change

While generating the changelist from 'p4 change -o' it's possible
that perforce hasn't caught up from the preceding 'p4 edit $file'.
This leaves us with a Files: section that is completely empty and
subsequently the p4_submit fails.

This fix loops over a flag for finding something in the Files: section.
We just did a p4_edit so there must be something there.  If nothing's
found, then sleep for a short time (3 secs) and try all over again.

Signed-off-by: Kevin Green <Kevin.Green@morganstanley.com>
---
 contrib/fast-import/git-p4 |   46 +++++++++++++++++++++++++------------------
 1 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index d8de9f6..5ae71ad 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -510,27 +510,35 @@ class P4Submit(Command):
 
     def prepareSubmitTemplate(self):
         # remove lines in the Files section that show changes to files outside the depot path we're committing into
-        template = ""
-        inFilesSection = False
-        for line in read_pipe_lines("p4 change -o"):
-            if line.endswith("\r\n"):
-                line = line[:-2] + "\n"
-            if inFilesSection:
-                if line.startswith("\t"):
-                    # path starts and ends with a tab
-                    path = line[1:]
-                    lastTab = path.rfind("\t")
-                    if lastTab != -1:
-                        path = path[:lastTab]
-                        if not path.startswith(self.depotPath):
-                            continue
+        notdone = True
+        while notdone:
+            template = ""
+            inFilesSection = False
+            for line in read_pipe_lines("p4 change -o"):
+                if line.endswith("\r\n"):
+                    line = line[:-2] + "\n"
+                if inFilesSection:
+                    if line.startswith("\t"):
+                        # path starts and ends with a tab
+                        path = line[1:]
+                        lastTab = path.rfind("\t")
+                        if lastTab != -1:
+                            path = path[:lastTab]
+                            if not path.startswith(self.depotPath):
+                                continue
+                            else:
+                                notdone = False
+                    else:
+                        inFilesSection = False
                 else:
-                    inFilesSection = False
-            else:
-                if line.startswith("Files:"):
-                    inFilesSection = True
+                    if line.startswith("Files:"):
+                        inFilesSection = True
+
+                template += line
 
-            template += line
+            # Perforce hasn't caught up with itself yet, so wait a bit and try again
+            print "Waiting for Perforce to catch up"
+            time.sleep(3)
 
         return template
 
-- 
1.5.4.2

Re: [PATCH] git-p4: Fix race between p4_edit and p4_change

[Simon Hausmann]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On Wednesday 02 April 2008 00:28:56 Kevin Green wrote:
> Hi,
>
> Ran into a nasty race today with git-p4.  The changelist Files: section was
> showing up empty and it turned out to be a race between the p4_edit and
> p4_change -o, e.g.
>
> $ p4 edit $file && p4 change -o
>
> will show no files in the Files: section.
>
> I attach a patch after my .sig as a suggested fix.  It simply loops over
> the p4_changes -o as long as we're not finding any files (and we always
> should since we just did a p4_edit!); sleeping for 3 secs in between to
> allow Perforce to catch up with itself.

I don't mind the workaround in general as I agree this race is a bit nasy, but 
shouldn't the sleep only happen if we didn't find any files? Right now even 
if the server reacted immediately we still sleep for three seconds.

Another condition could be to verify that the list of files in the files 
section is identical to the list of files we called 'p4 edit' on.

Last but not least we could of course also generate the entire Files: section 
ourselves, using 'p4 change -o' just to get the rest of the template right.

I almost prefer the last approach, since we know the base depot path and the 
relative paths of all edited/added files.

What do you think?


Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] git-p4: Fix race between p4_edit and p4_change

[Kevin Green]

On 04/03/08 14:32:32, Simon Hausmann wrote:
> On Wednesday 02 April 2008 00:28:56 Kevin Green wrote:
> > Hi,
> >
> > Ran into a nasty race today with git-p4.  The changelist Files: section was
> > showing up empty and it turned out to be a race between the p4_edit and
> > p4_change -o, e.g.
> >
> > $ p4 edit $file && p4 change -o
> >
> > will show no files in the Files: section.
> >
> > I attach a patch after my .sig as a suggested fix.  It simply loops over
> > the p4_changes -o as long as we're not finding any files (and we always
> > should since we just did a p4_edit!); sleeping for 3 secs in between to
> > allow Perforce to catch up with itself.
> 
> I don't mind the workaround in general as I agree this race is a bit nasy, but 
> shouldn't the sleep only happen if we didn't find any files? Right now even 
> if the server reacted immediately we still sleep for three seconds.
> 

Oops.  You're absolutely correct and that's not what I intended...  (darn
Python whitespace ;)

I didn't catch that logic error in my testing because I was expecting it to
sleep anyhow...

> 
> Last but not least we could of course also generate the entire Files: section 
> ourselves, using 'p4 change -o' just to get the rest of the template right.
> 
> I almost prefer the last approach, since we know the base depot path and the 
> relative paths of all edited/added files.
> 
> What do you think?
> 

Thank you...  That's the right approach.  Stop as soon as we get to the Files:
section and then just add in the depot + filepath string for each change...


--Kevin

[PATCH] git-p4: Work around race between p4_edit and p4_change

[Kevin Green]

On 04/03/08 14:45:38, Kevin Green wrote:
> On 04/03/08 14:32:32, Simon Hausmann wrote:
> > 
> > Last but not least we could of course also generate the entire Files: section 
> > ourselves, using 'p4 change -o' just to get the rest of the template right.
> > 
> > I almost prefer the last approach, since we know the base depot path and the 
> > relative paths of all edited/added files.
> > 
> > What do you think?
> > 
> 
> Thank you...  That's the right approach.  Stop as soon as we get to the Files:
> section and then just add in the depot + filepath string for each change...
> 

And here's the patch that does what we just described...


--Kevin


>From dff9c9a00e3aaf41023ff11ecc75902a87b4c16b Mon Sep 17 00:00:00 2001
From: Kevin Green <Kevin.Green@morganstanley.com>
Date: Thu, 3 Apr 2008 15:47:07 -0400
Subject: [PATCH] git-p4: Work around race between p4_edit and p4_change

There exists a race in p4, such that p4_edit immediately followed by a
p4_change will not show the new edits in the changelist template.

Instead of removing files not in our concerned depot from the Files: section
we instead use p4_change as a template only up to the Files: section and then
file in the files explicitly ourselves, since we know the full list of files
and they current state, e.g. add, delete, edit.

Signed-off-by: Kevin Green <Kevin.Green@morganstanley.com>
---
 contrib/fast-import/git-p4 |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index d8de9f6..7760764 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -511,29 +511,20 @@ class P4Submit(Command):
     def prepareSubmitTemplate(self):
         # remove lines in the Files section that show changes to files outside the depot path we're committing into
         template = ""
-        inFilesSection = False
         for line in read_pipe_lines("p4 change -o"):
             if line.endswith("\r\n"):
                 line = line[:-2] + "\n"
-            if inFilesSection:
-                if line.startswith("\t"):
-                    # path starts and ends with a tab
-                    path = line[1:]
-                    lastTab = path.rfind("\t")
-                    if lastTab != -1:
-                        path = path[:lastTab]
-                        if not path.startswith(self.depotPath):
-                            continue
-                else:
-                    inFilesSection = False
-            else:
-                if line.startswith("Files:"):
-                    inFilesSection = True
+            if line.startswith("Files:"):
+                template += line
+                break
 
             template += line
 
         return template
 
+    def addToFilesSection(self, path, type):
+        return "\t" + self.depotPath + "/" + path + "\t# " + type + "\n"
+
     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
         diffOpts = ("", "-M")[self.detectRename]
@@ -609,11 +600,19 @@ class P4Submit(Command):
 
         system(applyPatchCmd)
 
+        template = self.prepareSubmitTemplate()
+
         for f in filesToAdd:
             system("p4 add \"%s\"" % f)
+            template += self.addToFilesSection(f,"add")
+
         for f in filesToDelete:
             system("p4 revert \"%s\"" % f)
             system("p4 delete \"%s\"" % f)
+            template += self.addToFilesSection(f,"delete")
+
+        for f in editedFiles:
+            template += self.addToFilesSection(f,"edit")
 
         # Set/clear executable bits
         for f in filesToChangeExecBit.keys():
@@ -623,7 +622,6 @@ class P4Submit(Command):
         logMessage = extractLogMessageFromGitCommit(id)
         logMessage = logMessage.strip()
 
-        template = self.prepareSubmitTemplate()
 
         if self.interactive:
             submitTemplate = self.prepareLogMessage(template, logMessage)
-- 
1.5.4.2

Re: [PATCH] git-p4: Work around race between p4_edit and p4_change

[Kevin Green]

On 04/03/08 15:51:35, Kevin Green wrote:
> On 04/03/08 14:45:38, Kevin Green wrote:
> > On 04/03/08 14:32:32, Simon Hausmann wrote:
> > > 
> > > Last but not least we could of course also generate the entire Files: section 
> > > ourselves, using 'p4 change -o' just to get the rest of the template right.
> > > 
> > > I almost prefer the last approach, since we know the base depot path and the 
> > > relative paths of all edited/added files.
> > > 
> > > What do you think?
> > > 
> > 
> > Thank you...  That's the right approach.  Stop as soon as we get to the Files:
> > section and then just add in the depot + filepath string for each change...
> > 
> 
> And here's the patch that does what we just described...
> 

Haven't heard comment back on this patch.  Am wondering if it will be applied
to git, or if I need to start thinking about maintaining it myself on my end,
or if it's not appropriate and I should re-submit something else.


Thanks

--Kevin