git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Luke Diamand <luke@diamand.org>
Date
Jul 12, 2017, 08:25 UTC
Message-ID
<CAE5ih78mrTz1sfJbRSuPTNojxWyH_1JFDY2pe7GMAZdPhzcvpA@mail.gmail.com>
In-Reply-To
<20170711225316.10608-1-miguel.torroja@gmail.com>
On 11 July 2017 at 23:53, Miguel Torroja <miguel.torroja@gmail.com> wrote:
Show 66 quoted lines
> 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

Previous: Miguel TorrojaNext: Miguel Torroja
Message 24 of 31 in “git-p4: changelist template with p4 -G change -o”
  1. git-p4: changelist template with p4 -G change -oMiguel Torroja, Jun 20, 2017
  2. Junio C HamanoJun 22, 2017
  3. Luke DiamandJun 24, 2017
  4. miguel torrojaJun 24, 2017
  5. Lars SchneiderJun 24, 2017
  6. miguel torrojaJun 27, 2017
  7. git-p4: parse marshal output "p4 -G" in p4 changesMiguel Torroja, Jun 27, 2017
  8. Junio C HamanoJun 28, 2017
  9. Luke DiamandJun 28, 2017
  10. miguel torrojaJun 28, 2017
  11. miguel torrojaJun 29, 2017
  12. Luke DiamandJun 30, 2017
  13. miguel torrojaJun 30, 2017
  14. git-p4: parse marshal output "p4 -G" in p4 changesMiguel Torroja, Jun 29, 2017
  15. Lars SchneiderJun 30, 2017
  16. Miguel TorrojaJun 30, 2017
  17. Lars SchneiderJun 30, 2017
  18. Miguel TorrojaJun 30, 2017
  19. Miguel TorrojaJul 3, 2017
  20. git-p4: parse marshal output "p4 -G" in p4 changesMiguel Torroja, Jul 3, 2017
  21. Luke DiamandJul 11, 2017
  22. Miguel TorrojaJul 11, 2017
  23. git-p4: parse marshal output "p4 -G" in p4 changesMiguel Torroja, Jul 11, 2017
  24. Luke DiamandJul 12, 2017
  25. Miguel TorrojaJul 12, 2017
  26. Junio C HamanoJul 12, 2017
  27. 1/3 git-p4: git-p4 tests with p4 triggersMiguel Torroja, Jul 13, 2017
  28. 2/3 git-p4: parse marshal output "p4 -G" in p4 changesMiguel Torroja, Jul 13, 2017
  29. 3/3 git-p4: filter for {'code':'info'} in p4CmdListMiguel Torroja, Jul 13, 2017
  30. Miguel TorrojaJul 13, 2017
  31. Junio C HamanoJul 13, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.