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

> On 30 Jun 2017, at 00:46, 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'}
> 
> A new test has been created to 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> git-p4.py                | 85 ++++++++++++++++++++++++++++++++----------------
> t/t9807-git-p4-submit.sh | 27 +++++++++++++++
> 2 files changed, 84 insertions(+), 28 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index 8d151da91..61dc045f3 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -879,8 +879,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
> @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):
> 
>         [upstream, settings] = findUpstreamBranchPoint()
> 
> -        template = ""
> +        template = """\
> +# A Perforce Change Specification.
> +#
> +#  Change:      The change number. 'new' on a new changelist.
> +#  Date:        The date this specification was last modified.
> +#  Client:      The client on which the changelist was created.  Read-only.
> +#  User:        The user who created the changelist.
> +#  Status:      Either 'pending' or 'submitted'. Read-only.
> +#  Type:        Either 'public' or 'restricted'. Default is 'public'.
> +#  Description: Comments about the changelist.  Required.
> +#  Jobs:        What opened jobs are to be closed by this changelist.
> +#               You may delete jobs from this list.  (New changelists only.)
> +#  Files:       What opened files from the default changelist are to be added
> +#               to this changelist.  You may delete files from this list.
> +#               (New changelists only.)
> +"""
> +        files_list = []
>         inFilesSection = False
> +        change_entry = None
>         args = ['change', '-o']
>         if changelist:
>             args.append(str(changelist))
> -
> -        for line in p4_read_pipe_lines(args):
> -            if line.endswith("\r\n"):
> -                line = line[:-2] + "\n"
> -            if inFilesSection:
> -                if line.startswith("\t"):
> -                    # path starts and ends with a tab
> -                    path = line[1:]
> -                    lastTab = path.rfind("\t")
> -                    if lastTab != -1:
> -                        path = path[:lastTab]
> -                        if settings.has_key('depot-paths'):
> -                            if not [p for p in settings['depot-paths']
> -                                    if p4PathStartsWith(path, p)]:
> -                                continue
> -                        else:
> -                            if not p4PathStartsWith(path, self.depotPath):
> -                                continue
> +        for entry in p4CmdList(args):
> +            if not entry.has_key('code'):
> +                continue
> +            if entry['code'] == 'stat':
> +                change_entry = entry
> +                break
> +        if not change_entry:
> +            die('Failed to decode output of p4 change -o')
> +        for key, value in change_entry.iteritems():
> +            if key.startswith('File'):
> +                if settings.has_key('depot-paths'):
> +                    if not [p for p in settings['depot-paths']
> +                            if p4PathStartsWith(value, p)]:
> +                        continue
>                 else:
> -                    inFilesSection = False
> -            else:
> -                if line.startswith("Files:"):
> -                    inFilesSection = True
> -
> -            template += line
> -
> +                    if not p4PathStartsWith(value, self.depotPath):
> +                        continue
> +                files_list.append(value)
> +                continue
> +        # Output in the order expected by prepareLogMessage
> +        for key in ['Change','Client','User','Status','Description','Jobs']:
> +            if not change_entry.has_key(key):
> +                continue
> +            template += '\n'
> +            template += key + ':'
> +            if key == 'Description':
> +                template += '\n'
> +            for field_line in change_entry[key].splitlines():
> +                template += '\t'+field_line+'\n'
> +        if len(files_list) > 0:
> +            template += '\n'
> +            template += 'Files:\n'
> +        for path in files_list:
> +            template += '\t'+path+'\n'
>         return template
> 
>     def edit_template(self, template_file):
> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
> index 3457d5db6..4962b6862 100755
> --- a/t/t9807-git-p4-submit.sh
> +++ b/t/t9807-git-p4-submit.sh
> @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' '
> 	)
> '
> 

> +test_expect_success 'description with extra lines from verbose p4 trigger' '
> +	test_when_finished cleanup_git &&
> +	git p4 clone --dest="$git" //depot &&
> +	(
> +		p4 triggers -i <<-EOF
> +		Triggers: p4triggertest-command command pre-user-change "echo verbose trigger"
> +		EOF
> +	) &&

> +	(
> +		cd "$git" &&
> +		git config git-p4.skipSubmitEdit true &&
> +		echo file20 >file20 &&
> +		git add file20 &&
> +		git commit -m file20 &&
> +		git p4 submit
> +	) &&

> +	(
> +		p4 triggers -i <<-EOF
> +		Triggers:
> +		EOF
> +	) &&