Re: [PATCH v2 0/4] git-p4: Usability enhancements

[Luke Diamand <luke@diamand.org>]

On Tue, 10 Dec 2019 at 15:23, Ben Keene via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Some user interaction with git-p4 is not as user-friendly as the rest of the
> Git ecosystem. Here are three areas that can be improved on:
>
> 1) When a patch fails and the user is prompted, there is no sanitization of
> the user input so for a "yes/no" question, if the user enters "YES" instead
> of a lowercase "y", they will be re-prompted to enter their answer.
>
> Commit 1 addresses this by sanitizing the user text by trimming and
> lowercasing their input before testing. Now "YES" will succeed!
>
> 2) Git can handle scraping the RCS Keyword expansions out of source files
> when it is preparing to submit them to P4. However, if the config value
> "git-p4.attemptRCSCleanup" isn't set, it will just report that it fails.
>
> Commit 2 adds a helpful suggestion, that the user might want to set
> git-p4.attemptRCSCleanup.
>
> 3) If the command line arguments are incorrect for git-p4, the program
> reports that there was a syntax error, but doesn't show what the correct
> syntax is.
>
> Commit 3 displays the context help for the failed command.
>
> Ben Keene (4):
>   git-p4: yes/no prompts should sanitize user text
>   git-p4: show detailed help when parsing options fail
>   git-p4: wrap patchRCSKeywords test to revert changes on failure
>   git-p4: failure because of RCS keywords should show help
>
>  git-p4.py | 43 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
>
>
> base-commit: 083378cc35c4dbcc607e4cdd24a5fca440163d17
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v2
> Pull-Request: https://github.com/git/git/pull/675
>
> Range-diff vs v1:
>
>  1:  e721cdaa00 ! 1:  527b7b8f8a git-p4: [usability] yes/no prompts should sanitize user text
>      @@ -1,6 +1,6 @@
>       Author: Ben Keene <seraphire@gmail.com>
>
>      -    git-p4: [usability] yes/no prompts should sanitize user text
>      +    git-p4: yes/no prompts should sanitize user text
>
>           When prompting the user interactively for direction, the tests are
>           not forgiving of user input format.

Looks good to me!

>  3:  2a10890ef7 ! 2:  1d4f4e210b git-p4: [usability] Show detailed help when parsing options fail
>      @@ -1,6 +1,6 @@
>       Author: Ben Keene <seraphire@gmail.com>
>
>      -    git-p4: [usability] Show detailed help when parsing options fail
>      +    git-p4: show detailed help when parsing options fail
>
>           When a user provides invalid parameters to git-p4, the program
>           reports the failure but does not provide the correct command syntax.

This would make git-p4 more consistent with other git commands which
give some brief options, so seems like a sensible thing to do.


>  2:  d608f529a0 ! 3:  20aa557193 git-p4: [usability] RCS Keyword failure should suggest help
>      @@ -1,14 +1,13 @@
>       Author: Ben Keene <seraphire@gmail.com>
>
>      -    git-p4: [usability] RCS Keyword failure should suggest help
>      +    git-p4: wrap patchRCSKeywords test to revert changes on failure
>
>      -    When applying a commit fails because of RCS keywords, Git
>      -    will fail the P4 submit. It would help the user if Git suggested that
>      -    the user set git-p4.attemptRCSCleanup to true.
>      +    The patchRCSKeywords function has the potentional of throwing
>      +    an exception and this would leave files checked out in P4 and partially
>      +    modified.
>
>      -    Change the applyCommit() method that when applying a commit fails
>      -    becasue of the P4 RCS Keywords, the user should consider setting
>      -    git-p4.attemptRCSCleanup to true.
>      +    Add a try-catch block around the patchRCSKeywords call and revert
>      +    the edited files in P4 before leaving the method.
>
>           Signed-off-by: Ben Keene <seraphire@gmail.com>
>
>      @@ -30,14 +29,6 @@
>       +                        for f in editedFiles:
>       +                            p4_revert(f)
>       +                        raise
>      -+            else:
>      -+                # They do not have attemptRCSCleanup set, this might be the fail point
>      -+                # Check to see if the file has RCS keywords and suggest setting the property.
>      -+                for file in editedFiles | filesToDelete:
>      -+                    if p4_keywords_regexp_for_file(file) != None:
>      -+                        print("At least one file in this commit has RCS Keywords that may be causing problems. ")
>      -+                        print("Consider:\ngit config git-p4.attemptRCSCleanup true")
>      -+                        break

The code doesn't actually check if there are any files with RCS
keywords, it just looks to see if there are files that _could_ have
RCS keywords. There's some code in the other branch of the if-else
which checks for RCS keywords.

Perhaps factor that out, and then do the check (?).

Given that, this would be quite a useful hint for people.


>
>                    if fixed_rcs_keywords:
>                        print("Retrying the patch with RCS keywords cleaned up")
>  -:  ---------- > 4:  50e9a175c3 git-p4: failure because of RCS keywords should show help
>
> --
> gitgitgadget

In your branch there's also a "wrap patchRCSKeywords test" commit,
which isn't in here. That has some whitespace damage. But also I
wonder if the reverting should happen higher up, since there is
already some code around which tries to do this in other
circumstances.

Other than the small comments above, this looks good to me, thanks!

Luke