Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

From: Miguel Torroja <miguel.torroja@gmail.com>
Date: Jul 12, 2017, 10:16 UTC
Message-ID: <CAKYtbVb8T=edPG5539=uwDjHnCerLO2Oejy8bWK+giSS8nNGig@mail.gmail.com>
In-Reply-To: <CAE5ih78mrTz1sfJbRSuPTNojxWyH_1JFDY2pe7GMAZdPhzcvpA@mail.gmail.com>

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@diamand.org> wrote:

Show 77 quoted lines

> On 11 July 2017 at 23:53, Miguel Torroja <miguel.torroja@gmail.com> 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@gmail.com>
>> ---
>>  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