[v2,1/4] git-p4: yes/no prompts should sanitize user text

Commit Message

Johannes Schindelin via GitGitGadget Dec. 10, 2019, 3:22 p.m. UTC

From: Ben Keene <[e-mail]>

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 <[e-mail]>
---
 git-p4.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Denton Liu Dec. 11, 2019, 11:52 a.m. UTC | #1

Hi Ben,

On Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:
> 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 = []):

nit: remove space in the default assignment

But more importantly, perhaps we should use the empty tuple instead,
`()`. The reason why is in Python, the default object is initialised
once and the reference stays the same[1]. So if you appended something to
`choices`, that would stay between sucessive function invocations.

Since your function only reads `choices` and doesn't write, what you
have isn't wrong but I think it would be more future-proof to use `()`
instead.

Also, here's a stupid idea: perhaps instead of manually specifying
`choices` manually, could we extract it from `prompt_text` since all
possible choices are always placed within []?

Something like this?

	import re
	...
	choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))

> +    """ Prompt the user to choose one of the choices
> +    """
> +    while True:
> +        response = raw_input(prompt_text).strip().lower()
> +        if len(response) == 0:

It's more Pythonic to write `if not response`.

> +            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"])

Semantically, `["y", "n"]` should be a tuple too so that we emphasise
that the set of choices shouldn't be mutable.

>              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"])

Same here.

Thanks,

Denton

[1]: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

>                              if not response:
>                                  continue
>                          elif self.conflict_behavior == "skip":
> -- 
> gitgitgadget
>

Denton Liu Dec. 11, 2019, 11:59 a.m. UTC | #2

On Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:
> @@ -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':

One more thing, since we guarantee that prompt() returns 'y' or 'n' and
it handles the looping logic, we can get rid of the surrounding while.

> @@ -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":

Same with this, we can remove the surrounding `while`.

> -- 
> gitgitgadget
>

Patch

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":