[v4,09/11] git-p4: Add usability enhancements
diff mbox series

Message ID 4fc49313f0d68a913ad19085ddb337ac4c18d0fe.1575498578.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4.py: Cast byte strings to unicode strings in python3
Related show

Commit Message

Derrick Stolee via GitGitGadget Dec. 4, 2019, 10:29 p.m. UTC
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.

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.

Signed-off-by: Ben Keene <seraphire@gmail.com>
(cherry picked from commit 1fab571664f5b6ad4ef321199f52615a32a9f8c7)
---
 git-p4.py | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Dec. 5, 2019, 2:04 p.m. UTC | #1
"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:
Ben Keene Dec. 5, 2019, 3:40 p.m. UTC | #2
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:

Patch
diff mbox series

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: