{"thread":{"id":"60394","subject":"[PATCH] git-p4 shouldn't attempt to store symlinks in LFS","startedAt":"2023-10-18T18:59:38Z","lastAt":"2023-10-19T17:59:33Z","messageCount":6,"participants":["Matthew McClain","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"483447","messageId":"20231018185854.857674-1-mmcclain@noprivs.com","threadId":"60394","inReplyTo":null,"subject":"[PATCH] git-p4 shouldn't attempt to store symlinks in LFS","fromName":"Matthew McClain","fromEmail":"mmcclain@noprivs.com","sentAt":"2023-10-18T18:58:54Z","receivedAt":"2023-10-18T18:59:38Z","isPatch":true,"sender":{"key":"mmcclain@noprivs.com","avatar":null},"body":"If a symlink in your Perforce repository matches\ngit-p4.largeFileExtensions, git-p4.py will attempt to put the symlink in\nLFS but will blow up when it passes a string to generateTempFile.\n\nGit LFS normal behavior does not stash symlinks in LFS.\n\nImporting revision 42889 (100%)Traceback (most recent call last):\n File \"./git/git-p4.py\", line 4621, in \n main()\n File \"./git/git-p4.py\", line 4615, in main\n if not cmd.run(args):\n File \"./git/git-p4.py\", line 4225, in run\n self.importRevisions(args, branch_arg_given)\n File \"./git/git-p4.py\", line 4002, in importRevisions\n self.importChanges(changes)\n File \"./git/git-p4.py\", line 3876, in importChanges\n self.initialParent)\n File \"./git/git-p4.py\", line 3496, in commit\n self.streamP4Files(files)\n File \"./git/git-p4.py\", line 3336, in streamP4Files\n cb=streamP4FilesCbSelf)\n File \"./git/git-p4.py\", line 910, in p4CmdList\n cb(entry)\n File \"./git/git-p4.py\", line 3321, in streamP4FilesCbSelf\n self.streamP4FilesCb(entry)\n File \"./git/git-p4.py\", line 3266, in streamP4FilesCb\n self.streamOneP4File(self.stream_file, self.stream_contents)\n File \"./git/git-p4.py\", line 3217, in streamOneP4File\n git_mode, contents = self.largeFileSystem.processContent(git_mode, relPath, contents)\n File \"./git/git-p4.py\", line 1656, in processContent\n return LargeFileSystem.processContent(self, git_mode, relPath, contents)\n File \"./git/git-p4.py\", line 1526, in processContent\n contentTempFile = self.generateTempFile(contents)\n File \"./git/git-p4.py\", line 1488, in generateTempFile\n contentFile.write(d)\n File \"/usr/lib64/python3.6/tempfile.py\", line 485, in func_wrapper\n return func(*args, **kwargs)\nTypeError: a bytes-like object is required, not 'str'\n\nSigned-off-by: Matthew McClain \n---\n git-p4.py | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex d26a980e5a..f5fda2a3dc 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1522,6 +1522,10 @@ def processContent(self, git_mode, relPath, contents):\n file is stored in the large file system and handles all necessary\n steps.\n \"\"\"\n+ # no need to store symlinks in LFS (generateTempFile wants bytes)\n+ if git_mode == \"120000\":\n+ return (git_mode, contents)\n+\n if self.exceedsLargeFileThreshold(relPath, contents) or self.hasLargeFileExtension(relPath):\n contentTempFile = self.generateTempFile(contents)\n pointer_git_mode, contents, localLargeFile = self.generatePointer(contentTempFile)\n-- \n2.39.3\n\n"},{"id":"483448","messageId":"xmqqil73sp8l.fsf@gitster.g","threadId":"60394","inReplyTo":"20231018185854.857674-1-mmcclain@noprivs.com","subject":"Re: [PATCH] git-p4 shouldn't attempt to store symlinks in LFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-18T19:30:02Z","receivedAt":"2023-10-18T19:30:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew McClain writes:\n\n> If a symlink in your Perforce repository matches\n> git-p4.largeFileExtensions, git-p4.py will attempt to put the symlink in\n> LFS but will blow up when it passes a string to generateTempFile.\n>\n> Git LFS normal behavior does not stash symlinks in LFS.\n\nI am not a \"git p4\" (or \"git lfs\") user, but if nobody uses LFS to\nstore symbolic links (which is very much understandable given what\nit was invented for), then that alone is a good enough reason to\navoid the regular blob codepath, and that is true even if\ngenerateTempFile accepted a str and silently converted it to bytes\nto help callers, no?\n\nSo \"but will blow up ...\" and the stacktrace below are more or less\nirrelevant details and do not help convince readers why this change\nis desirable. I'd suggest removing it (and perhaps place more stress\non explaining why storing symbolic links in LFS is a bad practice\nnobody uses, but if it is so obvious perhaps the single sentence\nexplanation you have above would be sufficient).\n\nI know I can ask brian to take a look at this change from LFS angle,\nbut who's authoritatively reviewing \"git p4\" related changes these\ndays (Matthew, this is not a question for you, but to the list)?\n\nThanks.\n\n> Importing revision 42889 (100%)Traceback (most recent call last):\n> File \"./git/git-p4.py\", line 4621, in \n> main()\n> File \"./git/git-p4.py\", line 4615, in main\n> if not cmd.run(args):\n> File \"./git/git-p4.py\", line 4225, in run\n> self.importRevisions(args, branch_arg_given)\n> File \"./git/git-p4.py\", line 4002, in importRevisions\n> self.importChanges(changes)\n> File \"./git/git-p4.py\", line 3876, in importChanges\n> self.initialParent)\n> File \"./git/git-p4.py\", line 3496, in commit\n> self.streamP4Files(files)\n> File \"./git/git-p4.py\", line 3336, in streamP4Files\n> cb=streamP4FilesCbSelf)\n> File \"./git/git-p4.py\", line 910, in p4CmdList\n> cb(entry)\n> File \"./git/git-p4.py\", line 3321, in streamP4FilesCbSelf\n> self.streamP4FilesCb(entry)\n> File \"./git/git-p4.py\", line 3266, in streamP4FilesCb\n> self.streamOneP4File(self.stream_file, self.stream_contents)\n> File \"./git/git-p4.py\", line 3217, in streamOneP4File\n> git_mode, contents = self.largeFileSystem.processContent(git_mode, relPath, contents)\n> File \"./git/git-p4.py\", line 1656, in processContent\n> return LargeFileSystem.processContent(self, git_mode, relPath, contents)\n> File \"./git/git-p4.py\", line 1526, in processContent\n> contentTempFile = self.generateTempFile(contents)\n> File \"./git/git-p4.py\", line 1488, in generateTempFile\n> contentFile.write(d)\n> File \"/usr/lib64/python3.6/tempfile.py\", line 485, in func_wrapper\n> return func(*args, **kwargs)\n> TypeError: a bytes-like object is required, not 'str'\n>\n> Signed-off-by: Matthew McClain \n> ---\n> git-p4.py | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index d26a980e5a..f5fda2a3dc 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1522,6 +1522,10 @@ def processContent(self, git_mode, relPath, contents):\n> file is stored in the large file system and handles all necessary\n> steps.\n> \"\"\"\n> + # no need to store symlinks in LFS (generateTempFile wants bytes)\n> + if git_mode == \"120000\":\n> + return (git_mode, contents)\n> +\n> if self.exceedsLargeFileThreshold(relPath, contents) or self.hasLargeFileExtension(relPath):\n> contentTempFile = self.generateTempFile(contents)\n> pointer_git_mode, contents, localLargeFile = self.generatePointer(contentTempFile)\n"},{"id":"483457","messageId":"ZTBbqobbqQpxHPI2@tapette.crustytoothpaste.net","threadId":"60394","inReplyTo":"xmqqil73sp8l.fsf@gitster.g","subject":"Re: [PATCH] git-p4 shouldn't attempt to store symlinks in LFS","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-18T22:26:50Z","receivedAt":"2023-10-18T22:26:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-10-18 at 19:30:02, Junio C Hamano wrote:\n> Matthew McClain writes:\n> \n> > If a symlink in your Perforce repository matches\n> > git-p4.largeFileExtensions, git-p4.py will attempt to put the symlink in\n> > LFS but will blow up when it passes a string to generateTempFile.\n> >\n> > Git LFS normal behavior does not stash symlinks in LFS.\n> \n> I am not a \"git p4\" (or \"git lfs\") user, but if nobody uses LFS to\n> store symbolic links (which is very much understandable given what\n> it was invented for), then that alone is a good enough reason to\n> avoid the regular blob codepath, and that is true even if\n> generateTempFile accepted a str and silently converted it to bytes\n> to help callers, no?\n\nGit LFS doesn't store symlinks because smudge/clean filters don't handle\nsymlinks. They never get passed to the filter process nor the\nsmudge/clean filters, nor could that occur without a change to the\nprotocol or command-line interface. Unless Git learned how to send them\nto the filters, Git LFS would have a hard time using them in any useful\nway.\n\nAlso, for Git LFS, whose goal is to move large files out of the\nrepository history, symlinks are typically not an interesting thing to\nprocess because they are functionally limited to 4 KiB or a similar size\non most systems.\n\n> So \"but will blow up ...\" and the stacktrace below are more or less\n> irrelevant details and do not help convince readers why this change\n> is desirable. I'd suggest removing it (and perhaps place more stress\n> on explaining why storing symbolic links in LFS is a bad practice\n> nobody uses, but if it is so obvious perhaps the single sentence\n> explanation you have above would be sufficient).\n\nHopefully my explanation above can be part of that commit message.\n\n> I know I can ask brian to take a look at this change from LFS angle,\n> but who's authoritatively reviewing \"git p4\" related changes these\n> days (Matthew, this is not a question for you, but to the list)?\n\nIt looks fine to me from an LFS angle, but I know nothing about Perforce\nor git-p4.\n\nAlso, as a minor request, it would be great if you could continue to\nemail me at my personal address, since that's the address with which I\nread the list. My work address appearing on series is more of a\nreflection that I'm more frequently finding time to work on things on\nwork time (hence the work address) versus personal time (where I'd be\nusing my personal email for patches). I've fixed it up above.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"483458","messageId":"xmqqttqnpn0d.fsf@gitster.g","threadId":"60394","inReplyTo":"ZTBbqobbqQpxHPI2@tapette.crustytoothpaste.net","subject":"Re: [PATCH] git-p4 shouldn't attempt to store symlinks in LFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-18T22:46:26Z","receivedAt":"2023-10-18T22:46:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Also, as a minor request, it would be great if you could continue to\n> email me at my personal address, since that's the address with which I\n> read the list. My work address appearing on series is more of a\n> reflection that I'm more frequently finding time to work on things on\n> work time (hence the work address) versus personal time (where I'd be\n> using my personal email for patches). I've fixed it up above.\n\nThanks.\n\nWith\n\n [alias]\n\twho = !sh -c 'git log -1 --format=\\\"%an <%ae>\\\" --author=\\\"$1\\\"' -\n\nI say \"git who sandals\" to get your e-mail address, but I guess I\nshould use %aE instead to let .mailmap kick in.\n"},{"id":"483468","messageId":"20231019002558.867830-1-mmcclain@noprivs.com","threadId":"60394","inReplyTo":"ZTBbqobbqQpxHPI2@tapette.crustytoothpaste.net","subject":"[PATCH v2] git-p4 shouldn't attempt to store symlinks in LFS","fromName":"Matthew McClain","fromEmail":"mmcclain@noprivs.com","sentAt":"2023-10-19T00:25:58Z","receivedAt":"2023-10-19T00:26:26Z","isPatch":true,"sender":{"key":"mmcclain@noprivs.com","avatar":null},"body":"git-p4.py would attempt to put a symlink in LFS if its file extension\nmatched git-p4.largeFileExtensions.\n\nGit LFS doesn't store symlinks because smudge/clean filters don't handle\nsymlinks. They never get passed to the filter process nor the\nsmudge/clean filters, nor could that occur without a change to the\nprotocol or command-line interface. Unless Git learned how to send them\nto the filters, Git LFS would have a hard time using them in any useful\nway.\n\nGit LFS's goal is to move large files out of the repository history, and\nsymlinks are functionally limited to 4 KiB or a similar size on most\nsystems.\n\nSigned-off-by: Matthew McClain \n---\n git-p4.py | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex d26a980e5a..0eb3bb4c47 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1522,6 +1522,10 @@ def processContent(self, git_mode, relPath, contents):\n file is stored in the large file system and handles all necessary\n steps.\n \"\"\"\n+ # symlinks aren't processed by smudge/clean filters\n+ if git_mode == \"120000\":\n+ return (git_mode, contents)\n+\n if self.exceedsLargeFileThreshold(relPath, contents) or self.hasLargeFileExtension(relPath):\n contentTempFile = self.generateTempFile(contents)\n pointer_git_mode, contents, localLargeFile = self.generatePointer(contentTempFile)\n-- \n2.39.3\n\n"},{"id":"483510","messageId":"xmqqzg0ewl1e.fsf@gitster.g","threadId":"60394","inReplyTo":"20231019002558.867830-1-mmcclain@noprivs.com","subject":"Re: [PATCH v2] git-p4 shouldn't attempt to store symlinks in LFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-19T17:59:25Z","receivedAt":"2023-10-19T17:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew McClain writes:\n\n> git-p4.py would attempt to put a symlink in LFS if its file extension\n> matched git-p4.largeFileExtensions.\n>\n> Git LFS doesn't store symlinks because smudge/clean filters don't handle\n> symlinks. They never get passed to the filter process nor the\n> smudge/clean filters, nor could that occur without a change to the\n> protocol or command-line interface. Unless Git learned how to send them\n> to the filters, Git LFS would have a hard time using them in any useful\n> way.\n>\n> Git LFS's goal is to move large files out of the repository history, and\n> symlinks are functionally limited to 4 KiB or a similar size on most\n> systems.\n\nReads much better and easier to reason about.\n\nWill queue and see if \"git p4\" stakeholders object for a few days;\nif nothing happens, let's merge it down to 'next' and then to\n'master' as usual.\n\nThanks.\n\n> Signed-off-by: Matthew McClain \n> ---\n> git-p4.py | 4 ++++\n> 1 file changed, 4 insertions(+)\n\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index d26a980e5a..0eb3bb4c47 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1522,6 +1522,10 @@ def processContent(self, git_mode, relPath, contents):\n> file is stored in the large file system and handles all necessary\n> steps.\n> \"\"\"\n> + # symlinks aren't processed by smudge/clean filters\n> + if git_mode == \"120000\":\n> + return (git_mode, contents)\n> +\n> if self.exceedsLargeFileThreshold(relPath, contents) or self.hasLargeFileExtension(relPath):\n> contentTempFile = self.generateTempFile(contents)\n> pointer_git_mode, contents, localLargeFile = self.generatePointer(contentTempFile)\n"}]}