[1/3] git-p4: [usability] yes/no prompts should sanitize user text
diff mbox series

Message ID e721cdaa008263b896c1d162e411c4e7a04c5710.1575901009.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4: Usability enhancements
Related show

Commit Message

Heba Waly via GitGitGadget Dec. 9, 2019, 2:16 p.m. UTC
From: Ben Keene <seraphire@gmail.com>

When prompting the user interactively for direction, the tests are
not forgiving of user input format.

For example, the first query asks for a yes/no response. If the user
enters the full word "yes" or "no" or enters a capital "Y" the test
will fail.

Create a new function, prompt(prompt_text, choices) where
  * promt_text is the text prompt for the user
  * is a list of lower-case, single letter choices.
This new function must  prompt the user for input and sanitize it by
converting the response to a lower case string, trimming leading and
trailing spaces, and checking if the first character is in the list
of choices. If it is, return the first letter.

Change the current references to raw_input() to use this new function.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Dec. 9, 2019, 10 p.m. UTC | #1
"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
>
> When prompting the user interactively for direction, the tests are
> not forgiving of user input format.
>
> For example, the first query asks for a yes/no response. If the user
> enters the full word "yes" or "no" or enters a capital "Y" the test
> will fail.
>
> Create a new function, prompt(prompt_text, choices) where
>   * promt_text is the text prompt for the user
>   * is a list of lower-case, single letter choices.
> This new function must  prompt the user for input and sanitize it by
> converting the response to a lower case string, trimming leading and
> trailing spaces, and checking if the first character is in the list
> of choices. If it is, return the first letter.
>
> Change the current references to raw_input() to use this new function.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  git-p4.py | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 60c73b6a37..0fa562fac9 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -167,6 +167,17 @@ def die(msg):
>          sys.stderr.write(msg + "\n")
>          sys.exit(1)
>  
> +def prompt(prompt_text, choices = []):
> +    """ Prompt the user to choose one of the choices
> +    """
> +    while True:
> +        response = raw_input(prompt_text).strip().lower()
> +        if len(response) == 0:
> +            continue
> +        response = response[0]
> +        if response in choices:
> +            return response

I think this is a strict improvement compared to the original, but
the new loop makes me wonder if we need to worry more about getting
EOF while calling raw_input() here.  I am assuming that we would get
EOFError either way so this is no worse/better than the status quo,
and we can keep it outside the topic (even though it may be a good
candidate for a low-hanging fruit for newbies).

>  def write_pipe(c, stdin):
>      if verbose:
>          sys.stderr.write('Writing pipe: %s\n' % str(c))
> @@ -1779,7 +1790,7 @@ 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 = prompt("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ", ["y", "n"])
>              if response == 'y':
>                  return True
>              if response == 'n':
> @@ -2350,8 +2361,8 @@ def run(self, args):
>                          # prompt for what to do, or use the option/variable
>                          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? ")
> +                            response = prompt("[s]kip this commit but apply"
> +                                                 " the rest, or [q]uit? ", ["s", "q"])
>                              if not response:
>                                  continue
>                          elif self.conflict_behavior == "skip":
Ben Keene Dec. 10, 2019, 2:26 p.m. UTC | #2
On 12/9/2019 5:00 PM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> When prompting the user interactively for direction, the tests are
>> not forgiving of user input format.
>>
>> For example, the first query asks for a yes/no response. If the user
>> enters the full word "yes" or "no" or enters a capital "Y" the test
>> will fail.
>>
>> Create a new function, prompt(prompt_text, choices) where
>>    * promt_text is the text prompt for the user
>>    * is a list of lower-case, single letter choices.
>> This new function must  prompt the user for input and sanitize it by
>> converting the response to a lower case string, trimming leading and
>> trailing spaces, and checking if the first character is in the list
>> of choices. If it is, return the first letter.
>>
>> Change the current references to raw_input() to use this new function.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> ---
>>
>> +def prompt(prompt_text, choices = []):
>> +    """ Prompt the user to choose one of the choices
>> +    """
>> +    while True:
>> +        response = raw_input(prompt_text).strip().lower()
>> +        if len(response) == 0:
>> +            continue
>> +        response = response[0]
>> +        if response in choices:
>> +            return response
> I think this is a strict improvement compared to the original, but
> the new loop makes me wonder if we need to worry more about getting
> EOF while calling raw_input() here.  I am assuming that we would get
> EOFError either way so this is no worse/better than the status quo,
> and we can keep it outside the topic (even though it may be a good
> candidate for a low-hanging fruit for newbies).
That is a good catch.  What should we expect the default behavior
to be in these two questions if the EOFError occurs?  I would think
that we should extend this to an abort of the process?
> response = prompt("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ", ["y", "n"])
> response = prompt("[s]kip this commit but apply the rest, or [q]uit? ", ["s", "q"])

Should a quit be added to the first prompt and have those be the 
defaults on EOFError?

Patch
diff mbox series

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..0fa562fac9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -167,6 +167,17 @@  def die(msg):
         sys.stderr.write(msg + "\n")
         sys.exit(1)
 
+def prompt(prompt_text, choices = []):
+    """ Prompt the user to choose one of the choices
+    """
+    while True:
+        response = raw_input(prompt_text).strip().lower()
+        if len(response) == 0:
+            continue
+        response = response[0]
+        if response in choices:
+            return response
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))
@@ -1779,7 +1790,7 @@  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 = prompt("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ", ["y", "n"])
             if response == 'y':
                 return True
             if response == 'n':
@@ -2350,8 +2361,8 @@  def run(self, args):
                         # prompt for what to do, or use the option/variable
                         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? ")
+                            response = prompt("[s]kip this commit but apply"
+                                                 " the rest, or [q]uit? ", ["s", "q"])
                             if not response:
                                 continue
                         elif self.conflict_behavior == "skip":