{"thread":{"id":"52415","subject":"[PATCH 0/3] git-p4: Usability enhancements","startedAt":"2019-12-09T14:16:54Z","lastAt":"2020-01-02T21:44:40Z","messageCount":46,"participants":["Ben Keene via GitGitGadget","Junio C Hamano","Ben Keene","Luke Diamand","Denton Liu","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"387772","messageId":"pull.675.git.git.1575901009.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":null,"subject":"[PATCH 0/3] git-p4: Usability enhancements","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T14:16:46Z","receivedAt":"2019-12-09T14:16:54Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"Some user interaction with git-p4 is not as user-friendly as the rest of the\nGit ecosystem. Here are three areas that can be improved on:\n\n1) When a patch fails and the user is prompted, there is no sanitization of\nthe user input so for a \"yes/no\" question, if the user enters \"YES\" instead\nof a lowercase \"y\", they will be re-prompted to enter their answer. \n\nCommit 1 addresses this by sanitizing the user text by trimming and\nlowercasing their input before testing. Now \"YES\" will succeed!\n\n2) Git can handle scraping the RCS Keyword expansions out of source files\nwhen it is preparing to submit them to P4. However, if the config value\n\"git-p4.attemptRCSCleanup\" isn't set, it will just report that it fails.\n\nCommit 2 adds a helpful suggestion, that the user might want to set\ngit-p4.attemptRCSCleanup.\n\n3) If the command line arguments are incorrect for git-p4, the program\nreports that there was a syntax error, but doesn't show what the correct\nsyntax is.\n\nCommit 3 displays the context help for the failed command.\n\nBen Keene (3):\n git-p4: [usability] yes/no prompts should sanitize user text\n git-p4: [usability] RCS Keyword failure should suggest help\n git-p4: [usability] Show detailed help when parsing options fail\n\n git-p4.py | 43 +++++++++++++++++++++++++++++++++++++------\n 1 file changed, 37 insertions(+), 6 deletions(-)\n\n\nbase-commit: 083378cc35c4dbcc607e4cdd24a5fca440163d17\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v1\nPull-Request: https://github.com/git/git/pull/675\n-- \ngitgitgadget\n"},{"id":"387773","messageId":"e721cdaa008263b896c1d162e411c4e7a04c5710.1575901009.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.git.git.1575901009.gitgitgadget@gmail.com","subject":"[PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T14:16:47Z","receivedAt":"2019-12-09T14:16:56Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen prompting the user interactively for direction, the tests are\nnot forgiving of user input format.\n\nFor example, the first query asks for a yes/no response. If the user\nenters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\nwill fail.\n\nCreate a new function, prompt(prompt_text, choices) where\n * promt_text is the text prompt for the user\n * is a list of lower-case, single letter choices.\nThis new function must prompt the user for input and sanitize it by\nconverting the response to a lower case string, trimming leading and\ntrailing spaces, and checking if the first character is in the list\nof choices. If it is, return the first letter.\n\nChange the current references to raw_input() to use this new function.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 17 ++++++++++++++---\n 1 file changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 60c73b6a37..0fa562fac9 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -167,6 +167,17 @@ def die(msg):\n sys.stderr.write(msg + \"\\n\")\n sys.exit(1)\n \n+def prompt(prompt_text, choices = []):\n+ \"\"\" Prompt the user to choose one of the choices\n+ \"\"\"\n+ while True:\n+ response = raw_input(prompt_text).strip().lower()\n+ if len(response) == 0:\n+ continue\n+ response = response[0]\n+ if response in choices:\n+ return response\n+\n def write_pipe(c, stdin):\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n@@ -1779,7 +1790,7 @@ def edit_template(self, template_file):\n return True\n \n while True:\n- response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n+ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n if response == 'y':\n return True\n if response == 'n':\n@@ -2350,8 +2361,8 @@ def run(self, args):\n # prompt for what to do, or use the option/variable\n if self.conflict_behavior == \"ask\":\n print(\"What do you want to do?\")\n- response = raw_input(\"[s]kip this commit but apply\"\n- \" the rest, or [q]uit? \")\n+ response = prompt(\"[s]kip this commit but apply\"\n+ \" the rest, or [q]uit? \", [\"s\", \"q\"])\n if not response:\n continue\n elif self.conflict_behavior == \"skip\":\n-- \ngitgitgadget\n\n"},{"id":"387774","messageId":"d608f529a0e01e99c97e895ab483000da068a7ac.1575901009.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.git.git.1575901009.gitgitgadget@gmail.com","subject":"[PATCH 2/3] git-p4: [usability] RCS Keyword failure should suggest help","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T14:16:48Z","receivedAt":"2019-12-09T14:16:58Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen applying a commit fails because of RCS keywords, Git\nwill fail the P4 submit. It would help the user if Git suggested that\nthe user set git-p4.attemptRCSCleanup to true.\n\nChange the applyCommit() method that when applying a commit fails\nbecasue of the P4 RCS Keywords, the user should consider setting\ngit-p4.attemptRCSCleanup to true.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 19 +++++++++++++++++--\n 1 file changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0fa562fac9..856fe82079 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1950,8 +1950,23 @@ def applyCommit(self, id):\n # disable the read-only bit on windows.\n if self.isWindows and file not in editedFiles:\n os.chmod(file, stat.S_IWRITE)\n- self.patchRCSKeywords(file, kwfiles[file])\n- fixed_rcs_keywords = True\n+ \n+ try:\n+ self.patchRCSKeywords(file, kwfiles[file])\n+ fixed_rcs_keywords = True\n+ except:\n+ # We are throwing an exception, undo all open edits\n+ for f in editedFiles:\n+ p4_revert(f)\n+ raise\n+ else:\n+ # They do not have attemptRCSCleanup set, this might be the fail point\n+ # Check to see if the file has RCS keywords and suggest setting the property.\n+ for file in editedFiles | filesToDelete:\n+ if p4_keywords_regexp_for_file(file) != None:\n+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n+ break\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n\n"},{"id":"387775","messageId":"2a10890ef76697dbdd67b4c416077726100f88be.1575901009.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.git.git.1575901009.gitgitgadget@gmail.com","subject":"[PATCH 3/3] git-p4: [usability] Show detailed help when parsing options fail","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T14:16:49Z","receivedAt":"2019-12-09T14:16:58Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen a user provides invalid parameters to git-p4, the program\nreports the failure but does not provide the correct command syntax.\n\nAdd an exception handler to the command-line argument parser to display\nthe command's specific command line parameter syntax when an exception\nis thrown. Rethrow the exception so the current behavior is retained.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 856fe82079..cb594baeef 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4166,7 +4166,12 @@ def main():\n description = cmd.description,\n formatter = HelpFormatter())\n \n- (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ try:\n+ (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ except:\n+ parser.print_help()\n+ raise\n+\n global verbose\n verbose = cmd.verbose\n if cmd.needsGit:\n-- \ngitgitgadget\n"},{"id":"387851","messageId":"xmqqimmptazs.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"e721cdaa008263b896c1d162e411c4e7a04c5710.1575901009.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T22:00:55Z","receivedAt":"2019-12-09T22:01:04Z","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 prompting the user interactively for direction, the tests are\n> not forgiving of user input format.\n>\n> For example, the first query asks for a yes/no response. If the user\n> enters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\n> will fail.\n>\n> Create a new function, prompt(prompt_text, choices) where\n> * promt_text is the text prompt for the user\n> * is a list of lower-case, single letter choices.\n> This new function must prompt the user for input and sanitize it by\n> converting the response to a lower case string, trimming leading and\n> trailing spaces, and checking if the first character is in the list\n> of choices. If it is, return the first letter.\n>\n> Change the current references to raw_input() to use this new function.\n>\n> Signed-off-by: Ben Keene \n> ---\n> git-p4.py | 17 ++++++++++++++---\n> 1 file changed, 14 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 60c73b6a37..0fa562fac9 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -167,6 +167,17 @@ def die(msg):\n> sys.stderr.write(msg + \"\\n\")\n> sys.exit(1)\n> \n> +def prompt(prompt_text, choices = []):\n> + \"\"\" Prompt the user to choose one of the choices\n> + \"\"\"\n> + while True:\n> + response = raw_input(prompt_text).strip().lower()\n> + if len(response) == 0:\n> + continue\n> + response = response[0]\n> + if response in choices:\n> + return response\n\nI think this is a strict improvement compared to the original, but\nthe new loop makes me wonder if we need to worry more about getting\nEOF while calling raw_input() here. I am assuming that we would get\nEOFError either way so this is no worse/better than the status quo,\nand we can keep it outside the topic (even though it may be a good\ncandidate for a low-hanging fruit for newbies).\n\n> def write_pipe(c, stdin):\n> if verbose:\n> sys.stderr.write('Writing pipe: %s\\n' % str(c))\n> @@ -1779,7 +1790,7 @@ def edit_template(self, template_file):\n> return True\n> \n> while True:\n> - response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n> + response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n> if response == 'y':\n> return True\n> if response == 'n':\n> @@ -2350,8 +2361,8 @@ def run(self, args):\n> # prompt for what to do, or use the option/variable\n> if self.conflict_behavior == \"ask\":\n> print(\"What do you want to do?\")\n> - response = raw_input(\"[s]kip this commit but apply\"\n> - \" the rest, or [q]uit? \")\n> + response = prompt(\"[s]kip this commit but apply\"\n> + \" the rest, or [q]uit? \", [\"s\", \"q\"])\n> if not response:\n> continue\n> elif self.conflict_behavior == \"skip\":\n"},{"id":"387852","messageId":"xmqqeexdtaqy.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"pull.675.git.git.1575901009.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] git-p4: Usability enhancements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T22:06:13Z","receivedAt":"2019-12-09T22:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ben Keene via GitGitGadget\" writes:\n\n> Some user interaction with git-p4 is not as user-friendly as the rest of the\n> Git ecosystem. Here are three areas that can be improved on:\n\nSorry, but I am not a git-p4 person and I barely speak passable\nPython, so I am not a great reviewer for the initial round of any\npatch in this area. IOW it is not all that useful to Cc me, as\nopposed to somebody who have been working with git-p4 code longer\nand more deeply.\n\n $ git shortlog --no-merges --since=2.years git-p4.py\n\nmay be a good way to choose whom we would want to ask for an initial\nround of review (and Luke, who I consider the de-facto git-p4 person,.\nis Cc'ed).\n\nThanks.\n"},{"id":"387854","messageId":"xmqqa781t9zf.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"d608f529a0e01e99c97e895ab483000da068a7ac.1575901009.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] git-p4: [usability] RCS Keyword failure should suggest help","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T22:22:44Z","receivedAt":"2019-12-09T22:22:56Z","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 applying a commit fails because of RCS keywords, Git\n> will fail the P4 submit. It would help the user if Git suggested that\n> the user set git-p4.attemptRCSCleanup to true.\n>\n> Change the applyCommit() method that when applying a commit fails\n> becasue of the P4 RCS Keywords, the user should consider setting\n\ns/becasue/because/\n\n> git-p4.attemptRCSCleanup to true.\n\nThe above explains the new \"else:\" clause really well. The\noriginal's logic was to\n\n - tryPatchCmd to apply a commit, which might fail,\n - when the above fails, only if attemptrcscleanup is set, munge\n the lines with rcs keywords and rerun tryPatchCmd\n\nbut your new \"else:\" gives a suggestion to use the (experimental?)\nattemptRCSCleanup feature.\n\nHowever, it does not explain the change to the \"if :\" clause. I can\nsee that patchRCSKeywords() method does want to raise an exception,\nand it is a good idea to prepare for the case and clean up the mess\nit may create. At least that deserves a mention in the proposed log\nmessage---I actually think that the new try/except is an equally\nimportant improvement that deserves to be a separate patch.\n\n> Signed-off-by: Ben Keene \n> ---\n> git-p4.py | 19 +++++++++++++++++--\n> 1 file changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 0fa562fac9..856fe82079 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1950,8 +1950,23 @@ def applyCommit(self, id):\n> # disable the read-only bit on windows.\n> if self.isWindows and file not in editedFiles:\n> os.chmod(file, stat.S_IWRITE)\n> - self.patchRCSKeywords(file, kwfiles[file])\n> - fixed_rcs_keywords = True\n> + \n> + try:\n> + self.patchRCSKeywords(file, kwfiles[file])\n> + fixed_rcs_keywords = True\n> + except:\n> + # We are throwing an exception, undo all open edits\n> + for f in editedFiles:\n> + p4_revert(f)\n> + raise\n> + else:\n> + # They do not have attemptRCSCleanup set, this might be the fail point\n> + # Check to see if the file has RCS keywords and suggest setting the property.\n> + for file in editedFiles | filesToDelete:\n> + if p4_keywords_regexp_for_file(file) != None:\n> + print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n> + print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n> + break\n> \n> if fixed_rcs_keywords:\n> print(\"Retrying the patch with RCS keywords cleaned up\")\n"},{"id":"387855","messageId":"xmqq5zipt9w4.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"2a10890ef76697dbdd67b4c416077726100f88be.1575901009.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] git-p4: [usability] Show detailed help when parsing options fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T22:24:43Z","receivedAt":"2019-12-09T22:24:48Z","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 a user provides invalid parameters to git-p4, the program\n> reports the failure but does not provide the correct command syntax.\n>\n> Add an exception handler to the command-line argument parser to display\n> the command's specific command line parameter syntax when an exception\n> is thrown. Rethrow the exception so the current behavior is retained.\n\nMakes sense, I guess.\n\nI forgot to mention this, but from the titles of all three patches I\nwould probably drop [usability] thing and downcase the first verb,\ne.g.\n\n Subject: [PATCH 3/3] git-p4: show detailed help when parsing options fail\n\nif I were writing these patches.\n\nThanks.\n"},{"id":"387891","messageId":"179dd921-d9d0-d26d-33e9-3664bf97fcc2@gmail.com","threadId":"52415","inReplyTo":"xmqqimmptazs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text","fromName":"Ben Keene","fromEmail":"seraphire@gmail.com","sentAt":"2019-12-10T14:26:22Z","receivedAt":"2019-12-10T14:26:27Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"\nOn 12/9/2019 5:00 PM, Junio C Hamano wrote:\n> \"Ben Keene via GitGitGadget\" writes:\n>\n>> From: Ben Keene \n>>\n>> When prompting the user interactively for direction, the tests are\n>> not forgiving of user input format.\n>>\n>> For example, the first query asks for a yes/no response. If the user\n>> enters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\n>> will fail.\n>>\n>> Create a new function, prompt(prompt_text, choices) where\n>> * promt_text is the text prompt for the user\n>> * is a list of lower-case, single letter choices.\n>> This new function must prompt the user for input and sanitize it by\n>> converting the response to a lower case string, trimming leading and\n>> trailing spaces, and checking if the first character is in the list\n>> of choices. If it is, return the first letter.\n>>\n>> Change the current references to raw_input() to use this new function.\n>>\n>> Signed-off-by: Ben Keene \n>> ---\n>>\n>> +def prompt(prompt_text, choices = []):\n>> + \"\"\" Prompt the user to choose one of the choices\n>> + \"\"\"\n>> + while True:\n>> + response = raw_input(prompt_text).strip().lower()\n>> + if len(response) == 0:\n>> + continue\n>> + response = response[0]\n>> + if response in choices:\n>> + return response\n> I think this is a strict improvement compared to the original, but\n> the new loop makes me wonder if we need to worry more about getting\n> EOF while calling raw_input() here. I am assuming that we would get\n> EOFError either way so this is no worse/better than the status quo,\n> and we can keep it outside the topic (even though it may be a good\n> candidate for a low-hanging fruit for newbies).\nThat is a good catch.  What should we expect the default behavior\nto be in these two questions if the EOFError occurs?  I would think\nthat we should extend this to an abort of the process?\n> response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n> response = prompt(\"[s]kip this commit but apply the rest, or [q]uit? \", [\"s\", \"q\"])\n\nShould a quit be added to the first prompt and have those be the \ndefaults on EOFError?\n\n"},{"id":"387894","messageId":"1d4f4e210b480b6267207044b1bf79a59a70de24.1575991374.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] git-p4: show detailed help when parsing options fail","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-10T15:22:52Z","receivedAt":"2019-12-10T15:23:01Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen a user provides invalid parameters to git-p4, the program\nreports the failure but does not provide the correct command syntax.\n\nAdd an exception handler to the command-line argument parser to display\nthe command's specific command line parameter syntax when an exception\nis thrown. Rethrow the exception so the current behavior is retained.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0fa562fac9..daa6e8a57a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4151,7 +4151,12 @@ def main():\n description = cmd.description,\n formatter = HelpFormatter())\n \n- (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ try:\n+ (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ except:\n+ parser.print_help()\n+ raise\n+\n global verbose\n verbose = cmd.verbose\n if cmd.needsGit:\n-- \ngitgitgadget\n\n"},{"id":"387895","messageId":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.git.git.1575901009.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] git-p4: Usability enhancements","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-10T15:22:50Z","receivedAt":"2019-12-10T15:23:01Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"Some user interaction with git-p4 is not as user-friendly as the rest of the\nGit ecosystem. Here are three areas that can be improved on:\n\n1) When a patch fails and the user is prompted, there is no sanitization of\nthe user input so for a \"yes/no\" question, if the user enters \"YES\" instead\nof a lowercase \"y\", they will be re-prompted to enter their answer. \n\nCommit 1 addresses this by sanitizing the user text by trimming and\nlowercasing their input before testing. Now \"YES\" will succeed!\n\n2) Git can handle scraping the RCS Keyword expansions out of source files\nwhen it is preparing to submit them to P4. However, if the config value\n\"git-p4.attemptRCSCleanup\" isn't set, it will just report that it fails.\n\nCommit 2 adds a helpful suggestion, that the user might want to set\ngit-p4.attemptRCSCleanup.\n\n3) If the command line arguments are incorrect for git-p4, the program\nreports that there was a syntax error, but doesn't show what the correct\nsyntax is.\n\nCommit 3 displays the context help for the failed command.\n\nBen Keene (4):\n git-p4: yes/no prompts should sanitize user text\n git-p4: show detailed help when parsing options fail\n git-p4: wrap patchRCSKeywords test to revert changes on failure\n git-p4: failure because of RCS keywords should show help\n\n git-p4.py | 43 +++++++++++++++++++++++++++++++++++++------\n 1 file changed, 37 insertions(+), 6 deletions(-)\n\n\nbase-commit: 083378cc35c4dbcc607e4cdd24a5fca440163d17\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v2\nPull-Request: https://github.com/git/git/pull/675\n\nRange-diff vs v1:\n\n 1: e721cdaa00 ! 1: 527b7b8f8a git-p4: [usability] yes/no prompts should sanitize user text\n @@ -1,6 +1,6 @@\n Author: Ben Keene \n \n - git-p4: [usability] yes/no prompts should sanitize user text\n + git-p4: yes/no prompts should sanitize user text\n \n When prompting the user interactively for direction, the tests are\n not forgiving of user input format.\n 3: 2a10890ef7 ! 2: 1d4f4e210b git-p4: [usability] Show detailed help when parsing options fail\n @@ -1,6 +1,6 @@\n Author: Ben Keene \n \n - git-p4: [usability] Show detailed help when parsing options fail\n + git-p4: show detailed help when parsing options fail\n \n When a user provides invalid parameters to git-p4, the program\n reports the failure but does not provide the correct command syntax.\n 2: d608f529a0 ! 3: 20aa557193 git-p4: [usability] RCS Keyword failure should suggest help\n @@ -1,14 +1,13 @@\n Author: Ben Keene \n \n - git-p4: [usability] RCS Keyword failure should suggest help\n + git-p4: wrap patchRCSKeywords test to revert changes on failure\n \n - When applying a commit fails because of RCS keywords, Git\n - will fail the P4 submit. It would help the user if Git suggested that\n - the user set git-p4.attemptRCSCleanup to true.\n + The patchRCSKeywords function has the potentional of throwing\n + an exception and this would leave files checked out in P4 and partially\n + modified.\n \n - Change the applyCommit() method that when applying a commit fails\n - becasue of the P4 RCS Keywords, the user should consider setting\n - git-p4.attemptRCSCleanup to true.\n + Add a try-catch block around the patchRCSKeywords call and revert\n + the edited files in P4 before leaving the method.\n \n Signed-off-by: Ben Keene \n \n @@ -30,14 +29,6 @@\n + for f in editedFiles:\n + p4_revert(f)\n + raise\n -+ else:\n -+ # They do not have attemptRCSCleanup set, this might be the fail point\n -+ # Check to see if the file has RCS keywords and suggest setting the property.\n -+ for file in editedFiles | filesToDelete:\n -+ if p4_keywords_regexp_for_file(file) != None:\n -+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n -+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n -+ break\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n -: ---------- > 4: 50e9a175c3 git-p4: failure because of RCS keywords should show help\n\n-- \ngitgitgadget\n"},{"id":"387896","messageId":"50e9a175c3323074ceec848c0d4054edd240e862.1575991375.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] git-p4: failure because of RCS keywords should show help","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-10T15:22:54Z","receivedAt":"2019-12-10T15:23:02Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen applying a commit fails because of RCS keywords, Git\nwill fail the P4 submit. It would help the user if Git suggested that\nthe user set git-p4.attemptRCSCleanup to true.\n\nChange the applyCommit() method that when applying a commit fails\nbecasue of the P4 RCS Keywords, the user should consider setting\ngit-p4.attemptRCSCleanup to true.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 174200bb6c..cb594baeef 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1959,6 +1959,14 @@ def applyCommit(self, id):\n for f in editedFiles:\n p4_revert(f)\n raise\n+ else:\n+ # They do not have attemptRCSCleanup set, this might be the fail point\n+ # Check to see if the file has RCS keywords and suggest setting the property.\n+ for file in editedFiles | filesToDelete:\n+ if p4_keywords_regexp_for_file(file) != None:\n+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n+ break\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n"},{"id":"387897","messageId":"20aa557193c14292a31f91a93a5a8f4ea3ff332b.1575991375.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-10T15:22:53Z","receivedAt":"2019-12-10T15:23:04Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nThe patchRCSKeywords function has the potentional of throwing\nan exception and this would leave files checked out in P4 and partially\nmodified.\n\nAdd a try-catch block around the patchRCSKeywords call and revert\nthe edited files in P4 before leaving the method.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex daa6e8a57a..174200bb6c 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1950,8 +1950,15 @@ def applyCommit(self, id):\n # disable the read-only bit on windows.\n if self.isWindows and file not in editedFiles:\n os.chmod(file, stat.S_IWRITE)\n- self.patchRCSKeywords(file, kwfiles[file])\n- fixed_rcs_keywords = True\n+ \n+ try:\n+ self.patchRCSKeywords(file, kwfiles[file])\n+ fixed_rcs_keywords = True\n+ except:\n+ # We are throwing an exception, undo all open edits\n+ for f in editedFiles:\n+ p4_revert(f)\n+ raise\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n\n"},{"id":"387898","messageId":"527b7b8f8a25a9f8abc326004792507f7fe5e373.1575991374.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-10T15:22:51Z","receivedAt":"2019-12-10T15:23:06Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen prompting the user interactively for direction, the tests are\nnot forgiving of user input format.\n\nFor example, the first query asks for a yes/no response. If the user\nenters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\nwill fail.\n\nCreate a new function, prompt(prompt_text, choices) where\n * promt_text is the text prompt for the user\n * is a list of lower-case, single letter choices.\nThis new function must prompt the user for input and sanitize it by\nconverting the response to a lower case string, trimming leading and\ntrailing spaces, and checking if the first character is in the list\nof choices. If it is, return the first letter.\n\nChange the current references to raw_input() to use this new function.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 17 ++++++++++++++---\n 1 file changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 60c73b6a37..0fa562fac9 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -167,6 +167,17 @@ def die(msg):\n sys.stderr.write(msg + \"\\n\")\n sys.exit(1)\n \n+def prompt(prompt_text, choices = []):\n+ \"\"\" Prompt the user to choose one of the choices\n+ \"\"\"\n+ while True:\n+ response = raw_input(prompt_text).strip().lower()\n+ if len(response) == 0:\n+ continue\n+ response = response[0]\n+ if response in choices:\n+ return response\n+\n def write_pipe(c, stdin):\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n@@ -1779,7 +1790,7 @@ def edit_template(self, template_file):\n return True\n \n while True:\n- response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n+ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n if response == 'y':\n return True\n if response == 'n':\n@@ -2350,8 +2361,8 @@ def run(self, args):\n # prompt for what to do, or use the option/variable\n if self.conflict_behavior == \"ask\":\n print(\"What do you want to do?\")\n- response = raw_input(\"[s]kip this commit but apply\"\n- \" the rest, or [q]uit? \")\n+ response = prompt(\"[s]kip this commit but apply\"\n+ \" the rest, or [q]uit? \", [\"s\", \"q\"])\n if not response:\n continue\n elif self.conflict_behavior == \"skip\":\n-- \ngitgitgadget\n\n"},{"id":"387933","messageId":"CAE5ih78hTOjXcwOxQvKLz+MAvThXLSF+CGM9RFAr7zJcSKr8pw@mail.gmail.com","threadId":"52415","inReplyTo":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] git-p4: Usability enhancements","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2019-12-11T09:43:03Z","receivedAt":"2019-12-11T09:43:14Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Tue, 10 Dec 2019 at 15:23, Ben Keene via GitGitGadget\n wrote:\n>\n> Some user interaction with git-p4 is not as user-friendly as the rest of the\n> Git ecosystem. Here are three areas that can be improved on:\n>\n> 1) When a patch fails and the user is prompted, there is no sanitization of\n> the user input so for a \"yes/no\" question, if the user enters \"YES\" instead\n> of a lowercase \"y\", they will be re-prompted to enter their answer.\n>\n> Commit 1 addresses this by sanitizing the user text by trimming and\n> lowercasing their input before testing. Now \"YES\" will succeed!\n>\n> 2) Git can handle scraping the RCS Keyword expansions out of source files\n> when it is preparing to submit them to P4. However, if the config value\n> \"git-p4.attemptRCSCleanup\" isn't set, it will just report that it fails.\n>\n> Commit 2 adds a helpful suggestion, that the user might want to set\n> git-p4.attemptRCSCleanup.\n>\n> 3) If the command line arguments are incorrect for git-p4, the program\n> reports that there was a syntax error, but doesn't show what the correct\n> syntax is.\n>\n> Commit 3 displays the context help for the failed command.\n>\n> Ben Keene (4):\n> git-p4: yes/no prompts should sanitize user text\n> git-p4: show detailed help when parsing options fail\n> git-p4: wrap patchRCSKeywords test to revert changes on failure\n> git-p4: failure because of RCS keywords should show help\n>\n> git-p4.py | 43 +++++++++++++++++++++++++++++++++++++------\n> 1 file changed, 37 insertions(+), 6 deletions(-)\n>\n>\n> base-commit: 083378cc35c4dbcc607e4cdd24a5fca440163d17\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v2\n> Pull-Request: https://github.com/git/git/pull/675\n>\n> Range-diff vs v1:\n>\n> 1: e721cdaa00 ! 1: 527b7b8f8a git-p4: [usability] yes/no prompts should sanitize user text\n> @@ -1,6 +1,6 @@\n> Author: Ben Keene \n>\n> - git-p4: [usability] yes/no prompts should sanitize user text\n> + git-p4: yes/no prompts should sanitize user text\n>\n> When prompting the user interactively for direction, the tests are\n> not forgiving of user input format.\n\nLooks good to me!\n\n> 3: 2a10890ef7 ! 2: 1d4f4e210b git-p4: [usability] Show detailed help when parsing options fail\n> @@ -1,6 +1,6 @@\n> Author: Ben Keene \n>\n> - git-p4: [usability] Show detailed help when parsing options fail\n> + git-p4: show detailed help when parsing options fail\n>\n> When a user provides invalid parameters to git-p4, the program\n> reports the failure but does not provide the correct command syntax.\n\nThis would make git-p4 more consistent with other git commands which\ngive some brief options, so seems like a sensible thing to do.\n\n\n> 2: d608f529a0 ! 3: 20aa557193 git-p4: [usability] RCS Keyword failure should suggest help\n> @@ -1,14 +1,13 @@\n> Author: Ben Keene \n>\n> - git-p4: [usability] RCS Keyword failure should suggest help\n> + git-p4: wrap patchRCSKeywords test to revert changes on failure\n>\n> - When applying a commit fails because of RCS keywords, Git\n> - will fail the P4 submit. It would help the user if Git suggested that\n> - the user set git-p4.attemptRCSCleanup to true.\n> + The patchRCSKeywords function has the potentional of throwing\n> + an exception and this would leave files checked out in P4 and partially\n> + modified.\n>\n> - Change the applyCommit() method that when applying a commit fails\n> - becasue of the P4 RCS Keywords, the user should consider setting\n> - git-p4.attemptRCSCleanup to true.\n> + Add a try-catch block around the patchRCSKeywords call and revert\n> + the edited files in P4 before leaving the method.\n>\n> Signed-off-by: Ben Keene \n>\n> @@ -30,14 +29,6 @@\n> + for f in editedFiles:\n> + p4_revert(f)\n> + raise\n> -+ else:\n> -+ # They do not have attemptRCSCleanup set, this might be the fail point\n> -+ # Check to see if the file has RCS keywords and suggest setting the property.\n> -+ for file in editedFiles | filesToDelete:\n> -+ if p4_keywords_regexp_for_file(file) != None:\n> -+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n> -+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n> -+ break\n\nThe code doesn't actually check if there are any files with RCS\nkeywords, it just looks to see if there are files that _could_ have\nRCS keywords. There's some code in the other branch of the if-else\nwhich checks for RCS keywords.\n\nPerhaps factor that out, and then do the check (?).\n\nGiven that, this would be quite a useful hint for people.\n\n\n>\n> if fixed_rcs_keywords:\n> print(\"Retrying the patch with RCS keywords cleaned up\")\n> -: ---------- > 4: 50e9a175c3 git-p4: failure because of RCS keywords should show help\n>\n> --\n> gitgitgadget\n\nIn your branch there's also a \"wrap patchRCSKeywords test\" commit,\nwhich isn't in here. That has some whitespace damage. But also I\nwonder if the reverting should happen higher up, since there is\nalready some code around which tries to do this in other\ncircumstances.\n\nOther than the small comments above, this looks good to me, thanks!\n\nLuke\n"},{"id":"387934","messageId":"20191211112908.GA41678@generichostname","threadId":"52415","inReplyTo":"50e9a175c3323074ceec848c0d4054edd240e862.1575991375.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] git-p4: failure because of RCS keywords should show help","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-11T11:29:08Z","receivedAt":"2019-12-11T11:28:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Tue, Dec 10, 2019 at 03:22:54PM +0000, Ben Keene via GitGitGadget wrote:\n> From: Ben Keene \n> \n> When applying a commit fails because of RCS keywords, Git\n> will fail the P4 submit. It would help the user if Git suggested that\n> the user set git-p4.attemptRCSCleanup to true.\n> \n> Change the applyCommit() method that when applying a commit fails\n> becasue of the P4 RCS Keywords, the user should consider setting\n> git-p4.attemptRCSCleanup to true.\n> \n> Signed-off-by: Ben Keene \n> ---\n> git-p4.py | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index 174200bb6c..cb594baeef 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1959,6 +1959,14 @@ def applyCommit(self, id):\n> for f in editedFiles:\n> p4_revert(f)\n> raise\n> + else:\n> + # They do not have attemptRCSCleanup set, this might be the fail point\n> + # Check to see if the file has RCS keywords and suggest setting the property.\n> + for file in editedFiles | filesToDelete:\n> + if p4_keywords_regexp_for_file(file) != None:\n\nsmall nit: we should use `is not None` here.\n\n> + print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n> + print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n> + break\n> \n> if fixed_rcs_keywords:\n> print(\"Retrying the patch with RCS keywords cleaned up\")\n> -- \n> gitgitgadget\n"},{"id":"387936","messageId":"20191211115252.GB41678@generichostname","threadId":"52415","inReplyTo":"527b7b8f8a25a9f8abc326004792507f7fe5e373.1575991374.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-11T11:52:52Z","receivedAt":"2019-12-11T11:52:00Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Ben,\n\nOn Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:\n> diff --git a/git-p4.py b/git-p4.py\n> index 60c73b6a37..0fa562fac9 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -167,6 +167,17 @@ def die(msg):\n> sys.stderr.write(msg + \"\\n\")\n> sys.exit(1)\n> \n> +def prompt(prompt_text, choices = []):\n\nnit: remove space in the default assignment\n\nBut more importantly, perhaps we should use the empty tuple instead,\n`()`. The reason why is in Python, the default object is initialised\nonce and the reference stays the same[1]. So if you appended something to\n`choices`, that would stay between sucessive function invocations.\n\nSince your function only reads `choices` and doesn't write, what you\nhave isn't wrong but I think it would be more future-proof to use `()`\ninstead.\n\nAlso, here's a stupid idea: perhaps instead of manually specifying\n`choices` manually, could we extract it from `prompt_text` since all\npossible choices are always placed within []?\n\nSomething like this?\n\n\timport re\n\t...\n\tchoices = set(m.group(1) for m in re.finditer(r\"\\[(.)\\]\", prompt_text))\n\n> + \"\"\" Prompt the user to choose one of the choices\n> + \"\"\"\n> + while True:\n> + response = raw_input(prompt_text).strip().lower()\n> + if len(response) == 0:\n\nIt's more Pythonic to write `if not response`.\n\n> + continue\n> + response = response[0]\n> + if response in choices:\n> + return response\n> +\n> def write_pipe(c, stdin):\n> if verbose:\n> sys.stderr.write('Writing pipe: %s\\n' % str(c))\n> @@ -1779,7 +1790,7 @@ def edit_template(self, template_file):\n> return True\n> \n> while True:\n> - response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n> + response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n\nSemantically, `[\"y\", \"n\"]` should be a tuple too so that we emphasise\nthat the set of choices shouldn't be mutable.\n\n> if response == 'y':\n> return True\n> if response == 'n':\n> @@ -2350,8 +2361,8 @@ def run(self, args):\n> # prompt for what to do, or use the option/variable\n> if self.conflict_behavior == \"ask\":\n> print(\"What do you want to do?\")\n> - response = raw_input(\"[s]kip this commit but apply\"\n> - \" the rest, or [q]uit? \")\n> + response = prompt(\"[s]kip this commit but apply\"\n> + \" the rest, or [q]uit? \", [\"s\", \"q\"])\n\nSame here.\n\nThanks,\n\nDenton\n\n[1]: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments\n\n> if not response:\n> continue\n> elif self.conflict_behavior == \"skip\":\n> -- \n> gitgitgadget\n> \n"},{"id":"387937","messageId":"20191211115900.GC41678@generichostname","threadId":"52415","inReplyTo":"527b7b8f8a25a9f8abc326004792507f7fe5e373.1575991374.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-11T11:59:00Z","receivedAt":"2019-12-11T11:58:04Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:\n> @@ -1779,7 +1790,7 @@ def edit_template(self, template_file):\n> return True\n> \n> while True:\n> - response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n> + response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n> if response == 'y':\n> return True\n> if response == 'n':\n\nOne more thing, since we guarantee that prompt() returns 'y' or 'n' and\nit handles the looping logic, we can get rid of the surrounding while.\n\n> @@ -2350,8 +2361,8 @@ def run(self, args):\n> # prompt for what to do, or use the option/variable\n> if self.conflict_behavior == \"ask\":\n> print(\"What do you want to do?\")\n> - response = raw_input(\"[s]kip this commit but apply\"\n> - \" the rest, or [q]uit? \")\n> + response = prompt(\"[s]kip this commit but apply\"\n> + \" the rest, or [q]uit? \", [\"s\", \"q\"])\n> if not response:\n> continue\n> elif self.conflict_behavior == \"skip\":\n\nSame with this, we can remove the surrounding `while`.\n\n> -- \n> gitgitgadget\n> \n"},{"id":"388039","messageId":"5c5c9816322583e36b59cb0649bb03fb6e06e8a7.1576179987.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v3.git.git.1576179987.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] git-p4: show detailed help when parsing options fail","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-12T19:46:25Z","receivedAt":"2019-12-12T19:46:34Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen a user provides invalid parameters to git-p4, the program\nreports the failure but does not provide the correct command syntax.\n\nAdd an exception handler to the command-line argument parser to display\nthe command's specific command line parameter syntax when an exception\nis thrown. Rethrow the exception so the current behavior is retained.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex a05385ee2a..45c0175a68 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4145,7 +4145,12 @@ def main():\n description = cmd.description,\n formatter = HelpFormatter())\n \n- (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ try:\n+ (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ except:\n+ parser.print_help()\n+ raise\n+\n global verbose\n verbose = cmd.verbose\n if cmd.needsGit:\n-- \ngitgitgadget\n\n"},{"id":"388040","messageId":"pull.675.v3.git.git.1576179987.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v2.git.git.1575991374.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] git-p4: Usability enhancements","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-12T19:46:23Z","receivedAt":"2019-12-12T19:46:34Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"Some user interaction with git-p4 is not as user-friendly as the rest of the\nGit ecosystem. Here are three areas that can be improved on:\n\n1) When a patch fails and the user is prompted, there is no sanitization of\nthe user input so for a \"yes/no\" question, if the user enters \"YES\" instead\nof a lowercase \"y\", they will be re-prompted to enter their answer. \n\nCommit 1 addresses this by sanitizing the user text by trimming and\nlowercasing their input before testing. Now \"YES\" will succeed!\n\n2) Git can handle scraping the RCS Keyword expansions out of source files\nwhen it is preparing to submit them to P4. However, if the config value\n\"git-p4.attemptRCSCleanup\" isn't set, it will just report that it fails.\n\nCommit 2 adds a helpful suggestion, that the user might want to set\ngit-p4.attemptRCSCleanup.\n\n3) If the command line arguments are incorrect for git-p4, the program\nreports that there was a syntax error, but doesn't show what the correct\nsyntax is.\n\nCommit 3 displays the context help for the failed command.\n\nBen Keene (4):\n git-p4: yes/no prompts should sanitize user text\n git-p4: show detailed help when parsing options fail\n git-p4: wrap patchRCSKeywords test to revert changes on failure\n git-p4: failure because of RCS keywords should show help\n\n git-p4.py | 94 +++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 60 insertions(+), 34 deletions(-)\n\n\nbase-commit: ad05a3d8e5a6a06443836b5e40434262d992889a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v3\nPull-Request: https://github.com/git/git/pull/675\n\nRange-diff vs v2:\n\n 1: 527b7b8f8a ! 1: fff93acf44 git-p4: yes/no prompts should sanitize user text\n @@ -9,9 +9,12 @@\n enters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\n will fail.\n \n - Create a new function, prompt(prompt_text, choices) where\n + Create a new function, prompt(prompt_text) where\n * promt_text is the text prompt for the user\n - * is a list of lower-case, single letter choices.\n + * choices are extracted from the prompt text [.]\n + a single letter surrounded by square brackets\n + is selected as a valid choice.\n +\n This new function must prompt the user for input and sanitize it by\n converting the response to a lower case string, trimming leading and\n trailing spaces, and checking if the first character is in the list\n @@ -19,6 +22,10 @@\n \n Change the current references to raw_input() to use this new function.\n \n + Since the method requires the returned text to be one of the available\n + choices, remove the loop from the calling code that handles response\n + verification.\n +\n Signed-off-by: Ben Keene \n \n diff --git a/git-p4.py b/git-p4.py\n @@ -28,12 +35,16 @@\n sys.stderr.write(msg + \"\\n\")\n sys.exit(1)\n \n -+def prompt(prompt_text, choices = []):\n ++def prompt(prompt_text):\n + \"\"\" Prompt the user to choose one of the choices\n ++\n ++ Choices are identified in the prompt_text by square brackets around\n ++ a single letter option.\n + \"\"\"\n ++ choices = set(m.group(1) for m in re.finditer(r\"\\[(.)\\]\", prompt_text))\n + while True:\n + response = raw_input(prompt_text).strip().lower()\n -+ if len(response) == 0:\n ++ if not response:\n + continue\n + response = response[0]\n + if response in choices:\n @@ -43,22 +54,73 @@\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n @@\n + if os.stat(template_file).st_mtime > mtime:\n return True\n \n - while True:\n +- while True:\n - response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n -+ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \", [\"y\", \"n\"])\n - if response == 'y':\n - return True\n - if response == 'n':\n +- if response == 'y':\n +- return True\n +- if response == 'n':\n +- return False\n ++ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n ++ if response == 'y':\n ++ return True\n ++ if response == 'n':\n ++ return False\n + \n + def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n + # diff\n @@\n - # prompt for what to do, or use the option/variable\n - if self.conflict_behavior == \"ask\":\n - print(\"What do you want to do?\")\n + \" --prepare-p4-only\")\n + break\n + if i < last:\n +- quit = False\n +- while True:\n +- # prompt for what to do, or use the option/variable\n +- if self.conflict_behavior == \"ask\":\n +- print(\"What do you want to do?\")\n - response = raw_input(\"[s]kip this commit but apply\"\n - \" the rest, or [q]uit? \")\n -+ response = prompt(\"[s]kip this commit but apply\"\n -+ \" the rest, or [q]uit? \", [\"s\", \"q\"])\n - if not response:\n - continue\n - elif self.conflict_behavior == \"skip\":\n +- if not response:\n +- continue\n +- elif self.conflict_behavior == \"skip\":\n +- response = \"s\"\n +- elif self.conflict_behavior == \"quit\":\n +- response = \"q\"\n +- else:\n +- die(\"Unknown conflict_behavior '%s'\" %\n +- self.conflict_behavior)\n +-\n +- if response[0] == \"s\":\n +- print(\"Skipping this commit, but applying the rest\")\n +- break\n +- if response[0] == \"q\":\n +- print(\"Quitting\")\n +- quit = True\n +- break\n +- if quit:\n ++ # prompt for what to do, or use the option/variable\n ++ if self.conflict_behavior == \"ask\":\n ++ print(\"What do you want to do?\")\n ++ response = prompt(\"[s]kip this commit but apply the rest, or [q]uit? \")\n ++ elif self.conflict_behavior == \"skip\":\n ++ response = \"s\"\n ++ elif self.conflict_behavior == \"quit\":\n ++ response = \"q\"\n ++ else:\n ++ die(\"Unknown conflict_behavior '%s'\" %\n ++ self.conflict_behavior)\n ++\n ++ if response == \"s\":\n ++ print(\"Skipping this commit, but applying the rest\")\n ++ if response == \"q\":\n ++ print(\"Quitting\")\n + break\n + \n + chdir(self.oldWorkingDirectory)\n +@@\n + \n + if __name__ == '__main__':\n + main()\n ++\n 2: 1d4f4e210b = 2: 5c5c981632 git-p4: show detailed help when parsing options fail\n 3: 20aa557193 = 3: c466e79148 git-p4: wrap patchRCSKeywords test to revert changes on failure\n 4: 50e9a175c3 ! 4: 00307c3951 git-p4: failure because of RCS keywords should show help\n @@ -23,7 +23,7 @@\n + # They do not have attemptRCSCleanup set, this might be the fail point\n + # Check to see if the file has RCS keywords and suggest setting the property.\n + for file in editedFiles | filesToDelete:\n -+ if p4_keywords_regexp_for_file(file) != None:\n ++ if p4_keywords_regexp_for_file(file) is not None:\n + print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n + print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n + break\n\n-- \ngitgitgadget\n"},{"id":"388041","messageId":"c466e79148028364873189eb6fe4a1db97fb100f.1576179987.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v3.git.git.1576179987.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-12T19:46:26Z","receivedAt":"2019-12-12T19:46:37Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nThe patchRCSKeywords function has the potentional of throwing\nan exception and this would leave files checked out in P4 and partially\nmodified.\n\nAdd a try-catch block around the patchRCSKeywords call and revert\nthe edited files in P4 before leaving the method.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 45c0175a68..97fad8d3f0 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1953,8 +1953,15 @@ def applyCommit(self, id):\n # disable the read-only bit on windows.\n if self.isWindows and file not in editedFiles:\n os.chmod(file, stat.S_IWRITE)\n- self.patchRCSKeywords(file, kwfiles[file])\n- fixed_rcs_keywords = True\n+ \n+ try:\n+ self.patchRCSKeywords(file, kwfiles[file])\n+ fixed_rcs_keywords = True\n+ except:\n+ # We are throwing an exception, undo all open edits\n+ for f in editedFiles:\n+ p4_revert(f)\n+ raise\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n\n"},{"id":"388042","messageId":"00307c395157d798501a8776521779206f888563.1576179987.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v3.git.git.1576179987.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] git-p4: failure because of RCS keywords should show help","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-12T19:46:27Z","receivedAt":"2019-12-12T19:46:37Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen applying a commit fails because of RCS keywords, Git\nwill fail the P4 submit. It would help the user if Git suggested that\nthe user set git-p4.attemptRCSCleanup to true.\n\nChange the applyCommit() method that when applying a commit fails\nbecasue of the P4 RCS Keywords, the user should consider setting\ngit-p4.attemptRCSCleanup to true.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 97fad8d3f0..b659b8d4bf 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1962,6 +1962,14 @@ def applyCommit(self, id):\n for f in editedFiles:\n p4_revert(f)\n raise\n+ else:\n+ # They do not have attemptRCSCleanup set, this might be the fail point\n+ # Check to see if the file has RCS keywords and suggest setting the property.\n+ for file in editedFiles | filesToDelete:\n+ if p4_keywords_regexp_for_file(file) is not None:\n+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n+ break\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n"},{"id":"388043","messageId":"fff93acf4430e2e7702ae1345f9899244a9867aa.1576179987.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v3.git.git.1576179987.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-12T19:46:24Z","receivedAt":"2019-12-12T19:46:37Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen prompting the user interactively for direction, the tests are\nnot forgiving of user input format.\n\nFor example, the first query asks for a yes/no response. If the user\nenters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\nwill fail.\n\nCreate a new function, prompt(prompt_text) where\n * promt_text is the text prompt for the user\n * choices are extracted from the prompt text [.]\n a single letter surrounded by square brackets\n is selected as a valid choice.\n\nThis new function must prompt the user for input and sanitize it by\nconverting the response to a lower case string, trimming leading and\ntrailing spaces, and checking if the first character is in the list\nof choices. If it is, return the first letter.\n\nChange the current references to raw_input() to use this new function.\n\nSince the method requires the returned text to be one of the available\nchoices, remove the loop from the calling code that handles response\nverification.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 68 ++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 37 insertions(+), 31 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 60c73b6a37..a05385ee2a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -167,6 +167,21 @@ def die(msg):\n sys.stderr.write(msg + \"\\n\")\n sys.exit(1)\n \n+def prompt(prompt_text):\n+ \"\"\" Prompt the user to choose one of the choices\n+\n+ Choices are identified in the prompt_text by square brackets around\n+ a single letter option.\n+ \"\"\"\n+ choices = set(m.group(1) for m in re.finditer(r\"\\[(.)\\]\", prompt_text))\n+ while True:\n+ response = raw_input(prompt_text).strip().lower()\n+ if not response:\n+ continue\n+ response = response[0]\n+ if response in choices:\n+ return response\n+\n def write_pipe(c, stdin):\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n@@ -1778,12 +1793,11 @@ def edit_template(self, template_file):\n if os.stat(template_file).st_mtime > mtime:\n return True\n \n- while True:\n- response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n- if response == 'y':\n- return True\n- if response == 'n':\n- return False\n+ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n+ if response == 'y':\n+ return True\n+ if response == 'n':\n+ return False\n \n def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n # diff\n@@ -2345,31 +2359,22 @@ def run(self, args):\n \" --prepare-p4-only\")\n break\n if i < last:\n- quit = False\n- while True:\n- # prompt for what to do, or use the option/variable\n- if self.conflict_behavior == \"ask\":\n- print(\"What do you want to do?\")\n- response = raw_input(\"[s]kip this commit but apply\"\n- \" the rest, or [q]uit? \")\n- if not response:\n- continue\n- elif self.conflict_behavior == \"skip\":\n- response = \"s\"\n- elif self.conflict_behavior == \"quit\":\n- response = \"q\"\n- else:\n- die(\"Unknown conflict_behavior '%s'\" %\n- self.conflict_behavior)\n-\n- if response[0] == \"s\":\n- print(\"Skipping this commit, but applying the rest\")\n- break\n- if response[0] == \"q\":\n- print(\"Quitting\")\n- quit = True\n- break\n- if quit:\n+ # prompt for what to do, or use the option/variable\n+ if self.conflict_behavior == \"ask\":\n+ print(\"What do you want to do?\")\n+ response = prompt(\"[s]kip this commit but apply the rest, or [q]uit? \")\n+ elif self.conflict_behavior == \"skip\":\n+ response = \"s\"\n+ elif self.conflict_behavior == \"quit\":\n+ response = \"q\"\n+ else:\n+ die(\"Unknown conflict_behavior '%s'\" %\n+ self.conflict_behavior)\n+\n+ if response == \"s\":\n+ print(\"Skipping this commit, but applying the rest\")\n+ if response == \"q\":\n+ print(\"Quitting\")\n break\n \n chdir(self.oldWorkingDirectory)\n@@ -4170,3 +4175,4 @@ def main():\n \n if __name__ == '__main__':\n main()\n+\n-- \ngitgitgadget\n\n"},{"id":"388071","messageId":"20191213014537.GA13064@generichostname","threadId":"52415","inReplyTo":"fff93acf4430e2e7702ae1345f9899244a9867aa.1576179987.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-13T01:45:37Z","receivedAt":"2019-12-13T01:44:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Ben,\n\nOn Thu, Dec 12, 2019 at 07:46:24PM +0000, Ben Keene via GitGitGadget wrote:\n> From: Ben Keene \n> \n> When prompting the user interactively for direction, the tests are\n> not forgiving of user input format.\n> \n> For example, the first query asks for a yes/no response. If the user\n> enters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\n> will fail.\n> \n> Create a new function, prompt(prompt_text) where\n> * promt_text is the text prompt for the user\n\ns/promt/prompt/\n\n> * choices are extracted from the prompt text [.]\n> a single letter surrounded by square brackets\n> is selected as a valid choice.\n\nMaybe something like this?\n\n\t* returns a single character where valid return values are\n\t found by inspecting prompt_text for single characters\n\t surrounded by square brackets\n> \n> This new function must prompt the user for input and sanitize it by\n> converting the response to a lower case string, trimming leading and\n> trailing spaces, and checking if the first character is in the list\n> of choices. If it is, return the first letter.\n> \n> Change the current references to raw_input() to use this new function.\n> \n> Since the method requires the returned text to be one of the available\n> choices, remove the loop from the calling code that handles response\n> verification.\n> \n> Signed-off-by: Ben Keene \n> ---\n> git-p4.py | 68 ++++++++++++++++++++++++++++++-------------------------\n> 1 file changed, 37 insertions(+), 31 deletions(-)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index 60c73b6a37..a05385ee2a 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -167,6 +167,21 @@ def die(msg):\n> sys.stderr.write(msg + \"\\n\")\n> sys.exit(1)\n> \n> +def prompt(prompt_text):\n> + \"\"\" Prompt the user to choose one of the choices\n> +\n> + Choices are identified in the prompt_text by square brackets around\n> + a single letter option.\n> + \"\"\"\n> + choices = set(m.group(1) for m in re.finditer(r\"\\[(.)\\]\", prompt_text))\n\nNice ;)\n\n> + while True:\n> + response = raw_input(prompt_text).strip().lower()\n> + if not response:\n> + continue\n> + response = response[0]\n> + if response in choices:\n> + return response\n> +\n> def write_pipe(c, stdin):\n> if verbose:\n> sys.stderr.write('Writing pipe: %s\\n' % str(c))\n> @@ -1778,12 +1793,11 @@ def edit_template(self, template_file):\n> if os.stat(template_file).st_mtime > mtime:\n> return True\n> \n> - while True:\n> - response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n> - if response == 'y':\n> - return True\n> - if response == 'n':\n> - return False\n> + response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n> + if response == 'y':\n> + return True\n> + if response == 'n':\n> + return False\n> \n> def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n> # diff\n> @@ -2345,31 +2359,22 @@ def run(self, args):\n> \" --prepare-p4-only\")\n> break\n> if i < last:\n> - quit = False\n> - while True:\n> - # prompt for what to do, or use the option/variable\n> - if self.conflict_behavior == \"ask\":\n> - print(\"What do you want to do?\")\n> - response = raw_input(\"[s]kip this commit but apply\"\n> - \" the rest, or [q]uit? \")\n> - if not response:\n> - continue\n> - elif self.conflict_behavior == \"skip\":\n> - response = \"s\"\n> - elif self.conflict_behavior == \"quit\":\n> - response = \"q\"\n> - else:\n> - die(\"Unknown conflict_behavior '%s'\" %\n> - self.conflict_behavior)\n> -\n> - if response[0] == \"s\":\n> - print(\"Skipping this commit, but applying the rest\")\n> - break\n> - if response[0] == \"q\":\n> - print(\"Quitting\")\n> - quit = True\n> - break\n> - if quit:\n> + # prompt for what to do, or use the option/variable\n> + if self.conflict_behavior == \"ask\":\n> + print(\"What do you want to do?\")\n> + response = prompt(\"[s]kip this commit but apply the rest, or [q]uit? \")\n> + elif self.conflict_behavior == \"skip\":\n> + response = \"s\"\n> + elif self.conflict_behavior == \"quit\":\n> + response = \"q\"\n> + else:\n> + die(\"Unknown conflict_behavior '%s'\" %\n> + self.conflict_behavior)\n> +\n> + if response == \"s\":\n> + print(\"Skipping this commit, but applying the rest\")\n> + if response == \"q\":\n> + print(\"Quitting\")\n> break\n> \n> chdir(self.oldWorkingDirectory)\n\nAside from the one comment at the bottom, I reviewed the rest of this\npatch with `-w` and it looks good to me. Unfortunately, I don't use or\nknow p4 so I haven't tested it.\n\n> @@ -4170,3 +4175,4 @@ def main():\n> \n> if __name__ == '__main__':\n> main()\n> +\n\nSpurious trailing line. Perhaps we could make GGG error out on\nwhitespace errors before submissions are allowed?\n\n> -- \n> gitgitgadget\n> \n"},{"id":"388110","messageId":"0afed92d-6efb-380e-cf02-d0a5d35e63a7@gmail.com","threadId":"52415","inReplyTo":"20191213014537.GA13064@generichostname","subject":"Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene","fromEmail":"seraphire@gmail.com","sentAt":"2019-12-13T13:42:21Z","receivedAt":"2019-12-13T20:37:26Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"\nOn 12/12/2019 8:45 PM, Denton Liu wrote:\n> Hi Ben,\n>\n> On Thu, Dec 12, 2019 at 07:46:24PM +0000, Ben Keene via GitGitGadget wrote:\n>> From: Ben Keene \n>> ...\n>> * choices are extracted from the prompt text [.]\n>> a single letter surrounded by square brackets\n>> is selected as a valid choice.\n> Maybe something like this?\n>\n> \t* returns a single character where valid return values are\n> \t found by inspecting prompt_text for single characters\n> \t surrounded by square brackets\nYes, that is much more readable.\n> ...\n> Aside from the one comment at the bottom, I reviewed the rest of this\n> patch with `-w` and it looks good to me. Unfortunately, I don't use or\n> know p4 so I haven't tested it.\n>\n>> @@ -4170,3 +4175,4 @@ def main():\n>> \n>> if __name__ == '__main__':\n>> main()\n>> +\n> Spurious trailing line. Perhaps we could make GGG error out on\n> whitespace errors before submissions are allowed?\nThat is a good idea.  I obviously missed that and it would\nhelp if it reported the error before submission.\n>> -- \n>> gitgitgadget\n>>\n"},{"id":"388111","messageId":"bfdd3dc517c01e1e27be017b87de5e22b2525920.1576245481.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v4.git.git.1576245481.gitgitgadget@gmail.com","subject":"[PATCH v4 2/4] git-p4: show detailed help when parsing options fail","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-13T13:57:59Z","receivedAt":"2019-12-13T20:37:35Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen a user provides invalid parameters to git-p4, the program\nreports the failure but does not provide the correct command syntax.\n\nAdd an exception handler to the command-line argument parser to display\nthe command's specific command line parameter syntax when an exception\nis thrown. Rethrow the exception so the current behavior is retained.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 3b3f1469a6..9165ada2fd 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4145,7 +4145,12 @@ def main():\n description = cmd.description,\n formatter = HelpFormatter())\n \n- (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ try:\n+ (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ except:\n+ parser.print_help()\n+ raise\n+\n global verbose\n verbose = cmd.verbose\n if cmd.needsGit:\n-- \ngitgitgadget\n\n"},{"id":"388112","messageId":"pull.675.v4.git.git.1576245481.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v3.git.git.1576179987.gitgitgadget@gmail.com","subject":"[PATCH v4 0/4] git-p4: Usability enhancements","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-13T13:57:57Z","receivedAt":"2019-12-13T20:37:35Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"Some user interaction with git-p4 is not as user-friendly as the rest of the\nGit ecosystem. \n\nHere are three areas that can be improved on:\n\n1) When a patch fails and the user is prompted, there is no sanitization of\nthe user input so for a \"yes/no\" question, if the user enters \"YES\" instead\nof a lowercase \"y\", they will be re-prompted to enter their answer. \n\nCommit 1 addresses this by sanitizing the user text by trimming and\nlowercasing their input before testing. Now \"YES\" will succeed!\n\n2) If the command line arguments are incorrect for git-p4, the program\nreports that there was a syntax error, but doesn't show what the correct\nsyntax is.\n\nCommit 2 displays the context help for the failed command.\n\n3) If Git generates an error while attempting to clean up the RCS Keyword\nexpansions, it currently leaves P4 in an invalid state. Files that were\nchecked out by P4 are not revereted.\n\nCommit 3 adds and exception handler that catches this condition and issues a\nP4 Revert for the files that were previously edited.\n\n4) Git can handle scraping the RCS Keyword expansions out of source files\nwhen it is preparing to submit them to P4. However, if the config value\n\"git-p4.attemptRCSCleanup\" isn't set, it will just report that it fails.\n\nCommit 4 adds a helpful suggestion, that the user might want to set\ngit-p4.attemptRCSCleanup.\n\nRevisions\n=========\n\nv3 - Implemented the various suggestions from Luke and Denton.\n\nI did not add additional exception handling for the EOFError in the prompt\nmethod. I do believe that it is a good idea, but that would change the logic\nhandling of the existing code to handle this new \"no answer\" condition and I\ndidn't want to introduce that at this time.\n\nv4 - Whitespace clean up and commit clarifications.\n\nSubmit 3 suggested some clarifications to the commit test and revealed some\nwhitespace errors.\n\nBen Keene (4):\n git-p4: yes/no prompts should sanitize user text\n git-p4: show detailed help when parsing options fail\n git-p4: wrap patchRCSKeywords test to revert changes on failure\n git-p4: failure because of RCS keywords should show help\n\n git-p4.py | 93 +++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 59 insertions(+), 34 deletions(-)\n\n\nbase-commit: ad05a3d8e5a6a06443836b5e40434262d992889a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v4\nPull-Request: https://github.com/git/git/pull/675\n\nRange-diff vs v3:\n\n 1: fff93acf44 ! 1: 6c23cd5684 git-p4: yes/no prompts should sanitize user text\n @@ -10,10 +10,10 @@\n will fail.\n \n Create a new function, prompt(prompt_text) where\n - * promt_text is the text prompt for the user\n - * choices are extracted from the prompt text [.]\n - a single letter surrounded by square brackets\n - is selected as a valid choice.\n + * prompt_text is the text prompt for the user\n + * returns a single character where valid return values are\n + found by inspecting prompt_text for single characters\n + surrounded by square brackets\n \n This new function must prompt the user for input and sanitize it by\n converting the response to a lower case string, trimming leading and\n @@ -26,6 +26,7 @@\n choices, remove the loop from the calling code that handles response\n verification.\n \n + Thanks-to: Denton Liu \n Signed-off-by: Ben Keene \n \n diff --git a/git-p4.py b/git-p4.py\n @@ -119,8 +120,3 @@\n break\n \n chdir(self.oldWorkingDirectory)\n -@@\n - \n - if __name__ == '__main__':\n - main()\n -+\n 2: 5c5c981632 = 2: bfdd3dc517 git-p4: show detailed help when parsing options fail\n 3: c466e79148 = 3: 20f6398693 git-p4: wrap patchRCSKeywords test to revert changes on failure\n 4: 00307c3951 = 4: c78e2e4db1 git-p4: failure because of RCS keywords should show help\n\n-- \ngitgitgadget\n"},{"id":"388113","messageId":"20f63986935cd4ca850d0ecdbb5af5fa0658167b.1576245481.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v4.git.git.1576245481.gitgitgadget@gmail.com","subject":"[PATCH v4 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-13T13:58:00Z","receivedAt":"2019-12-13T20:37:35Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nThe patchRCSKeywords function has the potentional of throwing\nan exception and this would leave files checked out in P4 and partially\nmodified.\n\nAdd a try-catch block around the patchRCSKeywords call and revert\nthe edited files in P4 before leaving the method.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 9165ada2fd..03969052c8 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1953,8 +1953,15 @@ def applyCommit(self, id):\n # disable the read-only bit on windows.\n if self.isWindows and file not in editedFiles:\n os.chmod(file, stat.S_IWRITE)\n- self.patchRCSKeywords(file, kwfiles[file])\n- fixed_rcs_keywords = True\n+ \n+ try:\n+ self.patchRCSKeywords(file, kwfiles[file])\n+ fixed_rcs_keywords = True\n+ except:\n+ # We are throwing an exception, undo all open edits\n+ for f in editedFiles:\n+ p4_revert(f)\n+ raise\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n\n"},{"id":"388114","messageId":"6c23cd56842e76e5c11f32ba59fd7729769ab4b7.1576245481.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v4.git.git.1576245481.gitgitgadget@gmail.com","subject":"[PATCH v4 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-13T13:57:58Z","receivedAt":"2019-12-13T20:37:35Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen prompting the user interactively for direction, the tests are\nnot forgiving of user input format.\n\nFor example, the first query asks for a yes/no response. If the user\nenters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\nwill fail.\n\nCreate a new function, prompt(prompt_text) where\n * prompt_text is the text prompt for the user\n * returns a single character where valid return values are\n found by inspecting prompt_text for single characters\n surrounded by square brackets\n\nThis new function must prompt the user for input and sanitize it by\nconverting the response to a lower case string, trimming leading and\ntrailing spaces, and checking if the first character is in the list\nof choices. If it is, return the first letter.\n\nChange the current references to raw_input() to use this new function.\n\nSince the method requires the returned text to be one of the available\nchoices, remove the loop from the calling code that handles response\nverification.\n\nThanks-to: Denton Liu \nSigned-off-by: Ben Keene \n---\n git-p4.py | 67 ++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 60c73b6a37..3b3f1469a6 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -167,6 +167,21 @@ def die(msg):\n sys.stderr.write(msg + \"\\n\")\n sys.exit(1)\n \n+def prompt(prompt_text):\n+ \"\"\" Prompt the user to choose one of the choices\n+\n+ Choices are identified in the prompt_text by square brackets around\n+ a single letter option.\n+ \"\"\"\n+ choices = set(m.group(1) for m in re.finditer(r\"\\[(.)\\]\", prompt_text))\n+ while True:\n+ response = raw_input(prompt_text).strip().lower()\n+ if not response:\n+ continue\n+ response = response[0]\n+ if response in choices:\n+ return response\n+\n def write_pipe(c, stdin):\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n@@ -1778,12 +1793,11 @@ def edit_template(self, template_file):\n if os.stat(template_file).st_mtime > mtime:\n return True\n \n- while True:\n- response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n- if response == 'y':\n- return True\n- if response == 'n':\n- return False\n+ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n+ if response == 'y':\n+ return True\n+ if response == 'n':\n+ return False\n \n def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n # diff\n@@ -2345,31 +2359,22 @@ def run(self, args):\n \" --prepare-p4-only\")\n break\n if i < last:\n- quit = False\n- while True:\n- # prompt for what to do, or use the option/variable\n- if self.conflict_behavior == \"ask\":\n- print(\"What do you want to do?\")\n- response = raw_input(\"[s]kip this commit but apply\"\n- \" the rest, or [q]uit? \")\n- if not response:\n- continue\n- elif self.conflict_behavior == \"skip\":\n- response = \"s\"\n- elif self.conflict_behavior == \"quit\":\n- response = \"q\"\n- else:\n- die(\"Unknown conflict_behavior '%s'\" %\n- self.conflict_behavior)\n-\n- if response[0] == \"s\":\n- print(\"Skipping this commit, but applying the rest\")\n- break\n- if response[0] == \"q\":\n- print(\"Quitting\")\n- quit = True\n- break\n- if quit:\n+ # prompt for what to do, or use the option/variable\n+ if self.conflict_behavior == \"ask\":\n+ print(\"What do you want to do?\")\n+ response = prompt(\"[s]kip this commit but apply the rest, or [q]uit? \")\n+ elif self.conflict_behavior == \"skip\":\n+ response = \"s\"\n+ elif self.conflict_behavior == \"quit\":\n+ response = \"q\"\n+ else:\n+ die(\"Unknown conflict_behavior '%s'\" %\n+ self.conflict_behavior)\n+\n+ if response == \"s\":\n+ print(\"Skipping this commit, but applying the rest\")\n+ if response == \"q\":\n+ print(\"Quitting\")\n break\n \n chdir(self.oldWorkingDirectory)\n-- \ngitgitgadget\n\n"},{"id":"388115","messageId":"c78e2e4db14ce712375dbd63f9f45335902df2ae.1576245481.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v4.git.git.1576245481.gitgitgadget@gmail.com","subject":"[PATCH v4 4/4] git-p4: failure because of RCS keywords should show help","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-13T13:58:01Z","receivedAt":"2019-12-13T20:37:36Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen applying a commit fails because of RCS keywords, Git\nwill fail the P4 submit. It would help the user if Git suggested that\nthe user set git-p4.attemptRCSCleanup to true.\n\nChange the applyCommit() method that when applying a commit fails\nbecasue of the P4 RCS Keywords, the user should consider setting\ngit-p4.attemptRCSCleanup to true.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 03969052c8..690e5088cc 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1962,6 +1962,14 @@ def applyCommit(self, id):\n for f in editedFiles:\n p4_revert(f)\n raise\n+ else:\n+ # They do not have attemptRCSCleanup set, this might be the fail point\n+ # Check to see if the file has RCS keywords and suggest setting the property.\n+ for file in editedFiles | filesToDelete:\n+ if p4_keywords_regexp_for_file(file) is not None:\n+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n+ break\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n"},{"id":"388150","messageId":"xmqqsgloj9fd.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"20191213014537.GA13064@generichostname","subject":"Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-13T19:46:14Z","receivedAt":"2019-12-13T20:41:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n>> @@ -4170,3 +4175,4 @@ def main():\n>> \n>> if __name__ == '__main__':\n>> main()\n>> +\n>\n> Spurious trailing line. Perhaps we could make GGG error out on\n> whitespace errors before submissions are allowed?\n\nI think you are asking the tool for too much support. \n\nIt may help a lot more if we gave a Makefile target (or two) that\nthe contributors can run before going public. Perhaps\n\n\n\tO=origin/master\n\tupstream-check::\n\t\tgit log -p --check $(O)..\n\nthat can be used like so:\n\n\t$ make upstream-check\n\t$ make O=gitster/next upstream-check\n\nThat way, those who use format-patch+email without GGG or those who\npush to a shared repository to be reviewed among the peer developers\nbefore going public would benefit, not just GGG users.\n\nHmm?\n\n"},{"id":"388182","messageId":"20191213225444.GA31452@generichostname","threadId":"52415","inReplyTo":"6c23cd56842e76e5c11f32ba59fd7729769ab4b7.1576245481.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-13T22:54:44Z","receivedAt":"2019-12-13T22:53:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Ben,\n\nOn Fri, Dec 13, 2019 at 01:57:58PM +0000, Ben Keene via GitGitGadget wrote:\n> From: Ben Keene \n> \n> When prompting the user interactively for direction, the tests are\n> not forgiving of user input format.\n> \n> For example, the first query asks for a yes/no response. If the user\n> enters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\n> will fail.\n> \n> Create a new function, prompt(prompt_text) where\n> * prompt_text is the text prompt for the user\n> * returns a single character where valid return values are\n> found by inspecting prompt_text for single characters\n> surrounded by square brackets\n> \n> This new function must prompt the user for input and sanitize it by\n> converting the response to a lower case string, trimming leading and\n> trailing spaces, and checking if the first character is in the list\n> of choices. If it is, return the first letter.\n> \n> Change the current references to raw_input() to use this new function.\n> \n> Since the method requires the returned text to be one of the available\n> choices, remove the loop from the calling code that handles response\n> verification.\n> \n> Thanks-to: Denton Liu \n\nThanks-to: Denton Liu ?\n\nAnyway, it's probably not worth a reroll. Aside from that, all the\npatches look good to me from a Python perspective.\n\n> Signed-off-by: Ben Keene \n"},{"id":"388237","messageId":"nycvar.QRO.7.76.6.1912152125390.46@tvgsbejvaqbjf.bet","threadId":"52415","inReplyTo":"xmqqsgloj9fd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-15T20:30:40Z","receivedAt":"2019-12-15T20:31:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 13 Dec 2019, Junio C Hamano wrote:\n\n> Denton Liu writes:\n>\n> >> @@ -4170,3 +4175,4 @@ def main():\n> >>\n> >> if __name__ == '__main__':\n> >> main()\n> >> +\n> >\n> > Spurious trailing line. Perhaps we could make GGG error out on\n> > whitespace errors before submissions are allowed?\n>\n> I think you are asking the tool for too much support.\n>\n> It may help a lot more if we gave a Makefile target (or two) that\n> the contributors can run before going public. Perhaps\n>\n>\n> \tO=origin/master\n> \tupstream-check::\n> \t\tgit log -p --check $(O)..\n>\n> that can be used like so:\n>\n> \t$ make upstream-check\n> \t$ make O=gitster/next upstream-check\n>\n> That way, those who use format-patch+email without GGG or those who\n> push to a shared repository to be reviewed among the peer developers\n> before going public would benefit, not just GGG users.\n>\n> Hmm?\n\nI'd like that a lot, _and_ I think GitGitGadget could learn the trick of\nrunning that `Makefile` target and report failures back to the PR\n_especially_ because GitGitGadget knows the base branch of the PR.\n\nIn my opinion, there is a lot of value in having GitGitGadget doing this,\nas new contributors are likely to miss such a helpful `Makefile` target.\nFor example, I vividly remember when I contributed to cURL for the first\ntime and had totally and completely missed the invocation `make -C src\nchecksrc` to help me get the code into the preferred shape.\n\nCiao,\nDscho\n"},{"id":"388252","messageId":"750db524-8c99-dd9c-9fef-5936ce548edb@gmail.com","threadId":"52415","inReplyTo":"20191213225444.GA31452@generichostname","subject":"Re: [PATCH v4 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene","fromEmail":"seraphire@gmail.com","sentAt":"2019-12-16T13:53:16Z","receivedAt":"2019-12-16T13:53:20Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"\nOn 12/13/2019 5:54 PM, Denton Liu wrote:\n> Hi Ben,\n>\n> On Fri, Dec 13, 2019 at 01:57:58PM +0000, Ben Keene via GitGitGadget wrote:\n>> From: Ben Keene \n>> ...\n>> Thanks-to: Denton Liu \n> Thanks-to: Denton Liu ?\n>\n> Anyway, it's probably not worth a reroll. Aside from that, all the\n> patches look good to me from a Python perspective.\n\nI was /so/ close!  I'll reroll it anyway.\n\n>> Signed-off-by: Ben Keene \n"},{"id":"388253","messageId":"pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v4.git.git.1576245481.gitgitgadget@gmail.com","subject":"[PATCH v5 0/4] git-p4: Usability enhancements","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-16T14:02:18Z","receivedAt":"2019-12-16T14:02:27Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"Some user interaction with git-p4 is not as user-friendly as the rest of the\nGit ecosystem. \n\nHere are three areas that can be improved on:\n\n1) When a patch fails and the user is prompted, there is no sanitization of\nthe user input so for a \"yes/no\" question, if the user enters \"YES\" instead\nof a lowercase \"y\", they will be re-prompted to enter their answer. \n\nCommit 1 addresses this by sanitizing the user text by trimming and\nlowercasing their input before testing. Now \"YES\" will succeed!\n\n2) If the command line arguments are incorrect for git-p4, the program\nreports that there was a syntax error, but doesn't show what the correct\nsyntax is.\n\nCommit 2 displays the context help for the failed command.\n\n3) If Git generates an error while attempting to clean up the RCS Keyword\nexpansions, it currently leaves P4 in an invalid state. Files that were\nchecked out by P4 are not revereted.\n\nCommit 3 adds and exception handler that catches this condition and issues a\nP4 Revert for the files that were previously edited.\n\n4) Git can handle scraping the RCS Keyword expansions out of source files\nwhen it is preparing to submit them to P4. However, if the config value\n\"git-p4.attemptRCSCleanup\" isn't set, it will just report that it fails.\n\nCommit 4 adds a helpful suggestion, that the user might want to set\ngit-p4.attemptRCSCleanup.\n\nRevisions\n=========\n\nv3 - Implemented the various suggestions from Luke and Denton.\n\nI did not add additional exception handling for the EOFError in the prompt\nmethod. I do believe that it is a good idea, but that would change the logic\nhandling of the existing code to handle this new \"no answer\" condition and I\ndidn't want to introduce that at this time.\n\nv4 - Whitespace clean up and commit clarifications.\n\nSubmit 3 suggested some clarifications to the commit test and revealed some\nwhitespace errors.\n\nv5 - Fixed typo in a commit message. (Invalid attribute to Thanks-to: Denton\nLiu liu.denton@gmail.com [liu.denton@gmail.com])\n\nBen Keene (4):\n git-p4: yes/no prompts should sanitize user text\n git-p4: show detailed help when parsing options fail\n git-p4: wrap patchRCSKeywords test to revert changes on failure\n git-p4: failure because of RCS keywords should show help\n\n git-p4.py | 93 +++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 59 insertions(+), 34 deletions(-)\n\n\nbase-commit: ad05a3d8e5a6a06443836b5e40434262d992889a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v5\nPull-Request: https://github.com/git/git/pull/675\n\nRange-diff vs v4:\n\n 1: 6c23cd5684 ! 1: 7e0145fa32 git-p4: yes/no prompts should sanitize user text\n @@ -26,7 +26,7 @@\n choices, remove the loop from the calling code that handles response\n verification.\n \n - Thanks-to: Denton Liu \n + Thanks-to: Denton Liu \n Signed-off-by: Ben Keene \n \n diff --git a/git-p4.py b/git-p4.py\n 2: bfdd3dc517 = 2: 4960d1fa22 git-p4: show detailed help when parsing options fail\n 3: 20f6398693 = 3: 81a09a1228 git-p4: wrap patchRCSKeywords test to revert changes on failure\n 4: c78e2e4db1 = 4: 4c4b783fd5 git-p4: failure because of RCS keywords should show help\n\n-- \ngitgitgadget\n"},{"id":"388254","messageId":"7e0145fa321647e14ecb886707025ac3f516a137.1576504942.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com","subject":"[PATCH v5 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-16T14:02:19Z","receivedAt":"2019-12-16T14:02:30Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen prompting the user interactively for direction, the tests are\nnot forgiving of user input format.\n\nFor example, the first query asks for a yes/no response. If the user\nenters the full word \"yes\" or \"no\" or enters a capital \"Y\" the test\nwill fail.\n\nCreate a new function, prompt(prompt_text) where\n * prompt_text is the text prompt for the user\n * returns a single character where valid return values are\n found by inspecting prompt_text for single characters\n surrounded by square brackets\n\nThis new function must prompt the user for input and sanitize it by\nconverting the response to a lower case string, trimming leading and\ntrailing spaces, and checking if the first character is in the list\nof choices. If it is, return the first letter.\n\nChange the current references to raw_input() to use this new function.\n\nSince the method requires the returned text to be one of the available\nchoices, remove the loop from the calling code that handles response\nverification.\n\nThanks-to: Denton Liu \nSigned-off-by: Ben Keene \n---\n git-p4.py | 67 ++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 60c73b6a37..3b3f1469a6 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -167,6 +167,21 @@ def die(msg):\n sys.stderr.write(msg + \"\\n\")\n sys.exit(1)\n \n+def prompt(prompt_text):\n+ \"\"\" Prompt the user to choose one of the choices\n+\n+ Choices are identified in the prompt_text by square brackets around\n+ a single letter option.\n+ \"\"\"\n+ choices = set(m.group(1) for m in re.finditer(r\"\\[(.)\\]\", prompt_text))\n+ while True:\n+ response = raw_input(prompt_text).strip().lower()\n+ if not response:\n+ continue\n+ response = response[0]\n+ if response in choices:\n+ return response\n+\n def write_pipe(c, stdin):\n if verbose:\n sys.stderr.write('Writing pipe: %s\\n' % str(c))\n@@ -1778,12 +1793,11 @@ def edit_template(self, template_file):\n if os.stat(template_file).st_mtime > mtime:\n return True\n \n- while True:\n- response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n- if response == 'y':\n- return True\n- if response == 'n':\n- return False\n+ response = prompt(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n+ if response == 'y':\n+ return True\n+ if response == 'n':\n+ return False\n \n def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n # diff\n@@ -2345,31 +2359,22 @@ def run(self, args):\n \" --prepare-p4-only\")\n break\n if i < last:\n- quit = False\n- while True:\n- # prompt for what to do, or use the option/variable\n- if self.conflict_behavior == \"ask\":\n- print(\"What do you want to do?\")\n- response = raw_input(\"[s]kip this commit but apply\"\n- \" the rest, or [q]uit? \")\n- if not response:\n- continue\n- elif self.conflict_behavior == \"skip\":\n- response = \"s\"\n- elif self.conflict_behavior == \"quit\":\n- response = \"q\"\n- else:\n- die(\"Unknown conflict_behavior '%s'\" %\n- self.conflict_behavior)\n-\n- if response[0] == \"s\":\n- print(\"Skipping this commit, but applying the rest\")\n- break\n- if response[0] == \"q\":\n- print(\"Quitting\")\n- quit = True\n- break\n- if quit:\n+ # prompt for what to do, or use the option/variable\n+ if self.conflict_behavior == \"ask\":\n+ print(\"What do you want to do?\")\n+ response = prompt(\"[s]kip this commit but apply the rest, or [q]uit? \")\n+ elif self.conflict_behavior == \"skip\":\n+ response = \"s\"\n+ elif self.conflict_behavior == \"quit\":\n+ response = \"q\"\n+ else:\n+ die(\"Unknown conflict_behavior '%s'\" %\n+ self.conflict_behavior)\n+\n+ if response == \"s\":\n+ print(\"Skipping this commit, but applying the rest\")\n+ if response == \"q\":\n+ print(\"Quitting\")\n break\n \n chdir(self.oldWorkingDirectory)\n-- \ngitgitgadget\n\n"},{"id":"388255","messageId":"4c4b783fd52252200a607c83517e126988e67f19.1576504942.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com","subject":"[PATCH v5 4/4] git-p4: failure because of RCS keywords should show help","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-16T14:02:22Z","receivedAt":"2019-12-16T14:02:32Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen applying a commit fails because of RCS keywords, Git\nwill fail the P4 submit. It would help the user if Git suggested that\nthe user set git-p4.attemptRCSCleanup to true.\n\nChange the applyCommit() method that when applying a commit fails\nbecasue of the P4 RCS Keywords, the user should consider setting\ngit-p4.attemptRCSCleanup to true.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 03969052c8..690e5088cc 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1962,6 +1962,14 @@ def applyCommit(self, id):\n for f in editedFiles:\n p4_revert(f)\n raise\n+ else:\n+ # They do not have attemptRCSCleanup set, this might be the fail point\n+ # Check to see if the file has RCS keywords and suggest setting the property.\n+ for file in editedFiles | filesToDelete:\n+ if p4_keywords_regexp_for_file(file) is not None:\n+ print(\"At least one file in this commit has RCS Keywords that may be causing problems. \")\n+ print(\"Consider:\\ngit config git-p4.attemptRCSCleanup true\")\n+ break\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n"},{"id":"388256","messageId":"81a09a122836703a3cbc3239a66c6e5c84ea2c74.1576504942.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com","subject":"[PATCH v5 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-16T14:02:21Z","receivedAt":"2019-12-16T14:02:32Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nThe patchRCSKeywords function has the potentional of throwing\nan exception and this would leave files checked out in P4 and partially\nmodified.\n\nAdd a try-catch block around the patchRCSKeywords call and revert\nthe edited files in P4 before leaving the method.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 9165ada2fd..03969052c8 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1953,8 +1953,15 @@ def applyCommit(self, id):\n # disable the read-only bit on windows.\n if self.isWindows and file not in editedFiles:\n os.chmod(file, stat.S_IWRITE)\n- self.patchRCSKeywords(file, kwfiles[file])\n- fixed_rcs_keywords = True\n+ \n+ try:\n+ self.patchRCSKeywords(file, kwfiles[file])\n+ fixed_rcs_keywords = True\n+ except:\n+ # We are throwing an exception, undo all open edits\n+ for f in editedFiles:\n+ p4_revert(f)\n+ raise\n \n if fixed_rcs_keywords:\n print(\"Retrying the patch with RCS keywords cleaned up\")\n-- \ngitgitgadget\n\n"},{"id":"388257","messageId":"4960d1fa223fc00def148cdcade46855e0772a20.1576504942.git.gitgitgadget@gmail.com","threadId":"52415","inReplyTo":"pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com","subject":"[PATCH v5 2/4] git-p4: show detailed help when parsing options fail","fromName":"Ben Keene via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-16T14:02:20Z","receivedAt":"2019-12-16T14:02:33Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"From: Ben Keene \n\nWhen a user provides invalid parameters to git-p4, the program\nreports the failure but does not provide the correct command syntax.\n\nAdd an exception handler to the command-line argument parser to display\nthe command's specific command line parameter syntax when an exception\nis thrown. Rethrow the exception so the current behavior is retained.\n\nSigned-off-by: Ben Keene \n---\n git-p4.py | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 3b3f1469a6..9165ada2fd 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -4145,7 +4145,12 @@ def main():\n description = cmd.description,\n formatter = HelpFormatter())\n \n- (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ try:\n+ (cmd, args) = parser.parse_args(sys.argv[2:], cmd);\n+ except:\n+ parser.print_help()\n+ raise\n+\n global verbose\n verbose = cmd.verbose\n if cmd.needsGit:\n-- \ngitgitgadget\n\n"},{"id":"388289","messageId":"xmqq1rt4kvgf.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"nycvar.QRO.7.76.6.1912152125390.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-16T17:54:08Z","receivedAt":"2019-12-16T18:48:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Fri, 13 Dec 2019, Junio C Hamano wrote:\n>\n>> Denton Liu writes:\n>>\n>> >> @@ -4170,3 +4175,4 @@ def main():\n>> >>\n>> >> if __name__ == '__main__':\n>> >> main()\n>> >> +\n>> >\n>> > Spurious trailing line. Perhaps we could make GGG error out on\n>> > whitespace errors before submissions are allowed?\n>>\n>> I think you are asking the tool for too much support.\n>>\n>> It may help a lot more if we gave a Makefile target (or two) that\n>> the contributors can run before going public. Perhaps\n>>\n>>\n>> \tO=origin/master\n>> \tupstream-check::\n>> \t\tgit log -p --check $(O)..\n>>\n>> that can be used like so:\n>>\n>> \t$ make upstream-check\n>> \t$ make O=gitster/next upstream-check\n>>\n>> That way, those who use format-patch+email without GGG or those who\n>> push to a shared repository to be reviewed among the peer developers\n>> before going public would benefit, not just GGG users.\n>>\n>> Hmm?\n>\n> I'd like that a lot, _and_ I think GitGitGadget could learn the trick of\n> running that `Makefile` target and report failures back to the PR\n> _especially_ because GitGitGadget knows the base branch of the PR.\n\nYup. That's the right approach---provide a common base that\neverybody can use, and then teach the tool help its users with it.\nThat way, the improvement won't be limited to the audience of a\nsingle tool.\n"},{"id":"388292","messageId":"0ce91fe5-df41-b4de-6e74-036c346df577@gmail.com","threadId":"52415","inReplyTo":"nycvar.QRO.7.76.6.1912152125390.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text","fromName":"Ben Keene","fromEmail":"seraphire@gmail.com","sentAt":"2019-12-16T19:11:00Z","receivedAt":"2019-12-16T19:11:04Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"\nOn 12/15/2019 3:30 PM, Johannes Schindelin wrote:\n> Hi Junio,\n>\n> On Fri, 13 Dec 2019, Junio C Hamano wrote:\n>\n>> Denton Liu writes:\n>>\n>>>> @@ -4170,3 +4175,4 @@ def main():\n>>>>\n>>>> if __name__ == '__main__':\n>>>> main()\n>>>> +\n>>> Spurious trailing line. Perhaps we could make GGG error out on\n>>> whitespace errors before submissions are allowed?\n>> I think you are asking the tool for too much support.\n>>\n>> It may help a lot more if we gave a Makefile target (or two) that\n>> the contributors can run before going public. Perhaps\n>>\n>>\n>> \tO=origin/master\n>> \tupstream-check::\n>> \t\tgit log -p --check $(O)..\n>>\n>> that can be used like so:\n>>\n>> \t$ make upstream-check\n>> \t$ make O=gitster/next upstream-check\n>>\n>> That way, those who use format-patch+email without GGG or those who\n>> push to a shared repository to be reviewed among the peer developers\n>> before going public would benefit, not just GGG users.\n>>\n>> Hmm?\n> I'd like that a lot, _and_ I think GitGitGadget could learn the trick of\n> running that `Makefile` target and report failures back to the PR\n> _especially_ because GitGitGadget knows the base branch of the PR.\n>\n> In my opinion, there is a lot of value in having GitGitGadget doing this,\n> as new contributors are likely to miss such a helpful `Makefile` target.\n> For example, I vividly remember when I contributed to cURL for the first\n> time and had totally and completely missed the invocation `make -C src\n> checksrc` to help me get the code into the preferred shape.\n>\n> Ciao,\n> Dscho\n\nSame here for me.  My entry point into submissions was through GGG.  The\nmore suggestions it can offer prior to \"/submit\"ting code the easier it\nwould have been for me and the less noise I would have brought to the\nmailing list.\n\n- Ben\n\n"},{"id":"388296","messageId":"xmqq8sncj991.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/4] git-p4: Usability enhancements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-16T20:39:06Z","receivedAt":"2019-12-16T20:39:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ben Keene via GitGitGadget\" writes:\n\n> Some user interaction with git-p4 is not as user-friendly as the rest of the\n> ...\n>\n> Ben Keene (4):\n> git-p4: yes/no prompts should sanitize user text\n> git-p4: show detailed help when parsing options fail\n> git-p4: wrap patchRCSKeywords test to revert changes on failure\n> git-p4: failure because of RCS keywords should show help\n\nThe reviews on the list seem to be in favor of these and I didn't\nsee much wrong (I think I fixed up an indented empty line) in the\nseries. I'd appreciate a blessing from a git-p4 expert, though.\n\nThanks.\n"},{"id":"388706","messageId":"CAE5ih7-ptmmb2UurBw+k+2ZjZQuOkLJ3c-eBoOXKrPX0CJeErA@mail.gmail.com","threadId":"52415","inReplyTo":"xmqq8sncj991.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 0/4] git-p4: Usability enhancements","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2019-12-21T10:19:03Z","receivedAt":"2019-12-21T10:19:18Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Mon, 16 Dec 2019 at 20:39, Junio C Hamano wrote:\n>\n> \"Ben Keene via GitGitGadget\" writes:\n>\n> > Some user interaction with git-p4 is not as user-friendly as the rest of the\n> > ...\n> >\n> > Ben Keene (4):\n> > git-p4: yes/no prompts should sanitize user text\n> > git-p4: show detailed help when parsing options fail\n> > git-p4: wrap patchRCSKeywords test to revert changes on failure\n> > git-p4: failure because of RCS keywords should show help\n>\n> The reviews on the list seem to be in favor of these and I didn't\n> see much wrong (I think I fixed up an indented empty line) in the\n> series. I'd appreciate a blessing from a git-p4 expert, though.\n>\n\n$ git log --reverse --oneline --abbrev-commit\norigin/maint..origin/bk/p4-misc-usability\ne2aed5fd5b git-p4: yes/no prompts should sanitize user text\n - looks good to me\n\n608e380502 git-p4: show detailed help when parsing options fail\n - also looks good to me\n\nc4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure\n - why not just catch the exception, and then drop out of the \"if-\"\ncondition and fall into the cleanup section at the bottom of that\nfunction (line 1976)? As it stands, this is duplicating the cleanup\ncode now.\n\n89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of\nRCS keywords should show help\n - strictly speaking, the code does not actually check if there *are*\nany RCS keywords, it just checks if the filetype means that RCS kws\n*would* be expanded *if* they were present. The conflict might be just\nbecause....there's a conflict. As it stands this will be giving\nmisleading advice. I would get it to check to see if there really are\nany RCS keywords in the file.\n\n> Thanks.\n"},{"id":"388910","messageId":"xmqqh81omd5m.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"CAE5ih7-ptmmb2UurBw+k+2ZjZQuOkLJ3c-eBoOXKrPX0CJeErA@mail.gmail.com","subject":"Re: [PATCH v5 0/4] git-p4: Usability enhancements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-25T19:13:41Z","receivedAt":"2019-12-25T19:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> $ git log --reverse --oneline --abbrev-commit\n> origin/maint..origin/bk/p4-misc-usability\n> e2aed5fd5b git-p4: yes/no prompts should sanitize user text\n> - looks good to me\n>\n> 608e380502 git-p4: show detailed help when parsing options fail\n> - also looks good to me\n>\n> c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure\n> - why not just catch the exception, and then drop out of the \"if-\"\n> condition and fall into the cleanup section at the bottom of that\n> function (line 1976)? As it stands, this is duplicating the cleanup\n> code now.\n>\n> 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of\n> RCS keywords should show help\n> - strictly speaking, the code does not actually check if there *are*\n> any RCS keywords, it just checks if the filetype means that RCS kws\n> *would* be expanded *if* they were present. The conflict might be just\n> because....there's a conflict. As it stands this will be giving\n> misleading advice. I would get it to check to see if there really are\n> any RCS keywords in the file.\n\nThanks. Ben, let's keep the first two and discard the rest for now,\nwhich can later be replaced with updated ones.\n"},{"id":"389163","messageId":"c95fe073-9bea-9dcf-4579-e9125cc55f39@gmail.com","threadId":"52415","inReplyTo":"xmqqh81omd5m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 0/4] git-p4: Usability enhancements","fromName":"Ben Keene","fromEmail":"seraphire@gmail.com","sentAt":"2020-01-02T13:50:50Z","receivedAt":"2020-01-02T13:50:54Z","isPatch":true,"sender":{"key":"seraphire@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22523774?v=4"},"body":"\nOn 12/25/2019 2:13 PM, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> $ git log --reverse --oneline --abbrev-commit\n>> origin/maint..origin/bk/p4-misc-usability\n>> e2aed5fd5b git-p4: yes/no prompts should sanitize user text\n>> - looks good to me\n>>\n>> 608e380502 git-p4: show detailed help when parsing options fail\n>> - also looks good to me\n>>\n>> c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure\n>> - why not just catch the exception, and then drop out of the \"if-\"\n>> condition and fall into the cleanup section at the bottom of that\n>> function (line 1976)? As it stands, this is duplicating the cleanup\n>> code now.\n>>\n>> 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of\n>> RCS keywords should show help\n>> - strictly speaking, the code does not actually check if there *are*\n>> any RCS keywords, it just checks if the filetype means that RCS kws\n>> *would* be expanded *if* they were present. The conflict might be just\n>> because....there's a conflict. As it stands this will be giving\n>> misleading advice. I would get it to check to see if there really are\n>> any RCS keywords in the file.\n> Thanks. Ben, let's keep the first two and discard the rest for now,\n> which can later be replaced with updated ones.\nThat works for me.  So, are there any changes that I should make at\nthis time, or just let the rest die off?\n"},{"id":"389181","messageId":"xmqqblrledof.fsf@gitster-ct.c.googlers.com","threadId":"52415","inReplyTo":"c95fe073-9bea-9dcf-4579-e9125cc55f39@gmail.com","subject":"Re: [PATCH v5 0/4] git-p4: Usability enhancements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-02T21:44:32Z","receivedAt":"2020-01-02T21:44:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Keene writes:\n\n>> Thanks. Ben, let's keep the first two and discard the rest for now,\n>> which can later be replaced with updated ones.\n> That works for me. So, are there any changes that I should make at\n> this time, or just let the rest die off?\n\nI don't think of any, from this side. You can of course spend time\non salvaging and polishing these remaining patches for resubmission\nin future cycle(s).\n\nThanks.\n"}]}