{"thread":{"id":"26941","subject":"Problems with stale .keep files on git server","startedAt":"2011-03-31T10:46:25Z","lastAt":"2011-04-03T01:01:29Z","messageCount":14,"participants":["Johan Herland","Jeff King","Junio C Hamano","Shawn Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"164769","messageId":"201103311246.25645.johan@herland.net","threadId":"26941","inReplyTo":null,"subject":"Problems with stale .keep files on git server","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-03-31T10:46:25Z","receivedAt":"2011-03-31T10:46:25Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Hi,\n\nI have a problem in a server repo where it seems that some previous\n\"git push\" command by some user has left a stale .keep file in the\nrepo.git/objects/pack/ directory.\n\nNow, when trying to clone the repo on the server, the clone fails with:\n\n$ git clone --bare /path/to/repo.git myclone.git\nCloning into bare repository myclone.git...\nfatal: failed to copy file to 'myclone.git/objects/pack/pack-6195737bf980830662f9a44eced023ca4ebe083a.keep': Permission denied\n\n(This is a local clone across filesystems, which I assume simply\ncopies the objects/ directory from the source repo)\n\nLooking at the .keep file that it's trying to copy from the source\nrepo, it is owned by the same user as the corresponding .pack and\n.idx files, but while the .pack and .idx files have 0440 permissions,\nthe .keep file has 0600 permission (which explains the \"Permission\ndenied\" error). The .keep file itself contains the following text:\n\n receive-pack 6932 on gitmain\n\n(where gitmain is the hostname of this server)\n\nThe timestamp on the .keep file is over a month old, and there is\ncurrently no process with ID 6932 running on this server.\n\nAFAICS, this indicates that someone pushed this pack over a month ago,\nand for some reason it failed/aborted, and left the .keep file lying\naround. From browsing the source, I see that the .keep file is created\nby receive-pack protect the pack from a concurrent \"git gc\" while it is\nbeing created. However, I have yet to find under which conditions\nreceive-pack will die without removing the .keep file.\n\nSome questions:\n\n1. Why does the .keep file have 0600 permissions (preventing a local\n clone by any other user)\n\n2. Under which conditions will receive-pack leave stale .keep files\n in the filesystem? Is this a bug?\n\n3. Do I need to scan for and remove stale .keep files in a cron job\n in order to keep repos healthy and clonable?\n\n\nThanks,\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"164821","messageId":"20110331190429.GC16981@sigill.intra.peff.net","threadId":"26941","inReplyTo":"201103311246.25645.johan@herland.net","subject":"Re: Problems with stale .keep files on git server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-31T19:04:29Z","receivedAt":"2011-03-31T19:04:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:\n\n> Some questions:\n> \n> 1. Why does the .keep file have 0600 permissions (preventing a local\n> clone by any other user)\n\nThe relevant code is in 6e180cd (Make sure objects/pack exists before\ncreating a new pack, 2009-02-24). I don't see anything particular about\nthe mode, so I suspect it was simply habit to make tempfiles restricted.\n\nThere is nothing secret in the contents, so I don't see any reason to\nloosen it to the same permissions as the packfiles themselves.\n\n> 2. Under which conditions will receive-pack leave stale .keep files\n> in the filesystem? Is this a bug?\n\nI didn't look at the code, but I think we have to accept the possibility\nthat it may leave stale ones in the case of power failure or accidental\ndeath by signal. So maybe there are places to fix, but we will still\nhave to deal with stale ones.\n\n> 3. Do I need to scan for and remove stale .keep files in a cron job\n> in order to keep repos healthy and clonable?\n\nIf we fix (1), then hopefully it is not as much of an issue. But\nprobably \"git gc\" should clean up stale ones after a while.\n\n-Peff\n"},{"id":"164847","messageId":"201104010329.05299.johan@herland.net","threadId":"26941","inReplyTo":"20110331190429.GC16981@sigill.intra.peff.net","subject":"[PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-04-01T01:29:05Z","receivedAt":"2011-04-01T01:29:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"While pushing to a remote repo, Git transiently adds a .keep file for the\npack being pushed, to protect it from a concurrent \"git gc\". However, the\npermissions on this .keep file are such that if a different user attempts\na local cross-filesystem clone (\"git clone --no-hardlinks\") on the server\nwhile the .keep file is present (either because of a concurrent push, or\nbecause of a prior failed push that left a stale .keep file), the clone\nwill fail because the second user cannot access the .keep file created by\nthe first user.\n\nThere's no reason why the permission mode of a .keep file should be any\ndifferent from the permission mode of the corresponding .pack/.idx files.\nTherefore, adjust the permission of .keep files from 0600 to 0444 modulo\nthe shared_repository setting.\n\nIn the above scenario, the .keep file is now accessible to the second user,\nand will not prevent the clone.\n\nSigned-off-by: Johan Herland \n---\n\nOn Thursday 31 March 2011, Jeff King wrote:\n> On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:\n> > 1. Why does the .keep file have 0600 permissions (preventing a local\n> > clone by any other user)\n> \n> The relevant code is in 6e180cd (Make sure objects/pack exists before\n> creating a new pack, 2009-02-24). I don't see anything particular about\n> the mode, so I suspect it was simply habit to make tempfiles restricted.\n> \n> There is nothing secret in the contents, so I don't see any reason to\n> loosen it to the same permissions as the packfiles themselves.\n\nThis patch attempts to fix the permissions on .keep files.\n\n\n...Johan\n\n\n builtin/index-pack.c | 9 ++++++---\n environment.c | 4 ++--\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 5a67c81..586c9ac 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -792,10 +792,11 @@ static void final(const char *final_pack_name, const char \n*curr_pack_name,\n \tif (keep_msg) {\n \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n \n-\t\tif (!keep_name)\n+\t\tif (!keep_name) {\n \t\t\tkeep_fd = odb_pack_keep(name, sizeof(name), sha1);\n-\t\telse\n-\t\t\tkeep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);\n+\t\t\tkeep_name = name;\n+\t\t} else\n+\t\t\tkeep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0444);\n \n \t\tif (keep_fd < 0) {\n \t\t\tif (errno != EEXIST)\n@@ -811,6 +812,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\t\t keep_name);\n \t\t\treport = \"keep\";\n \t\t}\n+\t\tif (adjust_shared_perm(keep_name))\n+\t\t\terror(\"unable to set permission to '%s'\", keep_name);\n \t}\n \n \tif (final_pack_name != curr_pack_name) {\ndiff --git a/environment.c b/environment.c\nindex f4549d3..86bf8f4 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -191,13 +191,13 @@ int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)\n \n \tsnprintf(name, namesz, \"%s/pack/pack-%s.keep\",\n \t\t get_object_directory(), sha1_to_hex(sha1));\n-\tfd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n+\tfd = open(name, O_RDWR|O_CREAT|O_EXCL, 0444);\n \tif (0 <= fd)\n \t\treturn fd;\n \n \t/* slow path */\n \tsafe_create_leading_directories(name);\n-\treturn open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n+\treturn open(name, O_RDWR|O_CREAT|O_EXCL, 0444);\n }\n \n char *get_index_file(void)\n-- \n1.7.4\n"},{"id":"164849","messageId":"201104010334.28048.johan@herland.net","threadId":"26941","inReplyTo":"20110331190429.GC16981@sigill.intra.peff.net","subject":"[RFC/PATCH 2/2] repack: Remove stale .keep files before repacking","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-04-01T01:34:27Z","receivedAt":"2011-04-01T01:34:27Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When a push is aborted, receive-pack sometimes leave stale .keep files in the\nobjects/pack/ directory. Fortunately, these files are easily identified by\nlooking at their contents, which is of the form:\n\n receive-pack $pid on $host\n\nBy recognizing this format we can determine whether the .keep file is stale,\nand can be safely deleted: If the $host part matches the current hostname,\nand there is currently no process with $pid, we can safely assume that the\n.keep file no longer refers to a running receive-pack process, and deleting\nit should be perfectly safe.\n---\n\nOn Thursday 31 March 2011, Jeff King wrote:\n> On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:\n> > 3. Do I need to scan for and remove stale .keep files in a cron job\n> > \n> > in order to keep repos healthy and clonable?\n> \n> If we fix (1), then hopefully it is not as much of an issue. But\n> probably \"git gc\" should clean up stale ones after a while.\n\nThis patch tries to automatically remove stale .keep files. However,\nit's still work-in-progress, as I don't know how to portably (a) ask\nfor the current hostname (so that I can compare it to the one in the\n.keep file), or (b) test for whether a given PID is running on the\nsystem (to determine whether the receive-pack process that wrote the\n.keep file is still alive).\n\nFeedback appreciated.\n\n\n...Johan\n\n\n git-repack.sh | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 624feec..f59e4c4 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -56,6 +56,18 @@ PACKTMP=\"$PACKDIR/.tmp-$$-pack\"\n rm -f \"$PACKTMP\"-*\n trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n \n+HOST=`hostname || echo \"localhost\"` # FIXME: Portability?\n+for e in `cd \"$PACKDIR\" && find . -type f -name '*.keep' | sed -e 's/^\\.\\///'`\n+do\n+\tcat \"$PACKDIR/$e\" | if read word pid on host\n+\tthen\n+\t\ttest \"$word\" = \"receive-pack\" -a \"$on\" = \"on\" -a \"$host\" = \"$HOST\" || continue\n+\t\tps -p \"$pid\" > /dev/null && continue # FIXME: Portability?\n+\t\trm -f \"$PACKDIR/$e\"\n+\t\tsay Removed stale keep file $PACKDIR/$e.\n+\tfi\n+done\n+\n # There will be more repacking strategies to come...\n case \",$all_into_one,\" in\n ,,)\n-- \n1.7.4\n"},{"id":"164851","messageId":"20110401014150.GC21036@sigill.intra.peff.net","threadId":"26941","inReplyTo":"201104010334.28048.johan@herland.net","subject":"Re: [RFC/PATCH 2/2] repack: Remove stale .keep files before repacking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T01:41:50Z","receivedAt":"2011-04-01T01:41:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 03:34:27AM +0200, Johan Herland wrote:\n\n> On Thursday 31 March 2011, Jeff King wrote:\n> > On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:\n> > > 3. Do I need to scan for and remove stale .keep files in a cron job\n> > > \n> > > in order to keep repos healthy and clonable?\n> > \n> > If we fix (1), then hopefully it is not as much of an issue. But\n> > probably \"git gc\" should clean up stale ones after a while.\n> \n> This patch tries to automatically remove stale .keep files. However,\n> it's still work-in-progress, as I don't know how to portably (a) ask\n> for the current hostname (so that I can compare it to the one in the\n> .keep file), or (b) test for whether a given PID is running on the\n> system (to determine whether the receive-pack process that wrote the\n> .keep file is still alive).\n> \n> Feedback appreciated.\n\nSince your 1/2 turns them from an actual problem into just harmless\ncruft, there's no real rush to get rid of them. Could we just do\nsomething like \"there is no matching pack file, and the mtime is 2 weeks\nold\"?\n\nIf there is a matching pack file, I don't think we want to get rid of\nthem. People can have .keep files if they want to indicate the pack\nshould be kept. I do admit it would be weird to write the \"receive-pack\"\nmessage into them, though.\n\n-Peff\n"},{"id":"164861","messageId":"201104011012.33926.johan@herland.net","threadId":"26941","inReplyTo":"20110401014150.GC21036@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 2/2] repack: Remove stale .keep files before repacking","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-04-01T08:12:33Z","receivedAt":"2011-04-01T08:12:33Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 01 April 2011, Jeff King wrote:\n> On Fri, Apr 01, 2011 at 03:34:27AM +0200, Johan Herland wrote:\n> > On Thursday 31 March 2011, Jeff King wrote:\n> > > On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:\n> > > > 3. Do I need to scan for and remove stale .keep files in a cron job\n> > > > in order to keep repos healthy and clonable?\n> > > \n> > > If we fix (1), then hopefully it is not as much of an issue. But\n> > > probably \"git gc\" should clean up stale ones after a while.\n> > \n> > This patch tries to automatically remove stale .keep files. However,\n> > it's still work-in-progress, as I don't know how to portably (a) ask\n> > for the current hostname (so that I can compare it to the one in the\n> > .keep file), or (b) test for whether a given PID is running on the\n> > system (to determine whether the receive-pack process that wrote the\n> > .keep file is still alive).\n> > \n> > Feedback appreciated.\n> \n> Since your 1/2 turns them from an actual problem into just harmless\n> cruft, there's no real rush to get rid of them. Could we just do\n> something like \"there is no matching pack file, and the mtime is 2 weeks\n> old\"?\n\nTrue, except that in the case I encountered (and reported) yesterday, I \nbelieve there _was_ a matching .pack file...\n\n> If there is a matching pack file, I don't think we want to get rid of\n> them.\n\nAFAICS, in this case we do.\n\n> People can have .keep files if they want to indicate the pack\n> should be kept. I do admit it would be weird to write the \"receive-pack\"\n> message into them, though.\n\nYeah, I don't think we have to worry about that, and even if we do, we are \nfree to change the \"receive-pack ...\" string into something far less likely \nto generate a false positive.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"164926","messageId":"7v1v1lfy7q.fsf@alter.siamese.dyndns.org","threadId":"26941","inReplyTo":"201104010329.05299.johan@herland.net","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-01T21:39:21Z","receivedAt":"2011-04-01T21:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> While pushing to a remote repo, Git transiently adds a .keep file for the\n> pack being pushed, to protect it from a concurrent \"git gc\". However, the\n> permissions on this .keep file are such that if a different user attempts\n> a local cross-filesystem clone (\"git clone --no-hardlinks\") on the server\n> while the .keep file is present (either because of a concurrent push, or\n> because of a prior failed push that left a stale .keep file), the clone\n> will fail because the second user cannot access the .keep file created by\n> the first user.\n\nWhile I am not sure if letting a clone proceed while there is a concurrent\npush is even a good idea to begin with, I agree that a stale .keep file is\na problem.\n\nI am kind of surprised that we are not using atexit(3) to clean them just\nlike we do for lockfiles; wouldn't that be a better solution?\n"},{"id":"164927","messageId":"20110401214115.GA18679@sigill.intra.peff.net","threadId":"26941","inReplyTo":"7v1v1lfy7q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-01T21:41:15Z","receivedAt":"2011-04-01T21:41:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2011 at 02:39:21PM -0700, Junio C Hamano wrote:\n\n> Johan Herland writes:\n> \n> > While pushing to a remote repo, Git transiently adds a .keep file for the\n> > pack being pushed, to protect it from a concurrent \"git gc\". However, the\n> > permissions on this .keep file are such that if a different user attempts\n> > a local cross-filesystem clone (\"git clone --no-hardlinks\") on the server\n> > while the .keep file is present (either because of a concurrent push, or\n> > because of a prior failed push that left a stale .keep file), the clone\n> > will fail because the second user cannot access the .keep file created by\n> > the first user.\n> \n> While I am not sure if letting a clone proceed while there is a concurrent\n> push is even a good idea to begin with, I agree that a stale .keep file is\n> a problem.\n> \n> I am kind of surprised that we are not using atexit(3) to clean them just\n> like we do for lockfiles; wouldn't that be a better solution?\n\nWe definitely should do that, but it would also be nice if a power\nfailure, kill -9, or segfault in receive-pack didn't leave a repo\nunusable.\n\n-Peff\n"},{"id":"164929","messageId":"AANLkTimMJxni=Vuja+iHyMp8ydm7ZBvWnkSe68g6+pyH@mail.gmail.com","threadId":"26941","inReplyTo":"7v1v1lfy7q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-04-01T21:49:41Z","receivedAt":"2011-04-01T21:49:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Apr 1, 2011 at 17:39, Junio C Hamano wrote:\n> While I am not sure if letting a clone proceed while there is a concurrent\n> push is even a good idea to begin with,\n\nWhat? Why?\n\nAre you suggesting that Git hosting sites disable readers while there\nis a push occurring?\n\nWe have tried hard to design Git to be concurrent reader/writer safe,\n*except* the actual garbage collection part of `git gc` that deletes\nloose objects. There is no reason to prevent concurrent readers while\nthere is a push in progress.\n\nThe only problem is a cpio based clone, which may link the objects\ndirectory before the refs, and miss linking the new pack but wind up\nlinking the new ref, making the clone corrupt. But that is a bug in\nthe cpio clone implementation. Using file:// to use the classical pipe\nis safe here, because the refs are scanned before the objects are.\nIMHO, if you think clone during push is unsafe because of this, we\nshould fix the cpio clone path to do a `git ls-remote` on the source,\ncache the refs in memory, copy the objects, then write out a\npacked-refs file containing the refs we snapshotted *before* linking\nthe objects directory into the new clone.\n\n-- \nShawn.\n"},{"id":"164932","messageId":"7vk4fdehps.fsf@alter.siamese.dyndns.org","threadId":"26941","inReplyTo":"AANLkTimMJxni=Vuja+iHyMp8ydm7ZBvWnkSe68g6+pyH@mail.gmail.com","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-01T22:21:03Z","receivedAt":"2011-04-01T22:21:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> On Fri, Apr 1, 2011 at 17:39, Junio C Hamano wrote:\n>> While I am not sure if letting a clone proceed while there is a concurrent\n>> push is even a good idea to begin with,\n>\n> What? Why?\n>\n> Are you suggesting that Git hosting sites disable readers while there\n> is a push occurring?\n\nThis is an irrelevant comment isn't it? Hosting sites coming via git\nprotocol will not suffer from this \"in-progress .keep not readable\" issue\nat all.\n\nI was responding to the motivation stated in the commit log message, the\nfile-based \"cp -r\" copy or cpio clone, which are _not_ a safe thing to do.\n\nBecause \"leftover .keep\" alone is a good justification, I was hinting to\ndrop that other motivation from the description altogether.\n"},{"id":"164936","messageId":"201104020127.41774.johan@herland.net","threadId":"26941","inReplyTo":"7vk4fdehps.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-04-01T23:27:41Z","receivedAt":"2011-04-01T23:27:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 02 April 2011, Junio C Hamano wrote:\n> I was responding to the motivation stated in the commit log message, the\n> file-based \"cp -r\" copy or cpio clone, which are _not_ a safe thing to\n> do.\n\nHmpf. I didn't know that clone --local --no-hardlinks was unsafe. If it's \nnot safe, should it still be the default behavior for a cross-filesystem \nclone?\n\nFurthermore, this lack of safety is not at all mentioned in the clone \ndocumentation...\n\n> Because \"leftover .keep\" alone is a good justification, I was hinting to\n> drop that other motivation from the description altogether.\n\nWhatever works best for you. What about this commit message instead?\n\n\nWhile pushing to a remote repo, Git transiently adds a .keep file for the\npack being pushed, to protect it from a concurrent \"git gc\". Sometimes, when \nthe push fails or is aborted, the .keep file is left stale in the repo. This \ncauses problems for other users of the same repo, since the permissions on \nthe .keep file (0600) make it inaccessible even though the rest of the repo \nis accessible (0444 modulo shared_repository setting).\n\nThere is no reason why the permission mode of a .keep file should be any\ndifferent from the permission mode of the corresponding .pack/.idx files.\nTherefore, adjust the permission of .keep files from 0600 to 0444 modulo\nthe shared_repository setting.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"164937","messageId":"201104020137.28951.johan@herland.net","threadId":"26941","inReplyTo":"AANLkTimMJxni=Vuja+iHyMp8ydm7ZBvWnkSe68g6+pyH@mail.gmail.com","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-04-01T23:37:28Z","receivedAt":"2011-04-01T23:37:28Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 01 April 2011, Shawn Pearce wrote:\n> The only problem is a cpio based clone, which may link the objects\n> directory before the refs, and miss linking the new pack but wind up\n> linking the new ref, making the clone corrupt. But that is a bug in\n> the cpio clone implementation. Using file:// to use the classical pipe\n> is safe here, because the refs are scanned before the objects are.\n> IMHO, if you think clone during push is unsafe because of this, we\n> should fix the cpio clone path to do a `git ls-remote` on the source,\n> cache the refs in memory, copy the objects, then write out a\n> packed-refs file containing the refs we snapshotted *before* linking\n> the objects directory into the new clone.\n\nLooking at clone_local() in builtin/clone.c (which I guess is the\nculprit here), wouldn't we fix this simply by swapping the two parts\nof the function, so that the refs part is done before the objects\npart? Like this:\n\n\nstatic const struct ref *clone_local(const char *src_repo,\n\t\t\t\t const char *dest_repo)\n{\n\tconst struct ref *ret;\n\tstruct strbuf src = STRBUF_INIT;\n\tstruct strbuf dest = STRBUF_INIT;\n\tstruct remote *remote;\n\tstruct transport *transport;\n\n\tremote = remote_get(src_repo);\n\ttransport = transport_get(remote, src_repo);\n\tret = transport_get_remote_refs(transport);\n\ttransport_disconnect(transport);\n\n\tif (option_shared)\n\t\tadd_to_alternates_file(src_repo);\n\telse {\n\t\tstrbuf_addf(&src, \"%s/objects\", src_repo);\n\t\tstrbuf_addf(&dest, \"%s/objects\", dest_repo);\n\t\tcopy_or_link_directory(&src, &dest);\n\t\tstrbuf_release(&src);\n\t\tstrbuf_release(&dest);\n\t}\n\n\tif (0 <= option_verbosity)\n\t\tprintf(\"done.\\n\");\n\treturn ret;\n}\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"164943","messageId":"7v39m1e110.fsf@alter.siamese.dyndns.org","threadId":"26941","inReplyTo":"201104020127.41774.johan@herland.net","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-02T04:21:31Z","receivedAt":"2011-04-02T04:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Hmpf. I didn't know that clone --local --no-hardlinks was unsafe. If it's \n> not safe, should it still be the default behavior for a cross-filesystem \n> clone?\n\nUnsafe is not quite the right word to use here in the sense that it\nwouldn't lead to any repository _corruption_ per-se, but if you ended up\ncopying such a transient .keep file, the pack will stay forever in your\nclone target unless you notice and remove it yourself.\n\nHaving said that, I expect that the majority of use of a filesystem level\nlocal clone these days is to clone your own repository, likely on your own\nmachine, and you have absolute control on both ends (e.g. you wouldn't be\nrunning a repack on the source while running a clone---you would more\nlikely to see the repack finish and then clone). So in that sense I would\nstill think that file level clone being the default on a local machine is\na reasonable default.\n\n> While pushing to a remote repo, Git transiently adds a .keep file for the\n> pack being pushed, to protect it from a concurrent \"git gc\". Sometimes, when \n> the push fails or is aborted, the .keep file is left stale in the repo. This \n> causes problems for other users of the same repo, since the permissions on \n> the .keep file (0600) make it inaccessible even though the rest of the repo \n> is accessible (0444 modulo shared_repository setting).\n\nI was also wondering why you initialized with 0444 in your patch and then\neven adjusted for shared repository settings.\n\nThis is a tangent, but wouldn't it be wrong for index-pack to always leave\nthe idx and pack files in 0444 with an explicit chmod() in the first\nplace? I suspect that we simply forgot to fix it when we introduced\nadjust_shared_perm().\n"},{"id":"164972","messageId":"201104030301.29923.johan@herland.net","threadId":"26941","inReplyTo":"7v39m1e110.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-04-03T01:01:29Z","receivedAt":"2011-04-03T01:01:29Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 02 April 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > While pushing to a remote repo, Git transiently adds a .keep file for\n> > the pack being pushed, to protect it from a concurrent \"git gc\".\n> > Sometimes, when the push fails or is aborted, the .keep file is left\n> > stale in the repo. This causes problems for other users of the same\n> > repo, since the permissions on the .keep file (0600) make it\n> > inaccessible even though the rest of the repo is accessible (0444\n> > modulo shared_repository setting).\n> \n> I was also wondering why you initialized with 0444 in your patch and then\n> even adjusted for shared repository settings.\n\nI was simply emulating what is currently done for idx and pack files (see \nbelow).\n\n> This is a tangent, but wouldn't it be wrong for index-pack to always\n> leave the idx and pack files in 0444 with an explicit chmod() in the\n> first place? I suspect that we simply forgot to fix it when we\n> introduced adjust_shared_perm().\n\nYeah, probablby, but AFAICS in the receive-pack case, final_pack_name and \nfinal_index_name are both NULL (neither are specified on the index-pack \ncommand line passed from receive-pack), so the explicit chmod(..., 0444) is \nnever called. Instead the pack and idx files are both opened from \nodb_mkstemp() (via open_pack_file() and write_idx_file(), respectively), \nwhich uses mode 0444. We then call move_temp_to_file(), which calls \nadjust_shared_perm().\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"}]}