[PATCH 0/3] git-p4: Usability enhancements

[PATCH 0/3] git-p4: Usability enhancements

[Ben Keene via GitGitGadget]

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 (3):
  git-p4: [usability] yes/no prompts should sanitize user text
  git-p4: [usability] RCS Keyword failure should suggest help
  git-p4: [usability] Show detailed help when parsing options fail

 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-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v1
Pull-Request: https://github.com/git/git/pull/675
-- 
gitgitgadget

[PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text

[Ben Keene via GitGitGadget]

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>
---
 git-p4.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

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 = []):
+    """ 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
+
 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"])
             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"])
                             if not response:
                                 continue
                         elif self.conflict_behavior == "skip":
-- 
gitgitgadget

Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text

[Junio C Hamano]

"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>
> ---
>  git-p4.py | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> 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 = []):
> +    """ 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).

>  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"])
>              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"])
>                              if not response:
>                                  continue
>                          elif self.conflict_behavior == "skip":

Re: [PATCH 1/3] git-p4: [usability] yes/no prompts should sanitize user text

[Ben Keene]

On 12/9/2019 5:00 PM, Junio C Hamano wrote:
> "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?

[PATCH 2/3] git-p4: [usability] RCS Keyword failure should suggest help

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

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.

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.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 0fa562fac9..856fe82079 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1950,8 +1950,23 @@ def applyCommit(self, id):
                     # disable the read-only bit on windows.
                     if self.isWindows and file not in editedFiles:
                         os.chmod(file, stat.S_IWRITE)
-                    self.patchRCSKeywords(file, kwfiles[file])
-                    fixed_rcs_keywords = True
+                    
+                    try:
+                        self.patchRCSKeywords(file, kwfiles[file])
+                        fixed_rcs_keywords = True
+                    except:
+                        # We are throwing an exception, undo all open edits
+                        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
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

Re: [PATCH 2/3] git-p4: [usability] RCS Keyword failure should suggest help

[Junio C Hamano]

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

> From: Ben Keene <seraphire@gmail.com>
>
> 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.
>
> Change the applyCommit() method that when applying a commit fails
> becasue of the P4 RCS Keywords, the user should consider setting

s/becasue/because/

> git-p4.attemptRCSCleanup to true.

The above explains the new "else:" clause really well.  The
original's logic was to

 - tryPatchCmd to apply a commit, which might fail,
 - when the above fails, only if attemptrcscleanup is set, munge
   the lines with rcs keywords and rerun tryPatchCmd

but your new "else:" gives a suggestion to use the (experimental?)
attemptRCSCleanup feature.

However, it does not explain the change to the "if :" clause.  I can
see that patchRCSKeywords() method does want to raise an exception,
and it is a good idea to prepare for the case and clean up the mess
it may create.  At least that deserves a mention in the proposed log
message---I actually think that the new try/except is an equally
important improvement that deserves to be a separate patch.

> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  git-p4.py | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0fa562fac9..856fe82079 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1950,8 +1950,23 @@ def applyCommit(self, id):
>                      # disable the read-only bit on windows.
>                      if self.isWindows and file not in editedFiles:
>                          os.chmod(file, stat.S_IWRITE)
> -                    self.patchRCSKeywords(file, kwfiles[file])
> -                    fixed_rcs_keywords = True
> +                    
> +                    try:
> +                        self.patchRCSKeywords(file, kwfiles[file])
> +                        fixed_rcs_keywords = True
> +                    except:
> +                        # We are throwing an exception, undo all open edits
> +                        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
>  
>              if fixed_rcs_keywords:
>                  print("Retrying the patch with RCS keywords cleaned up")

[PATCH 3/3] git-p4: [usability] Show detailed help when parsing options fail

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When a user provides invalid parameters to git-p4, the program
reports the failure but does not provide the correct command syntax.

Add an exception handler to the command-line argument parser to display
the command's specific command line parameter syntax when an exception
is thrown. Rethrow the exception so the current behavior is retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 856fe82079..cb594baeef 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4166,7 +4166,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
+
     global verbose
     verbose = cmd.verbose
     if cmd.needsGit:
