On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
<larsxschneider@xxxxxxxxx> wrote:
>
>> On 30 Jun 2017, at 00:46, miguel torroja <miguel.torroja@xxxxxxxxx> wrote:
>>
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 triggers in the server side generate some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}
>>
>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>
>> Signed-off-by: Miguel Torroja <miguel.torroja@xxxxxxxxx>
>> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
>> ---
>> git-p4.py | 85 ++++++++++++++++++++++++++++++++----------------
>> t/t9807-git-p4-submit.sh | 27 +++++++++++++++
>> 2 files changed, 84 insertions(+), 28 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91..61dc045f3 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
>> cmd += ["%s...@%s" % (p, revisionRange)]
>>
>> # Insert changes in chronological order
>> - for line in reversed(p4_read_pipe_lines(cmd)):
>> - changes.add(int(line.split(" ")[1]))
>> + for entry in reversed(p4CmdList(cmd)):
>> + if entry.has_key('p4ExitCode'):
>> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode']))
>> + if not entry.has_key('change'):
>> + continue
>> + changes.add(int(entry['change']))
>>
>> if not block_size:
>> break
>> @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):
>>
>> [upstream, settings] = findUpstreamBranchPoint()
>>
>> - template = ""
>> + template = """\
>> +# A Perforce Change Specification.
>> +#
>> +# Change: The change number. 'new' on a new changelist.
>> +# Date: The date this specification was last modified.
>> +# Client: The client on which the changelist was created. Read-only.
>> +# User: The user who created the changelist.
>> +# Status: Either 'pending' or 'submitted'. Read-only.
>> +# Type: Either 'public' or 'restricted'. Default is 'public'.
>> +# Description: Comments about the changelist. Required.
>> +# Jobs: What opened jobs are to be closed by this changelist.
>> +# You may delete jobs from this list. (New changelists only.)
>> +# Files: What opened files from the default changelist are to be added
>> +# to this changelist. You may delete files from this list.
>> +# (New changelists only.)
>> +"""
>> + files_list = []
>> inFilesSection = False
>> + change_entry = None
>> args = ['change', '-o']
>> if changelist:
>> args.append(str(changelist))
>> -
>> - for line in p4_read_pipe_lines(args):
>> - if line.endswith("\r\n"):
>> - line = line[:-2] + "\n"
>> - if inFilesSection:
>> - if line.startswith("\t"):
>> - # path starts and ends with a tab
>> - path = line[1:]
>> - lastTab = path.rfind("\t")
>> - if lastTab != -1:
>> - path = path[:lastTab]
>> - if settings.has_key('depot-paths'):
>> - if not [p for p in settings['depot-paths']
>> - if p4PathStartsWith(path, p)]:
>> - continue
>> - else:
>> - if not p4PathStartsWith(path, self.depotPath):
>> - continue
>> + for entry in p4CmdList(args):
>> + if not entry.has_key('code'):
>> + continue
>> + if entry['code'] == 'stat':
>> + change_entry = entry
>> + break
>> + if not change_entry:
>> + die('Failed to decode output of p4 change -o')
>> + for key, value in change_entry.iteritems():
>> + if key.startswith('File'):
>> + if settings.has_key('depot-paths'):
>> + if not [p for p in settings['depot-paths']
>> + if p4PathStartsWith(value, p)]:
>> + continue
>> else:
>> - inFilesSection = False
>> - else:
>> - if line.startswith("Files:"):
>> - inFilesSection = True
>> -
>> - template += line
>> -
>> + if not p4PathStartsWith(value, self.depotPath):
>> + continue
>> + files_list.append(value)
>> + continue
>> + # Output in the order expected by prepareLogMessage
>> + for key in ['Change','Client','User','Status','Description','Jobs']:
>> + if not change_entry.has_key(key):
>> + continue
>> + template += '\n'
>> + template += key + ':'
>> + if key == 'Description':
>> + template += '\n'
>> + for field_line in change_entry[key].splitlines():
>> + template += '\t'+field_line+'\n'
>> + if len(files_list) > 0:
>> + template += '\n'
>> + template += 'Files:\n'
>> + for path in files_list:
>> + template += '\t'+path+'\n'
>> return template
>>
>> def edit_template(self, template_file):
>> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
>> index 3457d5db6..4962b6862 100755
>> --- a/t/t9807-git-p4-submit.sh
>> +++ b/t/t9807-git-p4-submit.sh
>> @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' '
>> )
>> '
>>
>
> I have never worked with p4 triggers and that might be
> the reason why I don't understand your test case.
> Maybe you can help me?
>
>> +test_expect_success 'description with extra lines from verbose p4 trigger' '
>> + test_when_finished cleanup_git &&
>> + git p4 clone --dest="$git" //depot &&
>> + (
>> + p4 triggers -i <<-EOF
>> + Triggers: p4triggertest-command command pre-user-change "echo verbose trigger"
>> + EOF
>> + ) &&
>
> You clone the test repo and install a trigger.
>
>> + (
>> + cd "$git" &&
>> + git config git-p4.skipSubmitEdit true &&
>> + echo file20 >file20 &&
>> + git add file20 &&
>> + git commit -m file20 &&
>> + git p4 submit
>> + ) &&
>
> You make a new commit. This should run the "echo verbose trigger", right?
Yes, that's correct. In this case the trigger is run with p4 change
and p4 changes
>
>> + (
>> + p4 triggers -i <<-EOF
>> + Triggers:
>> + EOF
>> + ) &&
>
> You delete the trigger.
>
>> + (
>> + cd "$cli" &&
>> + test_path_is_file file20
>> + )
>
> You check that the file20 is available in P4.
>
>
> What would happen if I run this test case without your patch?
> Wouldn't it pass just fine?
If you run it without the patch for git-p4.py, the test doesn't pass
> Wouldn't we need to check that no warning/error is in the
> real change description?
>
that can also be added, something like this: 'p4 change -o | grep
"verbose trigger"' after setting the trigger?
> Thanks,
> Lars
