{"thread":{"id":"55299","subject":"[PATCH] git-p4: fix failed submit by skip non-text data files","startedAt":"2021-03-12T07:48:44Z","lastAt":"2021-06-29T00:52:45Z","messageCount":9,"participants":["dorgon chang via GitGitGadget","Simon Hausmann","Johannes Schindelin","Junio C Hamano","dorgon.chang"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"418939","messageId":"pull.977.git.git.1615535270135.gitgitgadget@gmail.com","threadId":"55299","inReplyTo":null,"subject":"[PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"dorgon chang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-12T07:47:49Z","receivedAt":"2021-03-12T07:48:44Z","isPatch":true,"sender":{"key":"name:dorgon chang","avatar":null},"body":"From: \"dorgon.chang\" \n\nIf the submit contain binary files, it will throw exception and stop submit when try to append diff line description.\n\nThis commit will skip non-text data files when exception UnicodeDecodeError thrown.\n\nSigned-off-by: dorgon.chang \n---\n git-p4: fix failed submit by skip non-text data files\n \n git-p4: fix failed submit by skip non-text data files\n \n If the submit contain binary files, it will throw exception and stop\n submit when try to append diff line description.\n \n This commit will skip non-text data files when exception\n UnicodeDecodeError thrown.\n \n I am using git-p4 with UnrealEngine game projects and this fix works for\n me.\n \n Signed-off-by: dorgon.chang dorgonman@hotmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-977%2Fdorgonman%2Fdorgon%2Ffix_gitp4_get_diff_description-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v1\nPull-Request: https://github.com/git/git/pull/977\n\n git-p4.py | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 4433ca53de7e..29a8c202399a 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1977,8 +1977,11 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n newdiff += \"+%s\\n\" % os.readlink(newFile)\n else:\n f = open(newFile, \"r\")\n- for line in f.readlines():\n- newdiff += \"+\" + line\n+ try:\n+ for line in f.readlines():\n+ newdiff += \"+\" + line\n+ except UnicodeDecodeError:\n+ pass # Fond non-text data\n f.close()\n \n return (diff + newdiff).replace('\\r\\n', '\\n')\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n-- \ngitgitgadget\n"},{"id":"427738","messageId":"YMsveynHB8MNiz+S@bagger.lan","threadId":"55299","inReplyTo":"pull.977.git.git.1615535270135.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"Simon Hausmann","fromEmail":"simon@lst.de","sentAt":"2021-06-17T11:18:19Z","receivedAt":"2021-06-17T11:18:33Z","isPatch":true,"sender":{"key":"hausmann@kde.org","avatar":"https://gravatar.com/avatar/bc9aad4fb31dce17eb66e690e7b51fe980c62da3c225c785da35dd806b8da778?d=mp&s=160"},"body":"On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:\n> From: \"dorgon.chang\" \n> \n> If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.\n> \n> This commit will skip non-text data files when exception UnicodeDecodeError thrown.\n> \n> Signed-off-by: dorgon.chang \n\nAs suggested on\nhttps://github.com/git/git/pull/977#issuecomment-862197824, I'm happy to\nstate that the patch looks good to me. IIRC the diff there is solely for\nthe submit template, so it should only include text. That your patch\nensures in what seems an idiomatic way.\n\nSigned-off-by: Simon Hausmann \n\n\n\n\nSimon\n"},{"id":"427823","messageId":"nycvar.QRO.7.76.6.2106181523090.57@tvgsbejvaqbjf.bet","threadId":"55299","inReplyTo":"YMsveynHB8MNiz+S@bagger.lan","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-06-18T13:24:26Z","receivedAt":"2021-06-18T13:24:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Simon,\n\nOn Thu, 17 Jun 2021, Simon Hausmann wrote:\n\n> On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:\n> > From: \"dorgon.chang\" \n> >\n> > If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.\n> >\n> > This commit will skip non-text data files when exception UnicodeDecodeError thrown.\n> >\n> > Signed-off-by: dorgon.chang \n>\n> As suggested on\n> https://github.com/git/git/pull/977#issuecomment-862197824, I'm happy to\n> state that the patch looks good to me. IIRC the diff there is solely for\n> the submit template, so it should only include text. That your patch\n> ensures in what seems an idiomatic way.\n\nThank you for reviewing and chiming in.\n\n> Signed-off-by: Simon Hausmann \n\nThe typical way to record your review is to say `Reviewed-by:`. The\n`Signed-off-by:` footer is usually used to indicate that you wrote the\npatch, or that you shepherd it onto the Git mailing list.\n\nSorry to be so nit-picky...\n\nThanks,\nDscho\n"},{"id":"427832","messageId":"YMyzkzMdVvY3XlEs@bagger.lan","threadId":"55299","inReplyTo":"pull.977.git.git.1615535270135.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"Simon Hausmann","fromEmail":"simon@lst.de","sentAt":"2021-06-18T14:54:11Z","receivedAt":"2021-06-18T14:54:18Z","isPatch":true,"sender":{"key":"hausmann@kde.org","avatar":"https://gravatar.com/avatar/bc9aad4fb31dce17eb66e690e7b51fe980c62da3c225c785da35dd806b8da778?d=mp&s=160"},"body":"On Fri, Mar 12, 2021 at 07:47:49AM +0000, dorgon chang via GitGitGadget wrote:\n> From: \"dorgon.chang\" \n> \n> If the submit contain binary files, it will throw exception and stop submit when try to append diff line description.\n> \n> This commit will skip non-text data files when exception UnicodeDecodeError thrown.\n> \n> Signed-off-by: dorgon.chang \n> ---\n> git-p4: fix failed submit by skip non-text data files\n> \n> git-p4: fix failed submit by skip non-text data files\n> \n> If the submit contain binary files, it will throw exception and stop\n> submit when try to append diff line description.\n> \n> This commit will skip non-text data files when exception\n> UnicodeDecodeError thrown.\n> \n> I am using git-p4 with UnrealEngine game projects and this fix works for\n> me.\n> \n> Signed-off-by: dorgon.chang dorgonman@hotmail.com\n\nAs suggested on\nhttps://github.com/git/git/pull/977#issuecomment-862197824, I'm happy to\nstate that the patch looks good to me. IIRC the diff there is solely for\nthe submit template, so it should only include text. That your patch\nensures in what seems an idiomatic way.\n\nReviewed-by: Simon Hausmann \n\n\n\nSimon\n"},{"id":"427943","messageId":"xmqq35tel5ad.fsf@gitster.g","threadId":"55299","inReplyTo":"pull.977.git.git.1615535270135.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-19T06:47:22Z","receivedAt":"2021-06-19T06:47:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"dorgon chang via GitGitGadget\" writes:\n\n> From: \"dorgon.chang\" \n>\n> If the submit contain binary files, it will throw exception and\n> stop submit when try to append diff line description.\n\nOK, that explains how the program fails.\n\n> This commit will skip non-text data files when exception\n> UnicodeDecodeError thrown.\n\nIf there are changes in aText and aBinary file and you try to submit\na cl that contains both changes, you do want changes to both files\ngo together, no? If you skip non-text, does that mean you ignore\nthe changes to aBinary file and submit only the changes to aText\nfile?\n\nI guess my confusion comes from not understanding what you exactly\nmean by \"append diff line description\". Whatever that means, if\nthat is purely informational and does not affect what is actually\nsubmit in the resulting cl, then the patch would be an improvement.\nIf not, and if for example it loses changes to binary files, then it\nis merely sweeping the problem under the rug.\n\nIn short the explanation of the solution does not build confidence\nin the readers minds. You'd need to explain why such a skipping is\na safe thing to do a bit better.\n\nEven if we assuming that what happens in the loop you threw in\ntry/except block is purely cosmetic and optional thing that does not\naffect the correct operation of the program or its outcome, I\nwonder if we can do better. When you get a decode error, you'd have\nan early part of the change (which could be empty) before you hit\nthe error in newdiff, and that is returned to the caller without any\nsign that it is a truncated output. I wonder something like\n\n\texcept UnicodeDecodeError:\n\t\tnewdiff = '<>'\n\nmay be more helpful to the user. Assuming that this is purely for\nhuman consumption without affecting the correctness or outcome of\nthe program and we can place pretty much any text there, that is.\nBut because the proposed commit log message does not explain why\nskipping is safe, I do not know if that assumption holds in the\nfirst place.\n\nThanks.\n\n> diff --git a/git-p4.py b/git-p4.py\n> index 4433ca53de7e..29a8c202399a 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1977,8 +1977,11 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n> newdiff += \"+%s\\n\" % os.readlink(newFile)\n> else:\n> f = open(newFile, \"r\")\n> - for line in f.readlines():\n> - newdiff += \"+\" + line\n> + try:\n> + for line in f.readlines():\n> + newdiff += \"+\" + line\n> + except UnicodeDecodeError:\n> + pass # Fond non-text data\n\ns/Fond/Found/ I would think.\n\n> f.close()\n> \n> return (diff + newdiff).replace('\\r\\n', '\\n')\n>\n> base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n"},{"id":"427970","messageId":"20210620075607.1228-1-dorgonman@hotmail.com","threadId":"55299","inReplyTo":"xmqq35tel5ad.fsf@gitster.g","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"dorgon.chang","fromEmail":"dorgon.chang@gmail.com","sentAt":"2021-06-20T07:56:07Z","receivedAt":"2021-06-20T07:56:23Z","isPatch":true,"sender":{"key":"dorgon.chang@gmail.com","avatar":null},"body":"> [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)):\n> \n> ```\n> \"dorgon chang via GitGitGadget\" writes:\n> \n> > From: \"dorgon.chang\" \n> >\n> > If the submit contain binary files, it will throw exception and\n> > stop submit when try to append diff line description.\n> \n> OK, that explains how the program fails.\n> \n> > This commit will skip non-text data files when exception\n> > UnicodeDecodeError thrown.\n> \n> If there are changes in aText and aBinary file and you try to submit\n> a cl that contains both changes, you do want changes to both files\n> go together, no? If you skip non-text, does that mean you ignore\n> the changes to aBinary file and submit only the changes to aText\n> file?\n> \n\n> I guess my confusion comes from not understanding what you exactly\n> mean by \"append diff line description\". Whatever that means, if\n> that is purely informational and does not affect what is actually\n> submit in the resulting cl, then the patch would be an improvement.\n> If not, and if for example it loses changes to binary files, then it\n> is merely sweeping the problem under the rug.\n> \n\nThe skip will not affect actual submit files in the resulting cl,\nthe diff line description will only appear in submit template, \nso you can review what changed before actully submit to p4.\n\n> In short the explanation of the solution does not build confidence\n> in the readers minds. You'd need to explain why such a skipping is\n> a safe thing to do a bit better.\n> \n> Even if we assuming that what happens in the loop you threw in\n> try/except block is purely cosmetic and optional thing that does not\n> affect the correct operation of the program or its outcome, I\n> wonder if we can do better. When you get a decode error, you'd have\n> an early part of the change (which could be empty) before you hit\n> the error in newdiff, and that is returned to the caller without any\n> sign that it is a truncated output. I wonder something like\n> \n> \texcept UnicodeDecodeError:\n> \t\tnewdiff = '<>'\n> \n\nI don't know if add any message here will be helpful for users, \nso I choose to just skip binary content, since it already append filename previously. \n\n> may be more helpful to the user. Assuming that this is purely for\n> human consumption without affecting the correctness or outcome of\n> the program and we can place pretty much any text there, that is.\n> But because the proposed commit log message does not explain why\n> skipping is safe, I do not know if that assumption holds in the\n> first place.\n\n\n\n\n> Thanks.\n> \n> > diff --git a/git-p4.py b/git-p4.py\n> > index 4433ca53de7e..29a8c202399a 100755\n> > --- a/git-p4.py\n> > +++ b/git-p4.py\n> > @@ -1977,8 +1977,11 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n> > newdiff += \"+%s\\n\" % os.readlink(newFile)\n> > else:\n> > f = open(newFile, \"r\")\n> > - for line in f.readlines():\n> > - newdiff += \"+\" + line\n> > + try:\n> > + for line in f.readlines():\n> > + newdiff += \"+\" + line\n> > + except UnicodeDecodeError:\n> > + pass # Fond non-text data\n> \n> s/Fond/Found/ I would think.\n>\n\nJust fixed the typo, thanks.\n"},{"id":"428014","messageId":"xmqqr1gvj30m.fsf@gitster.g","threadId":"55299","inReplyTo":"20210620075607.1228-1-dorgonman@hotmail.com","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-21T03:43:53Z","receivedAt":"2021-06-21T03:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"dorgon.chang\" writes:\n\n> The skip will not affect actual submit files in the resulting cl,\n> the diff line description will only appear in submit template, \n> so you can review what changed before actully submit to p4.\n> ...\n> I don't know if add any message here will be helpful for users, \n> so I choose to just skip binary content, since it already append filename previously. \n\nThese are both good things to write in the proposed log message.\n\nThanks.\n"},{"id":"428016","messageId":"pull.977.v2.git.git.1624252574779.gitgitgadget@gmail.com","threadId":"55299","inReplyTo":"pull.977.git.git.1615535270135.gitgitgadget@gmail.com","subject":"[PATCH v2] git-p4: fix failed submit by skip non-text data files","fromName":"dorgon chang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-21T05:16:13Z","receivedAt":"2021-06-21T05:16:20Z","isPatch":true,"sender":{"key":"name:dorgon chang","avatar":null},"body":"From: \"dorgon.chang\" \n\nIf the submit contain binary files, it will throw exception and stop submit when try to append diff line description.\n\nThis commit will skip non-text data files when exception UnicodeDecodeError thrown.\n\nThe skip will not affect actual submit files in the resulting cl,\nthe diff line description will only appear in submit template,\nso you can review what changed before actully submit to p4.\n\nI don't know if add any message here will be helpful for users,\nso I choose to just skip binary content, since it already append filename previously.\n\nSigned-off-by: dorgon.chang \n---\n git-p4: fix failed submit by skip non-text data files\n \n git-p4: fix failed submit by skip non-text data files\n \n If the submit contain binary files, it will throw exception and stop\n submit when try to append diff line description.\n \n This commit will skip non-text data files when exception\n UnicodeDecodeError thrown.\n \n I am using git-p4 with UnrealEngine game projects and this fix works for\n me.\n \n Signed-off-by: dorgon.chang dorgonman@hotmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-977%2Fdorgonman%2Fdorgon%2Ffix_gitp4_get_diff_description-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-977/dorgonman/dorgon/fix_gitp4_get_diff_description-v2\nPull-Request: https://github.com/git/git/pull/977\n\nRange-diff vs v1:\n\n 1: 19b59f40b183 ! 1: 606729bda112 git-p4: fix failed submit by skip non-text data files\n @@ Commit message\n \n This commit will skip non-text data files when exception UnicodeDecodeError thrown.\n \n + The skip will not affect actual submit files in the resulting cl,\n + the diff line description will only appear in submit template,\n + so you can review what changed before actully submit to p4.\n +\n + I don't know if add any message here will be helpful for users,\n + so I choose to just skip binary content, since it already append filename previously.\n +\n Signed-off-by: dorgon.chang \n \n ## git-p4.py ##\n @@ git-p4.py: def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n + for line in f.readlines():\n + newdiff += \"+\" + line\n + except UnicodeDecodeError:\n -+ pass # Fond non-text data\n ++ pass # Found non-text data and skip, since diff description should only include text\n f.close()\n \n return (diff + newdiff).replace('\\r\\n', '\\n')\n\n\n git-p4.py | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 4433ca53de7e..dc1f46351845 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1977,8 +1977,11 @@ def get_diff_description(self, editedFiles, filesToAdd, symlinks):\n newdiff += \"+%s\\n\" % os.readlink(newFile)\n else:\n f = open(newFile, \"r\")\n- for line in f.readlines():\n- newdiff += \"+\" + line\n+ try:\n+ for line in f.readlines():\n+ newdiff += \"+\" + line\n+ except UnicodeDecodeError:\n+ pass # Found non-text data and skip, since diff description should only include text\n f.close()\n \n return (diff + newdiff).replace('\\r\\n', '\\n')\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n-- \ngitgitgadget\n"},{"id":"428653","messageId":"xmqqmtr94hm2.fsf@gitster.g","threadId":"55299","inReplyTo":"nycvar.QRO.7.76.6.2106181523090.57@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] git-p4: fix failed submit by skip non-text data files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T00:52:37Z","receivedAt":"2021-06-29T00:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> ... IIRC the diff there is solely for\n>> the submit template, so it should only include text. That your patch\n>> ensures in what seems an idiomatic way.\n\nThis is a crucial piece of information lacking in the proposed\ncommit log message that would help readers understand why this is a\nsafe change. An updated patch with a better log message would be\nappreciated.\n\n> Thank you for reviewing and chiming in.\n>\n>> Signed-off-by: Simon Hausmann \n>\n> The typical way to record your review is to say `Reviewed-by:`. The\n> `Signed-off-by:` footer is usually used to indicate that you wrote the\n> patch, or that you shepherd it onto the Git mailing list.\n\nYes to both.\n\nIt is unusual to see \"reviewed-by\" from those whose names do not\nappear even once in output of \"git shortlog --no-merges git-p4.py\"\non a patch that touches \"git-p4.py\", but this is a fringe area\n(compared to the more core-ish part of the system) where people\ntouch to scratch their own itch without staying around for a long\nhaul, so it is understandable that we do not always have resident\nexperts in the area. A review like this is highly appreciated.\n\nThanks, all.\n\n\n"}]}