{"thread":{"id":"12955","subject":"[PATCH] git-p4: Fix race between p4_edit and p4_change","startedAt":"2008-04-01T22:28:56Z","lastAt":"2008-04-11T16:27:59Z","messageCount":5,"participants":["Kevin Green","Simon Hausmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"73517","messageId":"20080401222856.GA22542@morganstanley.com","threadId":"12955","inReplyTo":null,"subject":"[PATCH] git-p4: Fix race between p4_edit and p4_change","fromName":"Kevin Green","fromEmail":"kevin.t.green@morganstanley.com","sentAt":"2008-04-01T22:28:56Z","receivedAt":"2008-04-01T22:28:56Z","isPatch":true,"sender":{"key":"kevin.t.green@morganstanley.com","avatar":null},"body":"\nHi,\n\nRan into a nasty race today with git-p4.  The changelist Files: section was\nshowing up empty and it turned out to be a race between the p4_edit and\np4_change -o, e.g.\n\n$ p4 edit $file && p4 change -o\n\nwill show no files in the Files: section.\n\nI attach a patch after my .sig as a suggested fix.  It simply loops over the\np4_changes -o as long as we're not finding any files (and we always should\nsince we just did a p4_edit!); sleeping for 3 secs in between to allow\nPerforce to catch up with itself.\n\nThanks\n\n--Kevin\n\n\n>From 9b3b151f46dc30b9087010bb06defad9d06dfc72 Mon Sep 17 00:00:00 2001\nFrom: Kevin Green <Kevin.Green@morganstanley.com>\nDate: Tue, 1 Apr 2008 18:11:32 -0400\nSubject: [PATCH] git-p4: Fix race between p4_edit and p4_change\n\nWhile generating the changelist from 'p4 change -o' it's possible\nthat perforce hasn't caught up from the preceding 'p4 edit $file'.\nThis leaves us with a Files: section that is completely empty and\nsubsequently the p4_submit fails.\n\nThis fix loops over a flag for finding something in the Files: section.\nWe just did a p4_edit so there must be something there.  If nothing's\nfound, then sleep for a short time (3 secs) and try all over again.\n\nSigned-off-by: Kevin Green <Kevin.Green@morganstanley.com>\n---\n contrib/fast-import/git-p4 |   46 +++++++++++++++++++++++++------------------\n 1 files changed, 27 insertions(+), 19 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex d8de9f6..5ae71ad 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -510,27 +510,35 @@ class P4Submit(Command):\n \n     def prepareSubmitTemplate(self):\n         # remove lines in the Files section that show changes to files outside the depot path we're committing into\n-        template = \"\"\n-        inFilesSection = False\n-        for line in read_pipe_lines(\"p4 change -o\"):\n-            if line.endswith(\"\\r\\n\"):\n-                line = line[:-2] + \"\\n\"\n-            if inFilesSection:\n-                if line.startswith(\"\\t\"):\n-                    # path starts and ends with a tab\n-                    path = line[1:]\n-                    lastTab = path.rfind(\"\\t\")\n-                    if lastTab != -1:\n-                        path = path[:lastTab]\n-                        if not path.startswith(self.depotPath):\n-                            continue\n+        notdone = True\n+        while notdone:\n+            template = \"\"\n+            inFilesSection = False\n+            for line in read_pipe_lines(\"p4 change -o\"):\n+                if line.endswith(\"\\r\\n\"):\n+                    line = line[:-2] + \"\\n\"\n+                if inFilesSection:\n+                    if line.startswith(\"\\t\"):\n+                        # path starts and ends with a tab\n+                        path = line[1:]\n+                        lastTab = path.rfind(\"\\t\")\n+                        if lastTab != -1:\n+                            path = path[:lastTab]\n+                            if not path.startswith(self.depotPath):\n+                                continue\n+                            else:\n+                                notdone = False\n+                    else:\n+                        inFilesSection = False\n                 else:\n-                    inFilesSection = False\n-            else:\n-                if line.startswith(\"Files:\"):\n-                    inFilesSection = True\n+                    if line.startswith(\"Files:\"):\n+                        inFilesSection = True\n+\n+                template += line\n \n-            template += line\n+            # Perforce hasn't caught up with itself yet, so wait a bit and try again\n+            print \"Waiting for Perforce to catch up\"\n+            time.sleep(3)\n \n         return template\n \n-- \n1.5.4.2\n"},{"id":"73618","messageId":"200804032032.39860.simon@lst.de","threadId":"12955","inReplyTo":"20080401222856.GA22542@morganstanley.com","subject":"Re: [PATCH] git-p4: Fix race between p4_edit and p4_change","fromName":"Simon Hausmann","fromEmail":"simon@lst.de","sentAt":"2008-04-03T18:32:32Z","receivedAt":"2008-04-03T18:32:32Z","isPatch":true,"sender":{"key":"hausmann@kde.org","avatar":"https://gravatar.com/avatar/bc9aad4fb31dce17eb66e690e7b51fe980c62da3c225c785da35dd806b8da778?d=mp&s=160"},"body":"On Wednesday 02 April 2008 00:28:56 Kevin Green wrote:\n> Hi,\n>\n> Ran into a nasty race today with git-p4.  The changelist Files: section was\n> showing up empty and it turned out to be a race between the p4_edit and\n> p4_change -o, e.g.\n>\n> $ p4 edit $file && p4 change -o\n>\n> will show no files in the Files: section.\n>\n> I attach a patch after my .sig as a suggested fix.  It simply loops over\n> the p4_changes -o as long as we're not finding any files (and we always\n> should since we just did a p4_edit!); sleeping for 3 secs in between to\n> allow Perforce to catch up with itself.\n\nI don't mind the workaround in general as I agree this race is a bit nasy, but \nshouldn't the sleep only happen if we didn't find any files? Right now even \nif the server reacted immediately we still sleep for three seconds.\n\nAnother condition could be to verify that the list of files in the files \nsection is identical to the list of files we called 'p4 edit' on.\n\nLast but not least we could of course also generate the entire Files: section \nourselves, using 'p4 change -o' just to get the rest of the template right.\n\nI almost prefer the last approach, since we know the base depot path and the \nrelative paths of all edited/added files.\n\nWhat do you think?\n\n\nSimon\n"},{"id":"73619","messageId":"20080403184537.GH22542@morganstanley.com","threadId":"12955","inReplyTo":"200804032032.39860.simon@lst.de","subject":"Re: [PATCH] git-p4: Fix race between p4_edit and p4_change","fromName":"Kevin Green","fromEmail":"kevin.t.green@morganstanley.com","sentAt":"2008-04-03T18:45:38Z","receivedAt":"2008-04-03T18:45:38Z","isPatch":true,"sender":{"key":"kevin.t.green@morganstanley.com","avatar":null},"body":"On 04/03/08 14:32:32, Simon Hausmann wrote:\n> On Wednesday 02 April 2008 00:28:56 Kevin Green wrote:\n> > Hi,\n> >\n> > Ran into a nasty race today with git-p4.  The changelist Files: section was\n> > showing up empty and it turned out to be a race between the p4_edit and\n> > p4_change -o, e.g.\n> >\n> > $ p4 edit $file && p4 change -o\n> >\n> > will show no files in the Files: section.\n> >\n> > I attach a patch after my .sig as a suggested fix.  It simply loops over\n> > the p4_changes -o as long as we're not finding any files (and we always\n> > should since we just did a p4_edit!); sleeping for 3 secs in between to\n> > allow Perforce to catch up with itself.\n> \n> I don't mind the workaround in general as I agree this race is a bit nasy, but \n> shouldn't the sleep only happen if we didn't find any files? Right now even \n> if the server reacted immediately we still sleep for three seconds.\n> \n\nOops.  You're absolutely correct and that's not what I intended...  (darn\nPython whitespace ;)\n\nI didn't catch that logic error in my testing because I was expecting it to\nsleep anyhow...\n\n> \n> Last but not least we could of course also generate the entire Files: section \n> ourselves, using 'p4 change -o' just to get the rest of the template right.\n> \n> I almost prefer the last approach, since we know the base depot path and the \n> relative paths of all edited/added files.\n> \n> What do you think?\n> \n\nThank you...  That's the right approach.  Stop as soon as we get to the Files:\nsection and then just add in the depot + filepath string for each change...\n\n\n--Kevin\n"},{"id":"73621","messageId":"20080403195135.GI22542@morganstanley.com","threadId":"12955","inReplyTo":"20080403184537.GH22542@morganstanley.com","subject":"[PATCH] git-p4: Work around race between p4_edit and p4_change","fromName":"Kevin Green","fromEmail":"kevin.t.green@morganstanley.com","sentAt":"2008-04-03T19:51:35Z","receivedAt":"2008-04-03T19:51:35Z","isPatch":true,"sender":{"key":"kevin.t.green@morganstanley.com","avatar":null},"body":"On 04/03/08 14:45:38, Kevin Green wrote:\n> On 04/03/08 14:32:32, Simon Hausmann wrote:\n> > \n> > Last but not least we could of course also generate the entire Files: section \n> > ourselves, using 'p4 change -o' just to get the rest of the template right.\n> > \n> > I almost prefer the last approach, since we know the base depot path and the \n> > relative paths of all edited/added files.\n> > \n> > What do you think?\n> > \n> \n> Thank you...  That's the right approach.  Stop as soon as we get to the Files:\n> section and then just add in the depot + filepath string for each change...\n> \n\nAnd here's the patch that does what we just described...\n\n\n--Kevin\n\n\n>From dff9c9a00e3aaf41023ff11ecc75902a87b4c16b Mon Sep 17 00:00:00 2001\nFrom: Kevin Green <Kevin.Green@morganstanley.com>\nDate: Thu, 3 Apr 2008 15:47:07 -0400\nSubject: [PATCH] git-p4: Work around race between p4_edit and p4_change\n\nThere exists a race in p4, such that p4_edit immediately followed by a\np4_change will not show the new edits in the changelist template.\n\nInstead of removing files not in our concerned depot from the Files: section\nwe instead use p4_change as a template only up to the Files: section and then\nfile in the files explicitly ourselves, since we know the full list of files\nand they current state, e.g. add, delete, edit.\n\nSigned-off-by: Kevin Green <Kevin.Green@morganstanley.com>\n---\n contrib/fast-import/git-p4 |   30 ++++++++++++++----------------\n 1 files changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex d8de9f6..7760764 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -511,29 +511,20 @@ class P4Submit(Command):\n     def prepareSubmitTemplate(self):\n         # remove lines in the Files section that show changes to files outside the depot path we're committing into\n         template = \"\"\n-        inFilesSection = False\n         for line in read_pipe_lines(\"p4 change -o\"):\n             if line.endswith(\"\\r\\n\"):\n                 line = line[:-2] + \"\\n\"\n-            if inFilesSection:\n-                if line.startswith(\"\\t\"):\n-                    # path starts and ends with a tab\n-                    path = line[1:]\n-                    lastTab = path.rfind(\"\\t\")\n-                    if lastTab != -1:\n-                        path = path[:lastTab]\n-                        if not path.startswith(self.depotPath):\n-                            continue\n-                else:\n-                    inFilesSection = False\n-            else:\n-                if line.startswith(\"Files:\"):\n-                    inFilesSection = True\n+            if line.startswith(\"Files:\"):\n+                template += line\n+                break\n \n             template += line\n \n         return template\n \n+    def addToFilesSection(self, path, type):\n+        return \"\\t\" + self.depotPath + \"/\" + path + \"\\t# \" + type + \"\\n\"\n+\n     def applyCommit(self, id):\n         print \"Applying %s\" % (read_pipe(\"git log --max-count=1 --pretty=oneline %s\" % id))\n         diffOpts = (\"\", \"-M\")[self.detectRename]\n@@ -609,11 +600,19 @@ class P4Submit(Command):\n \n         system(applyPatchCmd)\n \n+        template = self.prepareSubmitTemplate()\n+\n         for f in filesToAdd:\n             system(\"p4 add \\\"%s\\\"\" % f)\n+            template += self.addToFilesSection(f,\"add\")\n+\n         for f in filesToDelete:\n             system(\"p4 revert \\\"%s\\\"\" % f)\n             system(\"p4 delete \\\"%s\\\"\" % f)\n+            template += self.addToFilesSection(f,\"delete\")\n+\n+        for f in editedFiles:\n+            template += self.addToFilesSection(f,\"edit\")\n \n         # Set/clear executable bits\n         for f in filesToChangeExecBit.keys():\n@@ -623,7 +622,6 @@ class P4Submit(Command):\n         logMessage = extractLogMessageFromGitCommit(id)\n         logMessage = logMessage.strip()\n \n-        template = self.prepareSubmitTemplate()\n \n         if self.interactive:\n             submitTemplate = self.prepareLogMessage(template, logMessage)\n-- \n1.5.4.2\n"},{"id":"74127","messageId":"20080411162759.GO22542@morganstanley.com","threadId":"12955","inReplyTo":"20080403195135.GI22542@morganstanley.com","subject":"Re: [PATCH] git-p4: Work around race between p4_edit and p4_change","fromName":"Kevin Green","fromEmail":"kevin.t.green@morganstanley.com","sentAt":"2008-04-11T16:27:59Z","receivedAt":"2008-04-11T16:27:59Z","isPatch":true,"sender":{"key":"kevin.t.green@morganstanley.com","avatar":null},"body":"On 04/03/08 15:51:35, Kevin Green wrote:\n> On 04/03/08 14:45:38, Kevin Green wrote:\n> > On 04/03/08 14:32:32, Simon Hausmann wrote:\n> > > \n> > > Last but not least we could of course also generate the entire Files: section \n> > > ourselves, using 'p4 change -o' just to get the rest of the template right.\n> > > \n> > > I almost prefer the last approach, since we know the base depot path and the \n> > > relative paths of all edited/added files.\n> > > \n> > > What do you think?\n> > > \n> > \n> > Thank you...  That's the right approach.  Stop as soon as we get to the Files:\n> > section and then just add in the depot + filepath string for each change...\n> > \n> \n> And here's the patch that does what we just described...\n> \n\nHaven't heard comment back on this patch.  Am wondering if it will be applied\nto git, or if I need to start thinking about maintaining it myself on my end,\nor if it's not appropriate and I should re-submit something else.\n\n\nThanks\n\n--Kevin\n"}]}