Miguel Torroja <miguel.torroja@xxxxxxxxx> 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'
