[PATCH] git-p4: changelist template with p4 -G change -o

[PATCH] git-p4: changelist template with p4 -G change -o

[Miguel Torroja]

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 plugin/hooks in the server side generates 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'}

Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
---
 git-p4.py | 77 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da..a300474 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1526,37 +1526,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):
-- 
2.1.4

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[Junio C Hamano]

Miguel Torroja <miguel.torroja@gmail.com> writes:

> 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 plugin/hooks in the server side generates 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'}
>
> Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
> ---

Asking for help reviewing to those who have worked on git-p4 in the
past...

Thanks.

>  git-p4.py | 77 ++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 8d151da..a300474 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1526,37 +1526,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):

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[Luke Diamand]

On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote:
> Miguel Torroja <miguel.torroja@gmail.com> writes:
>
>> 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 plugin/hooks in the server side generates 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'}

I think this seems like a reasonable thing to do if "p4 change -o" is
jumbling up output.

One thing I notice trying it out by hand is that we seem to have lost
the annotation of the Perforce per-file modification type (is there a
proper name for this?).

For example, if I add a file called "baz", then the original version
creates a template which looks like this:

   //depot/baz    # add

But the new one creates a template which looks like:

   //depot/baz

Luke

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[miguel torroja]

You are right about the "# add"comment. I couldn't find any extra info
in the marshaled output that I can use to add the change action
comment after the path. That's one downside of that change.

On Sat, Jun 24, 2017 at 1:49 PM, Luke Diamand <luke@diamand.org> wrote:
> On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote:
>> Miguel Torroja <miguel.torroja@gmail.com> writes:
>>
>>> 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 plugin/hooks in the server side generates 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'}
>
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
>
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
>
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
>
>    //depot/baz    # add
>
> But the new one creates a template which looks like:
>
>    //depot/baz
>
> Luke

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[Lars Schneider]

> On 24 Jun 2017, at 13:49, Luke Diamand <luke@diamand.org> wrote:
> 
> On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote:
>> Miguel Torroja <miguel.torroja@gmail.com> writes:
>> 
>>> 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 plugin/hooks in the server side generates 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'}
> 
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
> 
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
> 
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
> 
>   //depot/baz    # add
> 
> But the new one creates a template which looks like:
> 
>   //depot/baz

@Miguel: You wrote that p4 plugins/hooks generate these warnings.
I wonder if you see a way to replicate that in a test case. Either
in t9800 or a new t98XX test case file?

- Lars

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[miguel torroja]

Hi Lars/Luke,

I tried a first test extending t9807-git-p4-submit.sh. I set this p4
trigger: 'p4test command pre-user-change "echo verbose trigger" '. I'm
able to reproduce the issue I wanted to fix. However I found yet
another issue, in this case when reading the result from
p4_read_pipe_lines (in function p4ChangesForPaths), the
pre-user-change is triggered with any "p4 change" and "p4 changes"
command (the p4 server we have in production, only shows "extra"
messages with p4 change).
   I'll collapse in one single commit the fix for p4 change/p4 changes
and the new test.

Thanks,

Miguel

On Sat, Jun 24, 2017 at 10:37 PM, miguel torroja
<miguel.torroja@gmail.com> wrote:
> Hi Lars,
>
> I think it's doable to set a custom p4 trigger, created by the test case,
> that outputs "extra info" when requesting a changelist description.
> I'll do a specific test and post it to this thread.
>
>
> Thanks,
>
>
> El 24 jun. 2017 7:36 p. m., "Lars Schneider" <larsxschneider@gmail.com>
> escribió:
>
>
>> On 24 Jun 2017, at 13:49, Luke Diamand <luke@diamand.org> wrote:
>>
>> On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote:
>>> Miguel Torroja <miguel.torroja@gmail.com> writes:
>>>
>>>> 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 plugin/hooks in the server side generates 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'}
>>
>> I think this seems like a reasonable thing to do if "p4 change -o" is
>> jumbling up output.
>>
>> One thing I notice trying it out by hand is that we seem to have lost
>> the annotation of the Perforce per-file modification type (is there a
>> proper name for this?).
>>
>> For example, if I add a file called "baz", then the original version
>> creates a template which looks like this:
>>
>>   //depot/baz    # add
>>
>> But the new one creates a template which looks like:
>>
>>   //depot/baz
>
> @Miguel: You wrote that p4 plugins/hooks generate these warnings.
> I wonder if you see a way to replicate that in a test case. Either
> in t9800 or a new t98XX test case file?
>
> - Lars
>
>

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

[Miguel Torroja]

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>
---
 git-p4.py                | 83 ++++++++++++++++++++++++++++++++----------------
 t/t9807-git-p4-submit.sh | 28 ++++++++++++++++
 2 files changed, 83 insertions(+), 28 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..239a8f144 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -879,8 +879,10 @@ 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 not entry.has_key('change'):
+                continue
+            changes.add(int(entry['change']))
 
         if not block_size:
             break
@@ -1526,37 +1528,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..05d7fc3f7 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -409,6 +409,34 @@ 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
+	) &&
+	(
+		cd "$cli" &&
+		test_path_is_file file20
+	)
+'
+
 test_expect_success 'submit --prepare-p4-only' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.11.0

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

