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

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

On Fri, Jun 30, 2017 at 12:13 PM, Lars Schneider
[off-list ref] wrote:

quoted

quoted

On 30 Jun 2017, at 11:41, Miguel Torroja [off-list ref] wrote:

On Fri, Jun 30, 2017 at 10:26 AM, Lars Schneider
[off-list ref] wrote:

quoted

quoted

On 30 Jun 2017, at 00:46, miguel torroja [off-list ref] 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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
...

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?

quoted

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

quoted

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

quoted

quoted

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

You delete the trigger.

quoted

+     (
+             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 :)

quoted

quoted

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

quoted

- Lars