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

[Miguel Torroja <miguel.torroja@gmail.com>]

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