[Junio C Hamano]

Miguel Torroja <miguel.torroja@gmail.com> writes:

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

It appears that https://travis-ci.org/git/git/builds/247724639
does not like this change.  For example:

    https://travis-ci.org/git/git/jobs/247724642#L1848

indicates that not just 9807 (new tests added by this patch) but
also 9800 starts to fail.

I'd wait for git-p4 experts to comment and help guiding this change
forward.

Thanks.

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

[Luke Diamand]

On 28 June 2017 at 05:08, Junio C Hamano <gitster@pobox.com> wrote:
> Miguel Torroja <miguel.torroja@gmail.com> writes:
>
>> 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>
>> ---
>
> It appears that https://travis-ci.org/git/git/builds/247724639
> does not like this change.  For example:
>
>     https://travis-ci.org/git/git/jobs/247724642#L1848
>
> indicates that not just 9807 (new tests added by this patch) but
> also 9800 starts to fail.
>
> I'd wait for git-p4 experts to comment and help guiding this change
> forward.

I only see a (very weird) failure in t9800. I wonder if there are some
P4 version differences.

Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).
Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)

There's also a whitespace error according to "git diff --check".
:
Sadly I don't think there's any way to do this and yet keep the "#
edit" comments. It looks like "p4 change -o" outputs lines with "'#
edit" on the end, but the (supposedly semantically equivalent) "p4 -G
change -o" command does not. I think that's a P4 bug.

So we have a choice of fixing a garbled message in the face of scripts
in the backend, or keeping the comments, or writing some extra Python
to infer them. I vote for fixing the garbled message.

Luke

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

[miguel torroja]

Thanks Luke,

regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
should display error), for me it's very weird too as it doesn't seem
to be related to this particular change, as the patch changes are not
exercised with that test.

The test 21 in t9807 was precisely the new test added to test the
change (it was passing with local setup), the test log is truncated
before the output of test 21 in t9807 but I'm afraid I'm not very
familiar with Travis, so maybe I'm missing something. Is there a way
to have the full logs or they are always truncated after some number
of lines?

I think you get an error with git diff --check because I added spaces
after a tab, but those spaces are intentional, the tabs are for the
"<<-EOF" and spaces are for the "p4 triggers" specificiation.

Thanks,


On Wed, Jun 28, 2017 at 11:54 AM, Luke Diamand <luke@diamand.org> wrote:
> On 28 June 2017 at 05:08, Junio C Hamano <gitster@pobox.com> wrote:
>> Miguel Torroja <miguel.torroja@gmail.com> writes:
>>
>>> 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>
>>> ---
>>
>> It appears that https://travis-ci.org/git/git/builds/247724639
>> does not like this change.  For example:
>>
>>     https://travis-ci.org/git/git/jobs/247724642#L1848
>>
>> indicates that not just 9807 (new tests added by this patch) but
>> also 9800 starts to fail.
>>
>> I'd wait for git-p4 experts to comment and help guiding this change
>> forward.
>
> I only see a (very weird) failure in t9800. I wonder if there are some
> P4 version differences.
>
> Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).
> Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)
>
> There's also a whitespace error according to "git diff --check".
> :
> Sadly I don't think there's any way to do this and yet keep the "#
> edit" comments. It looks like "p4 change -o" outputs lines with "'#
> edit" on the end, but the (supposedly semantically equivalent) "p4 -G
> change -o" command does not. I think that's a P4 bug.
>
> So we have a choice of fixing a garbled message in the face of scripts
> in the backend, or keeping the comments, or writing some extra Python
> to infer them. I vote for fixing the garbled message.
>
> Luke

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

[miguel torroja]

On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <luke@diamand.org> wrote:
> On 28 June 2017 at 14:14, miguel torroja <miguel.torroja@gmail.com> wrote:
>> Thanks Luke,
>>
>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
>> should display error), for me it's very weird too as it doesn't seem
>> to be related to this particular change, as the patch changes are not
>> exercised with that test.
>
> I had a look at this. The problem is that the old code uses
> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.
>
> But the new code calls p4CmdList() which has does error handling by
> setting "p4ExitCode" to a non-zero value in the returned dictionary.
>
> I think if you just check for that case, the test will then pass

Thank you for debugging this,  I did as you suggested and it passed that test!

>>
>> The test 21 in t9807 was precisely the new test added to test the
>> change (it was passing with local setup), the test log is truncated
>> before the output of test 21 in t9807 but I'm afraid I'm not very
>> familiar with Travis, so maybe I'm missing something. Is there a way
>> to have the full logs or they are always truncated after some number
>> of lines?
>
> For me, t9807 is working fine.
>
>>
>> I think you get an error with git diff --check because I added spaces
>> after a tab, but those spaces are intentional, the tabs are for the
>> "<<-EOF" and spaces are for the "p4 triggers" specificiation.
>
> OK.
>

In the end, ,the reason t9807 was not passing was precisely the tabs
and spaces of the patch. the original patch had:
<tab><tab><spaces>....., as I explained, the tabs were supposed to be
ignored by "<<-EOF" and the spaces were supposed to be sent to stdin
of p4 triggers, but when the patch was applied to upstream the
<spaces> were substituted by tabs what led to a malformed  "p4
trigger" description. I just collapsed the description in one single
line and now it's passing
>
> Luke


