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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2017, 17:13 UTC
Message-ID: <xmqqr2xl1suy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAKYtbVb8T=edPG5539=uwDjHnCerLO2Oejy8bWK+giSS8nNGig@mail.gmail.com>

Miguel Torroja <miguel.torroja@gmail.com> writes:

Show 24 quoted lines

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