| Message ID | 4fc49313f0d68a913ad19085ddb337ac4c18d0fe.1575498578.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-p4.py: Cast byte strings to unicode strings in python3 | expand |
"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Ben Keene <seraphire@gmail.com> > > Issue: when prompting the user with raw_input, the tests are not forgiving of user input. For example, on the first query asks for a yes/no response. If the user enters the full word "yes" or "no" the test will fail. Additionally, offer the suggestion of setting git-p4.attemptRCSCleanup when applying a commit fails because of RCS keywords. Both of these changes are usability enhancement suggestions. Drop "Issue: " and upcase "when" that follows. The rest of the paragraph reads a lot better without it as a human friendly description. "are usability enhancement suggestions"??? Leaves readers wonder who suggested them, or you are suggesting but are willing the change to be dropped, or what. Be a bit more assertive if you want to say that you believe these two would improve usability. > Change the code prompting the user for input to sanitize the user input before checking the response by asking the response as a lower case string, trimming leading/trailing spaces, and returning the first character. > > Change the applyCommit() method that when applying a commit fails becasue of the P4 RCS Keywords, the user should consider setting git-p4.attemptRCSCleanup. s/becasue/because/; I have a feeling that these two may be worth doing but are totally separate issues, deserving two separate commits. Is there a good reason why these two must go hand-in-hand? > Signed-off-by: Ben Keene <seraphire@gmail.com> > (cherry picked from commit 1fab571664f5b6ad4ef321199f52615a32a9f8c7) > --- > git-p4.py | 31 ++++++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/git-p4.py b/git-p4.py > index f7c0ef0c53..f13e4645a3 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -1909,7 +1909,8 @@ def edit_template(self, template_file): > return True > > while True: > - response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ") > + response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ").lower() \ > + .strip()[0] You could have saved the patch by doing + .lower().strip()[0] instead, no? I wonder if it would be better to write a thin wrapper around raw_input() that does the "downcase and take the first meaningful letter" thing for you and call it prompt() or something like that. > @@ -4327,7 +4343,12 @@ def main(): > description = cmd.description, > formatter = HelpFormatter()) > > - (cmd, args) = parser.parse_args(sys.argv[2:], cmd); > + try: > + (cmd, args) = parser.parse_args(sys.argv[2:], cmd); > + except: > + parser.print_help() > + raise > + This change may be a good idea to give help text when the command line parsing fails, but a good change deserves to be explained. I do not think I saw any mention of it in the proposed log message, though. > global verbose > verbose = cmd.verbose > if cmd.needsGit:
On 12/5/2019 9:04 AM, Junio C Hamano wrote: > "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Ben Keene <seraphire@gmail.com> >> >> Issue: when prompting the user with raw_input, the tests are not forgiving of user input. For example, on the first query asks for a yes/no response. If the user enters the full word "yes" or "no" the test will fail. Additionally, offer the suggestion of setting git-p4.attemptRCSCleanup when applying a commit fails because of RCS keywords. Both of these changes are usability enhancement suggestions. > Drop "Issue: " and upcase "when" that follows. The rest of the > paragraph reads a lot better without it as a human friendly > description. > > "are usability enhancement suggestions"??? Leaves readers wonder > who suggested them, or you are suggesting but are willing the change > to be dropped, or what. Be a bit more assertive if you want to say > that you believe these two would improve usability. Thank you and I reworked my submissions. I'm moving them to a separate PR and will split the commit into 3 separate commits. >> Change the code prompting the user for input to sanitize the user input before checking the response by asking the response as a lower case string, trimming leading/trailing spaces, and returning the first character. >> >> Change the applyCommit() method that when applying a commit fails becasue of the P4 RCS Keywords, the user should consider setting git-p4.attemptRCSCleanup. > s/becasue/because/; > > I have a feeling that these two may be worth doing but are totally > separate issues, deserving two separate commits. Is there a good > reason why these two must go hand-in-hand? > Good idea, and I split them out. >> Signed-off-by: Ben Keene <seraphire@gmail.com> >> (cherry picked from commit 1fab571664f5b6ad4ef321199f52615a32a9f8c7) >> --- >> git-p4.py | 31 ++++++++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index f7c0ef0c53..f13e4645a3 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -1909,7 +1909,8 @@ def edit_template(self, template_file): >> return True >> >> while True: >> - response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ") >> + response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ").lower() \ >> + .strip()[0] > You could have saved the patch by doing > > + .lower().strip()[0] > > instead, no? > > I wonder if it would be better to write a thin wrapper around raw_input() > that does the "downcase and take the first meaningful letter" thing > for you and call it prompt() or something like that. I created a new function prompt() as you suggested. >> @@ -4327,7 +4343,12 @@ def main(): >> description = cmd.description, >> formatter = HelpFormatter()) >> >> - (cmd, args) = parser.parse_args(sys.argv[2:], cmd); >> + try: >> + (cmd, args) = parser.parse_args(sys.argv[2:], cmd); >> + except: >> + parser.print_help() >> + raise >> + > This change may be a good idea to give help text when the command > line parsing fails, but a good change deserves to be explained. I > do not think I saw any mention of it in the proposed log message, > though. Yes, you're right. I split this out into a separate commit as well and gave it a place or prominence. >> global verbose >> verbose = cmd.verbose >> if cmd.needsGit:
diff --git a/git-p4.py b/git-p4.py index f7c0ef0c53..f13e4645a3 100755 --- a/git-p4.py +++ b/git-p4.py @@ -1909,7 +1909,8 @@ def edit_template(self, template_file): return True while True: - response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ") + response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ").lower() \ + .strip()[0] if response == 'y': return True if response == 'n': @@ -2069,8 +2070,23 @@ def applyCommit(self, id): # disable the read-only bit on windows. if self.isWindows and file not in editedFiles: os.chmod(file, stat.S_IWRITE) - self.patchRCSKeywords(file, kwfiles[file]) - fixed_rcs_keywords = True + + try: + self.patchRCSKeywords(file, kwfiles[file]) + fixed_rcs_keywords = True + except: + # We are throwing an exception, undo all open edits + for f in editedFiles: + p4_revert(f) + raise + else: + # They do not have attemptRCSCleanup set, this might be the fail point + # Check to see if the file has RCS keywords and suggest setting the property. + for file in editedFiles | filesToDelete: + if p4_keywords_regexp_for_file(file) != None: + print("At least one file in this commit has RCS Keywords that may be causing problems. ") + print("Consider:\ngit config git-p4.attemptRCSCleanup true") + break if fixed_rcs_keywords: print("Retrying the patch with RCS keywords cleaned up") @@ -2481,7 +2497,7 @@ def run(self, args): if self.conflict_behavior == "ask": print("What do you want to do?") response = raw_input("[s]kip this commit but apply" - " the rest, or [q]uit? ") + " the rest, or [q]uit? ").lower().strip()[0] if not response: continue elif self.conflict_behavior == "skip": @@ -4327,7 +4343,12 @@ def main(): description = cmd.description, formatter = HelpFormatter()) - (cmd, args) = parser.parse_args(sys.argv[2:], cmd); + try: + (cmd, args) = parser.parse_args(sys.argv[2:], cmd); + except: + parser.print_help() + raise + global verbose verbose = cmd.verbose if cmd.needsGit: