Hi Ben,
On Thu, Dec 12, 2019 at 07:46:24PM +0000, Ben Keene via GitGitGadget wrote:
> From: Ben Keene <seraphire@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
>
