{"thread":{"id":"57800","subject":"[PATCH] git-p4: properly initialize the largefiles","startedAt":"2022-04-26T18:59:40Z","lastAt":"2022-04-26T20:41:05Z","messageCount":2,"participants":["Bertin Philippe via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454459","messageId":"pull.1253.git.git.1650999569737.gitgitgadget@gmail.com","threadId":"57800","inReplyTo":null,"subject":"[PATCH] git-p4: properly initialize the largefiles","fromName":"Bertin Philippe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T18:59:29Z","receivedAt":"2022-04-26T18:59:40Z","isPatch":true,"sender":{"key":"name:Bertin Philippe","avatar":null},"body":"From: tumata <tumata.codes@gmail.com>\n\nThis commit takes care of initializing the largeFiles\nwith the already existing files referenced in the .gitattributes\ninstead of initializing with an empty set by default.\n\nThis makes it so that the 'git p4 sync' command works as\nexpected with git-lfs.\n\nSigned-off-by: tumata <tumata.codes@gmail.com>\n---\n    git-p4: Properly initialize the largeFiles set so that it includes the\n    already existing large files\n    \n    git-p4 sync isn't taking into account existing files that are managed by\n    git-lfs. This is fine when doing git p4 clone but doing git p4 sync\n    needs to take into account the files that are already managed by\n    git-lfs.\n    \n    This change reads-in the .gitattributes and re-generate the data so that\n    sync can iteratively add files to the git-lfs. It takes care of\n    initializing the largeFiles with the already existing files referenced\n    in the .gitattributes instead of initializing with an empty set by\n    default.\n    \n    We had issues cloning the first commit, then syncing the remaining\n    commits of a really large Perforce repo (making hundreds of smaller sync\n    instead of one massive sync). The sync was deleting the git-lfs each\n    time. We were able to fix this issue by first reading the current\n    content of git-lfs instead of initializing to an empty set.\n    \n    Signed-off-by: tumata 5684571+tumata@users.noreply.github.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1253%2Ftumata%2Fgit-p4-initialize-large-files-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1253/tumata/git-p4-initialize-large-files-v1\nPull-Request: https://github.com/git/git/pull/1253\n\n git-p4.py | 21 ++++++++++++++++++++-\n 1 file changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex a9b1f904410..576e923a1de 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1195,9 +1195,28 @@ class LargeFileSystem(object):\n     \"\"\"Base class for large file system support.\"\"\"\n \n     def __init__(self, writeToGitStream):\n-        self.largeFiles = set()\n+        self.largeFiles = self.parseLargeFiles()\n         self.writeToGitStream = writeToGitStream\n \n+    def parseLargeFiles(self):\n+        \"\"\"Parse large files in order to initially populate 'largeFiles'\"\"\"\n+        paths = set()\n+        try:\n+            cmd = ['git', 'show', 'p4/master:.gitattributes']\n+            p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)\n+            if p.returncode:\n+                print(\"Failed to read .gitattributed - error code: \" + p.returncode)\n+                raise\n+            out = p.stdout.readlines()\n+            for line in out:\n+                if line.startswith('/'):\n+                    path = line[1:].split(' ', 1)[0]\n+                    path = path.replace('[[:space:]]', ' ')\n+                    paths.add(path)\n+        except:\n+            print(\"parseLargeFiles: .gitattributes does not appear to exist.\")\n+        return paths\n+\n     def generatePointer(self, cloneDestination, contentFile):\n         \"\"\"Return the content of a pointer file that is stored in Git instead of\n            the actual content.\"\"\"\n\nbase-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e\n-- \ngitgitgadget\n"},{"id":"454466","messageId":"xmqqr15jpoh6.fsf@gitster.g","threadId":"57800","inReplyTo":"pull.1253.git.git.1650999569737.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: properly initialize the largefiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-26T20:40:53Z","receivedAt":"2022-04-26T20:41:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bertin Philippe via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: tumata <tumata.codes@gmail.com>\n>\n> This commit takes care of initializing the largeFiles\n> with the already existing files referenced in the .gitattributes\n> instead of initializing with an empty set by default.\n>\n> This makes it so that the 'git p4 sync' command works as\n> expected with git-lfs.\n\nWhy is this change needed?  How well did the old code work, and how\ndoes the code with this patch work, and what's the difference\nbetween them?\n\nTo answer such questions better, we prefer certain flow of logic in\nour proposed commit log message.  We start by talking about what the\ncurrent code does, e.g.\n\n    The LargeFileSystem class initializes the .largeFile attribute\n    to an empty set, ignoring the paths that are already known to be\n    X.  This causes \"git p4 sync\" subcommand to do Y.\n\n    But \"git p4 sync\" subcommand needs to do Z instead.  For that,\n    populate the .largeFile attribute upfront by readind W from V.\n\nI have little idea what problem this patch is trying to address, so\nthere are many placeholders in the above, but hopefully you get the\nidea.\n\n    ... the reviewer goes and looks ahead; sees a lot more plausible\n    explanation below ...\n\n> Signed-off-by: tumata <tumata.codes@gmail.com>\n\nName mismatch that should have said Bertin Philippe instead?\n\nAs Documentation/SubmittingPatches says, the Signed-off-by: line\nwants the patch author's real name, and the From: line wants to show\nthe same name-address that is on the Signed-off-by: line.\n\n> ---\n>     git-p4: Properly initialize the largeFiles set so that it includes the\n>     already existing large files\n>     \n>     git-p4 sync isn't taking into account existing files that are managed by\n>     git-lfs. This is fine when doing git p4 clone but doing git p4 sync\n>     needs to take into account the files that are already managed by\n>     git-lfs.\n>     \n>     This change reads-in the .gitattributes and re-generate the data so that\n>     sync can iteratively add files to the git-lfs. It takes care of\n>     initializing the largeFiles with the already existing files referenced\n>     in the .gitattributes instead of initializing with an empty set by\n>     default.\n>     \n>     We had issues cloning the first commit, then syncing the remaining\n>     commits of a really large Perforce repo (making hundreds of smaller sync\n>     instead of one massive sync). The sync was deleting the git-lfs each\n>     time. We were able to fix this issue by first reading the current\n>     content of git-lfs instead of initializing to an empty set.\n\nThat sounds more like what we want to record in the commit, but\nwriting them after the three-dash line does not help anybody.\n\n>  git-p4.py | 21 ++++++++++++++++++++-\n>  1 file changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index a9b1f904410..576e923a1de 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1195,9 +1195,28 @@ class LargeFileSystem(object):\n>      \"\"\"Base class for large file system support.\"\"\"\n>  \n>      def __init__(self, writeToGitStream):\n> -        self.largeFiles = set()\n> +        self.largeFiles = self.parseLargeFiles()\n>          self.writeToGitStream = writeToGitStream\n\nOK, we can see that it used to be an empty set, but now we are\ncomputing something.\n\n> +    def parseLargeFiles(self):\n> +        \"\"\"Parse large files in order to initially populate 'largeFiles'\"\"\"\n> +        paths = set()\n> +        try:\n> +            cmd = ['git', 'show', 'p4/master:.gitattributes']\n\nCurious.  How is 'p4/master' branch so special?  Is it always, no\nmatter what branch you are on, correct to read from that branch?\nShouldn't you be reading from the current branch instead?\n\n> +            p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)\n> +            if p.returncode:\n> +                print(\"Failed to read .gitattributed - error code: \" + p.returncode)\n\nted -> tes???\n\n> +                raise\n> +            out = p.stdout.readlines()\n> +            for line in out:\n\nSo, we read the contents of .gitattributes file line-by-line, and ...\n\n> +                if line.startswith('/'):\n> +                    path = line[1:].split(' ', 1)[0]\n> +                    path = path.replace('[[:space:]]', ' ')\n> +                    paths.add(path)\n\n... without checking what attribute each line is setting, we collect\nthe first token (the \"pattern\" to match paths, for which attributes\nare given to) and add it to the \"paths\" set.  The code does not care\nwhat attribute the line was trying to add, and it just collects the\npatterns that appear on each line.\n\nI am having a hard time to see how this could possibly be correct.\n\n    * The first token on .gitattributes file could be '/*'\n      (i.e. match everything at the top of this working tree); is\n      the set in the .largeFiles attribute prepared to take such a\n      pattern, instead of concrete paths?  Judging from the way the\n      set is used (e.g. isLargeFile() is fed a path and checks the\n      set membership, not pattern matching), I do not think so.\n\n    * The first token may happen to be the name of a single file but\n      it may be setting an attribute that has nothing to do with\n      LFS (e.g. \"/hello.txt    text\").  What good does it do to add\n      \"hello.txt\" to the set of largeFiles?\n\n    * The first token may happen to be the name of an existing file\n      that indeed is under LFS, but the path may not be in the\n      current branch at all, even though it appears in the p4/master\n      branch.  What good does it do to add such a non-existent path\n      to the set of largeFiles?\n\n> +        except:\n> +            print(\"parseLargeFiles: .gitattributes does not appear to exist.\")\n> +        return paths\n> +\n>      def generatePointer(self, cloneDestination, contentFile):\n>          \"\"\"Return the content of a pointer file that is stored in Git instead of\n>             the actual content.\"\"\"\n\nIn any case, if this is a patch to fix existing breakage, it should\ncome with a new test that demonstrates how the corrected command\n(\"git p4 sync\"?) behaves.\n\nThanks.\n"}]}