{"thread":{"id":"46218","subject":"[PATCH] git-p4: changelist template with p4 -G change -o","startedAt":"2017-06-20T12:19:54Z","lastAt":"2017-07-13T19:30:40Z","messageCount":31,"participants":["Miguel Torroja","Junio C Hamano","Luke Diamand","miguel torroja","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"322730","messageId":"1497961141-3144-1-git-send-email-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":null,"subject":"[PATCH] git-p4: changelist template with p4 -G change -o","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-20T12:19:01Z","receivedAt":"2017-06-20T12:19:54Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The option -G of p4 (python marshal output) gives more context about the\ndata being output. That's useful when using the command \"change -o\" as\nwe can distinguish between warning/error line and real change description.\n\nSome p4 plugin/hooks in the server side generates some warnings when\nexecuted. Unfortunately those messages are mixed with the output of\n\"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\nin python marshal output (-G). The real change output is reported as\n{'code':'stat'}\n\nSigned-off-by: Miguel Torroja \n---\n git-p4.py | 77 ++++++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 51 insertions(+), 26 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da..a300474 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap):\n \n [upstream, settings] = findUpstreamBranchPoint()\n \n- template = \"\"\n+ template = \"\"\"\\\n+# A Perforce Change Specification.\n+#\n+# Change: The change number. 'new' on a new changelist.\n+# Date: The date this specification was last modified.\n+# Client: The client on which the changelist was created. Read-only.\n+# User: The user who created the changelist.\n+# Status: Either 'pending' or 'submitted'. Read-only.\n+# Type: Either 'public' or 'restricted'. Default is 'public'.\n+# Description: Comments about the changelist. Required.\n+# Jobs: What opened jobs are to be closed by this changelist.\n+# You may delete jobs from this list. (New changelists only.)\n+# Files: What opened files from the default changelist are to be added\n+# to this changelist. You may delete files from this list.\n+# (New changelists only.)\n+\"\"\"\n+ files_list = []\n inFilesSection = False\n+ change_entry = None\n args = ['change', '-o']\n if changelist:\n args.append(str(changelist))\n-\n- for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n- if not [p for p in settings['depot-paths']\n- if p4PathStartsWith(path, p)]:\n- continue\n- else:\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ for entry in p4CmdList(args):\n+ if not entry.has_key('code'):\n+ continue\n+ if entry['code'] == 'stat':\n+ change_entry = entry\n+ break\n+ if not change_entry:\n+ die('Failed to decode output of p4 change -o')\n+ for key, value in change_entry.iteritems():\n+ if key.startswith('File'):\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(value, p)]:\n+ continue\n else:\n- inFilesSection = False\n- else:\n- if line.startswith(\"Files:\"):\n- inFilesSection = True\n-\n- template += line\n-\n+ if not p4PathStartsWith(value, self.depotPath):\n+ continue\n+ files_list.append(value)\n+ continue\n+ # Output in the order expected by prepareLogMessage\n+ for key in ['Change','Client','User','Status','Description','Jobs']:\n+ if not change_entry.has_key(key):\n+ continue\n+ template += '\\n'\n+ template += key + ':'\n+ if key == 'Description':\n+ template += '\\n'\n+ for field_line in change_entry[key].splitlines():\n+ template += '\\t'+field_line+'\\n'\n+ if len(files_list) > 0:\n+ template += '\\n'\n+ template += 'Files:\\n'\n+ for path in files_list:\n+ template += '\\t'+path+'\\n'\n return template\n \n def edit_template(self, template_file):\n-- \n2.1.4\n\n"},{"id":"322936","messageId":"xmqq4lv8kjxo.fsf@gitster.mtv.corp.google.com","threadId":"46218","inReplyTo":"1497961141-3144-1-git-send-email-miguel.torroja@gmail.com","subject":"Re: [PATCH] git-p4: changelist template with p4 -G change -o","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T17:32:03Z","receivedAt":"2017-06-22T17:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miguel Torroja writes:\n\n> The option -G of p4 (python marshal output) gives more context about the\n> data being output. That's useful when using the command \"change -o\" as\n> we can distinguish between warning/error line and real change description.\n>\n> Some p4 plugin/hooks in the server side generates some warnings when\n> executed. Unfortunately those messages are mixed with the output of\n> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n> in python marshal output (-G). The real change output is reported as\n> {'code':'stat'}\n>\n> Signed-off-by: Miguel Torroja \n> ---\n\nAsking for help reviewing to those who have worked on git-p4 in the\npast...\n\nThanks.\n\n> git-p4.py | 77 ++++++++++++++++++++++++++++++++++++++++++---------------------\n> 1 file changed, 51 insertions(+), 26 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 8d151da..a300474 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap):\n> \n> [upstream, settings] = findUpstreamBranchPoint()\n> \n> - template = \"\"\n> + template = \"\"\"\\\n> +# A Perforce Change Specification.\n> +#\n> +# Change: The change number. 'new' on a new changelist.\n> +# Date: The date this specification was last modified.\n> +# Client: The client on which the changelist was created. Read-only.\n> +# User: The user who created the changelist.\n> +# Status: Either 'pending' or 'submitted'. Read-only.\n> +# Type: Either 'public' or 'restricted'. Default is 'public'.\n> +# Description: Comments about the changelist. Required.\n> +# Jobs: What opened jobs are to be closed by this changelist.\n> +# You may delete jobs from this list. (New changelists only.)\n> +# Files: What opened files from the default changelist are to be added\n> +# to this changelist. You may delete files from this list.\n> +# (New changelists only.)\n> +\"\"\"\n> + files_list = []\n> inFilesSection = False\n> + change_entry = None\n> args = ['change', '-o']\n> if changelist:\n> args.append(str(changelist))\n> -\n> - for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n> - if not [p for p in settings['depot-paths']\n> - if p4PathStartsWith(path, p)]:\n> - continue\n> - else:\n> - if not p4PathStartsWith(path, self.depotPath):\n> - continue\n> + for entry in p4CmdList(args):\n> + if not entry.has_key('code'):\n> + continue\n> + if entry['code'] == 'stat':\n> + change_entry = entry\n> + break\n> + if not change_entry:\n> + die('Failed to decode output of p4 change -o')\n> + for key, value in change_entry.iteritems():\n> + if key.startswith('File'):\n> + if settings.has_key('depot-paths'):\n> + if not [p for p in settings['depot-paths']\n> + if p4PathStartsWith(value, p)]:\n> + continue\n> else:\n> - inFilesSection = False\n> - else:\n> - if line.startswith(\"Files:\"):\n> - inFilesSection = True\n> -\n> - template += line\n> -\n> + if not p4PathStartsWith(value, self.depotPath):\n> + continue\n> + files_list.append(value)\n> + continue\n> + # Output in the order expected by prepareLogMessage\n> + for key in ['Change','Client','User','Status','Description','Jobs']:\n> + if not change_entry.has_key(key):\n> + continue\n> + template += '\\n'\n> + template += key + ':'\n> + if key == 'Description':\n> + template += '\\n'\n> + for field_line in change_entry[key].splitlines():\n> + template += '\\t'+field_line+'\\n'\n> + if len(files_list) > 0:\n> + template += '\\n'\n> + template += 'Files:\\n'\n> + for path in files_list:\n> + template += '\\t'+path+'\\n'\n> return template\n> \n> def edit_template(self, template_file):\n"},{"id":"323187","messageId":"CAE5ih78YFSjcn6RNGzdxsjvn6B7xvHMgKKRqirjW00=9hWpDYA@mail.gmail.com","threadId":"46218","inReplyTo":"xmqq4lv8kjxo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-p4: changelist template with p4 -G change -o","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-06-24T11:49:36Z","receivedAt":"2017-06-24T11:49:43Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 22 June 2017 at 18:32, Junio C Hamano wrote:\n> Miguel Torroja writes:\n>\n>> The option -G of p4 (python marshal output) gives more context about the\n>> data being output. That's useful when using the command \"change -o\" as\n>> we can distinguish between warning/error line and real change description.\n>>\n>> Some p4 plugin/hooks in the server side generates some warnings when\n>> executed. Unfortunately those messages are mixed with the output of\n>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>> in python marshal output (-G). The real change output is reported as\n>> {'code':'stat'}\n\nI think this seems like a reasonable thing to do if \"p4 change -o\" is\njumbling up output.\n\nOne thing I notice trying it out by hand is that we seem to have lost\nthe annotation of the Perforce per-file modification type (is there a\nproper name for this?).\n\nFor example, if I add a file called \"baz\", then the original version\ncreates a template which looks like this:\n\n //depot/baz # add\n\nBut the new one creates a template which looks like:\n\n //depot/baz\n\nLuke\n"},{"id":"323201","messageId":"CAKYtbVbMKgPASP-ib7DSbDONnV988woV_=zU=XPwaMpWE=TqGQ@mail.gmail.com","threadId":"46218","inReplyTo":"CAE5ih78YFSjcn6RNGzdxsjvn6B7xvHMgKKRqirjW00=9hWpDYA@mail.gmail.com","subject":"Re: [PATCH] git-p4: changelist template with p4 -G change -o","fromName":"miguel torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-24T12:38:30Z","receivedAt":"2017-06-24T12:38:37Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"You are right about the \"# add\"comment. I couldn't find any extra info\nin the marshaled output that I can use to add the change action\ncomment after the path. That's one downside of that change.\n\nOn Sat, Jun 24, 2017 at 1:49 PM, Luke Diamand wrote:\n> On 22 June 2017 at 18:32, Junio C Hamano wrote:\n>> Miguel Torroja writes:\n>>\n>>> The option -G of p4 (python marshal output) gives more context about the\n>>> data being output. That's useful when using the command \"change -o\" as\n>>> we can distinguish between warning/error line and real change description.\n>>>\n>>> Some p4 plugin/hooks in the server side generates some warnings when\n>>> executed. Unfortunately those messages are mixed with the output of\n>>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>>> in python marshal output (-G). The real change output is reported as\n>>> {'code':'stat'}\n>\n> I think this seems like a reasonable thing to do if \"p4 change -o\" is\n> jumbling up output.\n>\n> One thing I notice trying it out by hand is that we seem to have lost\n> the annotation of the Perforce per-file modification type (is there a\n> proper name for this?).\n>\n> For example, if I add a file called \"baz\", then the original version\n> creates a template which looks like this:\n>\n> //depot/baz # add\n>\n> But the new one creates a template which looks like:\n>\n> //depot/baz\n>\n> Luke\n"},{"id":"323213","messageId":"DCC54592-4010-46D5-98A9-B7B4D1467169@gmail.com","threadId":"46218","inReplyTo":"CAE5ih78YFSjcn6RNGzdxsjvn6B7xvHMgKKRqirjW00=9hWpDYA@mail.gmail.com","subject":"Re: [PATCH] git-p4: changelist template with p4 -G change -o","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-24T17:36:38Z","receivedAt":"2017-06-24T17:36:47Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Jun 2017, at 13:49, Luke Diamand wrote:\n> \n> On 22 June 2017 at 18:32, Junio C Hamano wrote:\n>> Miguel Torroja writes:\n>> \n>>> The option -G of p4 (python marshal output) gives more context about the\n>>> data being output. That's useful when using the command \"change -o\" as\n>>> we can distinguish between warning/error line and real change description.\n>>> \n>>> Some p4 plugin/hooks in the server side generates some warnings when\n>>> executed. Unfortunately those messages are mixed with the output of\n>>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>>> in python marshal output (-G). The real change output is reported as\n>>> {'code':'stat'}\n> \n> I think this seems like a reasonable thing to do if \"p4 change -o\" is\n> jumbling up output.\n> \n> One thing I notice trying it out by hand is that we seem to have lost\n> the annotation of the Perforce per-file modification type (is there a\n> proper name for this?).\n> \n> For example, if I add a file called \"baz\", then the original version\n> creates a template which looks like this:\n> \n> //depot/baz # add\n> \n> But the new one creates a template which looks like:\n> \n> //depot/baz\n\n@Miguel: You wrote that p4 plugins/hooks generate these warnings.\nI wonder if you see a way to replicate that in a test case. Either\nin t9800 or a new t98XX test case file?\n\n- Lars\n\n"},{"id":"323319","messageId":"CAKYtbVY_=aMjcS=r2YyhcxKiUAaJUJA=OELTvXfau4GGz7Lz4Q@mail.gmail.com","threadId":"46218","inReplyTo":"CAKYtbVbGekXGAyPd7HeLot_MdZkp7-1Ss-iAi7o8ze2b+sNB6Q@mail.gmail.com","subject":"Re: [PATCH] git-p4: changelist template with p4 -G change -o","fromName":"miguel torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-27T09:20:22Z","receivedAt":"2017-06-27T09:20:29Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"Hi Lars/Luke,\n\nI tried a first test extending t9807-git-p4-submit.sh. I set this p4\ntrigger: 'p4test command pre-user-change \"echo verbose trigger\" '. I'm\nable to reproduce the issue I wanted to fix. However I found yet\nanother issue, in this case when reading the result from\np4_read_pipe_lines (in function p4ChangesForPaths), the\npre-user-change is triggered with any \"p4 change\" and \"p4 changes\"\ncommand (the p4 server we have in production, only shows \"extra\"\nmessages with p4 change).\n I'll collapse in one single commit the fix for p4 change/p4 changes\nand the new test.\n\nThanks,\n\nMiguel\n\nOn Sat, Jun 24, 2017 at 10:37 PM, miguel torroja\n wrote:\n> Hi Lars,\n>\n> I think it's doable to set a custom p4 trigger, created by the test case,\n> that outputs \"extra info\" when requesting a changelist description.\n> I'll do a specific test and post it to this thread.\n>\n>\n> Thanks,\n>\n>\n> El 24 jun. 2017 7:36 p. m., \"Lars Schneider\" \n> escribió:\n>\n>\n>> On 24 Jun 2017, at 13:49, Luke Diamand wrote:\n>>\n>> On 22 June 2017 at 18:32, Junio C Hamano wrote:\n>>> Miguel Torroja writes:\n>>>\n>>>> The option -G of p4 (python marshal output) gives more context about the\n>>>> data being output. That's useful when using the command \"change -o\" as\n>>>> we can distinguish between warning/error line and real change\n>>>> description.\n>>>>\n>>>> Some p4 plugin/hooks in the server side generates some warnings when\n>>>> executed. Unfortunately those messages are mixed with the output of\n>>>> \"p4 change -o\". Those extra warning lines are reported as\n>>>> {'code':'info'}\n>>>> in python marshal output (-G). The real change output is reported as\n>>>> {'code':'stat'}\n>>\n>> I think this seems like a reasonable thing to do if \"p4 change -o\" is\n>> jumbling up output.\n>>\n>> One thing I notice trying it out by hand is that we seem to have lost\n>> the annotation of the Perforce per-file modification type (is there a\n>> proper name for this?).\n>>\n>> For example, if I add a file called \"baz\", then the original version\n>> creates a template which looks like this:\n>>\n>> //depot/baz # add\n>>\n>> But the new one creates a template which looks like:\n>>\n>> //depot/baz\n>\n> @Miguel: You wrote that p4 plugins/hooks generate these warnings.\n> I wonder if you see a way to replicate that in a test case. Either\n> in t9800 or a new t98XX test case file?\n>\n> - Lars\n>\n>\n"},{"id":"323375","messageId":"20170627191704.4446-1-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"CAKYtbVY_=aMjcS=r2YyhcxKiUAaJUJA=OELTvXfau4GGz7Lz4Q@mail.gmail.com","subject":"[PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-27T19:17:04Z","receivedAt":"2017-06-27T19:17:39Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The option -G of p4 (python marshal output) gives more context about the\ndata being output. That's useful when using the command \"change -o\" as\nwe can distinguish between warning/error line and real change description.\n\nSome p4 triggers in the server side generate some warnings when\nexecuted. Unfortunately those messages are mixed with the output of\n\"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\nin python marshal output (-G). The real change output is reported as\n{'code':'stat'}\n\nA new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\nthat outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n\nSigned-off-by: Miguel Torroja \n---\n git-p4.py | 83 ++++++++++++++++++++++++++++++++----------------\n t/t9807-git-p4-submit.sh | 28 ++++++++++++++++\n 2 files changed, 83 insertions(+), 28 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da91..239a8f144 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -879,8 +879,10 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n cmd += [\"%s...@%s\" % (p, revisionRange)]\n \n # Insert changes in chronological order\n- for line in reversed(p4_read_pipe_lines(cmd)):\n- changes.add(int(line.split(\" \")[1]))\n+ for entry in reversed(p4CmdList(cmd)):\n+ if not entry.has_key('change'):\n+ continue\n+ changes.add(int(entry['change']))\n \n if not block_size:\n break\n@@ -1526,37 +1528,62 @@ class P4Submit(Command, P4UserMap):\n \n [upstream, settings] = findUpstreamBranchPoint()\n \n- template = \"\"\n+ template = \"\"\"\\\n+# A Perforce Change Specification.\n+#\n+# Change: The change number. 'new' on a new changelist.\n+# Date: The date this specification was last modified.\n+# Client: The client on which the changelist was created. Read-only.\n+# User: The user who created the changelist.\n+# Status: Either 'pending' or 'submitted'. Read-only.\n+# Type: Either 'public' or 'restricted'. Default is 'public'.\n+# Description: Comments about the changelist. Required.\n+# Jobs: What opened jobs are to be closed by this changelist.\n+# You may delete jobs from this list. (New changelists only.)\n+# Files: What opened files from the default changelist are to be added\n+# to this changelist. You may delete files from this list.\n+# (New changelists only.)\n+\"\"\"\n+ files_list = []\n inFilesSection = False\n+ change_entry = None\n args = ['change', '-o']\n if changelist:\n args.append(str(changelist))\n-\n- for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n- if not [p for p in settings['depot-paths']\n- if p4PathStartsWith(path, p)]:\n- continue\n- else:\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ for entry in p4CmdList(args):\n+ if not entry.has_key('code'):\n+ continue\n+ if entry['code'] == 'stat':\n+ change_entry = entry\n+ break\n+ if not change_entry:\n+ die('Failed to decode output of p4 change -o')\n+ for key, value in change_entry.iteritems():\n+ if key.startswith('File'):\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(value, p)]:\n+ continue\n else:\n- inFilesSection = False\n- else:\n- if line.startswith(\"Files:\"):\n- inFilesSection = True\n-\n- template += line\n-\n+ if not p4PathStartsWith(value, self.depotPath):\n+ continue\n+ files_list.append(value)\n+ continue\n+ # Output in the order expected by prepareLogMessage\n+ for key in ['Change','Client','User','Status','Description','Jobs']:\n+ if not change_entry.has_key(key):\n+ continue\n+ template += '\\n'\n+ template += key + ':'\n+ if key == 'Description':\n+ template += '\\n'\n+ for field_line in change_entry[key].splitlines():\n+ template += '\\t'+field_line+'\\n'\n+ if len(files_list) > 0:\n+ template += '\\n'\n+ template += 'Files:\\n'\n+ for path in files_list:\n+ template += '\\t'+path+'\\n'\n return template\n \n def edit_template(self, template_file):\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 3457d5db6..05d7fc3f7 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -409,6 +409,34 @@ test_expect_success 'description with Jobs section and bogus following text' '\n \t)\n '\n \n+test_expect_success 'description with extra lines from verbose p4 trigger' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\t p4triggertest-command command pre-user-change \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\techo file20 >file20 &&\n+\t\tgit add file20 &&\n+\t\tgit commit -m file20 &&\n+\t\tgit p4 submit\n+\t) &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\ttest_path_is_file file20\n+\t)\n+'\n+\n test_expect_success 'submit --prepare-p4-only' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.11.0\n\n"},{"id":"323414","messageId":"xmqqk23wycso.fsf@gitster.mtv.corp.google.com","threadId":"46218","inReplyTo":"20170627191704.4446-1-miguel.torroja@gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T04:08:23Z","receivedAt":"2017-06-28T04:08:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miguel Torroja writes:\n\n> The option -G of p4 (python marshal output) gives more context about the\n> data being output. That's useful when using the command \"change -o\" as\n> we can distinguish between warning/error line and real change description.\n>\n> Some p4 triggers in the server side generate some warnings when\n> executed. Unfortunately those messages are mixed with the output of\n> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n> in python marshal output (-G). The real change output is reported as\n> {'code':'stat'}\n>\n> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>\n> Signed-off-by: Miguel Torroja \n> ---\n\nIt appears that https://travis-ci.org/git/git/builds/247724639\ndoes not like this change. For example:\n\n https://travis-ci.org/git/git/jobs/247724642#L1848\n\nindicates that not just 9807 (new tests added by this patch) but\nalso 9800 starts to fail.\n\nI'd wait for git-p4 experts to comment and help guiding this change\nforward.\n\nThanks.\n"},{"id":"323421","messageId":"CAE5ih78VwBVT+XHnwgnt-JcLB-c4d_Gf+9Wfb_bL=LcgkjDrUQ@mail.gmail.com","threadId":"46218","inReplyTo":"xmqqk23wycso.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-06-28T09:54:43Z","receivedAt":"2017-06-28T09:54:51Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 28 June 2017 at 05:08, Junio C Hamano wrote:\n> Miguel Torroja writes:\n>\n>> The option -G of p4 (python marshal output) gives more context about the\n>> data being output. That's useful when using the command \"change -o\" as\n>> we can distinguish between warning/error line and real change description.\n>>\n>> Some p4 triggers in the server side generate some warnings when\n>> executed. Unfortunately those messages are mixed with the output of\n>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>> in python marshal output (-G). The real change output is reported as\n>> {'code':'stat'}\n>>\n>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>\n>> Signed-off-by: Miguel Torroja \n>> ---\n>\n> It appears that https://travis-ci.org/git/git/builds/247724639\n> does not like this change. For example:\n>\n> https://travis-ci.org/git/git/jobs/247724642#L1848\n>\n> indicates that not just 9807 (new tests added by this patch) but\n> also 9800 starts to fail.\n>\n> I'd wait for git-p4 experts to comment and help guiding this change\n> forward.\n\nI only see a (very weird) failure in t9800. I wonder if there are some\nP4 version differences.\n\nClient: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).\nServer: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)\n\nThere's also a whitespace error according to \"git diff --check\".\n:\nSadly I don't think there's any way to do this and yet keep the \"#\nedit\" comments. It looks like \"p4 change -o\" outputs lines with \"'#\nedit\" on the end, but the (supposedly semantically equivalent) \"p4 -G\nchange -o\" command does not. I think that's a P4 bug.\n\nSo we have a choice of fixing a garbled message in the face of scripts\nin the backend, or keeping the comments, or writing some extra Python\nto infer them. I vote for fixing the garbled message.\n\nLuke\n"},{"id":"323424","messageId":"CAKYtbVaLkt6_rFgehgSsrLzo-oO3sEVoMLBtS5XX59ymYYS7=w@mail.gmail.com","threadId":"46218","inReplyTo":"CAE5ih78VwBVT+XHnwgnt-JcLB-c4d_Gf+9Wfb_bL=LcgkjDrUQ@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"miguel torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-28T13:14:29Z","receivedAt":"2017-06-28T13:14:36Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"Thanks Luke,\n\nregarding the error in t9800 (not ok 18 - unresolvable host in P4PORT\nshould display error), for me it's very weird too as it doesn't seem\nto be related to this particular change, as the patch changes are not\nexercised with that test.\n\nThe test 21 in t9807 was precisely the new test added to test the\nchange (it was passing with local setup), the test log is truncated\nbefore the output of test 21 in t9807 but I'm afraid I'm not very\nfamiliar with Travis, so maybe I'm missing something. Is there a way\nto have the full logs or they are always truncated after some number\nof lines?\n\nI think you get an error with git diff --check because I added spaces\nafter a tab, but those spaces are intentional, the tabs are for the\n\"<<-EOF\" and spaces are for the \"p4 triggers\" specificiation.\n\nThanks,\n\n\nOn Wed, Jun 28, 2017 at 11:54 AM, Luke Diamand wrote:\n> On 28 June 2017 at 05:08, Junio C Hamano wrote:\n>> Miguel Torroja writes:\n>>\n>>> The option -G of p4 (python marshal output) gives more context about the\n>>> data being output. That's useful when using the command \"change -o\" as\n>>> we can distinguish between warning/error line and real change description.\n>>>\n>>> Some p4 triggers in the server side generate some warnings when\n>>> executed. Unfortunately those messages are mixed with the output of\n>>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>>> in python marshal output (-G). The real change output is reported as\n>>> {'code':'stat'}\n>>>\n>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>>\n>>> Signed-off-by: Miguel Torroja \n>>> ---\n>>\n>> It appears that https://travis-ci.org/git/git/builds/247724639\n>> does not like this change. For example:\n>>\n>> https://travis-ci.org/git/git/jobs/247724642#L1848\n>>\n>> indicates that not just 9807 (new tests added by this patch) but\n>> also 9800 starts to fail.\n>>\n>> I'd wait for git-p4 experts to comment and help guiding this change\n>> forward.\n>\n> I only see a (very weird) failure in t9800. I wonder if there are some\n> P4 version differences.\n>\n> Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).\n> Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)\n>\n> There's also a whitespace error according to \"git diff --check\".\n> :\n> Sadly I don't think there's any way to do this and yet keep the \"#\n> edit\" comments. It looks like \"p4 change -o\" outputs lines with \"'#\n> edit\" on the end, but the (supposedly semantically equivalent) \"p4 -G\n> change -o\" command does not. I think that's a P4 bug.\n>\n> So we have a choice of fixing a garbled message in the face of scripts\n> in the backend, or keeping the comments, or writing some extra Python\n> to infer them. I vote for fixing the garbled message.\n>\n> Luke\n"},{"id":"323508","messageId":"CAKYtbVaZ85LAvgz4+p29Q_n7wN0s0ocnXO4LtLDzjS7pnNmZXw@mail.gmail.com","threadId":"46218","inReplyTo":"CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"miguel torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-29T22:41:26Z","receivedAt":"2017-06-29T22:41:37Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand wrote:\n> On 28 June 2017 at 14:14, miguel torroja wrote:\n>> Thanks Luke,\n>>\n>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT\n>> should display error), for me it's very weird too as it doesn't seem\n>> to be related to this particular change, as the patch changes are not\n>> exercised with that test.\n>\n> I had a look at this. The problem is that the old code uses\n> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.\n>\n> But the new code calls p4CmdList() which has does error handling by\n> setting \"p4ExitCode\" to a non-zero value in the returned dictionary.\n>\n> I think if you just check for that case, the test will then pass\n\nThank you for debugging this, I did as you suggested and it passed that test!\n\n>>\n>> The test 21 in t9807 was precisely the new test added to test the\n>> change (it was passing with local setup), the test log is truncated\n>> before the output of test 21 in t9807 but I'm afraid I'm not very\n>> familiar with Travis, so maybe I'm missing something. Is there a way\n>> to have the full logs or they are always truncated after some number\n>> of lines?\n>\n> For me, t9807 is working fine.\n>\n>>\n>> I think you get an error with git diff --check because I added spaces\n>> after a tab, but those spaces are intentional, the tabs are for the\n>> \"<<-EOF\" and spaces are for the \"p4 triggers\" specificiation.\n>\n> OK.\n>\n\nIn the end, ,the reason t9807 was not passing was precisely the tabs\nand spaces of the patch. the original patch had:\n....., as I explained, the tabs were supposed to be\nignored by \"<<-EOF\" and the spaces were supposed to be sent to stdin\nof p4 triggers, but when the patch was applied to upstream the\n were substituted by tabs what led to a malformed \"p4\ntrigger\" description. I just collapsed the description in one single\nline and now it's passing\n>\n> Luke\n\n\nI'm sending a new patch with the two changes I just mentioned.\n\nThanks,\n"},{"id":"323509","messageId":"20170629224659.25677-1-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com","subject":"[PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-29T22:46:59Z","receivedAt":"2017-06-29T22:47:47Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The option -G of p4 (python marshal output) gives more context about the\ndata being output. That's useful when using the command \"change -o\" as\nwe can distinguish between warning/error line and real change description.\n\nSome p4 triggers in the server side generate some warnings when\nexecuted. Unfortunately those messages are mixed with the output of\n\"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\nin python marshal output (-G). The real change output is reported as\n{'code':'stat'}\n\nA new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\nthat outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n\nSigned-off-by: Miguel Torroja \nSigned-off-by: Junio C Hamano \n---\n git-p4.py | 85 ++++++++++++++++++++++++++++++++----------------\n t/t9807-git-p4-submit.sh | 27 +++++++++++++++\n 2 files changed, 84 insertions(+), 28 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da91..61dc045f3 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n cmd += [\"%s...@%s\" % (p, revisionRange)]\n \n # Insert changes in chronological order\n- for line in reversed(p4_read_pipe_lines(cmd)):\n- changes.add(int(line.split(\" \")[1]))\n+ for entry in reversed(p4CmdList(cmd)):\n+ if entry.has_key('p4ExitCode'):\n+ die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n+ if not entry.has_key('change'):\n+ continue\n+ changes.add(int(entry['change']))\n \n if not block_size:\n break\n@@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):\n \n [upstream, settings] = findUpstreamBranchPoint()\n \n- template = \"\"\n+ template = \"\"\"\\\n+# A Perforce Change Specification.\n+#\n+# Change: The change number. 'new' on a new changelist.\n+# Date: The date this specification was last modified.\n+# Client: The client on which the changelist was created. Read-only.\n+# User: The user who created the changelist.\n+# Status: Either 'pending' or 'submitted'. Read-only.\n+# Type: Either 'public' or 'restricted'. Default is 'public'.\n+# Description: Comments about the changelist. Required.\n+# Jobs: What opened jobs are to be closed by this changelist.\n+# You may delete jobs from this list. (New changelists only.)\n+# Files: What opened files from the default changelist are to be added\n+# to this changelist. You may delete files from this list.\n+# (New changelists only.)\n+\"\"\"\n+ files_list = []\n inFilesSection = False\n+ change_entry = None\n args = ['change', '-o']\n if changelist:\n args.append(str(changelist))\n-\n- for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n- if not [p for p in settings['depot-paths']\n- if p4PathStartsWith(path, p)]:\n- continue\n- else:\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ for entry in p4CmdList(args):\n+ if not entry.has_key('code'):\n+ continue\n+ if entry['code'] == 'stat':\n+ change_entry = entry\n+ break\n+ if not change_entry:\n+ die('Failed to decode output of p4 change -o')\n+ for key, value in change_entry.iteritems():\n+ if key.startswith('File'):\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(value, p)]:\n+ continue\n else:\n- inFilesSection = False\n- else:\n- if line.startswith(\"Files:\"):\n- inFilesSection = True\n-\n- template += line\n-\n+ if not p4PathStartsWith(value, self.depotPath):\n+ continue\n+ files_list.append(value)\n+ continue\n+ # Output in the order expected by prepareLogMessage\n+ for key in ['Change','Client','User','Status','Description','Jobs']:\n+ if not change_entry.has_key(key):\n+ continue\n+ template += '\\n'\n+ template += key + ':'\n+ if key == 'Description':\n+ template += '\\n'\n+ for field_line in change_entry[key].splitlines():\n+ template += '\\t'+field_line+'\\n'\n+ if len(files_list) > 0:\n+ template += '\\n'\n+ template += 'Files:\\n'\n+ for path in files_list:\n+ template += '\\t'+path+'\\n'\n return template\n \n def edit_template(self, template_file):\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 3457d5db6..4962b6862 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' '\n \t)\n '\n \n+test_expect_success 'description with extra lines from verbose p4 trigger' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\techo file20 >file20 &&\n+\t\tgit add file20 &&\n+\t\tgit commit -m file20 &&\n+\t\tgit p4 submit\n+\t) &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\ttest_path_is_file file20\n+\t)\n+'\n+\n test_expect_success 'submit --prepare-p4-only' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.11.0\n\n"},{"id":"323551","messageId":"CAE5ih7-go9PampG3Ltbx2-vYUezbN4QDHEVEHwpfXkpvUfLCaQ@mail.gmail.com","threadId":"46218","inReplyTo":"CAKYtbVaZ85LAvgz4+p29Q_n7wN0s0ocnXO4LtLDzjS7pnNmZXw@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-06-30T07:56:07Z","receivedAt":"2017-06-30T07:56:18Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 29 June 2017 at 23:41, miguel torroja wrote:\n> On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand wrote:\n>> On 28 June 2017 at 14:14, miguel torroja wrote:\n>>> Thanks Luke,\n>>>\n>>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT\n>>> should display error), for me it's very weird too as it doesn't seem\n>>> to be related to this particular change, as the patch changes are not\n>>> exercised with that test.\n>>\n>> I had a look at this. The problem is that the old code uses\n>> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.\n>>\n>> But the new code calls p4CmdList() which has does error handling by\n>> setting \"p4ExitCode\" to a non-zero value in the returned dictionary.\n>>\n>> I think if you just check for that case, the test will then pass\n>\n> Thank you for debugging this, I did as you suggested and it passed that test!\n>\n>>>\n>>> The test 21 in t9807 was precisely the new test added to test the\n>>> change (it was passing with local setup), the test log is truncated\n>>> before the output of test 21 in t9807 but I'm afraid I'm not very\n>>> familiar with Travis, so maybe I'm missing something. Is there a way\n>>> to have the full logs or they are always truncated after some number\n>>> of lines?\n>>\n>> For me, t9807 is working fine.\n>>\n>>>\n>>> I think you get an error with git diff --check because I added spaces\n>>> after a tab, but those spaces are intentional, the tabs are for the\n>>> \"<<-EOF\" and spaces are for the \"p4 triggers\" specificiation.\n>>\n>> OK.\n>>\n>\n> In the end, ,the reason t9807 was not passing was precisely the tabs\n> and spaces of the patch. the original patch had:\n> ....., as I explained, the tabs were supposed to be\n> ignored by \"<<-EOF\" and the spaces were supposed to be sent to stdin\n> of p4 triggers, but when the patch was applied to upstream the\n> were substituted by tabs what led to a malformed \"p4\n> trigger\" description. I just collapsed the description in one single\n> line and now it's passing\n>>\n>> Luke\n>\n>\n> I'm sending a new patch with the two changes I just mentioned.\n\nLooks good to me, Ack. Can we squash the two changes together?\n\nLuke\n"},{"id":"323552","messageId":"CAKYtbVajVpJomKOHG5ex7ib9Mtm8z+=mvQOrR1ws6wnASt9LFw@mail.gmail.com","threadId":"46218","inReplyTo":"CAE5ih7-go9PampG3Ltbx2-vYUezbN4QDHEVEHwpfXkpvUfLCaQ@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"miguel torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-30T08:22:19Z","receivedAt":"2017-06-30T08:22:25Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The Latest patch I sent was already the squashed version with the fix\nto pass the tests.\n\nThanks,\n\nOn Fri, Jun 30, 2017 at 9:56 AM, Luke Diamand wrote:\n> On 29 June 2017 at 23:41, miguel torroja wrote:\n>> On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand wrote:\n>>> On 28 June 2017 at 14:14, miguel torroja wrote:\n>>>> Thanks Luke,\n>>>>\n>>>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT\n>>>> should display error), for me it's very weird too as it doesn't seem\n>>>> to be related to this particular change, as the patch changes are not\n>>>> exercised with that test.\n>>>\n>>> I had a look at this. The problem is that the old code uses\n>>> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.\n>>>\n>>> But the new code calls p4CmdList() which has does error handling by\n>>> setting \"p4ExitCode\" to a non-zero value in the returned dictionary.\n>>>\n>>> I think if you just check for that case, the test will then pass\n>>\n>> Thank you for debugging this, I did as you suggested and it passed that test!\n>>\n>>>>\n>>>> The test 21 in t9807 was precisely the new test added to test the\n>>>> change (it was passing with local setup), the test log is truncated\n>>>> before the output of test 21 in t9807 but I'm afraid I'm not very\n>>>> familiar with Travis, so maybe I'm missing something. Is there a way\n>>>> to have the full logs or they are always truncated after some number\n>>>> of lines?\n>>>\n>>> For me, t9807 is working fine.\n>>>\n>>>>\n>>>> I think you get an error with git diff --check because I added spaces\n>>>> after a tab, but those spaces are intentional, the tabs are for the\n>>>> \"<<-EOF\" and spaces are for the \"p4 triggers\" specificiation.\n>>>\n>>> OK.\n>>>\n>>\n>> In the end, ,the reason t9807 was not passing was precisely the tabs\n>> and spaces of the patch. the original patch had:\n>> ....., as I explained, the tabs were supposed to be\n>> ignored by \"<<-EOF\" and the spaces were supposed to be sent to stdin\n>> of p4 triggers, but when the patch was applied to upstream the\n>> were substituted by tabs what led to a malformed \"p4\n>> trigger\" description. I just collapsed the description in one single\n>> line and now it's passing\n>>>\n>>> Luke\n>>\n>>\n>> I'm sending a new patch with the two changes I just mentioned.\n>\n> Looks good to me, Ack. Can we squash the two changes together?\n>\n> Luke\n"},{"id":"323553","messageId":"41BF267D-5F4D-4031-B9D4-15DB263D35D9@gmail.com","threadId":"46218","inReplyTo":"20170629224659.25677-1-miguel.torroja@gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-30T08:26:09Z","receivedAt":"2017-06-30T08:27:46Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 30 Jun 2017, at 00:46, miguel torroja wrote:\n> \n> The option -G of p4 (python marshal output) gives more context about the\n> data being output. That's useful when using the command \"change -o\" as\n> we can distinguish between warning/error line and real change description.\n> \n> Some p4 triggers in the server side generate some warnings when\n> executed. Unfortunately those messages are mixed with the output of\n> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n> in python marshal output (-G). The real change output is reported as\n> {'code':'stat'}\n> \n> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n> \n> Signed-off-by: Miguel Torroja \n> Signed-off-by: Junio C Hamano \n> ---\n> git-p4.py | 85 ++++++++++++++++++++++++++++++++----------------\n> t/t9807-git-p4-submit.sh | 27 +++++++++++++++\n> 2 files changed, 84 insertions(+), 28 deletions(-)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index 8d151da91..61dc045f3 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n> cmd += [\"%s...@%s\" % (p, revisionRange)]\n> \n> # Insert changes in chronological order\n> - for line in reversed(p4_read_pipe_lines(cmd)):\n> - changes.add(int(line.split(\" \")[1]))\n> + for entry in reversed(p4CmdList(cmd)):\n> + if entry.has_key('p4ExitCode'):\n> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n> + if not entry.has_key('change'):\n> + continue\n> + changes.add(int(entry['change']))\n> \n> if not block_size:\n> break\n> @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):\n> \n> [upstream, settings] = findUpstreamBranchPoint()\n> \n> - template = \"\"\n> + template = \"\"\"\\\n> +# A Perforce Change Specification.\n> +#\n> +# Change: The change number. 'new' on a new changelist.\n> +# Date: The date this specification was last modified.\n> +# Client: The client on which the changelist was created. Read-only.\n> +# User: The user who created the changelist.\n> +# Status: Either 'pending' or 'submitted'. Read-only.\n> +# Type: Either 'public' or 'restricted'. Default is 'public'.\n> +# Description: Comments about the changelist. Required.\n> +# Jobs: What opened jobs are to be closed by this changelist.\n> +# You may delete jobs from this list. (New changelists only.)\n> +# Files: What opened files from the default changelist are to be added\n> +# to this changelist. You may delete files from this list.\n> +# (New changelists only.)\n> +\"\"\"\n> + files_list = []\n> inFilesSection = False\n> + change_entry = None\n> args = ['change', '-o']\n> if changelist:\n> args.append(str(changelist))\n> -\n> - for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n> - if not [p for p in settings['depot-paths']\n> - if p4PathStartsWith(path, p)]:\n> - continue\n> - else:\n> - if not p4PathStartsWith(path, self.depotPath):\n> - continue\n> + for entry in p4CmdList(args):\n> + if not entry.has_key('code'):\n> + continue\n> + if entry['code'] == 'stat':\n> + change_entry = entry\n> + break\n> + if not change_entry:\n> + die('Failed to decode output of p4 change -o')\n> + for key, value in change_entry.iteritems():\n> + if key.startswith('File'):\n> + if settings.has_key('depot-paths'):\n> + if not [p for p in settings['depot-paths']\n> + if p4PathStartsWith(value, p)]:\n> + continue\n> else:\n> - inFilesSection = False\n> - else:\n> - if line.startswith(\"Files:\"):\n> - inFilesSection = True\n> -\n> - template += line\n> -\n> + if not p4PathStartsWith(value, self.depotPath):\n> + continue\n> + files_list.append(value)\n> + continue\n> + # Output in the order expected by prepareLogMessage\n> + for key in ['Change','Client','User','Status','Description','Jobs']:\n> + if not change_entry.has_key(key):\n> + continue\n> + template += '\\n'\n> + template += key + ':'\n> + if key == 'Description':\n> + template += '\\n'\n> + for field_line in change_entry[key].splitlines():\n> + template += '\\t'+field_line+'\\n'\n> + if len(files_list) > 0:\n> + template += '\\n'\n> + template += 'Files:\\n'\n> + for path in files_list:\n> + template += '\\t'+path+'\\n'\n> return template\n> \n> def edit_template(self, template_file):\n> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\n> index 3457d5db6..4962b6862 100755\n> --- a/t/t9807-git-p4-submit.sh\n> +++ b/t/t9807-git-p4-submit.sh\n> @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' '\n> \t)\n> '\n> \n\nI have never worked with p4 triggers and that might be\nthe reason why I don't understand your test case.\nMaybe you can help me?\n\n> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n> +\ttest_when_finished cleanup_git &&\n> +\tgit p4 clone --dest=\"$git\" //depot &&\n> +\t(\n> +\t\tp4 triggers -i <<-EOF\n> +\t\tTriggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n> +\t\tEOF\n> +\t) &&\n\nYou clone the test repo and install a trigger.\n\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n> +\t\techo file20 >file20 &&\n> +\t\tgit add file20 &&\n> +\t\tgit commit -m file20 &&\n> +\t\tgit p4 submit\n> +\t) &&\n\nYou make a new commit. This should run the \"echo verbose trigger\", right?\n\n> +\t(\n> +\t\tp4 triggers -i <<-EOF\n> +\t\tTriggers:\n> +\t\tEOF\n> +\t) &&\n\nYou delete the trigger.\n\n> +\t(\n> +\t\tcd \"$cli\" &&\n> +\t\ttest_path_is_file file20\n> +\t)\n\nYou check that the file20 is available in P4.\n\n\nWhat would happen if I run this test case without your patch?\nWouldn't it pass just fine?\nWouldn't we need to check that no warning/error is in the\nreal change description?\n\nThanks,\nLars"},{"id":"323556","messageId":"CAKYtbVbOXZiZrsFGOKu=sFroSL-FBQo2wMaA9GmJvc-Uh7QZEA@mail.gmail.com","threadId":"46218","inReplyTo":"41BF267D-5F4D-4031-B9D4-15DB263D35D9@gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-30T09:41:55Z","receivedAt":"2017-06-30T09:42:02Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider\n wrote:\n>\n>> On 30 Jun 2017, at 00:46, miguel torroja wrote:\n>>\n>> The option -G of p4 (python marshal output) gives more context about the\n>> data being output. That's useful when using the command \"change -o\" as\n>> we can distinguish between warning/error line and real change description.\n>>\n>> Some p4 triggers in the server side generate some warnings when\n>> executed. Unfortunately those messages are mixed with the output of\n>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>> in python marshal output (-G). The real change output is reported as\n>> {'code':'stat'}\n>>\n>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>\n>> Signed-off-by: Miguel Torroja \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> git-p4.py | 85 ++++++++++++++++++++++++++++++++----------------\n>> t/t9807-git-p4-submit.sh | 27 +++++++++++++++\n>> 2 files changed, 84 insertions(+), 28 deletions(-)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 8d151da91..61dc045f3 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n>> cmd += [\"%s...@%s\" % (p, revisionRange)]\n>>\n>> # Insert changes in chronological order\n>> - for line in reversed(p4_read_pipe_lines(cmd)):\n>> - changes.add(int(line.split(\" \")[1]))\n>> + for entry in reversed(p4CmdList(cmd)):\n>> + if entry.has_key('p4ExitCode'):\n>> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n>> + if not entry.has_key('change'):\n>> + continue\n>> + changes.add(int(entry['change']))\n>>\n>> if not block_size:\n>> break\n>> @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):\n>>\n>> [upstream, settings] = findUpstreamBranchPoint()\n>>\n>> - template = \"\"\n>> + template = \"\"\"\\\n>> +# A Perforce Change Specification.\n>> +#\n>> +# Change: The change number. 'new' on a new changelist.\n>> +# Date: The date this specification was last modified.\n>> +# Client: The client on which the changelist was created. Read-only.\n>> +# User: The user who created the changelist.\n>> +# Status: Either 'pending' or 'submitted'. Read-only.\n>> +# Type: Either 'public' or 'restricted'. Default is 'public'.\n>> +# Description: Comments about the changelist. Required.\n>> +# Jobs: What opened jobs are to be closed by this changelist.\n>> +# You may delete jobs from this list. (New changelists only.)\n>> +# Files: What opened files from the default changelist are to be added\n>> +# to this changelist. You may delete files from this list.\n>> +# (New changelists only.)\n>> +\"\"\"\n>> + files_list = []\n>> inFilesSection = False\n>> + change_entry = None\n>> args = ['change', '-o']\n>> if changelist:\n>> args.append(str(changelist))\n>> -\n>> - for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n>> - if not [p for p in settings['depot-paths']\n>> - if p4PathStartsWith(path, p)]:\n>> - continue\n>> - else:\n>> - if not p4PathStartsWith(path, self.depotPath):\n>> - continue\n>> + for entry in p4CmdList(args):\n>> + if not entry.has_key('code'):\n>> + continue\n>> + if entry['code'] == 'stat':\n>> + change_entry = entry\n>> + break\n>> + if not change_entry:\n>> + die('Failed to decode output of p4 change -o')\n>> + for key, value in change_entry.iteritems():\n>> + if key.startswith('File'):\n>> + if settings.has_key('depot-paths'):\n>> + if not [p for p in settings['depot-paths']\n>> + if p4PathStartsWith(value, p)]:\n>> + continue\n>> else:\n>> - inFilesSection = False\n>> - else:\n>> - if line.startswith(\"Files:\"):\n>> - inFilesSection = True\n>> -\n>> - template += line\n>> -\n>> + if not p4PathStartsWith(value, self.depotPath):\n>> + continue\n>> + files_list.append(value)\n>> + continue\n>> + # Output in the order expected by prepareLogMessage\n>> + for key in ['Change','Client','User','Status','Description','Jobs']:\n>> + if not change_entry.has_key(key):\n>> + continue\n>> + template += '\\n'\n>> + template += key + ':'\n>> + if key == 'Description':\n>> + template += '\\n'\n>> + for field_line in change_entry[key].splitlines():\n>> + template += '\\t'+field_line+'\\n'\n>> + if len(files_list) > 0:\n>> + template += '\\n'\n>> + template += 'Files:\\n'\n>> + for path in files_list:\n>> + template += '\\t'+path+'\\n'\n>> return template\n>>\n>> def edit_template(self, template_file):\n>> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\n>> index 3457d5db6..4962b6862 100755\n>> --- a/t/t9807-git-p4-submit.sh\n>> +++ b/t/t9807-git-p4-submit.sh\n>> @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' '\n>> )\n>> '\n>>\n>\n> I have never worked with p4 triggers and that might be\n> the reason why I don't understand your test case.\n> Maybe you can help me?\n>\n>> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n>> + test_when_finished cleanup_git &&\n>> + git p4 clone --dest=\"$git\" //depot &&\n>> + (\n>> + p4 triggers -i <<-EOF\n>> + Triggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n>> + EOF\n>> + ) &&\n>\n> You clone the test repo and install a trigger.\n>\n>> + (\n>> + cd \"$git\" &&\n>> + git config git-p4.skipSubmitEdit true &&\n>> + echo file20 >file20 &&\n>> + git add file20 &&\n>> + git commit -m file20 &&\n>> + git p4 submit\n>> + ) &&\n>\n> You make a new commit. This should run the \"echo verbose trigger\", right?\n\nYes, that's correct. In this case the trigger is run with p4 change\nand p4 changes\n\n>\n>> + (\n>> + p4 triggers -i <<-EOF\n>> + Triggers:\n>> + EOF\n>> + ) &&\n>\n> You delete the trigger.\n>\n>> + (\n>> + cd \"$cli\" &&\n>> + test_path_is_file file20\n>> + )\n>\n> You check that the file20 is available in P4.\n>\n>\n> What would happen if I run this test case without your patch?\n> Wouldn't it pass just fine?\n\nIf you run it without the patch for git-p4.py, the test doesn't pass\n\n> Wouldn't we need to check that no warning/error is in the\n> real change description?\n>\n\nthat can also be added, something like this: 'p4 change -o | grep\n\"verbose trigger\"' after setting the trigger?\n\n> Thanks,\n> Lars\n"},{"id":"323563","messageId":"94F87EDC-4F34-455E-88D5-F99C606EF628@gmail.com","threadId":"46218","inReplyTo":"CAKYtbVbOXZiZrsFGOKu=sFroSL-FBQo2wMaA9GmJvc-Uh7QZEA@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-30T10:13:02Z","receivedAt":"2017-06-30T10:13:26Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 30 Jun 2017, at 11:41, Miguel Torroja wrote:\n> \n> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider\n> wrote:\n>> \n>>> On 30 Jun 2017, at 00:46, miguel torroja wrote:\n>>> \n>>> The option -G of p4 (python marshal output) gives more context about the\n>>> data being output. That's useful when using the command \"change -o\" as\n>>> we can distinguish between warning/error line and real change description.\n>>> \n>>> Some p4 triggers in the server side generate some warnings when\n>>> executed. Unfortunately those messages are mixed with the output of\n>>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>>> in python marshal output (-G). The real change output is reported as\n>>> {'code':'stat'}\n>>> \n>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>> \n>>> Signed-off-by: Miguel Torroja \n>>> Signed-off-by: Junio C Hamano \n>>> ---\n>>> ...\n>> \n>> I have never worked with p4 triggers and that might be\n>> the reason why I don't understand your test case.\n>> Maybe you can help me?\n>> \n>>> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n>>> + test_when_finished cleanup_git &&\n>>> + git p4 clone --dest=\"$git\" //depot &&\n>>> + (\n>>> + p4 triggers -i <<-EOF\n>>> + Triggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n>>> + EOF\n>>> + ) &&\n>> \n>> You clone the test repo and install a trigger.\n>> \n>>> + (\n>>> + cd \"$git\" &&\n>>> + git config git-p4.skipSubmitEdit true &&\n>>> + echo file20 >file20 &&\n>>> + git add file20 &&\n>>> + git commit -m file20 &&\n>>> + git p4 submit\n>>> + ) &&\n>> \n>> You make a new commit. This should run the \"echo verbose trigger\", right?\n> \n> Yes, that's correct. In this case the trigger is run with p4 change\n> and p4 changes\n> \n>> \n>>> + (\n>>> + p4 triggers -i <<-EOF\n>>> + Triggers:\n>>> + EOF\n>>> + ) &&\n>> \n>> You delete the trigger.\n>> \n>>> + (\n>>> + cd \"$cli\" &&\n>>> + test_path_is_file file20\n>>> + )\n>> \n>> You check that the file20 is available in P4.\n>> \n>> \n>> What would happen if I run this test case without your patch?\n>> Wouldn't it pass just fine?\n> \n> If you run it without the patch for git-p4.py, the test doesn't pass\n\nYou are right. I did not run \"make\" properly before running the test :)\n\n\n>> Wouldn't we need to check that no warning/error is in the\n>> real change description?\n>> \n> \n> that can also be added, something like this: 'p4 change -o | grep\n> \"verbose trigger\"' after setting the trigger?\n\nYeah, maybe. I hope this is no stupid question, but: If you clone the\nrepo with git-p4 *again* ... would you see the \"verbose trigger\" output\nin the Git commit message?\n\n\n- Lars"},{"id":"323570","messageId":"CAKYtbVYCK_8jjW_B-Mmd3heUabTiTq0Lakf1Znz2ptipQwhEJQ@mail.gmail.com","threadId":"46218","inReplyTo":"94F87EDC-4F34-455E-88D5-F99C606EF628@gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-06-30T16:02:12Z","receivedAt":"2017-06-30T16:02:20Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider\n wrote:\n>\n>> On 30 Jun 2017, at 11:41, Miguel Torroja wrote:\n>>\n>> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider\n>> wrote:\n>>>\n>>>> On 30 Jun 2017, at 00:46, miguel torroja wrote:\n>>>>\n>>>> The option -G of p4 (python marshal output) gives more context about the\n>>>> data being output. That's useful when using the command \"change -o\" as\n>>>> we can distinguish between warning/error line and real change description.\n>>>>\n>>>> Some p4 triggers in the server side generate some warnings when\n>>>> executed. Unfortunately those messages are mixed with the output of\n>>>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>>>> in python marshal output (-G). The real change output is reported as\n>>>> {'code':'stat'}\n>>>>\n>>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>>>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>>>\n>>>> Signed-off-by: Miguel Torroja \n>>>> Signed-off-by: Junio C Hamano \n>>>> ---\n>>>> ...\n>>>\n>>> I have never worked with p4 triggers and that might be\n>>> the reason why I don't understand your test case.\n>>> Maybe you can help me?\n>>>\n>>>> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n>>>> + test_when_finished cleanup_git &&\n>>>> + git p4 clone --dest=\"$git\" //depot &&\n>>>> + (\n>>>> + p4 triggers -i <<-EOF\n>>>> + Triggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n>>>> + EOF\n>>>> + ) &&\n>>>\n>>> You clone the test repo and install a trigger.\n>>>\n>>>> + (\n>>>> + cd \"$git\" &&\n>>>> + git config git-p4.skipSubmitEdit true &&\n>>>> + echo file20 >file20 &&\n>>>> + git add file20 &&\n>>>> + git commit -m file20 &&\n>>>> + git p4 submit\n>>>> + ) &&\n>>>\n>>> You make a new commit. This should run the \"echo verbose trigger\", right?\n>>\n>> Yes, that's correct. In this case the trigger is run with p4 change\n>> and p4 changes\n>>\n>>>\n>>>> + (\n>>>> + p4 triggers -i <<-EOF\n>>>> + Triggers:\n>>>> + EOF\n>>>> + ) &&\n>>>\n>>> You delete the trigger.\n>>>\n>>>> + (\n>>>> + cd \"$cli\" &&\n>>>> + test_path_is_file file20\n>>>> + )\n>>>\n>>> You check that the file20 is available in P4.\n>>>\n>>>\n>>> What would happen if I run this test case without your patch?\n>>> Wouldn't it pass just fine?\n>>\n>> If you run it without the patch for git-p4.py, the test doesn't pass\n>\n> You are right. I did not run \"make\" properly before running the test :)\n>\n>\n>>> Wouldn't we need to check that no warning/error is in the\n>>> real change description?\n>>>\n>>\n>> that can also be added, something like this: 'p4 change -o | grep\n>> \"verbose trigger\"' after setting the trigger?\n>\n> Yeah, maybe. I hope this is no stupid question, but: If you clone the\n> repo with git-p4 *again* ... would you see the \"verbose trigger\" output\n> in the Git commit message?\n>\n\nThe commands that are affected are the ones that don't use the -G\noption, as everything is sent to the standard output without being\nable to filter out what is the real contents or just info messages.\nThat's not the case with the python output (-G). Having said that... I\ntried what you just said (just to be sure) and the function\np4_last_change fails... as it expects the first dictionary returned by\np4CmdList is the one that contains the change:\n\"int(results[0]['change'])\" and that's not the case as it's an info\nentry (no 'change' key, that's in the next entry...) I'll update with\nnew patches\n\nI didn't notice that before because the P4 server we have in our\noffice only outputs extra info messages with the command \"p4 change\".\n\n\n> - Lars\n"},{"id":"323791","messageId":"CAKYtbVYwzSJJ=tS-GoXRPbXr6hX2K=bkHS+s2wYD_VujTwHo5A@mail.gmail.com","threadId":"46218","inReplyTo":"CAKYtbVYCK_8jjW_B-Mmd3heUabTiTq0Lakf1Znz2ptipQwhEJQ@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-03T22:53:30Z","receivedAt":"2017-07-03T22:53:37Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"I changed the patch a little bit, first change is to ignore by default\nany {'code':'info'} in p4CmdList (they are not exposed to the caller)\nas those are the verbose messages from triggers (p4 Debug does show\nthem). the second change is to check the p4 trigger is really set in\nthe test (Lars suggestion),\n\nOn Fri, Jun 30, 2017 at 6:02 PM, Miguel Torroja\n wrote:\n> On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider\n> wrote:\n>>\n>>> On 30 Jun 2017, at 11:41, Miguel Torroja wrote:\n>>>\n>>> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider\n>>> wrote:\n>>>>\n>>>>> On 30 Jun 2017, at 00:46, miguel torroja wrote:\n>>>>>\n>>>>> The option -G of p4 (python marshal output) gives more context about the\n>>>>> data being output. That's useful when using the command \"change -o\" as\n>>>>> we can distinguish between warning/error line and real change description.\n>>>>>\n>>>>> Some p4 triggers in the server side generate some warnings when\n>>>>> executed. Unfortunately those messages are mixed with the output of\n>>>>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>>>>> in python marshal output (-G). The real change output is reported as\n>>>>> {'code':'stat'}\n>>>>>\n>>>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>>>>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>>>>\n>>>>> Signed-off-by: Miguel Torroja \n>>>>> Signed-off-by: Junio C Hamano \n>>>>> ---\n>>>>> ...\n>>>>\n>>>> I have never worked with p4 triggers and that might be\n>>>> the reason why I don't understand your test case.\n>>>> Maybe you can help me?\n>>>>\n>>>>> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n>>>>> + test_when_finished cleanup_git &&\n>>>>> + git p4 clone --dest=\"$git\" //depot &&\n>>>>> + (\n>>>>> + p4 triggers -i <<-EOF\n>>>>> + Triggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n>>>>> + EOF\n>>>>> + ) &&\n>>>>\n>>>> You clone the test repo and install a trigger.\n>>>>\n>>>>> + (\n>>>>> + cd \"$git\" &&\n>>>>> + git config git-p4.skipSubmitEdit true &&\n>>>>> + echo file20 >file20 &&\n>>>>> + git add file20 &&\n>>>>> + git commit -m file20 &&\n>>>>> + git p4 submit\n>>>>> + ) &&\n>>>>\n>>>> You make a new commit. This should run the \"echo verbose trigger\", right?\n>>>\n>>> Yes, that's correct. In this case the trigger is run with p4 change\n>>> and p4 changes\n>>>\n>>>>\n>>>>> + (\n>>>>> + p4 triggers -i <<-EOF\n>>>>> + Triggers:\n>>>>> + EOF\n>>>>> + ) &&\n>>>>\n>>>> You delete the trigger.\n>>>>\n>>>>> + (\n>>>>> + cd \"$cli\" &&\n>>>>> + test_path_is_file file20\n>>>>> + )\n>>>>\n>>>> You check that the file20 is available in P4.\n>>>>\n>>>>\n>>>> What would happen if I run this test case without your patch?\n>>>> Wouldn't it pass just fine?\n>>>\n>>> If you run it without the patch for git-p4.py, the test doesn't pass\n>>\n>> You are right. I did not run \"make\" properly before running the test :)\n>>\n>>\n>>>> Wouldn't we need to check that no warning/error is in the\n>>>> real change description?\n>>>>\n>>>\n>>> that can also be added, something like this: 'p4 change -o | grep\n>>> \"verbose trigger\"' after setting the trigger?\n>>\n>> Yeah, maybe. I hope this is no stupid question, but: If you clone the\n>> repo with git-p4 *again* ... would you see the \"verbose trigger\" output\n>> in the Git commit message?\n>>\n>\n> The commands that are affected are the ones that don't use the -G\n> option, as everything is sent to the standard output without being\n> able to filter out what is the real contents or just info messages.\n> That's not the case with the python output (-G). Having said that... I\n> tried what you just said (just to be sure) and the function\n> p4_last_change fails... as it expects the first dictionary returned by\n> p4CmdList is the one that contains the change:\n> \"int(results[0]['change'])\" and that's not the case as it's an info\n> entry (no 'change' key, that's in the next entry...) I'll update with\n> new patches\n>\n> I didn't notice that before because the P4 server we have in our\n> office only outputs extra info messages with the command \"p4 change\".\n>\n>\n>> - Lars\n"},{"id":"323792","messageId":"20170703225731.21212-1-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"94F87EDC-4F34-455E-88D5-F99C606EF628@gmail.com","subject":"[PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-03T22:57:31Z","receivedAt":"2017-07-03T22:58:18Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The option -G of p4 (python marshal output) gives more context about the\ndata being output. That's useful when using the command \"change -o\" as\nwe can distinguish between warning/error line and real change description.\n\nSome p4 triggers in the server side generate some warnings when\nexecuted. Unfortunately those messages are mixed with the output of\n\"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\nin python marshal output (-G). The real change output is reported as\n{'code':'stat'}\n\nthe function p4CmdList accepts a new argument: skip_info. When set to\nTrue it ignores any 'code':'info' entry (skip_info=True by default).\n\nA new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\nthat outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n\nSigned-off-by: Miguel Torroja \n---\n git-p4.py | 90 ++++++++++++++++++++++++++++++++----------------\n t/t9807-git-p4-submit.sh | 30 ++++++++++++++++\n 2 files changed, 91 insertions(+), 29 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da91..a262e3253 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -509,7 +509,7 @@ def isModeExec(mode):\n def isModeExecChanged(src_mode, dst_mode):\n return isModeExec(src_mode) != isModeExec(dst_mode)\n \n-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):\n \n if isinstance(cmd,basestring):\n cmd = \"-G \" + cmd\n@@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n try:\n while True:\n entry = marshal.load(p4.stdout)\n+ if skip_info:\n+ if 'code' in entry and entry['code'] == 'info':\n+ continue\n if cb is not None:\n cb(entry)\n else:\n@@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n cmd += [\"%s...@%s\" % (p, revisionRange)]\n \n # Insert changes in chronological order\n- for line in reversed(p4_read_pipe_lines(cmd)):\n- changes.add(int(line.split(\" \")[1]))\n+ for entry in reversed(p4CmdList(cmd)):\n+ if entry.has_key('p4ExitCode'):\n+ die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n+ if not entry.has_key('change'):\n+ continue\n+ changes.add(int(entry['change']))\n \n if not block_size:\n break\n@@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):\n \n [upstream, settings] = findUpstreamBranchPoint()\n \n- template = \"\"\n+ template = \"\"\"\\\n+# A Perforce Change Specification.\n+#\n+# Change: The change number. 'new' on a new changelist.\n+# Date: The date this specification was last modified.\n+# Client: The client on which the changelist was created. Read-only.\n+# User: The user who created the changelist.\n+# Status: Either 'pending' or 'submitted'. Read-only.\n+# Type: Either 'public' or 'restricted'. Default is 'public'.\n+# Description: Comments about the changelist. Required.\n+# Jobs: What opened jobs are to be closed by this changelist.\n+# You may delete jobs from this list. (New changelists only.)\n+# Files: What opened files from the default changelist are to be added\n+# to this changelist. You may delete files from this list.\n+# (New changelists only.)\n+\"\"\"\n+ files_list = []\n inFilesSection = False\n+ change_entry = None\n args = ['change', '-o']\n if changelist:\n args.append(str(changelist))\n-\n- for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n- if not [p for p in settings['depot-paths']\n- if p4PathStartsWith(path, p)]:\n- continue\n- else:\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ for entry in p4CmdList(args):\n+ if not entry.has_key('code'):\n+ continue\n+ if entry['code'] == 'stat':\n+ change_entry = entry\n+ break\n+ if not change_entry:\n+ die('Failed to decode output of p4 change -o')\n+ for key, value in change_entry.iteritems():\n+ if key.startswith('File'):\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(value, p)]:\n+ continue\n else:\n- inFilesSection = False\n- else:\n- if line.startswith(\"Files:\"):\n- inFilesSection = True\n-\n- template += line\n-\n+ if not p4PathStartsWith(value, self.depotPath):\n+ continue\n+ files_list.append(value)\n+ continue\n+ # Output in the order expected by prepareLogMessage\n+ for key in ['Change','Client','User','Status','Description','Jobs']:\n+ if not change_entry.has_key(key):\n+ continue\n+ template += '\\n'\n+ template += key + ':'\n+ if key == 'Description':\n+ template += '\\n'\n+ for field_line in change_entry[key].splitlines():\n+ template += '\\t'+field_line+'\\n'\n+ if len(files_list) > 0:\n+ template += '\\n'\n+ template += 'Files:\\n'\n+ for path in files_list:\n+ template += '\\t'+path+'\\n'\n return template\n \n def edit_template(self, template_file):\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 3457d5db6..b630895a7 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' '\n \t)\n '\n \n+test_expect_success 'description with extra lines from verbose p4 trigger' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tp4 change -o | grep -s \"verbose trigger\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\techo file20 >file20 &&\n+\t\tgit add file20 &&\n+\t\tgit commit -m file20 &&\n+\t\tgit p4 submit\n+\t) &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\ttest_path_is_file file20\n+\t)\n+'\n+\n test_expect_success 'submit --prepare-p4-only' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.11.0\n\n"},{"id":"324167","messageId":"CAE5ih7-Sy9YmGbLs=wzfxXCSFLkEotqLRuu_xNz9x=7BhvrvnA@mail.gmail.com","threadId":"46218","inReplyTo":"20170703225731.21212-1-miguel.torroja@gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-07-11T08:35:17Z","receivedAt":"2017-07-11T08:35:54Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 3 July 2017 at 23:57, Miguel Torroja wrote:\n> The option -G of p4 (python marshal output) gives more context about the\n> data being output. That's useful when using the command \"change -o\" as\n> we can distinguish between warning/error line and real change description.\n>\n> Some p4 triggers in the server side generate some warnings when\n> executed. Unfortunately those messages are mixed with the output of\n> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n> in python marshal output (-G). The real change output is reported as\n> {'code':'stat'}\n>\n> the function p4CmdList accepts a new argument: skip_info. When set to\n> True it ignores any 'code':'info' entry (skip_info=True by default).\n>\n> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n\nThe latest version of mt/p4-parse-G-output (09521c7a0) seems to break\nt9813-git-p4-preserve-users.sh.\n\nI don't quite know why, but I wonder if it's the change to p4CmdList() ?\n\nLuke\n\n>\n> Signed-off-by: Miguel Torroja \n> ---\n> git-p4.py | 90 ++++++++++++++++++++++++++++++++----------------\n> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++\n> 2 files changed, 91 insertions(+), 29 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 8d151da91..a262e3253 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -509,7 +509,7 @@ def isModeExec(mode):\n> def isModeExecChanged(src_mode, dst_mode):\n> return isModeExec(src_mode) != isModeExec(dst_mode)\n>\n> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):\n>\n> if isinstance(cmd,basestring):\n> cmd = \"-G \" + cmd\n> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n> try:\n> while True:\n> entry = marshal.load(p4.stdout)\n> + if skip_info:\n> + if 'code' in entry and entry['code'] == 'info':\n> + continue\n> if cb is not None:\n> cb(entry)\n> else:\n> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n> cmd += [\"%s...@%s\" % (p, revisionRange)]\n>\n> # Insert changes in chronological order\n> - for line in reversed(p4_read_pipe_lines(cmd)):\n> - changes.add(int(line.split(\" \")[1]))\n> + for entry in reversed(p4CmdList(cmd)):\n> + if entry.has_key('p4ExitCode'):\n> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n> + if not entry.has_key('change'):\n> + continue\n> + changes.add(int(entry['change']))\n>\n> if not block_size:\n> break\n> @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):\n>\n> [upstream, settings] = findUpstreamBranchPoint()\n>\n> - template = \"\"\n> + template = \"\"\"\\\n> +# A Perforce Change Specification.\n> +#\n> +# Change: The change number. 'new' on a new changelist.\n> +# Date: The date this specification was last modified.\n> +# Client: The client on which the changelist was created. Read-only.\n> +# User: The user who created the changelist.\n> +# Status: Either 'pending' or 'submitted'. Read-only.\n> +# Type: Either 'public' or 'restricted'. Default is 'public'.\n> +# Description: Comments about the changelist. Required.\n> +# Jobs: What opened jobs are to be closed by this changelist.\n> +# You may delete jobs from this list. (New changelists only.)\n> +# Files: What opened files from the default changelist are to be added\n> +# to this changelist. You may delete files from this list.\n> +# (New changelists only.)\n> +\"\"\"\n> + files_list = []\n> inFilesSection = False\n> + change_entry = None\n> args = ['change', '-o']\n> if changelist:\n> args.append(str(changelist))\n> -\n> - for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n> - if not [p for p in settings['depot-paths']\n> - if p4PathStartsWith(path, p)]:\n> - continue\n> - else:\n> - if not p4PathStartsWith(path, self.depotPath):\n> - continue\n> + for entry in p4CmdList(args):\n> + if not entry.has_key('code'):\n> + continue\n> + if entry['code'] == 'stat':\n> + change_entry = entry\n> + break\n> + if not change_entry:\n> + die('Failed to decode output of p4 change -o')\n> + for key, value in change_entry.iteritems():\n> + if key.startswith('File'):\n> + if settings.has_key('depot-paths'):\n> + if not [p for p in settings['depot-paths']\n> + if p4PathStartsWith(value, p)]:\n> + continue\n> else:\n> - inFilesSection = False\n> - else:\n> - if line.startswith(\"Files:\"):\n> - inFilesSection = True\n> -\n> - template += line\n> -\n> + if not p4PathStartsWith(value, self.depotPath):\n> + continue\n> + files_list.append(value)\n> + continue\n> + # Output in the order expected by prepareLogMessage\n> + for key in ['Change','Client','User','Status','Description','Jobs']:\n> + if not change_entry.has_key(key):\n> + continue\n> + template += '\\n'\n> + template += key + ':'\n> + if key == 'Description':\n> + template += '\\n'\n> + for field_line in change_entry[key].splitlines():\n> + template += '\\t'+field_line+'\\n'\n> + if len(files_list) > 0:\n> + template += '\\n'\n> + template += 'Files:\\n'\n> + for path in files_list:\n> + template += '\\t'+path+'\\n'\n> return template\n>\n> def edit_template(self, template_file):\n> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\n> index 3457d5db6..b630895a7 100755\n> --- a/t/t9807-git-p4-submit.sh\n> +++ b/t/t9807-git-p4-submit.sh\n> @@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' '\n> )\n> '\n>\n> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n> + test_when_finished cleanup_git &&\n> + git p4 clone --dest=\"$git\" //depot &&\n> + (\n> + p4 triggers -i <<-EOF\n> + Triggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n> + EOF\n> + ) &&\n> + (\n> + p4 change -o | grep -s \"verbose trigger\"\n> + ) &&\n> + (\n> + cd \"$git\" &&\n> + git config git-p4.skipSubmitEdit true &&\n> + echo file20 >file20 &&\n> + git add file20 &&\n> + git commit -m file20 &&\n> + git p4 submit\n> + ) &&\n> + (\n> + p4 triggers -i <<-EOF\n> + Triggers:\n> + EOF\n> + ) &&\n> + (\n> + cd \"$cli\" &&\n> + test_path_is_file file20\n> + )\n> +'\n> +\n> test_expect_success 'submit --prepare-p4-only' '\n> test_when_finished cleanup_git &&\n> git p4 clone --dest=\"$git\" //depot &&\n> --\n> 2.11.0\n>\n"},{"id":"324239","messageId":"CAKYtbVbNMzPe17rBUihm5Q+mgsJUhWMs3LKDp5jEJFdL+ieafg@mail.gmail.com","threadId":"46218","inReplyTo":"CAE5ih7-Sy9YmGbLs=wzfxXCSFLkEotqLRuu_xNz9x=7BhvrvnA@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-11T22:35:49Z","receivedAt":"2017-07-11T22:35:56Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"Hi Luke,\n\n\nMy bad as I didn't check that case.\nIt was p4CmdList as you said. the default value of the new field\nskip_info (set to True) ignores any info messages. and the script is\nwaiting for a valid message.\nIf I set it to False, then it does return an info entry and it accepts\nthe submit change\n\nI'm sending another patch update\n\nOn Tue, Jul 11, 2017 at 10:35 AM, Luke Diamand wrote:\n> On 3 July 2017 at 23:57, Miguel Torroja wrote:\n>> The option -G of p4 (python marshal output) gives more context about the\n>> data being output. That's useful when using the command \"change -o\" as\n>> we can distinguish between warning/error line and real change description.\n>>\n>> Some p4 triggers in the server side generate some warnings when\n>> executed. Unfortunately those messages are mixed with the output of\n>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>> in python marshal output (-G). The real change output is reported as\n>> {'code':'stat'}\n>>\n>> the function p4CmdList accepts a new argument: skip_info. When set to\n>> True it ignores any 'code':'info' entry (skip_info=True by default).\n>>\n>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger\n>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>\n> The latest version of mt/p4-parse-G-output (09521c7a0) seems to break\n> t9813-git-p4-preserve-users.sh.\n>\n> I don't quite know why, but I wonder if it's the change to p4CmdList() ?\n>\n> Luke\n>\n>>\n>> Signed-off-by: Miguel Torroja \n>> ---\n>> git-p4.py | 90 ++++++++++++++++++++++++++++++++----------------\n>> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++\n>> 2 files changed, 91 insertions(+), 29 deletions(-)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 8d151da91..a262e3253 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -509,7 +509,7 @@ def isModeExec(mode):\n>> def isModeExecChanged(src_mode, dst_mode):\n>> return isModeExec(src_mode) != isModeExec(dst_mode)\n>>\n>> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n>> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):\n>>\n>> if isinstance(cmd,basestring):\n>> cmd = \"-G \" + cmd\n>> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n>> try:\n>> while True:\n>> entry = marshal.load(p4.stdout)\n>> + if skip_info:\n>> + if 'code' in entry and entry['code'] == 'info':\n>> + continue\n>> if cb is not None:\n>> cb(entry)\n>> else:\n>> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n>> cmd += [\"%s...@%s\" % (p, revisionRange)]\n>>\n>> # Insert changes in chronological order\n>> - for line in reversed(p4_read_pipe_lines(cmd)):\n>> - changes.add(int(line.split(\" \")[1]))\n>> + for entry in reversed(p4CmdList(cmd)):\n>> + if entry.has_key('p4ExitCode'):\n>> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n>> + if not entry.has_key('change'):\n>> + continue\n>> + changes.add(int(entry['change']))\n>>\n>> if not block_size:\n>> break\n>> @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):\n>>\n>> [upstream, settings] = findUpstreamBranchPoint()\n>>\n>> - template = \"\"\n>> + template = \"\"\"\\\n>> +# A Perforce Change Specification.\n>> +#\n>> +# Change: The change number. 'new' on a new changelist.\n>> +# Date: The date this specification was last modified.\n>> +# Client: The client on which the changelist was created. Read-only.\n>> +# User: The user who created the changelist.\n>> +# Status: Either 'pending' or 'submitted'. Read-only.\n>> +# Type: Either 'public' or 'restricted'. Default is 'public'.\n>> +# Description: Comments about the changelist. Required.\n>> +# Jobs: What opened jobs are to be closed by this changelist.\n>> +# You may delete jobs from this list. (New changelists only.)\n>> +# Files: What opened files from the default changelist are to be added\n>> +# to this changelist. You may delete files from this list.\n>> +# (New changelists only.)\n>> +\"\"\"\n>> + files_list = []\n>> inFilesSection = False\n>> + change_entry = None\n>> args = ['change', '-o']\n>> if changelist:\n>> args.append(str(changelist))\n>> -\n>> - for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n>> - if not [p for p in settings['depot-paths']\n>> - if p4PathStartsWith(path, p)]:\n>> - continue\n>> - else:\n>> - if not p4PathStartsWith(path, self.depotPath):\n>> - continue\n>> + for entry in p4CmdList(args):\n>> + if not entry.has_key('code'):\n>> + continue\n>> + if entry['code'] == 'stat':\n>> + change_entry = entry\n>> + break\n>> + if not change_entry:\n>> + die('Failed to decode output of p4 change -o')\n>> + for key, value in change_entry.iteritems():\n>> + if key.startswith('File'):\n>> + if settings.has_key('depot-paths'):\n>> + if not [p for p in settings['depot-paths']\n>> + if p4PathStartsWith(value, p)]:\n>> + continue\n>> else:\n>> - inFilesSection = False\n>> - else:\n>> - if line.startswith(\"Files:\"):\n>> - inFilesSection = True\n>> -\n>> - template += line\n>> -\n>> + if not p4PathStartsWith(value, self.depotPath):\n>> + continue\n>> + files_list.append(value)\n>> + continue\n>> + # Output in the order expected by prepareLogMessage\n>> + for key in ['Change','Client','User','Status','Description','Jobs']:\n>> + if not change_entry.has_key(key):\n>> + continue\n>> + template += '\\n'\n>> + template += key + ':'\n>> + if key == 'Description':\n>> + template += '\\n'\n>> + for field_line in change_entry[key].splitlines():\n>> + template += '\\t'+field_line+'\\n'\n>> + if len(files_list) > 0:\n>> + template += '\\n'\n>> + template += 'Files:\\n'\n>> + for path in files_list:\n>> + template += '\\t'+path+'\\n'\n>> return template\n>>\n>> def edit_template(self, template_file):\n>> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\n>> index 3457d5db6..b630895a7 100755\n>> --- a/t/t9807-git-p4-submit.sh\n>> +++ b/t/t9807-git-p4-submit.sh\n>> @@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' '\n>> )\n>> '\n>>\n>> +test_expect_success 'description with extra lines from verbose p4 trigger' '\n>> + test_when_finished cleanup_git &&\n>> + git p4 clone --dest=\"$git\" //depot &&\n>> + (\n>> + p4 triggers -i <<-EOF\n>> + Triggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n>> + EOF\n>> + ) &&\n>> + (\n>> + p4 change -o | grep -s \"verbose trigger\"\n>> + ) &&\n>> + (\n>> + cd \"$git\" &&\n>> + git config git-p4.skipSubmitEdit true &&\n>> + echo file20 >file20 &&\n>> + git add file20 &&\n>> + git commit -m file20 &&\n>> + git p4 submit\n>> + ) &&\n>> + (\n>> + p4 triggers -i <<-EOF\n>> + Triggers:\n>> + EOF\n>> + ) &&\n>> + (\n>> + cd \"$cli\" &&\n>> + test_path_is_file file20\n>> + )\n>> +'\n>> +\n>> test_expect_success 'submit --prepare-p4-only' '\n>> test_when_finished cleanup_git &&\n>> git p4 clone --dest=\"$git\" //depot &&\n>> --\n>> 2.11.0\n>>\n"},{"id":"324243","messageId":"20170711225316.10608-1-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"CAE5ih7-Sy9YmGbLs=wzfxXCSFLkEotqLRuu_xNz9x=7BhvrvnA@mail.gmail.com","subject":"[PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-11T22:53:16Z","receivedAt":"2017-07-11T22:53:33Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The option -G of p4 (python marshal output) gives more context about the\ndata being output. That's useful when using the command \"change -o\" as\nwe can distinguish between warning/error line and real change description.\n\nSome p4 triggers in the server side generate some warnings when\nexecuted. Unfortunately those messages are mixed with the output of\n\"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\nin python marshal output (-G). The real change output is reported as\n{'code':'stat'}\n\nthe function p4CmdList accepts a new argument: skip_info. When set to\nTrue it ignores any 'code':'info' entry (skip_info=True by default).\n\nA new test has been created in t9807-git-p4-submit.sh adding a p4 trigger\nthat outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n\nSigned-off-by: Miguel Torroja \n---\n git-p4.py | 92 ++++++++++++++++++++++++++++++++----------------\n t/t9807-git-p4-submit.sh | 30 ++++++++++++++++\n 2 files changed, 92 insertions(+), 30 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da91..1facf32db 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -509,7 +509,7 @@ def isModeExec(mode):\n def isModeExecChanged(src_mode, dst_mode):\n return isModeExec(src_mode) != isModeExec(dst_mode)\n \n-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):\n \n if isinstance(cmd,basestring):\n cmd = \"-G \" + cmd\n@@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n try:\n while True:\n entry = marshal.load(p4.stdout)\n+ if skip_info:\n+ if 'code' in entry and entry['code'] == 'info':\n+ continue\n if cb is not None:\n cb(entry)\n else:\n@@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n cmd += [\"%s...@%s\" % (p, revisionRange)]\n \n # Insert changes in chronological order\n- for line in reversed(p4_read_pipe_lines(cmd)):\n- changes.add(int(line.split(\" \")[1]))\n+ for entry in reversed(p4CmdList(cmd)):\n+ if entry.has_key('p4ExitCode'):\n+ die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n+ if not entry.has_key('change'):\n+ continue\n+ changes.add(int(entry['change']))\n \n if not block_size:\n break\n@@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap):\n c['User'] = newUser\n input = marshal.dumps(c)\n \n- result = p4CmdList(\"change -f -i\", stdin=input)\n+ result = p4CmdList(\"change -f -i\", stdin=input,skip_info=False)\n for r in result:\n if r.has_key('code'):\n if r['code'] == 'error':\n@@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap):\n \n [upstream, settings] = findUpstreamBranchPoint()\n \n- template = \"\"\n+ template = \"\"\"\\\n+# A Perforce Change Specification.\n+#\n+# Change: The change number. 'new' on a new changelist.\n+# Date: The date this specification was last modified.\n+# Client: The client on which the changelist was created. Read-only.\n+# User: The user who created the changelist.\n+# Status: Either 'pending' or 'submitted'. Read-only.\n+# Type: Either 'public' or 'restricted'. Default is 'public'.\n+# Description: Comments about the changelist. Required.\n+# Jobs: What opened jobs are to be closed by this changelist.\n+# You may delete jobs from this list. (New changelists only.)\n+# Files: What opened files from the default changelist are to be added\n+# to this changelist. You may delete files from this list.\n+# (New changelists only.)\n+\"\"\"\n+ files_list = []\n inFilesSection = False\n+ change_entry = None\n args = ['change', '-o']\n if changelist:\n args.append(str(changelist))\n-\n- for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n- if not [p for p in settings['depot-paths']\n- if p4PathStartsWith(path, p)]:\n- continue\n- else:\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ for entry in p4CmdList(args):\n+ if not entry.has_key('code'):\n+ continue\n+ if entry['code'] == 'stat':\n+ change_entry = entry\n+ break\n+ if not change_entry:\n+ die('Failed to decode output of p4 change -o')\n+ for key, value in change_entry.iteritems():\n+ if key.startswith('File'):\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(value, p)]:\n+ continue\n else:\n- inFilesSection = False\n- else:\n- if line.startswith(\"Files:\"):\n- inFilesSection = True\n-\n- template += line\n-\n+ if not p4PathStartsWith(value, self.depotPath):\n+ continue\n+ files_list.append(value)\n+ continue\n+ # Output in the order expected by prepareLogMessage\n+ for key in ['Change','Client','User','Status','Description','Jobs']:\n+ if not change_entry.has_key(key):\n+ continue\n+ template += '\\n'\n+ template += key + ':'\n+ if key == 'Description':\n+ template += '\\n'\n+ for field_line in change_entry[key].splitlines():\n+ template += '\\t'+field_line+'\\n'\n+ if len(files_list) > 0:\n+ template += '\\n'\n+ template += 'Files:\\n'\n+ for path in files_list:\n+ template += '\\t'+path+'\\n'\n return template\n \n def edit_template(self, template_file):\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 3457d5db6..b630895a7 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' '\n \t)\n '\n \n+test_expect_success 'description with extra lines from verbose p4 trigger' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tp4 change -o | grep -s \"verbose trigger\"\n+\t) &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\techo file20 >file20 &&\n+\t\tgit add file20 &&\n+\t\tgit commit -m file20 &&\n+\t\tgit p4 submit\n+\t) &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\ttest_path_is_file file20\n+\t)\n+'\n+\n test_expect_success 'submit --prepare-p4-only' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.11.0\n\n"},{"id":"324257","messageId":"CAE5ih78mrTz1sfJbRSuPTNojxWyH_1JFDY2pe7GMAZdPhzcvpA@mail.gmail.com","threadId":"46218","inReplyTo":"20170711225316.10608-1-miguel.torroja@gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-07-12T08:25:01Z","receivedAt":"2017-07-12T08:25:09Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 11 July 2017 at 23:53, Miguel Torroja wrote:\n> The option -G of p4 (python marshal output) gives more context about the\n> data being output. That's useful when using the command \"change -o\" as\n> we can distinguish between warning/error line and real change description.\n>\n> Some p4 triggers in the server side generate some warnings when\n> executed. Unfortunately those messages are mixed with the output of\n> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n> in python marshal output (-G). The real change output is reported as\n> {'code':'stat'}\n>\n> the function p4CmdList accepts a new argument: skip_info. When set to\n> True it ignores any 'code':'info' entry (skip_info=True by default).\n>\n> A new test has been created in t9807-git-p4-submit.sh adding a p4 trigger\n> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>\n> Signed-off-by: Miguel Torroja \n> ---\n> git-p4.py | 92 ++++++++++++++++++++++++++++++++----------------\n> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++\n> 2 files changed, 92 insertions(+), 30 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 8d151da91..1facf32db 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -509,7 +509,7 @@ def isModeExec(mode):\n> def isModeExecChanged(src_mode, dst_mode):\n> return isModeExec(src_mode) != isModeExec(dst_mode)\n>\n> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):\n>\n> if isinstance(cmd,basestring):\n> cmd = \"-G \" + cmd\n> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n> try:\n> while True:\n> entry = marshal.load(p4.stdout)\n> + if skip_info:\n> + if 'code' in entry and entry['code'] == 'info':\n> + continue\n> if cb is not None:\n> cb(entry)\n> else:\n> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n> cmd += [\"%s...@%s\" % (p, revisionRange)]\n>\n> # Insert changes in chronological order\n> - for line in reversed(p4_read_pipe_lines(cmd)):\n> - changes.add(int(line.split(\" \")[1]))\n> + for entry in reversed(p4CmdList(cmd)):\n> + if entry.has_key('p4ExitCode'):\n> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n> + if not entry.has_key('change'):\n> + continue\n> + changes.add(int(entry['change']))\n>\n> if not block_size:\n> break\n> @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap):\n> c['User'] = newUser\n> input = marshal.dumps(c)\n>\n> - result = p4CmdList(\"change -f -i\", stdin=input)\n> + result = p4CmdList(\"change -f -i\", stdin=input,skip_info=False)\n\nIs there any reason this change sets skip_info to False in this one\nplace, rather than defaulting to False (the original behavior) and\nsetting it to True where it's needed?\n\nI worry that there might be other unexpected side effects in places\nnot covered by the tests.\n\nThanks\nLuke\n"},{"id":"324258","messageId":"CAKYtbVb8T=edPG5539=uwDjHnCerLO2Oejy8bWK+giSS8nNGig@mail.gmail.com","threadId":"46218","inReplyTo":"CAE5ih78mrTz1sfJbRSuPTNojxWyH_1JFDY2pe7GMAZdPhzcvpA@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-12T10:16:44Z","receivedAt":"2017-07-12T10:16:51Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The motivation for setting skip_info default to True is because any\nextra message output by a p4 trigger to stdout, seems to be reported\nas {'code':'info'} when the p4 command output is marshalled.\n\nI though it was the less intrusive way to filter out the verbose\nserver trigger scripts, as some commands are waiting for a specific\norder and size of the list returned e.g:\n\n def p4_last_change():\n results = p4CmdList([\"changes\", \"-m\", \"1\"])\n return int(results[0]['change'])\n .\n def p4_describe(change):\n ds = p4CmdList([\"describe\", \"-s\", str(change)])\n if len(ds) != 1:\n die(\"p4 describe -s %d did not return 1 result: %s\" % (change, str(ds)))\n\nPrevious examples would be broken if we allow extra \"info\" marshalled\nmessages to be exposed.\n\nIn the case of the command that was broken with the new default\nbehaviour , when calling modfyChangelistUser, it is waiting for any\nmessage with 'data' that is not an error to consider command was\nsuccesful\n\n\nThanks,\n\n\nOn Wed, Jul 12, 2017 at 10:25 AM, Luke Diamand wrote:\n> On 11 July 2017 at 23:53, Miguel Torroja wrote:\n>> The option -G of p4 (python marshal output) gives more context about the\n>> data being output. That's useful when using the command \"change -o\" as\n>> we can distinguish between warning/error line and real change description.\n>>\n>> Some p4 triggers in the server side generate some warnings when\n>> executed. Unfortunately those messages are mixed with the output of\n>> \"p4 change -o\". Those extra warning lines are reported as {'code':'info'}\n>> in python marshal output (-G). The real change output is reported as\n>> {'code':'stat'}\n>>\n>> the function p4CmdList accepts a new argument: skip_info. When set to\n>> True it ignores any 'code':'info' entry (skip_info=True by default).\n>>\n>> A new test has been created in t9807-git-p4-submit.sh adding a p4 trigger\n>> that outputs extra lines with \"p4 change -o\" and \"p4 changes\"\n>>\n>> Signed-off-by: Miguel Torroja \n>> ---\n>> git-p4.py | 92 ++++++++++++++++++++++++++++++++----------------\n>> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++\n>> 2 files changed, 92 insertions(+), 30 deletions(-)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 8d151da91..1facf32db 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -509,7 +509,7 @@ def isModeExec(mode):\n>> def isModeExecChanged(src_mode, dst_mode):\n>> return isModeExec(src_mode) != isModeExec(dst_mode)\n>>\n>> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n>> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):\n>>\n>> if isinstance(cmd,basestring):\n>> cmd = \"-G \" + cmd\n>> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n>> try:\n>> while True:\n>> entry = marshal.load(p4.stdout)\n>> + if skip_info:\n>> + if 'code' in entry and entry['code'] == 'info':\n>> + continue\n>> if cb is not None:\n>> cb(entry)\n>> else:\n>> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n>> cmd += [\"%s...@%s\" % (p, revisionRange)]\n>>\n>> # Insert changes in chronological order\n>> - for line in reversed(p4_read_pipe_lines(cmd)):\n>> - changes.add(int(line.split(\" \")[1]))\n>> + for entry in reversed(p4CmdList(cmd)):\n>> + if entry.has_key('p4ExitCode'):\n>> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n>> + if not entry.has_key('change'):\n>> + continue\n>> + changes.add(int(entry['change']))\n>>\n>> if not block_size:\n>> break\n>> @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap):\n>> c['User'] = newUser\n>> input = marshal.dumps(c)\n>>\n>> - result = p4CmdList(\"change -f -i\", stdin=input)\n>> + result = p4CmdList(\"change -f -i\", stdin=input,skip_info=False)\n>\n> Is there any reason this change sets skip_info to False in this one\n> place, rather than defaulting to False (the original behavior) and\n> setting it to True where it's needed?\n>\n> I worry that there might be other unexpected side effects in places\n> not covered by the tests.\n>\n> Thanks\n> Luke\n"},{"id":"324269","messageId":"xmqqr2xl1suy.fsf@gitster.mtv.corp.google.com","threadId":"46218","inReplyTo":"CAKYtbVb8T=edPG5539=uwDjHnCerLO2Oejy8bWK+giSS8nNGig@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-12T17:13:09Z","receivedAt":"2017-07-12T17:13:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miguel Torroja writes:\n\n> The motivation for setting skip_info default to True is because any\n> extra message output by a p4 trigger to stdout, seems to be reported\n> as {'code':'info'} when the p4 command output is marshalled.\n>\n> I though it was the less intrusive way to filter out the verbose\n> server trigger scripts, as some commands are waiting for a specific\n> order and size of the list returned e.g:\n>\n> def p4_last_change():\n> results = p4CmdList([\"changes\", \"-m\", \"1\"])\n> return int(results[0]['change'])\n> .\n> def p4_describe(change):\n> ds = p4CmdList([\"describe\", \"-s\", str(change)])\n> if len(ds) != 1:\n> die(\"p4 describe -s %d did not return 1 result: %s\" % (change, str(ds)))\n>\n> Previous examples would be broken if we allow extra \"info\" marshalled\n> messages to be exposed.\n>\n> In the case of the command that was broken with the new default\n> behaviour , when calling modfyChangelistUser, it is waiting for any\n> message with 'data' that is not an error to consider command was\n> succesful\n\nI somehow feel that this logic is totally backwards. The current\ncallers of p4CmdList() before your patch did not special case an\nentry that was marked as 'info' in its 'code' field. Your new\ncaller, which switched from using p4_read_pipe_lines() to p4CmdList()\nis one caller that you *know* wants to special case such an entry\nand wanted to skip.\n\nYour original patch that was queued to 'pu' for a while and then\nejected from it after Travis saw an issue *assumed* that all other\ncallers to p4CmdList() also want to special case such an entry, and\nthat is why it made skip_info parameter default to True.\n\nThe difference between knowing and assuming is the cause of the bug\nyour original patch introduced into modifyChangelistUser(). \n\nThe way I read Luke's suggestion was that you can avoid making the\nsame mistake by not changing the behaviour for existing callers you\ndidn't look at.\n\nInstead of assuming everybody else do not want an entry with 'code'\nset to 'info', assume all the callers before your patch is doing\nfine, and when you *know* some of them are better off ignoring such\nan entry, explicitly tell them to do so, by:\n\n * The first patch adds skip_info parameter that defaults to False\n to p4CmdList() and do the special casing when it is set to True.\n\n * The second patch updates p4ChangesForPaths() to use the updated\n p4CmdList() and pass skip_info=True. It is OK to squash this\n step into the first patch.\n\n * The third and later patches, if you need them, each examines an\n existing caller of p4CmdList(), and add a new test to demonstrate\n the existing breakage that comes from not ignoring an entry whose\n 'code' is 'info'. That test would serve as a good documentation\n to explain why it is better for the caller to pass skip_info=True,\n so the same patch would also update the code to do so.\n\nWhile I was thinking the above through, here are a few cosmetic\nthings that I noticed. There is another comma that is not followed\nby a space in existing code that might want to be corrected in a\nclean-up patch but that is totally outside of the scope of this\nseries.\n\nThanks.\n\n git-p4.py | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 1facf32db7..0d75753bce 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1501,7 +1501,7 @@ class P4Submit(Command, P4UserMap):\n c['User'] = newUser\n input = marshal.dumps(c)\n \n- result = p4CmdList(\"change -f -i\", stdin=input,skip_info=False)\n+ result = p4CmdList(\"change -f -i\", stdin=input, skip_info=False)\n for r in result:\n if r.has_key('code'):\n if r['code'] == 'error':\n@@ -1575,7 +1575,7 @@ class P4Submit(Command, P4UserMap):\n files_list.append(value)\n continue\n # Output in the order expected by prepareLogMessage\n- for key in ['Change','Client','User','Status','Description','Jobs']:\n+ for key in ['Change', 'Client', 'User', 'Status', 'Description', 'Jobs']:\n if not change_entry.has_key(key):\n continue\n template += '\\n'\n"},{"id":"324342","messageId":"20170713070035.12731-1-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"xmqqr2xl1suy.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/3] git-p4: git-p4 tests with p4 triggers","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-13T07:00:33Z","receivedAt":"2017-07-13T07:01:06Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"Some p4 triggers in the server side generate some warnings when\nexecuted. Unfortunately those messages are mixed with the output of\np4 commands. A few git-p4 commands don't expect extra messages or output\nlines and may fail with verbose triggers.\nNew tests added are known to be broken.\n\nSigned-off-by: Miguel Torroja \n---\n t/t9831-git-p4-triggers.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 103 insertions(+)\n create mode 100755 t/t9831-git-p4-triggers.sh\n\ndiff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh\nnew file mode 100755\nindex 000000000..28cafe469\n--- /dev/null\n+++ b/t/t9831-git-p4-triggers.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='git p4 with server triggers'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\techo file1 >file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"change 1\"\n+\t\techo file2 >file2 &&\n+\t\tp4 add file2 &&\n+\t\tp4 submit -d \"change 2\"\n+\t)\n+'\n+\n+test_expect_failure 'clone with extra info lines from verbose p4 trigger' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tp4 change -o | grep -s \"verbose trigger\"\n+\t) &&\n+\tgit p4 clone --dest=\"$git\" //depot/@all &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t)\n+'\n+\n+test_expect_failure 'import with extra info lines from verbose p4 trigger' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\techo file3 >file3 &&\n+\t\tp4 add file3 &&\n+\t\tp4 submit -d \"change 3\"\n+\t) &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers: p4triggertest-command command pre-user-describe \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tp4 describe 1 | grep -s \"verbose trigger\"\n+\t) &&\n+\tgit p4 clone --dest=\"$git\" //depot/@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit p4 sync\n+\t)&&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t)\n+'\n+\n+test_expect_failure 'submit description with extra info lines from verbose p4 change trigger' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers: p4triggertest-command command pre-user-change \"echo verbose trigger\"\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tp4 change -o | grep -s \"verbose trigger\"\n+\t) &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\techo file4 >file4 &&\n+\t\tgit add file4 &&\n+\t\tgit commit -m file4 &&\n+\t\tgit p4 submit\n+\t) &&\n+\t(\n+\t\tp4 triggers -i <<-EOF\n+\t\tTriggers:\n+\t\tEOF\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\ttest_path_is_file file4\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n2.11.0\n\n"},{"id":"324343","messageId":"20170713070035.12731-2-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"20170713070035.12731-1-miguel.torroja@gmail.com","subject":"[PATCH 2/3] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-13T07:00:34Z","receivedAt":"2017-07-13T07:01:08Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The option -G of p4 (python marshal output) gives more context about the\ndata being output. That's useful when using the command \"change -o\" as\nwe can distinguish between warning/error line and real change description.\n\nThis fixes the case where a p4 trigger for \"p4 change\" is set and the command git-p4 submit is run.\n\nSigned-off-by: Miguel Torroja \n---\n git-p4.py | 85 +++++++++++++++++++++++++++++++---------------\n t/t9831-git-p4-triggers.sh | 2 +-\n 2 files changed, 58 insertions(+), 29 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 8d151da91..e3a2791e0 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n cmd += [\"%s...@%s\" % (p, revisionRange)]\n \n # Insert changes in chronological order\n- for line in reversed(p4_read_pipe_lines(cmd)):\n- changes.add(int(line.split(\" \")[1]))\n+ for entry in reversed(p4CmdList(cmd)):\n+ if entry.has_key('p4ExitCode'):\n+ die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))\n+ if not entry.has_key('change'):\n+ continue\n+ changes.add(int(entry['change']))\n \n if not block_size:\n break\n@@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):\n \n [upstream, settings] = findUpstreamBranchPoint()\n \n- template = \"\"\n+ template = \"\"\"\\\n+# A Perforce Change Specification.\n+#\n+# Change: The change number. 'new' on a new changelist.\n+# Date: The date this specification was last modified.\n+# Client: The client on which the changelist was created. Read-only.\n+# User: The user who created the changelist.\n+# Status: Either 'pending' or 'submitted'. Read-only.\n+# Type: Either 'public' or 'restricted'. Default is 'public'.\n+# Description: Comments about the changelist. Required.\n+# Jobs: What opened jobs are to be closed by this changelist.\n+# You may delete jobs from this list. (New changelists only.)\n+# Files: What opened files from the default changelist are to be added\n+# to this changelist. You may delete files from this list.\n+# (New changelists only.)\n+\"\"\"\n+ files_list = []\n inFilesSection = False\n+ change_entry = None\n args = ['change', '-o']\n if changelist:\n args.append(str(changelist))\n-\n- for line in p4_read_pipe_lines(args):\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 settings.has_key('depot-paths'):\n- if not [p for p in settings['depot-paths']\n- if p4PathStartsWith(path, p)]:\n- continue\n- else:\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ for entry in p4CmdList(args):\n+ if not entry.has_key('code'):\n+ continue\n+ if entry['code'] == 'stat':\n+ change_entry = entry\n+ break\n+ if not change_entry:\n+ die('Failed to decode output of p4 change -o')\n+ for key, value in change_entry.iteritems():\n+ if key.startswith('File'):\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(value, p)]:\n+ continue\n else:\n- inFilesSection = False\n- else:\n- if line.startswith(\"Files:\"):\n- inFilesSection = True\n-\n- template += line\n-\n+ if not p4PathStartsWith(value, self.depotPath):\n+ continue\n+ files_list.append(value)\n+ continue\n+ # Output in the order expected by prepareLogMessage\n+ for key in ['Change', 'Client', 'User', 'Status', 'Description', 'Jobs']:\n+ if not change_entry.has_key(key):\n+ continue\n+ template += '\\n'\n+ template += key + ':'\n+ if key == 'Description':\n+ template += '\\n'\n+ for field_line in change_entry[key].splitlines():\n+ template += '\\t'+field_line+'\\n'\n+ if len(files_list) > 0:\n+ template += '\\n'\n+ template += 'Files:\\n'\n+ for path in files_list:\n+ template += '\\t'+path+'\\n'\n return template\n \n def edit_template(self, template_file):\ndiff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh\nindex 28cafe469..871544b1c 100755\n--- a/t/t9831-git-p4-triggers.sh\n+++ b/t/t9831-git-p4-triggers.sh\n@@ -66,7 +66,7 @@ test_expect_failure 'import with extra info lines from verbose p4 trigger' '\n \t)\n '\n \n-test_expect_failure 'submit description with extra info lines from verbose p4 change trigger' '\n+test_expect_success 'submit description with extra info lines from verbose p4 change trigger' '\n \ttest_when_finished cleanup_git &&\n \t(\n \t\tp4 triggers -i <<-EOF\n-- \n2.11.0\n\n"},{"id":"324344","messageId":"20170713070035.12731-3-miguel.torroja@gmail.com","threadId":"46218","inReplyTo":"20170713070035.12731-1-miguel.torroja@gmail.com","subject":"[PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-13T07:00:35Z","receivedAt":"2017-07-13T07:01:36Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"The function p4CmdList accepts a new argument: skip_info. When set to\nTrue it ignores any 'code':'info' entry (skip_info=False by default).\n\nThat allows us to fix some of the tests in t9831-git-p4-triggers.sh\nknown to be broken with verobse p4 triggers\n\nSigned-off-by: Miguel Torroja \n---\n git-p4.py | 9 ++++++---\n t/t9831-git-p4-triggers.sh | 4 ++--\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex e3a2791e0..2fa581789 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -313,7 +313,7 @@ def p4_move(src, dest):\n p4_system([\"move\", \"-k\", wildcard_encode(src), wildcard_encode(dest)])\n \n def p4_last_change():\n- results = p4CmdList([\"changes\", \"-m\", \"1\"])\n+ results = p4CmdList([\"changes\", \"-m\", \"1\"], skip_info=True)\n return int(results[0]['change'])\n \n def p4_describe(change):\n@@ -321,7 +321,7 @@ def p4_describe(change):\n the presence of field \"time\". Return a dict of the\n results.\"\"\"\n \n- ds = p4CmdList([\"describe\", \"-s\", str(change)])\n+ ds = p4CmdList([\"describe\", \"-s\", str(change)], skip_info=True)\n if len(ds) != 1:\n die(\"p4 describe -s %d did not return 1 result: %s\" % (change, str(ds)))\n \n@@ -509,7 +509,7 @@ def isModeExec(mode):\n def isModeExecChanged(src_mode, dst_mode):\n return isModeExec(src_mode) != isModeExec(dst_mode)\n \n-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):\n \n if isinstance(cmd,basestring):\n cmd = \"-G \" + cmd\n@@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):\n try:\n while True:\n entry = marshal.load(p4.stdout)\n+ if skip_info:\n+ if 'code' in entry and entry['code'] == 'info':\n+ continue\n if cb is not None:\n cb(entry)\n else:\ndiff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh\nindex 871544b1c..bbcf14c66 100755\n--- a/t/t9831-git-p4-triggers.sh\n+++ b/t/t9831-git-p4-triggers.sh\n@@ -20,7 +20,7 @@ test_expect_success 'init depot' '\n \t)\n '\n \n-test_expect_failure 'clone with extra info lines from verbose p4 trigger' '\n+test_expect_success 'clone with extra info lines from verbose p4 trigger' '\n \ttest_when_finished cleanup_git &&\n \t(\n \t\tp4 triggers -i <<-EOF\n@@ -38,7 +38,7 @@ test_expect_failure 'clone with extra info lines from verbose p4 trigger' '\n \t)\n '\n \n-test_expect_failure 'import with extra info lines from verbose p4 trigger' '\n+test_expect_success 'import with extra info lines from verbose p4 trigger' '\n \ttest_when_finished cleanup_git &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.11.0\n\n"},{"id":"324345","messageId":"CAKYtbVaxR0sdL_k=vy-aT5wEzvCTzDcM6Q-i0hO6jLMzjEUwmA@mail.gmail.com","threadId":"46218","inReplyTo":"xmqqr2xl1suy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Miguel Torroja","fromEmail":"miguel.torroja@gmail.com","sentAt":"2017-07-13T07:12:32Z","receivedAt":"2017-07-13T07:12:40Z","isPatch":true,"sender":{"key":"miguel.torroja@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5366212?v=4"},"body":"Thanks,\n\nI've just sent in reply to your previous e-mail three different patches.\n\n* The first patch is just to show some broken tests,\n* Second patch is to fix the original issue I had (the one that\ninitiated this thread)\n* Third patch is the one that filters out \"info\" messages in p4CmdList\n(this time default is reversed and set to False, what is the original\nbehaviour). The two test cases that are cured with this change have to\nset explicitely skip_info=True.\n\n\nOn Wed, Jul 12, 2017 at 7:13 PM, Junio C Hamano wrote:\n> Miguel Torroja writes:\n>\n>> The motivation for setting skip_info default to True is because any\n>> extra message output by a p4 trigger to stdout, seems to be reported\n>> as {'code':'info'} when the p4 command output is marshalled.\n>>\n>> I though it was the less intrusive way to filter out the verbose\n>> server trigger scripts, as some commands are waiting for a specific\n>> order and size of the list returned e.g:\n>>\n>> def p4_last_change():\n>> results = p4CmdList([\"changes\", \"-m\", \"1\"])\n>> return int(results[0]['change'])\n>> .\n>> def p4_describe(change):\n>> ds = p4CmdList([\"describe\", \"-s\", str(change)])\n>> if len(ds) != 1:\n>> die(\"p4 describe -s %d did not return 1 result: %s\" % (change, str(ds)))\n>>\n>> Previous examples would be broken if we allow extra \"info\" marshalled\n>> messages to be exposed.\n>>\n>> In the case of the command that was broken with the new default\n>> behaviour , when calling modfyChangelistUser, it is waiting for any\n>> message with 'data' that is not an error to consider command was\n>> succesful\n>\n> I somehow feel that this logic is totally backwards. The current\n> callers of p4CmdList() before your patch did not special case an\n> entry that was marked as 'info' in its 'code' field. Your new\n> caller, which switched from using p4_read_pipe_lines() to p4CmdList()\n> is one caller that you *know* wants to special case such an entry\n> and wanted to skip.\n>\n> Your original patch that was queued to 'pu' for a while and then\n> ejected from it after Travis saw an issue *assumed* that all other\n> callers to p4CmdList() also want to special case such an entry, and\n> that is why it made skip_info parameter default to True.\n>\n> The difference between knowing and assuming is the cause of the bug\n> your original patch introduced into modifyChangelistUser().\n>\n> The way I read Luke's suggestion was that you can avoid making the\n> same mistake by not changing the behaviour for existing callers you\n> didn't look at.\n>\n> Instead of assuming everybody else do not want an entry with 'code'\n> set to 'info', assume all the callers before your patch is doing\n> fine, and when you *know* some of them are better off ignoring such\n> an entry, explicitly tell them to do so, by:\n>\n> * The first patch adds skip_info parameter that defaults to False\n> to p4CmdList() and do the special casing when it is set to True.\n>\n> * The second patch updates p4ChangesForPaths() to use the updated\n> p4CmdList() and pass skip_info=True. It is OK to squash this\n> step into the first patch.\n>\n> * The third and later patches, if you need them, each examines an\n> existing caller of p4CmdList(), and add a new test to demonstrate\n> the existing breakage that comes from not ignoring an entry whose\n> 'code' is 'info'. That test would serve as a good documentation\n> to explain why it is better for the caller to pass skip_info=True,\n> so the same patch would also update the code to do so.\n>\n> While I was thinking the above through, here are a few cosmetic\n> things that I noticed. There is another comma that is not followed\n> by a space in existing code that might want to be corrected in a\n> clean-up patch but that is totally outside of the scope of this\n> series.\n>\n> Thanks.\n>\n> git-p4.py | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 1facf32db7..0d75753bce 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1501,7 +1501,7 @@ class P4Submit(Command, P4UserMap):\n> c['User'] = newUser\n> input = marshal.dumps(c)\n>\n> - result = p4CmdList(\"change -f -i\", stdin=input,skip_info=False)\n> + result = p4CmdList(\"change -f -i\", stdin=input, skip_info=False)\n> for r in result:\n> if r.has_key('code'):\n> if r['code'] == 'error':\n> @@ -1575,7 +1575,7 @@ class P4Submit(Command, P4UserMap):\n> files_list.append(value)\n> continue\n> # Output in the order expected by prepareLogMessage\n> - for key in ['Change','Client','User','Status','Description','Jobs']:\n> + for key in ['Change', 'Client', 'User', 'Status', 'Description', 'Jobs']:\n> if not change_entry.has_key(key):\n> continue\n> template += '\\n'\n"},{"id":"324421","messageId":"xmqqa848w2w6.fsf@gitster.mtv.corp.google.com","threadId":"46218","inReplyTo":"CAKYtbVaxR0sdL_k=vy-aT5wEzvCTzDcM6Q-i0hO6jLMzjEUwmA@mail.gmail.com","subject":"Re: [PATCH] git-p4: parse marshal output \"p4 -G\" in p4 changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T19:30:33Z","receivedAt":"2017-07-13T19:30:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miguel Torroja writes:\n\n> I've just sent in reply to your previous e-mail three different patches.\n>\n> * The first patch is just to show some broken tests,\n> * Second patch is to fix the original issue I had (the one that\n> initiated this thread)\n> * Third patch is the one that filters out \"info\" messages in p4CmdList\n> (this time default is reversed and set to False, what is the original\n> behaviour). The two test cases that are cured with this change have to\n> set explicitely skip_info=True.\n\nThe approach looks reasonable. By having tests that expect failure\nupfront, the series clearly shows how the code changes in later\nsteps make things better.\n\nThanks. Will replace.\n"}]}