I'm sending a new patch with the two changes I just mentioned.

Thanks,

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

[Luke Diamand]

On 29 June 2017 at 23:41, miguel torroja <miguel.torroja@gmail.com> wrote:
> On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <luke@diamand.org> wrote:
>> On 28 June 2017 at 14:14, miguel torroja <miguel.torroja@gmail.com> wrote:
>>> Thanks Luke,
>>>
>>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
>>> should display error), for me it's very weird too as it doesn't seem
>>> to be related to this particular change, as the patch changes are not
>>> exercised with that test.
>>
>> I had a look at this. The problem is that the old code uses
>> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.
>>
>> But the new code calls p4CmdList() which has does error handling by
>> setting "p4ExitCode" to a non-zero value in the returned dictionary.
>>
>> I think if you just check for that case, the test will then pass
>
> Thank you for debugging this,  I did as you suggested and it passed that test!
>
>>>
>>> The test 21 in t9807 was precisely the new test added to test the
>>> change (it was passing with local setup), the test log is truncated
>>> before the output of test 21 in t9807 but I'm afraid I'm not very
>>> familiar with Travis, so maybe I'm missing something. Is there a way
>>> to have the full logs or they are always truncated after some number
>>> of lines?
>>
>> For me, t9807 is working fine.
>>
>>>
>>> I think you get an error with git diff --check because I added spaces
>>> after a tab, but those spaces are intentional, the tabs are for the
>>> "<<-EOF" and spaces are for the "p4 triggers" specificiation.
>>
>> OK.
>>
>
> In the end, ,the reason t9807 was not passing was precisely the tabs
> and spaces of the patch. the original patch had:
> <tab><tab><spaces>....., as I explained, the tabs were supposed to be
> ignored by "<<-EOF" and the spaces were supposed to be sent to stdin
> of p4 triggers, but when the patch was applied to upstream the
> <spaces> were substituted by tabs what led to a malformed  "p4
> trigger" description. I just collapsed the description in one single
> line and now it's passing
>>
>> Luke
>
>
> I'm sending a new patch with the two changes I just mentioned.

Looks good to me, Ack. Can we squash the two changes together?

Luke

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

[miguel torroja]

The Latest patch I sent was already the squashed version with the fix
to pass the tests.

Thanks,

On Fri, Jun 30, 2017 at 9:56 AM, Luke Diamand <luke@diamand.org> wrote:
> On 29 June 2017 at 23:41, miguel torroja <miguel.torroja@gmail.com> wrote:
>> On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <luke@diamand.org> wrote:
>>> On 28 June 2017 at 14:14, miguel torroja <miguel.torroja@gmail.com> wrote:
>>>> Thanks Luke,
>>>>
>>>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
>>>> should display error), for me it's very weird too as it doesn't seem
>>>> to be related to this particular change, as the patch changes are not
>>>> exercised with that test.
>>>
>>> I had a look at this. The problem is that the old code uses
>>> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.
>>>
>>> But the new code calls p4CmdList() which has does error handling by
>>> setting "p4ExitCode" to a non-zero value in the returned dictionary.
>>>
>>> I think if you just check for that case, the test will then pass
>>
>> Thank you for debugging this,  I did as you suggested and it passed that test!
>>
>>>>
>>>> The test 21 in t9807 was precisely the new test added to test the
>>>> change (it was passing with local setup), the test log is truncated
>>>> before the output of test 21 in t9807 but I'm afraid I'm not very
>>>> familiar with Travis, so maybe I'm missing something. Is there a way
>>>> to have the full logs or they are always truncated after some number
>>>> of lines?
>>>
>>> For me, t9807 is working fine.
>>>
>>>>
>>>> I think you get an error with git diff --check because I added spaces
>>>> after a tab, but those spaces are intentional, the tabs are for the
>>>> "<<-EOF" and spaces are for the "p4 triggers" specificiation.
>>>
>>> OK.
>>>
>>
>> In the end, ,the reason t9807 was not passing was precisely the tabs
>> and spaces of the patch. the original patch had:
>> <tab><tab><spaces>....., as I explained, the tabs were supposed to be
>> ignored by "<<-EOF" and the spaces were supposed to be sent to stdin
>> of p4 triggers, but when the patch was applied to upstream the
>> <spaces> were substituted by tabs what led to a malformed  "p4
>> trigger" description. I just collapsed the description in one single
>> line and now it's passing
>>>
>>> Luke
>>
>>
>> I'm sending a new patch with the two changes I just mentioned.
>
> Looks good to me, Ack. Can we squash the two changes together?
>
> Luke

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

[Miguel Torroja]

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
+	) &&
+	(
+		cd "$cli" &&
+		test_path_is_file file20
+	)
+'
+
 test_expect_success 'submit --prepare-p4-only' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.11.0

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

[Lars Schneider]

> 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' '
> 	)
> '
> 

I have never worked with p4 triggers and that might be
the reason why I don't understand your test case.
Maybe you can help me?

> +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
> +	) &&

You clone the test repo and install a trigger.

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

You make a new commit. This should run the "echo verbose trigger", right?

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

