{"thread":{"id":"42083","subject":"[PATCH v2 0/2] git-p4: fix Git LFS pointer parsing","startedAt":"2016-04-20T08:10:07Z","lastAt":"2016-04-22T07:55:09Z","messageCount":9,"participants":["larsxschneider@gmail.com","Sebastian Schuberth","Junio C Hamano","Lars Schneider"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"283914","messageId":"1461139809-6573-1-git-send-email-larsxschneider@gmail.com","threadId":"42083","inReplyTo":null,"subject":"[PATCH v2 0/2] git-p4: fix Git LFS pointer parsing","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-04-20T08:10:07Z","receivedAt":"2016-04-20T08:10:07Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nv1: http://thread.gmane.org/gmane.comp.version-control.git/291917/\n\ndiff to v1:\n* make git-p4 LFS compatible with Git LFS versions prior to 1.2.0\n  Thanks Junio for the little push ... I guess I was a bit lazy here :)\n* improve the git-p4 LFS tests to detect invalid LFS pointer files\n\nThanks,\nLars\n\nLars Schneider (2):\n  travis-ci: update Git-LFS and P4 to the latest version\n  git-p4: fix Git LFS pointer parsing\n\n .travis.yml               |  4 ++--\n git-p4.py                 | 15 ++++++++++++---\n t/t9824-git-p4-git-lfs.sh |  4 ++++\n 3 files changed, 18 insertions(+), 5 deletions(-)\n\n--\n2.5.1\n"},{"id":"283915","messageId":"1461139809-6573-2-git-send-email-larsxschneider@gmail.com","threadId":"42083","inReplyTo":"1461139809-6573-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v2 1/2] travis-ci: update Git-LFS and P4 to the latest version","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-04-20T08:10:08Z","receivedAt":"2016-04-20T08:10:08Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n .travis.yml | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 78e433b..4acf617 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -22,8 +22,8 @@ addons:\n env:\n   global:\n     - DEVELOPER=1\n-    - P4_VERSION=\"15.2\"\n-    - GIT_LFS_VERSION=\"1.1.0\"\n+    - P4_VERSION=\"16.1\"\n+    - GIT_LFS_VERSION=\"1.2.0\"\n     - DEFAULT_TEST_TARGET=prove\n     - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n     - GIT_TEST_OPTS=\"--verbose --tee\"\n-- \n2.5.1\n"},{"id":"283916","messageId":"1461139809-6573-3-git-send-email-larsxschneider@gmail.com","threadId":"42083","inReplyTo":"1461139809-6573-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-04-20T08:10:09Z","receivedAt":"2016-04-20T08:10:09Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nGit LFS 1.2.0 removed a preamble from the output of the 'git lfs pointer'\ncommand [1] which broke the parsing of this output. Adjust the parser\nto support the old and the new format.\n\n[1] https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n git-p4.py                 | 15 ++++++++++++---\n t/t9824-git-p4-git-lfs.sh |  4 ++++\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 527d44b..ab52bc3 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1064,8 +1064,17 @@ class GitLFS(LargeFileSystem):\n         if pointerProcess.wait():\n             os.remove(contentFile)\n             die('git-lfs pointer command failed. Did you install the extension?')\n-        pointerContents = [i+'\\n' for i in pointerFile.split('\\n')[2:][:-1]]\n-        oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]\n+\n+        # Git LFS removed the preamble in the output of the 'pointer' command\n+        # starting from version 1.2.0. Check for the preamble here to support\n+        # earlier versions.\n+        # c.f. https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43\n+        preamble = 'Git LFS pointer for ' + contentFile + '\\n\\n'\n+        if pointerFile.startswith(preamble):\n+            pointerFile = pointerFile[len(preamble):]\n+\n+        oidEntry = [i for i in pointerFile.split('\\n') if i.startswith('oid')]\n+        oid = oidEntry[0].split(' ')[1].split(':')[1]\n         localLargeFile = os.path.join(\n             os.getcwd(),\n             '.git', 'lfs', 'objects', oid[:2], oid[2:4],\n@@ -1073,7 +1082,7 @@ class GitLFS(LargeFileSystem):\n         )\n         # LFS Spec states that pointer files should not have the executable bit set.\n         gitMode = '100644'\n-        return (gitMode, pointerContents, localLargeFile)\n+        return (gitMode, pointerFile, localLargeFile)\n \n     def pushFile(self, localLargeFile):\n         uploadProcess = subprocess.Popen(\ndiff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh\nindex 0b664a3..ca93ac8 100755\n--- a/t/t9824-git-p4-git-lfs.sh\n+++ b/t/t9824-git-p4-git-lfs.sh\n@@ -13,6 +13,10 @@ test_file_in_lfs () {\n \tFILE=\"$1\" &&\n \tSIZE=\"$2\" &&\n \tEXPECTED_CONTENT=\"$3\" &&\n+\tsed -n '1,1 p' \"$FILE\" | grep \"^version \" &&\n+\tsed -n '2,2 p' \"$FILE\" | grep \"^oid \" &&\n+\tsed -n '3,3 p' \"$FILE\" | grep \"^size \" &&\n+\ttest_line_count = 3 \"$FILE\" &&\n \tcat \"$FILE\" | grep \"size $SIZE\" &&\n \tHASH=$(cat \"$FILE\" | grep \"oid sha256:\" | sed -e \"s/oid sha256://g\") &&\n \tLFS_FILE=\".git/lfs/objects/$(echo \"$HASH\" | cut -c1-2)/$(echo \"$HASH\" | cut -c3-4)/$HASH\" &&\n-- \n2.5.1\n"},{"id":"283918","messageId":"CAHGBnuOjb+zmzwJeY-hbU4MYKpOMCQSRtDC4hS4zvCD5DQzv3w@mail.gmail.com","threadId":"42083","inReplyTo":"1461139809-6573-3-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-04-20T08:59:32Z","receivedAt":"2016-04-20T08:59:32Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Apr 20, 2016 at 10:10 AM,  <larsxschneider@gmail.com> wrote:\n\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1064,8 +1064,17 @@ class GitLFS(LargeFileSystem):\n>          if pointerProcess.wait():\n>              os.remove(contentFile)\n>              die('git-lfs pointer command failed. Did you install the extension?')\n> -        pointerContents = [i+'\\n' for i in pointerFile.split('\\n')[2:][:-1]]\n> -        oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]\n> +\n> +        # Git LFS removed the preamble in the output of the 'pointer' command\n\ngit-lfs did not remove the output. It simply goes to stderr instead of\nstdout now. That said, could a fix simply be to capture both stdout\nand sterr? If the output to the streams remain interleaved it should\nlook exactly like before.\n\n> +        # starting from version 1.2.0. Check for the preamble here to support\n> +        # earlier versions.\n> +        # c.f. https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43\n> +        preamble = 'Git LFS pointer for ' + contentFile + '\\n\\n'\n> +        if pointerFile.startswith(preamble):\n> +            pointerFile = pointerFile[len(preamble):]\n> +\n> +        oidEntry = [i for i in pointerFile.split('\\n') if i.startswith('oid')]\n> +        oid = oidEntry[0].split(' ')[1].split(':')[1]\n\nWhy do we need to remove the preamble at all, if present? If all we\nwant is the oid, we should simply only look at the line that starts\nwith that keyword, which would skip any preamble. Which is what you\nalready do here. However, I'd probably use .splitlines() instead of\n.split('\\n') and .startswith('oid ') (note the trailing space) instead\nof .startswith('oid') to ensure \"oid\" is a separate word.\n\nBut then again, I wonder why there's so much split() logic involved in\nextracting the oid. Couldn't we replace all of that with a regexp like\n\noid = re.search(r\"^oid \\w+:(\\w+)\", pointerFile, re.MULTILINE).group(1)\n\n-- \nSebastian Schuberth\n"},{"id":"283976","messageId":"xmqq8u08w2tb.fsf@gitster.mtv.corp.google.com","threadId":"42083","inReplyTo":"CAHGBnuOjb+zmzwJeY-hbU4MYKpOMCQSRtDC4hS4zvCD5DQzv3w@mail.gmail.com","subject":"Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T18:30:24Z","receivedAt":"2016-04-20T18:30:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth <sschuberth@gmail.com> writes:\n\n> Why do we need to remove the preamble at all, if present? If all we\n> want is the oid, we should simply only look at the line that starts\n> with that keyword, which would skip any preamble. Which is what you\n> already do here. However, I'd probably use .splitlines() instead of\n> .split('\\n') and .startswith('oid ') (note the trailing space) instead\n> of .startswith('oid') to ensure \"oid\" is a separate word.\n>\n> But then again, I wonder why there's so much split() logic involved in\n> extracting the oid. Couldn't we replace all of that with a regexp like\n>\n> oid = re.search(r\"^oid \\w+:(\\w+)\", pointerFile, re.MULTILINE).group(1)\n\nYup, all of that is a very useful suggestion.  If we know how the\npiece of information we want is identified in the output,\nspecifically looking for it would future-proof the code better, as\nit will not be affected by future change that adds unexpected cruft\nto the output we are reading from.\n"},{"id":"283992","messageId":"ADDF0944-7EBE-4DCC-B4D9-F4662AEFBEC7@gmail.com","threadId":"42083","inReplyTo":"xmqq8u08w2tb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-04-20T20:03:59Z","receivedAt":"2016-04-20T20:03:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 20 Apr 2016, at 20:30, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Sebastian Schuberth <sschuberth@gmail.com> writes:\n> \n>> Why do we need to remove the preamble at all, if present?\n\nBecause I need the content of a valid Git LFS pointer file \na few lines below:\nhttps://github.com/larsxschneider/git/blob/5ee7601c49b6eaa9da5eb47db5cf12b5d8d672c9/git-p4.py#L1085\n\nThis pointer file content is then written to the Git repository \ninstead of the actual file content.\n\n\n>> If all we\n>> want is the oid, we should simply only look at the line that starts\n>> with that keyword, which would skip any preamble. Which is what you\n>> already do here. However, I'd probably use .splitlines() instead of\n>> .split('\\n') and .startswith('oid ') (note the trailing space) instead\n>> of .startswith('oid') to ensure \"oid\" is a separate word.\n>> \n>> But then again, I wonder why there's so much split() logic involved in\n>> extracting the oid. Couldn't we replace all of that with a regexp like\n>> \n>> oid = re.search(r\"^oid \\w+:(\\w+)\", pointerFile, re.MULTILINE).group(1)\n> \n> Yup, all of that is a very useful suggestion.  If we know how the\n> piece of information we want is identified in the output,\n> specifically looking for it would future-proof the code better, as\n> it will not be affected by future change that adds unexpected cruft\n> to the output we are reading from.\n\nI agree that this is a better solution.\n\n@Junio: Can you squash the patch below or do you prefer a v3?\n\nThanks,\nLars\n\n\ndiff --git a/git-p4.py b/git-p4.py\nindex ab52bc3..a0d529b 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1073,8 +1073,7 @@ class GitLFS(LargeFileSystem):\n         if pointerFile.startswith(preamble):\n             pointerFile = pointerFile[len(preamble):]\n\n-        oidEntry = [i for i in pointerFile.split('\\n') if i.startswith('oid')]\n-        oid = oidEntry[0].split(' ')[1].split(':')[1]\n+        oid = re.search(r'^oid \\w+:(\\w+)', pointerFile, re.MULTILINE).group(1)\n         localLargeFile = os.path.join(\n             os.getcwd(),\n             '.git', 'lfs', 'objects', oid[:2], oid[2:4],"},{"id":"284000","messageId":"xmqq60vcuh9v.fsf@gitster.mtv.corp.google.com","threadId":"42083","inReplyTo":"1461139809-6573-3-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T21:01:00Z","receivedAt":"2016-04-20T21:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> -        pointerContents = [i+'\\n' for i in pointerFile.split('\\n')[2:][:-1]]\n> -        oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]\n> +\n> +        # Git LFS removed the preamble in the output of the 'pointer' command\n> +        # starting from version 1.2.0. Check for the preamble here to support\n> +        # earlier versions.\n> +        # c.f. https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43\n> +        preamble = 'Git LFS pointer for ' + contentFile + '\\n\\n'\n> +        if pointerFile.startswith(preamble):\n> +            pointerFile = pointerFile[len(preamble):]\n> +\n> +        oidEntry = [i for i in pointerFile.split('\\n') if i.startswith('oid')]\n> +        oid = oidEntry[0].split(' ')[1].split(':')[1]\n>          localLargeFile = os.path.join(\n>              os.getcwd(),\n>              '.git', 'lfs', 'objects', oid[:2], oid[2:4],\n> @@ -1073,7 +1082,7 @@ class GitLFS(LargeFileSystem):\n>          )\n>          # LFS Spec states that pointer files should not have the executable bit set.\n>          gitMode = '100644'\n> -        return (gitMode, pointerContents, localLargeFile)\n> +        return (gitMode, pointerFile, localLargeFile)\n\nIt seems to me that you used to return pointerContents which is an\narray of lines (each of which are LF terminated); the updated one\nreturns pointerFile which is a bare string with many lines.\n\nIs that change intentional?  Does the difference matter to the\ncaller of this method?  Even if it doesn't, is it a good idea to\nchange it as part of this commit?\n\n>      def pushFile(self, localLargeFile):\n>          uploadProcess = subprocess.Popen(\n> diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh\n> index 0b664a3..ca93ac8 100755\n> --- a/t/t9824-git-p4-git-lfs.sh\n> +++ b/t/t9824-git-p4-git-lfs.sh\n> @@ -13,6 +13,10 @@ test_file_in_lfs () {\n>  \tFILE=\"$1\" &&\n>  \tSIZE=\"$2\" &&\n>  \tEXPECTED_CONTENT=\"$3\" &&\n> +\tsed -n '1,1 p' \"$FILE\" | grep \"^version \" &&\n> +\tsed -n '2,2 p' \"$FILE\" | grep \"^oid \" &&\n> +\tsed -n '3,3 p' \"$FILE\" | grep \"^size \" &&\n> +\ttest_line_count = 3 \"$FILE\" &&\n>  \tcat \"$FILE\" | grep \"size $SIZE\" &&\n>  \tHASH=$(cat \"$FILE\" | grep \"oid sha256:\" | sed -e \"s/oid sha256://g\") &&\n>  \tLFS_FILE=\".git/lfs/objects/$(echo \"$HASH\" | cut -c1-2)/$(echo \"$HASH\" | cut -c3-4)/$HASH\" &&\n"},{"id":"284100","messageId":"780ED4D6-AD96-4BAF-A522-C724C3938EED@gmail.com","threadId":"42083","inReplyTo":"xmqq60vcuh9v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-04-22T07:53:30Z","receivedAt":"2016-04-22T07:53:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 20 Apr 2016, at 23:01, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> larsxschneider@gmail.com writes:\n> \n>> -        pointerContents = [i+'\\n' for i in pointerFile.split('\\n')[2:][:-1]]\npointerFile was split here by '\\n'. The splitting removes the newline and\nthe i+'\\n' adds it again. This back and forth makes it unnecessary hard\nto read.\n\n\n>> -        oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]\n>> +\n>> +        # Git LFS removed the preamble in the output of the 'pointer' command\n>> +        # starting from version 1.2.0. Check for the preamble here to support\n>> +        # earlier versions.\n>> +        # c.f. https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43\n>> +        preamble = 'Git LFS pointer for ' + contentFile + '\\n\\n'\n>> +        if pointerFile.startswith(preamble):\n>> +            pointerFile = pointerFile[len(preamble):]\n>> +\n>> +        oidEntry = [i for i in pointerFile.split('\\n') if i.startswith('oid')]\n>> +        oid = oidEntry[0].split(' ')[1].split(':')[1]\n>>         localLargeFile = os.path.join(\n>>             os.getcwd(),\n>>             '.git', 'lfs', 'objects', oid[:2], oid[2:4],\n>> @@ -1073,7 +1082,7 @@ class GitLFS(LargeFileSystem):\n>>         )\n>>         # LFS Spec states that pointer files should not have the executable bit set.\n>>         gitMode = '100644'\n>> -        return (gitMode, pointerContents, localLargeFile)\n>> +        return (gitMode, pointerFile, localLargeFile)\n> \n> It seems to me that you used to return pointerContents which is an\n> array of lines (each of which are LF terminated); the updated one\n> returns pointerFile which is a bare string with many lines.\nThe pointerContents is a string with LF separators (see comment above). \nThe only difference is that the old implementation was a list of strings \neach with a LF at the end ['line1\\n'],['line2\\n'] and the new \nimplementation is one string with LF separators 'line1\\nline2\\n'.\n\npointerContents goes through a few layers and is eventually used in \nthe \"writeToGitStream\" method [1] like this:\n\n        for d in contents:\n            self.gitStream.write(d)\n\nAt this point it doesn't matter if it is an array of strings or one\nstring.\n\n[1] https://github.com/git/git/blob/e6ac6e1f7d54584c2b03f073b5f329a37f4a9561/git-p4.py#L2401-L2402\n\n\n> Is that change intentional?  Does the difference matter to the\n> caller of this method?  Even if it doesn't, is it a good idea to\n> change it as part of this commit?\nYes, it was intentional. I wanted to make the code simpler/more readable.\nAs shown above the change doesn't matter to the caller method.\n\nI understand your concern of making this change part of the commit.\nHowever, I don't see another way because passing pointerFile as\nis in the old implementation would brake Git LFS 1.x support (because\npointerFile contains the preamble).\n\nWhat would be the best way forward? A v3 with a better commit message\nmentioning the array -> string change?\n\nThanks for the review,\nLars"},{"id":"284101","messageId":"CAHGBnuN_Tck3TY0DO38HpXLbe5mew-76Y0okaxmJtGEGs2g04A@mail.gmail.com","threadId":"42083","inReplyTo":"780ED4D6-AD96-4BAF-A522-C724C3938EED@gmail.com","subject":"Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-04-22T07:55:09Z","receivedAt":"2016-04-22T07:55:09Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Apr 22, 2016 at 9:53 AM, Lars Schneider\n<larsxschneider@gmail.com> wrote:\n\n> What would be the best way forward? A v3 with a better commit message\n> mentioning the array -> string change?\n\nI'd vote for that, yes. Also v3 could then properly incorporate my regexp.\n\n-- \nSebastian Schuberth\n"}]}