{"thread":{"id":"53368","subject":"[PATCH] git-p4.py: fix --prepare-p4-only error with multiple commits","startedAt":"2020-05-01T17:09:20Z","lastAt":"2020-05-17T08:32:09Z","messageCount":6,"participants":["Ben Keene via GitGitGadget","Junio C Hamano","Luke Diamand"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396772","messageId":"pull.775.git.git.1588352952840.gitgitgadget@gmail.com","threadId":"53368","inReplyTo":null,"subject":"[PATCH] git-p4.py: fix --prepare-p4-only error with multiple commits","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-01T17:09:12Z","receivedAt":"2020-05-01T17:09:20Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen using git p4 submit with the --prepare-p4-only option, the program\nshould prepare a single p4 changelist and notify the user that more\ncommits are pending and then stop processing.\n\nA bug has been introduced by the p4-changelist hook feature that\ncauses the program to continue to try and process all pending\nchangelists at the same time.\n\nThe function applyCommit should return True when applying the commit\nwas successful and the program should continue. In the case of the\n--prepare-p4-only flag, the function should return False, alerting the\ncaller that the program should not proceed with additional commits.\n\nChange the return value from True to False in the applyCommit function\nwhen git-p4 is executed with --prepare-p4-only flag.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py: fix --prepare-p4-only error with multiple commits\n \n When using git p4 submit with the --prepare-p4-only option, the program\n should prepare a single p4 changelist and notify the user that more\n commits are pending and then stop processing.\n \n A bug has been introduced by the p4-changelist hook feature that causes\n the program to continue to try and process all pending changelists at\n the same time.\n \n The function applyCommit should return True when applying the commit was\n successful and the program should continue. In the case of the\n --prepare-p4-only flag, the function should return False, alerting the\n caller that the program should not proceed with additional commits.\n \n Change the return value from True to False in the applyCommit function\n when git-p4 is executed with --prepare-p4-only flag.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-775%2Fseraphire%2Fseraphire%2Fp4-hook-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-775/seraphire/seraphire/p4-hook-v1\nPull-Request: https://github.com/git/git/pull/775\n\n git-p4.py | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b8b2a1679e7..d9ced1bf552 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1984,7 +1984,7 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n return (diff + newdiff).replace('\\r\\n', '\\n')\n \n def applyCommit(self, id):\n- \"\"\"Apply one commit, return True if it succeeded.\"\"\"\n+ \"\"\"Apply one commit, return True if it should continue processing.\"\"\"\n \n print(\"Applying\", read_pipe([\"git\", \"show\", \"-s\",\n \"--format=format:%h %s\", id]))\n@@ -2222,7 +2222,7 @@ def applyCommit(self, id):\n print(\" \" + f)\n print(\"\")\n sys.stdout.flush()\n- return True\n+ return False\n \n if self.edit_template(fileName):\n if not self.no_verify:\n\nbase-commit: d61d20c9b413225793f8a0b491bbbec61c184e26\n-- \ngitgitgadget\n"},{"id":"396776","messageId":"xmqq1ro3mrd5.fsf@gitster.c.googlers.com","threadId":"53368","inReplyTo":"pull.775.git.git.1588352952840.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4.py: fix --prepare-p4-only error with multiple commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T18:39:50Z","receivedAt":"2020-05-01T18:39:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ben Keene via GitGitGadget\" writes:\n\n> From: Ben Keene \n>\n> When using git p4 submit with the --prepare-p4-only option, the program\n> should prepare a single p4 changelist and notify the user that more\n> commits are pending and then stop processing.\n>\n> A bug has been introduced by the p4-changelist hook feature that\n> causes the program to continue to try and process all pending\n> changelists at the same time.\n>\n> The function applyCommit should return True when applying the commit\n> was successful and the program should continue. In the case of the\n> --prepare-p4-only flag, the function should return False, alerting the\n> caller that the program should not proceed with additional commits.\n>\n> Change the return value from True to False in the applyCommit function\n> when git-p4 is executed with --prepare-p4-only flag.\n>\n> Signed-off-by: Ben Keene \n> ---\n\nI do not do P4, so asking those who touched the program in the past\nyear and half for opinions.\n\nThanks.\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-775%2Fseraphire%2Fseraphire%2Fp4-hook-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-775/seraphire/seraphire/p4-hook-v1\n> Pull-Request: https://github.com/git/git/pull/775\n>\n> git-p4.py | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index b8b2a1679e7..d9ced1bf552 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1984,7 +1984,7 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n> return (diff + newdiff).replace('\\r\\n', '\\n')\n> \n> def applyCommit(self, id):\n> - \"\"\"Apply one commit, return True if it succeeded.\"\"\"\n> + \"\"\"Apply one commit, return True if it should continue processing.\"\"\"\n> \n> print(\"Applying\", read_pipe([\"git\", \"show\", \"-s\",\n> \"--format=format:%h %s\", id]))\n> @@ -2222,7 +2222,7 @@ def applyCommit(self, id):\n> print(\" \" + f)\n> print(\"\")\n> sys.stdout.flush()\n> - return True\n> + return False\n> \n> if self.edit_template(fileName):\n> if not self.no_verify:\n>\n> base-commit: d61d20c9b413225793f8a0b491bbbec61c184e26\n"},{"id":"397163","messageId":"xmqqwo5p918d.fsf@gitster.c.googlers.com","threadId":"53368","inReplyTo":"pull.775.git.git.1588352952840.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4.py: fix --prepare-p4-only error with multiple commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T03:44:02Z","receivedAt":"2020-05-06T03:44:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ben Keene via GitGitGadget\" writes:\n\n> From: Ben Keene \n>\n> When using git p4 submit with the --prepare-p4-only option, the program\n> should prepare a single p4 changelist and notify the user that more\n> commits are pending and then stop processing.\n> ...\n\nWhen merged to 'pu', this seems to break builds/tests at Travis CI.\n\nCompare a good run with this topic deliberately ejected from 'pu':\n\n https://travis-ci.org/github/git/git/builds/683614214\n\nand a bad one with this topic kept in 'pu':\n\n https://travis-ci.org/github/git/git/jobs/683604200\n\n"},{"id":"397598","messageId":"pull.775.v2.git.git.1589289359302.gitgitgadget@gmail.com","threadId":"53368","inReplyTo":"pull.775.git.git.1588352952840.gitgitgadget@gmail.com","subject":"[PATCH v2] git-p4.py: fix --prepare-p4-only error with multiple commits","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-12T13:15:59Z","receivedAt":"2020-05-12T13:16:03Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen using git p4 submit with the --prepare-p4-only option, the program\nshould prepare a single p4 changelist and notify the user that more\ncommits are pending and then stop processing.\n\nA bug has been introduced by the p4-changelist hook feature that\ncauses the program to continue to try and process all pending\nchangelists at the same time.\n\nThe function applyCommit returns True when applying the commit\nwas successful and the program should continue. However, when the\noptional flag --prepare-p4-only is set, the program should stop\nafter the first application.\n\nChange the logic in the run method for P4Submit to check for the\nflag --prepare-p4-only after successfully completing the applyCommit\nmethod.\n\nBe aware - this change will fix the existing test error in t9807.23\nfor --prepare-p4-only. However there is insufficent coverage for\nthis flag. If more than 1 commit is pending submission to P4, the\nmethod will properly prepare the P4 changelist, however it will\nstill exit the application with an exitcode of 1.\n\nThe current documentation does not define what the exit code should be\nin this condition.\n(See: https://git-scm.com/docs/git-p4#Documentation/git-p4.txt---prepare-p4-only)\n\nSigned-off-by: Ben Keene \n---\n git-p4.py: fix --prepare-p4-only error with multiple commits\n \n When using git p4 submit with the --prepare-p4-only option, the program\n should prepare a single p4 changelist and notify the user that more\n commits are pending and then stop processing.\n \n A bug has been introduced by the p4-changelist hook feature that causes\n the program to continue to try and process all pending changelists at\n the same time.\n \n The function applyCommit should return True when applying the commit was\n successful and the program should continue. In the case of the\n --prepare-p4-only flag, the function should return False, alerting the\n caller that the program should not proceed with additional commits.\n \n Change the return value from True to False in the applyCommit function\n when git-p4 is executed with --prepare-p4-only flag.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-775%2Fseraphire%2Fseraphire%2Fp4-hook-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-775/seraphire/seraphire/p4-hook-v2\nPull-Request: https://github.com/git/git/pull/775\n\nRange-diff vs v1:\n\n 1: 57a34af2b86 < -: ----------- git-p4.py: fix --prepare-p4-only error with multiple commits\n -: ----------- > 1: a554a65503e git-p4.py: fix --prepare-p4-only error with multiple commits\n\n\n git-p4.py | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b8b2a1679e7..c4a4012bcc1 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2537,11 +2537,12 @@ def run(self, args):\n ok = self.applyCommit(commit)\n if ok:\n applied.append(commit)\n- else:\n- if self.prepare_p4_only and i < last:\n- print(\"Processing only the first commit due to option\" \\\n- \" --prepare-p4-only\")\n+ if self.prepare_p4_only:\n+ if i < last:\n+ print(\"Processing only the first commit due to option\" \\\n+ \" --prepare-p4-only\")\n break\n+ else:\n if i < last:\n # prompt for what to do, or use the option/variable\n if self.conflict_behavior == \"ask\":\n\nbase-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17\n-- \ngitgitgadget\n"},{"id":"397623","messageId":"xmqq1rnodimp.fsf@gitster.c.googlers.com","threadId":"53368","inReplyTo":"pull.775.v2.git.git.1589289359302.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] git-p4.py: fix --prepare-p4-only error with multiple commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-12T20:05:02Z","receivedAt":"2020-05-12T20:05:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ben Keene via GitGitGadget\" writes:\n\n> ...\n> Be aware - this change will fix the existing test error in t9807.23\n> for --prepare-p4-only. However there is insufficent coverage for\n> this flag. If more than 1 commit is pending submission to P4, the\n> method will properly prepare the P4 changelist, however it will\n> still exit the application with an exitcode of 1.\n>\n> The current documentation does not define what the exit code should be\n> in this condition.\n> (See: https://git-scm.com/docs/git-p4#Documentation/git-p4.txt---prepare-p4-only)\n\nThen some proposal to define what the behaviour should be is needed,\nthe consensus implements and then documented.\n\n>\n> Signed-off-by: Ben Keene \n> ---\n\n\n> git-p4.py: fix --prepare-p4-only error with multiple commits\n> \n> When using git p4 submit with the --prepare-p4-only option, the program\n> should prepare a single p4 changelist and notify the user that more\n> commits are pending and then stop processing.\n> \n> A bug has been introduced by the p4-changelist hook feature that causes\n> the program to continue to try and process all pending changelists at\n> the same time.\n> \n> The function applyCommit should return True when applying the commit was\n> successful and the program should continue. In the case of the\n> --prepare-p4-only flag, the function should return False, alerting the\n> caller that the program should not proceed with additional commits.\n> \n> Change the return value from True to False in the applyCommit function\n> when git-p4 is executed with --prepare-p4-only flag.\n\nThis space below the three-dash-line is the best place to describe\nthe difference between the previous version and this one. It seems\nthat the above text is not such a \"here are what was bad/missing in\nv1 that got fixed/extended\", or a copy of the log message (like many\npatches that come from GGG has). I am a bit puzzled what it is, but\nfor now let's pretend there wasn't any text below the three-dash-line\nand read on.\n\n> git-p4.py | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index b8b2a1679e7..c4a4012bcc1 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2537,11 +2537,12 @@ def run(self, args):\n> ok = self.applyCommit(commit)\n> if ok:\n> applied.append(commit)\n> - else:\n> - if self.prepare_p4_only and i < last:\n> - print(\"Processing only the first commit due to option\" \\\n> - \" --prepare-p4-only\")\n\nSo, it used to be that after failing to apply a commit, unless we\nare at the last step, we gave a message and left the loop under the\nprepare-p4-only mode. We did not do anything special under the\nprepare-p4-only mode if applyCommit returned a success.\n\n> + if self.prepare_p4_only:\n> + if i < last:\n> + print(\"Processing only the first commit due to option\" \\\n> + \" --prepare-p4-only\")\n\nNow, after successfully applying, we leave the loop under the\nprepare-p4-only mode. We give the message only when we are not at\nthe last step.\n\n> break\n> + else:\n\nSo..., what happens when the first step fails to apply and then the\nuser tells us to skip the commit? We'll go on to the next commit\nand then applyCommit() may say 'ok' this time around. Does that\ncount as \"processed only the first commit and we are done\"?\n\n> if i < last:\n> # prompt for what to do, or use the option/variable\n> if self.conflict_behavior == \"ask\":\n>\n> base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17\n"},{"id":"398005","messageId":"CAE5ih797YYxsR2H0TA65w9W-1jF4jQLayja_nGjQMGtc=PB6Jw@mail.gmail.com","threadId":"53368","inReplyTo":"xmqq1rnodimp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] git-p4.py: fix --prepare-p4-only error with multiple commits","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-05-17T08:31:50Z","receivedAt":"2020-05-17T08:32:09Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Tue, 12 May 2020 at 21:05, Junio C Hamano wrote:\n>\n> \"Ben Keene via GitGitGadget\" writes:\n>\n> > ...\n> > Be aware - this change will fix the existing test error in t9807.23\n> > for --prepare-p4-only. However there is insufficent coverage for\n> > this flag. If more than 1 commit is pending submission to P4, the\n> > method will properly prepare the P4 changelist, however it will\n> > still exit the application with an exitcode of 1.\n> >\n> > The current documentation does not define what the exit code should be\n> > in this condition.\n> > (See: https://git-scm.com/docs/git-p4#Documentation/git-p4.txt---prepare-p4-only)\n>\n> Then some proposal to define what the behaviour should be is needed,\n> the consensus implements and then documented.\n\nIt looks like it's always been slightly confusing (at least to me).\n\nIf prepare_p4_only is True, applyCommit() will only fail if the patch\ncould not be applied (which is what you would expect).\n\nIf that happens I would think it would make sense to bail out - I\nwould think most users will want to rebase and try again rather than\nblithely carrying on.\n\nThe \"i < last\" logic in the original patch from 2012 seems to mean\nthat you only get the message \"Processing only the first commit due to\noption prepare-p4-only\" if there is an error and you're not about to\ndrop out anyway.\n\nI personally no longer use --prepare-p4-only, now that we have the\ngit-p4 shelve support. I would imagine this is true for a lot of\npeople.\n\nThe logic in Ben's change seems much more sensible.\n\nLuke\n\n\n>\n> >\n> > Signed-off-by: Ben Keene \n> > ---\n>\n>\n> > git-p4.py: fix --prepare-p4-only error with multiple commits\n> >\n> > When using git p4 submit with the --prepare-p4-only option, the program\n> > should prepare a single p4 changelist and notify the user that more\n> > commits are pending and then stop processing.\n> >\n> > A bug has been introduced by the p4-changelist hook feature that causes\n> > the program to continue to try and process all pending changelists at\n> > the same time.\n> >\n> > The function applyCommit should return True when applying the commit was\n> > successful and the program should continue. In the case of the\n> > --prepare-p4-only flag, the function should return False, alerting the\n> > caller that the program should not proceed with additional commits.\n> >\n> > Change the return value from True to False in the applyCommit function\n> > when git-p4 is executed with --prepare-p4-only flag.\n>\n> This space below the three-dash-line is the best place to describe\n> the difference between the previous version and this one. It seems\n> that the above text is not such a \"here are what was bad/missing in\n> v1 that got fixed/extended\", or a copy of the log message (like many\n> patches that come from GGG has). I am a bit puzzled what it is, but\n> for now let's pretend there wasn't any text below the three-dash-line\n> and read on.\n>\n> > git-p4.py | 9 +++++----\n> > 1 file changed, 5 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/git-p4.py b/git-p4.py\n> > index b8b2a1679e7..c4a4012bcc1 100755\n> > --- a/git-p4.py\n> > +++ b/git-p4.py\n> > @@ -2537,11 +2537,12 @@ def run(self, args):\n> > ok = self.applyCommit(commit)\n> > if ok:\n> > applied.append(commit)\n> > - else:\n> > - if self.prepare_p4_only and i < last:\n> > - print(\"Processing only the first commit due to option\" \\\n> > - \" --prepare-p4-only\")\n>\n> So, it used to be that after failing to apply a commit, unless we\n> are at the last step, we gave a message and left the loop under the\n> prepare-p4-only mode. We did not do anything special under the\n> prepare-p4-only mode if applyCommit returned a success.\n>\n> > + if self.prepare_p4_only:\n> > + if i < last:\n> > + print(\"Processing only the first commit due to option\" \\\n> > + \" --prepare-p4-only\")\n>\n> Now, after successfully applying, we leave the loop under the\n> prepare-p4-only mode. We give the message only when we are not at\n> the last step.\n>\n> > break\n> > + else:\n>\n> So..., what happens when the first step fails to apply and then the\n> user tells us to skip the commit? We'll go on to the next commit\n> and then applyCommit() may say 'ok' this time around. Does that\n> count as \"processed only the first commit and we are done\"?\n>\n> > if i < last:\n> > # prompt for what to do, or use the option/variable\n> > if self.conflict_behavior == \"ask\":\n> >\n> > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17\n"}]}