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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux