The motivation for setting skip_info default to True is because any
extra message output by a p4 trigger to stdout, seems to be reported
as {'code':'info'} when the p4 command output is marshalled.
I though it was the less intrusive way to filter out the verbose
server trigger scripts, as some commands are waiting for a specific
order and size of the list returned e.g:
def p4_last_change():
results = p4CmdList(["changes", "-m", "1"])
return int(results[0]['change'])
.
def p4_describe(change):
ds = p4CmdList(["describe", "-s", str(change)])
if len(ds) != 1:
die("p4 describe -s %d did not return 1 result: %s" % (change, str(ds)))
Previous examples would be broken if we allow extra "info" marshalled
messages to be exposed.
In the case of the command that was broken with the new default
behaviour , when calling modfyChangelistUser, it is waiting for any
message with 'data' that is not an error to consider command was
succesful
Thanks,
On Wed, Jul 12, 2017 at 10:25 AM, Luke Diamand <luke@xxxxxxxxxxx> wrote:
> On 11 July 2017 at 23:53, 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'}
>>
>> the function p4CmdList accepts a new argument: skip_info. When set to
>> True it ignores any 'code':'info' entry (skip_info=True by default).
>>
>> A new test has been created in 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>
>> ---
>> git-p4.py | 92 ++++++++++++++++++++++++++++++++----------------
>> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++
>> 2 files changed, 92 insertions(+), 30 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91..1facf32db 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -509,7 +509,7 @@ def isModeExec(mode):
>> def isModeExecChanged(src_mode, dst_mode):
>> return isModeExec(src_mode) != isModeExec(dst_mode)
>>
>> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
>> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True):
>>
>> if isinstance(cmd,basestring):
>> cmd = "-G " + cmd
>> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
>> try:
>> while True:
>> entry = marshal.load(p4.stdout)
>> + if skip_info:
>> + if 'code' in entry and entry['code'] == 'info':
>> + continue
>> if cb is not None:
>> cb(entry)
>> else:
>> @@ -879,8 +882,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
>> @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap):
>> c['User'] = newUser
>> input = marshal.dumps(c)
>>
>> - result = p4CmdList("change -f -i", stdin=input)
>> + result = p4CmdList("change -f -i", stdin=input,skip_info=False)
>
> Is there any reason this change sets skip_info to False in this one
> place, rather than defaulting to False (the original behavior) and
> setting it to True where it's needed?
>
> I worry that there might be other unexpected side effects in places
> not covered by the tests.
>
> Thanks
> Luke
