{"thread":{"id":"41275","subject":"Bugs in git filter-branch (git replace related)","startedAt":"2016-01-28T14:46:40Z","lastAt":"2016-02-22T21:13:28Z","messageCount":6,"participants":["Anatoly Borodin","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"276996","messageId":"loom.20160128T153147-396@post.gmane.org","threadId":"41275","inReplyTo":null,"subject":"Bugs in git filter-branch (git replace related)","fromName":"Anatoly Borodin","fromEmail":"anatoly.borodin@gmail.com","sentAt":"2016-01-28T14:46:40Z","receivedAt":"2016-01-28T14:46:40Z","isPatch":false,"sender":{"key":"anatoly.borodin@gmail.com","avatar":"https://gravatar.com/avatar/59f11f1f45cf5ebb7bcca8da94b43cfe5f596d91c0a3e27ad0d2a861cc2bbce7?d=mp&s=160"},"body":"Hi All!\n\nThere are two bugs in `git filter-branch`, present in the most recent\nversions (d10e2cb9d0299a26f43d57dd5bdcf2b3f86a30b3), as well as in the old\nones (I couldn't find a version where it works properly).\n\nThe script:\n\n#!/bin/sh\n\nset -e\n\nGIT_EXEC_PATH=/tmp/git\nexport GIT_EXEC_PATH\nGIT=$GIT_EXEC_PATH/git\n\nrm -rf a\nmkdir a\ncd a\n$GIT init\necho aaa > a.txt && $GIT add a.txt && $GIT commit -a -m a\necho bbb > a.txt && $GIT add a.txt && $GIT commit -a -m b\necho ccc > a.txt && $GIT add a.txt && $GIT commit -a -m c\n$GIT replace f761ec192d9f0dca3329044b96ebdb12839dbff6\n72943a16fb2c8f38f9dde202b7a70ccc19c52f34\necho && echo One && $GIT filter-branch --prune-empty -- master\necho && echo Two && $GIT filter-branch --prune-empty -- --all\n\nThe output is:\n\n...\nOne\nRewrite 98af0305b778bf56e25a0d4f85acdf82f435f9b3 (3/3) (0 seconds passed,\nremaining 0 predicted)    \nWARNING: Ref 'refs/heads/master' is unchanged\n\nTwo\nRewrite 98af0305b778bf56e25a0d4f85acdf82f435f9b3 (3/3) (0 seconds passed,\nremaining 0 predicted)    \nWARNING: Ref 'refs/heads/master' is unchanged\nerror: object 72943a16fb2c8f38f9dde202b7a70ccc19c52f34 is a blob, not a commit\nerror: object 72943a16fb2c8f38f9dde202b7a70ccc19c52f34 is a blob, not a commit\nfatal: ambiguous argument\n'refs/replace/f761ec192d9f0dca3329044b96ebdb12839dbff6^0': unknown revision\nor path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\nWARNING: Ref 'refs/replace/f761ec192d9f0dca3329044b96ebdb12839dbff6' is\nunchanged\n\n\nThe `git replace` makes the second commit empty (use the file content from\nthe first commit). It should disappear after `git filter-branch`, but it\ndoesn't happen.\n\nBug 1: the empty commit stays.\nBug 2: the replace refs are not ignored (they can epresent blobs, trees etc,\nbut even if they represent commits - should they be rewritten?).\n\nAny ideas?\n\nPS I've found http://article.gmane.org/gmane.comp.version-control.git/220931\n, seems like the bug 1. But it's old!\n"},{"id":"277038","messageId":"20160129061820.GB23106@sigill.intra.peff.net","threadId":"41275","inReplyTo":"loom.20160128T153147-396@post.gmane.org","subject":"Re: Bugs in git filter-branch (git replace related)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-29T06:18:20Z","receivedAt":"2016-01-29T06:18:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2016 at 02:46:40PM +0000, Anatoly Borodin wrote:\n\n> The `git replace` makes the second commit empty (use the file content from\n> the first commit). It should disappear after `git filter-branch`, but it\n> doesn't happen.\n> \n> Bug 1: the empty commit stays.\n\nI'm not sure if this is a bug or not. The \"empty commit\" check works by\nchecking the tree sha1s, without doing a full diff respecting replace\nrefs.\n\nYou're expecting git to notice a tree change, even though it never even\nexamined the tree in the first place (because you didn't give it a tree\nor index filter).\n\nTry:\n\n  git filter-branch --prune-empty --tree-filter true master\n\nwhich will force git to go through the motions of checking out the\nreplaced content and re-examining it.\n\nThis will run much more slowly, as it actually touches the filesystem.\nIn the general case, it would be interesting if filter-branch (or a\nsimilar tool) could \"cement\" replacement objects into place as\nefficiently as possible. But I'm not sure whether that should be the\ndefault mode for filter-branch.\n\n> Bug 2: the replace refs are not ignored (they can epresent blobs, trees etc,\n> but even if they represent commits - should they be rewritten?).\n\nYou told it \"--all\", which is passed to rev-list, where it means \"all\nrefs\". I agree that running filter-branch on refs/replace is probably\nnot going to yield useful results, but I'm not sure if it is\nfilter-branch's responsibility to second-guess the rev-list options.\n\nProbably the documentation for filter-branch should recommend\n\"--branches --tags\" instead of \"--all\", though.\n\n-Peff\n"},{"id":"277054","messageId":"n8gao5$3c6$1@ger.gmane.org","threadId":"41275","inReplyTo":"20160129061820.GB23106@sigill.intra.peff.net","subject":"Re: Bugs in git filter-branch (git replace related)","fromName":"Anatoly Borodin","fromEmail":"anatoly.borodin@gmail.com","sentAt":"2016-01-29T18:24:07Z","receivedAt":"2016-01-29T18:24:07Z","isPatch":false,"sender":{"key":"anatoly.borodin@gmail.com","avatar":"https://gravatar.com/avatar/59f11f1f45cf5ebb7bcca8da94b43cfe5f596d91c0a3e27ad0d2a861cc2bbce7?d=mp&s=160"},"body":"Hi Jeff,\n\n\nI've created a gist with the script\nhttps://gist.github.com/anatolyborodin/6505a364a68584f13846\n\nI've added some changes and a second test (will be discussed in the\ncomments).\n\n\nJeff King <peff@peff.net> wrote:\n> I'm not sure if this is a bug or not. The \"empty commit\" check works by\n> checking the tree sha1s, without doing a full diff respecting replace\n> refs.\n> \n> You're expecting git to notice a tree change, even though it never even\n> examined the tree in the first place (because you didn't give it a tree\n> or index filter).\n\nWhen git-filter-branch(1) says \"If you have any grafts or replacement\nrefs defined, running this command will make them permanent.\", and it\ndoesn't work like that because of some optimization, it *is* a bug.\n\n> Try:\n> \n>   git filter-branch --prune-empty --tree-filter true master\n> \n> which will force git to go through the motions of checking out the\n> replaced content and re-examining it.\n\nThank you, I've added this command to the script, and it works! I think\nit should be added to git-filter-branch(1), if there is no other way to\nrewrite the optimization.\n\n>> Bug 2: the replace refs are not ignored (they can epresent blobs, trees etc,\n>> but even if they represent commits - should they be rewritten?).\n> \n> You told it \"--all\", which is passed to rev-list, where it means \"all\n> refs\". I agree that running filter-branch on refs/replace is probably\n> not going to yield useful results, but I'm not sure if it is\n> filter-branch's responsibility to second-guess the rev-list options.\n\nLook how `git log --all` works (see the second test in the script): it\nignores (without any messages) the blobs, and writes only the commits.\nFor example, the same commit log message is printed twice in the second\ntestcase.\n\nMaybe it makes sence, I don't know. I would suggest that all\nrefs/replace/* heads should be ignored by `git log`. `git rev-list\n--no-replace` maybe?\n\n> Probably the documentation for filter-branch should recommend\n> \"--branches --tags\" instead of \"--all\", though.\n\nOr redefine `--all` as \"all refs excepting refs/replace/*\". Who would\nreally want to run `--all` the way it works now?\n\nThe blobs replaces should be ignored, as in `git log --all`. Is there\nany reason to rewrite refs/rebase/hash if it's a replace commit?\n\n\n-- \nMit freundlichen Grüßen,\nAnatoly Borodin\n"},{"id":"277068","messageId":"20160129231127.GA31798@sigill.intra.peff.net","threadId":"41275","inReplyTo":"n8gao5$3c6$1@ger.gmane.org","subject":"Re: Bugs in git filter-branch (git replace related)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-29T23:11:27Z","receivedAt":"2016-01-29T23:11:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 29, 2016 at 06:24:07PM +0000, Anatoly Borodin wrote:\n\n> > You're expecting git to notice a tree change, even though it never even\n> > examined the tree in the first place (because you didn't give it a tree\n> > or index filter).\n> \n> When git-filter-branch(1) says \"If you have any grafts or replacement\n> refs defined, running this command will make them permanent.\", and it\n> doesn't work like that because of some optimization, it *is* a bug.\n\nI think the bug is in the documentation. That has always worked for\ncommit grafts and replacements, but never for blob and tree replacements\n(and in fact, filter-branch quite predates refs/replace).\n\nI don't think this is just an optimization; filter-branch does not touch\nor rewrite bits that you did not ask it to touch, and that is a\nuser-visible behavior.\n\n> Thank you, I've added this command to the script, and it works! I think\n> it should be added to git-filter-branch(1), if there is no other way to\n> rewrite the optimization.\n\nYeah, I agree.  Documentation patches are welcome.\n\nI think with an \"--index-filter\", you could cement a replacement tree\ninto place, but you need a \"--tree-filter\" to do a blob replacement\n(because otherwise we insert only the name of the blob sha1 into the\nindex).\n\n> > You told it \"--all\", which is passed to rev-list, where it means \"all\n> > refs\". I agree that running filter-branch on refs/replace is probably\n> > not going to yield useful results, but I'm not sure if it is\n> > filter-branch's responsibility to second-guess the rev-list options.\n> \n> Look how `git log --all` works (see the second test in the script): it\n> ignores (without any messages) the blobs, and writes only the commits.\n> For example, the same commit log message is printed twice in the second\n> testcase.\n\nI'm not sure what you mean here. \"git log --all\" is not looking at blobs\nat all (you did not ask it do any diffs, nor simplify history based on\nTREESAME commits). The \"--all\" here means \"start traversing from commits\nfound at all ref tips\". It _does_ look at refs/replace, but there are no\ncommits to traverse there.\n\nLikewise, \"git rev-list --all\" would not show any. But \"git rev-list\n--objects --all\" would (both the refs/replace tips, as well as objects\nreachable from the other commits).\n\n> Maybe it makes sence, I don't know. I would suggest that all\n> refs/replace/* heads should be ignored by `git log`. `git rev-list\n> --no-replace` maybe?\n\nIt is too late for that without an extra option. \"rev-list --all\"\nalready has a well-established meaning, and changing it would break\nother uses (e.g., commit reachability done during object transfer and\nrepacking, not to mention any third-party scripts). But...\n\n> > Probably the documentation for filter-branch should recommend\n> > \"--branches --tags\" instead of \"--all\", though.\n> \n> Or redefine `--all` as \"all refs excepting refs/replace/*\". Who would\n> really want to run `--all` the way it works now?\n\nIf you mean \"rev-list --all\", then: lots of things that aren't\nfilter-branch. :)\n\nIf we were to change anything, it would be to intercept \"--all\" in\nfilter-branch and rewrite it to \"--exclude=refs/replace/* --all\". I'm\nslightly negative on that, just because we advertise filter-branch as\ntaking arbitrary rev-list args, and this is violating that. I think I'd\nbe more in favor if it were done robustly and clearly, like:\n\n  - teach rev-list --all-does-not-include-refs-replace (but with a less\n    horrible name)\n\n  - advertise that filter-branch always passes that option to rev-list\n\nThen at least it is robust (we are not mucking with rev-list options,\nwhich we cannot do completely accurately without having the full\nknowledge of all of its options), and simple to explain to the user (if\nthey want to work around it, they simple add \"--glob=refs/replace/*\" to\ntheir invocation).\n\n-Peff\n"},{"id":"277791","messageId":"n9b9tp$gbr$1@ger.gmane.org","threadId":"41275","inReplyTo":"20160129231127.GA31798@sigill.intra.peff.net","subject":"A different bug in git-filter-branch (v2.7.0)","fromName":"Anatoly Borodin","fromEmail":"anatoly.borodin@gmail.com","sentAt":"2016-02-08T23:55:37Z","receivedAt":"2016-02-08T23:55:37Z","isPatch":false,"sender":{"key":"anatoly.borodin@gmail.com","avatar":"https://gravatar.com/avatar/59f11f1f45cf5ebb7bcca8da94b43cfe5f596d91c0a3e27ad0d2a861cc2bbce7?d=mp&s=160"},"body":"Hi Jeff,\n\n\nunfortunately, `--tree-filter true` doesn't solve the problem with the\nrepo at my work: not all old blobs are replaced with the new ones. I've\nmade a test repository to demonstrate it; it's a huge one (115M), but I\ncouldn't make it much smaller, because the bug fails to reproduce if the\nrepo is not big enough:\n\nhttps://github.com/anatolyborodin/git-filter-branch-bug\n\nThere are some description and instructions in `README.md`. I hope you\nwill be able to reproduce it on your machine, if not - just add more\nfiles :)\n\nI've debugged the test case and found the place where `git diff-index`\nbehaves differently regarding `aa/bb.dat`:\n\nread-cache.c +351\tie_match_stat():\n...\n\tif (!changed && is_racy_timestamp(istate, ce)) {\n\t\tif (assume_racy_is_modified)\n\t\t\tchanged |= DATA_CHANGED;\n\t\telse\n\t\t\tchanged |= ce_modified_check_fs(ce, st);\n\t}\n...\n\nAfter `git-checkout-index` the blob hash for `aa/bb.dat` in the index is\n88a0f09b9b2e4ccf2faec89ab37d416fba4ee79d (the huge binary), but the file\non disk is a text file \"This file was to big, and it has been removed.\"\nwith the hash 16e0939430610600301680d5bf8a24a22ff8b6c4.\n\nIn the case of a \"good behaving\" commit, the timestamps of the index and\nthe cache entry are the same, is_racy_timestamp() returns 1, and\nce_modified_check_fs() finds that the content of the file has changed.\n`git diff-index` lists the file `aa/bb.dat`.\n\nIn the case of a \"bad behaving\" commit, the timestamps of the index and\nthe cache entry are different (the index is 1 second newer),\nis_racy_timestamp() returns 0, and the file is assumed unchanged; `git\ndiff-index` prints nothing.\n\nI don't know if it should be considered to be a bug in in the logic of\n`git checkout-index`, or `git diff-index` / `git update-index`.\n\n\n-- \nMit freundlichen Grüßen,\nAnatoly Borodin\n"},{"id":"278903","messageId":"20160222211328.GA16512@sigill.intra.peff.net","threadId":"41275","inReplyTo":"n9b9tp$gbr$1@ger.gmane.org","subject":"Re: A different bug in git-filter-branch (v2.7.0)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-22T21:13:28Z","receivedAt":"2016-02-22T21:13:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 11:55:37PM +0000, Anatoly Borodin wrote:\n\n> unfortunately, `--tree-filter true` doesn't solve the problem with the\n> repo at my work: not all old blobs are replaced with the new ones. I've\n> made a test repository to demonstrate it; it's a huge one (115M), but I\n> couldn't make it much smaller, because the bug fails to reproduce if the\n> repo is not big enough:\n> \n> https://github.com/anatolyborodin/git-filter-branch-bug\n> \n> There are some description and instructions in `README.md`. I hope you\n> will be able to reproduce it on your machine, if not - just add more\n> files :)\n> \n> I've debugged the test case and found the place where `git diff-index`\n> behaves differently regarding `aa/bb.dat`:\n> \n> read-cache.c +351\tie_match_stat():\n> ...\n> \tif (!changed && is_racy_timestamp(istate, ce)) {\n> \t\tif (assume_racy_is_modified)\n> \t\t\tchanged |= DATA_CHANGED;\n> \t\telse\n> \t\t\tchanged |= ce_modified_check_fs(ce, st);\n> \t}\n> ...\n> \n> After `git-checkout-index` the blob hash for `aa/bb.dat` in the index is\n> 88a0f09b9b2e4ccf2faec89ab37d416fba4ee79d (the huge binary), but the file\n> on disk is a text file \"This file was to big, and it has been removed.\"\n> with the hash 16e0939430610600301680d5bf8a24a22ff8b6c4.\n\nRight, that makes sense. The index doesn't know anything about the\nreplace mechanism. So it assumes that what it wrote matches what is in\nthe stat cache (i.e., some sha1 and the matching stat info). Later, when\ngit wants to know \"should I bother reading this file back in and\ncomputing its sha1\", the stat cache says no.\n\nAnd then as you noticed, it sometimes \"works\" because if the file and\nindex timestamps are the same, we err on the side of re-reading. So more\nfiles means more likelihood of crossing the one-second boundary.\n\n> I don't know if it should be considered to be a bug in in the logic of\n> `git checkout-index`, or `git diff-index` / `git update-index`.\n\nI'd say if any, it is the fault of checkout-index for writing out\ncontent that does not match the sha1 in the index, but writing out the\nstat information as if it is clean. For your case, you'd obviously\nprefer that the file be marked dirty and re-read later.\n\nBut I'm not sure whether other users of replace refs would want the same\nbehavior. They may want to silently pretend as if the data is\nunmodified. I'm not sure if anyone is doing that in practice, though; as\nyou noted, the results are not deterministic, so it probably ends up\njust being a huge pain. So perhaps it would make sense to make the\ncheckout operation aware of replaced blobs.\n\nAs a workaround for your filter-branch, I suspect you could do\n`--tree-filter 'git ls-files | xargs touch'` or something, but that is\ngoing to be rather inefficient.\n\nBy now you've probably realized that replaced blobs are not widely used,\nand there are a lot of corner cases around checking them out. So let's\ntake a step back for a moment. What are you trying to accomplish with\nyour filter-branch? If you just want to replace blob A with blob B, I\nthink it might be easier to skip refs/replace entirely and just do so\nexplicitly in an index-filter, like:\n\n  --index-filter '\n    git ls-files -s |\n    sed \"s/$old_sha1/$new_sha1/\" |\n    git update-index --index-info\n  '\n\n-Peff\n"}]}