{"thread":{"id":"31879","subject":"[PATCH] Fix git p4 sync errors","startedAt":"2012-10-21T01:59:29Z","lastAt":"2012-10-28T15:06:23Z","messageCount":5,"participants":["Matt Arsenault","Junio C Hamano","Christian Couder","Pete Wyckoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"201604","messageId":"AC43C2B4-623F-4590-9F92-6CCA26645EFE@gmail.com","threadId":"31879","inReplyTo":null,"subject":"[PATCH] Fix git p4 sync errors","fromName":"Matt Arsenault","fromEmail":"arsenm2@gmail.com","sentAt":"2012-10-21T01:59:29Z","receivedAt":"2012-10-21T01:59:29Z","isPatch":true,"sender":{"key":"arsenm2@gmail.com","avatar":null},"body":">From 425e4dc6992d07aa00039c5bb8e8c76def591fd3 Mon Sep 17 00:00:00 2001\nFrom: Matt Arsenault \nDate: Sat, 20 Oct 2012 18:48:45 -0700\nSubject: [PATCH] git-p4: Fix not using -s option to describe\n\nThis solves errors in some cases when syncing renamed files.\nOther places where describe is used use the -s, except this one.\n\nSigned-off-by: Matt Arsenault \n---\n git-p4.py | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 882b1bb..e203508 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2543,7 +2543,7 @@ class P4Sync(Command, P4UserMap):\n def importChanges(self, changes):\n cnt = 1\n for change in changes:\n- description = p4Cmd([\"describe\", str(change)])\n+ description = p4Cmd([\"describe\", \"-s\", str(change)])\n self.updateOptionDict(description)\n \n if not self.silent:\n-- \n1.7.12.2\n"},{"id":"201642","messageId":"7vwqyjfxwd.fsf@alter.siamese.dyndns.org","threadId":"31879","inReplyTo":"AC43C2B4-623F-4590-9F92-6CCA26645EFE@gmail.com","subject":"Re: [PATCH] Fix git p4 sync errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-21T19:06:10Z","receivedAt":"2012-10-21T19:06:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[Added area experts of git-p4 to Cc.]\n\nMatt Arsenault writes:\n\n> From 425e4dc6992d07aa00039c5bb8e8c76def591fd3 Mon Sep 17 00:00:00 2001\n> From: Matt Arsenault \n> Date: Sat, 20 Oct 2012 18:48:45 -0700\n> Subject: [PATCH] git-p4: Fix not using -s option to describe\n\nPlease do not include these four lines in the body of the message.\nThe \"Subject: \" line is there so that you can copy & paste to your\nMUA (instead of having to type something different from scratch),\nand we use the authorship From/Date from your e-mail message.\n\n>\n> This solves errors in some cases when syncing renamed files.\n\nCan you be a bit more descriptive? What are \"errors in some case\"?\n\nA good rule of thumb is to imagine a user of this command who is\nhaving a problem who reads this commit log message. Does the\nsymptom described to have fixed by the commit detailed enough to\nallow her to tell if this change may potentially fix the problem she\nis having?\n\n> Other places where describe is used use the -s, except this one.\n\nThis makes it sound as if the changed line was an odd-man out among\nmany others, and it is clear we always want \"describe -s\" throughout\nthis command. But my \"git grep 'p4.*describe'\" shows there are two\nplaces, one with \"-s\" and one without.\n\nWhat are returned from this invocation of p4Cmd() seem to be passed\nto self.splitFilesIntoBranches() as its commit[] and the fields\n'depotFile#', 'rev#', 'action#' and 'type#' are looked at in the\nmethod, and then to self.commit() as its details[], for fields\n'time', 'user', 'change', 'desc', and 'options'.\n\nWhat field, if any, gets wrong value when \"-s\" is not used? Or\nperhaps some fields are omitted incorrectly? What field are we\ngetting out of p4Cmd() that we are not using in the existing\ncallchain by not passing \"-s\" [*1*]?\n\nIn short, what I am getting at are:\n\n - What breaks by not passing \"-s\"? What are the user visible\n symptoms?\n\n - Why is it a bug not to pass \"-s\"? How does the bug happen?\n\nThanks.\n\n[footnote]\n\n*1*\n\nhttp://www.perforce.com/perforce/r12.1/manuals/cmdref/describe.html\nsays that \"describe -s\" omits files diffs, but I do not know how it\naffects \"-G describe -s\" output that we are reading (what fields in\nthe unmarshalled dict we are reading from is affected).\n\n> Signed-off-by: Matt Arsenault \n> ---\n> git-p4.py | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 882b1bb..e203508 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2543,7 +2543,7 @@ class P4Sync(Command, P4UserMap):\n> def importChanges(self, changes):\n> cnt = 1\n> for change in changes:\n> - description = p4Cmd([\"describe\", str(change)])\n> + description = p4Cmd([\"describe\", \"-s\", str(change)])\n> self.updateOptionDict(description)\n> \n> if not self.silent:\n"},{"id":"201837","messageId":"F0F92B0A-D37F-40D4-A0DF-43EEDA2818B9@gmail.com","threadId":"31879","inReplyTo":"7vwqyjfxwd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix git p4 sync errors","fromName":"Matt Arsenault","fromEmail":"arsenm2@gmail.com","sentAt":"2012-10-25T02:41:36Z","receivedAt":"2012-10-25T02:41:36Z","isPatch":true,"sender":{"key":"arsenm2@gmail.com","avatar":null},"body":"\nOn Oct 21, 2012, at 12:06 , Junio C Hamano wrote:\n> \n>> \n>> This solves errors in some cases when syncing renamed files.\n> \n> Can you be a bit more descriptive? What are \"errors in some case\"?\n> \nIt might just be when files are renamed. I ran into this after months of using it, and I'm skeptical that in that time no files were ever renamed. I'm not sure what was special about the file that was renamed. (There also might have been deleted files in the same commit, not sure if that matters)\n\n> In short, what I am getting at are:\n> \n> - What breaks by not passing \"-s\"? What are the user visible\n> symptoms?\n\nThere's a key error on the line\nline 2198: epoch = details[\"time\"]\nThe details object is an error different fields set (I don't remember what it is exactly, I'm not at work right now)\n\n> \n> - Why is it a bug not to pass \"-s\"? How does the bug happen?\n\nI encountered this one time after using it for months. One day I couldn't git p4 rebase with the key error. I searched for the error and found some version of git-p4 that fixed a similar error by adding the -s to describe. Adding the -s fixed the error and everything seemed to be working correctly.\n"},{"id":"201956","messageId":"CAP8UFD0+FPDERfkTeJWuq=T6RQ65yjHwbhTP_SYv6aA+1xq32Q@mail.gmail.com","threadId":"31879","inReplyTo":"F0F92B0A-D37F-40D4-A0DF-43EEDA2818B9@gmail.com","subject":"Re: [PATCH] Fix git p4 sync errors","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-10-26T15:44:13Z","receivedAt":"2012-10-26T15:44:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Thu, Oct 25, 2012 at 4:41 AM, Matt Arsenault wrote:\n>\n> On Oct 21, 2012, at 12:06 , Junio C Hamano wrote:\n>>\n>> - Why is it a bug not to pass \"-s\"? How does the bug happen?\n>\n> I encountered this one time after using it for months. One day I couldn't git p4 rebase\n> with the key error. I searched for the error and found some version of git-p4 that fixed\n> a similar error by adding the -s to describe. Adding the -s fixed the error and\n> everything seemed to be working correctly.\n\nThe perforce documentation says the following about this flag:\n\n\"The -s flag omits the diffs of files that were updated.\"\n\nSo if the diffs are not used, there is no downside to use -s.\nMaybe the patch should just state this.\n\nBest,\nChristian.\n"},{"id":"202047","messageId":"20121028150623.GA9249@padd.com","threadId":"31879","inReplyTo":"F0F92B0A-D37F-40D4-A0DF-43EEDA2818B9@gmail.com","subject":"Re: [PATCH] Fix git p4 sync errors","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-10-28T15:06:23Z","receivedAt":"2012-10-28T15:06:23Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"arsenm2@gmail.com wrote on Wed, 24 Oct 2012 19:41 -0700:\n> \n> On Oct 21, 2012, at 12:06 , Junio C Hamano wrote:\n> > \n> >> \n> >> This solves errors in some cases when syncing renamed files.\n> > \n> > Can you be a bit more descriptive? What are \"errors in some case\"?\n> > \n> It might just be when files are renamed. I ran into this after months of using it, and I'm skeptical that in that time no files were ever renamed. I'm not sure what was special about the file that was renamed. (There also might have been deleted files in the same commit, not sure if that matters)\n\nI set up a test case where I did a \"p4 move\" on a file and tried\nsyncing it, with and without \"-s\" to describe. It works in both\ncases, for an old (2009.2) and new (2012.1) version of p4.\n\nThe output of -s versus no -s does differ, and the differences\nare different with server version worse yet. But in no case is\nthere ever a set of file differences. -G does seem to disable\nthat.\n\nI'd love to track this down, but can't seem to provoke anything\non my own. Let me know if you have any hints based on what is in\nyour depot or server/client config. Or if you see it again.\n\n> > In short, what I am getting at are:\n> > \n> > - What breaks by not passing \"-s\"? What are the user visible\n> > symptoms?\n> \n> There's a key error on the line\n> line 2198: epoch = details[\"time\"]\n> The details object is an error different fields set (I don't remember what it is exactly, I'm not at work right now)\n\nThis would happen if describe did not return a \"time\" field, but\nthere's an explicit check for that:\n\n res = p4CmdList(\"describe -s %d\" % newestRevision)\n newestTime = None \n for r in res: \n if r.has_key('time'):\n newestTime = int(r['time'])\n if newestTime is None:\n die(\"Output from \\\"describe -s\\\" on newest change %d did not give a time\" % \n newestRevision) \n details[\"time\"] = newestTime\n\nso I'm confused how this could happen. Maybe your version is\nolder/different than what is in the git source?\n\n\nI'm not against putting in your patch, since it is true we don't\nwant the file diff, and adding \"-s\" should be harmless in theory.\nAnd it doesn't cause any existing tests to fail. It just scares\nme that there's something else going on we haven't figured out.\n\n\t\t-- Pete\n"}]}