You delete the trigger.

> +	(
> +		cd "$cli" &&
> +		test_path_is_file file20
> +	)

You check that the file20 is available in P4.


What would happen if I run this test case without your patch?
Wouldn't it pass just fine?
Wouldn't we need to check that no warning/error is in the
real change description?

Thanks,
Lars

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

[Miguel Torroja]

On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> 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' '
>>       )
>> '
>>
>
> I have never worked with p4 triggers and that might be
> the reason why I don't understand your test case.
> Maybe you can help me?
>
>> +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
>> +     ) &&
>
> You clone the test repo and install a trigger.
>
>> +     (
>> +             cd "$git" &&
>> +             git config git-p4.skipSubmitEdit true &&
>> +             echo file20 >file20 &&
>> +             git add file20 &&
>> +             git commit -m file20 &&
>> +             git p4 submit
>> +     ) &&
>
> You make a new commit. This should run the "echo verbose trigger", right?

Yes, that's correct. In this case the trigger is run with p4 change
and p4 changes

>
>> +     (
>> +             p4 triggers -i <<-EOF
>> +             Triggers:
>> +             EOF
>> +     ) &&
>
> You delete the trigger.
>
>> +     (
>> +             cd "$cli" &&
>> +             test_path_is_file file20
>> +     )
>
> You check that the file20 is available in P4.
>
>
> What would happen if I run this test case without your patch?
> Wouldn't it pass just fine?

If you run it without the patch for git-p4.py, the test doesn't pass

> Wouldn't we need to check that no warning/error is in the
> real change description?
>

that can also be added, something like this: 'p4 change -o | grep
"verbose trigger"' after setting the trigger?

> Thanks,
> Lars

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

[Lars Schneider]

> On 30 Jun 2017, at 11:41, Miguel Torroja <miguel.torroja@gmail.com> wrote:
> 
> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> 
>>> 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>
>>> ---
>>> ...
>> 
>> I have never worked with p4 triggers and that might be
>> the reason why I don't understand your test case.
>> Maybe you can help me?
>> 
>>> +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
>>> +     ) &&
>> 
>> You clone the test repo and install a trigger.
>> 
>>> +     (
>>> +             cd "$git" &&
>>> +             git config git-p4.skipSubmitEdit true &&
>>> +             echo file20 >file20 &&
>>> +             git add file20 &&
>>> +             git commit -m file20 &&
>>> +             git p4 submit
>>> +     ) &&
>> 
>> You make a new commit. This should run the "echo verbose trigger", right?
> 
> Yes, that's correct. In this case the trigger is run with p4 change
> and p4 changes
> 
>> 
>>> +     (
>>> +             p4 triggers -i <<-EOF
>>> +             Triggers:
>>> +             EOF
>>> +     ) &&
>> 
>> You delete the trigger.
>> 
>>> +     (
>>> +             cd "$cli" &&
>>> +             test_path_is_file file20
>>> +     )
>> 
>> You check that the file20 is available in P4.
>> 
>> 
>> What would happen if I run this test case without your patch?
>> Wouldn't it pass just fine?
> 
> If you run it without the patch for git-p4.py, the test doesn't pass

You are right. I did not run "make" properly before running the test :)


>> Wouldn't we need to check that no warning/error is in the
>> real change description?
>> 
> 
> that can also be added, something like this: 'p4 change -o | grep
> "verbose trigger"' after setting the trigger?

Yeah, maybe. I hope this is no stupid question, but: If you clone the
repo with git-p4 *again* ... would you see the "verbose trigger" output
in the Git commit message?


- Lars

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

[Miguel Torroja]

On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 30 Jun 2017, at 11:41, Miguel Torroja <miguel.torroja@gmail.com> wrote:
>>
>> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
>> <larsxschneider@gmail.com> wrote:
>>>
>>>> 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>
>>>> ---
>>>> ...
>>>
>>> I have never worked with p4 triggers and that might be
>>> the reason why I don't understand your test case.
>>> Maybe you can help me?
>>>
>>>> +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
>>>> +     ) &&
>>>
>>> You clone the test repo and install a trigger.
>>>
>>>> +     (
>>>> +             cd "$git" &&
>>>> +             git config git-p4.skipSubmitEdit true &&
>>>> +             echo file20 >file20 &&
>>>> +             git add file20 &&
>>>> +             git commit -m file20 &&
>>>> +             git p4 submit
>>>> +     ) &&
>>>
>>> You make a new commit. This should run the "echo verbose trigger", right?
>>
>> Yes, that's correct. In this case the trigger is run with p4 change
>> and p4 changes
>>
>>>
>>>> +     (
>>>> +             p4 triggers -i <<-EOF
>>>> +             Triggers:
>>>> +             EOF
>>>> +     ) &&
>>>
>>> You delete the trigger.
>>>
>>>> +     (
>>>> +             cd "$cli" &&
>>>> +             test_path_is_file file20
>>>> +     )
>>>
>>> You check that the file20 is available in P4.
>>>
>>>
>>> What would happen if I run this test case without your patch?
>>> Wouldn't it pass just fine?
>>
>> If you run it without the patch for git-p4.py, the test doesn't pass
>
> You are right. I did not run "make" properly before running the test :)
>
>
>>> Wouldn't we need to check that no warning/error is in the
>>> real change description?
>>>
>>
>> that can also be added, something like this: 'p4 change -o | grep
>> "verbose trigger"' after setting the trigger?
>
> Yeah, maybe. I hope this is no stupid question, but: If you clone the
> repo with git-p4 *again* ... would you see the "verbose trigger" output
> in the Git commit message?
>

