{"thread":{"id":"43253","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","startedAt":"2006-10-29T09:37:54Z","lastAt":"2006-10-31T02:17:39Z","messageCount":17,"participants":["Junio C Hamano","Nicolas Pitre","Shawn Pearce","Jan Harkes"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"297917","messageId":"20061029093754.GD3847@spearce.org","threadId":"43253","inReplyTo":null,"subject":"[PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-29T09:37:54Z","receivedAt":"2006-10-29T09:37:54Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"During `git repack -a -d` only repack objects which are loose or\nwhich reside in an active (a non-kept) pack.  This allows the user\nto keep large packs as-is without continuous repacking and can be\nvery helpful on large repositories.  It should also help us resolve\na race condition between `git repack -a -d` and the new pack store\nfunctionality in `git-receive-pack`.\n\nKept packs are those which have a corresponding .keep file in\n$GIT_OBJECT_DIRECTORY/pack.  That is pack-X.pack will be kept\n(not repacked and not deleted) if pack-X.keep exists in the same\ndirectory when `git repack -a -d` starts.\n\nCurrently this feature is not documented and there is no user\ninterface to keep an existing pack.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n git-repack.sh |   27 +++++++++++++++++----------\n 1 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 17e2452..f150a55 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -45,11 +45,19 @@ case \",$all_into_one,\" in\n \targs='--unpacked --incremental'\n \t;;\n ,t,)\n-\targs=\n-\n-\t# Redundancy check in all-into-one case is trivial.\n-\texisting=`test -d \"$PACKDIR\" && cd \"$PACKDIR\" && \\\n-\t    find . -type f \\( -name '*.pack' -o -name '*.idx' \\) -print`\n+\tif [ -d \"$PACKDIR\" ]; then\n+\t\tfor e in `cd \"$PACKDIR\" && find . -type f -name '*.pack' \\\n+\t\t\t| sed -e 's/^\\.\\///' -e 's/\\.pack$//'`\n+\t\tdo\n+\t\t\tif [ -e \"$PACKDIR/$e.keep\" ]; then\n+\t\t\t\t: keep\n+\t\t\telse\n+\t\t\t\targs=\"$args --unpacked=$e.pack\"\n+\t\t\t\texisting=\"$existing $e\"\n+\t\t\tfi\n+\t\tdone\n+\tfi\n+\t[ -z \"$args\" ] && args='--unpacked --incremental'\n \t;;\n esac\n \n@@ -86,17 +94,16 @@ fi\n \n if test \"$remove_redundant\" = t\n then\n-\t# We know $existing are all redundant only when\n-\t# all-into-one is used.\n-\tif test \"$all_into_one\" != '' && test \"$existing\" != ''\n+\t# We know $existing are all redundant.\n+\tif [ -n \"$existing\" ]\n \tthen\n \t\tsync\n \t\t( cd \"$PACKDIR\" &&\n \t\t  for e in $existing\n \t\t  do\n \t\t\tcase \"$e\" in\n-\t\t\t./pack-$name.pack | ./pack-$name.idx) ;;\n-\t\t\t*)\trm -f $e ;;\n+\t\t\tpack-$name) ;;\n+\t\t\t*)\trm -f \"$e.pack\" \"$e.idx\" \"$e.keep\" ;;\n \t\t\tesac\n \t\t  done\n \t\t)\n-- \n1.4.3.3.g7d63\n"},{"id":"294609","messageId":"Pine.LNX.4.64.0610301332440.11384@xanadu.home","threadId":"43253","inReplyTo":"20061029093754.GD3847@spearce.org","subject":"WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-10-30T19:07:57Z","receivedAt":"2006-10-30T19:07:57Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 29 Oct 2006, Shawn Pearce wrote:\n\n> During `git repack -a -d` only repack objects which are loose or\n> which reside in an active (a non-kept) pack.  This allows the user\n> to keep large packs as-is without continuous repacking and can be\n> very helpful on large repositories.\n\nSomething is really broken here.\n\nHere's how to destroy your GIT's git repository.\n\nWARNING: MAKE A COPY BEFORE TRYING THIS!  I'm serious.\n\nFirst, let's make a single pack just to make things simpler and \nreproducible:\n\n$ git-repack -a -f -d\n$ git-prune\n$ git-fsck-objects --full\n\nSo far everything should be fine.  It's still time to make a backup copy \nof your .git directory if you've not done so.\n\nNow let's create a second pack containing a subset of the existing one.\n\n$ git-rev-list --objects v1.4.3..v1.4.3.3 | \\\n  git-pack-objects --stdout | \\\n  git-index-pack --stdin -v --keep\n$ git-fsck-objects --full\n\nAt this point the repository is still fine, but the --keep to \ngit-index-pack above will have created a file called \n.git/objects/pack/pack-aceb4c6394c586abaf65d76dd6cf088f50a5b806.keep and \nthat is the source of all the trouble to come.  You still can remove \nthat file if you don't have a backup yet.\n\nIf you still want to give it the coup de grace, just do:\n\n$ git-repack -a -d\n\nAnd now you've just lost a large amount of objects.  To see the extent \nof the dammage, just do:\n\n$ git-fsck-objects\n\nSo... what is the --unpacked=<pack>.pack switch supposed to mean?  It is \nnot documented anywhere and it certainly doesn't produce the expected \nresult with a repack.\n\n\n"},{"id":"294829","messageId":"20061030192339.GA5504@spearce.org","threadId":"43253","inReplyTo":"Pine.LNX.4.64.0610301332440.11384@xanadu.home","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-30T19:23:39Z","receivedAt":"2006-10-30T19:23:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> On Sun, 29 Oct 2006, Shawn Pearce wrote:\n> \n> > During `git repack -a -d` only repack objects which are loose or\n> > which reside in an active (a non-kept) pack.  This allows the user\n> > to keep large packs as-is without continuous repacking and can be\n> > very helpful on large repositories.\n> \n> Something is really broken here.\n\nHoly cow.  Since this is now in 'next', 'next' is now seriously\nbroken if you have a .keep file.\n\n> So... what is the --unpacked=<pack>.pack switch supposed to mean?  It is \n> not documented anywhere and it certainly doesn't produce the expected \n> result with a repack.\n\nJunio introduced --unpacked=<pack>.pack a while ago for this\napplication.  What it does is skip an object unless its a loose\nobject file or it is in the named pack.  The idea being that\npack-objects would only consider object files which are loose or\nready to be repacked.\n\nIn your example above we should have copied all objects from your\nfirst pack into the new pack during the final destructive repack,\nbut we didn't.  I don't know why.\n\n-- \n"},{"id":"294779","messageId":"20061030202611.GA5775@spearce.org","threadId":"43253","inReplyTo":"Pine.LNX.4.64.0610301332440.11384@xanadu.home","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-30T20:26:11Z","receivedAt":"2006-10-30T20:26:11Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> On Sun, 29 Oct 2006, Shawn Pearce wrote:\n> \n> > During `git repack -a -d` only repack objects which are loose or\n> > which reside in an active (a non-kept) pack.  This allows the user\n> > to keep large packs as-is without continuous repacking and can be\n> > very helpful on large repositories.\n> \n> Something is really broken here.\n> \n> Here's how to destroy your GIT's git repository.\n> \n> WARNING: MAKE A COPY BEFORE TRYING THIS!  I'm serious.\n> \n> First, let's make a single pack just to make things simpler and \n> reproducible:\n> \n> $ git-repack -a -f -d\n> $ git-prune\n> $ git-fsck-objects --full\n\nActually the breakage is easier to reproduce without trashing\na repository.\n\nDo the above so you have everything in one pack.  Now use rev-list\nto simulate the object list construction in pack-objects as though\nwe were doing a 'git repack -a -d':\n\n  git-rev-list --objects --all \\\n    --unpacked=.git/objects/pack/pack-*.pack \\\n\t| wc -l\n\ngives me 102 (WRONG WRONG WRONG WRONG!!!!!!)\n\nand\n \n  git-rev-list --objects --all | wc -l\n\ngives me 31912 (correct).  The --unpacked flag is horribly broken.\n\n-- \n"},{"id":"297996","messageId":"20061030205200.GA20236@delft.aura.cs.cmu.edu","threadId":"43253","inReplyTo":"20061030202611.GA5775@spearce.org","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Jan Harkes","fromEmail":"jaharkes@cs.cmu.edu","sentAt":"2006-10-30T20:52:00Z","receivedAt":"2006-10-30T20:52:00Z","isPatch":true,"sender":{"key":"jaharkes@cs.cmu.edu","avatar":"https://gravatar.com/avatar/cf95aecd150ca8ef33d6edc337ac4bb9e13aa4246fc3679257d578c7fddc1633?d=mp&s=160"},"body":"On Mon, Oct 30, 2006 at 03:26:11PM -0500, Shawn Pearce wrote:\n> Actually the breakage is easier to reproduce without trashing\n> a repository.\n> \n> Do the above so you have everything in one pack.  Now use rev-list\n> to simulate the object list construction in pack-objects as though\n> we were doing a 'git repack -a -d':\n> \n>   git-rev-list --objects --all \\\n>     --unpacked=.git/objects/pack/pack-*.pack \\\n> \t| wc -l\n> \n> gives me 102 (WRONG WRONG WRONG WRONG!!!!!!)\n\nThe problem seems to be that as soon as we hit something that is found\nin a pack that is not on the ignore list, that object and all it's\nparents are marked as uninteresting. So if the kept pack contains a\nslice of commits (v1.4.3..v1.4.3.3) the revision walker will only return\nthe recent stuff (v1.4.3.3..) and drop the older data (..v1.4.3).\n\nThe following patch does fix the problem Nicolas reported, but for some\nreason I'm still getting only 102 objects (only tags and the commits\nthey refer to?) with your test.\n\nJan\n\n----\ndiff --git a/revision.c b/revision.c\nindex 280e92b..a69c873 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -418,9 +418,6 @@ static void limit_list(struct rev_info *\n \n \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n \t\t\tobj->flags |= UNINTERESTING;\n-\t\tif (revs->unpacked &&\n-\t\t    has_sha1_pack(obj->sha1, revs->ignore_packed))\n-\t\t\tobj->flags |= UNINTERESTING;\n \t\tadd_parents_to_list(revs, commit, &list);\n \t\tif (obj->flags & UNINTERESTING) {\n \t\t\tmark_parents_uninteresting(commit);\n@@ -1149,17 +1146,18 @@ struct commit *get_revision(struct rev_i\n \t\t * that we'd otherwise have done in limit_list().\n \t\t */\n \t\tif (!revs->limited) {\n-\t\t\tif ((revs->unpacked &&\n-\t\t\t     has_sha1_pack(commit->object.sha1,\n-\t\t\t\t\t   revs->ignore_packed)) ||\n-\t\t\t    (revs->max_age != -1 &&\n-\t\t\t     (commit->date < revs->max_age)))\n+\t\t\tif (revs->max_age != -1 &&\n+\t\t\t    (commit->date < revs->max_age))\n \t\t\t\tcontinue;\n \t\t\tadd_parents_to_list(revs, commit, &revs->commits);\n \t\t}\n \t\tif (commit->object.flags & SHOWN)\n \t\t\tcontinue;\n \n+\t\tif (revs->unpacked && has_sha1_pack(commit->object.sha1,\n+\t\t\t\t\t\t    revs->ignore_packed))\n+\t\t    continue;\n+\n \t\t/* We want to show boundary commits only when their\n \t\t * children are shown.  When path-limiter is in effect,\n"},{"id":"297717","messageId":"20061030210751.GB20236@delft.aura.cs.cmu.edu","threadId":"43253","inReplyTo":"20061030205200.GA20236@delft.aura.cs.cmu.edu","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Jan Harkes","fromEmail":"jaharkes@cs.cmu.edu","sentAt":"2006-10-30T21:07:51Z","receivedAt":"2006-10-30T21:07:51Z","isPatch":true,"sender":{"key":"jaharkes@cs.cmu.edu","avatar":"https://gravatar.com/avatar/cf95aecd150ca8ef33d6edc337ac4bb9e13aa4246fc3679257d578c7fddc1633?d=mp&s=160"},"body":"On Mon, Oct 30, 2006 at 03:52:00PM -0500, Jan Harkes wrote:\n> On Mon, Oct 30, 2006 at 03:26:11PM -0500, Shawn Pearce wrote:\n> > Do the above so you have everything in one pack.  Now use rev-list\n> > to simulate the object list construction in pack-objects as though\n> > we were doing a 'git repack -a -d':\n> > \n> >   git-rev-list --objects --all \\\n> >     --unpacked=.git/objects/pack/pack-*.pack \\\n> > \t| wc -l\n> > \n> > gives me 102 (WRONG WRONG WRONG WRONG!!!!!!)\n> \n...\n> \n> The following patch does fix the problem Nicolas reported, but for some\n> reason I'm still getting only 102 objects (only tags and the commits\n> they refer to?) with your test.\n\nSeems to be operator error, I guess the shell can't (won't) expand\n--unpacked=.git/objects/pack/pack-*.pack and there is no pack named\npack-*.pack, so rev-list will actually find every object in one of the\npacks and skip them.\n\nThe following works correctly,\n\n    $ git-rev-list --objects --all --unpacked=.git/objects/pack/pack-234f8136e45fb34d118bb346c15267535e80e5f0.pack --unpacked=.git/objects/pack/pack-aceb4c6394c586abaf65d76dd6cf088f50a5b806.pack | wc -l\n    28713\n\n    $ ~/git/git/git-rev-list --objects --all | wc -l\n    28713\n\nJan\n"},{"id":"295039","messageId":"20061030210908.GB5775@spearce.org","threadId":"43253","inReplyTo":"20061030205200.GA20236@delft.aura.cs.cmu.edu","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-30T21:09:08Z","receivedAt":"2006-10-30T21:09:08Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jan Harkes <jaharkes@cs.cmu.edu> wrote:\n> On Mon, Oct 30, 2006 at 03:26:11PM -0500, Shawn Pearce wrote:\n> > Actually the breakage is easier to reproduce without trashing\n> > a repository.\n> > \n> > Do the above so you have everything in one pack.  Now use rev-list\n> > to simulate the object list construction in pack-objects as though\n> > we were doing a 'git repack -a -d':\n> > \n> >   git-rev-list --objects --all \\\n> >     --unpacked=.git/objects/pack/pack-*.pack \\\n> > \t| wc -l\n> > \n> > gives me 102 (WRONG WRONG WRONG WRONG!!!!!!)\n> \n> The problem seems to be that as soon as we hit something that is found\n> in a pack that is not on the ignore list, that object and all it's\n> parents are marked as uninteresting. So if the kept pack contains a\n> slice of commits (v1.4.3..v1.4.3.3) the revision walker will only return\n> the recent stuff (v1.4.3.3..) and drop the older data (..v1.4.3).\n\nRight - I got that far in my own research and then saw your patch\ndrop into my inbox. :-)\n \n> The following patch does fix the problem Nicolas reported, but for some\n> reason I'm still getting only 102 objects (only tags and the commits\n> they refer to?) with your test.\n\nAck'd.\n\nYour patch fixes both bugs for me.  The rev-list test I talked about\nabove is now turning up 31846 objects which is the correct count.\nThe repack test Nico crafted works correctly too.\n\n-- \n"},{"id":"296721","messageId":"7v7iyhwk47.fsf@assigned-by-dhcp.cox.net","threadId":"43253","inReplyTo":"20061030202611.GA5775@spearce.org","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T21:48:24Z","receivedAt":"2006-10-30T21:48:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce <spearce@spearce.org> writes:\n\n> Do the above so you have everything in one pack.  Now use rev-list\n> to simulate the object list construction in pack-objects as though\n> we were doing a 'git repack -a -d':\n>\n>   git-rev-list --objects --all \\\n>     --unpacked=.git/objects/pack/pack-*.pack \\\n> \t| wc -l\n>\n> gives me 102 (WRONG WRONG WRONG WRONG!!!!!!)\n\nNow I think I know what is going on.\n\nThe meaning of \"unpacked\" (with or without the \"pretend as if\nall objects in this pack are loose\") has always been to stop\ntraversing once we hit a packed object, not \"do not include\nalready packed object\".\n\nSo --unpacked=pretend-this-is-loose was wrong to begin with; it\nprobably should have been --incremental=pretend-this-is-loose.\n\nHow about reverting the following:\n\ncommit ce8590748b918687abc4c7cd2d432dd23f07ae40\nAuthor: Shawn Pearce <spearce@spearce.org>\n\n    Only repack active packs by skipping over kept packs.\n\n\ncommit 106d710bc13f34aec1a15c4cff80f062f384edf6\nAuthor: Junio C Hamano <junkio@cox.net>\n\n    pack-objects --unpacked=<existing pack> option.\n\n\n"},{"id":"295723","messageId":"20061030215529.GC5775@spearce.org","threadId":"43253","inReplyTo":"7v7iyhwk47.fsf@assigned-by-dhcp.cox.net","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-30T21:55:29Z","receivedAt":"2006-10-30T21:55:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> Shawn Pearce <spearce@spearce.org> writes:\n> \n> > Do the above so you have everything in one pack.  Now use rev-list\n> > to simulate the object list construction in pack-objects as though\n> > we were doing a 'git repack -a -d':\n> >\n> >   git-rev-list --objects --all \\\n> >     --unpacked=.git/objects/pack/pack-*.pack \\\n> > \t| wc -l\n> >\n> > gives me 102 (WRONG WRONG WRONG WRONG!!!!!!)\n> \n> Now I think I know what is going on.\n> \n> The meaning of \"unpacked\" (with or without the \"pretend as if\n> all objects in this pack are loose\") has always been to stop\n> traversing once we hit a packed object, not \"do not include\n> already packed object\".\n\nDid you see Jan Harkes' patch that changes the behavior to be what\nit should have been?\n \n> So --unpacked=pretend-this-is-loose was wrong to begin with; it\n> probably should have been --incremental=pretend-this-is-loose.\n\nI don't care about what the option name is.  If you want to change\nit to --incremental we can but the --unpacked actually makes more\nsense now...  Its saying pretend every object in this pack is\nunpacked and therefore should be packed.\n \n> How about reverting the following:\n> \n> commit ce8590748b918687abc4c7cd2d432dd23f07ae40\n> Author: Shawn Pearce <spearce@spearce.org>\n> \n>     Only repack active packs by skipping over kept packs.\n> \n> \n> commit 106d710bc13f34aec1a15c4cff80f062f384edf6\n> Author: Junio C Hamano <junkio@cox.net>\n> \n>     pack-objects --unpacked=<existing pack> option.\n> \n\nNah.  I think Jan's patch fixes the bug and the --unpacked option\nnow makes sense as is, so I don't see why we would revert these.\n\n-- \n"},{"id":"298440","messageId":"7v3b95wjmg.fsf@assigned-by-dhcp.cox.net","threadId":"43253","inReplyTo":"20061030205200.GA20236@delft.aura.cs.cmu.edu","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T21:59:03Z","receivedAt":"2006-10-30T21:59:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Harkes <jaharkes@cs.cmu.edu> writes:\n\n> The following patch does fix the problem Nicolas reported, but for some\n> reason I'm still getting only 102 objects (only tags and the commits\n> they refer to?) with your test.\n\nOne potential downside of this is that this makes an obscure but\nuseful \"gitk --unpacked\" useless (robs performance).\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/19197/focus=19207\n\nBut other than that, I think it is an Ok change.  The original\nsemantics of --unpacked (with or without \"pretend as if objects\nin this pack are loose\") were, eh, \"strange\".\n\n"},{"id":"294142","messageId":"7vy7qxv4oq.fsf@assigned-by-dhcp.cox.net","threadId":"43253","inReplyTo":"20061030215529.GC5775@spearce.org","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T22:07:01Z","receivedAt":"2006-10-30T22:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce <spearce@spearce.org> writes:\n\n> Did you see Jan Harkes' patch that changes the behavior to be what\n> it should have been?\n>  \n>> So --unpacked=pretend-this-is-loose was wrong to begin with; it\n>> probably should have been --incremental=pretend-this-is-loose.\n>\n> I don't care about what the option name is.\n\nIt is not about name, but what --unpacked means.  See my other\nmail.\n"},{"id":"298017","messageId":"7vbqntv2h5.fsf@assigned-by-dhcp.cox.net","threadId":"43253","inReplyTo":"7v3b95wjmg.fsf@assigned-by-dhcp.cox.net","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T22:54:46Z","receivedAt":"2006-10-30T22:54:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Jan Harkes <jaharkes@cs.cmu.edu> writes:\n>\n>> The following patch does fix the problem Nicolas reported, but for some\n>> reason I'm still getting only 102 objects (only tags and the commits\n>> they refer to?) with your test.\n>\n> One potential downside of this is that this makes an obscure but\n> useful \"gitk --unpacked\" useless (robs performance).\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/19197/focus=19207\n>\n> But other than that, I think it is an Ok change.  The original\n> semantics of --unpacked (with or without \"pretend as if objects\n> in this pack are loose\") were, eh, \"strange\".\n\nI changed my mind.\n\nEven without --unpacked=pretend-this-is-loose nor .keep flag,\nthe original semantics of --unpacked and git repack do not play\nwith each other well.  You can have a history where you have a\npack in the middle of the history, and would expect \"git repack\"\nwithout -a to make your .git/objects/??/ directories empty but\nit would not because --unpacked has been defined to mean \"stop\ntraversal when we hit a packed commit\".  That would _not_\ncorrupt the repository, but is very counter-intuitive.\n\nUnfortunately this is a semantic change in the middle of the\nroad (and it would change the _output_ not just performance of\n\"gitk --unpacked\"), but I think it is a semantic change of a\ngood kind.\n\nSo I'll take Jan's patch as is.  It needs to go all the way down\nto \"maint\", since we have --unpacked= there already.\n"},{"id":"296061","messageId":"20061030225500.GG3617@delft.aura.cs.cmu.edu","threadId":"43253","inReplyTo":"7v3b95wjmg.fsf@assigned-by-dhcp.cox.net","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Jan Harkes","fromEmail":"jaharkes@cs.cmu.edu","sentAt":"2006-10-30T22:55:00Z","receivedAt":"2006-10-30T22:55:00Z","isPatch":true,"sender":{"key":"jaharkes@cs.cmu.edu","avatar":"https://gravatar.com/avatar/cf95aecd150ca8ef33d6edc337ac4bb9e13aa4246fc3679257d578c7fddc1633?d=mp&s=160"},"body":"On Mon, Oct 30, 2006 at 01:59:03PM -0800, Junio C Hamano wrote:\n> Jan Harkes <jaharkes@cs.cmu.edu> writes:\n> \n> > The following patch does fix the problem Nicolas reported, but for some\n> > reason I'm still getting only 102 objects (only tags and the commits\n> > they refer to?) with your test.\n> \n> One potential downside of this is that this makes an obscure but\n> useful \"gitk --unpacked\" useless (robs performance).\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/19197/focus=19207\n\nIf I use 'git fetch' followed later on by a 'git fetch -k', the result\nfrom --unpacked would not include the unpacked objects created by the\nfirst fetch. Although it may have been fast, it seems to be somewhat\ncounter-intuitive.\n\n> But other than that, I think it is an Ok change.  The original\n> semantics of --unpacked (with or without \"pretend as if objects\n> in this pack are loose\") were, eh, \"strange\".\n\nDo you need a resend with a proper 'Signed-Off-By' line?\n\nJan\n"},{"id":"298565","messageId":"7vhcxltmit.fsf@assigned-by-dhcp.cox.net","threadId":"43253","inReplyTo":"20061030225500.GG3617@delft.aura.cs.cmu.edu","subject":"Re: WARNING: THIS PATCH CAN BREAK YOUR REPO, was Re: [PATCH 2/3] Only repack active packs by skipping over kept packs.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-30T23:24:42Z","receivedAt":"2006-10-30T23:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Harkes <jaharkes@cs.cmu.edu> writes:\n\n> On Mon, Oct 30, 2006 at 01:59:03PM -0800, Junio C Hamano wrote:\n>> Jan Harkes <jaharkes@cs.cmu.edu> writes:\n>> \n>> > The following patch does fix the problem Nicolas reported, but for some\n>> > reason I'm still getting only 102 objects (only tags and the commits\n>> > they refer to?) with your test.\n>> \n>> One potential downside of this is that this makes an obscure but\n>> useful \"gitk --unpacked\" useless (robs performance).\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/19197/focus=19207\n>\n> If I use 'git fetch' followed later on by a 'git fetch -k', the result\n> from --unpacked would not include the unpacked objects created by the\n> first fetch. Although it may have been fast, it seems to be somewhat\n> counter-intuitive.\n>\n>> But other than that, I think it is an Ok change.  The original\n>> semantics of --unpacked (with or without \"pretend as if objects\n>> in this pack are loose\") were, eh, \"strange\".\n>\n> Do you need a resend with a proper 'Signed-Off-By' line?\n\nOh, I was planning to write a fairly detailed explanation\nmyself, but the description with S-o-b by the original author\nwould certainly be more appropriate.  Thanks.\n"},{"id":"296118","messageId":"20061031013749.GA19885@delft.aura.cs.cmu.edu","threadId":"43253","inReplyTo":"7vhcxltmit.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Continue traversal when rev-list --unpacked finds a packed commit.","fromName":"Jan Harkes","fromEmail":"jaharkes@cs.cmu.edu","sentAt":"2006-10-31T01:37:49Z","receivedAt":"2006-10-31T01:37:49Z","isPatch":true,"sender":{"key":"jaharkes@cs.cmu.edu","avatar":"https://gravatar.com/avatar/cf95aecd150ca8ef33d6edc337ac4bb9e13aa4246fc3679257d578c7fddc1633?d=mp&s=160"},"body":"\nWhen getting the list of all unpacked objects by walking the commit history,\nwe would stop traversal whenever we hit a packed commit. However the fact\nthat we found a packed commit does not guarantee that all previous commits\nare also packed. As a result the commit walkers did not show all reachable\nunpacked objects.\n\nSigned-off-by: Jan Harkes <jaharkes@cs.cmu.edu>\n---\n revision.c |   14 ++++++--------\n 1 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 280e92b..a69c873 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -418,9 +418,6 @@ static void limit_list(struct rev_info *\n \n \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n \t\t\tobj->flags |= UNINTERESTING;\n-\t\tif (revs->unpacked &&\n-\t\t    has_sha1_pack(obj->sha1, revs->ignore_packed))\n-\t\t\tobj->flags |= UNINTERESTING;\n \t\tadd_parents_to_list(revs, commit, &list);\n \t\tif (obj->flags & UNINTERESTING) {\n \t\t\tmark_parents_uninteresting(commit);\n@@ -1149,17 +1146,18 @@ struct commit *get_revision(struct rev_i\n \t\t * that we'd otherwise have done in limit_list().\n \t\t */\n \t\tif (!revs->limited) {\n-\t\t\tif ((revs->unpacked &&\n-\t\t\t     has_sha1_pack(commit->object.sha1,\n-\t\t\t\t\t   revs->ignore_packed)) ||\n-\t\t\t    (revs->max_age != -1 &&\n-\t\t\t     (commit->date < revs->max_age)))\n+\t\t\tif (revs->max_age != -1 &&\n+\t\t\t    (commit->date < revs->max_age))\n \t\t\t\tcontinue;\n \t\t\tadd_parents_to_list(revs, commit, &revs->commits);\n \t\t}\n \t\tif (commit->object.flags & SHOWN)\n \t\t\tcontinue;\n \n+\t\tif (revs->unpacked && has_sha1_pack(commit->object.sha1,\n+\t\t\t\t\t\t    revs->ignore_packed))\n+\t\t    continue;\n+\n \t\t/* We want to show boundary commits only when their\n \t\t * children are shown.  When path-limiter is in effect,\n \t\t * rewrite_parents() drops some commits from getting shown,\n-- \n1.4.2.4.gd5de\n"},{"id":"296413","messageId":"7vk62hs1ct.fsf@assigned-by-dhcp.cox.net","threadId":"43253","inReplyTo":"20061031013749.GA19885@delft.aura.cs.cmu.edu","subject":"Re: [PATCH] Continue traversal when rev-list --unpacked finds a packed commit.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-31T01:47:14Z","receivedAt":"2006-10-31T01:47:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Harkes <jaharkes@cs.cmu.edu> writes:\n\n> When getting the list of all unpacked objects by walking the commit history,\n> we would stop traversal whenever we hit a packed commit. However the fact\n> that we found a packed commit does not guarantee that all previous commits\n> are also packed. As a result the commit walkers did not show all reachable\n> unpacked objects.\n>\n> Signed-off-by: Jan Harkes <jaharkes@cs.cmu.edu>\n\nThanks.\n\nWith this, I think revs->unpacked should not mean \"limited\", so\nI suspect this is also needed, no?\n\ndiff --git a/revision.c b/revision.c\nindex 93f2513..2d7cad9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1010,7 +1010,7 @@ int setup_revisions(int argc, const char\n \t\tadd_pending_object(revs, object, def);\n \t}\n \n-\tif (revs->topo_order || revs->unpacked)\n+\tif (revs->topo_order)\n \t\trevs->limited = 1;\n \n \tif (revs->prune_data) {\n\n\n"},{"id":"298518","messageId":"20061031021739.GH3617@delft.aura.cs.cmu.edu","threadId":"43253","inReplyTo":"7vk62hs1ct.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Continue traversal when rev-list --unpacked finds a packed commit.","fromName":"Jan Harkes","fromEmail":"jaharkes@cs.cmu.edu","sentAt":"2006-10-31T02:17:39Z","receivedAt":"2006-10-31T02:17:39Z","isPatch":true,"sender":{"key":"jaharkes@cs.cmu.edu","avatar":"https://gravatar.com/avatar/cf95aecd150ca8ef33d6edc337ac4bb9e13aa4246fc3679257d578c7fddc1633?d=mp&s=160"},"body":"On Mon, Oct 30, 2006 at 05:47:14PM -0800, Junio C Hamano wrote:\n> Jan Harkes <jaharkes@cs.cmu.edu> writes:\n> \n> > When getting the list of all unpacked objects by walking the commit history,\n> > we would stop traversal whenever we hit a packed commit. However the fact\n> > that we found a packed commit does not guarantee that all previous commits\n> > are also packed. As a result the commit walkers did not show all reachable\n> > unpacked objects.\n> >\n> > Signed-off-by: Jan Harkes <jaharkes@cs.cmu.edu>\n> \n> Thanks.\n> \n> With this, I think revs->unpacked should not mean \"limited\", so\n> I suspect this is also needed, no?\n\nI'm not familiar enough with the code to know for sure, but my gut\nfeeling is that that would be needed. Let me check...\n\nWhen that flag is set, the code calls limit_list, which no longer stops\ntraversal when we hit a packed commit. So we end up with a list of all\ncommits in memory. If the flag is not set, the list is kept minimal and\nparents are only traversed as they are encountered.\n\nSo it looks like not setting the flag reduces memory usage we traverse\nall parents in both cases. Yes, you are correct.\n\nJan\n"}]}