{"thread":{"id":"38994","subject":"[PATCH] git-p4: Fetch the proper revision of utf16 files","startedAt":"2015-04-03T21:13:46Z","lastAt":"2015-04-03T22:49:13Z","messageCount":5,"participants":["Daniel Bingham","Eric Sunshine","Domain Admin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258943","messageId":"1428095627-8772-1-git-send-email-git@dbingham.com","threadId":"38994","inReplyTo":null,"subject":"[PATCH] git-p4: Fetch the proper revision of utf16 files","fromName":"Daniel Bingham","fromEmail":"daniel@dbingham.com","sentAt":"2015-04-03T21:13:46Z","receivedAt":"2015-04-03T21:13:46Z","isPatch":true,"sender":{"key":"daniel@dbingham.com","avatar":null},"body":"I discovered what appears to be a bug with the handling of utf16 type files \nwhen cloning from P4. It appears that it always fetches the content of the \nlatest version rather than getting the revision specified in the changelist \nbeing processed. This patch is an attempt to resolve that by formatting\nthe print command using the revision number as is done in streamP4Files().\n\nDaniel Bingham (1):\n git-p4: Fetch the proper revision of utf16 files\n\n git-p4.py | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\n-- \n2.3.5\n"},{"id":"258944","messageId":"1428095627-8772-2-git-send-email-git@dbingham.com","threadId":"38994","inReplyTo":"1428095627-8772-1-git-send-email-git@dbingham.com","subject":"[PATCH] git-p4: Fetch the proper revision of utf16 files","fromName":"Daniel Bingham","fromEmail":"daniel@dbingham.com","sentAt":"2015-04-03T21:13:47Z","receivedAt":"2015-04-03T21:13:47Z","isPatch":true,"sender":{"key":"daniel@dbingham.com","avatar":null},"body":"git-p4 always fetches the latest revision of UTF16\nfiles from P4 rather than the revision at the commit being sync'd.\n\nThe print command should, instead, specify the revision number from the\ncommit in question using the file#revision syntax.\n\nThe file#revision syntax is preferable over file@changelist for\nconsistency with how streamP4Files formats the fileArgs list.\n\nSigned-off-by: Daniel Bingham \n---\n git-p4.py | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex ff132b2..156f3a4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2101,7 +2101,8 @@ class P4Sync(Command, P4UserMap):\n # them back too. This is not needed to the cygwin windows version,\n # just the native \"NT\" type.\n #\n- text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])\n+ ufile = \"%s#%s\" % (file['depotFile'], file['rev'])\n+ text = p4_read_pipe(['print', '-q', '-o', '-', ufile])\n if p4_version_string().find(\"/NT\") >= 0:\n text = text.replace(\"\\r\\n\", \"\\n\")\n contents = [ text ]\n-- \n2.3.5\n"},{"id":"258950","messageId":"CAPig+cT59B-ccvbfyPvVt_1dTO7jFPn7YQdhu81WSz_1WFM2GA@mail.gmail.com","threadId":"38994","inReplyTo":"1428095627-8772-2-git-send-email-git@dbingham.com","subject":"Re: [PATCH] git-p4: Fetch the proper revision of utf16 files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-03T21:46:17Z","receivedAt":"2015-04-03T21:46:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 3, 2015 at 5:13 PM, Daniel Bingham wrote:\n> git-p4 always fetches the latest revision of UTF16\n> files from P4 rather than the revision at the commit being sync'd.\n>\n> The print command should, instead, specify the revision number from the\n> commit in question using the file#revision syntax.\n>\n> The file#revision syntax is preferable over file@changelist for\n> consistency with how streamP4Files formats the fileArgs list.\n\nAs a non-Perforce reader trying to understand this patch, there are a\ncouple issues which are unclear or inadequately explained. Perhaps you\ncould provide a bit more detail or cite relevant sources.\n\nFirst, does \"UTF16 file\" refer to the content or the filename?\n\nSecond, I may be entirely missing it, but the commit message doesn't\nseem to explain why this impacts only \"UTF16 files\", and why this\nsolution is the appropriate fix.\n\nIf the answer to the first question is that the filename is UTF-16,\nthen would an alternate fix be to convert the value of\nfile['depotFile'] to have the same encoding as the \"print -q -o - ...\"\ncommand-line? (Again, please excuse my Perforce-ignorance if I'm\ncompletely off the mark.)\n\n> Signed-off-by: Daniel Bingham \n> ---\n> git-p4.py | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index ff132b2..156f3a4 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2101,7 +2101,8 @@ class P4Sync(Command, P4UserMap):\n> # them back too. This is not needed to the cygwin windows version,\n> # just the native \"NT\" type.\n> #\n> - text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])\n> + ufile = \"%s#%s\" % (file['depotFile'], file['rev'])\n> + text = p4_read_pipe(['print', '-q', '-o', '-', ufile])\n> if p4_version_string().find(\"/NT\") >= 0:\n> text = text.replace(\"\\r\\n\", \"\\n\")\n> contents = [ text ]\n> --\n> 2.3.5\n"},{"id":"258961","messageId":"CAFzU1RLqQ6WPJuxQMjBOmASoNauMv5StW-rC6d6DuL6EiOHUPQ@mail.gmail.com","threadId":"38994","inReplyTo":"CAPig+cT59B-ccvbfyPvVt_1dTO7jFPn7YQdhu81WSz_1WFM2GA@mail.gmail.com","subject":"Re: [PATCH] git-p4: Fetch the proper revision of utf16 files","fromName":"Domain Admin","fromEmail":"daniel@dbingham.com","sentAt":"2015-04-03T22:49:01Z","receivedAt":"2015-04-03T22:49:01Z","isPatch":true,"sender":{"key":"daniel@dbingham.com","avatar":null},"body":"*note* I tried to send this to the list earlier but forgot to set the\nmode to plain-text so it got rejected. Resending. Apologies for any\nduplicates.\n\nI think the context of the patch doesn't make this clear, but if you\nlook at git-p4.py in this spot you'll see that this is in a block that\nbegins:\n\nif type_base == \"utf16\":\n\nWhere \"type_base\" is extracted, by the script, from the filetype\nprovided by p4 (i.e. metadata provided by p4). In the particular\nscenario I encountered, P4 said that the file was of type xutf16 from\nwhich the split_p4_type() method extracts \"utf16\" as the type_base.\n\nThe essence of the issue here has nothing to do with UTF16, its just\ncoincidental that it happens for files of that type. The problem is\nthat the script wasn't requesting the proper revision from P4 and thus\nwould always get the *latest* revision. This corrects that by having\nthe script request the appropriate revision from P4.\n\nDaniel\n\nOn Fri, Apr 3, 2015 at 2:46 PM, Eric Sunshine wrote:\n> On Fri, Apr 3, 2015 at 5:13 PM, Daniel Bingham wrote:\n>> git-p4 always fetches the latest revision of UTF16\n>> files from P4 rather than the revision at the commit being sync'd.\n>>\n>> The print command should, instead, specify the revision number from the\n>> commit in question using the file#revision syntax.\n>>\n>> The file#revision syntax is preferable over file@changelist for\n>> consistency with how streamP4Files formats the fileArgs list.\n>\n> As a non-Perforce reader trying to understand this patch, there are a\n> couple issues which are unclear or inadequately explained. Perhaps you\n> could provide a bit more detail or cite relevant sources.\n>\n> First, does \"UTF16 file\" refer to the content or the filename?\n>\n> Second, I may be entirely missing it, but the commit message doesn't\n> seem to explain why this impacts only \"UTF16 files\", and why this\n> solution is the appropriate fix.\n>\n> If the answer to the first question is that the filename is UTF-16,\n> then would an alternate fix be to convert the value of\n> file['depotFile'] to have the same encoding as the \"print -q -o - ...\"\n> command-line? (Again, please excuse my Perforce-ignorance if I'm\n> completely off the mark.)\n>\n>> Signed-off-by: Daniel Bingham \n>> ---\n>> git-p4.py | 3 ++-\n>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index ff132b2..156f3a4 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2101,7 +2101,8 @@ class P4Sync(Command, P4UserMap):\n>> # them back too. This is not needed to the cygwin windows version,\n>> # just the native \"NT\" type.\n>> #\n>> - text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])\n>> + ufile = \"%s#%s\" % (file['depotFile'], file['rev'])\n>> + text = p4_read_pipe(['print', '-q', '-o', '-', ufile])\n>> if p4_version_string().find(\"/NT\") >= 0:\n>> text = text.replace(\"\\r\\n\", \"\\n\")\n>> contents = [ text ]\n>> --\n>> 2.3.5\n"},{"id":"258962","messageId":"CAPig+cT6DAu-AF-tKU=SKvh89imG_T2NfLCYxN6AsLe97CkPVw@mail.gmail.com","threadId":"38994","inReplyTo":"CAFzU1R+m8Gw3O_HHKmKq0rTALh+TJ1kK84=L2rsGCNft4XcT=Q@mail.gmail.com","subject":"Re: [PATCH] git-p4: Fetch the proper revision of utf16 files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-03T22:49:13Z","receivedAt":"2015-04-03T22:49:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[Daniel: Your HTML-formatted response probably did not make it to the\nGit mailing list since only plain text is accepted. Also, please\nrespond inline on this list rather than top-posting.]\n\nOn Fri, Apr 3, 2015 at 5:54 PM, Domain Admin wrote:\n> I think the context of the patch doesn't make this clear, but if you look at\n> git-p4.py in this spot you'll see that this is in a block that begins:\n>\n> if type_base == \"utf16\":\n>\n> Where \"type_base\" is extracted, by the script, from the filetype provided by\n> p4 (i.e. metadata provided by p4). In the particular scenario I encountered,\n> P4 said that the file was of type xutf16 from which the split_p4_type()\n> method extracts \"utf16\" as the type_base.\n\nSo, the patch's mention of \"UTF16 file\" indeed refers to the content\nrather than the filename.\n\nNow that I've studied up on Perforce a bit and read through some of\nthe git-p4.py code, I understand that there is a special-case for\nfiles with utf-16 content in which git-p4 re-reads the content in\norder to work around a problem with ASCII text saved with type utf-16\ngetting mangled. The work-around, 55aa5714 (git-p4: handle utf16\nfiletype properly; 2011-10-17), is buggy in that it neglects to\nspecify a particular revision of the file, and your patch fixes that.\nPerhaps your commit message could make that a bit clearer by\nmentioning the special-case and citing 55aa5714? Maybe something like:\n\n The special-case handling of files with UTF-16 content done by\n 55aa5714 (git-p4: handle utf16 filetype properly; 2011-10-17)\n neglects to specify a revision when re-reading the file content.\n As a result, the latest revision of the UTF-16 file is always\n fetched rather than the desired one.\n\nAlso, is it possible to craft a test for this issue and place it in\none of the t98xx scripts?\n\n> Daniel\n>\n> On Fri, Apr 3, 2015 at 2:46 PM, Eric Sunshine \n> wrote:\n>>\n>> On Fri, Apr 3, 2015 at 5:13 PM, Daniel Bingham \n>> wrote:\n>> > git-p4 always fetches the latest revision of UTF16\n>> > files from P4 rather than the revision at the commit being sync'd.\n>> >\n>> > The print command should, instead, specify the revision number from the\n>> > commit in question using the file#revision syntax.\n>> >\n>> > The file#revision syntax is preferable over file@changelist for\n>> > consistency with how streamP4Files formats the fileArgs list.\n>>\n>> As a non-Perforce reader trying to understand this patch, there are a\n>> couple issues which are unclear or inadequately explained. Perhaps you\n>> could provide a bit more detail or cite relevant sources.\n>>\n>> First, does \"UTF16 file\" refer to the content or the filename?\n>>\n>> Second, I may be entirely missing it, but the commit message doesn't\n>> seem to explain why this impacts only \"UTF16 files\", and why this\n>> solution is the appropriate fix.\n>>\n>> If the answer to the first question is that the filename is UTF-16,\n>> then would an alternate fix be to convert the value of\n>> file['depotFile'] to have the same encoding as the \"print -q -o - ...\"\n>> command-line? (Again, please excuse my Perforce-ignorance if I'm\n>> completely off the mark.)\n>>\n>> > Signed-off-by: Daniel Bingham \n>> > ---\n>> > git-p4.py | 3 ++-\n>> > 1 file changed, 2 insertions(+), 1 deletion(-)\n>> >\n>> > diff --git a/git-p4.py b/git-p4.py\n>> > index ff132b2..156f3a4 100755\n>> > --- a/git-p4.py\n>> > +++ b/git-p4.py\n>> > @@ -2101,7 +2101,8 @@ class P4Sync(Command, P4UserMap):\n>> > # them back too. This is not needed to the cygwin windows\n>> > version,\n>> > # just the native \"NT\" type.\n>> > #\n>> > - text = p4_read_pipe(['print', '-q', '-o', '-',\n>> > file['depotFile']])\n>> > + ufile = \"%s#%s\" % (file['depotFile'], file['rev'])\n>> > + text = p4_read_pipe(['print', '-q', '-o', '-', ufile])\n>> > if p4_version_string().find(\"/NT\") >= 0:\n>> > text = text.replace(\"\\r\\n\", \"\\n\")\n>> > contents = [ text ]\n>> > --\n>> > 2.3.5\n"}]}