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

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 [off-list ref] wrote:

On 11 July 2017 at 23:53, Miguel Torroja [off-list ref] wrote:

quoted

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 <redacted>
---
 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