{"thread":{"id":"46157","subject":"[PATCH] Fix KeyError \"fileSize\" in verbose mode","startedAt":"2017-06-12T08:02:39Z","lastAt":"2017-06-13T07:35:15Z","messageCount":3,"participants":["Sergey Yurzin","Junio C Hamano","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"321949","messageId":"0102015c9b535a84-fd59d55d-387a-419e-b8ec-439873c4b7f5-000000@eu-west-1.amazonses.com","threadId":"46157","inReplyTo":null,"subject":"[PATCH] Fix KeyError \"fileSize\" in verbose mode","fromName":"Sergey Yurzin","fromEmail":"jurzin.s@gmail.com","sentAt":"2017-06-12T08:02:30Z","receivedAt":"2017-06-12T08:02:39Z","isPatch":true,"sender":{"key":"jurzin.s@gmail.com","avatar":null},"body":"From: Sergei Iurzin \n\n---\n git-p4.py | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da91b969..b3666eddf12e3 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents):\n relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)\n relPath = self.encodeWithUTF8(relPath)\n if verbose:\n- size = int(self.stream_file['fileSize'])\n- sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n+ if 'fileSize' in self.stream_file:\n+ size = int(self.stream_file['fileSize'])\n+ sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n+ else:\n+ sys.stdout.write('\\r%s --> %s\\n' % (file['depotFile'], relPath))\n sys.stdout.flush()\n \n (type_base, type_mods) = split_p4_type(file[\"type\"])\n\n--\nhttps://github.com/git/git/pull/373\n"},{"id":"321966","messageId":"xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com","threadId":"46157","inReplyTo":"0102015c9b535a84-fd59d55d-387a-419e-b8ec-439873c4b7f5-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] Fix KeyError \"fileSize\" in verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-12T17:11:16Z","receivedAt":"2017-06-12T17:11:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Yurzin writes:\n\n> Subject: Re: [PATCH] Fix KeyError \"fileSize\" in verbose mode\n\nThe convention around here is to think how a single change appears\nin \"git shortlog --no-merges\" output. The above commit title does\nnot tell readers of \"shortlog\" what the change is about in the\ncontext of the whole system.\n\n Subject: [PATCH] git-p4: do not fail in verbose mode for missing `fileSize`\n\nor something?\n\n> From: Sergei Iurzin \n>\n> ---\n\nThe blank space above is to explain why this change is needed and a\ngood idea, followed by your Signed-off-by: line (please see\nDocumentation/SubmittingPatches).\n\n> git-p4.py | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 8d151da91b969..b3666eddf12e3 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents):\n> relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)\n> relPath = self.encodeWithUTF8(relPath)\n> if verbose:\n> - size = int(self.stream_file['fileSize'])\n> - sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n> + if 'fileSize' in self.stream_file:\n> + size = int(self.stream_file['fileSize'])\n> + sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n> + else:\n> + sys.stdout.write('\\r%s --> %s\\n' % (file['depotFile'], relPath))\n> sys.stdout.flush()\n\nI can see from your patch that self.stream_file[] sometimes may not\nhave `fileSize` and when that happens the current code will barf. I\nalso can see that with your patch, the code will NOT barf but output\nwould lack the size information (obviously, because it is not\navailable).\n\nHowever, it is not at all obvious if this is fixing the problem or\nsweeping the problem under the rug. The proposed log message you\nwrite before the patch is the ideal place to say something like\n\n In such and such circumstances, it is perfectly normal that\n P4Sync.stream_file does not know its file and lacks `fileSize`.\n streamOneP4File() method, however, assumes that this key is\n always available and tries to write it under the verbose mode.\n\n Check the existence of the `fileSize` key and show it only when\n available.\n\nNote that the above _assumes_ that a stream_file that lacks\n`fileSize` is perfectly normal; if that assumption is incorrect,\nthen perhaps a real fix may be to set `fileSize` in the codepath\nthat forgets to set it (I am not a git-p4 expert, and I am asking\nLuke to review this patch by CC'ing him).\n\nAlso note that in a real log message that is helpful for future\nreaders, \"In such and such circumstances\" in the above illustration\nneeds to become a more concrete description.\n\nThanks, and welcome to Git development community.\n\n> (type_base, type_mods) = split_p4_type(file[\"type\"])\n>\n> --\n> https://github.com/git/git/pull/373\n"},{"id":"322054","messageId":"B3689D6E-A6B8-42F8-9105-242178FE16E0@gmail.com","threadId":"46157","inReplyTo":"xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Fix KeyError \"fileSize\" in verbose mode","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-13T07:35:06Z","receivedAt":"2017-06-13T07:35:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Jun 2017, at 19:11, Junio C Hamano wrote:\n> \n> Sergey Yurzin writes:\n> \n>> Subject: Re: [PATCH] Fix KeyError \"fileSize\" in verbose mode\n> \n> ...\n> \n>> git-p4.py | 7 +++++--\n>> 1 file changed, 5 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/git-p4.py b/git-p4.py\n>> index 8d151da91b969..b3666eddf12e3 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents):\n>> relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)\n>> relPath = self.encodeWithUTF8(relPath)\n>> if verbose:\n>> - size = int(self.stream_file['fileSize'])\n>> - sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n>> + if 'fileSize' in self.stream_file:\n>> + size = int(self.stream_file['fileSize'])\n>> + sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n>> + else:\n>> + sys.stdout.write('\\r%s --> %s\\n' % (file['depotFile'], relPath))\n>> sys.stdout.flush()\n> \n\nAre you working with P4 Streams? I think I ran into the same problem\nif Streams are involved and I solved it like this:\n\n if verbose:\n- size = int(self.stream_file['fileSize'])\n+ if 'fileSize' in self.stream_file:\n+ size = int(self.stream_file['fileSize'])\n+ elif 'streamContentSize' in self.stream_file:\n+ size = int(self.stream_file['streamContentSize'])\n+ else:\n+ size = 0\n sys.stdout.write('\\r%s --> %s (%i MB)\\n' % (file['depotFile'], relPath, size/1024/1024))\n sys.stdout.flush()\n\nHowever \"streamContentSize\" is something we define ourselves elsewhere\nin git-p4 and not something we get from P4. Therefore it might be\ngarbage. I haven't looked into it further, yet.\n\nYour 'fileSize' check is definitively correct!\n\nCheers,\nLars\n\n\n> I can see from your patch that self.stream_file[] sometimes may not\n> have `fileSize` and when that happens the current code will barf. I\n> also can see that with your patch, the code will NOT barf but output\n> would lack the size information (obviously, because it is not\n> available).\n> \n> However, it is not at all obvious if this is fixing the problem or\n> sweeping the problem under the rug. The proposed log message you\n> write before the patch is the ideal place to say something like\n> \n> In such and such circumstances, it is perfectly normal that\n> P4Sync.stream_file does not know its file and lacks `fileSize`.\n> streamOneP4File() method, however, assumes that this key is\n> always available and tries to write it under the verbose mode.\n> \n> Check the existence of the `fileSize` key and show it only when\n> available.\n> \n> Note that the above _assumes_ that a stream_file that lacks\n> `fileSize` is perfectly normal; if that assumption is incorrect,\n> then perhaps a real fix may be to set `fileSize` in the codepath\n> that forgets to set it (I am not a git-p4 expert, and I am asking\n> Luke to review this patch by CC'ing him).\n> \n> Also note that in a real log message that is helpful for future\n> readers, \"In such and such circumstances\" in the above illustration\n> needs to become a more concrete description.\n> \n> Thanks, and welcome to Git development community.\n> \n>> (type_base, type_mods) = split_p4_type(file[\"type\"])\n>> \n>> --\n>> https://github.com/git/git/pull/373\n\n"}]}