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

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

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'