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

From: Denton Liu <liu.denton@gmail.com>
To: Ben Keene via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Ben Keene <seraphire@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text
Date: Wed, 11 Dec 2019 03:52:52 -0800	[thread overview]
Message-ID: <20191211115252.GB41678@generichostname> (raw)
In-Reply-To: <527b7b8f8a25a9f8abc326004792507f7fe5e373.1575991374.git.gitgitgadget@gmail.com>

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
> 