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

[dorgonman]

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
cc: Simon Hausmann simon@lst.de
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: "dorgon.chang" dorgon.chang@gmail.com

[gitgitgadget-git]

Welcome to GitGitGadget

Hi @dorgonman, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget-git]

There is an issue in commit 844ce48:
Commit checks stopped - the message is too short

[dorgonman]

/allow

[gitgitgadget-git]

Error: User dorgonman is not permitted to use GitGitGadget

[dscho]

/allow

[gitgitgadget-git]

User dorgonman is now allowed to use GitGitGadget.

[dscho]

@dorgonman please have a look at the commit messages of git-p4's history... in particular, start your commit message with "git-p4: fix ..." and use the commit message's body to explain the problem a bit better?

[gitgitgadget-git]

There are issues in commit 10e8c06:
First line of commit message is too long (> 76 columns): git-p4: Fix failed submit by skip non-text data files in get_diff_description
Prefixed commit message must be in lower case: git-p4: Fix failed submit by skip non-text data files in get_diff_description

[gitgitgadget-git]

There is an issue in commit f5ff206:
Prefixed commit message must be in lower case: git-p4: Fix failed submit by skip non-text data files

[dorgonman]

/submit

[gitgitgadget-git]

Submitted as pull.977.git.git.1615535270135.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v1

To fetch this version to local tag pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v1

[dscho]

@dorgonman it might make sense to send a ping in reply to that mail, potentially Cc:ing Luke Diamand (see the commit log of git-p4.py for their email address) who is de facto maintainer of git p4.

[tronical]

@dorgonman thanks for cc’ing me. 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.

[dscho]

@tronical would you mind sending that as a reply to https://lore.kernel.org/git/pull.977.git.git.1615535270135.gitgitgadget@gmail.com, so that the Git mailing list (and most importantly, the Git maintainer) has a record of your review? That will make it much more likely that the patch will be accepted.

[tronical]

Done. I hope that I got it right :-)

[dscho]

Done. I hope that I got it right :-)

Looks like it worked: https://lore.kernel.org/git/YMsveynHB8MNiz+S@bagger.lan/

[gitgitgadget-git]

On the Git mailing list, Simon Hausmann wrote (reply to this):

On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:
> From: "dorgon.chang" <dorgonman@hotmail.com>
> 
> 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 <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.

Signed-off-by: Simon Hausmann <simon@lst.de>




Simon

[gitgitgadget-git]

User Simon Hausmann <simon@lst.de> has been added to the cc: list.

[gitgitgadget-git]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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:
> > From: "dorgon.chang" <dorgonman@hotmail.com>
> >
> > 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 <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.

Thank you for reviewing and chiming in.

> Signed-off-by: Simon Hausmann <simon@lst.de>

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

[gitgitgadget-git]

User Johannes Schindelin <Johannes.Schindelin@gmx.de> has been added to the cc: list.

[gitgitgadget-git]

On the Git mailing list, "dorgon.chang" wrote (reply to this):

> [On the Git mailing list](https://lore.kernel.org/git/xmqq35tel5ad.fsf@gitster.g), Junio C Hamano wrote ([reply to this](https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis)):
> 
> ```
> "dorgon chang via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: "dorgon.chang" <dorgonman@hotmail.com>
> >
> > If the submit contain binary files, it will throw exception and
> > stop submit when try to append diff line description.
> 
> OK, that explains how the program fails.
> 
> > This commit will skip non-text data files when exception
> > UnicodeDecodeError thrown.
> 
> If there are changes in aText and aBinary file and you try to submit
> a cl that contains both changes, you do want changes to both files
> go together, no?  If you skip non-text, does that mean you ignore
> the changes to aBinary file and submit only the changes to aText
> file?
> 

> I guess my confusion comes from not understanding what you exactly
> mean by "append diff line description".  Whatever that means, if
> that is purely informational and does not affect what is actually
> submit in the resulting cl, then the patch would be an improvement.
> If not, and if for example it loses changes to binary files, then it
> is merely sweeping the problem under the rug.
> 

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.

> In short the explanation of the solution does not build confidence
> in the readers minds.  You'd need to explain why such a skipping is
> a safe thing to do a bit better.
> 
> Even if we assuming that what happens in the loop you threw in
> try/except block is purely cosmetic and optional thing that does not
> affect the correct operation of the program or its outcome,  I
> wonder if we can do better.  When you get a decode error, you'd have
> an early part of the change (which could be empty) before you hit
> the error in newdiff, and that is returned to the caller without any
> sign that it is a truncated output.  I wonder something like
> 
> 	except UnicodeDecodeError:
> 		newdiff = '<<new binary file>>'
> 

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. 

> may be more helpful to the user.  Assuming that this is purely for
> human consumption without affecting the correctness or outcome of
> the program and we can place pretty much any text there, that is.
> But because the proposed commit log message does not explain why
> skipping is safe, I do not know if that assumption holds in the
> first place.




> Thanks.
> 
> > 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
> 
> s/Fond/Found/ I would think.
>

Just fixed the typo, thanks.

[gitgitgadget-git]

User "dorgon.chang" <dorgon.chang@gmail.com> has been added to the cc: list.

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"dorgon.chang" <dorgon.chang@gmail.com> writes:

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

These are both good things to write in the proposed log message.

Thanks.

[dorgonman]

/submit

[gitgitgadget-git]

Submitted as pull.977.v2.git.git.1624252574779.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v2

To fetch this version to local tag pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v2

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

>> ... 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.
>
>> Signed-off-by: Simon Hausmann <simon@lst.de>
>
> 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.

[gitgitgadget-git]

This branch is now known as dc/p4-binary-submit-fix.

[gitgitgadget-git]

This patch series was integrated into seen via d8f273a.

[gitgitgadget-git]

This patch series was integrated into seen via e4653d7.

[gitgitgadget-git]

The branch dc/p4-binary-submit-fix was mentioned in the "New Topics" section of the status updates on the Git mailing list.

[gitgitgadget-git]

This patch series was integrated into seen via 081b907.

[gitgitgadget-git]

This patch series was integrated into seen via 761d003.

[gitgitgadget-git]

This patch series was integrated into seen via 7aca59a.

[gitgitgadget-git]

There was a status update in the "Cooking" section about the branch dc/p4-binary-submit-fix on the Git mailing list:

Prevent "git p4" from failing to submit changes to binary file.

Will merge to 'next'.

[gitgitgadget-git]

This patch series was integrated into seen via bffa9ad.

[gitgitgadget-git]

This patch series was integrated into next via dec280f.

[gitgitgadget-git]

This patch series was integrated into seen via 6b354e0.

[gitgitgadget-git]

There was a status update in the "Cooking" section about the branch dc/p4-binary-submit-fix on the Git mailing list:

Prevent "git p4" from failing to submit changes to binary file.

Will merge to 'master'.

[gitgitgadget-git]

This patch series was integrated into seen via b6bd704.

[gitgitgadget-git]

This patch series was integrated into next via b6bd704.

[gitgitgadget-git]

This patch series was integrated into master via b6bd704.

[gitgitgadget-git]

Closed via b6bd704.