Re: [PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text
From: Denton Liu <hidden>
Date: 2019-12-11 11:52:00
Hi Ben, On Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
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`.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
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