{"thread":{"id":"13370","subject":"[PATCH] Be more careful with objects directory permissions on clone","startedAt":"2008-05-04T11:37:00Z","lastAt":"2008-05-06T08:08:58Z","messageCount":10,"participants":["Mark Hills","Johannes Sixt","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"76015","messageId":"alpine.BSO.1.10.0805041234570.5819@zrgural.vwaro.pbz","threadId":"13370","inReplyTo":null,"subject":"[PATCH] Be more careful with objects directory permissions on clone","fromName":"Mark Hills","fromEmail":"mark@pogo.org.uk","sentAt":"2008-05-04T11:37:00Z","receivedAt":"2008-05-04T11:37:00Z","isPatch":true,"sender":{"key":"mark@pogo.org.uk","avatar":null},"body":"Honour the setgid and umask when re-creating the objects directory\nat the destination.\n\ncpio in copy-pass mode aims to copy file permissions which causes this\nproblem and cannot be disabled. Be explicit by copying the directory\nstructure first, honouring the permissions at the destination, then copy\nthe files with their existing read-only permissions.\n\nSigned-off-by: Mark Hills <mark@pogo.org.uk>\n---\n  git-clone.sh |    5 ++++-\n  1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/git-clone.sh b/git-clone.sh\nindex 8c7fc7f..53c7e06 100755\n--- a/git-clone.sh\n+++ b/git-clone.sh\n@@ -334,7 +334,10 @@ yes)\n  \t\t\tfi\n  \t\tfi &&\n  \t\tcd \"$repo\" &&\n-\t\tfind objects -depth -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n+\t\t# Create dirs using umask and permissions and destination\n+\t\tfind objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n+\t\t# Copy 0444 permissions on files\n+\t\tfind objects -type f -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n  \t\t\texit 1\n  \tfi\n  \tgit-ls-remote \"$repo\" >\"$GIT_DIR/CLONE_HEAD\" || exit 1\n-- \n1.5.5.1.126.g48d0c\n"},{"id":"76093","messageId":"481ECCF0.6080308@viscovery.net","threadId":"13370","inReplyTo":"alpine.BSO.1.10.0805041234570.5819@zrgural.vwaro.pbz","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-05T09:01:36Z","receivedAt":"2008-05-05T09:01:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mark Hills schrieb:\n> Honour the setgid and umask when re-creating the objects directory\n> at the destination.\n> \n> cpio in copy-pass mode aims to copy file permissions which causes this\n> problem and cannot be disabled. Be explicit by copying the directory\n> structure first, honouring the permissions at the destination, then copy\n> the files with their existing read-only permissions.\n...\n>          cd \"$repo\" &&\n> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l\n> \"$GIT_DIR/\" || \\\n> +        # Create dirs using umask and permissions and destination\n> +        find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n> +        # Copy 0444 permissions on files\n> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l\n> \"$GIT_DIR/\" || \\\n\nWouldn't that be better:\n\n\tfind objects ! -type d -print | cpio ...\n\n?\n\n-- Hannes\n"},{"id":"76106","messageId":"alpine.BSO.1.10.0805051047260.7723@zrgural.vwaro.pbz","threadId":"13370","inReplyTo":"481ECCF0.6080308@viscovery.net","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Mark Hills","fromEmail":"mark@pogo.org.uk","sentAt":"2008-05-05T09:56:57Z","receivedAt":"2008-05-05T09:56:57Z","isPatch":true,"sender":{"key":"mark@pogo.org.uk","avatar":null},"body":"On Mon, 5 May 2008, Johannes Sixt wrote:\n\n> Mark Hills schrieb:\n>> Honour the setgid and umask when re-creating the objects directory\n>> at the destination.\n>>\n>> cpio in copy-pass mode aims to copy file permissions which causes this\n>> problem and cannot be disabled. Be explicit by copying the directory\n>> structure first, honouring the permissions at the destination, then copy\n>> the files with their existing read-only permissions.\n> ...\n>>          cd \"$repo\" &&\n>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l\n>> \"$GIT_DIR/\" || \\\n>> +        # Create dirs using umask and permissions and destination\n>> +        find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n>> +        # Copy 0444 permissions on files\n>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l\n>> \"$GIT_DIR/\" || \\\n>\n> Wouldn't that be better:\n>\n> \tfind objects ! -type d -print | cpio ...\n>\n> ?\n\nThis was my first suggestion, unfortunately it shows up broken behaviour \nin all but the latest version of cpio. It creates 0700 directory \npermissions which is even worse.\n\nI didn't find the later versions of cpio to be widespread, so I wrote the \npatch to work in all cases.\n\nSee the thread 'git-clone file permissions and cpio'.\n\nMark\n"},{"id":"76107","messageId":"481EDC4B.2010105@viscovery.net","threadId":"13370","inReplyTo":"alpine.BSO.1.10.0805051047260.7723@zrgural.vwaro.pbz","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-05T10:07:07Z","receivedAt":"2008-05-05T10:07:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mark Hills schrieb:\n> On Mon, 5 May 2008, Johannes Sixt wrote:\n> \n>> Mark Hills schrieb:\n>>>          cd \"$repo\" &&\n>>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l\n>>> \"$GIT_DIR/\" || \\\n>>> +        # Create dirs using umask and permissions and destination\n>>> +        find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir\n>>> -p) &&\n>>> +        # Copy 0444 permissions on files\n>>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l\n>>> \"$GIT_DIR/\" || \\\n>>\n>> Wouldn't that be better:\n>>\n>>     find objects ! -type d -print | cpio ...\n>>\n>> ?\n> \n> This was my first suggestion, unfortunately it shows up broken behaviour\n> in all but the latest version of cpio. It creates 0700 directory\n> permissions which is even worse.\n\nSorry, I should have mentioned that I meant to keep the 'find | xargs\nmkdir' and replace only the second 'find | cpio'.\n\n-- Hannes\n"},{"id":"76108","messageId":"alpine.BSO.1.10.0805051121090.32248@zrgural.vwaro.pbz","threadId":"13370","inReplyTo":"481EDC4B.2010105@viscovery.net","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Mark Hills","fromEmail":"mark@pogo.org.uk","sentAt":"2008-05-05T10:25:15Z","receivedAt":"2008-05-05T10:25:15Z","isPatch":true,"sender":{"key":"mark@pogo.org.uk","avatar":null},"body":"On Mon, 5 May 2008, Johannes Sixt wrote:\n\n> Mark Hills schrieb:\n>> On Mon, 5 May 2008, Johannes Sixt wrote:\n>>\n>>> Mark Hills schrieb:\n>>>>          cd \"$repo\" &&\n>>>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l\n>>>> \"$GIT_DIR/\" || \\\n>>>> +        # Create dirs using umask and permissions and destination\n>>>> +        find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir\n>>>> -p) &&\n>>>> +        # Copy 0444 permissions on files\n>>>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l\n>>>> \"$GIT_DIR/\" || \\\n>>>\n>>> Wouldn't that be better:\n>>>\n>>>     find objects ! -type d -print | cpio ...\n>>>\n>>> ?\n>>\n>> This was my first suggestion, unfortunately it shows up broken behaviour\n>> in all but the latest version of cpio. It creates 0700 directory\n>> permissions which is even worse.\n>\n> Sorry, I should have mentioned that I meant to keep the 'find | xargs\n> mkdir' and replace only the second 'find | cpio'.\n\nAh, I see.\n\nI tend to think that an inclusive match has less scope for future bad \nbehaviour than an exclusive match. Have I missed the possiblity that there \nmay be content in the objects directory which is not a directory or file?\n\nMark\n"},{"id":"76110","messageId":"481EE478.9000704@viscovery.net","threadId":"13370","inReplyTo":"alpine.BSO.1.10.0805051121090.32248@zrgural.vwaro.pbz","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-05T10:42:00Z","receivedAt":"2008-05-05T10:42:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Mark Hills schrieb:\n> On Mon, 5 May 2008, Johannes Sixt wrote:\n> \n>> Mark Hills schrieb:\n>>> On Mon, 5 May 2008, Johannes Sixt wrote:\n>>>\n>>>> Mark Hills schrieb:\n>>>>>          cd \"$repo\" &&\n>>>>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l\n>>>>> \"$GIT_DIR/\" || \\\n>>>>> +        # Create dirs using umask and permissions and destination\n>>>>> +        find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir\n>>>>> -p) &&\n>>>>> +        # Copy 0444 permissions on files\n>>>>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l\n>>>>> \"$GIT_DIR/\" || \\\n>>>>\n>>>> Wouldn't that be better:\n>>>>\n>>>>     find objects ! -type d -print | cpio ...\n>>>>\n>>>> ?\n>>>\n>>> This was my first suggestion, unfortunately it shows up broken behaviour\n>>> in all but the latest version of cpio. It creates 0700 directory\n>>> permissions which is even worse.\n>>\n>> Sorry, I should have mentioned that I meant to keep the 'find | xargs\n>> mkdir' and replace only the second 'find | cpio'.\n> \n> Ah, I see.\n> \n> I tend to think that an inclusive match has less scope for future bad\n> behaviour than an exclusive match. Have I missed the possiblity that\n> there may be content in the objects directory which is not a directory\n> or file?\n\nTheoretically, you could have symbolic links to loose objects or packs.\n\nGenerally, you shouldn't change the behavior of a program more than necessary.\n\n-- Hannes\n"},{"id":"76141","messageId":"alpine.BSO.1.10.0805051742020.10940@zrgural.vwaro.pbz","threadId":"13370","inReplyTo":"481EE478.9000704@viscovery.net","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Mark Hills","fromEmail":"mark@pogo.org.uk","sentAt":"2008-05-05T16:46:42Z","receivedAt":"2008-05-05T16:46:42Z","isPatch":true,"sender":{"key":"mark@pogo.org.uk","avatar":null},"body":"On Mon, 5 May 2008, Johannes Sixt wrote:\n\n> Mark Hills schrieb:\n\n>> I tend to think that an inclusive match has less scope for future bad \n>> behaviour than an exclusive match. Have I missed the possiblity that \n>> there may be content in the objects directory which is not a directory \n>> or file?\n>\n> Theoretically, you could have symbolic links to loose objects or packs.\n>\n> Generally, you shouldn't change the behavior of a program more than \n> necessary.\n\nOkay, here's a revised patch (I made a note about the cpio bug in the \ncommit message too).\n\nThanks,\n\nMark\n\n\n\nBe more careful with objects directory permissions on clone\n\nHonour the setgid and umask when re-creating the objects directory\nat the destination.\n\ncpio in copy-pass mode aims to copy file permissions which causes this\nproblem and cannot be disabled. Be explicit by copying the directory\nstructure first, honouring the permissions at the destination, then copy\nthe files with 0444 permissions. This also avoids bugs in some versions\nof cpio.\n\nSigned-off-by: Mark Hills <mark@pogo.org.uk>\n---\n  git-clone.sh |    5 ++++-\n  1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/git-clone.sh b/git-clone.sh\nindex 8c7fc7f..9d88d1c 100755\n--- a/git-clone.sh\n+++ b/git-clone.sh\n@@ -334,7 +334,10 @@ yes)\n  \t\t\tfi\n  \t\tfi &&\n  \t\tcd \"$repo\" &&\n-\t\tfind objects -depth -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n+\t\t# Create dirs using umask and permissions and destination\n+\t\tfind objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n+\t\t# Copy existing 0444 permissions on content\n+\t\tfind objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n  \t\t\texit 1\n  \tfi\n  \tgit-ls-remote \"$repo\" >\"$GIT_DIR/CLONE_HEAD\" || exit 1\n-- \n1.5.5.1.126.geeac\n"},{"id":"76183","messageId":"7vprrzhqk3.fsf@gitster.siamese.dyndns.org","threadId":"13370","inReplyTo":"alpine.BSO.1.10.0805051742020.10940@zrgural.vwaro.pbz","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-06T06:53:00Z","receivedAt":"2008-05-06T06:53:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Hills <mark@pogo.org.uk> writes:\n\n> diff --git a/git-clone.sh b/git-clone.sh\n> index 8c7fc7f..9d88d1c 100755\n> --- a/git-clone.sh\n> +++ b/git-clone.sh\n> @@ -334,7 +334,10 @@ yes)\n>  \t\t\tfi\n>  \t\tfi &&\n>  \t\tcd \"$repo\" &&\n> -\t\tfind objects -depth -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n> +\t\t# Create dirs using umask and permissions and destination\n> +\t\tfind objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n> +\t\t# Copy existing 0444 permissions on content\n> +\t\tfind objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n>  \t\t\texit 1\n\nLooks much better.  Thanks.\n"},{"id":"76185","messageId":"fvp14f$8v0$1@ger.gmane.org","threadId":"13370","inReplyTo":"alpine.BSO.1.10.0805051742020.10940@zrgural.vwaro.pbz","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-05-06T07:25:03Z","receivedAt":"2008-05-06T07:25:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Mark Hills wrote:\n\n> -               find objects -depth -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n> +               # Create dirs using umask and permissions and destination\n> +               find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n> +               # Copy existing 0444 permissions on content\n> +               find objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n\nBy the way, it is important that previous version had -depth, and\nproposed one doesn't? Was it about creating directories before files?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"76186","messageId":"4820121A.9090606@viscovery.net","threadId":"13370","inReplyTo":"fvp14f$8v0$1@ger.gmane.org","subject":"Re: [PATCH] Be more careful with objects directory permissions on clone","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-06T08:08:58Z","receivedAt":"2008-05-06T08:08:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jakub Narebski schrieb:\n> Mark Hills wrote:\n> \n>> -               find objects -depth -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n>> +               # Create dirs using umask and permissions and destination\n>> +               find objects -type d -print | (cd \"$GIT_DIR\" && xargs mkdir -p) &&\n>> +               # Copy existing 0444 permissions on content\n>> +               find objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l \"$GIT_DIR/\" || \\\n> \n> By the way, it is important that previous version had -depth, and\n> proposed one doesn't? Was it about creating directories before files?\n\n-depth means that directory names are listed *after* their content.\nConsequently, it was about setting modification times and permissions on\ndirectories *after* all of their content is created at the destination.\n\nThe intent of the new version is to not copy permissions of directories;\nand since the modification times don't matter, the absence of -depth is ok.\n\n-- Hannes\n"}]}