[PATCH] git-p4: fix failed submit by skip non-text data files

From: "dorgon.chang" <redacted>

If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.

This commit will skip non-text data files when exception UnicodeDecodeError thrown.

Signed-off-by: dorgon.chang <redacted>
---
    git-p4: fix failed submit by skip non-text data files
    
    git-p4: fix failed submit by skip non-text data files
    
    If the submit contain binary files, it will throw exception and stop
    submit when try to append diff line description.
    
    This commit will skip non-text data files when exception
    UnicodeDecodeError thrown.
    
    I am using git-p4 with UnrealEngine game projects and this fix works for
    me.
    
    Signed-off-by: dorgon.chang dorgonman@hotmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-977%2Fdorgonman%2Fdorgon%2Ffix_gitp4_get_diff_description-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v1
Pull-Request: https://github.com/git/git/pull/977

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

diff --git a/git-p4.py b/git-p4.py
index 4433ca53de7e..29a8c202399a 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -1977,8 +1977,11 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):
                 newdiff += "+%s\n" % os.readlink(newFile)
             else:
                 f = open(newFile, "r")
-                for line in f.readlines():
-                    newdiff += "+" + line
+                try:
+                    for line in f.readlines():
+                        newdiff += "+" + line
+                except UnicodeDecodeError:
+                    pass # Fond non-text data
                 f.close()
 
         return (diff + newdiff).replace('\r\n', '\n')

base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
-- 
gitgitgadget

On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:

From: "dorgon.chang" <redacted>

If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.

This commit will skip non-text data files when exception UnicodeDecodeError thrown.

Signed-off-by: dorgon.chang <redacted>

As suggested on
https://github.com/git/git/pull/977#issuecomment-862197824, I'm happy to
state that the patch looks good to me. IIRC the diff there is solely for
the submit template, so it should only include text. That your patch
ensures in what seems an idiomatic way.

Signed-off-by: Simon Hausmann <redacted>




Simon

Hi Simon,

On Thu, 17 Jun 2021, Simon Hausmann wrote:

On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:

quoted

From: "dorgon.chang" <redacted>

If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.

This commit will skip non-text data files when exception UnicodeDecodeError thrown.

Signed-off-by: dorgon.chang <redacted>

As suggested on
https://github.com/git/git/pull/977#issuecomment-862197824, I'm happy to
state that the patch looks good to me. IIRC the diff there is solely for
the submit template, so it should only include text. That your patch
ensures in what seems an idiomatic way.

Thank you for reviewing and chiming in.

Signed-off-by: Simon Hausmann <redacted>

The typical way to record your review is to say `Reviewed-by:`. The
`Signed-off-by:` footer is usually used to indicate that you wrote the
patch, or that you shepherd it onto the Git mailing list.

Sorry to be so nit-picky...

Thanks,
Dscho

On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:

From: "dorgon.chang" <redacted>

If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.

This commit will skip non-text data files when exception UnicodeDecodeError thrown.

Signed-off-by: dorgon.chang <redacted>
---
    git-p4: fix failed submit by skip non-text data files
    
    git-p4: fix failed submit by skip non-text data files
    
    If the submit contain binary files, it will throw exception and stop
    submit when try to append diff line description.
    
    This commit will skip non-text data files when exception
    UnicodeDecodeError thrown.
    
    I am using git-p4 with UnrealEngine game projects and this fix works for
    me.
    
    Signed-off-by: dorgon.chang dorgonman@hotmail.com

As suggested on
https://github.com/git/git/pull/977#issuecomment-862197824, I'm happy to
state that the patch looks good to me. IIRC the diff there is solely for
the submit template, so it should only include text. That your patch
ensures in what seems an idiomatic way.

Reviewed-by: Simon Hausmann <redacted>



Simon

From: "dorgon.chang" <redacted>

If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.

This commit will skip non-text data files when exception UnicodeDecodeError thrown.

The skip will not affect actual submit files in the resulting cl,
the diff line description will only appear in submit template,
so you can review what changed before actully submit to p4.

I don't know if add any message here will be helpful for users,
so I choose to just skip binary content, since it already append filename previously.

Signed-off-by: dorgon.chang <redacted>
---
    git-p4: fix failed submit by skip non-text data files
    
    git-p4: fix failed submit by skip non-text data files
    
    If the submit contain binary files, it will throw exception and stop
    submit when try to append diff line description.
    
    This commit will skip non-text data files when exception
    UnicodeDecodeError thrown.
    
    I am using git-p4 with UnrealEngine game projects and this fix works for
    me.
    
    Signed-off-by: dorgon.chang dorgonman@hotmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-977%2Fdorgonman%2Fdorgon%2Ffix_gitp4_get_diff_description-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v2
Pull-Request: https://github.com/git/git/pull/977

Range-diff vs v1:

 1:  19b59f40b183 ! 1:  606729bda112 git-p4: fix failed submit by skip non-text data files
     @@ Commit message
      
          This commit will skip non-text data files when exception UnicodeDecodeError thrown.
      
     +    The skip will not affect actual submit files in the resulting cl,
     +    the diff line description will only appear in submit template,
     +    so you can review what changed before actully submit to p4.
     +
     +    I don't know if add any message here will be helpful for users,
     +    so I choose to just skip binary content, since it already append filename previously.
     +
          Signed-off-by: dorgon.chang [off-list ref]
      
       ## git-p4.py ##
     @@ git-p4.py: def get_diff_description(self, editedFiles, filesToAdd, symlinks):
      +                    for line in f.readlines():
      +                        newdiff += "+" + line
      +                except UnicodeDecodeError:
     -+                    pass # Fond non-text data
     ++                    pass # Found non-text data and skip, since diff description should only include text
                       f.close()
       
               return (diff + newdiff).replace('\r\n', '\n')


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

diff --git a/git-p4.py b/git-p4.py
index 4433ca53de7e..dc1f46351845 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -1977,8 +1977,11 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):
                 newdiff += "+%s\n" % os.readlink(newFile)
             else:
                 f = open(newFile, "r")
-                for line in f.readlines():
-                    newdiff += "+" + line
+                try:
+                    for line in f.readlines():
+                        newdiff += "+" + line
+                except UnicodeDecodeError:
+                    pass # Found non-text data and skip, since diff description should only include text
                 f.close()
 
         return (diff + newdiff).replace('\r\n', '\n')

base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
-- 
gitgitgadget

Johannes Schindelin [off-list ref] writes:

quoted

... IIRC the diff there is solely for
the submit template, so it should only include text. That your patch
ensures in what seems an idiomatic way.

This is a crucial piece of information lacking in the proposed
commit log message that would help readers understand why this is a
safe change.  An updated patch with a better log message would be
appreciated.

Thank you for reviewing and chiming in.

quoted

Signed-off-by: Simon Hausmann <redacted>

The typical way to record your review is to say `Reviewed-by:`. The
`Signed-off-by:` footer is usually used to indicate that you wrote the
patch, or that you shepherd it onto the Git mailing list.

Yes to both.

It is unusual to see "reviewed-by" from those whose names do not
appear even once in output of "git shortlog --no-merges git-p4.py"
on a patch that touches "git-p4.py", but this is a fringe area
(compared to the more core-ish part of the system) where people
touch to scratch their own itch without staying around for a long
haul, so it is understandable that we do not always have resident
experts in the area.  A review like this is highly appreciated.

Thanks, all.