{"thread":{"id":"48882","subject":"[PATCH 0/2] repack -ad: fix after `fetch --prune` in a shallow repository","startedAt":"2018-07-13T20:18:17Z","lastAt":"2018-10-29T20:45:17Z","messageCount":47,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Johannes Schindelin","Junio C Hamano","Duy Nguyen","Eric Sunshine","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"352531","messageId":"pull.9.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":null,"subject":"[PATCH 0/2] repack -ad: fix after `fetch --prune` in a shallow repository","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-13T20:18:12Z","receivedAt":"2018-07-13T20:18:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Under certain circumstances, commits that were reachable can be made unreachable, e.g. via `git fetch --prune`. These commits might have been packed already, in which case `git repack -adlf` will just drop them without giving them the usual grace period before an eventual `git prune` (via `git gc`) prunes them.\n\nThis is a problem when the `shallow` file still lists them, which is the reason why `git prune` edits that file. And with the proposed changes, `git repack -ad` will now do the same.\n\nReported by Alejandro Pauly.\n\nJohannes Schindelin (2):\n repack: point out a bug handling stale shallow info\n repack -ad: prune the list of shallow commits\n\n builtin/repack.c | 4 ++++\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\n\nbase-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/9\n-- \ngitgitgadget\n"},{"id":"352532","messageId":"d2be4013134c0e9dc032f934c6af82721fec4eba.1531513093.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] repack: point out a bug handling stale shallow info","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-11T22:17:33Z","receivedAt":"2018-07-13T20:18:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nA `git fetch --prune` can turn previously-reachable objects unreachable,\neven commits that are in the `shallow` list. A subsequent `git repack\n-ad` will then unceremoniously drop those unreachable commits, and the\n`shallow` list will become stale. This means that when we try to fetch\nwith a larger `--depth` the next time, we may end up with:\n\n\tfatal: error in object: unshallow \n\nReported by Alejandro Pauly.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 943231af9..561485d31 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,4 +186,31 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '.git/shallow is edited by repack' '\n+\tgit init shallow-server &&\n+\ttest_commit -C shallow-server A &&\n+\ttest_commit -C shallow-server B &&\n+\tgit -C shallow-server checkout -b branch &&\n+\ttest_commit -C shallow-server C &&\n+\ttest_commit -C shallow-server E &&\n+\ttest_commit -C shallow-server D &&\n+\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n+\tgit -C shallow-server checkout master &&\n+\n+\tgit clone --depth=1 --no-tags --no-single-branch \\\n+\t\t\"file://$PWD/shallow-server\" shallow-client &&\n+\n+\t: now remove the branch and fetch with prune &&\n+\tgit -C shallow-server branch -D branch &&\n+\tgit -C shallow-client fetch --prune --depth=1 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\" &&\n+\tgit -C shallow-client repack -adfl &&\n+\ttest_must_fail git -C shallow-client rev-parse --verify $d^0 &&\n+\t! grep $d shallow-client/.git/shallow &&\n+\n+\tgit -C shallow-server branch branch-orig D^0 &&\n+\tgit -C shallow-client fetch --prune --depth=2 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"352533","messageId":"b4e01a963fd16f50d12c1f67c6e64bec8b1e9673.1531513093.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-11T22:23:28Z","receivedAt":"2018-07-13T20:18:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhile it is true that we never add unreachable commits into pack files\nintentionally (as `git repack`'s documentation states), we must not\nforget that a `git fetch --prune` (or even a `git fetch` when a ref was\nforce-pushed in the meantime) can make a commit unreachable that was\nreachable before.\n\nTherefore it is not safe to assume that a `git repack -adlf` will keep\nunreachable commits alone (under the assumption that they had not been\npacked in the first place).\n\nThis is particularly important to keep in mind when looking at the\n`.git/shallow` file: if any commits listed in that file become\nunreachable, it is not a problem, but if they go missing, it *is* a\nproblem. One symptom of this problem is that a deepening fetch may now\nfail with\n\n\tfatal: error in object: unshallow \n\nTo avoid this problem, let's prune the shallow list in `git repack` when\nthe `-d` option is passed, unless `-A` is passed, too (which would force\nthe now-unreachable objects to be turned into loose objects instead of\nbeing deleted).\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/repack.c | 4 ++++\n t/t5537-fetch-shallow.sh | 2 +-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6c636e159..45f321b23 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -444,6 +444,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (!quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n+\n+\t\tif (!(pack_everything & LOOSEN_UNREACHABLE) &&\n+\t\t is_repository_shallow())\n+\t\t\tprune_shallow(0);\n \t}\n \n \tif (!no_update_server_info)\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 561485d31..d32ba20f9 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,7 +186,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '.git/shallow is edited by repack' '\n+test_expect_success '.git/shallow is edited by repack' '\n \tgit init shallow-server &&\n \ttest_commit -C shallow-server A &&\n \ttest_commit -C shallow-server B &&\n-- \ngitgitgadget\n"},{"id":"352536","messageId":"20180713203140.GB17670@sigill.intra.peff.net","threadId":"48882","inReplyTo":"b4e01a963fd16f50d12c1f67c6e64bec8b1e9673.1531513093.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T20:31:40Z","receivedAt":"2018-07-13T20:31:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 12, 2018 at 12:23:28AM +0200, Johannes Schindelin via GitGitGadget wrote:\n\n> This is particularly important to keep in mind when looking at the\n> `.git/shallow` file: if any commits listed in that file become\n> unreachable, it is not a problem, but if they go missing, it *is* a\n> problem. One symptom of this problem is that a deepening fetch may now\n> fail with\n> \n> \tfatal: error in object: unshallow \n> \n> To avoid this problem, let's prune the shallow list in `git repack` when\n> the `-d` option is passed, unless `-A` is passed, too (which would force\n> the now-unreachable objects to be turned into loose objects instead of\n> being deleted).\n\nI'm not sure if this covers all cases:\n\n - even with \"-A\", we may still drop objects subject to\n --unpack-unreachable. So if your pack has an old mtime (e.g., because\n you haven't packed in a while) I think you'd see the same problem.\n\n - if you use \"-adk\", we'd keep all objects, and this pruning would not\n be necessary\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 6c636e159..45f321b23 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -444,6 +444,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\tif (!quiet && isatty(2))\n> \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> \t\tprune_packed_objects(opts);\n> +\n> +\t\tif (!(pack_everything & LOOSEN_UNREACHABLE) &&\n> +\t\t is_repository_shallow())\n> +\t\t\tprune_shallow(0);\n> \t}\n\nI understand how this solves your immediate problem, but it feels like a\nweird layering violation (which I think is a result of existing layering\nviolations ;) ).\n\nI.e., it seems unexpected that \"git repack\" is going to tweak your\nshallow lists. If we were designing from scratch, the sane behavior\nseems to me to be:\n\n 1. Shallow pruning should be its own separate command (distinct from\n either repacking or loose object pruning), and should be triggered\n as part of a normal git-gc.\n\n AND ONE OF:\n\n 2a. Objects mentions in the shallow file are important, and therefore\n _are_ considered reachable on their own. Neither repack nor prune\n needs to know or care.\n\n OR\n\n 2b. It's OK for shallow objects to be missing, and the shallow code\n should be more resilient to missing objects. Neither repack nor\n prune needs to know or care.\n\nI don't know how hard it would be to get there from the current code\nstate, though.\n\n-Peff\n"},{"id":"352573","messageId":"nycvar.QRO.7.76.6.1807142351100.75@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"20180713203140.GB17670@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-14T21:56:57Z","receivedAt":"2018-07-14T21:58:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 13 Jul 2018, Jeff King wrote:\n\n> On Thu, Jul 12, 2018 at 12:23:28AM +0200, Johannes Schindelin via\n> GitGitGadget wrote:\n> \n> > This is particularly important to keep in mind when looking at the\n> > `.git/shallow` file: if any commits listed in that file become\n> > unreachable, it is not a problem, but if they go missing, it *is* a\n> > problem. One symptom of this problem is that a deepening fetch may now\n> > fail with\n> > \n> > \tfatal: error in object: unshallow \n> > \n> > To avoid this problem, let's prune the shallow list in `git repack` when\n> > the `-d` option is passed, unless `-A` is passed, too (which would force\n> > the now-unreachable objects to be turned into loose objects instead of\n> > being deleted).\n> \n> I'm not sure if this covers all cases:\n> \n> - even with \"-A\", we may still drop objects subject to\n> --unpack-unreachable. So if your pack has an old mtime (e.g., because\n> you haven't packed in a while) I think you'd see the same problem.\n> \n> - if you use \"-adk\", we'd keep all objects, and this pruning would not\n> be necessary\n\nSure. I can add those cases.\n\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index 6c636e159..45f321b23 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -444,6 +444,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \t\tif (!quiet && isatty(2))\n> > \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> > \t\tprune_packed_objects(opts);\n> > +\n> > +\t\tif (!(pack_everything & LOOSEN_UNREACHABLE) &&\n> > +\t\t is_repository_shallow())\n> > +\t\t\tprune_shallow(0);\n> > \t}\n> \n> I understand how this solves your immediate problem, but it feels like a\n> weird layering violation (which I think is a result of existing layering\n> violations ;) ).\n\nOkay, but please don't punish me for those existing layering violations by\nforcing me to address them, instead of a quick bug fix for a very real bug\nthat was reported to me privately, and that I would like to see fixed\nrelatively quickly.\n\n> I.e., it seems unexpected that \"git repack\" is going to tweak your\n> shallow lists. If we were designing from scratch, the sane behavior\n> seems to me to be:\n> \n> 1. Shallow pruning should be its own separate command (distinct from\n> either repacking or loose object pruning), and should be triggered\n> as part of a normal git-gc.\n\nI fail to see the value in a separate command.\n\nAnd: `git gc` already calls `git prune`, which *does* prune the shallow\nlist.\n\n> AND ONE OF:\n> \n> 2a. Objects mentions in the shallow file are important, and therefore\n> _are_ considered reachable on their own. Neither repack nor prune\n> needs to know or care.\n\nIf we were to do that, we would never be able to gc any stale shallow\ncommits.\n\nThat would be rather bad a design, don't you agree?\n\n> OR\n> \n> 2b. It's OK for shallow objects to be missing, and the shallow code\n> should be more resilient to missing objects. Neither repack nor\n> prune needs to know or care.\n\nThat would be possible. Kind of like saying: we do have this list of\nshallow commits, but oh, BTW, it is likely incorrect, so we painstakingly\nverify for every entry during every fetch and push that those commits\nobjects are still there.\n\nIt looks to me like a rather bad design, too, as the whole idea of having\na `git gc` is to update such information *then*.\n\nSadly, we also allow `git repack` to drop objects, and it is really the\nresponsibility of a command that drops objects to update things like the\n`shallow` list. Because that is exactly the time when its contents can go\nstale.\n\nCiao,\nDscho\n"},{"id":"352636","messageId":"20180716173636.GA18636@sigill.intra.peff.net","threadId":"48882","inReplyTo":"nycvar.QRO.7.76.6.1807142351100.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T17:36:36Z","receivedAt":"2018-07-16T17:36:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 14, 2018 at 11:56:57PM +0200, Johannes Schindelin wrote:\n\n> > > To avoid this problem, let's prune the shallow list in `git repack` when\n> > > the `-d` option is passed, unless `-A` is passed, too (which would force\n> > > the now-unreachable objects to be turned into loose objects instead of\n> > > being deleted).\n> > \n> > I'm not sure if this covers all cases:\n> > \n> > - even with \"-A\", we may still drop objects subject to\n> > --unpack-unreachable. So if your pack has an old mtime (e.g., because\n> > you haven't packed in a while) I think you'd see the same problem.\n> > \n> > - if you use \"-adk\", we'd keep all objects, and this pruning would not\n> > be necessary\n> \n> Sure. I can add those cases.\n\nThanks.\n\n> > I understand how this solves your immediate problem, but it feels like a\n> > weird layering violation (which I think is a result of existing layering\n> > violations ;) ).\n> \n> Okay, but please don't punish me for those existing layering violations by\n> forcing me to address them, instead of a quick bug fix for a very real bug\n> that was reported to me privately, and that I would like to see fixed\n> relatively quickly.\n\nI'm OK with having a partial fix, or one that fixes immediate pain\nwithout doing a big cleanup, as long as it doesn't make anything _worse_\nin a user-visible way. And that's really my question: is pruning here\ngoing to bite people unexpectedly (not rhetorical -- I really don't\nknow)?\n\nFor instance, at GitHub we do not ever run \"git gc\", but have our\nmaintenance regimen that builds around \"git repack\". I don't think this\npatch will matter to us either way, because we would never have a\nshallow repository in the first place. But I'm wondering if people in a\nsimilar situation might.\n\nI'm also not entirely sure if people _care_ if their shallow list is\npruned. Maybe it's not a big deal, and should just be quietly cleaned\nup.\n\nAnd I know that you're probably coming at it from the opposite angle.\nSomebody isn't running git-gc but doing a \"repack -adl\" and they _do_\nwant these shallow files cleaned up (because their repo is broken after\ndropping the objects). I just want to make sure we don't regress some\nother case.\n\n> > I.e., it seems unexpected that \"git repack\" is going to tweak your\n> > shallow lists. If we were designing from scratch, the sane behavior\n> > seems to me to be:\n> > \n> > 1. Shallow pruning should be its own separate command (distinct from\n> > either repacking or loose object pruning), and should be triggered\n> > as part of a normal git-gc.\n> \n> I fail to see the value in a separate command.\n\nThe value of a separate command is that you can run those other commands\nwithout triggering this behavior. I actually think \"git prune\" does too\nmuch already (e.g., imagine that I do not ever want to prune objects,\nbut I _do_ want to get rid of tmp_pack and tmp_obj cruft; what command\ndo I run?). But that is definitely not a new problem. And I'm OK with\nnot fixing it for now. My main concern is that we are using the presence\nof that mistake to justify making it again.\n\n> And: `git gc` already calls `git prune`, which *does* prune the shallow\n> list.\n\nRight, I was trying to propose that each individual cleanup step which\n_could_ be done independently should be done so, but that \"gc\" should\ncontinue to do them all.\n\nI think in the case of git-prune and prune_shallow(), it's a little\ntricky, though. It looks like prune_shallow() relies on somebody having\nmarked the SEEN flag, which implies doing a full reachability walk. So\nit's really amortizing the work already being done by prune.\n\nSpeaking of which, I don't see how your patch can work as-is. The repack\ncommand does not do such a walk, so it has no idea which commits are\nSEEN or not, and will delete all of them! Try this:\n\n [shallow of clone of git.git (or any repo)]\n $ git clone --no-local --depth=1 /path/to/git tmp\n ...\n $ cd tmp\n\n [we have a commit]\n $ git log --oneline -1\n de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits\n\n [repacking with existing git is fine]\n $ git repack -adl\n ...\n $ git log --oneline -1\n de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits\n\n [repacking with your patch empties the shallow file, which\n makes the repository look corrupt]\n $ /path/to/git repack -adl\n $ git log --oneline -1\n error: Could not read a2df50675979af639cf9490f7fc9b86fa18fd907\n fatal: Failed to traverse parents of commit de46fca5b43f47f3c5c6f9232a17490d39fc80b1\n\nSo either we have to do a reachability walk (which is expensive), or we\nhave to rely on some other command (like prune) that is doing a\nreachability walk and reuse that work.\n\nThat also probably means my \"separate command\" suggestion is a bad one.\nIf we're trying to amortize work, then we're better to have a single\ncommand with options to enable/disable certain operations (so in theory\nI could use \"git prune --no-prune-objects\" to just clean up tempfiles,\nand users of \"repack -adl\" could do something similar to _just_ prune\nthe shallow if they really didn't want to delete loose objects.\n\n> > AND ONE OF:\n> > \n> > 2a. Objects mentions in the shallow file are important, and therefore\n> > _are_ considered reachable on their own. Neither repack nor prune\n> > needs to know or care.\n> \n> If we were to do that, we would never be able to gc any stale shallow\n> commits.\n> \n> That would be rather bad a design, don't you agree?\n\nI had meant for this to be coupled with my (1), which would presumably\nremove unreachable commits from the shallow list in a separate step\n(based on what criteria, I don't know -- again, I'm not really a shallow\nuser).\n\n> > 2b. It's OK for shallow objects to be missing, and the shallow code\n> > should be more resilient to missing objects. Neither repack nor\n> > prune needs to know or care.\n> \n> That would be possible. Kind of like saying: we do have this list of\n> shallow commits, but oh, BTW, it is likely incorrect, so we painstakingly\n> verify for every entry during every fetch and push that those commits\n> objects are still there.\n\nObviously verifying reachability on each fetch is a bad idea. But my\nunderstanding of the shallow list is that it says \"this is a graft point\nwhere we do not have any of the parents\". If we find that we do not have\nsuch an object, would it be OK to quietly ignore that? We used to claim\n\"we do not have the parents of X\", but now we know \"well, we do not have\nX anymore either\".\n\nAgain, I may be showing my ignorance of the shallow code here.\n\n> It looks to me like a rather bad design, too, as the whole idea of having\n> a `git gc` is to update such information *then*.\n\nRight, I think \"git gc\" is absolutely the place to do such cleanups. My\nonly complaint was that having it as part of repack may be unexpected.\n\n-Peff\n"},{"id":"352784","messageId":"pull.9.v2.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-17T13:51:39Z","receivedAt":"2018-07-17T13:51:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Under certain circumstances, commits that were reachable can be made unreachable, e.g. via `git fetch --prune`. These commits might have been packed already, in which case `git repack -adlf` will just drop them without giving them the usual grace period before an eventual `git prune` (via `git gc`) prunes them.\n\nThis is a problem when the `shallow` file still lists them, which is the reason why `git prune` edits that file. And with the proposed changes, `git repack -ad` will now do the same.\n\nReported by Alejandro Pauly.\n\nChanges since v1:\n\n- Also trigger `prune_shallow()` when `--unpack-unreachable=` was passed to `git repack`.\n- No need to trigger `prune_shallow()` when `git repack` was called with `-k`.\n\nJohannes Schindelin (2):\n repack: point out a bug handling stale shallow info\n repack -ad: prune the list of shallow commits\n\n builtin/repack.c | 6 ++++++\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\n\nbase-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/9\n\nRange-diff vs v1:\n\n 1: d2be40131 = 1: d2be40131 repack: point out a bug handling stale shallow info\n 2: b4e01a963 ! 2: c7ee6e008 repack -ad: prune the list of shallow commits\n @@ -23,7 +23,8 @@\n To avoid this problem, let's prune the shallow list in `git repack` when\n the `-d` option is passed, unless `-A` is passed, too (which would force\n the now-unreachable objects to be turned into loose objects instead of\n - being deleted).\n + being deleted). Additionally, e also need to take `--keep-reachable` and\n + `--unpack-unreachable=` into account.\n \n Signed-off-by: Johannes Schindelin \n \n @@ -35,7 +36,9 @@\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n +\n -+\t\tif (!(pack_everything & LOOSEN_UNREACHABLE) &&\n ++\t\tif (!keep_unreachable &&\n ++\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n ++\t\t unpack_unreachable) &&\n +\t\t is_repository_shallow())\n +\t\t\tprune_shallow(0);\n \t}\n\n-- \ngitgitgadget\n"},{"id":"352785","messageId":"d2be4013134c0e9dc032f934c6af82721fec4eba.1531835498.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] repack: point out a bug handling stale shallow info","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-17T13:51:41Z","receivedAt":"2018-07-17T13:51:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nA `git fetch --prune` can turn previously-reachable objects unreachable,\neven commits that are in the `shallow` list. A subsequent `git repack\n-ad` will then unceremoniously drop those unreachable commits, and the\n`shallow` list will become stale. This means that when we try to fetch\nwith a larger `--depth` the next time, we may end up with:\n\n\tfatal: error in object: unshallow \n\nReported by Alejandro Pauly.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 943231af9..561485d31 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,4 +186,31 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '.git/shallow is edited by repack' '\n+\tgit init shallow-server &&\n+\ttest_commit -C shallow-server A &&\n+\ttest_commit -C shallow-server B &&\n+\tgit -C shallow-server checkout -b branch &&\n+\ttest_commit -C shallow-server C &&\n+\ttest_commit -C shallow-server E &&\n+\ttest_commit -C shallow-server D &&\n+\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n+\tgit -C shallow-server checkout master &&\n+\n+\tgit clone --depth=1 --no-tags --no-single-branch \\\n+\t\t\"file://$PWD/shallow-server\" shallow-client &&\n+\n+\t: now remove the branch and fetch with prune &&\n+\tgit -C shallow-server branch -D branch &&\n+\tgit -C shallow-client fetch --prune --depth=1 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\" &&\n+\tgit -C shallow-client repack -adfl &&\n+\ttest_must_fail git -C shallow-client rev-parse --verify $d^0 &&\n+\t! grep $d shallow-client/.git/shallow &&\n+\n+\tgit -C shallow-server branch branch-orig D^0 &&\n+\tgit -C shallow-client fetch --prune --depth=2 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"352786","messageId":"c7ee6e00875bbfe6108dcdcc43cb389096a28188.1531835498.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-17T13:51:42Z","receivedAt":"2018-07-17T13:51:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhile it is true that we never add unreachable commits into pack files\nintentionally (as `git repack`'s documentation states), we must not\nforget that a `git fetch --prune` (or even a `git fetch` when a ref was\nforce-pushed in the meantime) can make a commit unreachable that was\nreachable before.\n\nTherefore it is not safe to assume that a `git repack -adlf` will keep\nunreachable commits alone (under the assumption that they had not been\npacked in the first place).\n\nThis is particularly important to keep in mind when looking at the\n`.git/shallow` file: if any commits listed in that file become\nunreachable, it is not a problem, but if they go missing, it *is* a\nproblem. One symptom of this problem is that a deepening fetch may now\nfail with\n\n\tfatal: error in object: unshallow \n\nTo avoid this problem, let's prune the shallow list in `git repack` when\nthe `-d` option is passed, unless `-A` is passed, too (which would force\nthe now-unreachable objects to be turned into loose objects instead of\nbeing deleted). Additionally, e also need to take `--keep-reachable` and\n`--unpack-unreachable=` into account.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/repack.c | 6 ++++++\n t/t5537-fetch-shallow.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6c636e159..4caf57221 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -444,6 +444,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (!quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n+\n+\t\tif (!keep_unreachable &&\n+\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n+\t\t unpack_unreachable) &&\n+\t\t is_repository_shallow())\n+\t\t\tprune_shallow(0);\n \t}\n \n \tif (!no_update_server_info)\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 561485d31..d32ba20f9 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,7 +186,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '.git/shallow is edited by repack' '\n+test_expect_success '.git/shallow is edited by repack' '\n \tgit init shallow-server &&\n \ttest_commit -C shallow-server A &&\n \ttest_commit -C shallow-server B &&\n-- \ngitgitgadget\n"},{"id":"352791","messageId":"xmqqh8kxrfrf.fsf@gitster-ct.c.googlers.com","threadId":"48882","inReplyTo":"20180716173636.GA18636@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T16:25:24Z","receivedAt":"2018-07-17T16:25:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm OK with having a partial fix, or one that fixes immediate pain\n> without doing a big cleanup, as long as it doesn't make anything _worse_\n> in a user-visible way. And that's really my question: is pruning here\n> going to bite people unexpectedly (not rhetorical -- I really don't\n> know)?\n\nYeah, that matches the general guideline I follow when reviewing a\npatch that claims to make existing things better. And I do not\nthink I can explain to a third person why pruning here is a good\nidea and won't cause problems, after seeing these patches and\nthe discussion from the sideline.\n\n> Right, I think \"git gc\" is absolutely the place to do such cleanups. My\n> only complaint was that having it as part of repack may be unexpected.\n\n"},{"id":"352794","messageId":"CACsJy8Ce0kXTCzLf6jFq1QjY4Q4QePfk1JbgGUr-kQqQDCc7mA@mail.gmail.com","threadId":"48882","inReplyTo":"b4e01a963fd16f50d12c1f67c6e64bec8b1e9673.1531513093.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-17T16:39:58Z","receivedAt":"2018-07-17T16:40:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 13, 2018 at 10:19 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> While it is true that we never add unreachable commits into pack files\n> intentionally (as `git repack`'s documentation states), we must not\n> forget that a `git fetch --prune` (or even a `git fetch` when a ref was\n> force-pushed in the meantime) can make a commit unreachable that was\n> reachable before.\n>\n> Therefore it is not safe to assume that a `git repack -adlf` will keep\n> unreachable commits alone (under the assumption that they had not been\n> packed in the first place).\n>\n> This is particularly important to keep in mind when looking at the\n> `.git/shallow` file: if any commits listed in that file become\n> unreachable, it is not a problem, but if they go missing, it *is* a\n> problem. One symptom of this problem is that a deepening fetch may now\n> fail with\n>\n> fatal: error in object: unshallow \n>\n\nCould you elaborate a bit more? I don't quite see the connection\nbetween the reachability in the first two paragraphs and .git/shallow\nin the third one. I'm guessing we drop objects in between because\n\"they go missing\"? Where? How does prune_shallow() in prune.c play any\nrole in this (if it does)?\n-- \nDuy\n"},{"id":"352797","messageId":"CACsJy8Cxp6Gtt3YdWvQ0=t4vqBAqNxrGPeY4B7mtjsiLNpG5TQ@mail.gmail.com","threadId":"48882","inReplyTo":"CACsJy8Ce0kXTCzLf6jFq1QjY4Q4QePfk1JbgGUr-kQqQDCc7mA@mail.gmail.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-17T16:48:27Z","receivedAt":"2018-07-17T16:48:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 17, 2018 at 6:39 PM Duy Nguyen wrote:\n>\n> On Fri, Jul 13, 2018 at 10:19 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > From: Johannes Schindelin \n> >\n> > While it is true that we never add unreachable commits into pack files\n> > intentionally (as `git repack`'s documentation states), we must not\n> > forget that a `git fetch --prune` (or even a `git fetch` when a ref was\n> > force-pushed in the meantime) can make a commit unreachable that was\n> > reachable before.\n> >\n> > Therefore it is not safe to assume that a `git repack -adlf` will keep\n> > unreachable commits alone (under the assumption that they had not been\n> > packed in the first place).\n> >\n> > This is particularly important to keep in mind when looking at the\n> > `.git/shallow` file: if any commits listed in that file become\n> > unreachable, it is not a problem, but if they go missing, it *is* a\n> > problem. One symptom of this problem is that a deepening fetch may now\n> > fail with\n> >\n> > fatal: error in object: unshallow \n> >\n>\n> Could you elaborate a bit more?\n\nNever mind. The problem is described in another patch.. sigh.. I\nunderstand you want to flip that \"failure\" to \"success\" but personally\nI don't think it worth it to look back in history and have \"what?\"\nmoments like when I read this patch alone.\n-- \nDuy\n"},{"id":"352801","messageId":"CACsJy8BV3ii3MkYf6UObsX_JdDbT9ovY_K9dCCeYvQ3FWqnRUQ@mail.gmail.com","threadId":"48882","inReplyTo":"20180716173636.GA18636@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-17T17:28:07Z","receivedAt":"2018-07-17T17:28:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 16, 2018 at 7:38 PM Jeff King wrote:\n> in a user-visible way. And that's really my question: is pruning here\n> going to bite people unexpectedly (not rhetorical -- I really don't\n> know)?\n\nIf shallow points are no longer reachable, removing them should not\nbite anybody, I think.\n\n> For instance, at GitHub we do not ever run \"git gc\", but have our\n> maintenance regimen that builds around \"git repack\". I don't think this\n> patch will matter to us either way, because we would never have a\n> shallow repository in the first place. But I'm wondering if people in a\n> similar situation might.\n>\n> I'm also not entirely sure if people _care_ if their shallow list is\n> pruned. Maybe it's not a big deal, and should just be quietly cleaned\n> up.\n>\n> And I know that you're probably coming at it from the opposite angle.\n> Somebody isn't running git-gc but doing a \"repack -adl\" and they _do_\n> want these shallow files cleaned up (because their repo is broken after\n> dropping the objects). I just want to make sure we don't regress some\n> other case.\n>\n> > > I.e., it seems unexpected that \"git repack\" is going to tweak your\n> > > shallow lists. If we were designing from scratch, the sane behavior\n> > > seems to me to be:\n> > >\n> > > 1. Shallow pruning should be its own separate command (distinct from\n> > > either repacking or loose object pruning), and should be triggered\n> > > as part of a normal git-gc.\n> >\n> > I fail to see the value in a separate command.\n>\n> The value of a separate command is that you can run those other commands\n> without triggering this behavior. I actually think \"git prune\" does too\n> much already (e.g., imagine that I do not ever want to prune objects,\n> but I _do_ want to get rid of tmp_pack and tmp_obj cruft; what command\n> do I run?). But that is definitely not a new problem. And I'm OK with\n> not fixing it for now. My main concern is that we are using the presence\n> of that mistake to justify making it again.\n>\n> > And: `git gc` already calls `git prune`, which *does* prune the shallow\n> > list.\n>\n> Right, I was trying to propose that each individual cleanup step which\n> _could_ be done independently should be done so, but that \"gc\" should\n> continue to do them all.\n>\n> I think in the case of git-prune and prune_shallow(), it's a little\n> tricky, though. It looks like prune_shallow() relies on somebody having\n> marked the SEEN flag, which implies doing a full reachability walk.\n\nSorry, my bad for hiding this SEEN flag deep in library code like this\nin eab3296c7e (prune: clean .git/shallow after pruning objects -\n2013-12-05) But in my defense I didn't realize the horror of sharing\nobject flags a year later and added the \"object flag allocation\" in\nobject.h\n\n> So it's really amortizing the work already being done by prune.\n>\n> Speaking of which, I don't see how your patch can work as-is. The repack\n> command does not do such a walk, so it has no idea which commits are\n> SEEN or not, and will delete all of them! Try this:\n>\n> [shallow of clone of git.git (or any repo)]\n> $ git clone --no-local --depth=1 /path/to/git tmp\n> ...\n> $ cd tmp\n>\n> [we have a commit]\n> $ git log --oneline -1\n> de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits\n>\n> [repacking with existing git is fine]\n> $ git repack -adl\n> ...\n> $ git log --oneline -1\n> de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits\n>\n> [repacking with your patch empties the shallow file, which\n> makes the repository look corrupt]\n> $ /path/to/git repack -adl\n> $ git log --oneline -1\n> error: Could not read a2df50675979af639cf9490f7fc9b86fa18fd907\n> fatal: Failed to traverse parents of commit de46fca5b43f47f3c5c6f9232a17490d39fc80b1\n>\n> So either we have to do a reachability walk (which is expensive), or we\n> have to rely on some other command (like prune) that is doing a\n> reachability walk and reuse that work.\n\nSince \"git prune\" took care of loose objects, if we're going to delete\nsome redundant packs, we can perform a reasonably cheap \"reachability\"\ntest in repack, I think. We have the list of new packs from\npack-objects which should contain all reachable objects (and even some\nunreachable ones). If we traverse all of their idx files and mark as\nSEEN, then whatever shallow points that are not marked SEEN _and_ not\nloose objects could be safely removed.\n\n> > > 2b. It's OK for shallow objects to be missing, and the shallow code\n> > > should be more resilient to missing objects. Neither repack nor\n> > > prune needs to know or care.\n> >\n> > That would be possible. Kind of like saying: we do have this list of\n> > shallow commits, but oh, BTW, it is likely incorrect, so we painstakingly\n> > verify for every entry during every fetch and push that those commits\n> > objects are still there.\n>\n> Obviously verifying reachability on each fetch is a bad idea. But my\n> understanding of the shallow list is that it says \"this is a graft point\n> where we do not have any of the parents\". If we find that we do not have\n> such an object, would it be OK to quietly ignore that? We used to claim\n> \"we do not have the parents of X\", but now we know \"well, we do not have\n> X anymore either\".\n>\n> Again, I may be showing my ignorance of the shallow code here.\n\nI don't see any problem with this either, but then I've not worked on\nshallow code for quite some time. The only potential problem is where\nwe do this check. If we check (and drop) shallow points when we read\n.git/info/shallow, then when we write it down some time in the future\nwe accidentally prune them. Which might be ok but I feel safer when we\nonly prune at one place (prune/gc/repack) where we can be more careful\nand can do more testing.\n\nIf we read the shallow list as-is, then just not send \"shallow\" lines\nin fetch-pack for shallow points that do not exist, I don't see a\nproblem, but it may be a bit more work and we could get to some\nconfusing state where upload-pack gives us the same shallow point that\nwe have but ignore because we don't have objects behind it. Hm...\nactually I might see a problem :)\n-- \nDuy\n"},{"id":"352804","messageId":"CAPig+cRh5qezsFFkecwM1uVdeCwgTFqFnpUPDmb+GdNFVUUx4g@mail.gmail.com","threadId":"48882","inReplyTo":"c7ee6e00875bbfe6108dcdcc43cb389096a28188.1531835498.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] repack -ad: prune the list of shallow commits","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-17T17:45:03Z","receivedAt":"2018-07-17T17:45:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 17, 2018 at 9:51 AM Johannes Schindelin via GitGitGadget\n wrote:\n> While it is true that we never add unreachable commits into pack files\n> intentionally (as `git repack`'s documentation states), we must not\n> forget that a `git fetch --prune` (or even a `git fetch` when a ref was\n> force-pushed in the meantime) can make a commit unreachable that was\n> reachable before.\n>\n> Therefore it is not safe to assume that a `git repack -adlf` will keep\n> unreachable commits alone (under the assumption that they had not been\n> packed in the first place).\n>\n> This is particularly important to keep in mind when looking at the\n> `.git/shallow` file: if any commits listed in that file become\n> unreachable, it is not a problem, but if they go missing, it *is* a\n> problem. One symptom of this problem is that a deepening fetch may now\n> fail with\n>\n> fatal: error in object: unshallow \n>\n> To avoid this problem, let's prune the shallow list in `git repack` when\n> the `-d` option is passed, unless `-A` is passed, too (which would force\n> the now-unreachable objects to be turned into loose objects instead of\n> being deleted). Additionally, e also need to take `--keep-reachable` and\n\ns/, e/, we/\n\n> `--unpack-unreachable=` into account.\n>\n> Signed-off-by: Johannes Schindelin \n"},{"id":"352820","messageId":"20180717191531.GA30594@sigill.intra.peff.net","threadId":"48882","inReplyTo":"pull.9.v2.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T19:15:31Z","receivedAt":"2018-07-17T19:15:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 17, 2018 at 06:51:39AM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> Under certain circumstances, commits that were reachable can be made\n> unreachable, e.g. via `git fetch --prune`. These commits might have\n> been packed already, in which case `git repack -adlf` will just drop\n> them without giving them the usual grace period before an eventual\n> `git prune` (via `git gc`) prunes them.\n> \n> This is a problem when the `shallow` file still lists them, which is\n> the reason why `git prune` edits that file. And with the proposed\n> changes, `git repack -ad` will now do the same.\n> \n> Reported by Alejandro Pauly.\n> \n> Changes since v1:\n> \n> - Also trigger `prune_shallow()` when `--unpack-unreachable=` was passed to `git repack`.\n> - No need to trigger `prune_shallow()` when `git repack` was called with `-k`.\n\nI think you might have missed the bigger problem I pointed at, as it was\nburied deep within my later reply. Try applying this patch on top, which\ntests the opposite case (reachable shallow commits are retained):\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex d32ba20f9d..911e457ae1 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,17 +186,20 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_success '.git/shallow is edited by repack' '\n+test_expect_success 'set up shallow server' '\n \tgit init shallow-server &&\n \ttest_commit -C shallow-server A &&\n \ttest_commit -C shallow-server B &&\n \tgit -C shallow-server checkout -b branch &&\n \ttest_commit -C shallow-server C &&\n \ttest_commit -C shallow-server E &&\n \ttest_commit -C shallow-server D &&\n \td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n-\tgit -C shallow-server checkout master &&\n+\tgit -C shallow-server checkout master\n+'\n \n+test_expect_success 'repack drops unreachable objects from .git/shallow' '\n+\ttest_when_finished \"rm -rf shallow-client\" &&\n \tgit clone --depth=1 --no-tags --no-single-branch \\\n \t\t\"file://$PWD/shallow-server\" shallow-client &&\n \n@@ -213,4 +216,13 @@ test_expect_success '.git/shallow is edited by repack' '\n \t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n '\n \n+test_expect_success 'repack keeps reachable objects in .git/shallow' '\n+\ttest_when_finished \"rm -rf shallow-client\" &&\n+\tgit clone --depth=1 --no-tags --no-single-branch \\\n+\t\t\"file://$PWD/shallow-server\" shallow-client &&\n+\n+\tgit -C shallow-client repack -adfl &&\n+\tgrep $d shallow-client/.git/shallow\n+'\n+\n test_done\n"},{"id":"352821","messageId":"20180717192002.GB30594@sigill.intra.peff.net","threadId":"48882","inReplyTo":"20180717191531.GA30594@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T19:20:02Z","receivedAt":"2018-07-17T19:20:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 17, 2018 at 03:15:31PM -0400, Jeff King wrote:\n\n> > - Also trigger `prune_shallow()` when `--unpack-unreachable=` was passed to `git repack`.\n> > - No need to trigger `prune_shallow()` when `git repack` was called with `-k`.\n> \n> I think you might have missed the bigger problem I pointed at, as it was\n> buried deep within my later reply. Try applying this patch on top, which\n> tests the opposite case (reachable shallow commits are retained):\n\nBy the way, I notice that the patches themselves are cc'd to you, but\nthe cover letter isn't. So my reply went only to gitgitgadget, which\n(AFAIK) has not yet learned to work as a mail-to-comment gateway.\n\nSo I'm copying this to you directly to make sure you see it, but also\nbecause I'm not sure if the gitgitgadget cc behavior is intentional or\nnot.\n\n-Peff\n"},{"id":"352822","messageId":"20180717194148.GC30594@sigill.intra.peff.net","threadId":"48882","inReplyTo":"CACsJy8BV3ii3MkYf6UObsX_JdDbT9ovY_K9dCCeYvQ3FWqnRUQ@mail.gmail.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T19:41:48Z","receivedAt":"2018-07-17T19:41:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 17, 2018 at 07:28:07PM +0200, Duy Nguyen wrote:\n\n> On Mon, Jul 16, 2018 at 7:38 PM Jeff King wrote:\n> > in a user-visible way. And that's really my question: is pruning here\n> > going to bite people unexpectedly (not rhetorical -- I really don't\n> > know)?\n> \n> If shallow points are no longer reachable, removing them should not\n> bite anybody, I think.\n\nI slept on this to see if I could brainstorm any other ways.\n\nThe only thing I really came up with is that removing shallows is racy\nwith respect to the reachability check. For instance, if \"prune\" runs\nat the same as an incoming shallow fetch, we'll compute the reachability\nwithout holding the shallow lock. Somebody else may write an entry for\nan object we've never heard of (because it just showed up), and we'd\nerase it, making the repository appear corrupt.\n\nBut note that I used \"prune\" in the above example, because this bug\nalready exists. So probably we're not changing anything material here.\nThough if we wanted to fix it, I think it would require holding the\nshallow lock during the reachability analysis, which is probably not\nsomething that repack would want to do. Unlike prune, it then does\na lot of other expensive operations, like delta compression and writing\nout the pack, before it would get to the shallow prune.\n\nAlthough even holding the lock for the duration of \"prune\" is more than\nwe need. Shallow commits must be commits, so we don't need to do a full\ngraph walk (and in my experience there's often an order-of-magnitude\ndifference between the two; even more so if we're using Stolee's\ncommit-graph cache).\n\n> > I think in the case of git-prune and prune_shallow(), it's a little\n> > tricky, though. It looks like prune_shallow() relies on somebody having\n> > marked the SEEN flag, which implies doing a full reachability walk.\n> \n> Sorry, my bad for hiding this SEEN flag deep in library code like this\n> in eab3296c7e (prune: clean .git/shallow after pruning objects -\n> 2013-12-05) But in my defense I didn't realize the horror of sharing\n> object flags a year later and added the \"object flag allocation\" in\n> object.h\n\nSort of an aside to the patch under discussion, but I think it may make\nsense for prune_shallow() to take a callback function for determining\nwhether a commit is reachable.\n\nI have an old patch that teaches git-prune to lazily do the reachability\ncheck, since in many cases \"git repack\" will have just packed all of the\nloose objects. But it just occurred to me that this patch is totally\nbroken with respect to prune_shallow(), because it would not set the\nSEEN flag (I've literally been running with it for years, which goes to\nshow how often I use the shallow feature).\n\nAnd if we were to have repack do a prune_shallow(), it may want to use a\ndifferent method than traversing and marking each object SEEN.\n\n> > So either we have to do a reachability walk (which is expensive), or we\n> > have to rely on some other command (like prune) that is doing a\n> > reachability walk and reuse that work.\n> \n> Since \"git prune\" took care of loose objects, if we're going to delete\n> some redundant packs, we can perform a reasonably cheap \"reachability\"\n> test in repack, I think. We have the list of new packs from\n> pack-objects which should contain all reachable objects (and even some\n> unreachable ones). If we traverse all of their idx files and mark as\n> SEEN, then whatever shallow points that are not marked SEEN _and_ not\n> loose objects could be safely removed.\n\nHmm. I don't immediately see any reason that would not work with the\ncurrent code. But I am a little uncomfortable adding these sorts of\nsubtle dependencies and assumptions. It feels like we're building a\nhouse of cards.\n\n> I don't see any problem with this either, but then I've not worked on\n> shallow code for quite some time. The only potential problem is where\n> we do this check. If we check (and drop) shallow points when we read\n> .git/info/shallow, then when we write it down some time in the future\n> we accidentally prune them. Which might be ok but I feel safer when we\n> only prune at one place (prune/gc/repack) where we can be more careful\n> and can do more testing.\n> \n> If we read the shallow list as-is, then just not send \"shallow\" lines\n> in fetch-pack for shallow points that do not exist, I don't see a\n> problem, but it may be a bit more work and we could get to some\n> confusing state where upload-pack gives us the same shallow point that\n> we have but ignore because we don't have objects behind it. Hm...\n> actually I might see a problem :)\n\nYeah, I agree if we were to do this it would definitely be in a\n\"read-only\" way: let the current command skip those grafts for its\noperation, but do not impact the on-disk shallow file. Then races or\nother assumptions can at worst impact that operation, and not cause a\nlasting corruption (and in particular I think we'd be subject to the\nrace I described at the start of this mail).\n\n-Peff\n"},{"id":"352925","messageId":"CACsJy8DW8=FoRyEwAy48S76q0gxQbrS3emHou7QDhHqzwJRu+g@mail.gmail.com","threadId":"48882","inReplyTo":"20180717194148.GC30594@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-18T17:31:40Z","receivedAt":"2018-07-18T17:32:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 17, 2018 at 9:41 PM Jeff King wrote:\n> I slept on this to see if I could brainstorm any other ways.\n>\n> ..\n>\n> Sort of an aside to the patch under discussion, but I think it may make\n> sense for prune_shallow() to take a callback function for determining\n> whether a commit is reachable.\n>\n> I have an old patch that teaches git-prune to lazily do the reachability\n> check, since in many cases \"git repack\" will have just packed all of the\n> loose objects. But it just occurred to me that this patch is totally\n> broken with respect to prune_shallow(), because it would not set the\n> SEEN flag (I've literally been running with it for years, which goes to\n> show how often I use the shallow feature).\n>\n> And if we were to have repack do a prune_shallow(), it may want to use a\n> different method than traversing and marking each object SEEN.\n\nAll of this sounds good. The only thing I'd like to do a bit\ndifferently is to pass the reachable commits in prune_shallow() as a\ncommit-slab instead of taking a callback function. I'll refactor this\ncode, move prune_shallow() to a separate command prune-shallow and do\nthe locking thing.\n\nJohannes I could take over this bug too (since I introduced the\nshallow pruning and missed this part) if you are busy, but if you want\nit to be done fast, I can't help. I still have a list of things to go\nthrough.\n-- \nDuy\n"},{"id":"352927","messageId":"20180718174505.GA3084@sigill.intra.peff.net","threadId":"48882","inReplyTo":"CACsJy8DW8=FoRyEwAy48S76q0gxQbrS3emHou7QDhHqzwJRu+g@mail.gmail.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-18T17:45:05Z","receivedAt":"2018-07-18T17:45:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2018 at 07:31:40PM +0200, Duy Nguyen wrote:\n\n> > Sort of an aside to the patch under discussion, but I think it may make\n> > sense for prune_shallow() to take a callback function for determining\n> > whether a commit is reachable.\n> >\n> > I have an old patch that teaches git-prune to lazily do the reachability\n> > check, since in many cases \"git repack\" will have just packed all of the\n> > loose objects. But it just occurred to me that this patch is totally\n> > broken with respect to prune_shallow(), because it would not set the\n> > SEEN flag (I've literally been running with it for years, which goes to\n> > show how often I use the shallow feature).\n> >\n> > And if we were to have repack do a prune_shallow(), it may want to use a\n> > different method than traversing and marking each object SEEN.\n> \n> All of this sounds good. The only thing I'd like to do a bit\n> differently is to pass the reachable commits in prune_shallow() as a\n> commit-slab instead of taking a callback function. I'll refactor this\n> code, move prune_shallow() to a separate command prune-shallow and do\n> the locking thing.\n\nI think using a slab is much nicer than the current global-struct flags.\nBut it's not as flexible as a callback, for two reasons:\n\n - in the lazy case, the caller might not even have loaded the slab\n yet. On the other hand, it might be sufficient to just be broad\n there, and just always pre-populate the slab when\n is_repository_shallow(), before we even call into prune_shallow().\n If we have _any_ entries in the shallow file, we'd need to compute\n reachability.\n\n - it precludes any optimizations that compute partial reachability.\n Asking \"is XYZ reachable\" is a much easier question to answer than\n \"show me the full reachability graph.\" At the least, it lets you\n stop the graph traversal early. And with generation numbers, it can\n even avoid traversing down unproductive segments of the graph.\n\nI think it's OK to switch to a slab for now if you don't want to do the\ncallback thing. But I think a callback is probably long-term the right\nthing (and I actually think it may be _less_ work to implement, since\nthen prune would probably be OK sticking with the global struct flags).\n\n-Peff\n"},{"id":"352930","messageId":"CACsJy8DzUW69h-EdUs1nn227b3azFZYz5HPeG6+yp0FcAOQvNA@mail.gmail.com","threadId":"48882","inReplyTo":"20180718174505.GA3084@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-18T17:48:17Z","receivedAt":"2018-07-18T17:48:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 18, 2018 at 7:45 PM Jeff King wrote:\n>\n> On Wed, Jul 18, 2018 at 07:31:40PM +0200, Duy Nguyen wrote:\n>\n> > > Sort of an aside to the patch under discussion, but I think it may make\n> > > sense for prune_shallow() to take a callback function for determining\n> > > whether a commit is reachable.\n> > >\n> > > I have an old patch that teaches git-prune to lazily do the reachability\n> > > check, since in many cases \"git repack\" will have just packed all of the\n> > > loose objects. But it just occurred to me that this patch is totally\n> > > broken with respect to prune_shallow(), because it would not set the\n> > > SEEN flag (I've literally been running with it for years, which goes to\n> > > show how often I use the shallow feature).\n> > >\n> > > And if we were to have repack do a prune_shallow(), it may want to use a\n> > > different method than traversing and marking each object SEEN.\n> >\n> > All of this sounds good. The only thing I'd like to do a bit\n> > differently is to pass the reachable commits in prune_shallow() as a\n> > commit-slab instead of taking a callback function. I'll refactor this\n> > code, move prune_shallow() to a separate command prune-shallow and do\n> > the locking thing.\n>\n> I think using a slab is much nicer than the current global-struct flags.\n> But it's not as flexible as a callback, for two reasons:\n>\n> - in the lazy case, the caller might not even have loaded the slab\n> yet. On the other hand, it might be sufficient to just be broad\n> there, and just always pre-populate the slab when\n> is_repository_shallow(), before we even call into prune_shallow().\n> If we have _any_ entries in the shallow file, we'd need to compute\n> reachability.\n>\n> - it precludes any optimizations that compute partial reachability.\n> Asking \"is XYZ reachable\" is a much easier question to answer than\n> \"show me the full reachability graph.\" At the least, it lets you\n> stop the graph traversal early. And with generation numbers, it can\n> even avoid traversing down unproductive segments of the graph.\n\nBoth good points. Callback it is then.\n-- \nDuy\n"},{"id":"353040","messageId":"nycvar.QRO.7.76.6.1807191840490.71@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"xmqqh8kxrfrf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-19T16:42:56Z","receivedAt":"2018-07-19T16:43:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 17 Jul 2018, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm OK with having a partial fix, or one that fixes immediate pain\n> > without doing a big cleanup, as long as it doesn't make anything _worse_\n> > in a user-visible way. And that's really my question: is pruning here\n> > going to bite people unexpectedly (not rhetorical -- I really don't\n> > know)?\n> \n> Yeah, that matches the general guideline I follow when reviewing a\n> patch that claims to make existing things better. And I do not\n> think I can explain to a third person why pruning here is a good\n> idea and won't cause problems, after seeing these patches and\n> the discussion from the sideline.\n\nIt is very easy to explain: `git repack` can drop unreachable commits\nwithout further warning, making the corresponding entries in\n`.git/shallow` invalid, which causes serious problems when deepening the\nbranches.\n\nThe solution is easy: drop also the now-invalid entries in `.git/shallow`\nafter dropping unreachable commits unceremoniously.\n\nWhile I am sympathetic to Peff's attempt to make this a bit more general,\nI think he overthinks it, and we do not even need to complexify the code\nfor now.\n\nCiao,\nDscho\n"},{"id":"353050","messageId":"nycvar.QRO.7.76.6.1807191946090.71@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"20180717192002.GB30594@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-19T17:48:11Z","receivedAt":"2018-07-19T17:48:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 17 Jul 2018, Jeff King wrote:\n\n> On Tue, Jul 17, 2018 at 03:15:31PM -0400, Jeff King wrote:\n> \n> > > - Also trigger `prune_shallow()` when `--unpack-unreachable=` was passed to `git repack`.\n> > > - No need to trigger `prune_shallow()` when `git repack` was called with `-k`.\n> > \n> > I think you might have missed the bigger problem I pointed at, as it was\n> > buried deep within my later reply. Try applying this patch on top, which\n> > tests the opposite case (reachable shallow commits are retained):\n> \n> By the way, I notice that the patches themselves are cc'd to you, but\n> the cover letter isn't. So my reply went only to gitgitgadget, which\n> (AFAIK) has not yet learned to work as a mail-to-comment gateway.\n> \n> So I'm copying this to you directly to make sure you see it, but also\n> because I'm not sure if the gitgitgadget cc behavior is intentional or\n> not.\n\nIt is one of the existing TODOs to Cc: the person who hit \"/submit\". So I\nam aware of it, and it is recorded in the TODO.md file.\n\nBut as the GitHub user might not be associated with a public email, this\nwill need an internal map that will be extended to whoever is in the list\nof allowed users.\n\nCiao,\nDscho\n"},{"id":"353051","messageId":"nycvar.QRO.7.76.6.1807191949150.71@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"CACsJy8Cxp6Gtt3YdWvQ0=t4vqBAqNxrGPeY4B7mtjsiLNpG5TQ@mail.gmail.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-19T17:50:03Z","receivedAt":"2018-07-19T17:50:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 17 Jul 2018, Duy Nguyen wrote:\n\n> On Tue, Jul 17, 2018 at 6:39 PM Duy Nguyen wrote:\n> >\n> > On Fri, Jul 13, 2018 at 10:19 PM Johannes Schindelin via GitGitGadget\n> > wrote:\n> > >\n> > > From: Johannes Schindelin \n> > >\n> > > While it is true that we never add unreachable commits into pack files\n> > > intentionally (as `git repack`'s documentation states), we must not\n> > > forget that a `git fetch --prune` (or even a `git fetch` when a ref was\n> > > force-pushed in the meantime) can make a commit unreachable that was\n> > > reachable before.\n> > >\n> > > Therefore it is not safe to assume that a `git repack -adlf` will keep\n> > > unreachable commits alone (under the assumption that they had not been\n> > > packed in the first place).\n> > >\n> > > This is particularly important to keep in mind when looking at the\n> > > `.git/shallow` file: if any commits listed in that file become\n> > > unreachable, it is not a problem, but if they go missing, it *is* a\n> > > problem. One symptom of this problem is that a deepening fetch may now\n> > > fail with\n> > >\n> > > fatal: error in object: unshallow \n> > >\n> >\n> > Could you elaborate a bit more?\n> \n> Never mind. The problem is described in another patch.. sigh.. I\n> understand you want to flip that \"failure\" to \"success\" but personally\n> I don't think it worth it to look back in history and have \"what?\"\n> moments like when I read this patch alone.\n\nThe split was not meant for your benefit, but for the benefit of verifying\nthat I indeed fixed a problem.\n\nI don't think it is a wise idea to squash them together.\n\nCiao,\nDscho\n"},{"id":"353078","messageId":"xmqq7elrdk8x.fsf@gitster-ct.c.googlers.com","threadId":"48882","inReplyTo":"nycvar.QRO.7.76.6.1807191840490.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-19T20:49:02Z","receivedAt":"2018-07-19T20:49:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Tue, 17 Jul 2018, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > I'm OK with having a partial fix, or one that fixes immediate pain\n>> > without doing a big cleanup, as long as it doesn't make anything _worse_\n>> > in a user-visible way. And that's really my question: is pruning here\n>> > going to bite people unexpectedly (not rhetorical -- I really don't\n>> > know)?\n>> \n>> Yeah, that matches the general guideline I follow when reviewing a\n>> patch that claims to make existing things better. And I do not\n>> think I can explain to a third person why pruning here is a good\n>> idea and won't cause problems, after seeing these patches and\n>> the discussion from the sideline.\n>\n> It is very easy to explain: `git repack` can drop unreachable commits\n> without further warning, making the corresponding entries in\n> `.git/shallow` invalid, which causes serious problems when deepening the\n> branches.\n\nThat explains why you wrote the patch very clearly.\n\n> The solution is easy: drop also the now-invalid entries in `.git/shallow`\n> after dropping unreachable commits unceremoniously.\n\nSorry, but I do not think I can relay that as an explanation why it\nwon't cause problems to a third person.\n\nThe entries in shallow file says that history behind them may not\nexist in the repository due to its shallowness but history after\nthem are supposed to be traversable (otherwise we have a repository\ncorruption). It is true that an entry that itself no longer exists\nin this repository should not be in shallow file, as the presence of\nthat entry breaks that promise the file is making---that commit\nought to exist and it is safe to traverse down to it, so keeping the\nentry in the file is absolutely a wrong thing to do.\n\nBut that does not automatically mean that just simply removing it\nmakes the resulting repository good, does it? Wouldn't the solution\nfor that corruption be to set a new entry to stop history traversal\nbefore reaching that (now-missing) commit? If your solution and\nexplanatoin were like that, then I can understand why it won't cause\nproblems, because the resulting repository would be shallower than\nit originally was, as if you cloned with a smaller depth, but it is\nnot corrupt.\n\nPerhaps your rationale is that by trading one shape of corrupt\nrepository (where a commit that does not even exist is in the\nshallow file, breaking the early start-up sequence when it tries to\nread the entries) with another shape of corrupt repsitory (where\nshallow does not completely tell where to stop, and traversing the\nhistory can eventually hit a missing commit because no entry in the\nshallow file stops such a traversal), at least deepening fetch can\nstart (instead of dying while trying to see how shallow the\nrepository currently is) and that can be used to recover the corrupt\nrepository back into a usable state? If that is the justification,\nI can fully buy that, but that is not what I am hearing in your easy\nto explain answer, so I am still puzzled.\n\n"},{"id":"353118","messageId":"xmqqeffycl00.fsf@gitster-ct.c.googlers.com","threadId":"48882","inReplyTo":"xmqq7elrdk8x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T09:30:23Z","receivedAt":"2018-07-20T09:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sorry, but I do not think I can relay that as an explanation why it\n> won't cause problems to a third person.\n\nOK, ignore this. I was being stupid.\n\n> The entries in shallow file says that history behind them may not\n> exist in the repository due to its shallowness but history after\n> them are supposed to be traversable (otherwise we have a repository\n> corruption). It is true that an entry that itself no longer exists\n> in this repository should not be in shallow file, as the presence of\n> that entry breaks that promise the file is making---that commit\n> ought to exist and it is safe to traverse down to it, so keeping the\n> entry in the file is absolutely a wrong thing to do.\n>\n> But that does not automatically mean that just simply removing it\n> makes the resulting repository good, does it? Wouldn't the solution\n> for that corruption be to set a new entry to stop history traversal\n> before reaching that (now-missing) commit?\n\nThe above is overly pessimistic and worried about an impossible\nsituation, I would think. The reason why a commit that used to be\nin the shallow file is being pruned during a \"repack\" is because it\nhas become unreachable. By definition, no future history traversal\nthat wants to enumerate reachable commits needs to be stopped from\nfinding that commits that are older than this commit being pruned\nare missing by having this in the shallow list. If there is a ref\nor a reflog entry from which such a problematic traversal starts at,\nwe wouldn't be pruing this commit in the first place, because the\ncommit has not become unreachable yet.\n\nSo a repository does not become corrupt by pruning the commit *and*\nremoving it from the shallow file at the same time.\n\n\n"},{"id":"353177","messageId":"20180720193150.GC26403@sigill.intra.peff.net","threadId":"48882","inReplyTo":"xmqqeffycl00.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] repack -ad: prune the list of shallow commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T19:31:50Z","receivedAt":"2018-07-20T19:31:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 20, 2018 at 02:30:23AM -0700, Junio C Hamano wrote:\n\n> > The entries in shallow file says that history behind them may not\n> > exist in the repository due to its shallowness but history after\n> > them are supposed to be traversable (otherwise we have a repository\n> > corruption). It is true that an entry that itself no longer exists\n> > in this repository should not be in shallow file, as the presence of\n> > that entry breaks that promise the file is making---that commit\n> > ought to exist and it is safe to traverse down to it, so keeping the\n> > entry in the file is absolutely a wrong thing to do.\n> >\n> > But that does not automatically mean that just simply removing it\n> > makes the resulting repository good, does it? Wouldn't the solution\n> > for that corruption be to set a new entry to stop history traversal\n> > before reaching that (now-missing) commit?\n> \n> The above is overly pessimistic and worried about an impossible\n> situation, I would think. The reason why a commit that used to be\n> in the shallow file is being pruned during a \"repack\" is because it\n> has become unreachable. By definition, no future history traversal\n> that wants to enumerate reachable commits needs to be stopped from\n> finding that commits that are older than this commit being pruned\n> are missing by having this in the shallow list. If there is a ref\n> or a reflog entry from which such a problematic traversal starts at,\n> we wouldn't be pruing this commit in the first place, because the\n> commit has not become unreachable yet.\n> \n> So a repository does not become corrupt by pruning the commit *and*\n> removing it from the shallow file at the same time.\n\nRight. I think a lot of this is rethinking how shallow pruning works,\ntoo, which is not something Dscho is trying to change. The simplest\nargument (which I think Dscho has made elsewhere, too) is: this is\nnecessary in the current shallow code when dropping objects. We do it\ntherefore from prune, but miss the case when git-repack is run itself\noutside of git-gc.\n\nI do still think the gc/prune architecture is a bit muddled, but at this\npoint in the discussion I feel OK saying that people running \"git repack\n-ad\" would not be upset to have their shallows pruned.\n\nBut the patch is still not OK as-is because prune_shallow() requires the\nSEEN flag on each reachable object struct, which we have not set in the\nrepack process (hence the failing test I posted earlier). So we need a\nsolution for that, which may impact ideas about how the call works.\nE.g., some possible solutions are:\n\n - teach pack-objects to optionally trigger the shallow prune based on\n its internal walk\n\n - have repack use the just-completed pack as a hint about reachability\n\n - introduce a mechanism to trigger the shallow prune based on a\n commit-only reachability check, and run that from repack (or from gc\n and document that it must be run if you are using repack as a manual\n gc replacement)\n\nI'm not advocating any particular solution there, but just showing that\nthere's an array of them (and probably more that I didn't mention).\n\n-Peff\n"},{"id":"361212","messageId":"pull.9.v3.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] repack -ad: fix after fetch --prune in a shallow repository","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:05:35Z","receivedAt":"2018-10-22T22:05:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Under certain circumstances, commits that were reachable can be made\nunreachable, e.g. via git fetch --prune. These commits might have been\npacked already, in which case git repack -adlf will just drop them without\ngiving them the usual grace period before an eventual git prune (via git gc)\nprunes them.\n\nThis is a problem when the shallow file still lists them, which is the\nreason why git prune edits that file. And with the proposed changes, git\nrepack -ad will now do the same.\n\nReported by Alejandro Pauly.\n\nChanges since v2:\n\n * Fixed a typo in the last commit message.\n * Added an explanation to the last commit message why we do not simply skip\n non-existing shallow commits at fetch time.\n * Introduced a new, \"quick prune\" mode where prune_shallow() does not try\n to drop unreachable commits, but only non-existing ones.\n * Rebased to current master because there were too many merge conflicts for\n my liking otherwise.\n\nChanges since v1:\n\n * Also trigger prune_shallow() when --unpack-unreachable= was\n passed to git repack.\n * No need to trigger prune_shallow() when git repack was called with -k.\n\nJohannes Schindelin (3):\n repack: point out a bug handling stale shallow info\n shallow: offer to prune only non-existing entries\n repack -ad: prune the list of shallow commits\n\n builtin/prune.c | 2 +-\n builtin/repack.c | 6 ++++++\n commit.h | 2 +-\n shallow.c | 22 +++++++++++++++++-----\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 52 insertions(+), 7 deletions(-)\n\n\nbase-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/9\n\nRange-diff vs v2:\n\n 1: d2be40131 ! 1: ed8559b91 repack: point out a bug handling stale shallow info\n @@ -48,4 +48,6 @@\n +\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n +'\n +\n - test_done\n + . \"$TEST_DIRECTORY\"/lib-httpd.sh\n + start_httpd\n + \n -: --------- > 2: f085eb4f7 shallow: offer to prune only non-existing entries\n 2: c7ee6e008 ! 3: 1f9ff57d5 repack -ad: prune the list of shallow commits\n @@ -2,15 +2,19 @@\n \n repack -ad: prune the list of shallow commits\n \n - While it is true that we never add unreachable commits into pack files\n - intentionally (as `git repack`'s documentation states), we must not\n - forget that a `git fetch --prune` (or even a `git fetch` when a ref was\n + `git repack` can drop unreachable commits without further warning,\n + making the corresponding entries in `.git/shallow` invalid, which causes\n + serious problems when deepening the branches.\n +\n + One scenario where unreachable commits are dropped by `git repack` is\n + when a `git fetch --prune` (or even a `git fetch` when a ref was\n force-pushed in the meantime) can make a commit unreachable that was\n reachable before.\n \n Therefore it is not safe to assume that a `git repack -adlf` will keep\n unreachable commits alone (under the assumption that they had not been\n - packed in the first place).\n + packed in the first place, which is an assumption at least some of Git's\n + code seems to make).\n \n This is particularly important to keep in mind when looking at the\n `.git/shallow` file: if any commits listed in that file become\n @@ -23,8 +27,16 @@\n To avoid this problem, let's prune the shallow list in `git repack` when\n the `-d` option is passed, unless `-A` is passed, too (which would force\n the now-unreachable objects to be turned into loose objects instead of\n - being deleted). Additionally, e also need to take `--keep-reachable` and\n - `--unpack-unreachable=` into account.\n + being deleted). Additionally, we also need to take `--keep-reachable`\n + and `--unpack-unreachable=` into account.\n +\n + Note: an alternative solution discussed during the review of this patch\n + was to teach `git fetch` to simply ignore entries in .git/shallow if the\n + corresponding commits do not exist locally. A quick test, however,\n + revealed that the .git/shallow file is written during a shallow *clone*,\n + in which case the commits do not exist, either, but the \"shallow\" line\n + *does* need to be sent. Therefore, this approach would be a lot more\n + finicky than the approach presented by the this patch.\n \n Signed-off-by: Johannes Schindelin \n \n @@ -32,15 +44,15 @@\n --- a/builtin/repack.c\n +++ b/builtin/repack.c\n @@\n - \t\tif (!quiet && isatty(2))\n + \t\tif (!po_args.quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n +\n +\t\tif (!keep_unreachable &&\n +\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n +\t\t unpack_unreachable) &&\n -+\t\t is_repository_shallow())\n -+\t\t\tprune_shallow(0);\n ++\t\t is_repository_shallow(the_repository))\n ++\t\t\tprune_shallow(0, 1);\n \t}\n \n \tif (!no_update_server_info)\n\n-- \ngitgitgadget\n"},{"id":"361213","messageId":"ed8559b91b7a3b51a5950d62e78fc726ed5b44c2.1540245934.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] repack: point out a bug handling stale shallow info","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:05:37Z","receivedAt":"2018-10-22T22:05:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nA `git fetch --prune` can turn previously-reachable objects unreachable,\neven commits that are in the `shallow` list. A subsequent `git repack\n-ad` will then unceremoniously drop those unreachable commits, and the\n`shallow` list will become stale. This means that when we try to fetch\nwith a larger `--depth` the next time, we may end up with:\n\n\tfatal: error in object: unshallow \n\nReported by Alejandro Pauly.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 7045685e2..2d0031703 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,6 +186,33 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '.git/shallow is edited by repack' '\n+\tgit init shallow-server &&\n+\ttest_commit -C shallow-server A &&\n+\ttest_commit -C shallow-server B &&\n+\tgit -C shallow-server checkout -b branch &&\n+\ttest_commit -C shallow-server C &&\n+\ttest_commit -C shallow-server E &&\n+\ttest_commit -C shallow-server D &&\n+\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n+\tgit -C shallow-server checkout master &&\n+\n+\tgit clone --depth=1 --no-tags --no-single-branch \\\n+\t\t\"file://$PWD/shallow-server\" shallow-client &&\n+\n+\t: now remove the branch and fetch with prune &&\n+\tgit -C shallow-server branch -D branch &&\n+\tgit -C shallow-client fetch --prune --depth=1 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\" &&\n+\tgit -C shallow-client repack -adfl &&\n+\ttest_must_fail git -C shallow-client rev-parse --verify $d^0 &&\n+\t! grep $d shallow-client/.git/shallow &&\n+\n+\tgit -C shallow-server branch branch-orig D^0 &&\n+\tgit -C shallow-client fetch --prune --depth=2 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \ngitgitgadget\n\n"},{"id":"361214","messageId":"f085eb4f728f5cd102f56b7a90ce9b10fdb59dee.1540245934.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:05:38Z","receivedAt":"2018-10-22T22:05:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `prune_shallow()` function wants a full reachability check to be\ncompleted before it goes to work, to ensure that all unreachable entries\nare removed from the shallow file.\n\nHowever, in the upcoming patch we do not even want to go that far. We\nreally only need to remove entries corresponding to pruned commits, i.e.\nto commits that no longer exist.\n\nLet's support that use case.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/prune.c | 2 +-\n commit.h | 2 +-\n shallow.c | 22 +++++++++++++++++-----\n 3 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 41230f821..6d6ab6cf1 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tfree(s);\n \n \tif (is_repository_shallow(the_repository))\n-\t\tprune_shallow(show_only);\n+\t\tprune_shallow(show_only, 0);\n \n \treturn 0;\n }\ndiff --git a/commit.h b/commit.h\nindex 1d260d62f..ff34447ab 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,\n \t\t\t\t\t uint32_t **used,\n \t\t\t\t\t int *ref_status);\n extern int delayed_reachability_test(struct shallow_info *si, int c);\n-extern void prune_shallow(int show_only);\n+extern void prune_shallow(int show_only, int quick_prune);\n extern struct trace_key trace_shallow;\n \n extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\ndiff --git a/shallow.c b/shallow.c\nindex 732e18d54..0a2671bc2 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)\n \n #define SEEN_ONLY 1\n #define VERBOSE 2\n+#define QUICK_PRUNE 4\n \n struct write_shallow_data {\n \tstruct strbuf *out;\n@@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \tconst char *hex = oid_to_hex(&graft->oid);\n \tif (graft->nr_parent != -1)\n \t\treturn 0;\n-\tif (data->flags & SEEN_ONLY) {\n+\tif (data->flags & QUICK_PRUNE) {\n+\t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n+\t\tif (!c || parse_commit(c))\n+\t\t\treturn 0;\n+\t} else if (data->flags & SEEN_ONLY) {\n \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n \t\tif (!c || !(c->object.flags & SEEN)) {\n \t\t\tif (data->flags & VERBOSE)\n@@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)\n \n /*\n * mark_reachable_objects() should have been run prior to this and all\n- * reachable commits marked as \"SEEN\".\n+ * reachable commits marked as \"SEEN\", except when quick_prune is non-zero,\n+ * in which case lines are excised from the shallow file if they refer to\n+ * commits that do not exist (any longer).\n */\n-void prune_shallow(int show_only)\n+void prune_shallow(int show_only, int quick_prune)\n {\n \tstruct lock_file shallow_lock = LOCK_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tunsigned flags = SEEN_ONLY;\n \tint fd;\n \n+\tif (quick_prune)\n+\t\tflags |= QUICK_PRUNE;\n+\n \tif (show_only) {\n-\t\twrite_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);\n+\t\tflags |= VERBOSE;\n+\t\twrite_shallow_commits_1(&sb, 0, NULL, flags);\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n@@ -388,7 +400,7 @@ void prune_shallow(int show_only)\n \t\t\t\t git_path_shallow(the_repository),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update(the_repository);\n-\tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n+\tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_lock_file_path(&shallow_lock));\n-- \ngitgitgadget\n\n"},{"id":"361215","messageId":"1f9ff57d52a72e3795ac4a924e23a64b91b1f83e.1540245934.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-22T22:05:40Z","receivedAt":"2018-10-22T22:05:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\n`git repack` can drop unreachable commits without further warning,\nmaking the corresponding entries in `.git/shallow` invalid, which causes\nserious problems when deepening the branches.\n\nOne scenario where unreachable commits are dropped by `git repack` is\nwhen a `git fetch --prune` (or even a `git fetch` when a ref was\nforce-pushed in the meantime) can make a commit unreachable that was\nreachable before.\n\nTherefore it is not safe to assume that a `git repack -adlf` will keep\nunreachable commits alone (under the assumption that they had not been\npacked in the first place, which is an assumption at least some of Git's\ncode seems to make).\n\nThis is particularly important to keep in mind when looking at the\n`.git/shallow` file: if any commits listed in that file become\nunreachable, it is not a problem, but if they go missing, it *is* a\nproblem. One symptom of this problem is that a deepening fetch may now\nfail with\n\n\tfatal: error in object: unshallow \n\nTo avoid this problem, let's prune the shallow list in `git repack` when\nthe `-d` option is passed, unless `-A` is passed, too (which would force\nthe now-unreachable objects to be turned into loose objects instead of\nbeing deleted). Additionally, we also need to take `--keep-reachable`\nand `--unpack-unreachable=` into account.\n\nNote: an alternative solution discussed during the review of this patch\nwas to teach `git fetch` to simply ignore entries in .git/shallow if the\ncorresponding commits do not exist locally. A quick test, however,\nrevealed that the .git/shallow file is written during a shallow *clone*,\nin which case the commits do not exist, either, but the \"shallow\" line\n*does* need to be sent. Therefore, this approach would be a lot more\nfinicky than the approach presented by the this patch.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/repack.c | 6 ++++++\n t/t5537-fetch-shallow.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c6a7943d5..9217fc832 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (!po_args.quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n+\n+\t\tif (!keep_unreachable &&\n+\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n+\t\t unpack_unreachable) &&\n+\t\t is_repository_shallow(the_repository))\n+\t\t\tprune_shallow(0, 1);\n \t}\n \n \tif (!no_update_server_info)\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 2d0031703..777c9d1dc 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,7 +186,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '.git/shallow is edited by repack' '\n+test_expect_success '.git/shallow is edited by repack' '\n \tgit init shallow-server &&\n \ttest_commit -C shallow-server A &&\n \ttest_commit -C shallow-server B &&\n-- \ngitgitgadget\n"},{"id":"361269","messageId":"nycvar.QRO.7.76.6.1810231214350.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"pull.9.v3.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] repack -ad: fix after fetch --prune in a shallow repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-23T10:15:46Z","receivedAt":"2018-10-23T10:15:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 22 Oct 2018, Johannes Schindelin via GitGitGadget wrote:\n\n> Under certain circumstances, commits that were reachable can be made\n> unreachable, e.g. via git fetch --prune. These commits might have been\n> packed already, in which case git repack -adlf will just drop them without\n> giving them the usual grace period before an eventual git prune (via git gc)\n> prunes them.\n> \n> This is a problem when the shallow file still lists them, which is the\n> reason why git prune edits that file. And with the proposed changes, git\n> repack -ad will now do the same.\n> \n> Reported by Alejandro Pauly.\n> \n> Changes since v2:\n> \n> * Fixed a typo in the last commit message.\n> * Added an explanation to the last commit message why we do not simply skip\n> non-existing shallow commits at fetch time.\n> * Introduced a new, \"quick prune\" mode where prune_shallow() does not try\n> to drop unreachable commits, but only non-existing ones.\n\nBTW this was supposed to address Peff's concern that the SEEN flag would\nnot be marking all reachable shallow commits at the time when\n`cmd_repack()` calls `prune_shallow()`, i.e. the last concern about this\nstop-gap patch series.\n\nCiao,\nDscho\n\n> * Rebased to current master because there were too many merge conflicts for\n> my liking otherwise.\n> \n> Changes since v1:\n> \n> * Also trigger prune_shallow() when --unpack-unreachable= was\n> passed to git repack.\n> * No need to trigger prune_shallow() when git repack was called with -k.\n> \n> Johannes Schindelin (3):\n> repack: point out a bug handling stale shallow info\n> shallow: offer to prune only non-existing entries\n> repack -ad: prune the list of shallow commits\n> \n> builtin/prune.c | 2 +-\n> builtin/repack.c | 6 ++++++\n> commit.h | 2 +-\n> shallow.c | 22 +++++++++++++++++-----\n> t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n> 5 files changed, 52 insertions(+), 7 deletions(-)\n> \n> \n> base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1\n> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/9\n> \n> Range-diff vs v2:\n> \n> 1: d2be40131 ! 1: ed8559b91 repack: point out a bug handling stale shallow info\n> @@ -48,4 +48,6 @@\n> +\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n> +'\n> +\n> - test_done\n> + . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> + start_httpd\n> + \n> -: --------- > 2: f085eb4f7 shallow: offer to prune only non-existing entries\n> 2: c7ee6e008 ! 3: 1f9ff57d5 repack -ad: prune the list of shallow commits\n> @@ -2,15 +2,19 @@\n> \n> repack -ad: prune the list of shallow commits\n> \n> - While it is true that we never add unreachable commits into pack files\n> - intentionally (as `git repack`'s documentation states), we must not\n> - forget that a `git fetch --prune` (or even a `git fetch` when a ref was\n> + `git repack` can drop unreachable commits without further warning,\n> + making the corresponding entries in `.git/shallow` invalid, which causes\n> + serious problems when deepening the branches.\n> +\n> + One scenario where unreachable commits are dropped by `git repack` is\n> + when a `git fetch --prune` (or even a `git fetch` when a ref was\n> force-pushed in the meantime) can make a commit unreachable that was\n> reachable before.\n> \n> Therefore it is not safe to assume that a `git repack -adlf` will keep\n> unreachable commits alone (under the assumption that they had not been\n> - packed in the first place).\n> + packed in the first place, which is an assumption at least some of Git's\n> + code seems to make).\n> \n> This is particularly important to keep in mind when looking at the\n> `.git/shallow` file: if any commits listed in that file become\n> @@ -23,8 +27,16 @@\n> To avoid this problem, let's prune the shallow list in `git repack` when\n> the `-d` option is passed, unless `-A` is passed, too (which would force\n> the now-unreachable objects to be turned into loose objects instead of\n> - being deleted). Additionally, e also need to take `--keep-reachable` and\n> - `--unpack-unreachable=` into account.\n> + being deleted). Additionally, we also need to take `--keep-reachable`\n> + and `--unpack-unreachable=` into account.\n> +\n> + Note: an alternative solution discussed during the review of this patch\n> + was to teach `git fetch` to simply ignore entries in .git/shallow if the\n> + corresponding commits do not exist locally. A quick test, however,\n> + revealed that the .git/shallow file is written during a shallow *clone*,\n> + in which case the commits do not exist, either, but the \"shallow\" line\n> + *does* need to be sent. Therefore, this approach would be a lot more\n> + finicky than the approach presented by the this patch.\n> \n> Signed-off-by: Johannes Schindelin \n> \n> @@ -32,15 +44,15 @@\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@\n> - \t\tif (!quiet && isatty(2))\n> + \t\tif (!po_args.quiet && isatty(2))\n> \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> \t\tprune_packed_objects(opts);\n> +\n> +\t\tif (!keep_unreachable &&\n> +\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n> +\t\t unpack_unreachable) &&\n> -+\t\t is_repository_shallow())\n> -+\t\t\tprune_shallow(0);\n> ++\t\t is_repository_shallow(the_repository))\n> ++\t\t\tprune_shallow(0, 1);\n> \t}\n> \n> \tif (!no_update_server_info)\n> \n> -- \n> gitgitgadget\n> \n"},{"id":"361356","messageId":"xmqqr2ggovxk.fsf@gitster-ct.c.googlers.com","threadId":"48882","inReplyTo":"ed8559b91b7a3b51a5950d62e78fc726ed5b44c2.1540245934.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T03:39:03Z","receivedAt":"2018-10-24T03:39:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> A `git fetch --prune` can turn previously-reachable objects unreachable,\n> even commits that are in the `shallow` list. A subsequent `git repack\n> -ad` will then unceremoniously drop those unreachable commits, and the\n> `shallow` list will become stale. This means that when we try to fetch\n> with a larger `--depth` the next time, we may end up with:\n>\n> \tfatal: error in object: unshallow \n\nNicely analysed and described. One minor thing nagged me in the\nimplementation but it is not a big issue.\n\n> +...\n> +\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n> +\tgit -C shallow-server checkout master &&\n> +\n> +...\n> +\t! grep $d shallow-client/.git/shallow &&\n\nWe know D (and $d) is not a tag, but perhaps the place that assigns\nto $d (way above) can do the rev-parse of D^0, not D, to make it\nmore clear what is going on, especially given that...\n\n> +\tgit -C shallow-server branch branch-orig D^0 &&\n\n... this does that D^0 thing here to makes us wonder if D needs\nunwrapping before using it as a commit (not commit-ish). \n\nIf we did so, we could use $d here instead of D^0.\n\n> +\tgit -C shallow-client fetch --prune --depth=2 \\\n> +\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n> +'\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n\n"},{"id":"361358","messageId":"xmqqmur4ovjt.fsf@gitster-ct.c.googlers.com","threadId":"48882","inReplyTo":"f085eb4f728f5cd102f56b7a90ce9b10fdb59dee.1540245934.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T03:47:18Z","receivedAt":"2018-10-24T03:47:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan, do you see any issues with the use of lookup_commit() in\nthis change wrt lazy clone? I am wondering what happens when the\ncommit in question is at, an immediate parent of, or an immediate\nchild of a promisor object. I _think_ this change won't make it\nworse for two features in playing together, but thought that it\nwould be better to double check.\n\n\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The `prune_shallow()` function wants a full reachability check to be\n> completed before it goes to work, to ensure that all unreachable entries\n> are removed from the shallow file.\n>\n> However, in the upcoming patch we do not even want to go that far. We\n> really only need to remove entries corresponding to pruned commits, i.e.\n> to commits that no longer exist.\n>\n> Let's support that use case.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/prune.c | 2 +-\n> commit.h | 2 +-\n> shallow.c | 22 +++++++++++++++++-----\n> 3 files changed, 19 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 41230f821..6d6ab6cf1 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n> \tfree(s);\n> \n> \tif (is_repository_shallow(the_repository))\n> -\t\tprune_shallow(show_only);\n> +\t\tprune_shallow(show_only, 0);\n> \n> \treturn 0;\n> }\n> diff --git a/commit.h b/commit.h\n> index 1d260d62f..ff34447ab 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,\n> \t\t\t\t\t uint32_t **used,\n> \t\t\t\t\t int *ref_status);\n> extern int delayed_reachability_test(struct shallow_info *si, int c);\n> -extern void prune_shallow(int show_only);\n> +extern void prune_shallow(int show_only, int quick_prune);\n> extern struct trace_key trace_shallow;\n> \n> extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n> diff --git a/shallow.c b/shallow.c\n> index 732e18d54..0a2671bc2 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)\n> \n> #define SEEN_ONLY 1\n> #define VERBOSE 2\n> +#define QUICK_PRUNE 4\n> \n> struct write_shallow_data {\n> \tstruct strbuf *out;\n> @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> \tconst char *hex = oid_to_hex(&graft->oid);\n> \tif (graft->nr_parent != -1)\n> \t\treturn 0;\n> -\tif (data->flags & SEEN_ONLY) {\n> +\tif (data->flags & QUICK_PRUNE) {\n> +\t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n> +\t\tif (!c || parse_commit(c))\n> +\t\t\treturn 0;\n> +\t} else if (data->flags & SEEN_ONLY) {\n> \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n> \t\tif (!c || !(c->object.flags & SEEN)) {\n> \t\t\tif (data->flags & VERBOSE)\n> @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)\n> \n> /*\n> * mark_reachable_objects() should have been run prior to this and all\n> - * reachable commits marked as \"SEEN\".\n> + * reachable commits marked as \"SEEN\", except when quick_prune is non-zero,\n> + * in which case lines are excised from the shallow file if they refer to\n> + * commits that do not exist (any longer).\n> */\n> -void prune_shallow(int show_only)\n> +void prune_shallow(int show_only, int quick_prune)\n> {\n> \tstruct lock_file shallow_lock = LOCK_INIT;\n> \tstruct strbuf sb = STRBUF_INIT;\n> +\tunsigned flags = SEEN_ONLY;\n> \tint fd;\n> \n> +\tif (quick_prune)\n> +\t\tflags |= QUICK_PRUNE;\n> +\n> \tif (show_only) {\n> -\t\twrite_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);\n> +\t\tflags |= VERBOSE;\n> +\t\twrite_shallow_commits_1(&sb, 0, NULL, flags);\n> \t\tstrbuf_release(&sb);\n> \t\treturn;\n> \t}\n> @@ -388,7 +400,7 @@ void prune_shallow(int show_only)\n> \t\t\t\t git_path_shallow(the_repository),\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \tcheck_shallow_file_for_update(the_repository);\n> -\tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n> +\tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n> \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n> \t\t\tdie_errno(\"failed to write to %s\",\n> \t\t\t\t get_lock_file_path(&shallow_lock));\n"},{"id":"361359","messageId":"xmqqin1sov48.fsf@gitster-ct.c.googlers.com","threadId":"48882","inReplyTo":"1f9ff57d52a72e3795ac4a924e23a64b91b1f83e.1540245934.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T03:56:39Z","receivedAt":"2018-10-24T03:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index c6a7943d5..9217fc832 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\tif (!po_args.quiet && isatty(2))\n> \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> \t\tprune_packed_objects(opts);\n> +\n> +\t\tif (!keep_unreachable &&\n> +\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n> +\t\t unpack_unreachable) &&\n> +\t\t is_repository_shallow(the_repository))\n> +\t\t\tprune_shallow(0, 1);\n\nThe logic looks correct (and the reasoning behind it, which was\nexplained in the log message, was sound). prune_shallow(0, 1)\nhowever is not easily understandable.\n\nThere are only two callsites of this function after these three\npatches, and the areas of the code that have these calls are in\nrelatively quiescent parts in the whole tree, so it shouldn't be too\ndistracting to do an update to make the function take a flag word,\nso that we can make callsites more immediately obvious, i.e.\n\n\tprune_shallow(PRUNE_SHALLOW_QUICK)\n\nIt of course can be left as a low-hanging fruit loose-end.\n\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index 2d0031703..777c9d1dc 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -186,7 +186,7 @@ EOF\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_failure '.git/shallow is edited by repack' '\n> +test_expect_success '.git/shallow is edited by repack' '\n> \tgit init shallow-server &&\n> \ttest_commit -C shallow-server A &&\n> \ttest_commit -C shallow-server B &&\n"},{"id":"361383","messageId":"nycvar.QRO.7.76.6.1810240941230.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"xmqqmur4ovjt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T08:01:16Z","receivedAt":"2018-10-24T08:01:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Jonathan,\n\nOn Wed, 24 Oct 2018, Junio C Hamano wrote:\n\n> Jonathan, do you see any issues with the use of lookup_commit() in\n> this change wrt lazy clone? I am wondering what happens when the\n> commit in question is at, an immediate parent of, or an immediate\n> child of a promisor object. I _think_ this change won't make it\n> worse for two features in playing together, but thought that it\n> would be better to double check.\n\nGood point.\n\nInstinctively, I would say that no promised object can be a shallow\ncommit. The entire idea of a shallow commit is that it *is* present, but\nnone of its parents.\n\nAlso, I would claim that the shallow feature does not make sense with lazy\nclones, as lazy clones offer a superset of shallow clone's functionality.\n\nHowever, I am curious whether there is a better way to check for the\npresence of a local commit? Do we have an API function for that, that I\nmissed? (I do not really want to parse the commit, after all, just verify\nthat it is not pruned.)\n\nThanks,\nDscho\n\n> \n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > The `prune_shallow()` function wants a full reachability check to be\n> > completed before it goes to work, to ensure that all unreachable entries\n> > are removed from the shallow file.\n> >\n> > However, in the upcoming patch we do not even want to go that far. We\n> > really only need to remove entries corresponding to pruned commits, i.e.\n> > to commits that no longer exist.\n> >\n> > Let's support that use case.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/prune.c | 2 +-\n> > commit.h | 2 +-\n> > shallow.c | 22 +++++++++++++++++-----\n> > 3 files changed, 19 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/builtin/prune.c b/builtin/prune.c\n> > index 41230f821..6d6ab6cf1 100644\n> > --- a/builtin/prune.c\n> > +++ b/builtin/prune.c\n> > @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n> > \tfree(s);\n> > \n> > \tif (is_repository_shallow(the_repository))\n> > -\t\tprune_shallow(show_only);\n> > +\t\tprune_shallow(show_only, 0);\n> > \n> > \treturn 0;\n> > }\n> > diff --git a/commit.h b/commit.h\n> > index 1d260d62f..ff34447ab 100644\n> > --- a/commit.h\n> > +++ b/commit.h\n> > @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,\n> > \t\t\t\t\t uint32_t **used,\n> > \t\t\t\t\t int *ref_status);\n> > extern int delayed_reachability_test(struct shallow_info *si, int c);\n> > -extern void prune_shallow(int show_only);\n> > +extern void prune_shallow(int show_only, int quick_prune);\n> > extern struct trace_key trace_shallow;\n> > \n> > extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n> > diff --git a/shallow.c b/shallow.c\n> > index 732e18d54..0a2671bc2 100644\n> > --- a/shallow.c\n> > +++ b/shallow.c\n> > @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)\n> > \n> > #define SEEN_ONLY 1\n> > #define VERBOSE 2\n> > +#define QUICK_PRUNE 4\n> > \n> > struct write_shallow_data {\n> > \tstruct strbuf *out;\n> > @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> > \tconst char *hex = oid_to_hex(&graft->oid);\n> > \tif (graft->nr_parent != -1)\n> > \t\treturn 0;\n> > -\tif (data->flags & SEEN_ONLY) {\n> > +\tif (data->flags & QUICK_PRUNE) {\n> > +\t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n> > +\t\tif (!c || parse_commit(c))\n> > +\t\t\treturn 0;\n> > +\t} else if (data->flags & SEEN_ONLY) {\n> > \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n> > \t\tif (!c || !(c->object.flags & SEEN)) {\n> > \t\t\tif (data->flags & VERBOSE)\n> > @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)\n> > \n> > /*\n> > * mark_reachable_objects() should have been run prior to this and all\n> > - * reachable commits marked as \"SEEN\".\n> > + * reachable commits marked as \"SEEN\", except when quick_prune is non-zero,\n> > + * in which case lines are excised from the shallow file if they refer to\n> > + * commits that do not exist (any longer).\n> > */\n> > -void prune_shallow(int show_only)\n> > +void prune_shallow(int show_only, int quick_prune)\n> > {\n> > \tstruct lock_file shallow_lock = LOCK_INIT;\n> > \tstruct strbuf sb = STRBUF_INIT;\n> > +\tunsigned flags = SEEN_ONLY;\n> > \tint fd;\n> > \n> > +\tif (quick_prune)\n> > +\t\tflags |= QUICK_PRUNE;\n> > +\n> > \tif (show_only) {\n> > -\t\twrite_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);\n> > +\t\tflags |= VERBOSE;\n> > +\t\twrite_shallow_commits_1(&sb, 0, NULL, flags);\n> > \t\tstrbuf_release(&sb);\n> > \t\treturn;\n> > \t}\n> > @@ -388,7 +400,7 @@ void prune_shallow(int show_only)\n> > \t\t\t\t git_path_shallow(the_repository),\n> > \t\t\t\t LOCK_DIE_ON_ERROR);\n> > \tcheck_shallow_file_for_update(the_repository);\n> > -\tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n> > +\tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n> > \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n> > \t\t\tdie_errno(\"failed to write to %s\",\n> > \t\t\t\t get_lock_file_path(&shallow_lock));\n> \n"},{"id":"361384","messageId":"nycvar.QRO.7.76.6.1810241001470.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"xmqqin1sov48.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T08:02:50Z","receivedAt":"2018-10-24T08:02:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 24 Oct 2018, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index c6a7943d5..9217fc832 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \t\tif (!po_args.quiet && isatty(2))\n> > \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> > \t\tprune_packed_objects(opts);\n> > +\n> > +\t\tif (!keep_unreachable &&\n> > +\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n> > +\t\t unpack_unreachable) &&\n> > +\t\t is_repository_shallow(the_repository))\n> > +\t\t\tprune_shallow(0, 1);\n> \n> The logic looks correct (and the reasoning behind it, which was\n> explained in the log message, was sound). prune_shallow(0, 1)\n> however is not easily understandable.\n> \n> There are only two callsites of this function after these three\n> patches, and the areas of the code that have these calls are in\n> relatively quiescent parts in the whole tree, so it shouldn't be too\n> distracting to do an update to make the function take a flag word,\n> so that we can make callsites more immediately obvious, i.e.\n> \n> \tprune_shallow(PRUNE_SHALLOW_QUICK)\n> \n> It of course can be left as a low-hanging fruit loose-end.\n\nI almost did that, but then decided not to clutter the previous patch with\nthis change (and global constant).\n\nHaving said that, since you also had this idea, I will make that change.\nIt will read nicer, you are right.\n\nCiao,\nDscho\n\n> \n> > diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> > index 2d0031703..777c9d1dc 100755\n> > --- a/t/t5537-fetch-shallow.sh\n> > +++ b/t/t5537-fetch-shallow.sh\n> > @@ -186,7 +186,7 @@ EOF\n> > \ttest_cmp expect actual\n> > '\n> > \n> > -test_expect_failure '.git/shallow is edited by repack' '\n> > +test_expect_success '.git/shallow is edited by repack' '\n> > \tgit init shallow-server &&\n> > \ttest_commit -C shallow-server A &&\n> > \ttest_commit -C shallow-server B &&\n> \n"},{"id":"361387","messageId":"nycvar.QRO.7.76.6.1810241004440.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"xmqqr2ggovxk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T08:12:09Z","receivedAt":"2018-10-24T08:12:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 24 Oct 2018, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > +...\n> > +\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n> > +\tgit -C shallow-server checkout master &&\n> > +\n> > +...\n> > +\t! grep $d shallow-client/.git/shallow &&\n> \n> We know D (and $d) is not a tag,\n\n\nActually, it is... the `test_commit D` creates that tag, and that is what\nI use here.\n\n> but perhaps the place that assigns to $d (way above) can do the\n> rev-parse of D^0, not D, to make it more clear what is going on,\n> especially given that...\n\n... that the `grep` really wants to test for the absence of the *commit*,\nnot the *tag* in .git/shallow?\n\nYes, you are right ;-)\n\nSo why did my test do the right thing, if it looked at a tag, but did not\ndereference it via ^0? The answer is: the `test_commit` function creates\nlight-weight tags, i.e. no tag objects. And therefore, the $d^0 you found\nbelow, that's just confusing.\n\nI will change it so that the `rev-parse` call uses ^0 (even if it is\ntechnically not necessary), to avoid said confusion.\n\nThanks,\nDscho\n\n> \n> > +\tgit -C shallow-server branch branch-orig D^0 &&\n> \n> ... this does that D^0 thing here to makes us wonder if D needs\n> unwrapping before using it as a commit (not commit-ish). \n> \n> If we did so, we could use $d here instead of D^0.\n> \n> > +\tgit -C shallow-client fetch --prune --depth=2 \\\n> > +\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n> > +'\n> > +\n> > . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> > start_httpd\n> \n> \n"},{"id":"361389","messageId":"nycvar.QRO.7.76.6.1810241033420.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"nycvar.QRO.7.76.6.1810241004440.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T08:38:43Z","receivedAt":"2018-10-24T08:38:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nme again. I was wrong.\n\nOn Wed, 24 Oct 2018, Johannes Schindelin wrote:\n\n> On Wed, 24 Oct 2018, Junio C Hamano wrote:\n> \n> > \"Johannes Schindelin via GitGitGadget\" \n> > writes:\n> > \n> > > +...\n> > > +\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n> > > +\tgit -C shallow-server checkout master &&\n> > > +\n> > > +...\n> > > +\t! grep $d shallow-client/.git/shallow &&\n> > \n> > We know D (and $d) is not a tag,\n> \n> Actually, it is... the `test_commit D` creates that tag, and that is what\n> I use here.\n> \n> > but perhaps the place that assigns to $d (way above) can do the\n> > rev-parse of D^0, not D, to make it more clear what is going on,\n> > especially given that...\n> \n> ... that the `grep` really wants to test for the absence of the *commit*,\n> not the *tag* in .git/shallow?\n> \n> Yes, you are right ;-)\n> \n> So why did my test do the right thing, if it looked at a tag, but did not\n> dereference it via ^0? The answer is: the `test_commit` function creates\n> light-weight tags, i.e. no tag objects. And therefore, the $d^0 you found\n> below, that's just confusing.\n\nWhat I was referring to was the call\n\n\ttest_must_fail git -C shallow-client rev-parse --verify $d^0\n\nHowever, here we *have* to append ^0, otherwise `rev-parse --verify` will\n(and quite confusingly so) *succeed*. I *repeatedly* fall into that trap\nthat `git rev-parse --verify 0000000000000000000000000000000000000000`\nwill succeed. Why? Because that is a valid 40-digit hex string. Not\nbecause the object exists. Because it does not.\n\nSo I managed to confuse myself again into believing that I only need to\nappend ^0 to the earlier rev-parse call, but can remove it from this one,\nwhen in reality, I have to append it to both. In the first case, to avoid\nhaving to think about dereferencing a tag, in the second case, to force\nrev-parse to look for the object.\n\nCiao,\nDscho\n\n> \n> I will change it so that the `rev-parse` call uses ^0 (even if it is\n> technically not necessary), to avoid said confusion.\n> \n> Thanks,\n> Dscho\n> \n> > \n> > > +\tgit -C shallow-server branch branch-orig D^0 &&\n> > \n> > ... this does that D^0 thing here to makes us wonder if D needs\n> > unwrapping before using it as a commit (not commit-ish). \n> > \n> > If we did so, we could use $d here instead of D^0.\n> > \n> > > +\tgit -C shallow-client fetch --prune --depth=2 \\\n> > > +\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n> > > +'\n> > > +\n> > > . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> > > start_httpd\n> > \n> > \n> \n"},{"id":"361422","messageId":"pull.9.v4.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] repack -ad: fix after fetch --prune in a shallow repository","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-24T15:56:08Z","receivedAt":"2018-10-24T15:56:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Under certain circumstances, commits that were reachable can be made\nunreachable, e.g. via git fetch --prune. These commits might have been\npacked already, in which case git repack -adlf will just drop them without\ngiving them the usual grace period before an eventual git prune (via git gc)\nprunes them.\n\nThis is a problem when the shallow file still lists them, which is the\nreason why git prune edits that file. And with the proposed changes, git\nrepack -ad will now do the same.\n\nReported by Alejandro Pauly.\n\nChanges since v3:\n\n * Made the regression test less confusing with regards to tags that might\n need to be dereferenced.\n * Introduced new global constants PRUNE_SHOW_ONLY and PRUNE_QUICK instead\n of extending the signature of prune_shallow() with Boolean parameters.\n * While at it, renamed the file-local constant from QUICK_PRUNE to QUICK.\n * Replaced the lookup_commit() && parse_commit() cadence (that wants to\n test for the existence of a commit) to has_object_file(), for ease of\n readability, and also to make it more obvious how to add a call to \n is_promisor_object() if we ever figure out that that would be necessary.\n\nChanges since v2:\n\n * Fixed a typo in the last commit message.\n * Added an explanation to the last commit message why we do not simply skip\n non-existing shallow commits at fetch time.\n * Introduced a new, \"quick prune\" mode where prune_shallow() does not try\n to drop unreachable commits, but only non-existing ones.\n * Rebased to current master because there were too many merge conflicts for\n my liking otherwise.\n\nChanges since v1:\n\n * Also trigger prune_shallow() when --unpack-unreachable= was\n passed to git repack.\n * No need to trigger prune_shallow() when git repack was called with -k.\n\nJohannes Schindelin (3):\n repack: point out a bug handling stale shallow info\n shallow: offer to prune only non-existing entries\n repack -ad: prune the list of shallow commits\n\n builtin/prune.c | 2 +-\n builtin/repack.c | 6 ++++++\n commit.h | 4 +++-\n shallow.c | 23 +++++++++++++++++------\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 54 insertions(+), 8 deletions(-)\n\n\nbase-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/9\n\nRange-diff vs v3:\n\n 1: ed8559b91 ! 1: d9720bd25 repack: point out a bug handling stale shallow info\n @@ -29,7 +29,7 @@\n +\ttest_commit -C shallow-server C &&\n +\ttest_commit -C shallow-server E &&\n +\ttest_commit -C shallow-server D &&\n -+\td=\"$(git -C shallow-server rev-parse --verify D)\" &&\n ++\td=\"$(git -C shallow-server rev-parse --verify D^0)\" &&\n +\tgit -C shallow-server checkout master &&\n +\n +\tgit clone --depth=1 --no-tags --no-single-branch \\\n @@ -43,7 +43,7 @@\n +\ttest_must_fail git -C shallow-client rev-parse --verify $d^0 &&\n +\t! grep $d shallow-client/.git/shallow &&\n +\n -+\tgit -C shallow-server branch branch-orig D^0 &&\n ++\tgit -C shallow-server branch branch-orig $d &&\n +\tgit -C shallow-client fetch --prune --depth=2 \\\n +\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n +'\n 2: f085eb4f7 ! 2: 18308c13e shallow: offer to prune only non-existing entries\n @@ -12,6 +12,10 @@\n \n Let's support that use case.\n \n + Rather than extending the signature of `prune_shallow()` to accept\n + another Boolean, let's turn it into a bit field and declare constants,\n + for readability.\n +\n Signed-off-by: Johannes Schindelin \n \n diff --git a/builtin/prune.c b/builtin/prune.c\n @@ -22,7 +26,7 @@\n \n \tif (is_repository_shallow(the_repository))\n -\t\tprune_shallow(show_only);\n -+\t\tprune_shallow(show_only, 0);\n ++\t\tprune_shallow(show_only ? PRUNE_SHOW_ONLY : 0);\n \n \treturn 0;\n }\n @@ -35,7 +39,9 @@\n \t\t\t\t\t int *ref_status);\n extern int delayed_reachability_test(struct shallow_info *si, int c);\n -extern void prune_shallow(int show_only);\n -+extern void prune_shallow(int show_only, int quick_prune);\n ++#define PRUNE_SHOW_ONLY 1\n ++#define PRUNE_QUICK 2\n ++extern void prune_shallow(unsigned options);\n extern struct trace_key trace_shallow;\n \n extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n @@ -47,7 +53,7 @@\n \n #define SEEN_ONLY 1\n #define VERBOSE 2\n -+#define QUICK_PRUNE 4\n ++#define QUICK 4\n \n struct write_shallow_data {\n \tstruct strbuf *out;\n @@ -56,9 +62,8 @@\n \tif (graft->nr_parent != -1)\n \t\treturn 0;\n -\tif (data->flags & SEEN_ONLY) {\n -+\tif (data->flags & QUICK_PRUNE) {\n -+\t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n -+\t\tif (!c || parse_commit(c))\n ++\tif (data->flags & QUICK) {\n ++\t\tif (!has_object_file(&graft->oid))\n +\t\t\treturn 0;\n +\t} else if (data->flags & SEEN_ONLY) {\n \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n @@ -74,18 +79,19 @@\n + * commits that do not exist (any longer).\n */\n -void prune_shallow(int show_only)\n -+void prune_shallow(int show_only, int quick_prune)\n ++void prune_shallow(unsigned options)\n {\n \tstruct lock_file shallow_lock = LOCK_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n +\tunsigned flags = SEEN_ONLY;\n \tint fd;\n \n -+\tif (quick_prune)\n -+\t\tflags |= QUICK_PRUNE;\n -+\n - \tif (show_only) {\n +-\tif (show_only) {\n -\t\twrite_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);\n ++\tif (options & PRUNE_QUICK)\n ++\t\tflags |= QUICK;\n ++\n ++\tif (options & PRUNE_SHOW_ONLY) {\n +\t\tflags |= VERBOSE;\n +\t\twrite_shallow_commits_1(&sb, 0, NULL, flags);\n \t\tstrbuf_release(&sb);\n 3: 1f9ff57d5 ! 3: 2858bc886 repack -ad: prune the list of shallow commits\n @@ -52,7 +52,7 @@\n +\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n +\t\t unpack_unreachable) &&\n +\t\t is_repository_shallow(the_repository))\n -+\t\t\tprune_shallow(0, 1);\n ++\t\t\tprune_shallow(PRUNE_QUICK);\n \t}\n \n \tif (!no_update_server_info)\n\n-- \ngitgitgadget\n"},{"id":"361423","messageId":"d9720bd25145c6e2b1b1a867b4eb49cdd779803d.1540396567.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] repack: point out a bug handling stale shallow info","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-24T15:56:10Z","receivedAt":"2018-10-24T15:56:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nA `git fetch --prune` can turn previously-reachable objects unreachable,\neven commits that are in the `shallow` list. A subsequent `git repack\n-ad` will then unceremoniously drop those unreachable commits, and the\n`shallow` list will become stale. This means that when we try to fetch\nwith a larger `--depth` the next time, we may end up with:\n\n\tfatal: error in object: unshallow \n\nReported by Alejandro Pauly.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 7045685e2..686fdc928 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,6 +186,33 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '.git/shallow is edited by repack' '\n+\tgit init shallow-server &&\n+\ttest_commit -C shallow-server A &&\n+\ttest_commit -C shallow-server B &&\n+\tgit -C shallow-server checkout -b branch &&\n+\ttest_commit -C shallow-server C &&\n+\ttest_commit -C shallow-server E &&\n+\ttest_commit -C shallow-server D &&\n+\td=\"$(git -C shallow-server rev-parse --verify D^0)\" &&\n+\tgit -C shallow-server checkout master &&\n+\n+\tgit clone --depth=1 --no-tags --no-single-branch \\\n+\t\t\"file://$PWD/shallow-server\" shallow-client &&\n+\n+\t: now remove the branch and fetch with prune &&\n+\tgit -C shallow-server branch -D branch &&\n+\tgit -C shallow-client fetch --prune --depth=1 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\" &&\n+\tgit -C shallow-client repack -adfl &&\n+\ttest_must_fail git -C shallow-client rev-parse --verify $d^0 &&\n+\t! grep $d shallow-client/.git/shallow &&\n+\n+\tgit -C shallow-server branch branch-orig $d &&\n+\tgit -C shallow-client fetch --prune --depth=2 \\\n+\t\torigin \"+refs/heads/*:refs/remotes/origin/*\"\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \ngitgitgadget\n\n"},{"id":"361424","messageId":"2858bc886ce2c687092f4b93b211ebcdf6281a22.1540396567.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] repack -ad: prune the list of shallow commits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-24T15:56:13Z","receivedAt":"2018-10-24T15:56:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\n`git repack` can drop unreachable commits without further warning,\nmaking the corresponding entries in `.git/shallow` invalid, which causes\nserious problems when deepening the branches.\n\nOne scenario where unreachable commits are dropped by `git repack` is\nwhen a `git fetch --prune` (or even a `git fetch` when a ref was\nforce-pushed in the meantime) can make a commit unreachable that was\nreachable before.\n\nTherefore it is not safe to assume that a `git repack -adlf` will keep\nunreachable commits alone (under the assumption that they had not been\npacked in the first place, which is an assumption at least some of Git's\ncode seems to make).\n\nThis is particularly important to keep in mind when looking at the\n`.git/shallow` file: if any commits listed in that file become\nunreachable, it is not a problem, but if they go missing, it *is* a\nproblem. One symptom of this problem is that a deepening fetch may now\nfail with\n\n\tfatal: error in object: unshallow \n\nTo avoid this problem, let's prune the shallow list in `git repack` when\nthe `-d` option is passed, unless `-A` is passed, too (which would force\nthe now-unreachable objects to be turned into loose objects instead of\nbeing deleted). Additionally, we also need to take `--keep-reachable`\nand `--unpack-unreachable=` into account.\n\nNote: an alternative solution discussed during the review of this patch\nwas to teach `git fetch` to simply ignore entries in .git/shallow if the\ncorresponding commits do not exist locally. A quick test, however,\nrevealed that the .git/shallow file is written during a shallow *clone*,\nin which case the commits do not exist, either, but the \"shallow\" line\n*does* need to be sent. Therefore, this approach would be a lot more\nfinicky than the approach presented by the this patch.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/repack.c | 6 ++++++\n t/t5537-fetch-shallow.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c6a7943d5..acd43c75d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (!po_args.quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n+\n+\t\tif (!keep_unreachable &&\n+\t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\n+\t\t unpack_unreachable) &&\n+\t\t is_repository_shallow(the_repository))\n+\t\t\tprune_shallow(PRUNE_QUICK);\n \t}\n \n \tif (!no_update_server_info)\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 686fdc928..6faf17e17 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,7 +186,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '.git/shallow is edited by repack' '\n+test_expect_success '.git/shallow is edited by repack' '\n \tgit init shallow-server &&\n \ttest_commit -C shallow-server A &&\n \ttest_commit -C shallow-server B &&\n-- \ngitgitgadget\n"},{"id":"361425","messageId":"18308c13eee4568dbda39fbf6567666bf4b98eeb.1540396567.git.gitgitgadget@gmail.com","threadId":"48882","inReplyTo":"pull.9.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] shallow: offer to prune only non-existing entries","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-24T15:56:12Z","receivedAt":"2018-10-24T15:56:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `prune_shallow()` function wants a full reachability check to be\ncompleted before it goes to work, to ensure that all unreachable entries\nare removed from the shallow file.\n\nHowever, in the upcoming patch we do not even want to go that far. We\nreally only need to remove entries corresponding to pruned commits, i.e.\nto commits that no longer exist.\n\nLet's support that use case.\n\nRather than extending the signature of `prune_shallow()` to accept\nanother Boolean, let's turn it into a bit field and declare constants,\nfor readability.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/prune.c | 2 +-\n commit.h | 4 +++-\n shallow.c | 23 +++++++++++++++++------\n 3 files changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 41230f821..1ec9ddd75 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tfree(s);\n \n \tif (is_repository_shallow(the_repository))\n-\t\tprune_shallow(show_only);\n+\t\tprune_shallow(show_only ? PRUNE_SHOW_ONLY : 0);\n \n \treturn 0;\n }\ndiff --git a/commit.h b/commit.h\nindex 1d260d62f..b80d6fdb5 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -249,7 +249,9 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,\n \t\t\t\t\t uint32_t **used,\n \t\t\t\t\t int *ref_status);\n extern int delayed_reachability_test(struct shallow_info *si, int c);\n-extern void prune_shallow(int show_only);\n+#define PRUNE_SHOW_ONLY 1\n+#define PRUNE_QUICK 2\n+extern void prune_shallow(unsigned options);\n extern struct trace_key trace_shallow;\n \n extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\ndiff --git a/shallow.c b/shallow.c\nindex 732e18d54..02fdbfc55 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)\n \n #define SEEN_ONLY 1\n #define VERBOSE 2\n+#define QUICK 4\n \n struct write_shallow_data {\n \tstruct strbuf *out;\n@@ -261,7 +262,10 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \tconst char *hex = oid_to_hex(&graft->oid);\n \tif (graft->nr_parent != -1)\n \t\treturn 0;\n-\tif (data->flags & SEEN_ONLY) {\n+\tif (data->flags & QUICK) {\n+\t\tif (!has_object_file(&graft->oid))\n+\t\t\treturn 0;\n+\t} else if (data->flags & SEEN_ONLY) {\n \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n \t\tif (!c || !(c->object.flags & SEEN)) {\n \t\t\tif (data->flags & VERBOSE)\n@@ -371,16 +375,23 @@ void advertise_shallow_grafts(int fd)\n \n /*\n * mark_reachable_objects() should have been run prior to this and all\n- * reachable commits marked as \"SEEN\".\n+ * reachable commits marked as \"SEEN\", except when quick_prune is non-zero,\n+ * in which case lines are excised from the shallow file if they refer to\n+ * commits that do not exist (any longer).\n */\n-void prune_shallow(int show_only)\n+void prune_shallow(unsigned options)\n {\n \tstruct lock_file shallow_lock = LOCK_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tunsigned flags = SEEN_ONLY;\n \tint fd;\n \n-\tif (show_only) {\n-\t\twrite_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);\n+\tif (options & PRUNE_QUICK)\n+\t\tflags |= QUICK;\n+\n+\tif (options & PRUNE_SHOW_ONLY) {\n+\t\tflags |= VERBOSE;\n+\t\twrite_shallow_commits_1(&sb, 0, NULL, flags);\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n@@ -388,7 +399,7 @@ void prune_shallow(int show_only)\n \t\t\t\t git_path_shallow(the_repository),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update(the_repository);\n-\tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n+\tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_lock_file_path(&shallow_lock));\n-- \ngitgitgadget\n\n"},{"id":"361426","messageId":"nycvar.QRO.7.76.6.1810241756390.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"nycvar.QRO.7.76.6.1810240941230.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T15:56:45Z","receivedAt":"2018-10-24T15:56:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 24 Oct 2018, Johannes Schindelin wrote:\n\n> On Wed, 24 Oct 2018, Junio C Hamano wrote:\n> \n> > Jonathan, do you see any issues with the use of lookup_commit() in\n> > this change wrt lazy clone? I am wondering what happens when the\n> > commit in question is at, an immediate parent of, or an immediate\n> > child of a promisor object. I _think_ this change won't make it\n> > worse for two features in playing together, but thought that it\n> > would be better to double check.\n> \n> Good point.\n> \n> Instinctively, I would say that no promised object can be a shallow\n> commit. The entire idea of a shallow commit is that it *is* present, but\n> none of its parents.\n> \n> Also, I would claim that the shallow feature does not make sense with lazy\n> clones, as lazy clones offer a superset of shallow clone's functionality.\n> \n> However, I am curious whether there is a better way to check for the\n> presence of a local commit? Do we have an API function for that, that I\n> missed? (I do not really want to parse the commit, after all, just verify\n> that it is not pruned.)\n\nOkay, I looked around a bit. It seems that there is an\n`is_promisor_object(oid)` function in `pu` to see whether an object was\npromised. If need be (and I am still not convinced that there is a need),\nthen we can always add a call to that function to the condition.\n\nComing back to my question whether there is a better way to check for the\npresence of a local commit, I figured that I can use `has_object_file()`\ninstead of looking up and parsing the commit, as this code does not really\nneed to verify that the shallow entry refers to a commit, but only that it\nrefers to a local object.\n\nSo I'll send another iteration shortly, with this diff applied to 2/3:\n\n-- snip --\ndiff --git a/shallow.c b/shallow.c\nindex 9c3faa8af243..02fdbfc554c4 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -263,8 +263,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \tif (graft->nr_parent != -1)\n \t\treturn 0;\n \tif (data->flags & QUICK) {\n-\t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n-\t\tif (!c || parse_commit(c))\n+\t\tif (!has_object_file(&graft->oid))\n \t\t\treturn 0;\n \t} else if (data->flags & SEEN_ONLY) {\n \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n--snap --\n\nCiao,\nDscho\n\n> \n> > \n> > \"Johannes Schindelin via GitGitGadget\" \n> > writes:\n> > \n> > > From: Johannes Schindelin \n> > >\n> > > The `prune_shallow()` function wants a full reachability check to be\n> > > completed before it goes to work, to ensure that all unreachable entries\n> > > are removed from the shallow file.\n> > >\n> > > However, in the upcoming patch we do not even want to go that far. We\n> > > really only need to remove entries corresponding to pruned commits, i.e.\n> > > to commits that no longer exist.\n> > >\n> > > Let's support that use case.\n> > >\n> > > Signed-off-by: Johannes Schindelin \n> > > ---\n> > > builtin/prune.c | 2 +-\n> > > commit.h | 2 +-\n> > > shallow.c | 22 +++++++++++++++++-----\n> > > 3 files changed, 19 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/builtin/prune.c b/builtin/prune.c\n> > > index 41230f821..6d6ab6cf1 100644\n> > > --- a/builtin/prune.c\n> > > +++ b/builtin/prune.c\n> > > @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n> > > \tfree(s);\n> > > \n> > > \tif (is_repository_shallow(the_repository))\n> > > -\t\tprune_shallow(show_only);\n> > > +\t\tprune_shallow(show_only, 0);\n> > > \n> > > \treturn 0;\n> > > }\n> > > diff --git a/commit.h b/commit.h\n> > > index 1d260d62f..ff34447ab 100644\n> > > --- a/commit.h\n> > > +++ b/commit.h\n> > > @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,\n> > > \t\t\t\t\t uint32_t **used,\n> > > \t\t\t\t\t int *ref_status);\n> > > extern int delayed_reachability_test(struct shallow_info *si, int c);\n> > > -extern void prune_shallow(int show_only);\n> > > +extern void prune_shallow(int show_only, int quick_prune);\n> > > extern struct trace_key trace_shallow;\n> > > \n> > > extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n> > > diff --git a/shallow.c b/shallow.c\n> > > index 732e18d54..0a2671bc2 100644\n> > > --- a/shallow.c\n> > > +++ b/shallow.c\n> > > @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)\n> > > \n> > > #define SEEN_ONLY 1\n> > > #define VERBOSE 2\n> > > +#define QUICK_PRUNE 4\n> > > \n> > > struct write_shallow_data {\n> > > \tstruct strbuf *out;\n> > > @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> > > \tconst char *hex = oid_to_hex(&graft->oid);\n> > > \tif (graft->nr_parent != -1)\n> > > \t\treturn 0;\n> > > -\tif (data->flags & SEEN_ONLY) {\n> > > +\tif (data->flags & QUICK_PRUNE) {\n> > > +\t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n> > > +\t\tif (!c || parse_commit(c))\n> > > +\t\t\treturn 0;\n> > > +\t} else if (data->flags & SEEN_ONLY) {\n> > > \t\tstruct commit *c = lookup_commit(the_repository, &graft->oid);\n> > > \t\tif (!c || !(c->object.flags & SEEN)) {\n> > > \t\t\tif (data->flags & VERBOSE)\n> > > @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)\n> > > \n> > > /*\n> > > * mark_reachable_objects() should have been run prior to this and all\n> > > - * reachable commits marked as \"SEEN\".\n> > > + * reachable commits marked as \"SEEN\", except when quick_prune is non-zero,\n> > > + * in which case lines are excised from the shallow file if they refer to\n> > > + * commits that do not exist (any longer).\n> > > */\n> > > -void prune_shallow(int show_only)\n> > > +void prune_shallow(int show_only, int quick_prune)\n> > > {\n> > > \tstruct lock_file shallow_lock = LOCK_INIT;\n> > > \tstruct strbuf sb = STRBUF_INIT;\n> > > +\tunsigned flags = SEEN_ONLY;\n> > > \tint fd;\n> > > \n> > > +\tif (quick_prune)\n> > > +\t\tflags |= QUICK_PRUNE;\n> > > +\n> > > \tif (show_only) {\n> > > -\t\twrite_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);\n> > > +\t\tflags |= VERBOSE;\n> > > +\t\twrite_shallow_commits_1(&sb, 0, NULL, flags);\n> > > \t\tstrbuf_release(&sb);\n> > > \t\treturn;\n> > > \t}\n> > > @@ -388,7 +400,7 @@ void prune_shallow(int show_only)\n> > > \t\t\t\t git_path_shallow(the_repository),\n> > > \t\t\t\t LOCK_DIE_ON_ERROR);\n> > > \tcheck_shallow_file_for_update(the_repository);\n> > > -\tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n> > > +\tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n> > > \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n> > > \t\t\tdie_errno(\"failed to write to %s\",\n> > > \t\t\t\t get_lock_file_path(&shallow_lock));\n> > \n> \n"},{"id":"361547","messageId":"20181025185459.206127-1-jonathantanmy@google.com","threadId":"48882","inReplyTo":"nycvar.QRO.7.76.6.1810241756390.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-25T18:54:59Z","receivedAt":"2018-10-25T18:55:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, 24 Oct 2018, Johannes Schindelin wrote:\n> \n> > On Wed, 24 Oct 2018, Junio C Hamano wrote:\n> > \n> > > Jonathan, do you see any issues with the use of lookup_commit() in\n> > > this change wrt lazy clone? I am wondering what happens when the\n> > > commit in question is at, an immediate parent of, or an immediate\n> > > child of a promisor object. I _think_ this change won't make it\n> > > worse for two features in playing together, but thought that it\n> > > would be better to double check.\n> > \n> > Good point.\n> > \n> > Instinctively, I would say that no promised object can be a shallow\n> > commit. The entire idea of a shallow commit is that it *is* present, but\n> > none of its parents.\n\nI'm envisioning a client repo with a single branch, cloned both with\n--depth=1 and with --filter=, that has just fetched to the same\nbranch also with --depth=1 resulting in a fast-forward from A to B.\n\nIf A is B's parent, then A would be known to be promised. (Incidentally,\nthe problem is greater in current Git, because for performance reasons,\nwe do not check promisor status when lazily fetching - so it doesn't\nmatter here whether an object is known to be promised or not.)\n\nWhen pruning shallow and checking the existence of A, this would trigger\na fetch for A, which would download all commits and trees reachable from\nit.\n\nIt sounds safer to me to use the fast approach in this patch when the\nrepository is not partial, and stick to the slow approach when it is.\n\n> > However, I am curious whether there is a better way to check for the\n> > presence of a local commit? Do we have an API function for that, that I\n> > missed? (I do not really want to parse the commit, after all, just verify\n> > that it is not pruned.)\n> \n> Okay, I looked around a bit. It seems that there is an\n> `is_promisor_object(oid)` function in `pu` to see whether an object was\n> promised. If need be (and I am still not convinced that there is a need),\n> then we can always add a call to that function to the condition.\n\nI don't think there is a need for is_promisor_object() either - as\ndescribed above, it doesn't completely solve the problem.\n\n> Coming back to my question whether there is a better way to check for the\n> presence of a local commit, I figured that I can use `has_object_file()`\n> instead of looking up and parsing the commit, as this code does not really\n> need to verify that the shallow entry refers to a commit, but only that it\n> refers to a local object.\n\nNote that has_object_file() also triggers the lazy fetch if needed, but\nI agree that it's better because you don't really need to parse the\ncommit.\n\nThere is the possibility of just checking for loose objects (which does\nnot trigger any lazy fetches), which works for builtin/prune since it\nonly prunes loose objects, but doesn't work in the general case, I\nguess.\n"},{"id":"361608","messageId":"nycvar.QRO.7.76.6.1810260956230.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"20181025185459.206127-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-26T07:59:46Z","receivedAt":"2018-10-26T07:59:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Thu, 25 Oct 2018, Jonathan Tan wrote:\n\n> > On Wed, 24 Oct 2018, Johannes Schindelin wrote:\n> > \n> > Coming back to my question whether there is a better way to check for\n> > the presence of a local commit, I figured that I can use\n> > `has_object_file()` instead of looking up and parsing the commit, as\n> > this code does not really need to verify that the shallow entry refers\n> > to a commit, but only that it refers to a local object.\n> \n> Note that has_object_file() also triggers the lazy fetch if needed, but\n> I agree that it's better because you don't really need to parse the\n> commit.\n\nThanks for confirming.\n\nSo even better would be to use `is_promisor_object(oid) ||\nhas_object_file(oid)`, right?\n\nThis is something that is probably not even needed: as I mentioned, the\nshallow commits are *expected* to be local. It should not ever happen that\nthey are fetched.\n\n> There is the possibility of just checking for loose objects (which does\n> not trigger any lazy fetches), which works for builtin/prune since it\n> only prunes loose objects, but doesn't work in the general case, I\n> guess.\n\nIn the test case I added, the commit object is actually packed. And then,\nbecause it became unreachable, it is dropped.\n\nIf I only checked for loose objects here, the shallow line would already\nbe removed when the commit gets packed, which would be the wrong thing to\ndo.\n\nIn short: thank you for confirming that the current version of the patch\nseems to be the best we can do for now.\n\nCiao,\nDscho\n"},{"id":"361647","messageId":"20181026204914.134946-1-jonathantanmy@google.com","threadId":"48882","inReplyTo":"nycvar.QRO.7.76.6.1810260956230.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-26T20:49:14Z","receivedAt":"2018-10-26T20:49:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Thanks for confirming.\n> \n> So even better would be to use `is_promisor_object(oid) ||\n> has_object_file(oid)`, right?\n> \n> This is something that is probably not even needed: as I mentioned, the\n> shallow commits are *expected* to be local. It should not ever happen that\n> they are fetched.\n\nThat would help, but I don't think it would help in the \"fast-forward\nfrom A to B where A is B's parent\" case I describe in [1].\n\nMy suggestion was:\n\n> It sounds safer to me to use the fast approach in this patch when the\n> repository is not partial, and stick to the slow approach when it is.\n\nwhich can be done by replacing \"prune_shallow(0, 1)\" in patch 3 with\n\"prune_shallow(0, !repository_format_partial_clone)\", possibly with a comment\nthat the fast method checks object existence for each shallow line directly,\nwhich is undesirable when the repository is a partial clone.\n(repository_format_partial_clone is non-NULL with the name of the promisor\nremote if the repository is a partial clone, and NULL otherwise).\n\n[1] https://public-inbox.org/git/20181025185459.206127-1-jonathantanmy@google.com/\n"},{"id":"361943","messageId":"nycvar.QRO.7.76.6.1810292047130.4546@tvgsbejvaqbjf.bet","threadId":"48882","inReplyTo":"20181026204914.134946-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-29T20:45:15Z","receivedAt":"2018-10-29T20:45:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Fri, 26 Oct 2018, Jonathan Tan wrote:\n\n> > So even better would be to use `is_promisor_object(oid) ||\n> > has_object_file(oid)`, right?\n> > \n> > This is something that is probably not even needed: as I mentioned,\n> > the shallow commits are *expected* to be local. It should not ever\n> > happen that they are fetched.\n> \n> That would help, but I don't think it would help in the \"fast-forward\n> from A to B where A is B's parent\" case I describe in [1].\n\nI don't think that that analysis is correct. It assumes that there could\nbe a promised commit that is also listed as shallow. I do not think that\nis a possible scenario.\n\nAnd even if it were, why would asking for a promised commit object\ndownload not only that object but also all of its trees and all of its\nancestors? That's not how I understood the idea of the lazy clone: I\nunderstood promised objects to be downloaded on demand, individually.\n\n> My suggestion was:\n> \n> > It sounds safer to me to use the fast approach in this patch when the\n> > repository is not partial, and stick to the slow approach when it is.\n> \n> which can be done by replacing \"prune_shallow(0, 1)\" in patch 3 with\n> \"prune_shallow(0, !repository_format_partial_clone)\", possibly with a comment\n> that the fast method checks object existence for each shallow line directly,\n> which is undesirable when the repository is a partial clone.\n\nI am afraid that that would re-introduce the bug pointed out by Peff: you\n*really* would need to traverse all reachable commits to use the non-fast\npruning method. And we simply don't.\n\nCiao,\nDscho\n\n> (repository_format_partial_clone is non-NULL with the name of the promisor\n> remote if the repository is a partial clone, and NULL otherwise).\n> \n> [1] https://public-inbox.org/git/20181025185459.206127-1-jonathantanmy@google.com/\n> \n"}]}