{"thread":{"id":"40310","subject":"[PATCH v4 1/5] git-p4: add optional type specifier to gitConfig reader","startedAt":"2015-09-09T11:59:05Z","lastAt":"2015-09-09T20:44:27Z","messageCount":9,"participants":["larsxschneider@gmail.com","Junio C Hamano","Lars Schneider"],"isPatch":true,"patchVersion":4,"patchTotal":5},"messages":[{"id":"269651","messageId":"1441799950-54633-1-git-send-email-larsxschneider@gmail.com","threadId":"40310","inReplyTo":null,"subject":"[PATCH v4 0/5] git-p4: add support for large file systems","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T11:59:05Z","receivedAt":"2015-09-09T11:59:05Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nDiff to v3:\n* add large file system mock class for testing (Thanks Luke!)\n* add early exit for compressed threshold: If a file is smaller then the threshold then we don't need to compress it.\n\nThanks,\nLars\n\nLars Schneider (5):\n  git-p4: add optional type specifier to gitConfig reader\n  git-p4: add gitConfigInt reader\n  git-p4: return an empty list if a list config has no values\n  git-p4: add support for large file systems\n  git-p4: add Git LFS backend for large file system\n\n Documentation/git-p4.txt   |  28 +++++\n git-p4.py                  | 223 +++++++++++++++++++++++++++++++++++++---\n t/t9823-git-p4-mock-lfs.sh | 106 +++++++++++++++++++\n t/t9824-git-p4-git-lfs.sh  | 249 +++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 593 insertions(+), 13 deletions(-)\n create mode 100755 t/t9823-git-p4-mock-lfs.sh\n create mode 100755 t/t9824-git-p4-git-lfs.sh\n\n--\n2.5.1\n"},{"id":"269650","messageId":"1441799950-54633-2-git-send-email-larsxschneider@gmail.com","threadId":"40310","inReplyTo":"1441799950-54633-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v4 1/5] git-p4: add optional type specifier to gitConfig reader","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T11:59:06Z","receivedAt":"2015-09-09T11:59:06Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nThe functions “gitConfig” and “gitConfigBool” are almost identical. Make “gitConfig” more generic by adding an optional type specifier. Use the type specifier “—bool” with “gitConfig” to implement “gitConfigBool. This prepares the implementation of other type specifiers such as “—int”.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n git-p4.py | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 073f87b..c139cab 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -604,9 +604,12 @@ def gitBranchExists(branch):\n \n _gitConfig = {}\n \n-def gitConfig(key):\n+def gitConfig(key, typeSpecifier=None):\n     if not _gitConfig.has_key(key):\n-        cmd = [ \"git\", \"config\", key ]\n+        cmd = [ \"git\", \"config\" ]\n+        if typeSpecifier:\n+            cmd += [ typeSpecifier ]\n+        cmd += [ key ]\n         s = read_pipe(cmd, ignore_error=True)\n         _gitConfig[key] = s.strip()\n     return _gitConfig[key]\n@@ -617,10 +620,7 @@ def gitConfigBool(key):\n        in the config.\"\"\"\n \n     if not _gitConfig.has_key(key):\n-        cmd = [ \"git\", \"config\", \"--bool\", key ]\n-        s = read_pipe(cmd, ignore_error=True)\n-        v = s.strip()\n-        _gitConfig[key] = v == \"true\"\n+        _gitConfig[key] = gitConfig(key, '--bool') == \"true\"\n     return _gitConfig[key]\n \n def gitConfigList(key):\n-- \n2.5.1\n"},{"id":"269653","messageId":"1441799950-54633-3-git-send-email-larsxschneider@gmail.com","threadId":"40310","inReplyTo":"1441799950-54633-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v4 2/5] git-p4: add gitConfigInt reader","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T11:59:07Z","receivedAt":"2015-09-09T11:59: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\nAdd a git config reader for integer variables. Please note that the git config implementation automatically supports k, m, and g suffixes.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n git-p4.py | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex c139cab..40ad4ae 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -623,6 +623,17 @@ def gitConfigBool(key):\n         _gitConfig[key] = gitConfig(key, '--bool') == \"true\"\n     return _gitConfig[key]\n \n+def gitConfigInt(key):\n+    if not _gitConfig.has_key(key):\n+        cmd = [ \"git\", \"config\", \"--int\", key ]\n+        s = read_pipe(cmd, ignore_error=True)\n+        v = s.strip()\n+        try:\n+            _gitConfig[key] = int(gitConfig(key, '--int'))\n+        except ValueError:\n+            _gitConfig[key] = None\n+    return _gitConfig[key]\n+\n def gitConfigList(key):\n     if not _gitConfig.has_key(key):\n         s = read_pipe([\"git\", \"config\", \"--get-all\", key], ignore_error=True)\n-- \n2.5.1\n"},{"id":"269652","messageId":"1441799950-54633-4-git-send-email-larsxschneider@gmail.com","threadId":"40310","inReplyTo":"1441799950-54633-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v4 3/5] git-p4: return an empty list if a list config has no values","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T11:59:08Z","receivedAt":"2015-09-09T11:59: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 git-p4.py | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 40ad4ae..90d3b90 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -638,6 +638,8 @@ def gitConfigList(key):\n     if not _gitConfig.has_key(key):\n         s = read_pipe([\"git\", \"config\", \"--get-all\", key], ignore_error=True)\n         _gitConfig[key] = s.strip().split(os.linesep)\n+        if _gitConfig[key] == ['']:\n+            _gitConfig[key] = []\n     return _gitConfig[key]\n \n def p4BranchesInGit(branchesAreInRemotes=True):\n-- \n2.5.1\n"},{"id":"269654","messageId":"1441799950-54633-5-git-send-email-larsxschneider@gmail.com","threadId":"40310","inReplyTo":"1441799950-54633-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v4 4/5] git-p4: add support for large file systems","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T11:59:09Z","receivedAt":"2015-09-09T11:59: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\nPerforce repositories can contain large (binary) files. Migrating these\nrepositories to Git generates very large local clones. External storage\nsystems such as Git LFS [1], Git Fat [2], or Git Media [2] try to\naddress this problem.\n\nAdd a generic mechanism to detect large files based on extension,\nuncompressed size, and/or compressed size.\n\n[1] https://git-lfs.github.com/\n[2] https://github.com/jedbrown/git-fat\n[3] https://github.com/alebedev/git-media\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n Documentation/git-p4.txt   |  26 ++++++++\n git-p4.py                  | 150 ++++++++++++++++++++++++++++++++++++++++++---\n t/t9823-git-p4-mock-lfs.sh | 106 ++++++++++++++++++++++++++++++++\n 3 files changed, 273 insertions(+), 9 deletions(-)\n create mode 100755 t/t9823-git-p4-mock-lfs.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex 82aa5d6..e0d0239 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -510,6 +510,32 @@ git-p4.useClientSpec::\n \toption '--use-client-spec'.  See the \"CLIENT SPEC\" section above.\n \tThis variable is a boolean, not the name of a p4 client.\n \n+git-p4.largeFileSystem::\n+\tSpecify the system that is used used for large (binary) files.\n+\tPlease note that most of these large file systems depend on the\n+\tGit clean/smudge filters. These filters are not applied through\n+\tgit-p4. You need to create a fresh clone of the repository after\n+\trunning git-p4.\n+\n+git-p4.largeFileExtensions::\n+\tAll files matching a file extension in the list will be processed\n+\tby the large file system. Do not prefix the extensions with '.'.\n+\n+git-p4.largeFileThreshold::\n+\tAll files with an uncompressed size exceeding the threshold will be\n+\tprocessed by the large file system. By default the threshold is\n+\tdefined in bytes. Add the suffix k, m, or g to change the unit.\n+\n+git-p4.largeFileCompressedThreshold::\n+\tAll files with a compressed size exceeding the threshold will be\n+\tprocessed by the large file system. This option might significantly\n+\tslow down your clone/sync process. By default the threshold is\n+\tdefined in bytes. Add the suffix k, m, or g to change the unit.\n+\n+git-p4.pushLargeFiles::\n+\tBoolean variable which defines if large files are automatically\n+\tpushed to a server.\n+\n Submit variables\n ~~~~~~~~~~~~~~~~\n git-p4.detectRenames::\ndiff --git a/git-p4.py b/git-p4.py\nindex 90d3b90..ada4174 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -22,6 +22,8 @@ import platform\n import re\n import shutil\n import stat\n+import zipfile\n+import zlib\n \n try:\n     from subprocess import CalledProcessError\n@@ -922,6 +924,50 @@ def wildcard_present(path):\n     m = re.search(\"[*#@%]\", path)\n     return m is not None\n \n+def largeFileSystem():\n+    try:\n+        largeFileSystem = getattr(sys.modules[__name__], gitConfig('git-p4.largeFileSystem'))\n+        assert(hasattr(largeFileSystem, \"attributeDescription\"))\n+        assert(hasattr(largeFileSystem, \"attributeFilter\"))\n+        assert(hasattr(largeFileSystem, \"generatePointer\"))\n+        assert(hasattr(largeFileSystem, \"pushFile\"))\n+        return largeFileSystem\n+    except AttributeError as e:\n+        die('Large file system not supported: %s' % gitConfig('git-p4.largeFileSystem'))\n+\n+class MockLFS:\n+    \"\"\"Mock large file system for testing.\"\"\"\n+\n+    @staticmethod\n+    def attributeDescription():\n+        return 'Mock LFS'\n+\n+    @staticmethod\n+    def attributeFilter():\n+        return 'mock'\n+\n+    @staticmethod\n+    def generatePointer(cloneDestination, contentFile):\n+        \"\"\"The pointer content is the original content prefixed with \"pointer-\".\n+           The local filename of the large file storage is derived from the file content.\n+           \"\"\"\n+        with open(contentFile, 'r') as f:\n+            content = next(f)\n+            gitMode = '100644'\n+            pointerContents = 'pointer-' + content\n+            localLargeFile = os.path.join(cloneDestination, '.git', 'mock-storage', 'local', content[:-1])\n+            return (gitMode, pointerContents, localLargeFile)\n+\n+    @staticmethod\n+    def pushFile(localLargeFile):\n+        \"\"\"The remote filename of the large file storage is the same as the local\n+           one but in a different directory.\n+           \"\"\"\n+        remotePath = os.path.join(os.path.dirname(localLargeFile), '..', 'remote')\n+        if not os.path.exists(remotePath):\n+            os.makedirs(remotePath)\n+        shutil.copyfile(localLargeFile, os.path.join(remotePath, os.path.basename(localLargeFile)))\n+\n class Command:\n     def __init__(self):\n         self.usage = \"usage: %prog [options]\"\n@@ -2038,6 +2084,7 @@ class P4Sync(Command, P4UserMap):\n         self.clientSpecDirs = None\n         self.tempBranches = []\n         self.tempBranchLocation = \"git-p4-tmp\"\n+        self.largeFiles = set()\n \n         if gitConfig(\"git-p4.syncFromOrigin\") == \"false\":\n             self.syncWithOrigin = False\n@@ -2158,6 +2205,88 @@ class P4Sync(Command, P4UserMap):\n \n         return branches\n \n+    def writeToGitStream(self, gitMode, relPath, contents):\n+        self.gitStream.write('M %s inline %s\\n' % (gitMode, relPath))\n+        self.gitStream.write('data %d\\n' % sum(len(d) for d in contents))\n+        for d in contents:\n+            self.gitStream.write(d)\n+        self.gitStream.write('\\n')\n+\n+    def writeGitAttributesToStream(self):\n+        self.writeToGitStream(\n+            '100644',\n+            '.gitattributes',\n+            [\n+                '\\n',\n+                '#\\n',\n+                '# %s\\n' % largeFileSystem().attributeDescription(),\n+                '#\\n',\n+            ] +\n+            ['*.' + f.replace(' ', '[:space:]') + ' filter=%s -text\\n'\n+                % largeFileSystem().attributeFilter()\n+                for f in sorted(gitConfigList(\"git-p4.largeFileExtensions\"))\n+            ] +\n+            ['/' + f.replace(' ', '[:space:]') + ' filter=%s -text\\n'\n+                % largeFileSystem().attributeFilter()\n+                for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)\n+            ]\n+        )\n+\n+    def hasLargeFileExtension(self, relPath):\n+        return reduce(\n+            lambda a, b: a or b,\n+            [relPath.endswith('.' + e) for e in gitConfigList('git-p4.largeFileExtensions')],\n+            False\n+        )\n+\n+    def exceedsLargeFileThreshold(self, relPath, contents):\n+        if gitConfigInt('git-p4.largeFileThreshold'):\n+            contentsSize = sum(len(d) for d in contents)\n+            if contentsSize > gitConfigInt('git-p4.largeFileThreshold'):\n+                return True\n+        if gitConfigInt('git-p4.largeFileCompressedThreshold'):\n+            contentsSize = sum(len(d) for d in contents)\n+            if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):\n+                return False\n+            contentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n+            for d in contents:\n+                contentFile.write(d)\n+            contentFile.flush()\n+            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n+            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')\n+            zf.write(contentFile.name, compress_type=zipfile.ZIP_DEFLATED)\n+            zf.close()\n+            compressedContentsSize = zf.infolist()[0].compress_size\n+            os.remove(contentFile.name)\n+            os.remove(compressedContentFile.name)\n+            if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):\n+                return True\n+        return False\n+\n+    def moveToLargeFileSystem(self, relPath, contents):\n+        # Write P4 content to temp file\n+        contentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)\n+        for d in contents:\n+            contentFile.write(d)\n+        contentFile.flush()\n+        contentFile.close()\n+        (git_mode, contents, localLargeFile) = \\\n+            largeFileSystem().generatePointer(self.cloneDestination, contentFile.name)\n+        # Move temp file to final location in large file system\n+        largeFileDir = os.path.dirname(localLargeFile)\n+        if not os.path.isdir(largeFileDir):\n+            os.makedirs(largeFileDir)\n+        shutil.move(contentFile.name, localLargeFile)\n+        if verbose:\n+            sys.stderr.write(\"%s moved to large file system (%s)\\n\" % (relPath, localLargeFile))\n+\n+        if gitConfigBool('git-p4.pushLargeFiles'):\n+            largeFileSystem().pushFile(localLargeFile)\n+\n+        self.largeFiles.add(relPath)\n+        self.writeGitAttributesToStream()\n+        return (git_mode, contents)\n+\n     # output one file from the P4 stream\n     # - helper for streamP4Files\n \n@@ -2226,17 +2355,16 @@ class P4Sync(Command, P4UserMap):\n             text = regexp.sub(r'$\\1$', text)\n             contents = [ text ]\n \n-        self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n+        if relPath == '.gitattributes':\n+            die('.gitattributes already exists in P4.')\n \n-        # total length...\n-        length = 0\n-        for d in contents:\n-            length = length + len(d)\n+        if (gitConfig('git-p4.largeFileSystem') and\n+            (   self.exceedsLargeFileThreshold(relPath, contents) or\n+                self.hasLargeFileExtension(relPath)\n+            )):\n+            (git_mode, contents) = self.moveToLargeFileSystem(relPath, contents)\n \n-        self.gitStream.write(\"data %d\\n\" % length)\n-        for d in contents:\n-            self.gitStream.write(d)\n-        self.gitStream.write(\"\\n\")\n+        self.writeToGitStream(git_mode, relPath, contents)\n \n     def streamOneP4Deletion(self, file):\n         relPath = self.stripRepoPath(file['path'], self.branchPrefixes)\n@@ -2244,6 +2372,10 @@ class P4Sync(Command, P4UserMap):\n             sys.stderr.write(\"delete %s\\n\" % relPath)\n         self.gitStream.write(\"D %s\\n\" % relPath)\n \n+        if relPath in self.largeFiles:\n+            self.largeFiles.remove(relPath)\n+            self.writeGitAttributesToStream()\n+\n     # handle another chunk of streaming data\n     def streamP4FilesCb(self, marshalled):\n \ndiff --git a/t/t9823-git-p4-mock-lfs.sh b/t/t9823-git-p4-mock-lfs.sh\nnew file mode 100755\nindex 0000000..6e4fd54\n--- /dev/null\n+++ b/t/t9823-git-p4-mock-lfs.sh\n@@ -0,0 +1,106 @@\n+#!/bin/sh\n+\n+test_description='Clone repositories and store files in Mock LFS'\n+\n+. ./lib-git-p4.sh\n+\n+test_file_in_mock () {\n+\tFILE=\"$1\"\n+\tCONTENT=\"$2\"\n+\tLOCAL_STORAGE=\".git/mock-storage/local/$CONTENT\"\n+\tSERVER_STORAGE=\".git/mock-storage/remote/$CONTENT\"\n+\techo \"pointer-$CONTENT\" >expect_pointer\n+\techo \"$CONTENT\" >expect_content\n+\ttest_path_is_file \"$FILE\" &&\n+\ttest_path_is_file \"$LOCAL_STORAGE\" &&\n+\ttest_path_is_file \"$SERVER_STORAGE\" &&\n+\ttest_cmp expect_pointer \"$FILE\" &&\n+\ttest_cmp expect_content \"$LOCAL_STORAGE\" &&\n+\ttest_cmp expect_content \"$SERVER_STORAGE\"\n+}\n+\n+test_file_count_in_dir () {\n+\tDIR=\"$1\"\n+\tEXPECTED_COUNT=\"$2\"\n+\tfind \"$DIR\" -type f >actual\n+\ttest_line_count = $EXPECTED_COUNT actual\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'Create repo with binary files' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\techo \"content 1 txt 23 bytes\" >file1.txt &&\n+\t\tp4 add file1.txt &&\n+\t\techo \"content 2-3 bin 25 bytes\" >file2.dat &&\n+\t\tp4 add file2.dat &&\n+\t\tp4 submit -d \"Add text and binary file\" &&\n+\n+\t\tmkdir \"path with spaces\" &&\n+\t\techo \"content 2-3 bin 25 bytes\" >\"path with spaces/file3.bin\" &&\n+\t\tp4 add \"path with spaces/file3.bin\" &&\n+\t\tp4 submit -d \"Add another binary file with same content and spaces in path\" &&\n+\n+\t\techo \"content 4 bin 26 bytes XX\" >file4.bin &&\n+\t\tp4 add file4.bin &&\n+\t\tp4 submit -d \"Add another binary file with different content\"\n+\t)\n+'\n+\n+test_expect_success 'Store files in Mock based on size (>24 bytes)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem MockLFS &&\n+\t\tgit config git-p4.largeFileThreshold 24 &&\n+\t\tgit config git-p4.pushLargeFiles True &&\n+\t\tgit p4 clone --destination=\"$git\" //depot@all &&\n+\n+\t\ttest_file_in_mock file2.dat \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_in_mock \"path with spaces/file3.bin\" \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_in_mock file4.bin \"content 4 bin 26 bytes XX\" &&\n+\n+\t\ttest_file_count_in_dir \".git/mock-storage/local\" 2 &&\n+\t\ttest_file_count_in_dir \".git/mock-storage/remote\" 2 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Mock LFS\n+\t\t#\n+\t\t/file2.dat filter=mock -text\n+\t\t/file4.bin filter=mock -text\n+\t\t/path[:space:]with[:space:]spaces/file3.bin filter=mock -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'Clone repo with existing .gitattributes file' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\techo \"*.txt text\" >.gitattributes &&\n+\t\tp4 add .gitattributes &&\n+\t\tp4 submit -d \"Add .gitattributes\"\n+\t) &&\n+\n+\ttest_must_fail git p4 clone --use-client-spec --destination=\"$git\" //depot 2>errs &&\n+\tgrep \".gitattributes already exists in P4.\" errs\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n2.5.1\n"},{"id":"269655","messageId":"1441799950-54633-6-git-send-email-larsxschneider@gmail.com","threadId":"40310","inReplyTo":"1441799950-54633-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v4 5/5] git-p4: add Git LFS backend for large file system","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T11:59:10Z","receivedAt":"2015-09-09T11:59:10Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nAdd example implementation including test cases for the large file\nsystem using Git LFS.\n\nPushing files to the Git LFS server is not tested.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n Documentation/git-p4.txt  |   4 +-\n git-p4.py                 |  52 ++++++++++\n t/t9824-git-p4-git-lfs.sh | 249 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 304 insertions(+), 1 deletion(-)\n create mode 100755 t/t9824-git-p4-git-lfs.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex e0d0239..3168a64 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -515,7 +515,9 @@ git-p4.largeFileSystem::\n \tPlease note that most of these large file systems depend on the\n \tGit clean/smudge filters. These filters are not applied through\n \tgit-p4. You need to create a fresh clone of the repository after\n-\trunning git-p4.\n+\trunning git-p4. Only Git LFS [1] is supported right now. Download\n+\tand install the Git LFS command line extension to use this option.\n+\t[1] https://git-lfs.github.com/\n \n git-p4.largeFileExtensions::\n \tAll files matching a file extension in the list will be processed\ndiff --git a/git-p4.py b/git-p4.py\nindex ada4174..58bdaf1 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -968,6 +968,58 @@ class MockLFS:\n             os.makedirs(remotePath)\n         shutil.copyfile(localLargeFile, os.path.join(remotePath, os.path.basename(localLargeFile)))\n \n+class GitLFS:\n+    \"\"\"Git LFS as backend for the git-p4 large file system.\n+       See https://git-lfs.github.com/ for details.\"\"\"\n+\n+    @staticmethod\n+    def attributeDescription():\n+        \"\"\"Return a description which is used to mark LFS entries in the\n+           .gitattributes file.\"\"\"\n+        return 'Git LFS (see https://git-lfs.github.com/)'\n+\n+    @staticmethod\n+    def attributeFilter():\n+        \"\"\"Return the name of the filter which is used for LFS entries in the\n+           .gitattributes file.\"\"\"\n+        return 'lfs'\n+\n+    @staticmethod\n+    def generatePointer(cloneDestination, contentFile):\n+        \"\"\"Generate a Git LFS pointer for the content. Return LFS Pointer file\n+           mode and content which is stored in the Git repository instead of\n+           the actual content. Return also the new location of the actual\n+           content.\n+           \"\"\"\n+        pointerProcess = subprocess.Popen(\n+            ['git', 'lfs', 'pointer', '--file=' + contentFile],\n+            stdout=subprocess.PIPE\n+        )\n+        pointerFile = pointerProcess.stdout.read()\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+        localLargeFile = os.path.join(\n+            cloneDestination,\n+            '.git', 'lfs', 'objects', oid[:2], oid[2:4],\n+            oid,\n+        )\n+        # LFS Spec states that pointer files should not have the executable bit set.\n+        gitMode = '100644'\n+        return (gitMode, pointerContents, localLargeFile)\n+\n+    @staticmethod\n+    def pushFile(localLargeFile):\n+        \"\"\"Push the actual content which is not stored in the Git repository to\n+        a server.\"\"\"\n+        uploadProcess = subprocess.Popen(\n+            ['git', 'lfs', 'push', '--object-id', 'origin', os.path.basename(localLargeFile)]\n+        )\n+        if uploadProcess.wait():\n+            die('git-lfs push command failed. Did you define a remote?')\n+\n class Command:\n     def __init__(self):\n         self.usage = \"usage: %prog [options]\"\ndiff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh\nnew file mode 100755\nindex 0000000..f46768f\n--- /dev/null\n+++ b/t/t9824-git-p4-git-lfs.sh\n@@ -0,0 +1,249 @@\n+#!/bin/sh\n+\n+test_description='Clone repositories and store files in Git LFS'\n+\n+. ./lib-git-p4.sh\n+\n+git lfs help >/dev/null 2>&1 || {\n+\tskip_all='skipping git p4 Git LFS tests; Git LFS not found'\n+\ttest_done\n+}\n+\n+test_file_in_lfs () {\n+\tFILE=\"$1\"\n+\tSIZE=\"$2\"\n+\tEXPECTED_CONTENT=\"$3\"\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/${HASH:0:2}/${HASH:2:2}/$HASH\"\n+\techo $EXPECTED_CONTENT >expect\n+\ttest_path_is_file \"$FILE\" &&\n+\ttest_path_is_file \"$LFS_FILE\" &&\n+\ttest_cmp expect \"$LFS_FILE\"\n+}\n+\n+test_file_count_in_dir () {\n+\tDIR=\"$1\"\n+\tEXPECTED_COUNT=\"$2\"\n+\tfind \"$DIR\" -type f >actual\n+\ttest_line_count = $EXPECTED_COUNT actual\n+}\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'Create repo with binary files' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\techo \"content 1 txt 23 bytes\" >file1.txt &&\n+\t\tp4 add file1.txt &&\n+\t\techo \"content 2-3 bin 25 bytes\" >file2.dat &&\n+\t\tp4 add file2.dat &&\n+\t\tp4 submit -d \"Add text and binary file\" &&\n+\n+\t\tmkdir \"path with spaces\" &&\n+\t\techo \"content 2-3 bin 25 bytes\" >\"path with spaces/file3.bin\" &&\n+\t\tp4 add \"path with spaces/file3.bin\" &&\n+\t\tp4 submit -d \"Add another binary file with same content and spaces in path\" &&\n+\n+\t\techo \"content 4 bin 26 bytes XX\" >file4.bin &&\n+\t\tp4 add file4.bin &&\n+\t\tp4 submit -d \"Add another binary file with different content\"\n+\t)\n+'\n+\n+test_expect_success 'Store files in LFS based on size (>24 bytes)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem GitLFS &&\n+\t\tgit config git-p4.largeFileThreshold 24 &&\n+\t\tgit p4 clone --destination=\"$git\" //depot@all &&\n+\n+\t\ttest_file_in_lfs file2.dat 25 \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_in_lfs \"path with spaces/file3.bin\" 25 \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_in_lfs file4.bin 26 \"content 4 bin 26 bytes XX\" &&\n+\n+\t\ttest_file_count_in_dir \".git/lfs/objects\" 2 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Git LFS (see https://git-lfs.github.com/)\n+\t\t#\n+\t\t/file2.dat filter=lfs -text\n+\t\t/file4.bin filter=lfs -text\n+\t\t/path[:space:]with[:space:]spaces/file3.bin filter=lfs -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'Store files in LFS based on size (>25 bytes)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem GitLFS &&\n+\t\tgit config git-p4.largeFileThreshold 25 &&\n+\t\tgit p4 clone --destination=\"$git\" //depot@all &&\n+\n+\t\ttest_file_in_lfs file4.bin 26 \"content 4 bin 26 bytes XX\" &&\n+\t\ttest_file_count_in_dir \".git/lfs/objects\" 1 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Git LFS (see https://git-lfs.github.com/)\n+\t\t#\n+\t\t/file4.bin filter=lfs -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'Store files in LFS based on extension (dat)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem GitLFS &&\n+\t\tgit config git-p4.largeFileExtensions dat &&\n+\t\tgit p4 clone --destination=\"$git\" //depot@all &&\n+\n+\t\ttest_file_in_lfs file2.dat 25 \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_count_in_dir \".git/lfs/objects\" 1 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Git LFS (see https://git-lfs.github.com/)\n+\t\t#\n+\t\t*.dat filter=lfs -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'Store files in LFS based on size (>25 bytes) and extension (dat)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem GitLFS &&\n+\t\tgit config git-p4.largeFileExtensions dat &&\n+\t\tgit config git-p4.largeFileThreshold 25 &&\n+\t\tgit p4 clone --destination=\"$git\" //depot@all &&\n+\n+\t\ttest_file_in_lfs file2.dat 25 \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_in_lfs file4.bin 26 \"content 4 bin 26 bytes XX\" &&\n+\t\ttest_file_count_in_dir \".git/lfs/objects\" 2 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Git LFS (see https://git-lfs.github.com/)\n+\t\t#\n+\t\t*.dat filter=lfs -text\n+\t\t/file4.bin filter=lfs -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'Remove file from repo and store files in LFS based on size (>24 bytes)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 delete file4.bin &&\n+\t\tp4 submit -d \"Remove file\"\n+\t) &&\n+\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem GitLFS &&\n+\t\tgit config git-p4.largeFileThreshold 24 &&\n+\t\tgit p4 clone --destination=\"$git\" //depot@all &&\n+\n+\t\ttest_file_in_lfs file2.dat 25 \"content 2-3 bin 25 bytes\" &&\n+\t\ttest_file_in_lfs \"path with spaces/file3.bin\" 25 \"content 2-3 bin 25 bytes\" &&\n+\t\t! test_path_is_file file4.bin &&\n+\t\ttest_file_count_in_dir \".git/lfs/objects\" 2 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Git LFS (see https://git-lfs.github.com/)\n+\t\t#\n+\t\t/file2.dat filter=lfs -text\n+\t\t/path[:space:]with[:space:]spaces/file3.bin filter=lfs -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'Add big files to repo and store files in LFS based on compressed size (>28 bytes)' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\techo \"content 5 bin 40 bytes XXXXXXXXXXXXXXXX\" >file5.bin &&\n+\t\tp4 add file5.bin &&\n+\t\tp4 submit -d \"Add file with small footprint after compression\" &&\n+\n+\t\techo \"content 6 bin 39 bytes XXXXXYYYYYZZZZZ\" >file6.bin &&\n+\t\tp4 add file6.bin &&\n+\t\tp4 submit -d \"Add file with large footprint after compression\"\n+\t) &&\n+\n+\tclient_view \"//depot/... //client/...\" &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit init . &&\n+\t\tgit config git-p4.useClientSpec true &&\n+\t\tgit config git-p4.largeFileSystem GitLFS &&\n+\t\tgit config git-p4.largeFileCompressedThreshold 28 &&\n+\t\t# We only import HEAD here (\"@all\" is missing!)\n+\t\tgit p4 clone --destination=\"$git\" //depot &&\n+\n+\t\ttest_file_in_lfs file6.bin 13 \"content 6 bin 39 bytes XXXXXYYYYYZZZZZ\"\n+\t\ttest_file_count_in_dir \".git/lfs/objects\" 1 &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\t#\n+\t\t# Git LFS (see https://git-lfs.github.com/)\n+\t\t#\n+\t\t/file6.bin filter=lfs -text\n+\t\tEOF\n+\t\ttest_path_is_file .gitattributes &&\n+\t\ttest_cmp expect .gitattributes\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n2.5.1\n"},{"id":"269662","messageId":"xmqq37yn7bni.fsf@gitster.mtv.corp.google.com","threadId":"40310","inReplyTo":"1441799950-54633-5-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v4 4/5] git-p4: add support for large file systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-09T17:20:01Z","receivedAt":"2015-09-09T17:20:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> @@ -2226,17 +2355,16 @@ class P4Sync(Command, P4UserMap):\n>              text = regexp.sub(r'$\\1$', text)\n>              contents = [ text ]\n>  \n> -        self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n> +        if relPath == '.gitattributes':\n> +            die('.gitattributes already exists in P4.')\n\nThis looks like an unfortunate limitation to me.\n\nIs it really necessary that you need to reject unrelated attributes\nthe user has (presumably for a good reason)?  It seems to me that\nyou only need to _add_ entries to make file extension based decision\nto send paths selectively to LFS?\n\nAlso the exact format of these attributes entries looks like very\nclosely tied to GitHub LFS and not generic (for example, there is no\nreason to expect that any large-file support would always use the\n\"filter\" mechanism or the gitattributes mechanism for that\nmatter), ...\n\n+    def writeGitAttributesToStream(self):\n+        self.writeToGitStream(\n+            '100644',\n+            '.gitattributes',\n+            [\n+                '\\n',\n+                '#\\n',\n+                '# %s\\n' % largeFileSystem().attributeDescription(),\n+                '#\\n',\n+            ] +\n+            ['*.' + f.replace(' ', '[:space:]') + ' filter=%s -text\\n'\n+                % largeFileSystem().attributeFilter()\n+                for f in sorted(gitConfigList(\"git-p4.largeFileExtensions\"))\n+            ] +\n+            ['/' + f.replace(' ', '[:space:]') + ' filter=%s -text\\n'\n+                % largeFileSystem().attributeFilter()\n+                for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)\n+            ]\n+        )\n+\n\n... so while I can see the code like the above needs to exist\nsomewhere in \"git p4\" to support GitHub LFS, I am not sure if it\nbelongs to the generic part of the code.  For the same reason, I do\nnot know if these replacements with largeFileSystem().getters() are\nreally adding much value.\n\nHow is collaboration between those who talk to the same p4 depot\nbacked by GitHub LFS expected to work?  You use config to set size\nlimits and list of file extensions in your repository, and grab new\nchanges from p4 and turn them into Git commits (with pointers to LFS\nand the .gitattributes file that records your choice of the config).\nI as a new member to the same project come, clone the resulting Git\nrepository from you and then what do I do before talking to the same\np4 depot to further update the Git history?  Are the values recorded\nin .gitattributes (which essentially were derived from your\nconfiguration) somehow reflected back automatically to my config so\nthat our world view would become consistent?  Perhaps you added\n'iso' to largeFileExtensions before I cloned from you, and that\nwould be present in the copy of .gitattributes I obtained from you.\nI may be trying to add a new \".iso\" file, and I presume that an\nexisting .gitattributes entry you created based on the file\nextension automatically covers it from the LFS side, but does my\n\"git p4 submit\" also know what to do, or would it be broken until I\nadd a set of configrations that matches yours?\n"},{"id":"269666","messageId":"353BE0F5-8262-4094-95E6-982C29570F6B@gmail.com","threadId":"40310","inReplyTo":"xmqq37yn7bni.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 4/5] git-p4: add support for large file systems","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-09T20:09:52Z","receivedAt":"2015-09-09T20:09:52Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 09 Sep 2015, at 19:20, Junio C Hamano <gitster@pobox.com> wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> @@ -2226,17 +2355,16 @@ class P4Sync(Command, P4UserMap):\n>>             text = regexp.sub(r'$\\1$', text)\n>>             contents = [ text ]\n>> \n>> -        self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n>> +        if relPath == '.gitattributes':\n>> +            die('.gitattributes already exists in P4.')\n> \n> This looks like an unfortunate limitation to me.\n> \n> Is it really necessary that you need to reject unrelated attributes\n> the user has (presumably for a good reason)?  It seems to me that\n> you only need to _add_ entries to make file extension based decision\n> to send paths selectively to LFS?\nNo, it is not necessary. I will remove this limitation.\n\n> \n> Also the exact format of these attributes entries looks like very\n> closely tied to GitHub LFS and not generic (for example, there is no\n> reason to expect that any large-file support would always use the\n> \"filter\" mechanism or the gitattributes mechanism for that\n> matter), …\nAgreed. Instead of just the filter name I will replace everything after the pathname with a single git-p4 config value:\n\n['*.' + f.replace(' ', '[:space:]') + ' %s\\n' % largeFileSystem().attributes()\n    for f in sorted(gitConfigList(\"git-p4.largeFileExtensions\"))\n] +\n['/' + f.replace(' ', '[:space:]') + ' %s\\n' % largeFileSystem().attributes()\n    for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)\n]\n\nThis, of course, would only work for gitattributes based solutions. \n\n> \n> +    def writeGitAttributesToStream(self):\n> +        self.writeToGitStream(\n> +            '100644',\n> +            '.gitattributes',\n> +            [\n> +                '\\n',\n> +                '#\\n',\n> +                '# %s\\n' % largeFileSystem().attributeDescription(),\n> +                '#\\n',\n> +            ] +\n> +            ['*.' + f.replace(' ', '[:space:]') + ' filter=%s -text\\n'\n> +                % largeFileSystem().attributeFilter()\n> +                for f in sorted(gitConfigList(\"git-p4.largeFileExtensions\"))\n> +            ] +\n> +            ['/' + f.replace(' ', '[:space:]') + ' filter=%s -text\\n'\n> +                % largeFileSystem().attributeFilter()\n> +                for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)\n> +            ]\n> +        )\n> +\n> \n> ... so while I can see the code like the above needs to exist\n> somewhere in \"git p4\" to support GitHub LFS, I am not sure if it\n> belongs to the generic part of the code.  For the same reason, I do\n> not know if these replacements with largeFileSystem().getters() are\n> really adding much value.\nI have the impression you would prefer to move all the attributes code from the generic code to the GitLFS code? I will explore that solution and see if I can come up with a nice generic interface.\n \n> \n> How is collaboration between those who talk to the same p4 depot\n> backed by GitHub LFS expected to work?  You use config to set size\n> limits and list of file extensions in your repository, and grab new\n> changes from p4 and turn them into Git commits (with pointers to LFS\n> and the .gitattributes file that records your choice of the config).\n> I as a new member to the same project come, clone the resulting Git\n> repository from you and then what do I do before talking to the same\n> p4 depot to further update the Git history?  Are the values recorded\n> in .gitattributes (which essentially were derived from your\n> configuration) somehow reflected back automatically to my config so\n> that our world view would become consistent?  Perhaps you added\n> 'iso' to largeFileExtensions before I cloned from you, and that\n> would be present in the copy of .gitattributes I obtained from you.\n> I may be trying to add a new \".iso\" file, and I presume that an\n> existing .gitattributes entry you created based on the file\n> extension automatically covers it from the LFS side, but does my\n> \"git p4 submit\" also know what to do, or would it be broken until I\n> add a set of configrations that matches yours?\nWell, you have a very good point here. From my point of view you can use git-p4 in two ways:\n\n_Way 1_: Your project is stored in Perforce and it will stay in Perforce. You use git-p4 on a regular basis to interact with the Perforce repository.\n_Way 2_: Your project is stored in Perforce and you want to migrate it to Git. You use git-p4 once to perform the migration. Afterwards you never touch git-p4 or Perforce again.\n\nMy large file system feature is intended to be used only for _Way 2_ for exactly the reasons you mentioned. Would it still be OK to add this feature to git-p4? Maybe with a appropriate warning in the docs?\n\nThanks,\nLars\n\n "},{"id":"269667","messageId":"xmqqtwr35nmc.fsf@gitster.mtv.corp.google.com","threadId":"40310","inReplyTo":"353BE0F5-8262-4094-95E6-982C29570F6B@gmail.com","subject":"Re: [PATCH v4 4/5] git-p4: add support for large file systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-09T20:44:27Z","receivedAt":"2015-09-09T20:44:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n>> Also the exact format of these attributes entries looks like very\n>> closely tied to GitHub LFS and not generic (for example, there is no\n>> reason to expect that any large-file support would always use the\n>> \"filter\" mechanism or the gitattributes mechanism for that\n>> matter), …\n> Agreed. Instead of just the filter name I will replace everything\n> after the pathname with a single git-p4 config value:\n\nI actually was going to suggest not doing any such replacing.\n\nYou obviously need to cutomize the extensions list and specific\npaths that hold large contents, but having .attributeDescription()\nand .attributeFilter() that appear to be customizable sends a\nmessage to the reader that the code aims to support things other\nthan GitHub LFS.  I somehow doubt that really is the case (as you\nlater mention, this would not work at all for solutions that are not\nbased on gitattributes in the first place).  It would be less\nmisleading if it is made painfully obvious that this part of the\ncode is about one specific backend.\n\n> Well, you have a very good point here. From my point of view you can\n> use git-p4 in two ways:\n>\n> _Way 1_: Your project is stored in Perforce and it will stay in\n> Perforce. You use git-p4 on a regular basis to interact with the\n> Perforce repository.\n> _Way 2_: Your project is stored in Perforce and you want to migrate it\n> to Git. You use git-p4 once to perform the migration. Afterwards you\n> never touch git-p4 or Perforce again.\n>\n> My large file system feature is intended to be used only for _Way 2_\n> for exactly the reasons you mentioned. Would it still be OK to add\n> this feature to git-p4? Maybe with a appropriate warning in the docs?\n\nI could not tell, from the documentation that came in the patch,\nwhich one between the above two ways is supported, which made me\nassume that it aimed to support #1, as the desire to have a bidi\nbridge seems to be fairly common.\n\nIt is a valid use case to migrate away and not looking back.  But\nthat is the only workflow that is supported, that fact needs to be\ndocumented more clearly, I would think.\n\nOtherwise those who expected a bidirectional bridge by reading the\ndocumentation would be disappointed, possibly after wasting a lot of\ntime trying to figure out why their \"git p4 submit\" is not sending\nthe full large blob contents back to the depot.\n\nThanks.\n"}]}