The commands that are affected are the ones that don't use the -G
option, as everything is sent to the standard output without being
able to filter out what is the real contents or just info messages.
That's not the case with the python output (-G). Having said that... I
tried what you just said (just to be sure) and the function
p4_last_change fails... as it expects the first dictionary returned by
p4CmdList is the one that contains the change:
"int(results[0]['change'])" and that's not the case as it's an info
entry (no 'change' key, that's in the next entry...)  I'll update with
new patches

I didn't notice that before because the P4 server we have in our
office only outputs extra info messages with the command "p4 change".


> - Lars

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

[Miguel Torroja]

I changed the patch a little bit, first change is to ignore by default
any {'code':'info'} in p4CmdList (they are not exposed to the caller)
as those are the verbose messages from triggers (p4 Debug does show
them). the second change is to check the p4 trigger is really set in
the test (Lars suggestion),

On Fri, Jun 30, 2017 at 6:02 PM, Miguel Torroja
<miguel.torroja@gmail.com> wrote:
> On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>>
>>> On 30 Jun 2017, at 11:41, Miguel Torroja <miguel.torroja@gmail.com> wrote:
>>>
>>> On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
>>> <larsxschneider@gmail.com> wrote:
>>>>
>>>>> 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>
>>>>> ---
>>>>> ...
>>>>
>>>> I have never worked with p4 triggers and that might be
>>>> the reason why I don't understand your test case.
>>>> Maybe you can help me?
>>>>
>>>>> +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
>>>>> +     ) &&
>>>>
>>>> You clone the test repo and install a trigger.
>>>>
>>>>> +     (
>>>>> +             cd "$git" &&
>>>>> +             git config git-p4.skipSubmitEdit true &&
>>>>> +             echo file20 >file20 &&
>>>>> +             git add file20 &&
>>>>> +             git commit -m file20 &&
>>>>> +             git p4 submit
>>>>> +     ) &&
>>>>
>>>> You make a new commit. This should run the "echo verbose trigger", right?
>>>
>>> Yes, that's correct. In this case the trigger is run with p4 change
>>> and p4 changes
>>>
>>>>
>>>>> +     (
>>>>> +             p4 triggers -i <<-EOF
>>>>> +             Triggers:
>>>>> +             EOF
>>>>> +     ) &&
>>>>
>>>> You delete the trigger.
>>>>
>>>>> +     (
>>>>> +             cd "$cli" &&
>>>>> +             test_path_is_file file20
>>>>> +     )
>>>>
>>>> You check that the file20 is available in P4.
>>>>
>>>>
>>>> What would happen if I run this test case without your patch?
>>>> Wouldn't it pass just fine?
>>>
>>> If you run it without the patch for git-p4.py, the test doesn't pass
>>
>> You are right. I did not run "make" properly before running the test :)
>>
>>
>>>> Wouldn't we need to check that no warning/error is in the
>>>> real change description?
>>>>
>>>
>>> that can also be added, something like this: 'p4 change -o | grep
>>> "verbose trigger"' after setting the trigger?
>>
>> Yeah, maybe. I hope this is no stupid question, but: If you clone the
>> repo with git-p4 *again* ... would you see the "verbose trigger" output
>> in the Git commit message?
>>
>
> The commands that are affected are the ones that don't use the -G
> option, as everything is sent to the standard output without being
> able to filter out what is the real contents or just info messages.
> That's not the case with the python output (-G). Having said that... I
> tried what you just said (just to be sure) and the function
> p4_last_change fails... as it expects the first dictionary returned by
> p4CmdList is the one that contains the change:
> "int(results[0]['change'])" and that's not the case as it's an info
> entry (no 'change' key, that's in the next entry...)  I'll update with
> new patches
>
> I didn't notice that before because the P4 server we have in our
> office only outputs extra info messages with the command "p4 change".
>
>
>> - Lars

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

[Miguel Torroja]

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 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>
---
 git-p4.py                | 90 ++++++++++++++++++++++++++++++++----------------
 t/t9807-git-p4-submit.sh | 30 ++++++++++++++++
 2 files changed, 91 insertions(+), 29 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..a262e3253 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
@@ -1526,37 +1533,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..b630895a7 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -409,6 +409,36 @@ 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
+	) &&
+	(
+		p4 change -o |  grep -s "verbose trigger"
+	) &&
+	(
+		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
+	) &&
+	(
+		cd "$cli" &&
+		test_path_is_file file20
+	)
+'
+
 test_expect_success 'submit --prepare-p4-only' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.11.0

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

[Luke Diamand]

On 3 July 2017 at 23:57, 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 to t9807-git-p4-submit.sh adding a p4 trigger
> that outputs extra lines with "p4 change -o" and "p4 changes"

The latest version of mt/p4-parse-G-output (09521c7a0) seems to break
t9813-git-p4-preserve-users.sh.

