git-p4: Usability enhancements

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

index 60c73b6a37..0fa562fac9 100755
@@ -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

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?

Should a quit be added to the first prompt and have those be the defaults on EOFError?

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

index 0fa562fac9..856fe82079 100755
@@ -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

s/becasue/because/

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.

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 856fe82079..cb594baeef 100755
@@ -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

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.

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.

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 <[email protected]>

- 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 <[email protected]>

- 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 <[email protected]>

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

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

index 60c73b6a37..0fa562fac9 100755
@@ -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

Hi Ben,

	import re
	...
	choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 0fa562fac9..daa6e8a57a 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index daa6e8a57a..174200bb6c 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

index 174200bb6c..cb594baeef 100755
@@ -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

Looks good to me!

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

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.

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

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

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 68 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 31 deletions(-)

index 60c73b6a37..a05385ee2a 100755
@@ -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

Hi Ben,

s/promt/prompt/

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.

That is a good idea.  I obviously missed that and it would help if it reported the error before submission.

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?

Hi Junio,

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

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.

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index a05385ee2a..45c0175a68 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 45c0175a68..97fad8d3f0 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

index 97fad8d3f0..b659b8d4bf 100755
@@ -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

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>

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

From: Ben Keene <[email protected]>

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>

---
 git-p4.py | 67 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

index 60c73b6a37..3b3f1469a6 100755
@@ -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

Hi Ben,

Thanks-to: Denton Liu <[email protected]>?

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.

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 3b3f1469a6..9165ada2fd 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 9165ada2fd..03969052c8 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

index 03969052c8..690e5088cc 100755
@@ -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

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 [email protected] [[email protected]])

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 <[email protected]>

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

From: Ben Keene <[email protected]>

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 <[email protected]>

---
 git-p4.py | 67 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

index 60c73b6a37..3b3f1469a6 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 3b3f1469a6..9165ada2fd 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 9165ada2fd..03969052c8 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)

index 03969052c8..690e5088cc 100755
@@ -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

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.

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

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.