-- 
gitgitgadget

Re: [PATCH 3/3] git-p4: [usability] Show detailed help when parsing options fail

[Junio C Hamano]

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

> From: Ben Keene <seraphire@gmail.com>
>
> When a user provides invalid parameters to git-p4, the program
> reports the failure but does not provide the correct command syntax.
>
> Add an exception handler to the command-line argument parser to display
> the command's specific command line parameter syntax when an exception
> is thrown. Rethrow the exception so the current behavior is retained.

Makes sense, I guess.

I forgot to mention this, but from the titles of all three patches I
would probably drop [usability] thing and downcase the first verb,
e.g.

    Subject: [PATCH 3/3] git-p4: show detailed help when parsing options fail

if I were writing these patches.

Thanks.

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

[Junio C Hamano]

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

> 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:

Sorry, but I am not a git-p4 person and I barely speak passable
Python, so I am not a great reviewer for the initial round of any
patch in this area.  IOW it is not all that useful to Cc me, as
opposed to somebody who have been working with git-p4 code longer
and more deeply.

    $ git shortlog --no-merges --since=2.years git-p4.py

may be a good way to choose whom we would want to ask for an initial
round of review (and Luke, who I consider the de-facto git-p4 person,.
is Cc'ed).

Thanks.

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

[Ben Keene via GitGitGadget]

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.
 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.
 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
       
                   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

[PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene via GitGitGadget]

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>
---
 git-p4.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

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 = []):
+    """ 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
+
 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"])
             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"])
                             if not response:
                                 continue
                         elif self.conflict_behavior == "skip":
-- 
gitgitgadget

Re: [PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text

[Denton Liu]

Hi Ben,

On Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:
> 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`.

> +            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.

>              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
> 

Re: [PATCH v2 1/4] git-p4: yes/no prompts should sanitize user text

[Denton Liu]

On Tue, Dec 10, 2019 at 03:22:51PM +0000, Ben Keene via GitGitGadget wrote:
> @@ -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"])
>              if response == 'y':
>                  return True
>              if response == 'n':

One more thing, since we guarantee that prompt() returns 'y' or 'n' and
it handles the looping logic, we can get rid of the surrounding while.

> @@ -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"])
>                              if not response:
>                                  continue
>                          elif self.conflict_behavior == "skip":

Same with this, we can remove the surrounding `while`.

> -- 
> gitgitgadget
> 

[PATCH v2 2/4] git-p4: show detailed help when parsing options fail

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When a user provides invalid parameters to git-p4, the program
reports the failure but does not provide the correct command syntax.

Add an exception handler to the command-line argument parser to display
the command's specific command line parameter syntax when an exception
is thrown. Rethrow the exception so the current behavior is retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 0fa562fac9..daa6e8a57a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4151,7 +4151,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
+
     global verbose
     verbose = cmd.verbose
     if cmd.needsGit:
-- 
gitgitgadget

[PATCH v2 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The patchRCSKeywords function has the potentional of throwing
an exception and this would leave files checked out in P4 and partially
modified.

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>
---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index daa6e8a57a..174200bb6c 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1950,8 +1950,15 @@ def applyCommit(self, id):
                     # disable the read-only bit on windows.
                     if self.isWindows and file not in editedFiles:
                         os.chmod(file, stat.S_IWRITE)
-                    self.patchRCSKeywords(file, kwfiles[file])
-                    fixed_rcs_keywords = True
+                    
+                    try:
+                        self.patchRCSKeywords(file, kwfiles[file])
+                        fixed_rcs_keywords = True
+                    except:
+                        # We are throwing an exception, undo all open edits
+                        for f in editedFiles:
+                            p4_revert(f)
+                        raise
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

[PATCH v2 4/4] git-p4: failure because of RCS keywords should show help

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

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.

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.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 174200bb6c..cb594baeef 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1959,6 +1959,14 @@ def applyCommit(self, id):
                         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
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

Re: [PATCH v2 4/4] git-p4: failure because of RCS keywords should show help

[Denton Liu]

On Tue, Dec 10, 2019 at 03:22:54PM +0000, Ben Keene via GitGitGadget wrote:
> From: Ben Keene <seraphire@gmail.com>
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  git-p4.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/git-p4.py b/git-p4.py
> index 174200bb6c..cb594baeef 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1959,6 +1959,14 @@ def applyCommit(self, id):
>                          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:

small nit: we should use `is not None` here.

> +                        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
>  
>              if fixed_rcs_keywords:
>                  print("Retrying the patch with RCS keywords cleaned up")
> -- 
> gitgitgadget

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

[Luke Diamand]

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

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

[Ben Keene via GitGitGadget]

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 | 94 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 34 deletions(-)


base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v3
Pull-Request: https://github.com/git/git/pull/675

Range-diff vs v2:

 1:  527b7b8f8a ! 1:  fff93acf44 git-p4: yes/no prompts should sanitize user text
     @@ -9,9 +9,12 @@
          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
     +    Create a new function, prompt(prompt_text) where
            * promt_text is the text prompt for the user
     -      * is a list of lower-case, single letter choices.
     +      * choices are extracted from the prompt text [.]
     +          a single letter surrounded by square brackets
     +          is selected as a valid choice.
     +
          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
     @@ -19,6 +22,10 @@
      
          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@gmail.com>
      
       diff --git a/git-p4.py b/git-p4.py
     @@ -28,12 +35,16 @@
               sys.stderr.write(msg + "\n")
               sys.exit(1)
       
     -+def prompt(prompt_text, choices = []):
     ++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))
      +    while True:
      +        response = raw_input(prompt_text).strip().lower()
     -+        if len(response) == 0:
     ++        if not response:
      +            continue
      +        response = response[0]
      +        if response in choices:
     @@ -43,22 +54,73 @@
           if verbose:
               sys.stderr.write('Writing pipe: %s\n' % str(c))
      @@
     +         if os.stat(template_file).st_mtime > mtime:
                   return True
       
     -         while 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"])
     -             if response == 'y':
     -                 return True
     -             if response == 'n':
     +-            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
      @@
     -                         # prompt for what to do, or use the option/variable
     -                         if self.conflict_behavior == "ask":
     -                             print("What do you want to do?")
     +                           " --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? ")
     -+                            response = prompt("[s]kip this commit but apply"
     -+                                                 " the rest, or [q]uit? ", ["s", "q"])
     -                             if not response:
     -                                 continue
     -                         elif self.conflict_behavior == "skip":
     +-                            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)
     +@@
     + 
     + if __name__ == '__main__':
     +     main()
     ++
 2:  1d4f4e210b = 2:  5c5c981632 git-p4: show detailed help when parsing options fail
 3:  20aa557193 = 3:  c466e79148 git-p4: wrap patchRCSKeywords test to revert changes on failure
 4:  50e9a175c3 ! 4:  00307c3951 git-p4: failure because of RCS keywords should show help
     @@ -23,7 +23,7 @@
      +                # 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:
     ++                    if p4_keywords_regexp_for_file(file) is not 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

-- 
gitgitgadget

[PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene via GitGitGadget]

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) where
  * promt_text is the text prompt for the user
  * choices are extracted from the prompt text [.]
      a single letter surrounded by square brackets
      is selected as a valid choice.

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@gmail.com>
---
 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))
+    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)
@@ -4170,3 +4175,4 @@ def main():
 
 if __name__ == '__main__':
     main()
+
-- 
gitgitgadget

Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Denton Liu]

Hi Ben,

On Thu, Dec 12, 2019 at 07:46:24PM +0000, Ben Keene via GitGitGadget wrote:
> 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) 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@gmail.com>
> ---
>  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
> 

Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene]

On 12/12/2019 8:45 PM, Denton Liu wrote:
> Hi Ben,
>
> On Thu, Dec 12, 2019 at 07:46:24PM +0000, Ben Keene via GitGitGadget wrote:
>> From: Ben Keene <seraphire@gmail.com>
>> ...
>>    * 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
Yes, that is much more readable.
> ...
> 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?
That is a good idea.  I obviously missed that and it would
help if it reported the error before submission.
>> -- 
>> gitgitgadget
>>

Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

>> @@ -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?

I think you are asking the tool for too much support.  

It may help a lot more if we gave a Makefile target (or two) that
the contributors can run before going public.  Perhaps


	O=origin/master
	upstream-check::
		git log -p --check $(O)..

that can be used like so:

	$ make upstream-check
	$ make O=gitster/next upstream-check

That way, those who use format-patch+email without GGG or those who
push to a shared repository to be reviewed among the peer developers
before going public would benefit, not just GGG users.

Hmm?

Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Johannes Schindelin]

Hi Junio,

On Fri, 13 Dec 2019, Junio C Hamano wrote:

> Denton Liu <liu.denton@gmail.com> writes:
>
> >> @@ -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?
>
> I think you are asking the tool for too much support.
>
> It may help a lot more if we gave a Makefile target (or two) that
> the contributors can run before going public.  Perhaps
>
>
> 	O=origin/master
> 	upstream-check::
> 		git log -p --check $(O)..
>
> that can be used like so:
>
> 	$ make upstream-check
> 	$ make O=gitster/next upstream-check
>
> That way, those who use format-patch+email without GGG or those who
> push to a shared repository to be reviewed among the peer developers
> before going public would benefit, not just GGG users.
>
> Hmm?

I'd like that a lot, _and_ I think GitGitGadget could learn the trick of
running that `Makefile` target and report failures back to the PR
_especially_ because GitGitGadget knows the base branch of the PR.

In my opinion, there is a lot of value in having GitGitGadget doing this,
as new contributors are likely to miss such a helpful `Makefile` target.
For example, I vividly remember when I contributed to cURL for the first
time and had totally and completely missed the invocation `make -C src
checksrc` to help me get the code into the preferred shape.

Ciao,
Dscho

Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Fri, 13 Dec 2019, Junio C Hamano wrote:
>
>> Denton Liu <liu.denton@gmail.com> writes:
>>
>> >> @@ -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?
>>
>> I think you are asking the tool for too much support.
>>
>> It may help a lot more if we gave a Makefile target (or two) that
>> the contributors can run before going public.  Perhaps
>>
>>
>> 	O=origin/master
>> 	upstream-check::
>> 		git log -p --check $(O)..
>>
>> that can be used like so:
>>
>> 	$ make upstream-check
>> 	$ make O=gitster/next upstream-check
>>
>> That way, those who use format-patch+email without GGG or those who
>> push to a shared repository to be reviewed among the peer developers
>> before going public would benefit, not just GGG users.
>>
>> Hmm?
>
> I'd like that a lot, _and_ I think GitGitGadget could learn the trick of
> running that `Makefile` target and report failures back to the PR
> _especially_ because GitGitGadget knows the base branch of the PR.

Yup.  That's the right approach---provide a common base that
everybody can use, and then teach the tool help its users with it.
That way, the improvement won't be limited to the audience of a
single tool.

Re: [PATCH v3 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene]

On 12/15/2019 3:30 PM, Johannes Schindelin wrote:
> Hi Junio,
>
> On Fri, 13 Dec 2019, Junio C Hamano wrote:
>
>> Denton Liu <liu.denton@gmail.com> writes:
>>
>>>> @@ -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?
>> I think you are asking the tool for too much support.
>>
>> It may help a lot more if we gave a Makefile target (or two) that
>> the contributors can run before going public.  Perhaps
>>
>>
>> 	O=origin/master
>> 	upstream-check::
>> 		git log -p --check $(O)..
>>
>> that can be used like so:
>>
>> 	$ make upstream-check
>> 	$ make O=gitster/next upstream-check
>>
>> That way, those who use format-patch+email without GGG or those who
>> push to a shared repository to be reviewed among the peer developers
>> before going public would benefit, not just GGG users.
>>
>> Hmm?
> I'd like that a lot, _and_ I think GitGitGadget could learn the trick of
> running that `Makefile` target and report failures back to the PR
> _especially_ because GitGitGadget knows the base branch of the PR.
>
> In my opinion, there is a lot of value in having GitGitGadget doing this,
> as new contributors are likely to miss such a helpful `Makefile` target.
> For example, I vividly remember when I contributed to cURL for the first
> time and had totally and completely missed the invocation `make -C src
> checksrc` to help me get the code into the preferred shape.
>
> Ciao,
> Dscho

Same here for me.  My entry point into submissions was through GGG.  The
more suggestions it can offer prior to "/submit"ting code the easier it
would have been for me and the less noise I would have brought to the
mailing list.

- Ben

[PATCH v3 2/4] git-p4: show detailed help when parsing options fail

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When a user provides invalid parameters to git-p4, the program
reports the failure but does not provide the correct command syntax.

Add an exception handler to the command-line argument parser to display
the command's specific command line parameter syntax when an exception
is thrown. Rethrow the exception so the current behavior is retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index a05385ee2a..45c0175a68 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4145,7 +4145,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
+
     global verbose
     verbose = cmd.verbose
     if cmd.needsGit:
-- 
gitgitgadget

[PATCH v3 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The patchRCSKeywords function has the potentional of throwing
an exception and this would leave files checked out in P4 and partially
modified.

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>
---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 45c0175a68..97fad8d3f0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1953,8 +1953,15 @@ def applyCommit(self, id):
                     # disable the read-only bit on windows.
                     if self.isWindows and file not in editedFiles:
                         os.chmod(file, stat.S_IWRITE)
-                    self.patchRCSKeywords(file, kwfiles[file])
-                    fixed_rcs_keywords = True
+                    
+                    try:
+                        self.patchRCSKeywords(file, kwfiles[file])
+                        fixed_rcs_keywords = True
+                    except:
+                        # We are throwing an exception, undo all open edits
+                        for f in editedFiles:
+                            p4_revert(f)
+                        raise
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

[PATCH v3 4/4] git-p4: failure because of RCS keywords should show help

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

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.

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.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 97fad8d3f0..b659b8d4bf 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1962,6 +1962,14 @@ def applyCommit(self, id):
                         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) is not 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
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

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

[Ben Keene via GitGitGadget]

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) 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 2 displays the context help for the failed command.

3) If Git generates an error while attempting to clean up the RCS Keyword
expansions, it currently leaves P4 in an invalid state. Files that were
checked out by P4 are not revereted.

Commit 3 adds and exception handler that catches this condition and issues a
P4 Revert for the files that were previously edited.

4) 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 4 adds a helpful suggestion, that the user might want to set
git-p4.attemptRCSCleanup.

Revisions
=========

v3 - Implemented the various suggestions from Luke and Denton.

I did not add additional exception handling for the EOFError in the prompt
method. I do believe that it is a good idea, but that would change the logic
handling of the existing code to handle this new "no answer" condition and I
didn't want to introduce that at this time.

v4 - Whitespace clean up and commit clarifications.

Submit 3 suggested some clarifications to the commit test and revealed some
whitespace errors.

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 | 93 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 34 deletions(-)


base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v4
Pull-Request: https://github.com/git/git/pull/675

Range-diff vs v3:

 1:  fff93acf44 ! 1:  6c23cd5684 git-p4: yes/no prompts should sanitize user text
     @@ -10,10 +10,10 @@
          will fail.
      
          Create a new function, prompt(prompt_text) where
     -      * promt_text is the text prompt for the user
     -      * choices are extracted from the prompt text [.]
     -          a single letter surrounded by square brackets
     -          is selected as a valid choice.
     +      * prompt_text is the text prompt for the user
     +      * 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
     @@ -26,6 +26,7 @@
          choices, remove the loop from the calling code that handles response
          verification.
      
     +    Thanks-to: Denton Liu <Denton Liu>
          Signed-off-by: Ben Keene <seraphire@gmail.com>
      
       diff --git a/git-p4.py b/git-p4.py
     @@ -119,8 +120,3 @@
                               break
       
               chdir(self.oldWorkingDirectory)
     -@@
     - 
     - if __name__ == '__main__':
     -     main()
     -+
 2:  5c5c981632 = 2:  bfdd3dc517 git-p4: show detailed help when parsing options fail
 3:  c466e79148 = 3:  20f6398693 git-p4: wrap patchRCSKeywords test to revert changes on failure
 4:  00307c3951 = 4:  c78e2e4db1 git-p4: failure because of RCS keywords should show help

-- 
gitgitgadget

[PATCH v4 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene via GitGitGadget]

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) where
  * prompt_text is the text prompt for the user
  * 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.

Thanks-to: Denton Liu <Denton Liu>
Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 67 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..3b3f1469a6 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))
+    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)
-- 
gitgitgadget

Re: [PATCH v4 1/4] git-p4: yes/no prompts should sanitize user text

[Denton Liu]

Hi Ben,

On Fri, Dec 13, 2019 at 01:57:58PM +0000, Ben Keene via GitGitGadget wrote:
> 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) where
>   * prompt_text is the text prompt for the user
>   * 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.
> 
> Thanks-to: Denton Liu <Denton Liu>

Thanks-to: Denton Liu <liu.denton@gmail.com>?

Anyway, it's probably not worth a reroll. Aside from that, all the
patches look good to me from a Python perspective.

> Signed-off-by: Ben Keene <seraphire@gmail.com>

Re: [PATCH v4 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene]

On 12/13/2019 5:54 PM, Denton Liu wrote:
> Hi Ben,
>
> On Fri, Dec 13, 2019 at 01:57:58PM +0000, Ben Keene via GitGitGadget wrote:
>> From: Ben Keene <seraphire@gmail.com>
>> ...
>> Thanks-to: Denton Liu <Denton Liu>
> Thanks-to: Denton Liu <liu.denton@gmail.com>?
>
> Anyway, it's probably not worth a reroll. Aside from that, all the
> patches look good to me from a Python perspective.

I was /so/ close!  I'll reroll it anyway.

>> Signed-off-by: Ben Keene <seraphire@gmail.com>

[PATCH v4 2/4] git-p4: show detailed help when parsing options fail

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When a user provides invalid parameters to git-p4, the program
reports the failure but does not provide the correct command syntax.

Add an exception handler to the command-line argument parser to display
the command's specific command line parameter syntax when an exception
is thrown. Rethrow the exception so the current behavior is retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 3b3f1469a6..9165ada2fd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4145,7 +4145,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
+
     global verbose
     verbose = cmd.verbose
     if cmd.needsGit:
-- 
gitgitgadget

[PATCH v4 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The patchRCSKeywords function has the potentional of throwing
an exception and this would leave files checked out in P4 and partially
modified.

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>
---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 9165ada2fd..03969052c8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1953,8 +1953,15 @@ def applyCommit(self, id):
                     # disable the read-only bit on windows.
                     if self.isWindows and file not in editedFiles:
                         os.chmod(file, stat.S_IWRITE)
-                    self.patchRCSKeywords(file, kwfiles[file])
-                    fixed_rcs_keywords = True
+                    
+                    try:
+                        self.patchRCSKeywords(file, kwfiles[file])
+                        fixed_rcs_keywords = True
+                    except:
+                        # We are throwing an exception, undo all open edits
+                        for f in editedFiles:
+                            p4_revert(f)
+                        raise
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

[PATCH v4 4/4] git-p4: failure because of RCS keywords should show help

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

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.

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.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 03969052c8..690e5088cc 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1962,6 +1962,14 @@ def applyCommit(self, id):
                         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) is not 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
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

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

[Ben Keene via GitGitGadget]

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) 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 2 displays the context help for the failed command.

3) If Git generates an error while attempting to clean up the RCS Keyword
expansions, it currently leaves P4 in an invalid state. Files that were
checked out by P4 are not revereted.

Commit 3 adds and exception handler that catches this condition and issues a
P4 Revert for the files that were previously edited.

4) 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 4 adds a helpful suggestion, that the user might want to set
git-p4.attemptRCSCleanup.

Revisions
=========

v3 - Implemented the various suggestions from Luke and Denton.

I did not add additional exception handling for the EOFError in the prompt
method. I do believe that it is a good idea, but that would change the logic
handling of the existing code to handle this new "no answer" condition and I
didn't want to introduce that at this time.

v4 - Whitespace clean up and commit clarifications.

Submit 3 suggested some clarifications to the commit test and revealed some
whitespace errors.

v5 - Fixed typo in a commit message. (Invalid attribute to Thanks-to: Denton
Liu liu.denton@gmail.com [liu.denton@gmail.com])

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 | 93 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 34 deletions(-)


base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v5
Pull-Request: https://github.com/git/git/pull/675

Range-diff vs v4:

 1:  6c23cd5684 ! 1:  7e0145fa32 git-p4: yes/no prompts should sanitize user text
     @@ -26,7 +26,7 @@
          choices, remove the loop from the calling code that handles response
          verification.
      
     -    Thanks-to: Denton Liu <Denton Liu>
     +    Thanks-to: Denton Liu <liu.denton@gmail.com>
          Signed-off-by: Ben Keene <seraphire@gmail.com>
      
       diff --git a/git-p4.py b/git-p4.py
 2:  bfdd3dc517 = 2:  4960d1fa22 git-p4: show detailed help when parsing options fail
 3:  20f6398693 = 3:  81a09a1228 git-p4: wrap patchRCSKeywords test to revert changes on failure
 4:  c78e2e4db1 = 4:  4c4b783fd5 git-p4: failure because of RCS keywords should show help

-- 
gitgitgadget

[PATCH v5 1/4] git-p4: yes/no prompts should sanitize user text

[Ben Keene via GitGitGadget]

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) where
  * prompt_text is the text prompt for the user
  * 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.

Thanks-to: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 67 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..3b3f1469a6 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))
+    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)
-- 
gitgitgadget

[PATCH v5 2/4] git-p4: show detailed help when parsing options fail

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When a user provides invalid parameters to git-p4, the program
reports the failure but does not provide the correct command syntax.

Add an exception handler to the command-line argument parser to display
the command's specific command line parameter syntax when an exception
is thrown. Rethrow the exception so the current behavior is retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 3b3f1469a6..9165ada2fd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4145,7 +4145,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
+
     global verbose
     verbose = cmd.verbose
     if cmd.needsGit:
-- 
gitgitgadget

[PATCH v5 3/4] git-p4: wrap patchRCSKeywords test to revert changes on failure

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The patchRCSKeywords function has the potentional of throwing
an exception and this would leave files checked out in P4 and partially
modified.

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>
---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 9165ada2fd..03969052c8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1953,8 +1953,15 @@ def applyCommit(self, id):
                     # disable the read-only bit on windows.
                     if self.isWindows and file not in editedFiles:
                         os.chmod(file, stat.S_IWRITE)
-                    self.patchRCSKeywords(file, kwfiles[file])
-                    fixed_rcs_keywords = True
+                    
+                    try:
+                        self.patchRCSKeywords(file, kwfiles[file])
+                        fixed_rcs_keywords = True
+                    except:
+                        # We are throwing an exception, undo all open edits
+                        for f in editedFiles:
+                            p4_revert(f)
+                        raise
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

[PATCH v5 4/4] git-p4: failure because of RCS keywords should show help

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

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.

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.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 03969052c8..690e5088cc 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1962,6 +1962,14 @@ def applyCommit(self, id):
                         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) is not 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
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-- 
gitgitgadget

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

[Junio C Hamano]

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

> Some user interaction with git-p4 is not as user-friendly as the rest of the
> ...
>
> 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

The reviews on the list seem to be in favor of these and I didn't
see much wrong (I think I fixed up an indented empty line) in the
series.  I'd appreciate a blessing from a git-p4 expert, though.

Thanks.

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

[Luke Diamand]

On Mon, 16 Dec 2019 at 20:39, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Some user interaction with git-p4 is not as user-friendly as the rest of the
> > ...
> >
> > 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
>
> The reviews on the list seem to be in favor of these and I didn't
> see much wrong (I think I fixed up an indented empty line) in the
> series.  I'd appreciate a blessing from a git-p4 expert, though.
>

$ git log --reverse --oneline --abbrev-commit
origin/maint..origin/bk/p4-misc-usability
e2aed5fd5b git-p4: yes/no prompts should sanitize user text
   - looks good to me

608e380502 git-p4: show detailed help when parsing options fail
   - also looks good to me

c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure
   - why not just catch the exception, and then drop out of the "if-"
condition and fall into the cleanup section at the bottom of that
function (line 1976)? As it stands, this is duplicating the cleanup
code now.

89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of
RCS keywords should show help
  - strictly speaking, the code does not actually check if there *are*
any RCS keywords, it just checks if the filetype means that RCS kws
*would* be expanded *if* they were present. The conflict might be just
because....there's a conflict. As it stands this will be giving
misleading advice. I would get it to check to see if there really are
any RCS keywords in the file.

> Thanks.

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> $ git log --reverse --oneline --abbrev-commit
> origin/maint..origin/bk/p4-misc-usability
> e2aed5fd5b git-p4: yes/no prompts should sanitize user text
>    - looks good to me
>
> 608e380502 git-p4: show detailed help when parsing options fail
>    - also looks good to me
>
> c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure
>    - why not just catch the exception, and then drop out of the "if-"
> condition and fall into the cleanup section at the bottom of that
> function (line 1976)? As it stands, this is duplicating the cleanup
> code now.
>
> 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of
> RCS keywords should show help
>   - strictly speaking, the code does not actually check if there *are*
> any RCS keywords, it just checks if the filetype means that RCS kws
> *would* be expanded *if* they were present. The conflict might be just
> because....there's a conflict. As it stands this will be giving
> misleading advice. I would get it to check to see if there really are
> any RCS keywords in the file.

Thanks.  Ben, let's keep the first two and discard the rest for now,
which can later be replaced with updated ones.

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

[Ben Keene]

On 12/25/2019 2:13 PM, Junio C Hamano wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> $ git log --reverse --oneline --abbrev-commit
>> origin/maint..origin/bk/p4-misc-usability
>> e2aed5fd5b git-p4: yes/no prompts should sanitize user text
>>     - looks good to me
>>
>> 608e380502 git-p4: show detailed help when parsing options fail
>>     - also looks good to me
>>
>> c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure
>>     - why not just catch the exception, and then drop out of the "if-"
>> condition and fall into the cleanup section at the bottom of that
>> function (line 1976)? As it stands, this is duplicating the cleanup
>> code now.
>>
>> 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of
>> RCS keywords should show help
>>    - strictly speaking, the code does not actually check if there *are*
>> any RCS keywords, it just checks if the filetype means that RCS kws
>> *would* be expanded *if* they were present. The conflict might be just
>> because....there's a conflict. As it stands this will be giving
>> misleading advice. I would get it to check to see if there really are
>> any RCS keywords in the file.
> Thanks.  Ben, let's keep the first two and discard the rest for now,
> which can later be replaced with updated ones.
That works for me.  So, are there any changes that I should make at
this time, or just let the rest die off?

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

[Junio C Hamano]

Ben Keene <seraphire@gmail.com> writes:

>> Thanks.  Ben, let's keep the first two and discard the rest for now,
>> which can later be replaced with updated ones.
> That works for me. So, are there any changes that I should make at
> this time, or just let the rest die off?

I don't think of any, from this side.  You can of course spend time
on salvaging and polishing these remaining patches for resubmission
in future cycle(s).

Thanks.