I don't quite know why, but I wonder if it's the change to p4CmdList() ?

Luke

>
> Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
> ---
>  git-p4.py                | 90 ++++++++++++++++++++++++++++++++----------------
>  t/t9807-git-p4-submit.sh | 30 ++++++++++++++++
>  2 files changed, 91 insertions(+), 29 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 8d151da91..a262e3253 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
> @@ -1526,37 +1533,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..b630895a7 100755
> --- a/t/t9807-git-p4-submit.sh
> +++ b/t/t9807-git-p4-submit.sh
> @@ -409,6 +409,36 @@ 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
> +       ) &&
> +       (
> +               p4 change -o |  grep -s "verbose trigger"
> +       ) &&
> +       (
> +               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
> +       ) &&
> +       (
> +               cd "$cli" &&
> +               test_path_is_file file20
> +       )
> +'
> +
>  test_expect_success 'submit --prepare-p4-only' '
>         test_when_finished cleanup_git &&
>         git p4 clone --dest="$git" //depot &&
> --
> 2.11.0
>

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

[Miguel Torroja]

Hi Luke,


My bad as I didn't check that case.
It was p4CmdList as you said. the default value of the new field
skip_info (set to True) ignores any info messages. and the script is
waiting for a valid message.
If I set it to False, then it does return an info entry and it accepts
the submit change

I'm sending another patch update

On Tue, Jul 11, 2017 at 10:35 AM, Luke Diamand <luke@diamand.org> wrote:
> On 3 July 2017 at 23:57, 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 to t9807-git-p4-submit.sh adding a p4 trigger
>> that outputs extra lines with "p4 change -o" and "p4 changes"
>
> The latest version of mt/p4-parse-G-output (09521c7a0) seems to break
> t9813-git-p4-preserve-users.sh.
>
> I don't quite know why, but I wonder if it's the change to p4CmdList() ?
>
> Luke
>
>>
>> Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
>> ---
>>  git-p4.py                | 90 ++++++++++++++++++++++++++++++++----------------
>>  t/t9807-git-p4-submit.sh | 30 ++++++++++++++++
>>  2 files changed, 91 insertions(+), 29 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91..a262e3253 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
>> @@ -1526,37 +1533,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..b630895a7 100755
>> --- a/t/t9807-git-p4-submit.sh
>> +++ b/t/t9807-git-p4-submit.sh
>> @@ -409,6 +409,36 @@ 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
>> +       ) &&
>> +       (
>> +               p4 change -o |  grep -s "verbose trigger"
>> +       ) &&
>> +       (
>> +               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
>> +       ) &&
>> +       (
>> +               cd "$cli" &&
>> +               test_path_is_file file20
>> +       )
>> +'
>> +
>>  test_expect_success 'submit --prepare-p4-only' '
>>         test_when_finished cleanup_git &&
>>         git p4 clone --dest="$git" //depot &&
>> --
>> 2.11.0
>>

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

[Miguel Torroja]

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)
         for r in result:
             if r.has_key('code'):
                 if r['code'] == 'error':
@@ -1526,37 +1533,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..b630895a7 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -409,6 +409,36 @@ 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
+	) &&
+	(
+		p4 change -o |  grep -s "verbose trigger"
+	) &&
+	(
+		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
+	) &&
+	(
+		cd "$cli" &&
+		test_path_is_file file20
+	)
+'
+
 test_expect_success 'submit --prepare-p4-only' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.11.0

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

[Luke Diamand]

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

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

[Miguel Torroja]

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

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

[Junio C Hamano]

Miguel Torroja <miguel.torroja@gmail.com> writes:

> 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

I somehow feel that this logic is totally backwards.  The current
callers of p4CmdList() before your patch did not special case an
entry that was marked as 'info' in its 'code' field.  Your new
caller, which switched from using p4_read_pipe_lines() to p4CmdList()
is one caller that you *know* wants to special case such an entry
and wanted to skip.

Your original patch that was queued to 'pu' for a while and then
ejected from it after Travis saw an issue *assumed* that all other
callers to p4CmdList() also want to special case such an entry, and
that is why it made skip_info parameter default to True.

The difference between knowing and assuming is the cause of the bug
your original patch introduced into modifyChangelistUser().  

The way I read Luke's suggestion was that you can avoid making the
same mistake by not changing the behaviour for existing callers you
didn't look at.

Instead of assuming everybody else do not want an entry with 'code'
set to 'info', assume all the callers before your patch is doing
fine, and when you *know* some of them are better off ignoring such
an entry, explicitly tell them to do so, by:

 * The first patch adds skip_info parameter that defaults to False
   to p4CmdList() and do the special casing when it is set to True.

 * The second patch updates p4ChangesForPaths() to use the updated
   p4CmdList() and pass skip_info=True.  It is OK to squash this
   step into the first patch.

 * The third and later patches, if you need them, each examines an
   existing caller of p4CmdList(), and add a new test to demonstrate
   the existing breakage that comes from not ignoring an entry whose
   'code' is 'info'.  That test would serve as a good documentation
   to explain why it is better for the caller to pass skip_info=True,
   so the same patch would also update the code to do so.

