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

> 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