Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text

From: Junio C Hamano <gitster@pobox.com>
To: "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Ben Keene <seraphire@gmail.com>
Subject: Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text
Date: Mon, 09 Dec 2019 14:00:55 -0800	[thread overview]
Message-ID: <xmqqimmptazs.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <e721cdaa008263b896c1d162e411c4e7a04c5710.1575901009.git.gitgitgadget@gmail.com> (Ben Keene via GitGitGadget's message of "Mon, 09 Dec 2019 14:16:47 +0000")

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