While I was thinking the above through, here are a few cosmetic
things that I noticed.  There is another comma that is not followed
by a space in existing code that might want to be corrected in a
clean-up patch but that is totally outside of the scope of this
series.

Thanks.

 git-p4.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 1facf32db7..0d75753bce 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1501,7 +1501,7 @@ class P4Submit(Command, P4UserMap):
         c['User'] = newUser
         input = marshal.dumps(c)
 
-        result = p4CmdList("change -f -i", stdin=input,skip_info=False)
+        result = p4CmdList("change -f -i", stdin=input, skip_info=False)
         for r in result:
             if r.has_key('code'):
                 if r['code'] == 'error':
@@ -1575,7 +1575,7 @@ class P4Submit(Command, P4UserMap):
                 files_list.append(value)
                 continue
         # Output in the order expected by prepareLogMessage
-        for key in ['Change','Client','User','Status','Description','Jobs']:
+        for key in ['Change', 'Client', 'User', 'Status', 'Description', 'Jobs']:
             if not change_entry.has_key(key):
                 continue
             template += '\n'

[PATCH 1/3] git-p4: git-p4 tests with p4 triggers

[Miguel Torroja]

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
p4 commands. A few git-p4 commands don't expect extra messages or output
lines and may fail with verbose triggers.
New tests added are known to be broken.

Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
---
 t/t9831-git-p4-triggers.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100755 t/t9831-git-p4-triggers.sh

diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
new file mode 100755
index 000000000..28cafe469
--- /dev/null
+++ b/t/t9831-git-p4-triggers.sh
@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='git p4 with server triggers'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "change 1"
+		echo file2 >file2 &&
+		p4 add file2 &&
+		p4 submit -d "change 2"
+	)
+'
+
+test_expect_failure 'clone with extra info lines from verbose p4 trigger' '
+	test_when_finished cleanup_git &&
+	(
+		p4 triggers -i <<-EOF
+		Triggers: p4triggertest-command command pre-user-change "echo verbose trigger"
+		EOF
+	) &&
+	(
+		p4 change -o |  grep -s "verbose trigger"
+	) &&
+	git p4 clone --dest="$git" //depot/@all &&
+	(
+		p4 triggers -i <<-EOF
+		Triggers:
+		EOF
+	)
+'
+
+test_expect_failure 'import with extra info lines from verbose p4 trigger' '
+	test_when_finished cleanup_git &&
+	(
+		cd "$cli" &&
+		echo file3 >file3 &&
+		p4 add file3 &&
+		p4 submit -d "change 3"
+	) &&
+	(
+		p4 triggers -i <<-EOF
+		Triggers: p4triggertest-command command pre-user-describe "echo verbose trigger"
+		EOF
+	) &&
+	(
+		p4 describe 1 |  grep -s "verbose trigger"
+	) &&
+	git p4 clone --dest="$git" //depot/@all &&
+	(
+		cd "$git" &&
+		git p4 sync
+	)&&
+	(
+		p4 triggers -i <<-EOF
+		Triggers:
+		EOF
+	)
+'
+
+test_expect_failure 'submit description with extra info lines from verbose p4 change trigger' '
+	test_when_finished cleanup_git &&
+	(
+		p4 triggers -i <<-EOF
+		Triggers: p4triggertest-command command pre-user-change "echo verbose trigger"
+		EOF
+	) &&
+	(
+		p4 change -o |  grep -s "verbose trigger"
+	) &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		echo file4 >file4 &&
+		git add file4 &&
+		git commit -m file4 &&
+		git p4 submit
+	) &&
+	(
+		p4 triggers -i <<-EOF
+		Triggers:
+		EOF
+	) &&
+	(
+		cd "$cli" &&
+		test_path_is_file file4
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
2.11.0

[PATCH 2/3] git-p4: parse marshal output "p4 -G" in p4 changes

[Miguel Torroja]

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.

This fixes the case where a p4 trigger for  "p4 change" is set and the command git-p4 submit is run.

Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
---
 git-p4.py                  | 85 +++++++++++++++++++++++++++++++---------------
 t/t9831-git-p4-triggers.sh |  2 +-
 2 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..e3a2791e0 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/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index 28cafe469..871544b1c 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -66,7 +66,7 @@ test_expect_failure 'import with extra info lines from verbose p4 trigger' '
 	)
 '
 
