{"thread":{"id":"5469","subject":"[PATCH] git-repack: create new packs inside $PACKDIR, not cwd","startedAt":"2006-09-04T05:42:32Z","lastAt":"2006-09-04T20:16:34Z","messageCount":8,"participants":["Martin Langhoff","Martin Waitz","Junio C Hamano","Martin Langhoff (CatalystIT)"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26291","messageId":"11573485523752-git-send-email-martin@catalyst.net.nz","threadId":"5469","inReplyTo":null,"subject":"[PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2006-09-04T05:42:32Z","receivedAt":"2006-09-04T05:42:32Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Avoid failing when cwd is !writable by writing the\npackfiles directly in the $PACKDIR.\n\nWithout this, git-repack was failing when run from crontab\nby non-root user accounts. For large repositories, this\nalso makes the mv operation a lot cheaper, and avoids leaving\ntemp packfiles around the fs upon failure.\n\nSigned-off-by: Martin Langhoff <martin@catalyst.net.nz>\n---\n git-repack.sh |    9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 584a732..ccc8e43 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -42,11 +42,13 @@ case \",$all_into_one,\" in\n \t    find . -type f \\( -name '*.pack' -o -name '*.idx' \\) -print`\n \t;;\n esac\n+\n+mkdir -p \"$PACKDIR\" || exit\n pack_objects=\"$pack_objects $local $quiet $no_reuse_delta$extra\"\n name=$( { git-rev-list --objects --all $rev_list ||\n \t  echo \"git-rev-list died with exit code $?\"\n \t} |\n-\tgit-pack-objects --non-empty $pack_objects .tmp-pack) ||\n+\tgit-pack-objects --non-empty $pack_objects \"$PACKDIR/.tmp-pack\") ||\n \texit 1\n if [ -z \"$name\" ]; then\n \techo Nothing new to pack.\n@@ -54,7 +56,6 @@ else\n \tif test \"$quiet\" != '-q'; then\n \t    echo \"Pack pack-$name created.\"\n \tfi\n-\tmkdir -p \"$PACKDIR\" || exit\n \n \tfor sfx in pack idx\n \tdo\n@@ -64,8 +65,8 @@ else\n \t\t\t\t\"$PACKDIR/old-pack-$name.$sfx\"\n \t\tfi\n \tdone &&\n-\tmv -f .tmp-pack-$name.pack \"$PACKDIR/pack-$name.pack\" &&\n-\tmv -f .tmp-pack-$name.idx  \"$PACKDIR/pack-$name.idx\" &&\n+\tmv -f \"$PACKDIR/.tmp-pack-$name.pack\" \"$PACKDIR/pack-$name.pack\" &&\n+\tmv -f \"$PACKDIR/.tmp-pack-$name.idx\"  \"$PACKDIR/pack-$name.idx\" &&\n \ttest -f \"$PACKDIR/pack-$name.pack\" &&\n \ttest -f \"$PACKDIR/pack-$name.idx\" || {\n \t\techo >&2 \"Couldn't replace the existing pack with updated one.\"\n-- \n1.4.2.gdfe7\n\n\n-- \nVGER BF report: S 1\n"},{"id":"26292","messageId":"20060904090833.GF17042@admingilde.org","threadId":"5469","inReplyTo":"11573485523752-git-send-email-martin@catalyst.net.nz","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Martin Waitz","fromEmail":"tali@admingilde.org","sentAt":"2006-09-04T09:08:33Z","receivedAt":"2006-09-04T09:08:33Z","isPatch":true,"sender":{"key":"tali@admingilde.org","avatar":"https://gravatar.com/avatar/3f89b03eee362187effabe257898735b475673a12265c398ea9161259ae91553?d=mp&s=160"},"body":"hoi :)\n\nOn Mon, Sep 04, 2006 at 05:42:32PM +1200, Martin Langhoff wrote:\n> Avoid failing when cwd is !writable by writing the\n> packfiles directly in the $PACKDIR.\n\nwhat if other GIT commands are being called while you repack?\nWouldn't they try to use the new packs, even while they are not\ncompletely written?\n\nPerhaps it is better to create a new subdirectory $PACKDIR/.tmp/\nand create the new pack files there?\n\n-- \nMartin Waitz\n"},{"id":"26294","messageId":"7vveo4nfbg.fsf@assigned-by-dhcp.cox.net","threadId":"5469","inReplyTo":"20060904090833.GF17042@admingilde.org","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-04T09:36:51Z","receivedAt":"2006-09-04T09:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Waitz <tali@admingilde.org> writes:\n\n> hoi :)\n>\n> On Mon, Sep 04, 2006 at 05:42:32PM +1200, Martin Langhoff wrote:\n>> Avoid failing when cwd is !writable by writing the\n>> packfiles directly in the $PACKDIR.\n>\n> what if other GIT commands are being called while you repack?\n> Wouldn't they try to use the new packs, even while they are not\n> completely written?\n\nGiven that a new pack is created first by writing out .pack and\nthen .idx, and that the using side ignores .pack without\ncorresponding .idx, we are talking about very small window, but\nyou are right.\n\nWriting into $cwd was certainly a carelessness; we tend to use\n$GIT_DIR/ for this kind of thing.\n\n\n\n-- \nVGER BF report: U 0.926108\n"},{"id":"26295","messageId":"7vr6ysneor.fsf@assigned-by-dhcp.cox.net","threadId":"5469","inReplyTo":"7vveo4nfbg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-04T09:50:28Z","receivedAt":"2006-09-04T09:50:28Z","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> Writing into $cwd was certainly a carelessness; we tend to use\n> $GIT_DIR/ for this kind of thing.\n\nIn other words...\n\n-- >8 --\nFrom: Martin Langhoff <martin@catalyst.net.nz>\nDate: Mon, 4 Sep 2006 17:42:32 +1200\nSubject: [PATCH] git-repack: create new packs inside $GIT_DIR, not cwd\n\nAvoid failing when cwd is !writable by writing the\npackfiles in $GIT_DIR, which is more in line with other commands.\n\nWithout this, git-repack was failing when run from crontab\nby non-root user accounts. For large repositories, this\nalso makes the mv operation a lot cheaper, and avoids leaving\ntemp packfiles around the fs upon failure.\n\nSigned-off-by: Martin Langhoff <martin@catalyst.net.nz>\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n git-repack.sh |   11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 584a732..b525fc5 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -24,8 +24,10 @@ do\n \tshift\n done\n \n-rm -f .tmp-pack-*\n PACKDIR=\"$GIT_OBJECT_DIRECTORY/pack\"\n+PACKTMP=\"$GIT_DIR/.tmp-$$-pack\"\n+rm -f \"$PACKTMP\"-*\n+trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n \n # There will be more repacking strategies to come...\n case \",$all_into_one,\" in\n@@ -42,11 +44,12 @@ case \",$all_into_one,\" in\n \t    find . -type f \\( -name '*.pack' -o -name '*.idx' \\) -print`\n \t;;\n esac\n+\n pack_objects=\"$pack_objects $local $quiet $no_reuse_delta$extra\"\n name=$( { git-rev-list --objects --all $rev_list ||\n \t  echo \"git-rev-list died with exit code $?\"\n \t} |\n-\tgit-pack-objects --non-empty $pack_objects .tmp-pack) ||\n+\tgit-pack-objects --non-empty $pack_objects \"$PACKTMP\") ||\n \texit 1\n if [ -z \"$name\" ]; then\n \techo Nothing new to pack.\n@@ -64,8 +67,8 @@ else\n \t\t\t\t\"$PACKDIR/old-pack-$name.$sfx\"\n \t\tfi\n \tdone &&\n-\tmv -f .tmp-pack-$name.pack \"$PACKDIR/pack-$name.pack\" &&\n-\tmv -f .tmp-pack-$name.idx  \"$PACKDIR/pack-$name.idx\" &&\n+\tmv -f \"$PACKTMP-$name.pack\" \"$PACKDIR/pack-$name.pack\" &&\n+\tmv -f \"$PACKTMP-$name.idx\"  \"$PACKDIR/pack-$name.idx\" &&\n \ttest -f \"$PACKDIR/pack-$name.pack\" &&\n \ttest -f \"$PACKDIR/pack-$name.idx\" || {\n \t\techo >&2 \"Couldn't replace the existing pack with updated one.\"\n-- \n1.4.2.g99d7d\n\n\n\n-- \nVGER BF report: U 0.870206\n"},{"id":"26296","messageId":"44FBF9E0.9050800@catalyst.net.nz","threadId":"5469","inReplyTo":"7vr6ysneor.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Martin Langhoff (CatalystIT)","fromEmail":"martin@catalyst.net.nz","sentAt":"2006-09-04T10:03:12Z","receivedAt":"2006-09-04T10:03:12Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n\n> In other words...\n\nCan't be offline 2 hs to read a book... ;-) Actually, I had thought the \npack reading code would focus on filenames following pack-<id>.pack \npattern and corresponding idx files, and that .tmp-* was safe to have \nthere. My bad.\n\nBTW, I think there's a small error.\n\n...\n\n> --- a/git-repack.sh\n> +++ b/git-repack.sh\n> @@ -24,8 +24,10 @@ do\n>  \tshift\n>  done\n>  \n> -rm -f .tmp-pack-*\n>  PACKDIR=\"$GIT_OBJECT_DIRECTORY/pack\"\n> +PACKTMP=\"$GIT_DIR/.tmp-$$-pack\"\n> +rm -f \"$PACKTMP\"-*\n> +trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n\nYour packtmp includes $$ which means that rm -f \"$PACKTMP\" will only \nclear out old packs only if the pid of the old-and-probably-dead process \nmatches ours... and then a hyphen.\n\nso instead I propose...\n\n+trap 'rm -f \"$GIT_DIR/.tmp-*-pack\"' 0 1 2 3 15\n\ncheers,\n\n\nmartin\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington\nWEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St\nOFFICE: +64(4)916-7224                              MOB: +64(21)364-017\n       Make things as simple as possible, but no simpler - Einstein\n-----------------------------------------------------------------------\n\n-- \nVGER BF report: U 0.900798\n"},{"id":"26297","messageId":"7vlkp0ndmj.fsf@assigned-by-dhcp.cox.net","threadId":"5469","inReplyTo":"44FBF9E0.9050800@catalyst.net.nz","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-04T10:13:24Z","receivedAt":"2006-09-04T10:13:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Martin Langhoff (CatalystIT)\" <martin@catalyst.net.nz> writes:\n\n> BTW, I think there's a small error.\n>\n> Your packtmp includes $$ which means that rm -f \"$PACKTMP\" will only\n> clear out old packs..\n\nThat was deliberate.  I hate programs that clean things up\nbehind user's back.  The first \"rm\" is to get rid of what would\ncollide with what we are going to do (i.e. protecting ourselves)\nand \"trap rm\" is to make sure we do not leave the cruft we know\nwe are going to create.  I'd rather leave other people's cruft\naround, unless the purpose of the command is to clean things up,\nand repack is hardly that.\n\n\n\n\n\n-- \nVGER BF report: H 0.149712\n"},{"id":"26298","messageId":"44FC041F.6010002@catalyst.net.nz","threadId":"5469","inReplyTo":"7vlkp0ndmj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Martin Langhoff (CatalystIT)","fromEmail":"martin@catalyst.net.nz","sentAt":"2006-09-04T10:46:55Z","receivedAt":"2006-09-04T10:46:55Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n\n> \"Martin Langhoff (CatalystIT)\" <martin@catalyst.net.nz> writes:\n> \n> \n>>BTW, I think there's a small error.\n>>\n>>Your packtmp includes $$ which means that rm -f \"$PACKTMP\" will only\n>>clear out old packs..\n> \n> \n> That was deliberate.  I hate programs that clean things up\n> behind user's back.  The first \"rm\" is to get rid of what would\n> collide with what we are going to do (i.e. protecting ourselves)\n> and \"trap rm\" is to make sure we do not leave the cruft we know\n> we are going to create.  I'd rather leave other people's cruft\n> around, unless the purpose of the command is to clean things up,\n> and repack is hardly that.\n> \n\nAh, ok. I misunderstood the use of trap -- of course, re-reading the man \npages, it makes sense.\n\nA-ok with me, then, and sorry about the noise.\n\n\n\nmartin\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington\nWEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St\nOFFICE: +64(4)916-7224                              MOB: +64(21)364-017\n       Make things as simple as possible, but no simpler - Einstein\n-----------------------------------------------------------------------\n\n-- \nVGER BF report: H 0.0618878\n"},{"id":"26317","messageId":"7vslj7mlp9.fsf@assigned-by-dhcp.cox.net","threadId":"5469","inReplyTo":"44FC041F.6010002@catalyst.net.nz","subject":"Re: [PATCH] git-repack: create new packs inside $PACKDIR, not cwd","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-04T20:16:34Z","receivedAt":"2006-09-04T20:16:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Martin Langhoff (CatalystIT)\" <martin@catalyst.net.nz> writes:\n\n> Ah, ok. I misunderstood the use of trap -- of course, re-reading the\n> man pages, it makes sense.\n\nHowever the more I think about it your original idea of using\n.tmp-pack directly in .git/objects/pack/ *should* have worked\n(modulo two repack instances using the same name, which is fixed\nby $$ there).  Not insisting on \"pack-\" prefix I consider is a\nfeature, but I do not see a reason to take a file that begin\nwith a dot.\n\nWell, probably too late to deprecate, maybe not.  I dunno.\n"}]}