* [PATCH] git-p4: changelist template with p4 -G change -o @ 2017-06-20 12:19 Miguel Torroja 2017-06-22 17:32 ` Junio C Hamano 0 siblings, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-06-20 12:19 UTC (permalink / raw) To: git; +Cc: Miguel Torroja 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 plugin/hooks in the server side generates 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'} Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> --- git-p4.py | 77 ++++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da..a300474 100755 --- a/git-p4.py +++ b/git-p4.py @@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap): [upstream, settings] = findUpstreamBranchPoint() - template = "" + template = """\ +# A Perforce Change Specification. +# +# Change: The change number. 'new' on a new changelist. +# Date: The date this specification was last modified. +# Client: The client on which the changelist was created. Read-only. +# User: The user who created the changelist. +# Status: Either 'pending' or 'submitted'. Read-only. +# Type: Either 'public' or 'restricted'. Default is 'public'. +# Description: Comments about the changelist. Required. +# Jobs: What opened jobs are to be closed by this changelist. +# You may delete jobs from this list. (New changelists only.) +# Files: What opened files from the default changelist are to be added +# to this changelist. You may delete files from this list. +# (New changelists only.) +""" + files_list = [] inFilesSection = False + change_entry = None args = ['change', '-o'] if changelist: args.append(str(changelist)) - - for line in p4_read_pipe_lines(args): - if line.endswith("\r\n"): - line = line[:-2] + "\n" - if inFilesSection: - if line.startswith("\t"): - # path starts and ends with a tab - path = line[1:] - lastTab = path.rfind("\t") - if lastTab != -1: - path = path[:lastTab] - if settings.has_key('depot-paths'): - if not [p for p in settings['depot-paths'] - if p4PathStartsWith(path, p)]: - continue - else: - if not p4PathStartsWith(path, self.depotPath): - continue + for entry in p4CmdList(args): + if not entry.has_key('code'): + continue + if entry['code'] == 'stat': + change_entry = entry + break + if not change_entry: + die('Failed to decode output of p4 change -o') + for key, value in change_entry.iteritems(): + if key.startswith('File'): + if settings.has_key('depot-paths'): + if not [p for p in settings['depot-paths'] + if p4PathStartsWith(value, p)]: + continue else: - inFilesSection = False - else: - if line.startswith("Files:"): - inFilesSection = True - - template += line - + if not p4PathStartsWith(value, self.depotPath): + continue + files_list.append(value) + continue + # Output in the order expected by prepareLogMessage + for key in ['Change','Client','User','Status','Description','Jobs']: + if not change_entry.has_key(key): + continue + template += '\n' + template += key + ':' + if key == 'Description': + template += '\n' + for field_line in change_entry[key].splitlines(): + template += '\t'+field_line+'\n' + if len(files_list) > 0: + template += '\n' + template += 'Files:\n' + for path in files_list: + template += '\t'+path+'\n' return template def edit_template(self, template_file): -- 2.1.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: changelist template with p4 -G change -o 2017-06-20 12:19 [PATCH] git-p4: changelist template with p4 -G change -o Miguel Torroja @ 2017-06-22 17:32 ` Junio C Hamano 2017-06-24 11:49 ` Luke Diamand 0 siblings, 1 reply; 31+ messages in thread From: Junio C Hamano @ 2017-06-22 17:32 UTC (permalink / raw) To: Luke Diamand, Lars Schneider; +Cc: git, Miguel Torroja Miguel Torroja <miguel.torroja@gmail.com> writes: > 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 plugin/hooks in the server side generates 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'} > > Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> > --- Asking for help reviewing to those who have worked on git-p4 in the past... Thanks. > git-p4.py | 77 ++++++++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 51 insertions(+), 26 deletions(-) > > diff --git a/git-p4.py b/git-p4.py > index 8d151da..a300474 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap): > > [upstream, settings] = findUpstreamBranchPoint() > > - template = "" > + template = """\ > +# A Perforce Change Specification. > +# > +# Change: The change number. 'new' on a new changelist. > +# Date: The date this specification was last modified. > +# Client: The client on which the changelist was created. Read-only. > +# User: The user who created the changelist. > +# Status: Either 'pending' or 'submitted'. Read-only. > +# Type: Either 'public' or 'restricted'. Default is 'public'. > +# Description: Comments about the changelist. Required. > +# Jobs: What opened jobs are to be closed by this changelist. > +# You may delete jobs from this list. (New changelists only.) > +# Files: What opened files from the default changelist are to be added > +# to this changelist. You may delete files from this list. > +# (New changelists only.) > +""" > + files_list = [] > inFilesSection = False > + change_entry = None > args = ['change', '-o'] > if changelist: > args.append(str(changelist)) > - > - for line in p4_read_pipe_lines(args): > - if line.endswith("\r\n"): > - line = line[:-2] + "\n" > - if inFilesSection: > - if line.startswith("\t"): > - # path starts and ends with a tab > - path = line[1:] > - lastTab = path.rfind("\t") > - if lastTab != -1: > - path = path[:lastTab] > - if settings.has_key('depot-paths'): > - if not [p for p in settings['depot-paths'] > - if p4PathStartsWith(path, p)]: > - continue > - else: > - if not p4PathStartsWith(path, self.depotPath): > - continue > + for entry in p4CmdList(args): > + if not entry.has_key('code'): > + continue > + if entry['code'] == 'stat': > + change_entry = entry > + break > + if not change_entry: > + die('Failed to decode output of p4 change -o') > + for key, value in change_entry.iteritems(): > + if key.startswith('File'): > + if settings.has_key('depot-paths'): > + if not [p for p in settings['depot-paths'] > + if p4PathStartsWith(value, p)]: > + continue > else: > - inFilesSection = False > - else: > - if line.startswith("Files:"): > - inFilesSection = True > - > - template += line > - > + if not p4PathStartsWith(value, self.depotPath): > + continue > + files_list.append(value) > + continue > + # Output in the order expected by prepareLogMessage > + for key in ['Change','Client','User','Status','Description','Jobs']: > + if not change_entry.has_key(key): > + continue > + template += '\n' > + template += key + ':' > + if key == 'Description': > + template += '\n' > + for field_line in change_entry[key].splitlines(): > + template += '\t'+field_line+'\n' > + if len(files_list) > 0: > + template += '\n' > + template += 'Files:\n' > + for path in files_list: > + template += '\t'+path+'\n' > return template > > def edit_template(self, template_file): ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: changelist template with p4 -G change -o 2017-06-22 17:32 ` Junio C Hamano @ 2017-06-24 11:49 ` Luke Diamand 2017-06-24 12:38 ` miguel torroja 2017-06-24 17:36 ` Lars Schneider 0 siblings, 2 replies; 31+ messages in thread From: Luke Diamand @ 2017-06-24 11:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lars Schneider, Git Users, Miguel Torroja On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote: > Miguel Torroja <miguel.torroja@gmail.com> writes: > >> 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 plugin/hooks in the server side generates 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'} I think this seems like a reasonable thing to do if "p4 change -o" is jumbling up output. One thing I notice trying it out by hand is that we seem to have lost the annotation of the Perforce per-file modification type (is there a proper name for this?). For example, if I add a file called "baz", then the original version creates a template which looks like this: //depot/baz # add But the new one creates a template which looks like: //depot/baz Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: changelist template with p4 -G change -o 2017-06-24 11:49 ` Luke Diamand @ 2017-06-24 12:38 ` miguel torroja 2017-06-24 17:36 ` Lars Schneider 1 sibling, 0 replies; 31+ messages in thread From: miguel torroja @ 2017-06-24 12:38 UTC (permalink / raw) To: Luke Diamand; +Cc: Junio C Hamano, Lars Schneider, Git Users You are right about the "# add"comment. I couldn't find any extra info in the marshaled output that I can use to add the change action comment after the path. That's one downside of that change. On Sat, Jun 24, 2017 at 1:49 PM, Luke Diamand <luke@diamand.org> wrote: > On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote: >> Miguel Torroja <miguel.torroja@gmail.com> writes: >> >>> 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 plugin/hooks in the server side generates 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'} > > I think this seems like a reasonable thing to do if "p4 change -o" is > jumbling up output. > > One thing I notice trying it out by hand is that we seem to have lost > the annotation of the Perforce per-file modification type (is there a > proper name for this?). > > For example, if I add a file called "baz", then the original version > creates a template which looks like this: > > //depot/baz # add > > But the new one creates a template which looks like: > > //depot/baz > > Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: changelist template with p4 -G change -o 2017-06-24 11:49 ` Luke Diamand 2017-06-24 12:38 ` miguel torroja @ 2017-06-24 17:36 ` Lars Schneider [not found] ` <CAKYtbVbGekXGAyPd7HeLot_MdZkp7-1Ss-iAi7o8ze2b+sNB6Q@mail.gmail.com> 1 sibling, 1 reply; 31+ messages in thread From: Lars Schneider @ 2017-06-24 17:36 UTC (permalink / raw) To: Luke Diamand; +Cc: Junio C Hamano, Git Users, Miguel Torroja > On 24 Jun 2017, at 13:49, Luke Diamand <luke@diamand.org> wrote: > > On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote: >> Miguel Torroja <miguel.torroja@gmail.com> writes: >> >>> 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 plugin/hooks in the server side generates 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'} > > I think this seems like a reasonable thing to do if "p4 change -o" is > jumbling up output. > > One thing I notice trying it out by hand is that we seem to have lost > the annotation of the Perforce per-file modification type (is there a > proper name for this?). > > For example, if I add a file called "baz", then the original version > creates a template which looks like this: > > //depot/baz # add > > But the new one creates a template which looks like: > > //depot/baz @Miguel: You wrote that p4 plugins/hooks generate these warnings. I wonder if you see a way to replicate that in a test case. Either in t9800 or a new t98XX test case file? - Lars ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CAKYtbVbGekXGAyPd7HeLot_MdZkp7-1Ss-iAi7o8ze2b+sNB6Q@mail.gmail.com>]
* Re: [PATCH] git-p4: changelist template with p4 -G change -o [not found] ` <CAKYtbVbGekXGAyPd7HeLot_MdZkp7-1Ss-iAi7o8ze2b+sNB6Q@mail.gmail.com> @ 2017-06-27 9:20 ` miguel torroja 2017-06-27 19:17 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 0 siblings, 1 reply; 31+ messages in thread From: miguel torroja @ 2017-06-27 9:20 UTC (permalink / raw) To: Lars Schneider; +Cc: Junio C Hamano (Gitster), Git Users, Luke Diamand Hi Lars/Luke, I tried a first test extending t9807-git-p4-submit.sh. I set this p4 trigger: 'p4test command pre-user-change "echo verbose trigger" '. I'm able to reproduce the issue I wanted to fix. However I found yet another issue, in this case when reading the result from p4_read_pipe_lines (in function p4ChangesForPaths), the pre-user-change is triggered with any "p4 change" and "p4 changes" command (the p4 server we have in production, only shows "extra" messages with p4 change). I'll collapse in one single commit the fix for p4 change/p4 changes and the new test. Thanks, Miguel On Sat, Jun 24, 2017 at 10:37 PM, miguel torroja <miguel.torroja@gmail.com> wrote: > Hi Lars, > > I think it's doable to set a custom p4 trigger, created by the test case, > that outputs "extra info" when requesting a changelist description. > I'll do a specific test and post it to this thread. > > > Thanks, > > > El 24 jun. 2017 7:36 p. m., "Lars Schneider" <larsxschneider@gmail.com> > escribió: > > >> On 24 Jun 2017, at 13:49, Luke Diamand <luke@diamand.org> wrote: >> >> On 22 June 2017 at 18:32, Junio C Hamano <gitster@pobox.com> wrote: >>> Miguel Torroja <miguel.torroja@gmail.com> writes: >>> >>>> 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 plugin/hooks in the server side generates 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'} >> >> I think this seems like a reasonable thing to do if "p4 change -o" is >> jumbling up output. >> >> One thing I notice trying it out by hand is that we seem to have lost >> the annotation of the Perforce per-file modification type (is there a >> proper name for this?). >> >> For example, if I add a file called "baz", then the original version >> creates a template which looks like this: >> >> //depot/baz # add >> >> But the new one creates a template which looks like: >> >> //depot/baz > > @Miguel: You wrote that p4 plugins/hooks generate these warnings. > I wonder if you see a way to replicate that in a test case. Either > in t9800 or a new t98XX test case file? > > - Lars > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-27 9:20 ` miguel torroja @ 2017-06-27 19:17 ` Miguel Torroja 2017-06-28 4:08 ` Junio C Hamano 0 siblings, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-06-27 19:17 UTC (permalink / raw) To: larsxschneider, luke, git; +Cc: gitster, Miguel Torroja 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> --- git-p4.py | 83 ++++++++++++++++++++++++++++++++---------------- t/t9807-git-p4-submit.sh | 28 ++++++++++++++++ 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da91..239a8f144 100755 --- a/git-p4.py +++ b/git-p4.py @@ -879,8 +879,10 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): cmd += ["%s...@%s" % (p, revisionRange)] # Insert changes in chronological order - for line in reversed(p4_read_pipe_lines(cmd)): - changes.add(int(line.split(" ")[1])) + for entry in reversed(p4CmdList(cmd)): + if not entry.has_key('change'): + continue + changes.add(int(entry['change'])) if not block_size: break @@ -1526,37 +1528,62 @@ class P4Submit(Command, P4UserMap): [upstream, settings] = findUpstreamBranchPoint() - template = "" + template = """\ +# A Perforce Change Specification. +# +# Change: The change number. 'new' on a new changelist. +# Date: The date this specification was last modified. +# Client: The client on which the changelist was created. Read-only. +# User: The user who created the changelist. +# Status: Either 'pending' or 'submitted'. Read-only. +# Type: Either 'public' or 'restricted'. Default is 'public'. +# Description: Comments about the changelist. Required. +# Jobs: What opened jobs are to be closed by this changelist. +# You may delete jobs from this list. (New changelists only.) +# Files: What opened files from the default changelist are to be added +# to this changelist. You may delete files from this list. +# (New changelists only.) +""" + files_list = [] inFilesSection = False + change_entry = None args = ['change', '-o'] if changelist: args.append(str(changelist)) - - for line in p4_read_pipe_lines(args): - if line.endswith("\r\n"): - line = line[:-2] + "\n" - if inFilesSection: - if line.startswith("\t"): - # path starts and ends with a tab - path = line[1:] - lastTab = path.rfind("\t") - if lastTab != -1: - path = path[:lastTab] - if settings.has_key('depot-paths'): - if not [p for p in settings['depot-paths'] - if p4PathStartsWith(path, p)]: - continue - else: - if not p4PathStartsWith(path, self.depotPath): - continue + for entry in p4CmdList(args): + if not entry.has_key('code'): + continue + if entry['code'] == 'stat': + change_entry = entry + break + if not change_entry: + die('Failed to decode output of p4 change -o') + for key, value in change_entry.iteritems(): + if key.startswith('File'): + if settings.has_key('depot-paths'): + if not [p for p in settings['depot-paths'] + if p4PathStartsWith(value, p)]: + continue else: - inFilesSection = False - else: - if line.startswith("Files:"): - inFilesSection = True - - template += line - + if not p4PathStartsWith(value, self.depotPath): + continue + files_list.append(value) + continue + # Output in the order expected by prepareLogMessage + for key in ['Change','Client','User','Status','Description','Jobs']: + if not change_entry.has_key(key): + continue + template += '\n' + template += key + ':' + if key == 'Description': + template += '\n' + for field_line in change_entry[key].splitlines(): + template += '\t'+field_line+'\n' + if len(files_list) > 0: + template += '\n' + template += 'Files:\n' + for path in files_list: + template += '\t'+path+'\n' return template def edit_template(self, template_file): diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index 3457d5db6..05d7fc3f7 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -409,6 +409,34 @@ test_expect_success 'description with Jobs section and bogus following text' ' ) ' +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 + ) && + ( + cd "$git" && + git config git-p4.skipSubmitEdit true && + echo file20 >file20 && + git add file20 && + git commit -m file20 && + git p4 submit + ) && + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) && + ( + cd "$cli" && + test_path_is_file file20 + ) +' + test_expect_success 'submit --prepare-p4-only' ' test_when_finished cleanup_git && git p4 clone --dest="$git" //depot && -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-27 19:17 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja @ 2017-06-28 4:08 ` Junio C Hamano 2017-06-28 9:54 ` Luke Diamand 0 siblings, 1 reply; 31+ messages in thread From: Junio C Hamano @ 2017-06-28 4:08 UTC (permalink / raw) To: Miguel Torroja; +Cc: larsxschneider, luke, git Miguel Torroja <miguel.torroja@gmail.com> writes: > 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> > --- It appears that https://travis-ci.org/git/git/builds/247724639 does not like this change. For example: https://travis-ci.org/git/git/jobs/247724642#L1848 indicates that not just 9807 (new tests added by this patch) but also 9800 starts to fail. I'd wait for git-p4 experts to comment and help guiding this change forward. Thanks. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-28 4:08 ` Junio C Hamano @ 2017-06-28 9:54 ` Luke Diamand 2017-06-28 13:14 ` miguel torroja 0 siblings, 1 reply; 31+ messages in thread From: Luke Diamand @ 2017-06-28 9:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miguel Torroja, Lars Schneider, Git Users On 28 June 2017 at 05:08, Junio C Hamano <gitster@pobox.com> wrote: > Miguel Torroja <miguel.torroja@gmail.com> writes: > >> 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> >> --- > > It appears that https://travis-ci.org/git/git/builds/247724639 > does not like this change. For example: > > https://travis-ci.org/git/git/jobs/247724642#L1848 > > indicates that not just 9807 (new tests added by this patch) but > also 9800 starts to fail. > > I'd wait for git-p4 experts to comment and help guiding this change > forward. I only see a (very weird) failure in t9800. I wonder if there are some P4 version differences. Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16). Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20) There's also a whitespace error according to "git diff --check". : Sadly I don't think there's any way to do this and yet keep the "# edit" comments. It looks like "p4 change -o" outputs lines with "'# edit" on the end, but the (supposedly semantically equivalent) "p4 -G change -o" command does not. I think that's a P4 bug. So we have a choice of fixing a garbled message in the face of scripts in the backend, or keeping the comments, or writing some extra Python to infer them. I vote for fixing the garbled message. Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-28 9:54 ` Luke Diamand @ 2017-06-28 13:14 ` miguel torroja [not found] ` <CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com> 0 siblings, 1 reply; 31+ messages in thread From: miguel torroja @ 2017-06-28 13:14 UTC (permalink / raw) To: Luke Diamand; +Cc: Junio C Hamano, Lars Schneider, Git Users Thanks Luke, regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT should display error), for me it's very weird too as it doesn't seem to be related to this particular change, as the patch changes are not exercised with that test. The test 21 in t9807 was precisely the new test added to test the change (it was passing with local setup), the test log is truncated before the output of test 21 in t9807 but I'm afraid I'm not very familiar with Travis, so maybe I'm missing something. Is there a way to have the full logs or they are always truncated after some number of lines? I think you get an error with git diff --check because I added spaces after a tab, but those spaces are intentional, the tabs are for the "<<-EOF" and spaces are for the "p4 triggers" specificiation. Thanks, On Wed, Jun 28, 2017 at 11:54 AM, Luke Diamand <luke@diamand.org> wrote: > On 28 June 2017 at 05:08, Junio C Hamano <gitster@pobox.com> wrote: >> Miguel Torroja <miguel.torroja@gmail.com> writes: >> >>> 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> >>> --- >> >> It appears that https://travis-ci.org/git/git/builds/247724639 >> does not like this change. For example: >> >> https://travis-ci.org/git/git/jobs/247724642#L1848 >> >> indicates that not just 9807 (new tests added by this patch) but >> also 9800 starts to fail. >> >> I'd wait for git-p4 experts to comment and help guiding this change >> forward. > > I only see a (very weird) failure in t9800. I wonder if there are some > P4 version differences. > > Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16). > Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20) > > There's also a whitespace error according to "git diff --check". > : > Sadly I don't think there's any way to do this and yet keep the "# > edit" comments. It looks like "p4 change -o" outputs lines with "'# > edit" on the end, but the (supposedly semantically equivalent) "p4 -G > change -o" command does not. I think that's a P4 bug. > > So we have a choice of fixing a garbled message in the face of scripts > in the backend, or keeping the comments, or writing some extra Python > to infer them. I vote for fixing the garbled message. > > Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com>]
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes [not found] ` <CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com> @ 2017-06-29 22:41 ` miguel torroja 2017-06-30 7:56 ` Luke Diamand 2017-06-29 22:46 ` Miguel Torroja 1 sibling, 1 reply; 31+ messages in thread From: miguel torroja @ 2017-06-29 22:41 UTC (permalink / raw) To: Luke Diamand; +Cc: Junio C Hamano, Lars Schneider, Git Users On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <luke@diamand.org> wrote: > On 28 June 2017 at 14:14, miguel torroja <miguel.torroja@gmail.com> wrote: >> Thanks Luke, >> >> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT >> should display error), for me it's very weird too as it doesn't seem >> to be related to this particular change, as the patch changes are not >> exercised with that test. > > I had a look at this. The problem is that the old code uses > p4_read_pipe_lines() which calls sys.exit() if the subprocess fails. > > But the new code calls p4CmdList() which has does error handling by > setting "p4ExitCode" to a non-zero value in the returned dictionary. > > I think if you just check for that case, the test will then pass Thank you for debugging this, I did as you suggested and it passed that test! >> >> The test 21 in t9807 was precisely the new test added to test the >> change (it was passing with local setup), the test log is truncated >> before the output of test 21 in t9807 but I'm afraid I'm not very >> familiar with Travis, so maybe I'm missing something. Is there a way >> to have the full logs or they are always truncated after some number >> of lines? > > For me, t9807 is working fine. > >> >> I think you get an error with git diff --check because I added spaces >> after a tab, but those spaces are intentional, the tabs are for the >> "<<-EOF" and spaces are for the "p4 triggers" specificiation. > > OK. > In the end, ,the reason t9807 was not passing was precisely the tabs and spaces of the patch. the original patch had: <tab><tab><spaces>....., as I explained, the tabs were supposed to be ignored by "<<-EOF" and the spaces were supposed to be sent to stdin of p4 triggers, but when the patch was applied to upstream the <spaces> were substituted by tabs what led to a malformed "p4 trigger" description. I just collapsed the description in one single line and now it's passing > > Luke I'm sending a new patch with the two changes I just mentioned. Thanks, ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-29 22:41 ` miguel torroja @ 2017-06-30 7:56 ` Luke Diamand 2017-06-30 8:22 ` miguel torroja 0 siblings, 1 reply; 31+ messages in thread From: Luke Diamand @ 2017-06-30 7:56 UTC (permalink / raw) To: miguel torroja; +Cc: Junio C Hamano, Lars Schneider, Git Users On 29 June 2017 at 23:41, miguel torroja <miguel.torroja@gmail.com> wrote: > On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <luke@diamand.org> wrote: >> On 28 June 2017 at 14:14, miguel torroja <miguel.torroja@gmail.com> wrote: >>> Thanks Luke, >>> >>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT >>> should display error), for me it's very weird too as it doesn't seem >>> to be related to this particular change, as the patch changes are not >>> exercised with that test. >> >> I had a look at this. The problem is that the old code uses >> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails. >> >> But the new code calls p4CmdList() which has does error handling by >> setting "p4ExitCode" to a non-zero value in the returned dictionary. >> >> I think if you just check for that case, the test will then pass > > Thank you for debugging this, I did as you suggested and it passed that test! > >>> >>> The test 21 in t9807 was precisely the new test added to test the >>> change (it was passing with local setup), the test log is truncated >>> before the output of test 21 in t9807 but I'm afraid I'm not very >>> familiar with Travis, so maybe I'm missing something. Is there a way >>> to have the full logs or they are always truncated after some number >>> of lines? >> >> For me, t9807 is working fine. >> >>> >>> I think you get an error with git diff --check because I added spaces >>> after a tab, but those spaces are intentional, the tabs are for the >>> "<<-EOF" and spaces are for the "p4 triggers" specificiation. >> >> OK. >> > > In the end, ,the reason t9807 was not passing was precisely the tabs > and spaces of the patch. the original patch had: > <tab><tab><spaces>....., as I explained, the tabs were supposed to be > ignored by "<<-EOF" and the spaces were supposed to be sent to stdin > of p4 triggers, but when the patch was applied to upstream the > <spaces> were substituted by tabs what led to a malformed "p4 > trigger" description. I just collapsed the description in one single > line and now it's passing >> >> Luke > > > I'm sending a new patch with the two changes I just mentioned. Looks good to me, Ack. Can we squash the two changes together? Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-30 7:56 ` Luke Diamand @ 2017-06-30 8:22 ` miguel torroja 0 siblings, 0 replies; 31+ messages in thread From: miguel torroja @ 2017-06-30 8:22 UTC (permalink / raw) To: Luke Diamand; +Cc: Junio C Hamano, Lars Schneider, Git Users The Latest patch I sent was already the squashed version with the fix to pass the tests. Thanks, On Fri, Jun 30, 2017 at 9:56 AM, Luke Diamand <luke@diamand.org> wrote: > On 29 June 2017 at 23:41, miguel torroja <miguel.torroja@gmail.com> wrote: >> On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand <luke@diamand.org> wrote: >>> On 28 June 2017 at 14:14, miguel torroja <miguel.torroja@gmail.com> wrote: >>>> Thanks Luke, >>>> >>>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT >>>> should display error), for me it's very weird too as it doesn't seem >>>> to be related to this particular change, as the patch changes are not >>>> exercised with that test. >>> >>> I had a look at this. The problem is that the old code uses >>> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails. >>> >>> But the new code calls p4CmdList() which has does error handling by >>> setting "p4ExitCode" to a non-zero value in the returned dictionary. >>> >>> I think if you just check for that case, the test will then pass >> >> Thank you for debugging this, I did as you suggested and it passed that test! >> >>>> >>>> The test 21 in t9807 was precisely the new test added to test the >>>> change (it was passing with local setup), the test log is truncated >>>> before the output of test 21 in t9807 but I'm afraid I'm not very >>>> familiar with Travis, so maybe I'm missing something. Is there a way >>>> to have the full logs or they are always truncated after some number >>>> of lines? >>> >>> For me, t9807 is working fine. >>> >>>> >>>> I think you get an error with git diff --check because I added spaces >>>> after a tab, but those spaces are intentional, the tabs are for the >>>> "<<-EOF" and spaces are for the "p4 triggers" specificiation. >>> >>> OK. >>> >> >> In the end, ,the reason t9807 was not passing was precisely the tabs >> and spaces of the patch. the original patch had: >> <tab><tab><spaces>....., as I explained, the tabs were supposed to be >> ignored by "<<-EOF" and the spaces were supposed to be sent to stdin >> of p4 triggers, but when the patch was applied to upstream the >> <spaces> were substituted by tabs what led to a malformed "p4 >> trigger" description. I just collapsed the description in one single >> line and now it's passing >>> >>> Luke >> >> >> I'm sending a new patch with the two changes I just mentioned. > > Looks good to me, Ack. Can we squash the two changes together? > > Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes [not found] ` <CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com> 2017-06-29 22:41 ` miguel torroja @ 2017-06-29 22:46 ` Miguel Torroja 2017-06-30 8:26 ` Lars Schneider 1 sibling, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-06-29 22:46 UTC (permalink / raw) To: Luke Diamand, git; +Cc: Junio C Hamano, Lars Schneider, Miguel Torroja 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> --- git-p4.py | 85 ++++++++++++++++++++++++++++++++---------------- t/t9807-git-p4-submit.sh | 27 +++++++++++++++ 2 files changed, 84 insertions(+), 28 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da91..61dc045f3 100755 --- a/git-p4.py +++ b/git-p4.py @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): cmd += ["%s...@%s" % (p, revisionRange)] # Insert changes in chronological order - for line in reversed(p4_read_pipe_lines(cmd)): - changes.add(int(line.split(" ")[1])) + for entry in reversed(p4CmdList(cmd)): + if entry.has_key('p4ExitCode'): + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) + if not entry.has_key('change'): + continue + changes.add(int(entry['change'])) if not block_size: break @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap): [upstream, settings] = findUpstreamBranchPoint() - template = "" + template = """\ +# A Perforce Change Specification. +# +# Change: The change number. 'new' on a new changelist. +# Date: The date this specification was last modified. +# Client: The client on which the changelist was created. Read-only. +# User: The user who created the changelist. +# Status: Either 'pending' or 'submitted'. Read-only. +# Type: Either 'public' or 'restricted'. Default is 'public'. +# Description: Comments about the changelist. Required. +# Jobs: What opened jobs are to be closed by this changelist. +# You may delete jobs from this list. (New changelists only.) +# Files: What opened files from the default changelist are to be added +# to this changelist. You may delete files from this list. +# (New changelists only.) +""" + files_list = [] inFilesSection = False + change_entry = None args = ['change', '-o'] if changelist: args.append(str(changelist)) - - for line in p4_read_pipe_lines(args): - if line.endswith("\r\n"): - line = line[:-2] + "\n" - if inFilesSection: - if line.startswith("\t"): - # path starts and ends with a tab - path = line[1:] - lastTab = path.rfind("\t") - if lastTab != -1: - path = path[:lastTab] - if settings.has_key('depot-paths'): - if not [p for p in settings['depot-paths'] - if p4PathStartsWith(path, p)]: - continue - else: - if not p4PathStartsWith(path, self.depotPath): - continue + for entry in p4CmdList(args): + if not entry.has_key('code'): + continue + if entry['code'] == 'stat': + change_entry = entry + break + if not change_entry: + die('Failed to decode output of p4 change -o') + for key, value in change_entry.iteritems(): + if key.startswith('File'): + if settings.has_key('depot-paths'): + if not [p for p in settings['depot-paths'] + if p4PathStartsWith(value, p)]: + continue else: - inFilesSection = False - else: - if line.startswith("Files:"): - inFilesSection = True - - template += line - + if not p4PathStartsWith(value, self.depotPath): + continue + files_list.append(value) + continue + # Output in the order expected by prepareLogMessage + for key in ['Change','Client','User','Status','Description','Jobs']: + if not change_entry.has_key(key): + continue + template += '\n' + template += key + ':' + if key == 'Description': + template += '\n' + for field_line in change_entry[key].splitlines(): + template += '\t'+field_line+'\n' + if len(files_list) > 0: + template += '\n' + template += 'Files:\n' + for path in files_list: + template += '\t'+path+'\n' return template def edit_template(self, template_file): diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index 3457d5db6..4962b6862 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' ' ) ' +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 + ) && + ( + cd "$git" && + git config git-p4.skipSubmitEdit true && + echo file20 >file20 && + git add file20 && + git commit -m file20 && + git p4 submit + ) && + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) && + ( + cd "$cli" && + test_path_is_file file20 + ) +' + test_expect_success 'submit --prepare-p4-only' ' test_when_finished cleanup_git && git p4 clone --dest="$git" //depot && -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-29 22:46 ` Miguel Torroja @ 2017-06-30 8:26 ` Lars Schneider 2017-06-30 9:41 ` Miguel Torroja 0 siblings, 1 reply; 31+ messages in thread From: Lars Schneider @ 2017-06-30 8:26 UTC (permalink / raw) To: miguel torroja; +Cc: Luke Diamand, git, Junio C Hamano > 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> > --- > git-p4.py | 85 ++++++++++++++++++++++++++++++++---------------- > t/t9807-git-p4-submit.sh | 27 +++++++++++++++ > 2 files changed, 84 insertions(+), 28 deletions(-) > > diff --git a/git-p4.py b/git-p4.py > index 8d151da91..61dc045f3 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): > cmd += ["%s...@%s" % (p, revisionRange)] > > # Insert changes in chronological order > - for line in reversed(p4_read_pipe_lines(cmd)): > - changes.add(int(line.split(" ")[1])) > + for entry in reversed(p4CmdList(cmd)): > + if entry.has_key('p4ExitCode'): > + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) > + if not entry.has_key('change'): > + continue > + changes.add(int(entry['change'])) > > if not block_size: > break > @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap): > > [upstream, settings] = findUpstreamBranchPoint() > > - template = "" > + template = """\ > +# A Perforce Change Specification. > +# > +# Change: The change number. 'new' on a new changelist. > +# Date: The date this specification was last modified. > +# Client: The client on which the changelist was created. Read-only. > +# User: The user who created the changelist. > +# Status: Either 'pending' or 'submitted'. Read-only. > +# Type: Either 'public' or 'restricted'. Default is 'public'. > +# Description: Comments about the changelist. Required. > +# Jobs: What opened jobs are to be closed by this changelist. > +# You may delete jobs from this list. (New changelists only.) > +# Files: What opened files from the default changelist are to be added > +# to this changelist. You may delete files from this list. > +# (New changelists only.) > +""" > + files_list = [] > inFilesSection = False > + change_entry = None > args = ['change', '-o'] > if changelist: > args.append(str(changelist)) > - > - for line in p4_read_pipe_lines(args): > - if line.endswith("\r\n"): > - line = line[:-2] + "\n" > - if inFilesSection: > - if line.startswith("\t"): > - # path starts and ends with a tab > - path = line[1:] > - lastTab = path.rfind("\t") > - if lastTab != -1: > - path = path[:lastTab] > - if settings.has_key('depot-paths'): > - if not [p for p in settings['depot-paths'] > - if p4PathStartsWith(path, p)]: > - continue > - else: > - if not p4PathStartsWith(path, self.depotPath): > - continue > + for entry in p4CmdList(args): > + if not entry.has_key('code'): > + continue > + if entry['code'] == 'stat': > + change_entry = entry > + break > + if not change_entry: > + die('Failed to decode output of p4 change -o') > + for key, value in change_entry.iteritems(): > + if key.startswith('File'): > + if settings.has_key('depot-paths'): > + if not [p for p in settings['depot-paths'] > + if p4PathStartsWith(value, p)]: > + continue > else: > - inFilesSection = False > - else: > - if line.startswith("Files:"): > - inFilesSection = True > - > - template += line > - > + if not p4PathStartsWith(value, self.depotPath): > + continue > + files_list.append(value) > + continue > + # Output in the order expected by prepareLogMessage > + for key in ['Change','Client','User','Status','Description','Jobs']: > + if not change_entry.has_key(key): > + continue > + template += '\n' > + template += key + ':' > + if key == 'Description': > + template += '\n' > + for field_line in change_entry[key].splitlines(): > + template += '\t'+field_line+'\n' > + if len(files_list) > 0: > + template += '\n' > + template += 'Files:\n' > + for path in files_list: > + template += '\t'+path+'\n' > return template > > def edit_template(self, template_file): > diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh > index 3457d5db6..4962b6862 100755 > --- a/t/t9807-git-p4-submit.sh > +++ b/t/t9807-git-p4-submit.sh > @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' ' > ) > ' > 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? > + ( > + 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? Wouldn't we need to check that no warning/error is in the real change description? Thanks, Lars ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-30 8:26 ` Lars Schneider @ 2017-06-30 9:41 ` Miguel Torroja 2017-06-30 10:13 ` Lars Schneider 0 siblings, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-06-30 9:41 UTC (permalink / raw) To: Lars Schneider; +Cc: Luke Diamand, Git Users, Junio C Hamano 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> >> --- >> git-p4.py | 85 ++++++++++++++++++++++++++++++++---------------- >> t/t9807-git-p4-submit.sh | 27 +++++++++++++++ >> 2 files changed, 84 insertions(+), 28 deletions(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index 8d151da91..61dc045f3 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): >> cmd += ["%s...@%s" % (p, revisionRange)] >> >> # Insert changes in chronological order >> - for line in reversed(p4_read_pipe_lines(cmd)): >> - changes.add(int(line.split(" ")[1])) >> + for entry in reversed(p4CmdList(cmd)): >> + if entry.has_key('p4ExitCode'): >> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) >> + if not entry.has_key('change'): >> + continue >> + changes.add(int(entry['change'])) >> >> if not block_size: >> break >> @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap): >> >> [upstream, settings] = findUpstreamBranchPoint() >> >> - template = "" >> + template = """\ >> +# A Perforce Change Specification. >> +# >> +# Change: The change number. 'new' on a new changelist. >> +# Date: The date this specification was last modified. >> +# Client: The client on which the changelist was created. Read-only. >> +# User: The user who created the changelist. >> +# Status: Either 'pending' or 'submitted'. Read-only. >> +# Type: Either 'public' or 'restricted'. Default is 'public'. >> +# Description: Comments about the changelist. Required. >> +# Jobs: What opened jobs are to be closed by this changelist. >> +# You may delete jobs from this list. (New changelists only.) >> +# Files: What opened files from the default changelist are to be added >> +# to this changelist. You may delete files from this list. >> +# (New changelists only.) >> +""" >> + files_list = [] >> inFilesSection = False >> + change_entry = None >> args = ['change', '-o'] >> if changelist: >> args.append(str(changelist)) >> - >> - for line in p4_read_pipe_lines(args): >> - if line.endswith("\r\n"): >> - line = line[:-2] + "\n" >> - if inFilesSection: >> - if line.startswith("\t"): >> - # path starts and ends with a tab >> - path = line[1:] >> - lastTab = path.rfind("\t") >> - if lastTab != -1: >> - path = path[:lastTab] >> - if settings.has_key('depot-paths'): >> - if not [p for p in settings['depot-paths'] >> - if p4PathStartsWith(path, p)]: >> - continue >> - else: >> - if not p4PathStartsWith(path, self.depotPath): >> - continue >> + for entry in p4CmdList(args): >> + if not entry.has_key('code'): >> + continue >> + if entry['code'] == 'stat': >> + change_entry = entry >> + break >> + if not change_entry: >> + die('Failed to decode output of p4 change -o') >> + for key, value in change_entry.iteritems(): >> + if key.startswith('File'): >> + if settings.has_key('depot-paths'): >> + if not [p for p in settings['depot-paths'] >> + if p4PathStartsWith(value, p)]: >> + continue >> else: >> - inFilesSection = False >> - else: >> - if line.startswith("Files:"): >> - inFilesSection = True >> - >> - template += line >> - >> + if not p4PathStartsWith(value, self.depotPath): >> + continue >> + files_list.append(value) >> + continue >> + # Output in the order expected by prepareLogMessage >> + for key in ['Change','Client','User','Status','Description','Jobs']: >> + if not change_entry.has_key(key): >> + continue >> + template += '\n' >> + template += key + ':' >> + if key == 'Description': >> + template += '\n' >> + for field_line in change_entry[key].splitlines(): >> + template += '\t'+field_line+'\n' >> + if len(files_list) > 0: >> + template += '\n' >> + template += 'Files:\n' >> + for path in files_list: >> + template += '\t'+path+'\n' >> return template >> >> def edit_template(self, template_file): >> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh >> index 3457d5db6..4962b6862 100755 >> --- a/t/t9807-git-p4-submit.sh >> +++ b/t/t9807-git-p4-submit.sh >> @@ -409,6 +409,33 @@ test_expect_success 'description with Jobs section and bogus following text' ' >> ) >> ' >> > > 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 > 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? > Thanks, > Lars ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-30 9:41 ` Miguel Torroja @ 2017-06-30 10:13 ` Lars Schneider 2017-06-30 16:02 ` Miguel Torroja 2017-07-03 22:57 ` Miguel Torroja 0 siblings, 2 replies; 31+ messages in thread From: Lars Schneider @ 2017-06-30 10:13 UTC (permalink / raw) To: Miguel Torroja; +Cc: Luke Diamand, Git Users, Junio C Hamano > 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? - Lars ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-30 10:13 ` Lars Schneider @ 2017-06-30 16:02 ` Miguel Torroja 2017-07-03 22:53 ` Miguel Torroja 2017-07-03 22:57 ` Miguel Torroja 1 sibling, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-06-30 16:02 UTC (permalink / raw) To: Lars Schneider; +Cc: Luke Diamand, Git Users, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-30 16:02 ` Miguel Torroja @ 2017-07-03 22:53 ` Miguel Torroja 0 siblings, 0 replies; 31+ messages in thread From: Miguel Torroja @ 2017-07-03 22:53 UTC (permalink / raw) To: Lars Schneider; +Cc: Luke Diamand, Git Users, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-06-30 10:13 ` Lars Schneider 2017-06-30 16:02 ` Miguel Torroja @ 2017-07-03 22:57 ` Miguel Torroja 2017-07-11 8:35 ` Luke Diamand 1 sibling, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-07-03 22:57 UTC (permalink / raw) To: Lars Schneider, Git Users; +Cc: Luke Diamand, Junio C Hamano, Miguel Torroja 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'} the function p4CmdList accepts a new argument: skip_info. When set to True it ignores any 'code':'info' entry (skip_info=True by default). 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> --- git-p4.py | 90 ++++++++++++++++++++++++++++++++---------------- t/t9807-git-p4-submit.sh | 30 ++++++++++++++++ 2 files changed, 91 insertions(+), 29 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da91..a262e3253 100755 --- a/git-p4.py +++ b/git-p4.py @@ -509,7 +509,7 @@ def isModeExec(mode): def isModeExecChanged(src_mode, dst_mode): return isModeExec(src_mode) != isModeExec(dst_mode) -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True): if isinstance(cmd,basestring): cmd = "-G " + cmd @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): try: while True: entry = marshal.load(p4.stdout) + if skip_info: + if 'code' in entry and entry['code'] == 'info': + continue if cb is not None: cb(entry) else: @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): cmd += ["%s...@%s" % (p, revisionRange)] # Insert changes in chronological order - for line in reversed(p4_read_pipe_lines(cmd)): - changes.add(int(line.split(" ")[1])) + for entry in reversed(p4CmdList(cmd)): + if entry.has_key('p4ExitCode'): + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) + if not entry.has_key('change'): + continue + changes.add(int(entry['change'])) if not block_size: break @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap): [upstream, settings] = findUpstreamBranchPoint() - template = "" + template = """\ +# A Perforce Change Specification. +# +# Change: The change number. 'new' on a new changelist. +# Date: The date this specification was last modified. +# Client: The client on which the changelist was created. Read-only. +# User: The user who created the changelist. +# Status: Either 'pending' or 'submitted'. Read-only. +# Type: Either 'public' or 'restricted'. Default is 'public'. +# Description: Comments about the changelist. Required. +# Jobs: What opened jobs are to be closed by this changelist. +# You may delete jobs from this list. (New changelists only.) +# Files: What opened files from the default changelist are to be added +# to this changelist. You may delete files from this list. +# (New changelists only.) +""" + files_list = [] inFilesSection = False + change_entry = None args = ['change', '-o'] if changelist: args.append(str(changelist)) - - for line in p4_read_pipe_lines(args): - if line.endswith("\r\n"): - line = line[:-2] + "\n" - if inFilesSection: - if line.startswith("\t"): - # path starts and ends with a tab - path = line[1:] - lastTab = path.rfind("\t") - if lastTab != -1: - path = path[:lastTab] - if settings.has_key('depot-paths'): - if not [p for p in settings['depot-paths'] - if p4PathStartsWith(path, p)]: - continue - else: - if not p4PathStartsWith(path, self.depotPath): - continue + for entry in p4CmdList(args): + if not entry.has_key('code'): + continue + if entry['code'] == 'stat': + change_entry = entry + break + if not change_entry: + die('Failed to decode output of p4 change -o') + for key, value in change_entry.iteritems(): + if key.startswith('File'): + if settings.has_key('depot-paths'): + if not [p for p in settings['depot-paths'] + if p4PathStartsWith(value, p)]: + continue else: - inFilesSection = False - else: - if line.startswith("Files:"): - inFilesSection = True - - template += line - + if not p4PathStartsWith(value, self.depotPath): + continue + files_list.append(value) + continue + # Output in the order expected by prepareLogMessage + for key in ['Change','Client','User','Status','Description','Jobs']: + if not change_entry.has_key(key): + continue + template += '\n' + template += key + ':' + if key == 'Description': + template += '\n' + for field_line in change_entry[key].splitlines(): + template += '\t'+field_line+'\n' + if len(files_list) > 0: + template += '\n' + template += 'Files:\n' + for path in files_list: + template += '\t'+path+'\n' return template def edit_template(self, template_file): diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index 3457d5db6..b630895a7 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' ' ) ' +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 + ) && + ( + p4 change -o | grep -s "verbose trigger" + ) && + ( + cd "$git" && + git config git-p4.skipSubmitEdit true && + echo file20 >file20 && + git add file20 && + git commit -m file20 && + git p4 submit + ) && + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) && + ( + cd "$cli" && + test_path_is_file file20 + ) +' + test_expect_success 'submit --prepare-p4-only' ' test_when_finished cleanup_git && git p4 clone --dest="$git" //depot && -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-03 22:57 ` Miguel Torroja @ 2017-07-11 8:35 ` Luke Diamand 2017-07-11 22:35 ` Miguel Torroja 2017-07-11 22:53 ` Miguel Torroja 0 siblings, 2 replies; 31+ messages in thread From: Luke Diamand @ 2017-07-11 8:35 UTC (permalink / raw) To: Miguel Torroja; +Cc: Lars Schneider, Git Users, Junio C Hamano On 3 July 2017 at 23:57, 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'} > > the function p4CmdList accepts a new argument: skip_info. When set to > True it ignores any 'code':'info' entry (skip_info=True by default). > > 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" The latest version of mt/p4-parse-G-output (09521c7a0) seems to break t9813-git-p4-preserve-users.sh. I don't quite know why, but I wonder if it's the change to p4CmdList() ? Luke > > Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> > --- > git-p4.py | 90 ++++++++++++++++++++++++++++++++---------------- > t/t9807-git-p4-submit.sh | 30 ++++++++++++++++ > 2 files changed, 91 insertions(+), 29 deletions(-) > > diff --git a/git-p4.py b/git-p4.py > index 8d151da91..a262e3253 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -509,7 +509,7 @@ def isModeExec(mode): > def isModeExecChanged(src_mode, dst_mode): > return isModeExec(src_mode) != isModeExec(dst_mode) > > -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): > +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True): > > if isinstance(cmd,basestring): > cmd = "-G " + cmd > @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): > try: > while True: > entry = marshal.load(p4.stdout) > + if skip_info: > + if 'code' in entry and entry['code'] == 'info': > + continue > if cb is not None: > cb(entry) > else: > @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): > cmd += ["%s...@%s" % (p, revisionRange)] > > # Insert changes in chronological order > - for line in reversed(p4_read_pipe_lines(cmd)): > - changes.add(int(line.split(" ")[1])) > + for entry in reversed(p4CmdList(cmd)): > + if entry.has_key('p4ExitCode'): > + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) > + if not entry.has_key('change'): > + continue > + changes.add(int(entry['change'])) > > if not block_size: > break > @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap): > > [upstream, settings] = findUpstreamBranchPoint() > > - template = "" > + template = """\ > +# A Perforce Change Specification. > +# > +# Change: The change number. 'new' on a new changelist. > +# Date: The date this specification was last modified. > +# Client: The client on which the changelist was created. Read-only. > +# User: The user who created the changelist. > +# Status: Either 'pending' or 'submitted'. Read-only. > +# Type: Either 'public' or 'restricted'. Default is 'public'. > +# Description: Comments about the changelist. Required. > +# Jobs: What opened jobs are to be closed by this changelist. > +# You may delete jobs from this list. (New changelists only.) > +# Files: What opened files from the default changelist are to be added > +# to this changelist. You may delete files from this list. > +# (New changelists only.) > +""" > + files_list = [] > inFilesSection = False > + change_entry = None > args = ['change', '-o'] > if changelist: > args.append(str(changelist)) > - > - for line in p4_read_pipe_lines(args): > - if line.endswith("\r\n"): > - line = line[:-2] + "\n" > - if inFilesSection: > - if line.startswith("\t"): > - # path starts and ends with a tab > - path = line[1:] > - lastTab = path.rfind("\t") > - if lastTab != -1: > - path = path[:lastTab] > - if settings.has_key('depot-paths'): > - if not [p for p in settings['depot-paths'] > - if p4PathStartsWith(path, p)]: > - continue > - else: > - if not p4PathStartsWith(path, self.depotPath): > - continue > + for entry in p4CmdList(args): > + if not entry.has_key('code'): > + continue > + if entry['code'] == 'stat': > + change_entry = entry > + break > + if not change_entry: > + die('Failed to decode output of p4 change -o') > + for key, value in change_entry.iteritems(): > + if key.startswith('File'): > + if settings.has_key('depot-paths'): > + if not [p for p in settings['depot-paths'] > + if p4PathStartsWith(value, p)]: > + continue > else: > - inFilesSection = False > - else: > - if line.startswith("Files:"): > - inFilesSection = True > - > - template += line > - > + if not p4PathStartsWith(value, self.depotPath): > + continue > + files_list.append(value) > + continue > + # Output in the order expected by prepareLogMessage > + for key in ['Change','Client','User','Status','Description','Jobs']: > + if not change_entry.has_key(key): > + continue > + template += '\n' > + template += key + ':' > + if key == 'Description': > + template += '\n' > + for field_line in change_entry[key].splitlines(): > + template += '\t'+field_line+'\n' > + if len(files_list) > 0: > + template += '\n' > + template += 'Files:\n' > + for path in files_list: > + template += '\t'+path+'\n' > return template > > def edit_template(self, template_file): > diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh > index 3457d5db6..b630895a7 100755 > --- a/t/t9807-git-p4-submit.sh > +++ b/t/t9807-git-p4-submit.sh > @@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' ' > ) > ' > > +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 > + ) && > + ( > + p4 change -o | grep -s "verbose trigger" > + ) && > + ( > + cd "$git" && > + git config git-p4.skipSubmitEdit true && > + echo file20 >file20 && > + git add file20 && > + git commit -m file20 && > + git p4 submit > + ) && > + ( > + p4 triggers -i <<-EOF > + Triggers: > + EOF > + ) && > + ( > + cd "$cli" && > + test_path_is_file file20 > + ) > +' > + > test_expect_success 'submit --prepare-p4-only' ' > test_when_finished cleanup_git && > git p4 clone --dest="$git" //depot && > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-11 8:35 ` Luke Diamand @ 2017-07-11 22:35 ` Miguel Torroja 2017-07-11 22:53 ` Miguel Torroja 1 sibling, 0 replies; 31+ messages in thread From: Miguel Torroja @ 2017-07-11 22:35 UTC (permalink / raw) To: Luke Diamand; +Cc: Lars Schneider, Git Users, Junio C Hamano Hi Luke, My bad as I didn't check that case. It was p4CmdList as you said. the default value of the new field skip_info (set to True) ignores any info messages. and the script is waiting for a valid message. If I set it to False, then it does return an info entry and it accepts the submit change I'm sending another patch update On Tue, Jul 11, 2017 at 10:35 AM, Luke Diamand <luke@diamand.org> wrote: > On 3 July 2017 at 23:57, 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'} >> >> the function p4CmdList accepts a new argument: skip_info. When set to >> True it ignores any 'code':'info' entry (skip_info=True by default). >> >> 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" > > The latest version of mt/p4-parse-G-output (09521c7a0) seems to break > t9813-git-p4-preserve-users.sh. > > I don't quite know why, but I wonder if it's the change to p4CmdList() ? > > Luke > >> >> Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> >> --- >> git-p4.py | 90 ++++++++++++++++++++++++++++++++---------------- >> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++ >> 2 files changed, 91 insertions(+), 29 deletions(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index 8d151da91..a262e3253 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -509,7 +509,7 @@ def isModeExec(mode): >> def isModeExecChanged(src_mode, dst_mode): >> return isModeExec(src_mode) != isModeExec(dst_mode) >> >> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): >> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True): >> >> if isinstance(cmd,basestring): >> cmd = "-G " + cmd >> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): >> try: >> while True: >> entry = marshal.load(p4.stdout) >> + if skip_info: >> + if 'code' in entry and entry['code'] == 'info': >> + continue >> if cb is not None: >> cb(entry) >> else: >> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): >> cmd += ["%s...@%s" % (p, revisionRange)] >> >> # Insert changes in chronological order >> - for line in reversed(p4_read_pipe_lines(cmd)): >> - changes.add(int(line.split(" ")[1])) >> + for entry in reversed(p4CmdList(cmd)): >> + if entry.has_key('p4ExitCode'): >> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) >> + if not entry.has_key('change'): >> + continue >> + changes.add(int(entry['change'])) >> >> if not block_size: >> break >> @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap): >> >> [upstream, settings] = findUpstreamBranchPoint() >> >> - template = "" >> + template = """\ >> +# A Perforce Change Specification. >> +# >> +# Change: The change number. 'new' on a new changelist. >> +# Date: The date this specification was last modified. >> +# Client: The client on which the changelist was created. Read-only. >> +# User: The user who created the changelist. >> +# Status: Either 'pending' or 'submitted'. Read-only. >> +# Type: Either 'public' or 'restricted'. Default is 'public'. >> +# Description: Comments about the changelist. Required. >> +# Jobs: What opened jobs are to be closed by this changelist. >> +# You may delete jobs from this list. (New changelists only.) >> +# Files: What opened files from the default changelist are to be added >> +# to this changelist. You may delete files from this list. >> +# (New changelists only.) >> +""" >> + files_list = [] >> inFilesSection = False >> + change_entry = None >> args = ['change', '-o'] >> if changelist: >> args.append(str(changelist)) >> - >> - for line in p4_read_pipe_lines(args): >> - if line.endswith("\r\n"): >> - line = line[:-2] + "\n" >> - if inFilesSection: >> - if line.startswith("\t"): >> - # path starts and ends with a tab >> - path = line[1:] >> - lastTab = path.rfind("\t") >> - if lastTab != -1: >> - path = path[:lastTab] >> - if settings.has_key('depot-paths'): >> - if not [p for p in settings['depot-paths'] >> - if p4PathStartsWith(path, p)]: >> - continue >> - else: >> - if not p4PathStartsWith(path, self.depotPath): >> - continue >> + for entry in p4CmdList(args): >> + if not entry.has_key('code'): >> + continue >> + if entry['code'] == 'stat': >> + change_entry = entry >> + break >> + if not change_entry: >> + die('Failed to decode output of p4 change -o') >> + for key, value in change_entry.iteritems(): >> + if key.startswith('File'): >> + if settings.has_key('depot-paths'): >> + if not [p for p in settings['depot-paths'] >> + if p4PathStartsWith(value, p)]: >> + continue >> else: >> - inFilesSection = False >> - else: >> - if line.startswith("Files:"): >> - inFilesSection = True >> - >> - template += line >> - >> + if not p4PathStartsWith(value, self.depotPath): >> + continue >> + files_list.append(value) >> + continue >> + # Output in the order expected by prepareLogMessage >> + for key in ['Change','Client','User','Status','Description','Jobs']: >> + if not change_entry.has_key(key): >> + continue >> + template += '\n' >> + template += key + ':' >> + if key == 'Description': >> + template += '\n' >> + for field_line in change_entry[key].splitlines(): >> + template += '\t'+field_line+'\n' >> + if len(files_list) > 0: >> + template += '\n' >> + template += 'Files:\n' >> + for path in files_list: >> + template += '\t'+path+'\n' >> return template >> >> def edit_template(self, template_file): >> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh >> index 3457d5db6..b630895a7 100755 >> --- a/t/t9807-git-p4-submit.sh >> +++ b/t/t9807-git-p4-submit.sh >> @@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' ' >> ) >> ' >> >> +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 >> + ) && >> + ( >> + p4 change -o | grep -s "verbose trigger" >> + ) && >> + ( >> + cd "$git" && >> + git config git-p4.skipSubmitEdit true && >> + echo file20 >file20 && >> + git add file20 && >> + git commit -m file20 && >> + git p4 submit >> + ) && >> + ( >> + p4 triggers -i <<-EOF >> + Triggers: >> + EOF >> + ) && >> + ( >> + cd "$cli" && >> + test_path_is_file file20 >> + ) >> +' >> + >> test_expect_success 'submit --prepare-p4-only' ' >> test_when_finished cleanup_git && >> git p4 clone --dest="$git" //depot && >> -- >> 2.11.0 >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-11 8:35 ` Luke Diamand 2017-07-11 22:35 ` Miguel Torroja @ 2017-07-11 22:53 ` Miguel Torroja 2017-07-12 8:25 ` Luke Diamand 1 sibling, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-07-11 22:53 UTC (permalink / raw) To: Luke Diamand, Git Users; +Cc: Lars Schneider, Junio C Hamano, Miguel Torroja 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'} the function p4CmdList accepts a new argument: skip_info. When set to True it ignores any 'code':'info' entry (skip_info=True by default). A new test has been created in 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> --- git-p4.py | 92 ++++++++++++++++++++++++++++++++---------------- t/t9807-git-p4-submit.sh | 30 ++++++++++++++++ 2 files changed, 92 insertions(+), 30 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da91..1facf32db 100755 --- a/git-p4.py +++ b/git-p4.py @@ -509,7 +509,7 @@ def isModeExec(mode): def isModeExecChanged(src_mode, dst_mode): return isModeExec(src_mode) != isModeExec(dst_mode) -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True): if isinstance(cmd,basestring): cmd = "-G " + cmd @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): try: while True: entry = marshal.load(p4.stdout) + if skip_info: + if 'code' in entry and entry['code'] == 'info': + continue if cb is not None: cb(entry) else: @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): cmd += ["%s...@%s" % (p, revisionRange)] # Insert changes in chronological order - for line in reversed(p4_read_pipe_lines(cmd)): - changes.add(int(line.split(" ")[1])) + for entry in reversed(p4CmdList(cmd)): + if entry.has_key('p4ExitCode'): + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) + if not entry.has_key('change'): + continue + changes.add(int(entry['change'])) if not block_size: break @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap): c['User'] = newUser input = marshal.dumps(c) - result = p4CmdList("change -f -i", stdin=input) + result = p4CmdList("change -f -i", stdin=input,skip_info=False) for r in result: if r.has_key('code'): if r['code'] == 'error': @@ -1526,37 +1533,62 @@ class P4Submit(Command, P4UserMap): [upstream, settings] = findUpstreamBranchPoint() - template = "" + template = """\ +# A Perforce Change Specification. +# +# Change: The change number. 'new' on a new changelist. +# Date: The date this specification was last modified. +# Client: The client on which the changelist was created. Read-only. +# User: The user who created the changelist. +# Status: Either 'pending' or 'submitted'. Read-only. +# Type: Either 'public' or 'restricted'. Default is 'public'. +# Description: Comments about the changelist. Required. +# Jobs: What opened jobs are to be closed by this changelist. +# You may delete jobs from this list. (New changelists only.) +# Files: What opened files from the default changelist are to be added +# to this changelist. You may delete files from this list. +# (New changelists only.) +""" + files_list = [] inFilesSection = False + change_entry = None args = ['change', '-o'] if changelist: args.append(str(changelist)) - - for line in p4_read_pipe_lines(args): - if line.endswith("\r\n"): - line = line[:-2] + "\n" - if inFilesSection: - if line.startswith("\t"): - # path starts and ends with a tab - path = line[1:] - lastTab = path.rfind("\t") - if lastTab != -1: - path = path[:lastTab] - if settings.has_key('depot-paths'): - if not [p for p in settings['depot-paths'] - if p4PathStartsWith(path, p)]: - continue - else: - if not p4PathStartsWith(path, self.depotPath): - continue + for entry in p4CmdList(args): + if not entry.has_key('code'): + continue + if entry['code'] == 'stat': + change_entry = entry + break + if not change_entry: + die('Failed to decode output of p4 change -o') + for key, value in change_entry.iteritems(): + if key.startswith('File'): + if settings.has_key('depot-paths'): + if not [p for p in settings['depot-paths'] + if p4PathStartsWith(value, p)]: + continue else: - inFilesSection = False - else: - if line.startswith("Files:"): - inFilesSection = True - - template += line - + if not p4PathStartsWith(value, self.depotPath): + continue + files_list.append(value) + continue + # Output in the order expected by prepareLogMessage + for key in ['Change','Client','User','Status','Description','Jobs']: + if not change_entry.has_key(key): + continue + template += '\n' + template += key + ':' + if key == 'Description': + template += '\n' + for field_line in change_entry[key].splitlines(): + template += '\t'+field_line+'\n' + if len(files_list) > 0: + template += '\n' + template += 'Files:\n' + for path in files_list: + template += '\t'+path+'\n' return template def edit_template(self, template_file): diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index 3457d5db6..b630895a7 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -409,6 +409,36 @@ test_expect_success 'description with Jobs section and bogus following text' ' ) ' +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 + ) && + ( + p4 change -o | grep -s "verbose trigger" + ) && + ( + cd "$git" && + git config git-p4.skipSubmitEdit true && + echo file20 >file20 && + git add file20 && + git commit -m file20 && + git p4 submit + ) && + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) && + ( + cd "$cli" && + test_path_is_file file20 + ) +' + test_expect_success 'submit --prepare-p4-only' ' test_when_finished cleanup_git && git p4 clone --dest="$git" //depot && -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-11 22:53 ` Miguel Torroja @ 2017-07-12 8:25 ` Luke Diamand 2017-07-12 10:16 ` Miguel Torroja 0 siblings, 1 reply; 31+ messages in thread From: Luke Diamand @ 2017-07-12 8:25 UTC (permalink / raw) To: Miguel Torroja; +Cc: Git Users, Lars Schneider, Junio C Hamano On 11 July 2017 at 23:53, 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'} > > the function p4CmdList accepts a new argument: skip_info. When set to > True it ignores any 'code':'info' entry (skip_info=True by default). > > A new test has been created in 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> > --- > git-p4.py | 92 ++++++++++++++++++++++++++++++++---------------- > t/t9807-git-p4-submit.sh | 30 ++++++++++++++++ > 2 files changed, 92 insertions(+), 30 deletions(-) > > diff --git a/git-p4.py b/git-p4.py > index 8d151da91..1facf32db 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -509,7 +509,7 @@ def isModeExec(mode): > def isModeExecChanged(src_mode, dst_mode): > return isModeExec(src_mode) != isModeExec(dst_mode) > > -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): > +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True): > > if isinstance(cmd,basestring): > cmd = "-G " + cmd > @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): > try: > while True: > entry = marshal.load(p4.stdout) > + if skip_info: > + if 'code' in entry and entry['code'] == 'info': > + continue > if cb is not None: > cb(entry) > else: > @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): > cmd += ["%s...@%s" % (p, revisionRange)] > > # Insert changes in chronological order > - for line in reversed(p4_read_pipe_lines(cmd)): > - changes.add(int(line.split(" ")[1])) > + for entry in reversed(p4CmdList(cmd)): > + if entry.has_key('p4ExitCode'): > + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) > + if not entry.has_key('change'): > + continue > + changes.add(int(entry['change'])) > > if not block_size: > break > @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap): > c['User'] = newUser > input = marshal.dumps(c) > > - result = p4CmdList("change -f -i", stdin=input) > + result = p4CmdList("change -f -i", stdin=input,skip_info=False) Is there any reason this change sets skip_info to False in this one place, rather than defaulting to False (the original behavior) and setting it to True where it's needed? I worry that there might be other unexpected side effects in places not covered by the tests. Thanks Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-12 8:25 ` Luke Diamand @ 2017-07-12 10:16 ` Miguel Torroja 2017-07-12 17:13 ` Junio C Hamano 0 siblings, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-07-12 10:16 UTC (permalink / raw) To: Luke Diamand; +Cc: Git Users, Lars Schneider, Junio C Hamano 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 Thanks, On Wed, Jul 12, 2017 at 10:25 AM, Luke Diamand <luke@diamand.org> wrote: > On 11 July 2017 at 23:53, 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'} >> >> the function p4CmdList accepts a new argument: skip_info. When set to >> True it ignores any 'code':'info' entry (skip_info=True by default). >> >> A new test has been created in 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> >> --- >> git-p4.py | 92 ++++++++++++++++++++++++++++++++---------------- >> t/t9807-git-p4-submit.sh | 30 ++++++++++++++++ >> 2 files changed, 92 insertions(+), 30 deletions(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index 8d151da91..1facf32db 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -509,7 +509,7 @@ def isModeExec(mode): >> def isModeExecChanged(src_mode, dst_mode): >> return isModeExec(src_mode) != isModeExec(dst_mode) >> >> -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): >> +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=True): >> >> if isinstance(cmd,basestring): >> cmd = "-G " + cmd >> @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): >> try: >> while True: >> entry = marshal.load(p4.stdout) >> + if skip_info: >> + if 'code' in entry and entry['code'] == 'info': >> + continue >> if cb is not None: >> cb(entry) >> else: >> @@ -879,8 +882,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): >> cmd += ["%s...@%s" % (p, revisionRange)] >> >> # Insert changes in chronological order >> - for line in reversed(p4_read_pipe_lines(cmd)): >> - changes.add(int(line.split(" ")[1])) >> + for entry in reversed(p4CmdList(cmd)): >> + if entry.has_key('p4ExitCode'): >> + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) >> + if not entry.has_key('change'): >> + continue >> + changes.add(int(entry['change'])) >> >> if not block_size: >> break >> @@ -1494,7 +1501,7 @@ class P4Submit(Command, P4UserMap): >> c['User'] = newUser >> input = marshal.dumps(c) >> >> - result = p4CmdList("change -f -i", stdin=input) >> + result = p4CmdList("change -f -i", stdin=input,skip_info=False) > > Is there any reason this change sets skip_info to False in this one > place, rather than defaulting to False (the original behavior) and > setting it to True where it's needed? > > I worry that there might be other unexpected side effects in places > not covered by the tests. > > Thanks > Luke ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-12 10:16 ` Miguel Torroja @ 2017-07-12 17:13 ` Junio C Hamano 2017-07-13 7:00 ` [PATCH 1/3] git-p4: git-p4 tests with p4 triggers Miguel Torroja 2017-07-13 7:12 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 0 siblings, 2 replies; 31+ messages in thread From: Junio C Hamano @ 2017-07-12 17:13 UTC (permalink / raw) To: Miguel Torroja; +Cc: Luke Diamand, Git Users, Lars Schneider 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' ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 1/3] git-p4: git-p4 tests with p4 triggers 2017-07-12 17:13 ` Junio C Hamano @ 2017-07-13 7:00 ` Miguel Torroja 2017-07-13 7:00 ` [PATCH 2/3] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 2017-07-13 7:00 ` [PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList Miguel Torroja 2017-07-13 7:12 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 1 sibling, 2 replies; 31+ messages in thread From: Miguel Torroja @ 2017-07-13 7:00 UTC (permalink / raw) To: Luke Diamand, Git Users, Junio C Hamano; +Cc: Lars Schneider, Miguel Torroja Some p4 triggers in the server side generate some warnings when executed. Unfortunately those messages are mixed with the output of p4 commands. A few git-p4 commands don't expect extra messages or output lines and may fail with verbose triggers. New tests added are known to be broken. Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> --- t/t9831-git-p4-triggers.sh | 103 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100755 t/t9831-git-p4-triggers.sh diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh new file mode 100755 index 000000000..28cafe469 --- /dev/null +++ b/t/t9831-git-p4-triggers.sh @@ -0,0 +1,103 @@ +#!/bin/sh + +test_description='git p4 with server triggers' + +. ./lib-git-p4.sh + +test_expect_success 'start p4d' ' + start_p4d +' + +test_expect_success 'init depot' ' + ( + cd "$cli" && + echo file1 >file1 && + p4 add file1 && + p4 submit -d "change 1" + echo file2 >file2 && + p4 add file2 && + p4 submit -d "change 2" + ) +' + +test_expect_failure 'clone with extra info lines from verbose p4 trigger' ' + test_when_finished cleanup_git && + ( + p4 triggers -i <<-EOF + Triggers: p4triggertest-command command pre-user-change "echo verbose trigger" + EOF + ) && + ( + p4 change -o | grep -s "verbose trigger" + ) && + git p4 clone --dest="$git" //depot/@all && + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) +' + +test_expect_failure 'import with extra info lines from verbose p4 trigger' ' + test_when_finished cleanup_git && + ( + cd "$cli" && + echo file3 >file3 && + p4 add file3 && + p4 submit -d "change 3" + ) && + ( + p4 triggers -i <<-EOF + Triggers: p4triggertest-command command pre-user-describe "echo verbose trigger" + EOF + ) && + ( + p4 describe 1 | grep -s "verbose trigger" + ) && + git p4 clone --dest="$git" //depot/@all && + ( + cd "$git" && + git p4 sync + )&& + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) +' + +test_expect_failure 'submit description with extra info lines from verbose p4 change trigger' ' + test_when_finished cleanup_git && + ( + p4 triggers -i <<-EOF + Triggers: p4triggertest-command command pre-user-change "echo verbose trigger" + EOF + ) && + ( + p4 change -o | grep -s "verbose trigger" + ) && + git p4 clone --dest="$git" //depot && + ( + cd "$git" && + git config git-p4.skipSubmitEdit true && + echo file4 >file4 && + git add file4 && + git commit -m file4 && + git p4 submit + ) && + ( + p4 triggers -i <<-EOF + Triggers: + EOF + ) && + ( + cd "$cli" && + test_path_is_file file4 + ) +' + +test_expect_success 'kill p4d' ' + kill_p4d +' + +test_done -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/3] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-13 7:00 ` [PATCH 1/3] git-p4: git-p4 tests with p4 triggers Miguel Torroja @ 2017-07-13 7:00 ` Miguel Torroja 2017-07-13 7:00 ` [PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList Miguel Torroja 1 sibling, 0 replies; 31+ messages in thread From: Miguel Torroja @ 2017-07-13 7:00 UTC (permalink / raw) To: Luke Diamand, Git Users, Junio C Hamano; +Cc: Lars Schneider, Miguel Torroja 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. This fixes the case where a p4 trigger for "p4 change" is set and the command git-p4 submit is run. Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> --- git-p4.py | 85 +++++++++++++++++++++++++++++++--------------- t/t9831-git-p4-triggers.sh | 2 +- 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da91..e3a2791e0 100755 --- a/git-p4.py +++ b/git-p4.py @@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): cmd += ["%s...@%s" % (p, revisionRange)] # Insert changes in chronological order - for line in reversed(p4_read_pipe_lines(cmd)): - changes.add(int(line.split(" ")[1])) + for entry in reversed(p4CmdList(cmd)): + if entry.has_key('p4ExitCode'): + die('Error retrieving changes descriptions ({})'.format(entry['p4ExitCode'])) + if not entry.has_key('change'): + continue + changes.add(int(entry['change'])) if not block_size: break @@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap): [upstream, settings] = findUpstreamBranchPoint() - template = "" + template = """\ +# A Perforce Change Specification. +# +# Change: The change number. 'new' on a new changelist. +# Date: The date this specification was last modified. +# Client: The client on which the changelist was created. Read-only. +# User: The user who created the changelist. +# Status: Either 'pending' or 'submitted'. Read-only. +# Type: Either 'public' or 'restricted'. Default is 'public'. +# Description: Comments about the changelist. Required. +# Jobs: What opened jobs are to be closed by this changelist. +# You may delete jobs from this list. (New changelists only.) +# Files: What opened files from the default changelist are to be added +# to this changelist. You may delete files from this list. +# (New changelists only.) +""" + files_list = [] inFilesSection = False + change_entry = None args = ['change', '-o'] if changelist: args.append(str(changelist)) - - for line in p4_read_pipe_lines(args): - if line.endswith("\r\n"): - line = line[:-2] + "\n" - if inFilesSection: - if line.startswith("\t"): - # path starts and ends with a tab - path = line[1:] - lastTab = path.rfind("\t") - if lastTab != -1: - path = path[:lastTab] - if settings.has_key('depot-paths'): - if not [p for p in settings['depot-paths'] - if p4PathStartsWith(path, p)]: - continue - else: - if not p4PathStartsWith(path, self.depotPath): - continue + for entry in p4CmdList(args): + if not entry.has_key('code'): + continue + if entry['code'] == 'stat': + change_entry = entry + break + if not change_entry: + die('Failed to decode output of p4 change -o') + for key, value in change_entry.iteritems(): + if key.startswith('File'): + if settings.has_key('depot-paths'): + if not [p for p in settings['depot-paths'] + if p4PathStartsWith(value, p)]: + continue else: - inFilesSection = False - else: - if line.startswith("Files:"): - inFilesSection = True - - template += line - + if not p4PathStartsWith(value, self.depotPath): + continue + files_list.append(value) + continue + # Output in the order expected by prepareLogMessage + for key in ['Change', 'Client', 'User', 'Status', 'Description', 'Jobs']: + if not change_entry.has_key(key): + continue + template += '\n' + template += key + ':' + if key == 'Description': + template += '\n' + for field_line in change_entry[key].splitlines(): + template += '\t'+field_line+'\n' + if len(files_list) > 0: + template += '\n' + template += 'Files:\n' + for path in files_list: + template += '\t'+path+'\n' return template def edit_template(self, template_file): diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh index 28cafe469..871544b1c 100755 --- a/t/t9831-git-p4-triggers.sh +++ b/t/t9831-git-p4-triggers.sh @@ -66,7 +66,7 @@ test_expect_failure 'import with extra info lines from verbose p4 trigger' ' ) ' -test_expect_failure 'submit description with extra info lines from verbose p4 change trigger' ' +test_expect_success 'submit description with extra info lines from verbose p4 change trigger' ' test_when_finished cleanup_git && ( p4 triggers -i <<-EOF -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList 2017-07-13 7:00 ` [PATCH 1/3] git-p4: git-p4 tests with p4 triggers Miguel Torroja 2017-07-13 7:00 ` [PATCH 2/3] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja @ 2017-07-13 7:00 ` Miguel Torroja 1 sibling, 0 replies; 31+ messages in thread From: Miguel Torroja @ 2017-07-13 7:00 UTC (permalink / raw) To: Luke Diamand, Git Users, Junio C Hamano; +Cc: Lars Schneider, Miguel Torroja The function p4CmdList accepts a new argument: skip_info. When set to True it ignores any 'code':'info' entry (skip_info=False by default). That allows us to fix some of the tests in t9831-git-p4-triggers.sh known to be broken with verobse p4 triggers Signed-off-by: Miguel Torroja <miguel.torroja@gmail.com> --- git-p4.py | 9 ++++++--- t/t9831-git-p4-triggers.sh | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/git-p4.py b/git-p4.py index e3a2791e0..2fa581789 100755 --- a/git-p4.py +++ b/git-p4.py @@ -313,7 +313,7 @@ def p4_move(src, dest): p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)]) def p4_last_change(): - results = p4CmdList(["changes", "-m", "1"]) + results = p4CmdList(["changes", "-m", "1"], skip_info=True) return int(results[0]['change']) def p4_describe(change): @@ -321,7 +321,7 @@ def p4_describe(change): the presence of field "time". Return a dict of the results.""" - ds = p4CmdList(["describe", "-s", str(change)]) + ds = p4CmdList(["describe", "-s", str(change)], skip_info=True) if len(ds) != 1: die("p4 describe -s %d did not return 1 result: %s" % (change, str(ds))) @@ -509,7 +509,7 @@ def isModeExec(mode): def isModeExecChanged(src_mode, dst_mode): return isModeExec(src_mode) != isModeExec(dst_mode) -def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): +def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False): if isinstance(cmd,basestring): cmd = "-G " + cmd @@ -545,6 +545,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None): try: while True: entry = marshal.load(p4.stdout) + if skip_info: + if 'code' in entry and entry['code'] == 'info': + continue if cb is not None: cb(entry) else: diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh index 871544b1c..bbcf14c66 100755 --- a/t/t9831-git-p4-triggers.sh +++ b/t/t9831-git-p4-triggers.sh @@ -20,7 +20,7 @@ test_expect_success 'init depot' ' ) ' -test_expect_failure 'clone with extra info lines from verbose p4 trigger' ' +test_expect_success 'clone with extra info lines from verbose p4 trigger' ' test_when_finished cleanup_git && ( p4 triggers -i <<-EOF @@ -38,7 +38,7 @@ test_expect_failure 'clone with extra info lines from verbose p4 trigger' ' ) ' -test_expect_failure 'import with extra info lines from verbose p4 trigger' ' +test_expect_success 'import with extra info lines from verbose p4 trigger' ' test_when_finished cleanup_git && ( cd "$cli" && -- 2.11.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-12 17:13 ` Junio C Hamano 2017-07-13 7:00 ` [PATCH 1/3] git-p4: git-p4 tests with p4 triggers Miguel Torroja @ 2017-07-13 7:12 ` Miguel Torroja 2017-07-13 19:30 ` Junio C Hamano 1 sibling, 1 reply; 31+ messages in thread From: Miguel Torroja @ 2017-07-13 7:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Luke Diamand, Git Users, Lars Schneider 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' ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes 2017-07-13 7:12 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja @ 2017-07-13 19:30 ` Junio C Hamano 0 siblings, 0 replies; 31+ messages in thread From: Junio C Hamano @ 2017-07-13 19:30 UTC (permalink / raw) To: Miguel Torroja; +Cc: Luke Diamand, Git Users, Lars Schneider Miguel Torroja <miguel.torroja@gmail.com> writes: > 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. The approach looks reasonable. By having tests that expect failure upfront, the series clearly shows how the code changes in later steps make things better. Thanks. Will replace. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-07-13 19:30 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-20 12:19 [PATCH] git-p4: changelist template with p4 -G change -o Miguel Torroja 2017-06-22 17:32 ` Junio C Hamano 2017-06-24 11:49 ` Luke Diamand 2017-06-24 12:38 ` miguel torroja 2017-06-24 17:36 ` Lars Schneider [not found] ` <CAKYtbVbGekXGAyPd7HeLot_MdZkp7-1Ss-iAi7o8ze2b+sNB6Q@mail.gmail.com> 2017-06-27 9:20 ` miguel torroja 2017-06-27 19:17 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 2017-06-28 4:08 ` Junio C Hamano 2017-06-28 9:54 ` Luke Diamand 2017-06-28 13:14 ` miguel torroja [not found] ` <CAE5ih7-x45MD1H6Ahr5oCVtTjgbBkeP4GbKCGB-Cwk6BSQwTcw@mail.gmail.com> 2017-06-29 22:41 ` miguel torroja 2017-06-30 7:56 ` Luke Diamand 2017-06-30 8:22 ` miguel torroja 2017-06-29 22:46 ` Miguel Torroja 2017-06-30 8:26 ` Lars Schneider 2017-06-30 9:41 ` Miguel Torroja 2017-06-30 10:13 ` Lars Schneider 2017-06-30 16:02 ` Miguel Torroja 2017-07-03 22:53 ` Miguel Torroja 2017-07-03 22:57 ` Miguel Torroja 2017-07-11 8:35 ` Luke Diamand 2017-07-11 22:35 ` Miguel Torroja 2017-07-11 22:53 ` Miguel Torroja 2017-07-12 8:25 ` Luke Diamand 2017-07-12 10:16 ` Miguel Torroja 2017-07-12 17:13 ` Junio C Hamano 2017-07-13 7:00 ` [PATCH 1/3] git-p4: git-p4 tests with p4 triggers Miguel Torroja 2017-07-13 7:00 ` [PATCH 2/3] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 2017-07-13 7:00 ` [PATCH 3/3] git-p4: filter for {'code':'info'} in p4CmdList Miguel Torroja 2017-07-13 7:12 ` [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes Miguel Torroja 2017-07-13 19:30 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.