-test_expect_failure 'submit description with extra info lines from verbose p4 change trigger' '
+test_expect_success 'submit description with extra info lines from verbose p4 change trigger' '
 	test_when_finished cleanup_git &&
 	(
 		p4 triggers -i <<-EOF
-- 
2.11.0

[PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList

[Miguel Torroja]

The function p4CmdList accepts a new argument: skip_info. When set to
True it ignores any 'code':'info' entry (skip_info=False by default).

That allows us to fix some of the tests in t9831-git-p4-triggers.sh
known to be broken with verobse p4 triggers

Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com>
---
 git-p4.py                  | 9 ++++++---
 t/t9831-git-p4-triggers.sh | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index e3a2791e0..2fa581789 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -313,7 +313,7 @@ def p4_move(src, dest):
     p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
 
 def p4_last_change():
-    results = p4CmdList(["changes", "-m", "1"])
+    results = p4CmdList(["changes", "-m", "1"], skip_info=True)
     return int(results[0]['change'])
 
 def p4_describe(change):
@@ -321,7 +321,7 @@ def p4_describe(change):
        the presence of field "time".  Return a dict of the
        results."""
 
-    ds = p4CmdList(["describe", "-s", str(change)])
+    ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
     if len(ds) != 1:
         die("p4 describe -s %d did not return 1 result: %s" % (change, str(ds)))
 
@@ -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=False):
 
     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:
diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index 871544b1c..bbcf14c66 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -20,7 +20,7 @@ test_expect_success 'init depot' '
 	)
 '
 
-test_expect_failure 'clone with extra info lines from verbose p4 trigger' '
+test_expect_success 'clone with extra info lines from verbose p4 trigger' '
 	test_when_finished cleanup_git &&
 	(
 		p4 triggers -i <<-EOF
@@ -38,7 +38,7 @@ test_expect_failure 'clone with extra info lines from verbose p4 trigger' '
 	)
 '
 
-test_expect_failure 'import with extra info lines from verbose p4 trigger' '
+test_expect_success 'import with extra info lines from verbose p4 trigger' '
 	test_when_finished cleanup_git &&
 	(
 		cd "$cli" &&
-- 
2.11.0

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

[Miguel Torroja]

Thanks,

I've just sent in reply to your previous e-mail three different patches.

* The first patch is just to show some broken tests,
* Second patch is to fix the original issue I had (the one that
initiated this thread)
* Third patch is the one that filters out "info" messages in p4CmdList
(this time default is reversed and set to False, what is the original
behaviour). The two test cases that are cured with this change have to
set explicitely skip_info=True.


On Wed, Jul 12, 2017 at 7:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Miguel Torroja <miguel.torroja@gmail.com> writes:
>
>> 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
>
> I somehow feel that this logic is totally backwards.  The current
> callers of p4CmdList() before your patch did not special case an
> entry that was marked as 'info' in its 'code' field.  Your new
> caller, which switched from using p4_read_pipe_lines() to p4CmdList()
> is one caller that you *know* wants to special case such an entry
> and wanted to skip.
>
> Your original patch that was queued to 'pu' for a while and then
> ejected from it after Travis saw an issue *assumed* that all other
> callers to p4CmdList() also want to special case such an entry, and
> that is why it made skip_info parameter default to True.
>
> The difference between knowing and assuming is the cause of the bug
> your original patch introduced into modifyChangelistUser().
>
> The way I read Luke's suggestion was that you can avoid making the
> same mistake by not changing the behaviour for existing callers you
> didn't look at.
>
> Instead of assuming everybody else do not want an entry with 'code'
> set to 'info', assume all the callers before your patch is doing
> fine, and when you *know* some of them are better off ignoring such
> an entry, explicitly tell them to do so, by:
>
>  * The first patch adds skip_info parameter that defaults to False
>    to p4CmdList() and do the special casing when it is set to True.
>
>  * The second patch updates p4ChangesForPaths() to use the updated
>    p4CmdList() and pass skip_info=True.  It is OK to squash this
>    step into the first patch.
>
>  * The third and later patches, if you need them, each examines an
>    existing caller of p4CmdList(), and add a new test to demonstrate
>    the existing breakage that comes from not ignoring an entry whose
>    'code' is 'info'.  That test would serve as a good documentation
>    to explain why it is better for the caller to pass skip_info=True,
>    so the same patch would also update the code to do so.
>
> While I was thinking the above through, here are a few cosmetic
> things that I noticed.  There is another comma that is not followed
> by a space in existing code that might want to be corrected in a
> clean-up patch but that is totally outside of the scope of this
> series.
>
> Thanks.
>
>  git-p4.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 1facf32db7..0d75753bce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1501,7 +1501,7 @@ class P4Submit(Command, P4UserMap):
>          c['User'] = newUser
>          input = marshal.dumps(c)
>
> -        result = p4CmdList("change -f -i", stdin=input,skip_info=False)
> +        result = p4CmdList("change -f -i", stdin=input, skip_info=False)
>          for r in result:
>              if r.has_key('code'):
>                  if r['code'] == 'error':
> @@ -1575,7 +1575,7 @@ class P4Submit(Command, P4UserMap):
>                  files_list.append(value)
>                  continue
>          # Output in the order expected by prepareLogMessage
> -        for key in ['Change','Client','User','Status','Description','Jobs']:
> +        for key in ['Change', 'Client', 'User', 'Status', 'Description', 'Jobs']:
>              if not change_entry.has_key(key):
>                  continue
>              template += '\n'

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

[Junio C Hamano]

Miguel Torroja <miguel.torroja@gmail.com> writes:

> I've just sent in reply to your previous e-mail three different patches.
>
> * The first patch is just to show some broken tests,
> * Second patch is to fix the original issue I had (the one that
> initiated this thread)
> * Third patch is the one that filters out "info" messages in p4CmdList
> (this time default is reversed and set to False, what is the original
> behaviour). The two test cases that are cured with this change have to
> set explicitely skip_info=True.

The approach looks reasonable.  By having tests that expect failure
upfront, the series clearly shows how the code changes in later
steps make things better.

Thanks.  Will replace.