{"thread":{"id":"53101","subject":"Re: [PATCH] fetch: allow running as different users in shared repositories","startedAt":"2020-03-26T01:09:56Z","lastAt":"2020-05-05T00:08:10Z","messageCount":8,"participants":["Vadim Zeitlin","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394036","messageId":"E1jHGdD-00079b-06@smtp.tt-solutions.com","threadId":"53101","inReplyTo":null,"subject":"Re: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2020-03-26T00:44:23Z","receivedAt":"2020-03-26T01:09:56Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"On Wed, 25 Mar 2020 20:04:09 +0100 Johannes Schindelin wrote:\n\nJS> Hi Vadim,\n\n Hello Johannes and thanks for your reply!\n\nJS> On Thu, 19 Mar 2020, Vadim Zeitlin wrote:\nJS> \nJS> > The function fopen_for_writing(), which was added in 79d7582e32 (commit:\nJS> > allow editing the commit message even in shared repos, 2016-01-06) and\nJS> > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file\nJS> > writes correctly in shared repos, 2016-01-11), didn't do it correctly in\nJS> > shared repositories under Linux.\nJS> >\nJS> > This happened because in this situation the file FETCH_HEAD has mode 644\nJS> \nJS> I wonder why that is. In a shared repository, it should have mode 664, I\nJS> thought.\n\n This file is created using a simple fopen(\"w\") and so is subject to umask.\nWith the usual default umask value (022) its mode would be 644, regardless\nof the repository settings.\n\n[...snip my original description...]\nJS> That rationale makes sense to me, as does the patch.\n\n Sorry for a possibly stupid question, but what is the next thing to do\nnow? The instructions in Documentation/SubmittingPatches indicate that I\nshould wait until the \"list forms consensus that [...] your patch is good\",\nbut it's not quite clear what indicates that a consensus has been reached.\nIs your comment above enough or should I wait for something else? And\nif/when it has been reached, do I really I need to resend the patch to\nthe maintainer and cc the list as written in that document? I'm a bit\nsurprised by this because I don't see (most) patches being resent to this\nlist.\n\n This is obviously very non-urgent, but I'd just like to understand what,\nif anything, is expected from me.\n\n Thanks in advance for your guidance!\nVZ\n"},{"id":"394095","messageId":"nycvar.QRO.7.76.6.2003261538170.46@tvgsbejvaqbjf.bet","threadId":"53101","inReplyTo":"E1jHGdD-00079b-06@smtp.tt-solutions.com","subject":"Re: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-26T14:40:47Z","receivedAt":"2020-03-26T14:40:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Vadim,\n\nOn Thu, 26 Mar 2020, Vadim Zeitlin wrote:\n\n> On Wed, 25 Mar 2020 20:04:09 +0100 Johannes Schindelin wrote:\n>\n> JS> Hi Vadim,\n>\n> Hello Johannes and thanks for your reply!\n>\n> JS> On Thu, 19 Mar 2020, Vadim Zeitlin wrote:\n> JS>\n> JS> > The function fopen_for_writing(), which was added in 79d7582e32 (commit:\n> JS> > allow editing the commit message even in shared repos, 2016-01-06) and\n> JS> > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file\n> JS> > writes correctly in shared repos, 2016-01-11), didn't do it correctly in\n> JS> > shared repositories under Linux.\n> JS> >\n> JS> > This happened because in this situation the file FETCH_HEAD has mode 644\n> JS>\n> JS> I wonder why that is. In a shared repository, it should have mode 664, I\n> JS> thought.\n>\n> This file is created using a simple fopen(\"w\") and so is subject to umask.\n> With the usual default umask value (022) its mode would be 644, regardless\n> of the repository settings.\n\nMaybe we should change that to an `open()` call with the explicit `0666`\nmode?\n\n> [...snip my original description...]\n> JS> That rationale makes sense to me, as does the patch.\n>\n> Sorry for a possibly stupid question, but what is the next thing to do\n> now? The instructions in Documentation/SubmittingPatches indicate that I\n> should wait until the \"list forms consensus that [...] your patch is good\",\n> but it's not quite clear what indicates that a consensus has been reached.\n> Is your comment above enough or should I wait for something else? And\n> if/when it has been reached, do I really I need to resend the patch to\n> the maintainer and cc the list as written in that document? I'm a bit\n> surprised by this because I don't see (most) patches being resent to this\n> list.\n\nMy take is that this was waiting for a review, and I provided it (*not*\nasking for any changes), and if there are no further reviews, the patch\nshould make it into the `pu` branch, then `next` and eventually `master`,\nat which point it will be slated for the next official `.0` version.\n\nIt might make sense to ask for it to be trickled down into the `maint`\nbranch, too, in case a `v2.26.1` is released. I would be in favor of that,\nbut would not do the asking myself ;-)\n\nCiao,\nJohannes\n\n>\n> This is obviously very non-urgent, but I'd just like to understand what,\n> if anything, is expected from me.\n>\n> Thanks in advance for your guidance!\n> VZ\n>\n"},{"id":"394126","messageId":"E1jHb2f-0004m5-TH@smtp.tt-solutions.com","threadId":"53101","inReplyTo":"nycvar.QRO.7.76.6.2003261538170.46@tvgsbejvaqbjf.bet","subject":"Re[2]: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2020-03-26T22:32:01Z","receivedAt":"2020-03-26T22:32:05Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"On Thu, 26 Mar 2020 15:40:47 +0100 (CET) Johannes Schindelin wrote:\n\nJS> On Thu, 26 Mar 2020, Vadim Zeitlin wrote:\nJS> \nJS> > On Wed, 25 Mar 2020 20:04:09 +0100 Johannes Schindelin wrote:\nJS> >\nJS> > JS> Hi Vadim,\nJS> >\nJS> > Hello Johannes and thanks for your reply!\nJS> >\nJS> > JS> On Thu, 19 Mar 2020, Vadim Zeitlin wrote:\nJS> > JS>\nJS> > JS> > The function fopen_for_writing(), which was added in 79d7582e32 (commit:\nJS> > JS> > allow editing the commit message even in shared repos, 2016-01-06) and\nJS> > JS> > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file\nJS> > JS> > writes correctly in shared repos, 2016-01-11), didn't do it correctly in\nJS> > JS> > shared repositories under Linux.\nJS> > JS> >\nJS> > JS> > This happened because in this situation the file FETCH_HEAD has mode 644\nJS> > JS>\nJS> > JS> I wonder why that is. In a shared repository, it should have mode 664, I\nJS> > JS> thought.\nJS> >\nJS> > This file is created using a simple fopen(\"w\") and so is subject to umask.\nJS> > With the usual default umask value (022) its mode would be 644, regardless\nJS> > of the repository settings.\nJS> \nJS> Maybe we should change that to an `open()` call with the explicit `0666`\nJS> mode?\n\n Hello again,\n\n Sorry if I'm missing something, but AFAICS this wouldn't change anything,\nopen() mode argument is still combined with the (negated) umask, and 0666 &\n!022 would still give 0644. The only ways to give this file the mode\nof 664 that I know about are to either temporarily reset the \"group\" byte\nof umask to 0 or to explicitly call [f]chmod() after creating it. I don't\nknow if this is really worthwhile to do...\n\nJS> My take is that this was waiting for a review, and I provided it (*not*\nJS> asking for any changes), and if there are no further reviews, the patch\nJS> should make it into the `pu` branch, then `next` and eventually `master`,\nJS> at which point it will be slated for the next official `.0` version.\n\n OK, thanks (both for the review and for the explanations)!\n\nJS> It might make sense to ask for it to be trickled down into the `maint`\nJS> branch, too, in case a `v2.26.1` is released. I would be in favor of that,\nJS> but would not do the asking myself ;-)\n\n This is not really urgent to me, so I don't think I want to bother people\nwith backporting it to `maint` neither, even if I definitely wouldn't have\nany objections to this. I'd just like this to work in some future version\nof Git without the workaround we have to use right now (which basically\nconsists in running chmod manually) in the bright future when we upgrade to\nit.\n\n Thanks again,\nVZ"},{"id":"396806","messageId":"E1jUeoi-000205-RT@smtp.tt-solutions.com","threadId":"53101","inReplyTo":"nycvar.QRO.7.76.6.2003261538170.46@tvgsbejvaqbjf.bet","subject":"Re[2]: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2020-05-01T23:11:36Z","receivedAt":"2020-05-01T23:39:55Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"On Thu, 26 Mar 2020 15:40:47 +0100 (CET) Johannes Schindelin wrote:\n\nJS> On Thu, 26 Mar 2020, Vadim Zeitlin wrote:\nJS> \nJS> > On Wed, 25 Mar 2020 20:04:09 +0100 Johannes Schindelin wrote:\n[...]\nJS> > JS> That rationale makes sense to me, as does the patch.\nJS> >\nJS> > Sorry for a possibly stupid question, but what is the next thing to do\nJS> > now? The instructions in Documentation/SubmittingPatches indicate that I\nJS> > should wait until the \"list forms consensus that [...] your patch is good\",\nJS> > but it's not quite clear what indicates that a consensus has been reached.\nJS> > Is your comment above enough or should I wait for something else? And\nJS> > if/when it has been reached, do I really I need to resend the patch to\nJS> > the maintainer and cc the list as written in that document? I'm a bit\nJS> > surprised by this because I don't see (most) patches being resent to this\nJS> > list.\nJS> \nJS> My take is that this was waiting for a review, and I provided it (*not*\nJS> asking for any changes), and if there are no further reviews, the patch\nJS> should make it into the `pu` branch, then `next` and eventually `master`,\nJS> at which point it will be slated for the next official `.0` version.\nJS> \nJS> It might make sense to ask for it to be trickled down into the `maint`\nJS> branch, too, in case a `v2.26.1` is released. I would be in favor of that,\nJS> but would not do the asking myself ;-)\n\n Hello again,\n\n Sorry to nag, but I'd like to return to this patch[*] because it looks\nlike it could have fallen through the cracks: there didn't seem to be any\nmore comments about it, except for Johannes' positive review, but it didn't\nget mentioned in any \"What's cooking\" threads since then neither.\n\n[*] https://public-inbox.org/git/20200319010321.18614-1-vz-git@zeitlins.org/\n\n\n So I'd just like to ask directly, hoping that it's not inappropriate:\nJunio, do I need to do anything to get this patch accepted or am I just\nbeing too impatient?\n\n Thanks in advance,\nVZ"},{"id":"396926","messageId":"xmqqr1vzhd8z.fsf@gitster.c.googlers.com","threadId":"53101","inReplyTo":"E1jUeoi-000205-RT@smtp.tt-solutions.com","subject":"Re: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T16:32:44Z","receivedAt":"2020-05-04T16:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vadim Zeitlin writes:\n\n> So I'd just like to ask directly, hoping that it's not inappropriate:\n> Junio, do I need to do anything to get this patch accepted or am I just\n> being too impatient?\n\nI do not even recall seeing the discussion, so you are right to\nsuspect that it fell thru the cracks, and it is quite appropriate to\nping the thread directly like you did. Mind resending the patch to\nthe list, just to make sure nobody else sees any problems with it?\n\nThanks.\n"},{"id":"396933","messageId":"E1jVeWV-0006Sj-JC@smtp.tt-solutions.com","threadId":"53101","inReplyTo":"xmqqr1vzhd8z.fsf@gitster.c.googlers.com","subject":"Re[2]: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2020-05-04T17:04:55Z","receivedAt":"2020-05-04T17:05:01Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"On Mon, 04 May 2020 09:32:44 -0700 Junio C Hamano wrote:\n\nJCH> Vadim Zeitlin writes:\nJCH> \nJCH> > So I'd just like to ask directly, hoping that it's not inappropriate:\nJCH> > Junio, do I need to do anything to get this patch accepted or am I just\nJCH> > being too impatient?\nJCH> \nJCH> I do not even recall seeing the discussion, so you are right to\nJCH> suspect that it fell thru the cracks, and it is quite appropriate to\nJCH> ping the thread directly like you did. Mind resending the patch to\nJCH> the list, just to make sure nobody else sees any problems with it?\n\n Hello,\n\n Thanks for your reply and here is the patch, with its commit message and\nthe extra notes about it, as it was sent initially. As you can see, it's a\npretty trivial change, I'm mostly just puzzled how did it go unnoticed\nsince ~4 years and was afraid I could be missing something, but it finally\nseems like my use case, i.e. calling git-fetch in shared repositories, is\njust much more rare than I thought.\n\n Thanks in advance for looking at this!\nVZ\n\n---------------------------------- >8 --------------------------------------\nFrom: Vadim Zeitlin \nSubject: [PATCH] fetch: allow running as different users in shared repositories\n\nThe function fopen_for_writing(), which was added in 79d7582e32 (commit:\nallow editing the commit message even in shared repos, 2016-01-06) and\nused for overwriting FETCH_HEAD since ea56518dfe (Handle more file\nwrites correctly in shared repos, 2016-01-11), didn't do it correctly in\nshared repositories under Linux.\n\nThis happened because in this situation the file FETCH_HEAD has mode 644\nand attempting to overwrite it when running git-fetch under an account\ndifferent from the one that was had originally created it, failed with\nEACCES, and not EPERM. However fopen_for_writing() only checked for the\nlatter, and not the former, so it didn't even try removing the existing\nfile and recreating it, as it was supposed to do.\n\nFix this by checking for either EACCES or EPERM. The latter doesn't seem\nto be ever returned in a typical situation by open(2) under Linux, but\nkeep checking for it as it is presumably returned under some other\nplatform, although it's not really clear where does this happen.\n\nSigned-off-by: Vadim Zeitlin \n---\nI couldn't find any system that would return EPERM for a \"normal\"\npermissions denied error, so maybe it's not worth checking for it, but I\nwanted to minimize the number of changes to the existing behaviour. At the\nvery least, testing for EACCES is definitely necessary under Linux, where\nopenat(2) returns it, and not EPERM, in the situation described above, i.e.\nnon-writable file (even if it's in a writable directory, allowing to unlink\nit without problems).\n---\n wrapper.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex e1eaef2e16..f5607241da 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -373,11 +373,12 @@ FILE *fopen_for_writing(const char *path)\n {\n \tFILE *ret = fopen(path, \"w\");\n \n-\tif (!ret && errno == EPERM) {\n+\tif (!ret && (errno == EACCES || errno == EPERM)) {\n+\t\tint open_error = errno;\n \t\tif (!unlink(path))\n \t\t\tret = fopen(path, \"w\");\n \t\telse\n-\t\t\terrno = EPERM;\n+\t\t\terrno = open_error;\n \t}\n \treturn ret;\n }\n-- \n2.26.0.rc2"},{"id":"396983","messageId":"xmqqlfm7fn8k.fsf@gitster.c.googlers.com","threadId":"53101","inReplyTo":"E1jVeWV-0006Sj-JC@smtp.tt-solutions.com","subject":"Re: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T20:39:55Z","receivedAt":"2020-05-04T20:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vadim Zeitlin writes:\n\n> From: Vadim Zeitlin \n> Subject: [PATCH] fetch: allow running as different users in shared repositories\n\nThis pretends the change to affect ONLY \"git fetch\", but ...\n\n> The function fopen_for_writing(), which was added in 79d7582e32 (commit:\n> allow editing the commit message even in shared repos, 2016-01-06) and\n> used for overwriting FETCH_HEAD since ea56518dfe (Handle more file\n> writes correctly in shared repos, 2016-01-11), didn't do it correctly in\n> shared repositories under Linux.\n\n... fopen_for_writing() is not only about FETCH_HEAD. In fact, the\nauthor of this patch knows \"git fetch\" was not the primary target.\n\nSo, we need to make sure that (1) this change is beneficial to those\nother codepaths that use the helper function, and (2) describe the\n(good) effect of the patch on these other users in the log message.\nWe also need to retitle the commit.\n\nHits from \"git grep fopen_for_writing\" are\n\nbuiltin/commit.c:812:\ts->fp = fopen_for_writing(git_path_commit_editmsg());\n\nThat's .git/COMMIT_EDITMSG file.\n\nbuiltin/fast-export.c:1049:\tf = fopen_for_writing(file);\n\nThis is inside export_marks() to create the marks file.\n\nbuiltin/fetch.c:1191:\tFILE *fp = fopen_for_writing(filename);\n\nThis is the .git/FETCH_HEAD.\n\n> This happened because in this situation the file FETCH_HEAD has mode 644\n> and attempting to overwrite it when running git-fetch under an account\n> different from the one that was had originally created it, failed with\n> EACCES, and not EPERM.\n\nIsn't that because FETCH_HEAD and others are not concluded with\nadjust_shared_perm()? The fopen_for_writing() that removes and\nrecreates the target file sounds like a band-aid to me. The right\nfix we should have done when we did 79d7582e (commit: allow editing\nthe commit message even in shared repos, 2016-01-06) would have been\nto open(2) with 0666 (and let the umask(2) adjust it), and then use\nadjust_shared_perm() to give it the desired protection bits. With\nthe existing band-aid, we won't be able to fix incorrectly created\nappend-only files, for example, as the band-aid depends on the\ncontents in the existing file being expendable.\n\nHaving said all that, I agree that EACCES is the right errno to\ndetect for this band-aid, at least for FETCH_HEAD.\n\nI think COMMIT_EDITMSG is also left after \"git commit\" finishes,\nso it will share the same issue with FETCH_HEAD and the same fix\nshould apply (this is just a hint for you to write an updated\nproposed log message for the patch).\n\nI haven't looked at or analysed how fast-export will get affected.\nI think it is used to create and leave a \"marks\" file, to be later\nread by another instance of the fast-export process, which may (or\nmay not) further write new contents to the (same?) \"marks\" file, but\nI do not know the ramifications of unlinking and recreating. In any\ncase, even if that is broken, it is not a new breakage this patch is\nintroducing. You may want to look at it further to make sure you\nare not breaking things, though.\n\nSo, here are the things I would like to see in this area:\n\n - The same patch text, but with updated commit log message, to tell\n readers that we have looked at all the callers that are affected,\n and retitle it (e.g. \"fopen_for_writing: detect the reason why fopen()\n failed correctly\" or something like that, perhaps?).\n \n - Audit other codepaths that create .git/ALL_CAPS_FILE (e.g. I see\n that \"git branch --edit-description\" creates a temporary file to\n edit without fopen_for_writing() band-aid and it does not use\n adjust_shared_perm(), but I think it should) and fix them.\n\n - The existing repositories have these files created and left whose\n permission bits were set according to the then-current umask\n without taking \"core.sharedrepository\" into account, so we have\n to keep the \"if unable to open for writing, unlink and recreate\"\n trick to salvage them. But it does not mean we need to keep\n creating the files with wrong mode. Update fopen_for_writing()\n and its users to leave the file created in the right mode by\n calling adjust_shared_perm(). I think fopen_for_writing() should\n switch from calling fopen(3) to calling open(2) and then fdopen(3)\n on the result as the first step.\n\nThe first one is better done by you to tie the loose ends for this\ndiscussion. \n\nOther two items do not have to be done by you. Anybody interested\ncan do them as a clean-up (only if people agree that it is a good\nidea to do so---so I won't mark this as a left-over-bits yet).\n\nThanks.\n\n> diff --git a/wrapper.c b/wrapper.c\n> index e1eaef2e16..f5607241da 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -373,11 +373,12 @@ FILE *fopen_for_writing(const char *path)\n> {\n> \tFILE *ret = fopen(path, \"w\");\n> \n> -\tif (!ret && errno == EPERM) {\n> +\tif (!ret && (errno == EACCES || errno == EPERM)) {\n> +\t\tint open_error = errno;\n> \t\tif (!unlink(path))\n> \t\t\tret = fopen(path, \"w\");\n> \t\telse\n> -\t\t\terrno = EPERM;\n> +\t\t\terrno = open_error;\n> \t}\n> \treturn ret;\n> }\n"},{"id":"397018","messageId":"E1jVl80-0002lI-D4@smtp.tt-solutions.com","threadId":"53101","inReplyTo":"xmqqlfm7fn8k.fsf@gitster.c.googlers.com","subject":"Re[2]: [PATCH] fetch: allow running as different users in shared repositories","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2020-05-05T00:08:04Z","receivedAt":"2020-05-05T00:08:10Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"On Mon, 04 May 2020 13:39:55 -0700 Junio C Hamano wrote:\n\nJCH> Vadim Zeitlin writes:\nJCH> \nJCH> > From: Vadim Zeitlin \nJCH> > Subject: [PATCH] fetch: allow running as different users in shared repositories\n\n Thanks for looking at this!\n\nJCH> This pretends the change to affect ONLY \"git fetch\", but ...\nJCH> \nJCH> > The function fopen_for_writing(), which was added in 79d7582e32 (commit:\nJCH> > allow editing the commit message even in shared repos, 2016-01-06) and\nJCH> > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file\nJCH> > writes correctly in shared repos, 2016-01-11), didn't do it correctly in\nJCH> > shared repositories under Linux.\nJCH> \nJCH> ... fopen_for_writing() is not only about FETCH_HEAD. In fact, the\nJCH> author of this patch knows \"git fetch\" was not the primary target.\n\n Right, sorry, I should have been more precise. FWIW I did look at the\nother uses of this function, but I didn't think to mention this because I\nonly checked that the change was not going to break the other users of this\nfunction: as all of them need a \"write-only\" file and just die in case of\nan error, recreating it and returning successfully couldn't possibly make\nthings worse.\n\nJCH> So, we need to make sure that (1) this change is beneficial to those\nJCH> other codepaths that use the helper function, and (2) describe the\nJCH> (good) effect of the patch on these other users in the log message.\nJCH> We also need to retitle the commit.\n\n OK, will do.\n\nJCH> > This happened because in this situation the file FETCH_HEAD has mode 644\nJCH> > and attempting to overwrite it when running git-fetch under an account\nJCH> > different from the one that was had originally created it, failed with\nJCH> > EACCES, and not EPERM.\nJCH> \nJCH> Isn't that because FETCH_HEAD and others are not concluded with\nJCH> adjust_shared_perm()?\n\n I didn't know about this function, but looking at its uses elsewhere, it\nseems indeed clear that it should be used here too.\n\nJCH> The fopen_for_writing() that removes and recreates the target file\nJCH> sounds like a band-aid to me.\n\n Definitely. I wondered how did we end up in a situation in which it became\nnecessary to do it, but didn't find the answer quickly and abandoned trying\nto understand it.\n\nJCH> The right fix we should have done when we did 79d7582e (commit: allow\nJCH> editing the commit message even in shared repos, 2016-01-06) would\nJCH> have been to open(2) with 0666 (and let the umask(2) adjust it), and\nJCH> then use adjust_shared_perm() to give it the desired protection bits.\nJCH> With the existing band-aid, we won't be able to fix incorrectly\nJCH> created append-only files, for example, as the band-aid depends on the\nJCH> contents in the existing file being expendable.\n\n Just to reiterate what you already know, right now the band-aid is only\nused for expendable files, as indicated rather clearly by the name of\nfopen_for_writing() function, so there is no real problem here, per se,\nit's just a bit ugly.\n\nJCH> I haven't looked at or analysed how fast-export will get affected.\nJCH> I think it is used to create and leave a \"marks\" file, to be later\nJCH> read by another instance of the fast-export process, which may (or\nJCH> may not) further write new contents to the (same?) \"marks\" file, but\nJCH> I do not know the ramifications of unlinking and recreating. In any\nJCH> case, even if that is broken, it is not a new breakage this patch is\nJCH> introducing. You may want to look at it further to make sure you\nJCH> are not breaking things, though.\n\n OK, I will do it. But I also think that, in principle, you could imagine a\nscenario in which the behaviour could change in case of multiple\nconcurrently running processes, e.g. if the group of the directory changed\nwhile they're running. I'm pretty sure it shouldn't be a realistic problem\nin practice, however, but I'll re-check it more carefully.\n\nJCH> So, here are the things I would like to see in this area:\nJCH> \nJCH> - The same patch text, but with updated commit log message, to tell\nJCH> readers that we have looked at all the callers that are affected,\nJCH> and retitle it (e.g. \"fopen_for_writing: detect the reason why fopen()\nJCH> failed correctly\" or something like that, perhaps?).\n\n OK, I'll do this.\n\nJCH> - Audit other codepaths that create .git/ALL_CAPS_FILE (e.g. I see\nJCH> that \"git branch --edit-description\" creates a temporary file to\nJCH> edit without fopen_for_writing() band-aid and it does not use\nJCH> adjust_shared_perm(), but I think it should) and fix them.\n\n This seems a bit more difficult. Do you have any hints about how could all\nsuch places be found effectively?\n\nJCH> - The existing repositories have these files created and left whose\nJCH> permission bits were set according to the then-current umask\nJCH> without taking \"core.sharedrepository\" into account, so we have\nJCH> to keep the \"if unable to open for writing, unlink and recreate\"\nJCH> trick to salvage them. But it does not mean we need to keep\nJCH> creating the files with wrong mode. Update fopen_for_writing()\nJCH> and its users to leave the file created in the right mode by\nJCH> calling adjust_shared_perm(). I think fopen_for_writing() should\nJCH> switch from calling fopen(3) to calling open(2) and then fdopen(3)\nJCH> on the result as the first step.\n\n Sorry, I'm not sure I follow you here. Do you want to use fchmod() here\ninstead of just calling adjust_shared_perm()? I.e. what is the problem with\nusing fopen()?\n\nJCH> The first one is better done by you to tie the loose ends for this\nJCH> discussion. \n\n I'll repost the patch after re-checking its effect on fast-export\n(assuming I don't find anything wrong).\n\nJCH> Other two items do not have to be done by you. Anybody interested\nJCH> can do them as a clean-up (only if people agree that it is a good\nJCH> idea to do so---so I won't mark this as a left-over-bits yet).\n\n FWIW this does seem like a good idea to me, but it's also going to be much\nless trivial than my patch and I'm not sure I can find the time needed to\nmake these changes and test them in the immediate future, so even though\nI'll try to do it, please don't count on me.\n\n Thanks again for your review,\nVZ\n"}]}