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

[Denton Liu <liu.denton@gmail.com>]

Hi Ben,

On Thu, Dec 12, 2019 at 07:46:24PM +0000, Ben Keene via GitGitGadget wrote:
> 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) where
>   * promt_text is the text prompt for the user

s/promt/prompt/

>   * choices are extracted from the prompt text [.]
>       a single letter surrounded by square brackets
>       is selected as a valid choice.

Maybe something like this?

	* returns a single character where valid return values are
	  found by inspecting prompt_text for single characters
	  surrounded by square brackets
> 
> 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.
> 
> Since the method requires the returned text to be one of the available
> choices, remove the loop from the calling code that handles response
> verification.
> 
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  git-p4.py | 68 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index 60c73b6a37..a05385ee2a 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -167,6 +167,21 @@ def die(msg):
>          sys.stderr.write(msg + "\n")
>          sys.exit(1)
>  
> +def prompt(prompt_text):
> +    """ Prompt the user to choose one of the choices
> +
> +    Choices are identified in the prompt_text by square brackets around
> +    a single letter option.
> +    """
> +    choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))

Nice ;)

> +    while True:
> +        response = raw_input(prompt_text).strip().lower()
> +        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))
> @@ -1778,12 +1793,11 @@ def edit_template(self, template_file):
>          if os.stat(template_file).st_mtime > mtime:
>              return True
>  
> -        while True:
> -            response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> -            if response == 'y':
> -                return True
> -            if response == 'n':
> -                return False
> +        response = prompt("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> +        if response == 'y':
> +            return True
> +        if response == 'n':
> +            return False
>  
>      def get_diff_description(self, editedFiles, filesToAdd, symlinks):
>          # diff
> @@ -2345,31 +2359,22 @@ def run(self, args):
>                            " --prepare-p4-only")
>                      break
>                  if i < last:
> -                    quit = False
> -                    while True:
> -                        # 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? ")
> -                            if not response:
> -                                continue
> -                        elif self.conflict_behavior == "skip":
> -                            response = "s"
> -                        elif self.conflict_behavior == "quit":
> -                            response = "q"
> -                        else:
> -                            die("Unknown conflict_behavior '%s'" %
> -                                self.conflict_behavior)
> -
> -                        if response[0] == "s":
> -                            print("Skipping this commit, but applying the rest")
> -                            break
> -                        if response[0] == "q":
> -                            print("Quitting")
> -                            quit = True
> -                            break
> -                    if quit:
> +                    # prompt for what to do, or use the option/variable
> +                    if self.conflict_behavior == "ask":
> +                        print("What do you want to do?")
> +                        response = prompt("[s]kip this commit but apply the rest, or [q]uit? ")
> +                    elif self.conflict_behavior == "skip":
> +                        response = "s"
> +                    elif self.conflict_behavior == "quit":
> +                        response = "q"
> +                    else:
> +                        die("Unknown conflict_behavior '%s'" %
> +                            self.conflict_behavior)
> +
> +                    if response == "s":
> +                        print("Skipping this commit, but applying the rest")
> +                    if response == "q":
> +                        print("Quitting")
>                          break
>  
>          chdir(self.oldWorkingDirectory)

Aside from the one comment at the bottom, I reviewed the rest of this
patch with `-w` and it looks good to me. Unfortunately, I don't use or
know p4 so I haven't tested it.

> @@ -4170,3 +4175,4 @@ def main():
>  
>  if __name__ == '__main__':
>      main()
> +

Spurious trailing line. Perhaps we could make GGG error out on
whitespace errors before submissions are allowed?

> -- 
> gitgitgadget
>