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

> 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

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