{"thread":{"id":"60228","subject":"[BUG] `git push` sends unnecessary objects","startedAt":"2023-09-13T22:59:50Z","lastAt":"2023-11-30T13:33:26Z","messageCount":4,"participants":["Javier Mora","Bagas Sanjaya"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"481819","messageId":"CAH1-q0iV+E73RrUDA8jcoFgNEfQDNwRnX5P5Z7r3Qj3GESV_7g@mail.gmail.com","threadId":"60228","inReplyTo":null,"subject":"[BUG] `git push` sends unnecessary objects","fromName":"Javier Mora","fromEmail":"cousteaulecommandant@gmail.com","sentAt":"2023-09-13T22:59:35Z","receivedAt":"2023-09-13T22:59:50Z","isPatch":false,"sender":{"key":"cousteaulecommandant@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6584870?v=4"},"body":"I came across this issue accidentally when trying to move a directory\ncontaining a very large file, and deleting another file in that\ndirectory while I was at it.\nIt seems to be caused by `pack.useSparse=true` being the default since\nv2.27 (which I found out after spending quite a while manually\nbisecting and compiling git since I noticed that this didn't happen in\nv2.25; commit de3a864 introduces this regression).\n\n* Expected:\n    Pushing a commit that moves a file without modifying it shouldn't\nrequire sending a blob object for that file, since the remote server\nalready has that blob object.\n* Observed:\n    Pushing a commit that moves a directory containing a file and also\nadds/deletes other files in that directory will for some reason also\nsend blobs for all the files in that directory, even the ones that\nwere already in the remote.\n* Consequences:\n    This has a very big impact in push times for very small commits\nthat just move around files, if those files are very big (I had this\nhappen with a >100MB file over a problematic connection... yikes!)\n* Note:\n    The commit introducing the regression does warn about possible\nscenarios involving a special arrangement of exact copies across\ndirectories, but these are not \"copies\", I just moved a file, which\nseems like a rather common operation.\n\nCode snippet for reproduction:\n```\nmkdir TEST_git\ncd TEST_git\n\nmkdir -p local remote/origin.git\ncd remote/origin.git\ngit init --bare\ncd ../../local\ngit init\ngit remote add origin file://\"${PWD%/*}\"/remote/origin.git\n\nmkdir zig\nfor i in a b c d e; do\n    dd if=/dev/urandom of=zig/\"$i\" bs=1M count=1\ndone\ngit add .\ngit commit -m 'Add big files'\ngit push -u origin master\n#>> Writing objects: 100% (8/8), 5.00 MiB | 13.27 MiB/s, done.\n#^ makes sense: 1 commit + 2 trees (/ and /zig) + 5 files = 8;\n#  5 MiB in total for the 5x 1 MiB binary files\n\ngit mv zig zag\ngit commit -m 'Move zig'\ngit push\n#>> Writing objects: 100% (2/2), 233 bytes | 233.00 KiB/s, done.\n#^ makes sense: 1 commit + 1 tree (/ renames /zig to /zag) = 2;\n#  a,b,c,d,e objects already in remote\n\ngit mv zag zog\ntouch zog/f\ngit add zog/f\ngit commit -m 'For great justice'\ngit push\n#>> Writing objects: 100% (9/9), 5.00 MiB | 24.63 MiB/s, done.\n#^ It re-uploaded the 5x 1 MiB blobs\n#  even though remote already had them.\n```\n\nNote that the latter doesn't happen if I use `git -c pack.useSparse=false push`.\n"},{"id":"481940","messageId":"ZQb9Thxa5X-Fo5mj@debian.me","threadId":"60228","inReplyTo":"CAH1-q0iV+E73RrUDA8jcoFgNEfQDNwRnX5P5Z7r3Qj3GESV_7g@mail.gmail.com","subject":"Re: [BUG] `git push` sends unnecessary objects","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2023-09-17T13:21:18Z","receivedAt":"2023-09-17T13:25:09Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On Wed, Sep 13, 2023 at 11:59:35PM +0100, Javier Mora wrote:\n> I came across this issue accidentally when trying to move a directory\n> containing a very large file, and deleting another file in that\n> directory while I was at it.\n> It seems to be caused by `pack.useSparse=true` being the default since\n> v2.27 (which I found out after spending quite a while manually\n> bisecting and compiling git since I noticed that this didn't happen in\n> v2.25; commit de3a864 introduces this regression).\n> \n> * Expected:\n>     Pushing a commit that moves a file without modifying it shouldn't\n> require sending a blob object for that file, since the remote server\n> already has that blob object.\n> * Observed:\n>     Pushing a commit that moves a directory containing a file and also\n> adds/deletes other files in that directory will for some reason also\n> send blobs for all the files in that directory, even the ones that\n> were already in the remote.\n> * Consequences:\n>     This has a very big impact in push times for very small commits\n> that just move around files, if those files are very big (I had this\n> happen with a >100MB file over a problematic connection... yikes!)\n> * Note:\n>     The commit introducing the regression does warn about possible\n> scenarios involving a special arrangement of exact copies across\n> directories, but these are not \"copies\", I just moved a file, which\n> seems like a rather common operation.\n> \n> Code snippet for reproduction:\n> ```\n> mkdir TEST_git\n> cd TEST_git\n> \n> mkdir -p local remote/origin.git\n> cd remote/origin.git\n> git init --bare\n> cd ../../local\n> git init\n> git remote add origin file://\"${PWD%/*}\"/remote/origin.git\n> \n> mkdir zig\n> for i in a b c d e; do\n>     dd if=/dev/urandom of=zig/\"$i\" bs=1M count=1\n> done\n> git add .\n> git commit -m 'Add big files'\n> git push -u origin master\n> #>> Writing objects: 100% (8/8), 5.00 MiB | 13.27 MiB/s, done.\n> #^ makes sense: 1 commit + 2 trees (/ and /zig) + 5 files = 8;\n> #  5 MiB in total for the 5x 1 MiB binary files\n> \n> git mv zig zag\n> git commit -m 'Move zig'\n> git push\n> #>> Writing objects: 100% (2/2), 233 bytes | 233.00 KiB/s, done.\n> #^ makes sense: 1 commit + 1 tree (/ renames /zig to /zag) = 2;\n> #  a,b,c,d,e objects already in remote\n> \n> git mv zag zog\n> touch zog/f\n> git add zog/f\n> git commit -m 'For great justice'\n> git push\n> #>> Writing objects: 100% (9/9), 5.00 MiB | 24.63 MiB/s, done.\n> #^ It re-uploaded the 5x 1 MiB blobs\n> #  even though remote already had them.\n> ```\n> \n> Note that the latter doesn't happen if I use `git -c pack.useSparse=false push`.\n\nI can reproduce this regression on v2.42.0 (self-compiled) on my Debian\ntesting system.\n\nCc'ing Derrick and Junio.\n\nThanks for the report!\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"485147","messageId":"CAH1-q0jGQBPZVYja3Sg2Xv4YGAxcUsnb1rL4MALKxCoywi0B=A@mail.gmail.com","threadId":"60228","inReplyTo":"ZQb9Thxa5X-Fo5mj@debian.me","subject":"Re: [BUG] `git push` sends unnecessary objects","fromName":"Javier Mora","fromEmail":"cousteaulecommandant@gmail.com","sentAt":"2023-11-25T14:54:09Z","receivedAt":"2023-11-25T14:54:22Z","isPatch":false,"sender":{"key":"cousteaulecommandant@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6584870?v=4"},"body":"Apparently if I do that in two commits (one to move the dir, and a\nsecond one add the file), and then push all that after the second\ncommit, this doesn't happen -- the resulting push will only contain 6\nobjects (2 commits, 3 trees, and 1 file), and be a few bytes large.\n\nEl dom, 17 sept 2023 a las 14:21, Bagas Sanjaya\n(<bagasdotme@gmail.com>) escribió:\n>\n> On Wed, Sep 13, 2023 at 11:59:35PM +0100, Javier Mora wrote:\n> > I came across this issue accidentally when trying to move a directory\n> > containing a very large file, and deleting another file in that\n> > directory while I was at it.\n> > It seems to be caused by `pack.useSparse=true` being the default since\n> > v2.27 (which I found out after spending quite a while manually\n> > bisecting and compiling git since I noticed that this didn't happen in\n> > v2.25; commit de3a864 introduces this regression).\n> >\n> > * Expected:\n> >     Pushing a commit that moves a file without modifying it shouldn't\n> > require sending a blob object for that file, since the remote server\n> > already has that blob object.\n> > * Observed:\n> >     Pushing a commit that moves a directory containing a file and also\n> > adds/deletes other files in that directory will for some reason also\n> > send blobs for all the files in that directory, even the ones that\n> > were already in the remote.\n> > * Consequences:\n> >     This has a very big impact in push times for very small commits\n> > that just move around files, if those files are very big (I had this\n> > happen with a >100MB file over a problematic connection... yikes!)\n> > * Note:\n> >     The commit introducing the regression does warn about possible\n> > scenarios involving a special arrangement of exact copies across\n> > directories, but these are not \"copies\", I just moved a file, which\n> > seems like a rather common operation.\n> >\n> > Code snippet for reproduction:\n> > ```\n> > mkdir TEST_git\n> > cd TEST_git\n> >\n> > mkdir -p local remote/origin.git\n> > cd remote/origin.git\n> > git init --bare\n> > cd ../../local\n> > git init\n> > git remote add origin file://\"${PWD%/*}\"/remote/origin.git\n> >\n> > mkdir zig\n> > for i in a b c d e; do\n> >     dd if=/dev/urandom of=zig/\"$i\" bs=1M count=1\n> > done\n> > git add .\n> > git commit -m 'Add big files'\n> > git push -u origin master\n> > #>> Writing objects: 100% (8/8), 5.00 MiB | 13.27 MiB/s, done.\n> > #^ makes sense: 1 commit + 2 trees (/ and /zig) + 5 files = 8;\n> > #  5 MiB in total for the 5x 1 MiB binary files\n> >\n> > git mv zig zag\n> > git commit -m 'Move zig'\n> > git push\n> > #>> Writing objects: 100% (2/2), 233 bytes | 233.00 KiB/s, done.\n> > #^ makes sense: 1 commit + 1 tree (/ renames /zig to /zag) = 2;\n> > #  a,b,c,d,e objects already in remote\n> >\n> > git mv zag zog\n> > touch zog/f\n> > git add zog/f\n> > git commit -m 'For great justice'\n> > git push\n> > #>> Writing objects: 100% (9/9), 5.00 MiB | 24.63 MiB/s, done.\n> > #^ It re-uploaded the 5x 1 MiB blobs\n> > #  even though remote already had them.\n> > ```\n> >\n> > Note that the latter doesn't happen if I use `git -c pack.useSparse=false push`.\n>\n> I can reproduce this regression on v2.42.0 (self-compiled) on my Debian\n> testing system.\n>\n> Cc'ing Derrick and Junio.\n>\n> Thanks for the report!\n>\n> --\n> An old man doll... just what I always wanted! - Clara\n"},{"id":"485268","messageId":"CAH1-q0jbN19gMF7_uwjYKVpH70q_5Qbt8TXvMZnB+fB_6bCvwg@mail.gmail.com","threadId":"60228","inReplyTo":"PH0PR00MB1349BB447657A94A8EA90A78A183A@PH0PR00MB1349.namprd00.prod.outlook.com","subject":"Re: [EXTERNAL] Re: [BUG] `git push` sends unnecessary objects","fromName":"Javier Mora","fromEmail":"cousteaulecommandant@gmail.com","sentAt":"2023-11-30T13:33:13Z","receivedAt":"2023-11-30T13:33:26Z","isPatch":false,"sender":{"key":"cousteaulecommandant@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6584870?v=4"},"body":"> I just want to push back on the word \"regression\" here as it is an expected result.\n\nYeah, I wouldn't quite call it a \"regression\" either; just an\ninconvenient side effect of a new feature.\n\n>> However, it is possible that extra objects are added to the pack-file if the included commits contain certain types of direct renames.\n>\n> [1] https://git-scm.com/docs/git-config#Documentation/git-config.txt-packuseSparse\n>\n> It is unfortunate that this user hit this problem, but it is easy to work around it\n\nI eventually found that note, but not until I figured out that that\noption was the one causing the trouble.  So yes, it is easy to work\naround this issue when you know about it.  Maybe this potential corner\ncase should be given more visibility in the documentation so that it\nis easier to spot?  (Not just on the documentation for the option\nitself, but maybe in the `git push` one.)\n\n> and the benefits of the sparse algorithm should outweigh these kinds of infrequent issues (in my opinion).\n\nWell, I don't use git THAT often and still got hit by this issue, so\nit might not be that uncommon.  And when the files are large, the time\nspent sending them through an internet connection will probably\noutweigh the processing time saved by skipping some files in the local\nprocessing.\n\nI wonder if it would make sense to try to make the algorithm smarter\nto avoid this corner case.  For example, ask the server if it already\nhas the objects before sending them, or modify the algorithm to look\ninto trees that have added OR deleted files (to detect that a file has\nactually been moved, and thus the server must already have it), or\nsimply be extra careful when large files are involved.  But I don't\nknow the details of the algorithm so I'm not sure all those\nsuggestions are feasible or even make sense at all.\n"}]}