Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text
- From
Ben Keene <seraphire@gmail.com>
- Date
- Dec 10, 2019, 14:26 UTC
- Message-ID
- <179dd921-d9d0-d26d-33e9-3664bf97fcc2@gmail.com>
- In-Reply-To
- <xmqqimmptazs.fsf@gitster-ct.c.googlers.com>
On 12/9/2019 5:00 PM, Junio C Hamano wrote:
Show 40 quoted lines
> "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> >> --- >> >> +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).
That is a good catch. What should we expect the default behavior to be in these two questions if the EOFError occurs? I would think that we should extend this to an abort of the process?
> response = prompt("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ", ["y", "n"])
> response = prompt("[s]kip this commit but apply the rest, or [q]uit? ", ["s", "q"])Should a quit be added to the first prompt and have those be the defaults on EOFError?