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

Message ID d608f529a0e01e99c97e895ab483000da068a7ac.1575901009.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4: Usability enhancements
Related show

Commit Message

Derrick Stolee via GitGitGadget Dec. 9, 2019, 2:16 p.m. UTC
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(-)

Comments

Junio C Hamano Dec. 9, 2019, 10:22 p.m. UTC | #1
"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
diff mbox series

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