Re: [PATCH v4 09/11] git-p4: Add usability enhancements

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2019, 14:04 UTC
Message-ID: <xmqqfthy27hy.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <4fc49313f0d68a913ad19085ddb337ac4c18d0fe.1575498578.git.gitgitgadget@gmail.com>

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
>
> Issue: when prompting the user with raw_input, the tests are not forgiving of user input.  For example, on the first query asks for a yes/no response. If the user enters the full word "yes" or "no" the test will fail. Additionally, offer the suggestion of setting git-p4.attemptRCSCleanup when applying a commit fails because of RCS keywords. Both of these changes are usability enhancement suggestions.

Drop "Issue: " and upcase "when" that follows. The rest of the paragraph reads a lot better without it as a human friendly description.

"are usability enhancement suggestions"??? Leaves readers wonder who suggested them, or you are suggesting but are willing the change to be dropped, or what. Be a bit more assertive if you want to say that you believe these two would improve usability.

> Change the code prompting the user for input to sanitize the user input before checking the response by asking the response as a lower case string, trimming leading/trailing spaces, and returning the first character.
>
> Change the applyCommit() method that when applying a commit fails becasue of the P4 RCS Keywords, the user should consider setting git-p4.attemptRCSCleanup.

s/becasue/because/;

I have a feeling that these two may be worth doing but are totally separate issues, deserving two separate commits. Is there a good reason why these two must go hand-in-hand?

Show 17 quoted lines

> Signed-off-by: Ben Keene <seraphire@gmail.com>
> (cherry picked from commit 1fab571664f5b6ad4ef321199f52615a32a9f8c7)
> ---
>  git-p4.py | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index f7c0ef0c53..f13e4645a3 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1909,7 +1909,8 @@ 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 = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ").lower() \
> +                .strip()[0]

You could have saved the patch by doing

	+	.lower().strip()[0]

instead, no?

I wonder if it would be better to write a thin wrapper around raw_input() that does the "downcase and take the first meaningful letter" thing for you and call it prompt() or something like that.

Show 11 quoted lines

> @@ -4327,7 +4343,12 @@ def main():
>                                     description = cmd.description,
>                                     formatter = HelpFormatter())
>  
> -    (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
> +    try:
> +        (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
> +    except:
> +        parser.print_help()
> +        raise
> +

This change may be a good idea to give help text when the command line parsing fails, but a good change deserves to be explained. I do not think I saw any mention of it in the proposed log message, though.

>      global verbose
>      verbose = cmd.verbose
>      if cmd.needsGit: