{"thread":{"id":"34298","subject":"[PATCH] clone: Skip pack-*.keep files when cloning locally","startedAt":"2013-06-28T14:42:18Z","lastAt":"2013-07-03T18:31:24Z","messageCount":8,"participants":["Jens Lindstrom","Junio C Hamano","Jens Lindström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"222183","messageId":"1372430538-19216-1-git-send-email-jl@opera.com","threadId":"34298","inReplyTo":null,"subject":"[PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Jens Lindstrom","fromEmail":"jl@opera.com","sentAt":"2013-06-28T14:42:18Z","receivedAt":"2013-06-28T14:42:18Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"The pack-*.keep files are temporary, and serve no purpose in the\nclone. They would probably also never be deleted from the clone if\ncopied, since the process that created them only expects to have to\ndelete them from the original repository.\n\nWorse, though, they are created with access bits 0600, so if the\nuser trying to clone the repository is different from the user that\ncaused the pack-*.keep file to be created, the clone will likely\nfail due to not being allowed to read (and thus copy) the file in\nthe first place.\n\nSigned-off-by: Jens Lindstrom \n---\n builtin/clone.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 035ab64..0ec0ec9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -342,6 +342,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\t\tcontinue;\n \t\t}\n \n+\t\t/* Skip pack-*.keep files, they are temporary and not\n+\t\t relevant to the clone, and might not be accessible. */\n+\t\tif (!strcmp(src->buf + strlen(src->buf) - 5, \".keep\"))\n+\t\t\tcontinue;\n+\n \t\t/* Files that cannot be copied bit-for-bit... */\n \t\tif (!strcmp(src->buf + src_baselen, \"/info/alternates\")) {\n \t\t\tcopy_alternates(src, dest, src_repo);\n-- \n1.7.10.4\n"},{"id":"222199","messageId":"7vvc4ynkrx.fsf@alter.siamese.dyndns.org","threadId":"34298","inReplyTo":"1372430538-19216-1-git-send-email-jl@opera.com","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-28T18:38:10Z","receivedAt":"2013-06-28T18:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindstrom writes:\n\nHmph. I am of two minds here.\n\n> The pack-*.keep files are temporary, and serve no purpose in the\n> clone.\n\nThey are not temporary, actually. A user can deliberatey create a\n\"keep\" marker after packing with a good set of parameters, so that\nthe resulting pack will be kept, instead of letting a later repack\n(with faster set of parameters) remove and replace it with less\noptimal results.\n\n> Worse, though, they are created with access bits 0600, so if the\n> user trying to clone the repository is different from the user that\n> caused the pack-*.keep file to be created, the clone will likely\n> fail due to not being allowed to read (and thus copy) the file in\n> the first place.\n\nI am perfectly fine with a change that allows a local clone to skip\nand not error out when such a \"keep\" marker cannot be copied, I do\nnot know if it is a good idea to unconditionally skip and not even\nattempt to copy it.\n"},{"id":"222212","messageId":"7vvc4ym0mz.fsf@alter.siamese.dyndns.org","threadId":"34298","inReplyTo":"7vvc4ynkrx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-28T20:38:28Z","receivedAt":"2013-06-28T20:38:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am perfectly fine with a change that allows a local clone to skip\n> and not error out when such a \"keep\" marker cannot be copied, I do\n> not know if it is a good idea to unconditionally skip and not even\n> attempt to copy it.\n\nThat is, something like this, perhaps?\n\nWe could even create an empty file, as it is only the presense that\nmatters for \".keep\" files, but I found it a bit too much special\ncasing to my taste.\n\n builtin/clone.c | 9 ++++++++-\n t/t5701-clone-local.sh | 15 +++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 66bff57..4b7cd9b 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -356,8 +356,15 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\t\t\tdie_errno(_(\"failed to create link '%s'\"), dest->buf);\n \t\t\toption_no_hardlinks = 1;\n \t\t}\n-\t\tif (copy_file_with_time(dest->buf, src->buf, 0666))\n+\t\tif (copy_file_with_time(dest->buf, src->buf, 0666)) {\n+\t\t\tif (!strncmp(src->buf + src_baselen, \"/pack/pack-\", 11) &&\n+\t\t\t !suffixcmp(src->buf, \".keep\"))\n+\t\t\t\tgoto skip;\n+\n \t\t\tdie_errno(_(\"failed to copy file to '%s'\"), dest->buf);\n+\t\tskip:\n+\t\t\twarning(\"skipping %s\", src->buf);\n+\t\t}\n \t}\n \tclosedir(dir);\n }\ndiff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\nindex 7ff6e0e..bb5dddd 100755\n--- a/t/t5701-clone-local.sh\n+++ b/t/t5701-clone-local.sh\n@@ -134,4 +134,19 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '\n \t! repo_is_hardlinked force-nonlocal\n '\n \n+test_expect_success SANITY 'clone --no-hardlinks with unreadable .keep' '\n+\tmkdir strictsrc &&\n+\t(\n+\t\tcd strictsrc &&\n+\t\tgit init &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit repack -a -d &&\n+\t\tpackname=$(echo .git/objects/pack/pack-*.idx) &&\n+\t\tkeepname=${packname%.idx}.keep &&\n+\t\t>\"$keepname\" &&\n+\t\tchmod a= \"$keepname\"\n+\t) &&\n+\tgit clone --local --no-hardlinks strictsrc dst\n+'\n+\n test_done\n"},{"id":"222274","messageId":"CAEef6WzAg8-QMH1c4v=1tm7TPgfeE3W3K+ue-eYgRL3pyYo6Vg@mail.gmail.com","threadId":"34298","inReplyTo":"7vvc4ynkrx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-07-01T10:24:03Z","receivedAt":"2013-07-01T10:24:03Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Fri, Jun 28, 2013 at 8:38 PM, Junio C Hamano wrote:\n\n>> The pack-*.keep files are temporary, and serve no purpose in the\n>> clone.\n>\n> They are not temporary, actually. A user can deliberatey create a\n> \"keep\" marker after packing with a good set of parameters, so that\n> the resulting pack will be kept, instead of letting a later repack\n> (with faster set of parameters) remove and replace it with less\n> optimal results.\n\nAh, I see. Was (obviously) not aware of that. It would perhaps be a\ngood idea to be able to differentiate between such permanent keep\nfiles and the temporary ones created by built-in commands.\n\nAlso, even if some keep files are permanent in the source repository,\nis it always a good idea to copy them over to the clone? This would\nonly happen for some types of clones, anyway.\n\n\nOn Fri, Jun 28, 2013 at 10:38 PM, Junio C Hamano wrote:\n\n> That is, something like this, perhaps?\n\nComments:\n\nWith this patch, it still fails with --local, when the link() call\nfails. This seems a bit odd, in particular in the cases where --local\nis implied. IOW, one would not expect that adding --local would\nchange behavior, but here adding it causes the operation to fail.\n\nAlso, since failing to link() once implicitly enables --no-hardlinks,\nit would copy the rest of the repository without trying to use link(),\nwhich might make the whole operation much more expensive.\n\nApplying the exception for inaccessible .keep files for link() as well\nwould seem a better solution to me.\n"},{"id":"222279","messageId":"7vip0ui757.fsf@alter.siamese.dyndns.org","threadId":"34298","inReplyTo":"CAEef6WzAg8-QMH1c4v=1tm7TPgfeE3W3K+ue-eYgRL3pyYo6Vg@mail.gmail.com","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-01T16:20:36Z","receivedAt":"2013-07-01T16:20:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström writes:\n\n> On Fri, Jun 28, 2013 at 8:38 PM, Junio C Hamano wrote:\n>\n>>> The pack-*.keep files are temporary, and serve no purpose in the\n>>> clone.\n>>\n>> They are not temporary, actually. A user can deliberatey create a\n>> \"keep\" marker after packing with a good set of parameters, so that\n>> the resulting pack will be kept, instead of letting a later repack\n>> (with faster set of parameters) remove and replace it with less\n>> optimal results.\n>\n> Ah, I see. Was (obviously) not aware of that. It would perhaps be a\n> good idea to be able to differentiate between such permanent keep\n> files and the temporary ones created by built-in commands.\n\nI am not sure if we should care that deeply about them in the first\nplace. For these temporary \".keep\" lockfiles to matter, you have to\nbe doing \"clone --local --no-hardlinks\", which is a \"cp -R\" that\nbypasses the usual Git transport mechanism, while there is still\nactivity in the source repository (a fetch is slurping new objects\ninto a newly created pack with such a temporary \".keep\" lockfile,\nthe refs may not have been updated yet). The result is expected to\nhave inconsistencies even if \".keep\" were readable in such a copy.\n\n> Also, even if some keep files are permanent in the source repository,\n> is it always a good idea to copy them over to the clone?\n\nThe local \"cheap cp -R clone\" are primarily used by people copying\ntheir own private repository that is in a quiescent state, and in\nthat set-up, copying \".keep\" by default has been a good idea. Of\ncourse, after copying to a new repository, if they want to repack\nwith different parameters, they need to unplug \".keep\" files\nmanually, and it can be argued that the default could have been not\nto copy and we could have forced those who want to reuse a tight\npack they created earlier to manually copy or create the \".keep\"\nfiles instead.\n>\n> On Fri, Jun 28, 2013 at 10:38 PM, Junio C Hamano wrote:\n>\n>> That is, something like this, perhaps?\n>\n> Comments:\n>\n> With this patch, it still fails with --local, when the link() call\n> fails.\n\nOh, I wasn't even attempting to look at the hardlink codepath in the\n\"something like this\" illustration patch ;-).\n\nBesides, I think you can make a hardlink to a file that you cannot\nread.\n\nIf you apply only the parts of the patch that adds the test, without\napplying either your patch or mine to builtin/clone.c, and remove\n\"--no-hardlinks\" to force the hardlink codepath, what happens? We\ncreate an unreadable $keepname in strictsrc repository and I think\nthe local clone (which is not \"cp -R\" but something like \"find\"\npiped to \"cpio -pluadm\") would make a \"copy\" of it just fine.\n"},{"id":"222455","messageId":"CAEef6WxnRbPVFgZP4asQrvYGVTTbGzeQCwPJrwj4a-6k9vFcbQ@mail.gmail.com","threadId":"34298","inReplyTo":"7vip0ui757.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-07-03T10:02:49Z","receivedAt":"2013-07-03T10:02:49Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Mon, Jul 1, 2013 at 6:20 PM, Junio C Hamano wrote:\n> I am not sure if we should care that deeply about them in the first\n> place.\n\nFine by me; I don't really have a strong opinion on the matter.\n\n> Besides, I think you can make a hardlink to a file that you cannot\n> read.\n\nNot always. The Linux kernel can at least be configured not to allow\nit. It seems this is enabled by default in at least Debian.\n\nThis restriction had me a bit confused when I was testing variations\nhere; I expected all \"access denied\" failures to be because of .keep\nfiles, but in fact creating hardlinks to other files (.idx and .pack)\nfailed too, even though they were readable. This caused \"git clone\n--local\" to fail, while \"git clone\" succeeded after falling back to\ncopying instead of linking.\n"},{"id":"222471","messageId":"7vzju3ftbo.fsf@alter.siamese.dyndns.org","threadId":"34298","inReplyTo":"CAEef6WxnRbPVFgZP4asQrvYGVTTbGzeQCwPJrwj4a-6k9vFcbQ@mail.gmail.com","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-03T17:26:35Z","receivedAt":"2013-07-03T17:26:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström writes:\n\n> On Mon, Jul 1, 2013 at 6:20 PM, Junio C Hamano wrote:\n>> I am not sure if we should care that deeply about them in the first\n>> place.\n>\n> Fine by me; I don't really have a strong opinion on the matter.\n>\n>> Besides, I think you can make a hardlink to a file that you cannot\n>> read.\n>\n> Not always. The Linux kernel can at least be configured not to allow\n> it. It seems this is enabled by default in at least Debian.\n\nYou learn a new thing every day, I guess. I am on Debian, I do not\nthink I did any customization in that area, and I can hardlink just\nfine.\n\n> This restriction had me a bit confused when I was testing variations\n> here; I expected all \"access denied\" failures to be because of .keep\n> files, but in fact creating hardlinks to other files (.idx and .pack)\n> failed too, even though they were readable. \n\nIs it possible that you are tripping cross-device link? The reason\nwhy we have \"attempt to hardlink but fall back to copy\" is exactly\nbecause it is fairly common that people try local-cheap clone without\nrealizing the source and the destination may be on separate filesystems.\n"},{"id":"222478","messageId":"CAEef6WyMVEM0Rj5Fj7BWeP72au9ynnP20Go3FhSzJKSkdcb-aA@mail.gmail.com","threadId":"34298","inReplyTo":"7vzju3ftbo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] clone: Skip pack-*.keep files when cloning locally","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-07-03T18:31:24Z","receivedAt":"2013-07-03T18:31:24Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Wed, Jul 3, 2013 at 7:26 PM, Junio C Hamano wrote:\n> Jens Lindström writes:\n>\n>> Not always. The Linux kernel can at least be configured not to allow\n>> it. It seems this is enabled by default in at least Debian.\n>\n> You learn a new thing every day, I guess. I am on Debian, I do not\n> think I did any customization in that area, and I can hardlink just\n> fine.\n\nTo configure it, write \"1\" (on) or \"0\" (off) to\n/proc/sys/fs/protected_{hard,sym}links. I can't remember (or imagine)\nthat I enabled it on any of my systems. One of my systems is Debian\nSqueeze with a 2.6.32 kernel, and it doesn't have those files, so I\nguess it might have been added in some more recent kernel version.\n\n>> This restriction had me a bit confused when I was testing variations\n>> here; I expected all \"access denied\" failures to be because of .keep\n>> files, but in fact creating hardlinks to other files (.idx and .pack)\n>> failed too, even though they were readable.\n>\n> Is it possible that you are tripping cross-device link? The reason\n> why we have \"attempt to hardlink but fall back to copy\" is exactly\n> because it is fairly common that people try local-cheap clone without\n> realizing the source and the destination may be on separate filesystems.\n\nNo, I was certainly cloning within a single file system, and I can\nconfirm that a plain \"ln src dest\" command fails unless the user can\nboth read and write the source file. So if the cloning user has\nread-only access to the repository, copying will work and linking\nwon't (depending on the kernel,) in which case it of course is\nexcellent that git falls back to copying instead of linking.\n"}]}