{"thread":{"id":"41041","subject":"[PATCH] commit: ensure correct permissions of the commit message","startedAt":"2015-12-19T18:21:59Z","lastAt":"2016-01-15T12:18:47Z","messageCount":45,"participants":["Johannes Schindelin","Jeff King","Torsten Bögershausen","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"274761","messageId":"4aa11f02f4de113bf38152b8815658da42690f43.1450549280.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":null,"subject":"[PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-12-19T18:21:59Z","receivedAt":"2015-12-19T18:21:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was pointed out by Yaroslav Halchenko that the file containing the\ncommit message had the wrong permissions in a shared setting.\n\nLet's fix that.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/commit.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d054f84..3bfd457 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -905,6 +905,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tstrbuf_release(&committer_ident);\n \n \tfclose(s->fp);\n+\tadjust_shared_perm(git_path(commit_editmsg));\n \n \t/*\n \t * Reject an attempt to record a non-merge empty commit without\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"274773","messageId":"20151220074559.GF30662@sigill.intra.peff.net","threadId":"41041","inReplyTo":"4aa11f02f4de113bf38152b8815658da42690f43.1450549280.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-20T07:45:59Z","receivedAt":"2015-12-20T07:45:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 19, 2015 at 07:21:59PM +0100, Johannes Schindelin wrote:\n\n> It was pointed out by Yaroslav Halchenko that the file containing the\n> commit message had the wrong permissions in a shared setting.\n> \n> Let's fix that.\n> \n> Signed-off-by: Johannes Schindelin \n\nI think this is probably a step forward, but I have to wonder how many\nother files are in a similar situation (e.g., git-am state files, etc).\nI think people generally haven't noticed because shared repositories are\ngenerally about a shared bare rendezvous repo. So refs and objects are\nimportant, but we don't expect people to commit.\n\nSo I don't have any real problem with this, but I suspect it's just the\ntip of the iceberg. We might want something like:\n\n FILE *fopen_shared(const char *path, const char *mode)\n {\n\tFILE *ret = fopen(path, mode);\n\tif (!ret)\n\t\treturn NULL;\n\tif (adjust_shared_perm(path)) {\n\t\tfclose(ret);\n\t\treturn NULL;\n\t}\n\treturn ret;\n }\n\nbut of course the hard part is auditing all of the existing fopen()\ncalls to see who needs to use it. :)\n\n-Peff\n"},{"id":"274778","messageId":"alpine.DEB.2.20.1512201519590.6483@virtualbox","threadId":"41041","inReplyTo":"20151220074559.GF30662@sigill.intra.peff.net","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-12-20T14:21:46Z","receivedAt":"2015-12-20T14:21:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 20 Dec 2015, Jeff King wrote:\n\n> On Sat, Dec 19, 2015 at 07:21:59PM +0100, Johannes Schindelin wrote:\n> \n> > It was pointed out by Yaroslav Halchenko that the file containing the\n> > commit message had the wrong permissions in a shared setting.\n> > \n> > Let's fix that.\n> > \n> > Signed-off-by: Johannes Schindelin \n> \n> I think this is probably a step forward, but I have to wonder how many\n> other files are in a similar situation (e.g., git-am state files, etc).\n\nTrue.\n\n> I think people generally haven't noticed because shared repositories are\n> generally about a shared bare rendezvous repo. So refs and objects are\n> important, but we don't expect people to commit.\n> \n> So I don't have any real problem with this, but I suspect it's just the\n> tip of the iceberg. We might want something like:\n> \n> FILE *fopen_shared(const char *path, const char *mode)\n> {\n> \tFILE *ret = fopen(path, mode);\n> \tif (!ret)\n> \t\treturn NULL;\n> \tif (adjust_shared_perm(path)) {\n> \t\tfclose(ret);\n> \t\treturn NULL;\n> \t}\n> \treturn ret;\n> }\n> \n> but of course the hard part is auditing all of the existing fopen()\n> calls to see who needs to use it. :)\n\nIn principle, I agree, but I have to point out that the\nadjust_shared_perm() call must come after the *fclose()* call, to avoid\nmodifying files to which we currently have open file handles.\n\nCiao,\nDscho\n"},{"id":"274785","messageId":"56773264.2050806@web.de","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1512201519590.6483@virtualbox","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-12-20T22:57:40Z","receivedAt":"2015-12-20T22:57:40Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-12-20 15.21, Johannes Schindelin wrote:\n> Hi Peff,\n> \n> On Sun, 20 Dec 2015, Jeff King wrote:\n> \n>> On Sat, Dec 19, 2015 at 07:21:59PM +0100, Johannes Schindelin wrote:\n>>\n>>> It was pointed out by Yaroslav Halchenko that the file containing the\n>>> commit message had the wrong permissions in a shared setting.\n>>>\n>>> Let's fix that.\n>>>\n>>> Signed-off-by: Johannes Schindelin \n>>\n>> I think this is probably a step forward, but I have to wonder how many\n>> other files are in a similar situation (e.g., git-am state files, etc).\n> \n> True.\n> \n>> I think people generally haven't noticed because shared repositories are\n>> generally about a shared bare rendezvous repo. So refs and objects are\n>> important, but we don't expect people to commit.\n>>\n>> So I don't have any real problem with this, but I suspect it's just the\n>> tip of the iceberg. We might want something like:\n>>\n>> FILE *fopen_shared(const char *path, const char *mode)\n>> {\n>> \tFILE *ret = fopen(path, mode);\n>> \tif (!ret)\n>> \t\treturn NULL;\n>> \tif (adjust_shared_perm(path)) {\n>> \t\tfclose(ret);\n>> \t\treturn NULL;\n>> \t}\n>> \treturn ret;\n>> }\n>>\n>> but of course the hard part is auditing all of the existing fopen()\n>> calls to see who needs to use it. :)\n> \n> In principle, I agree, but I have to point out that the\n> adjust_shared_perm() call must come after the *fclose()* call, to avoid\n> modifying files to which we currently have open file handles.\n\nI had the same concern, but couldn't find anything that gives a hint that we\ncan't adjust\nthe permissions on an open file.\n\n(In opposite: We can't rename an open file (under Windows))\n\nIn fact we have this in tempfile.c\n\nint create_tempfile(struct tempfile *tempfile, const char *path)\n{\n\tprepare_tempfile_object(tempfile);\n\n\tstrbuf_add_absolute_path(&tempfile->filename, path);\n\ttempfile->fd = open(tempfile->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n\tif (tempfile->fd < 0) {\n\t\tstrbuf_reset(&tempfile->filename);\n\t\treturn -1;\n\t}\n\ttempfile->owner = getpid();\n\ttempfile->active = 1;\n\tif (adjust_shared_perm(tempfile->filename.buf)) {\n[snip] Error out\n\n(And this seems to be the same in git.git and git-for-windows)\n"},{"id":"274792","messageId":"xmqq7fk8io3f.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"20151220074559.GF30662@sigill.intra.peff.net","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T01:31:48Z","receivedAt":"2015-12-21T01:31:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I don't have any real problem with this, but I suspect it's just the\n> tip of the iceberg. We might want something like:\n>\n> FILE *fopen_shared(const char *path, const char *mode)\n> {\n> \tFILE *ret = fopen(path, mode);\n> \tif (!ret)\n> \t\treturn NULL;\n> \tif (adjust_shared_perm(path)) {\n> \t\tfclose(ret);\n> \t\treturn NULL;\n> \t}\n> \treturn ret;\n> }\n\nActually, we do not even _need_ a sharedness for this ephemeral\nfile. The additional \"adjust-shared-perm\" is merely a workaround\nfor the fact the next person cannot write into it when it is left\nbehind, and because we do not want to remove it when we are done.\n\nThat does not measn that the next person cannot remove it when she\nfinds there is a file there left behind. So alternatively, we could\ndo something like this, perhaps?\n\n FILE *fopen_forcibly(const char *path, const char *mode)\n {\n FILE *ret = fopen(path, mode);\n\n if (!ret && errno == EPERM) {\n if (!unlink(path))\n ret = fopen(path, mode);\n else\n errno = EPERM;\n }\n return ret;\n }\n"},{"id":"274795","messageId":"20151221065944.GA3550@sigill.intra.peff.net","threadId":"41041","inReplyTo":"xmqq7fk8io3f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-21T06:59:44Z","receivedAt":"2015-12-21T06:59:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 20, 2015 at 05:31:48PM -0800, Junio C Hamano wrote:\n\n> Actually, we do not even _need_ a sharedness for this ephemeral\n> file. The additional \"adjust-shared-perm\" is merely a workaround\n> for the fact the next person cannot write into it when it is left\n> behind, and because we do not want to remove it when we are done.\n> \n> That does not measn that the next person cannot remove it when she\n> finds there is a file there left behind. So alternatively, we could\n> do something like this, perhaps?\n> \n> FILE *fopen_forcibly(const char *path, const char *mode)\n> {\n> FILE *ret = fopen(path, mode);\n> \n> if (!ret && errno == EPERM) {\n> if (!unlink(path))\n> ret = fopen(path, mode);\n> else\n> errno = EPERM;\n> }\n> return ret;\n> }\n\nYeah, I think that is a much nicer solution for this case. It should\nwork even in a shared repo, since we set the permissions for the\nsurrounding $GIT_DIR appropriately[1].\n\nI guess it would not apply to any files that do not want to truncate the\nexisting contents. Probably it should drop the \"mode\" parameter at all,\nsince anything but \"w\" would be crazy?\n\n-Peff\n\n[1] Assuming you use something like \"git init --shared=true\". I would\n not be surprised if there are many repos where somebody just set\n \"core.sharedrepository\" after the fact, but there is not much we can\n do. They _have_ to fix up the permissions in that case.\n"},{"id":"274810","messageId":"xmqqio3rhg2f.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"20151221065944.GA3550@sigill.intra.peff.net","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T17:22:48Z","receivedAt":"2015-12-21T17:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Dec 20, 2015 at 05:31:48PM -0800, Junio C Hamano wrote:\n>\n>> Actually, we do not even _need_ a sharedness for this ephemeral\n>> file. The additional \"adjust-shared-perm\" is merely a workaround\n>> for the fact the next person cannot write into it when it is left\n>> behind, and because we do not want to remove it when we are done.\n>> \n>> That does not measn that the next person cannot remove it when she\n>> finds there is a file there left behind. So alternatively, we could\n>> do something like this, perhaps?\n>> \n>> FILE *fopen_forcibly(const char *path, const char *mode)\n>> {\n>> FILE *ret = fopen(path, mode);\n>> \n>> if (!ret && errno == EPERM) {\n>> if (!unlink(path))\n>> ret = fopen(path, mode);\n>> else\n>> errno = EPERM;\n>> }\n>> return ret;\n>> }\n>\n> Yeah, I think that is a much nicer solution for this case. It should\n> work even in a shared repo, since we set the permissions for the\n> surrounding $GIT_DIR appropriately[1].\n>\n> I guess it would not apply to any files that do not want to truncate the\n> existing contents. Probably it should drop the \"mode\" parameter at all,\n> since anything but \"w\" would be crazy?\n\nAbsolutely. Thanks for spotting.\n"},{"id":"275185","messageId":"alpine.DEB.2.20.1512301535550.14434@virtualbox","threadId":"41041","inReplyTo":"xmqqio3rhg2f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-12-30T14:50:09Z","receivedAt":"2015-12-30T14:50:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 21 Dec 2015, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Sun, Dec 20, 2015 at 05:31:48PM -0800, Junio C Hamano wrote:\n> >\n> >> we could do something like this, perhaps?\n> >> \n> >> FILE *fopen_forcibly(const char *path, const char *mode)\n> >> {\n> >> FILE *ret = fopen(path, mode);\n> >> \n> >> if (!ret && errno == EPERM) {\n> >> if (!unlink(path))\n> >> ret = fopen(path, mode);\n> >> else\n> >> errno = EPERM;\n> >> }\n> >> return ret;\n> >> }\n> >\n> > Yeah, I think that is a much nicer solution for this case. It should\n> > work even in a shared repo, since we set the permissions for the\n> > surrounding $GIT_DIR appropriately[1].\n> >\n> > I guess it would not apply to any files that do not want to truncate the\n> > existing contents. Probably it should drop the \"mode\" parameter at all,\n> > since anything but \"w\" would be crazy?\n> \n> Absolutely. Thanks for spotting.\n\nSo maybe\n\n\tfcreate_or_truncate(const char *path)\n\t{\n\t\t FILE *ret = fopen(path, \"w\");\n\n\t\t if (!ret && errno == EPERM) {\n\t\t\t if (!unlink(path))\n\t\t\t\t ret = fopen(path, \"w\");\n\t\t\t else\n\t\t\t\t errno = EPERM;\n\t\t }\n\t\t return ret;\n\t }\n\n?\n\nCiao,\nDscho\n"},{"id":"275186","messageId":"alpine.DEB.2.20.1512301556490.14434@virtualbox","threadId":"41041","inReplyTo":"56773264.2050806@web.de","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-12-30T14:57:36Z","receivedAt":"2015-12-30T14:57:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Sun, 20 Dec 2015, Torsten Bögershausen wrote:\n\n> On 2015-12-20 15.21, Johannes Schindelin wrote:\n>\n> > I have to point out that the adjust_shared_perm() call must come after\n> > the *fclose()* call, to avoid modifying files to which we currently\n> > have open file handles.\n> \n> I had the same concern, but couldn't find anything that gives a hint\n> that we can't adjust the permissions on an open file.\n\nOops, okay! I misremembered. This makes our task much easier now.\n\nThanks!\nDscho"},{"id":"275214","messageId":"xmqqfuyjczpv.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1512301535550.14434@virtualbox","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-30T22:56:44Z","receivedAt":"2015-12-30T22:56:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> So maybe\n>\n> \tfcreate_or_truncate(const char *path)\n> \t{\n> \t\t FILE *ret = fopen(path, \"w\");\n>\n> \t\t if (!ret && errno == EPERM) {\n> \t\t\t if (!unlink(path))\n> \t\t\t\t ret = fopen(path, \"w\");\n> \t\t\t else\n> \t\t\t\t errno = EPERM;\n> \t\t }\n> \t\t return ret;\n> \t }\n>\n> ?\n\nI do not know fcreate_or_truncate() is a good name, though. Looking\nfor \"fcreate(3)\" seems to find something quite obscure and unrelated\nto what we want (http://linux.die.net/man/3/fcreate). Also we call\na function X_or_Y to mean \"try to do our primary thing X, and do Y\ninstead when it fails\", but what we want to do here is not that. We\nare not interested in creating (i.e. we can happily live with an\nexisting file opened for writing), and we do not just truncate if we\ncannot open the original for writing (i.e. we do return a writable\nfile handle) [*1*].\n\nIn any case, obviously the return value of the function needs to be\nof type \"FILE *\".\n\nOther than that, the code inside the function looks exactly like\nwhat I had in mind.\n\nThanks.\n\n\n[Footnote]\n\n*1* We tend to call a function sane_X() when we are fixing an\n undesirable behaviour in the underlying function X (e.g.\n sane_execvp()). In that sense it is very tempting to call this\n sane_fopen() but our use is limited to fopen(path, \"w\"), i.e.\n \"open for writing, while truncating if existing\" use case of\n fopen().\n"},{"id":"275255","messageId":"alpine.DEB.2.20.1601011604070.14434@virtualbox","threadId":"41041","inReplyTo":"xmqqfuyjczpv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-01T15:04:25Z","receivedAt":"2016-01-01T15:04:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 30 Dec 2015, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > So maybe\n> >\n> > \tfcreate_or_truncate(const char *path)\n> > \t{\n> > \t\t FILE *ret = fopen(path, \"w\");\n> >\n> > \t\t if (!ret && errno == EPERM) {\n> > \t\t\t if (!unlink(path))\n> > \t\t\t\t ret = fopen(path, \"w\");\n> > \t\t\t else\n> > \t\t\t\t errno = EPERM;\n> > \t\t }\n> > \t\t return ret;\n> > \t }\n> >\n> > ?\n> \n> I do not know fcreate_or_truncate() is a good name, though.\n\nSo what would be a good name?\n\nCiao,\nDscho\n"},{"id":"275314","messageId":"xmqq60z9p51f.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1601011604070.14434@virtualbox","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-04T18:34:36Z","receivedAt":"2016-01-04T18:34:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I do not know fcreate_or_truncate() is a good name, though.\n>\n> So what would be a good name?\n\nHave been thinking about it, but I did not come up with anything. I\njust know fcreate_or_truncate() is not a good name for multiple\nreasons I already explained X-<. sane_fopen_for_writing() perhaps?\n"},{"id":"275373","messageId":"alpine.DEB.2.20.1601051342050.14434@virtualbox","threadId":"41041","inReplyTo":"xmqq60z9p51f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-05T12:52:06Z","receivedAt":"2016-01-05T12:52:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 4 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> I do not know fcreate_or_truncate() is a good name, though.\n> >\n> > So what would be a good name?\n> \n> Have been thinking about it, but I did not come up with anything. I\n> just know fcreate_or_truncate() is not a good name for multiple\n> reasons I already explained X-<. sane_fopen_for_writing() perhaps?\n\nThat implies that fopen() is insane, though ;-) Let's just drop the sane_\nprefix?\n\nCiao,\nDscho\n"},{"id":"275393","messageId":"xmqq37ubke8q.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1601051342050.14434@virtualbox","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T19:39:17Z","receivedAt":"2016-01-05T19:39:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Mon, 4 Jan 2016, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> >> I do not know fcreate_or_truncate() is a good name, though.\n>> >\n>> > So what would be a good name?\n>> \n>> Have been thinking about it, but I did not come up with anything. I\n>> just know fcreate_or_truncate() is not a good name for multiple\n>> reasons I already explained X-<. sane_fopen_for_writing() perhaps?\n>\n> That implies that fopen() is insane, though ;-)\n\nWell, sane_X() does not imply X() is \"insane\". The reason why we\nintroduce sane_X() wrapper often is because X is inconvenient to use\nsanely as-is.\n\nLook at sane_istest(), sane_case(), etc. We have one byte and want\nto see if that is space, digit, etc., but we have to cast (possibly\nsigned) char to unsigned char and always doing so in the callers is\ninconvenient. Look at sane_grep. We have a string and want to see\nthat appears in its input, but some implementations of \"grep\" can\nbe configured in an inconvenient/unsuitable way to be used in\nscripts via environment variables, so we disable it not in the\ncallers but in a wrapper.\n\nAll we want to do here is to get a writable filehandle FILE * for a\npath and store the contents we will generate without having to worry\nabout the possibility that the path is already there and we may have\nto first unlink it to use fopen(3) to do so.\n\nSo in the sense, fopen(3) is inconvenient to use sanely as-is. You\ncan call it insane, too ;-)\n\nIf we want to follow the X_or_Y() pattern, fopen_or_create() may\ndescribe what it does better. I do not have strong preference\neither way, but again I am not good at naming things (and I suspect\nyou aren't either), so...\n"},{"id":"275418","messageId":"alpine.DEB.2.20.1601060916400.2964@virtualbox","threadId":"41041","inReplyTo":"xmqq37ubke8q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-06T08:20:34Z","receivedAt":"2016-01-06T08:20:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 5 Jan 2016, Junio C Hamano wrote:\n\n> If we want to follow the X_or_Y() pattern, fopen_or_create() may\n> describe what it does better. I do not have strong preference either\n> way, but again I am not good at naming things (and I suspect you aren't\n> either), so...\n\nHeh... You got that right...\n\nLet's let it simmer for a couple more days, maybe somebody else chimes in\nwith a brilliant idea... :-)\n\nCiao,\nDscho\n"},{"id":"275419","messageId":"20160106082341.GB19117@sigill.intra.peff.net","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1601060916400.2964@virtualbox","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-06T08:23:42Z","receivedAt":"2016-01-06T08:23:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 06, 2016 at 09:20:34AM +0100, Johannes Schindelin wrote:\n\n> Hi Junio,\n> \n> On Tue, 5 Jan 2016, Junio C Hamano wrote:\n> \n> > If we want to follow the X_or_Y() pattern, fopen_or_create() may\n> > describe what it does better. I do not have strong preference either\n> > way, but again I am not good at naming things (and I suspect you aren't\n> > either), so...\n> \n> Heh... You got that right...\n> \n> Let's let it simmer for a couple more days, maybe somebody else chimes in\n> with a brilliant idea... :-)\n\nI can be the anti-brilliant and just shoot down what has been said. :)\n\nI think fopen_or_create is confusing; it implies to me that we'll open\nan existing file or create it if it's not there. But we are always about\ntruncating/replacing the existing file.\n\nI think fopen_for_writing() is fine, or fopen_truncate().\n\n-Peff\n"},{"id":"275420","messageId":"alpine.DEB.2.20.1601060949530.2964@virtualbox","threadId":"41041","inReplyTo":"20160106082341.GB19117@sigill.intra.peff.net","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-06T08:50:21Z","receivedAt":"2016-01-06T08:50:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 6 Jan 2016, Jeff King wrote:\n\n> I think fopen_for_writing() is fine, or fopen_truncate().\n\nI like fopen_for_writing() better than fopen_truncate() because it states\nthe intention more clearly.\n\nThanks!\nDscho\n"},{"id":"275440","messageId":"cover.1452085713.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":"4aa11f02f4de113bf38152b8815658da42690f43.1450549280.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/2] Correctly handle transient files in shared repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-06T13:09:36Z","receivedAt":"2016-01-06T13:09:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Transient files, e.g. commit messages, are writable only by the owner,\neven in shared repositories, to avoid interference between competing\nusers working on the same files.\n\nThese files are typically not deleted after use. As a consequence, we\nhave to delete such files before writing when they are owned by someone\nelse than the current user.\n\nReported-by: Yaroslav Halchenko \n\n\nJohannes Schindelin (2):\n commit: allow editing the commit message even in shared repos\n Handle more file writes correctly in shared repos\n\n builtin/commit.c | 2 +-\n builtin/fast-export.c | 2 +-\n builtin/fetch.c | 2 +-\n git-compat-util.h | 1 +\n wrapper.c | 13 +++++++++++++\n 5 files changed, 17 insertions(+), 3 deletions(-)\n\nInterdiff vs v1:\n\n diff --git a/builtin/commit.c b/builtin/commit.c\n index 3bfd457..89bf6ad 100644\n --- a/builtin/commit.c\n +++ b/builtin/commit.c\n @@ -761,7 +761,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg2 = \"\";\n \t}\n \n -\ts->fp = fopen(git_path(commit_editmsg), \"w\");\n +\ts->fp = fopen_for_writing(git_path(commit_editmsg));\n \tif (s->fp == NULL)\n \t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n \n @@ -905,7 +905,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tstrbuf_release(&committer_ident);\n \n \tfclose(s->fp);\n -\tadjust_shared_perm(git_path(commit_editmsg));\n \n \t/*\n \t * Reject an attempt to record a non-merge empty commit without\n diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n index d9ac5d8..2471297 100644\n --- a/builtin/fast-export.c\n +++ b/builtin/fast-export.c\n @@ -880,7 +880,7 @@ static void export_marks(char *file)\n \tFILE *f;\n \tint e = 0;\n \n -\tf = fopen(file, \"w\");\n +\tf = fopen_for_writing(file);\n \tif (!f)\n \t\tdie_errno(\"Unable to open marks file %s for writing.\", file);\n \n diff --git a/builtin/fetch.c b/builtin/fetch.c\n index 586840d..33f04c1 100644\n --- a/builtin/fetch.c\n +++ b/builtin/fetch.c\n @@ -840,7 +840,7 @@ static void check_not_current_branch(struct ref *ref_map)\n static int truncate_fetch_head(void)\n {\n \tconst char *filename = git_path_fetch_head();\n -\tFILE *fp = fopen(filename, \"w\");\n +\tFILE *fp = fopen_for_writing(filename);\n \n \tif (!fp)\n \t\treturn error(_(\"cannot open %s: %s\\n\"), filename, strerror(errno));\n diff --git a/git-compat-util.h b/git-compat-util.h\n index 2da0a75..e8f2867 100644\n --- a/git-compat-util.h\n +++ b/git-compat-util.h\n @@ -733,6 +733,7 @@ extern int xmkstemp_mode(char *template, int mode);\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n extern char *xgetcwd(void);\n +extern FILE *fopen_for_writing(const char *path);\n \n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))\n \n diff --git a/wrapper.c b/wrapper.c\n index b43d437..29a45d2 100644\n --- a/wrapper.c\n +++ b/wrapper.c\n @@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)\n \treturn stream;\n }\n \n +FILE *fopen_for_writing(const char *path)\n +{\n +\tFILE *ret = fopen(path, \"w\");\n +\n +\tif (!ret && errno == EPERM) {\n +\t\tif (!unlink(path))\n +\t\t\tret = fopen(path, \"w\");\n +\t\telse\n +\t\t\terrno = EPERM;\n +\t}\n +\treturn ret;\n +}\n +\n int xmkstemp(char *template)\n {\n \tint fd;\n\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275439","messageId":"38701d9358765af334ce405eb4a29f0063dae670.1452085713.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":"cover.1452085713.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/2] commit: allow editing the commit message even in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-06T13:09:43Z","receivedAt":"2016-01-06T13:09:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was pointed out by Yaroslav Halchenko that the file containing the\ncommit message is writable only by the owner, which means that we have\nto rewrite it from scratch in a shared repository.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/commit.c | 2 +-\n git-compat-util.h | 1 +\n wrapper.c | 13 +++++++++++++\n 3 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d054f84..89bf6ad 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -761,7 +761,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg2 = \"\";\n \t}\n \n-\ts->fp = fopen(git_path(commit_editmsg), \"w\");\n+\ts->fp = fopen_for_writing(git_path(commit_editmsg));\n \tif (s->fp == NULL)\n \t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2da0a75..e8f2867 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -733,6 +733,7 @@ extern int xmkstemp_mode(char *template, int mode);\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n extern char *xgetcwd(void);\n+extern FILE *fopen_for_writing(const char *path);\n \n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))\n \ndiff --git a/wrapper.c b/wrapper.c\nindex b43d437..29a45d2 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)\n \treturn stream;\n }\n \n+FILE *fopen_for_writing(const char *path)\n+{\n+\tFILE *ret = fopen(path, \"w\");\n+\n+\tif (!ret && errno == EPERM) {\n+\t\tif (!unlink(path))\n+\t\t\tret = fopen(path, \"w\");\n+\t\telse\n+\t\t\terrno = EPERM;\n+\t}\n+\treturn ret;\n+}\n+\n int xmkstemp(char *template)\n {\n \tint fd;\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275438","messageId":"c03e5a9d367b76d8a249680c752b4c4d935e9b91.1452085713.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":"cover.1452085713.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-06T13:09:49Z","receivedAt":"2016-01-06T13:09:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In shared repositories, we have to be careful when writing files whose\npermissions do not allow users other than the owner to write them.\n\nIn particular, we force the marks file of fast-export and the FETCH_HEAD\nwhen fetching to be rewritten from scratch.\n\nThis commit does not touch the following users of fopen() that want to\nwrite files:\n\n- git am, when splitting mails (git-am correctly cleans up its directory\n after finishing, so there is no need to share those files between users)\n\n- git apply, when writing rejected hunks\n\n- git fsck, when writing lost&found blobs\n\n- git merge-file, when writing merged files (when Git itself calls\n merge-file, the file in question was already there, with shared\n permissions).\n\n- git submodule clone, when writing the .git file, because the file will\n not be overwritten\n\n- git_terminal_prompt() in compat/terminal.c, because it is not writing to\n a file at all\n\n- git diff --output, because the output file is clearly not intended to be\n shared between the users of the current repository\n\n- git fast-import, when writing a crash report\n\n- mailinfo() in mailinfo.c, because the output is clearly not intended to\n be shared between the users of the current repository\n\n- check_or_regenerate_marks() in remote-testsvn.c, because this is only\n used for Git's internal testing\n\n- git rerere, when writing resolved files, because the files in question\n were already written with the correct permissions\n\nNote that this patch does not touch callers of write_file() and\nwrite_file_gently(), which would benefit from the same scrutiny as to\nusage in shared repositories. Most notable users: branch, daemon,\nsubmodule & worktree, and a worrisome call in transport.c when updating\none ref (which ignores the shared flag).\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fast-export.c | 2 +-\n builtin/fetch.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d9ac5d8..2471297 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -880,7 +880,7 @@ static void export_marks(char *file)\n \tFILE *f;\n \tint e = 0;\n \n-\tf = fopen(file, \"w\");\n+\tf = fopen_for_writing(file);\n \tif (!f)\n \t\tdie_errno(\"Unable to open marks file %s for writing.\", file);\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 586840d..33f04c1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -840,7 +840,7 @@ static void check_not_current_branch(struct ref *ref_map)\n static int truncate_fetch_head(void)\n {\n \tconst char *filename = git_path_fetch_head();\n-\tFILE *fp = fopen(filename, \"w\");\n+\tFILE *fp = fopen_for_writing(filename);\n \n \tif (!fp)\n \t\treturn error(_(\"cannot open %s: %s\\n\"), filename, strerror(errno));\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275473","messageId":"20160107124117.GA14421@sigill.intra.peff.net","threadId":"41041","inReplyTo":"38701d9358765af334ce405eb4a29f0063dae670.1452085713.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/2] commit: allow editing the commit message even in shared repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-07T12:41:18Z","receivedAt":"2016-01-07T12:41:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 06, 2016 at 02:09:43PM +0100, Johannes Schindelin wrote:\n\n> It was pointed out by Yaroslav Halchenko that the file containing the\n> commit message is writable only by the owner, which means that we have\n> to rewrite it from scratch in a shared repository.\n> [...]\n> diff --git a/wrapper.c b/wrapper.c\n> index b43d437..29a45d2 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)\n> \treturn stream;\n> }\n> \n> +FILE *fopen_for_writing(const char *path)\n> +{\n> +\tFILE *ret = fopen(path, \"w\");\n> +\n> +\tif (!ret && errno == EPERM) {\n> +\t\tif (!unlink(path))\n> +\t\t\tret = fopen(path, \"w\");\n> +\t\telse\n> +\t\t\terrno = EPERM;\n> +\t}\n> +\treturn ret;\n> +}\n\nThanks, this looks good to me. Having seen the implementation, it really\nis just \"try harder to fopen()\". I guess calling it \"fopen_me_harder()\"\nwould be too obscure. :)\n\n-Peff\n"},{"id":"275475","messageId":"20160107124608.GB14421@sigill.intra.peff.net","threadId":"41041","inReplyTo":"c03e5a9d367b76d8a249680c752b4c4d935e9b91.1452085713.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-07T12:46:08Z","receivedAt":"2016-01-07T12:46:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 06, 2016 at 02:09:49PM +0100, Johannes Schindelin wrote:\n\n> In shared repositories, we have to be careful when writing files whose\n> permissions do not allow users other than the owner to write them.\n> \n> In particular, we force the marks file of fast-export and the FETCH_HEAD\n> when fetching to be rewritten from scratch.\n> \n> This commit does not touch the following users of fopen() that want to\n> write files:\n\nThe patch looks good, and thanks for cataloguing the other cases.\n\n> - git am, when splitting mails (git-am correctly cleans up its directory\n> after finishing, so there is no need to share those files between users)\n> \n> - git apply, when writing rejected hunks\n> \n> - git fsck, when writing lost&found blobs\n\nFor these latter two, I had to ask myself \"why not?\". You gave such nice\nreasons for the other items in the list, I wondered what your reasoning\nwas here. In the second case, for example, I think it would prevent a\nsecond shared user from running `git fsck --lost-found`, because we are\nlikely to find the same \"old\" dangling objects and fail to write them.\n\nI also wondered why we do not do the usual write-to-temp and rename in\nsome of these cases, but that is not really relevant to your patch.\n\n-Peff\n"},{"id":"275504","messageId":"xmqq8u413wee.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"20160107124117.GA14421@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] commit: allow editing the commit message even in shared repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T21:35:53Z","receivedAt":"2016-01-07T21:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 06, 2016 at 02:09:43PM +0100, Johannes Schindelin wrote:\n>\n>> It was pointed out by Yaroslav Halchenko that the file containing the\n>> commit message is writable only by the owner, which means that we have\n>> to rewrite it from scratch in a shared repository.\n>> [...]\n>> diff --git a/wrapper.c b/wrapper.c\n>> index b43d437..29a45d2 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)\n>> \treturn stream;\n>> }\n>> \n>> +FILE *fopen_for_writing(const char *path)\n>> +{\n>> +\tFILE *ret = fopen(path, \"w\");\n>> +\n>> +\tif (!ret && errno == EPERM) {\n>> +\t\tif (!unlink(path))\n>> +\t\t\tret = fopen(path, \"w\");\n>> +\t\telse\n>> +\t\t\terrno = EPERM;\n>> +\t}\n>> +\treturn ret;\n>> +}\n>\n> Thanks, this looks good to me. Having seen the implementation, it really\n> is just \"try harder to fopen()\". I guess calling it \"fopen_me_harder()\"\n> would be too obscure. :)\n\nobscure or you meant some other obsxxxx? fopen_harder() does not\nsound too bad, but fopen_for_writing() is OK to me.\n\nAnd its use in this patch looks sensible.\n\nThanks.\n"},{"id":"275506","messageId":"xmqq1t9t3vn8.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"c03e5a9d367b76d8a249680c752b4c4d935e9b91.1452085713.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T21:52:11Z","receivedAt":"2016-01-07T21:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> - git apply, when writing rejected hunks\n\nToday I may try to apply and leave hello.c.rej; tomorrow you may try\nto apply a different patch and get rejection for the same file. And\nyou would not be able to if my umask is 077.\n\nI do not know \"intended to be shared\" is a good criteria, unless it\nis a synomym to \"what is under $GIT_DIR\". core.sharedRepository is\nabout the shared-ness of $GIT_DIR and never about the working tree,\nso I consider it is correct that not adjusting the permission is the\nright thing to do when 'git apply' writes .rej files, though.\n\n> - git fsck, when writing lost&found blobs\n\nSo this _is_ conceptually a problem, but writing anything indexed\nwith the object name is an idempotent operation, so this will not\nmatter in practice, I think.\n\n> - git merge-file, when writing merged files (when Git itself calls\n> merge-file, the file in question was already there, with shared\n> permissions).\n\nAgain, this is not a problem (i.e. not touching it in this patch is\nthe right thing to do) as this is about files in the working tree.\n\n> - git fast-import, when writing a crash report\n\nI am not sure about this one. Is the crash report designed to get\nunique filename every time you run? Otherwise, the fixed name\ninside $GIT_DIR/ is a shared resource, so I suspect it would want to\noverwrite. \"Not overwriting the crash report is safer, because the\nexistence of it is a sign that the earlier crash hasn't been dealt\nwith\" is also a valid position to take, but then it shouldn't even\noverwrite my own crash report from an earlier run.\n\nSo I am inclined to say that this should be changed (either\nconsistently overwrite using fopen_harder(), or consistently fail\nwhen my earlier crash report is already there). I however do not\nthink that change belongs to this topic.\n\n> - mailinfo() in mailinfo.c, because the output is clearly not intended to\n> be shared between the users of the current repository\n\nThis is more because \"not intended to be run multiple simultaneously\nusing the same filename\" plus \"the callers clean up after they are\ndone\" (similar to what you wrote for \"git am\"), I think.\n\n> - git rerere, when writing resolved files, because the files in question\n> were already written with the correct permissions\n\nThis again is more because the result goes to the working tree, not\n$GIT_DIR.\n"},{"id":"275553","messageId":"alpine.DEB.2.20.1601081702070.2964@virtualbox","threadId":"41041","inReplyTo":"20160107124608.GB14421@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-08T16:04:17Z","receivedAt":"2016-01-08T16:04:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 7 Jan 2016, Jeff King wrote:\n\n> On Wed, Jan 06, 2016 at 02:09:49PM +0100, Johannes Schindelin wrote:\n> \n> > - git am, when splitting mails (git-am correctly cleans up its\n> > directory after finishing, so there is no need to share those files\n> > between users)\n> > \n> > - git apply, when writing rejected hunks\n> > \n> > - git fsck, when writing lost&found blobs\n> \n> For these latter two, I had to ask myself \"why not?\". You gave such nice\n> reasons for the other items in the list, I wondered what your reasoning\n> was here.\n\nMy bad. My reading on both rejected hunks and lost & found blobs is that\nthe files should be cleaned up by the user who generated them. And if\nanother user can interfere with that cleaning up, that is bad. So I left\nthem non-shared to avoid such an interference.\n\n> I also wondered why we do not do the usual write-to-temp and rename in\n> some of these cases, but that is not really relevant to your patch.\n\nOh yeah, I'd love to stop here (I already extended this rather simple\none-line patch to something vastly larger, and that was not my intention).\n\nCiao,\nDscho\n"},{"id":"275555","messageId":"alpine.DEB.2.20.1601081704280.2964@virtualbox","threadId":"41041","inReplyTo":"xmqq1t9t3vn8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-08T16:05:49Z","receivedAt":"2016-01-08T16:05:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 7 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > - git apply, when writing rejected hunks\n> \n> Today I may try to apply and leave hello.c.rej; tomorrow you may try to\n> apply a different patch and get rejection for the same file. And you\n> would not be able to if my umask is 077.\n\nAs I just wrote in my reply to Peff: my experience with .rej files is that\nI want to inspect them, and then delete them once I addressed them. I do\nnot want anybody to interfere with that, as the presence of .rej files\nserves also as a TODO list.\n\n> > - git fast-import, when writing a crash report\n> \n> I am not sure about this one. Is the crash report designed to get\n> unique filename every time you run?\n\nYes, the pid is part of the file name. So we're golden.\n\nCiao,\nDscho\n"},{"id":"275565","messageId":"xmqqbn8w2brm.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1601081704280.2964@virtualbox","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-08T17:59:09Z","receivedAt":"2016-01-08T17:59:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 7 Jan 2016, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > - git apply, when writing rejected hunks\n>> \n>> Today I may try to apply and leave hello.c.rej; tomorrow you may try to\n>> apply a different patch and get rejection for the same file. And you\n>> would not be able to if my umask is 077.\n>\n> As I just wrote in my reply to Peff: my experience with .rej files is that\n> I want to inspect them, and then delete them once I addressed them. I do\n> not want anybody to interfere with that, as the presence of .rej files\n> serves also as a TODO list.\n\nThat argues for protecting .rej files against overwriting by myself,\ntoo, which means (1) we do not want to loosen it by using\nfopen_for_writing(), and (2) relying on permission bits and\nownership is not sufficient, i.e. just using regular fopen(3) is\nwrong.\n\nI think it is correct not to touch this codepath in this patch,\nbecause of the above two reasons, but more simply and generally, it\nis correct not to touch this codepath because core.sharedRepository\nis not about working tree files, and .rej is a file you use in your\nworking tree.\n\nAs the log messages are often used to guide future developers, I\nthink the log message of this commit should mention that criterion.\nIt would cover multiple codepaths you listed in your proposed log\nmessage in a more generic way, helping other people to reason about\nwhen they see other instances of fopen(..., \"w\") and wonder if it\nshould become fopen_for_writing().\n\nThanks.\n"},{"id":"275631","messageId":"alpine.DEB.2.20.1601111018550.2964@virtualbox","threadId":"41041","inReplyTo":"xmqqbn8w2brm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-11T09:28:28Z","receivedAt":"2016-01-11T09:28:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 8 Jan 2016, Junio C Hamano wrote:\n\n> I think it is correct not to touch this codepath in this patch,\n> because of the above two reasons, but more simply and generally, it\n> is correct not to touch this codepath because core.sharedRepository\n> is not about working tree files, and .rej is a file you use in your\n> working tree.\n\nI am happy to adjust the log message, but I am pretty certain that the\n`core.sharedRepository` setting actually also affects the working tree. At\nleast in my hands, calling\n\n\tgit clone -c core.sharedRepository=group . test-shared\n\nresults in all of the working tree files being group-writable.\n\nTherefore I am not convinced this is a sound line of reasoning.\n\nCiao,\nDscho\n"},{"id":"275651","messageId":"xmqqio30yur4.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"alpine.DEB.2.20.1601111018550.2964@virtualbox","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-11T15:57:03Z","receivedAt":"2016-01-11T15:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 8 Jan 2016, Junio C Hamano wrote:\n>\n>> I think it is correct not to touch this codepath in this patch,\n>> because of the above two reasons, but more simply and generally, it\n>> is correct not to touch this codepath because core.sharedRepository\n>> is not about working tree files, and .rej is a file you use in your\n>> working tree.\n>\n> I am happy to adjust the log message, but I am pretty certain that the\n> `core.sharedRepository` setting actually also affects the working tree. At\n> least in my hands, calling\n>\n> \tgit clone -c core.sharedRepository=group . test-shared\n>\n> results in all of the working tree files being group-writable.\n\nInteresting. I have a suspicion that \"clone\" does not honor the\nconfiguration given that way, though.\n\n $ umask 077\n $ git clone -c core.sharedRepository=group ~/w/git.git sharedtest\n $ cd sharedtest\n $ ls -l COPYING .git/index\n -rw------- 1 jch eng 18765 Jan 11 07:43 COPYING\n -rw------- 1 jch eng 272037 Jan 11 07:43 .git/index\n\nNotice that the permission bits in the working tree is correct, but\nin the resulting .git/ they are bogus, so from this we cannot\nclearly see the reason why COPYING is not group-readable is because\nthe checkout codepath (write_entry(), I think) is correctly omitting\nthe call to adjust_perm(), or simply the configuration is ignored\nduring the clone.\n\nWith a workaround to ensure that checkout happens definitely after\nthe configuration gets in effect by doing config and pull/checkout\nas two separate steps:\n\n $ rm -fr sharedtest\n $ umask 077\n $ git init sharedtest && cd sharedtest\n $ git config core.sharedRepository group\n $ git pull ~/w/git.git/ master\n $ ls -l COPYING .git/index\n -rw------- 1 jch eng 18765 Jan 11 07:48 COPYING\n -rw-rw---- 1 jch eng 272037 Jan 11 07:48 .git/index\n\nwe can see that the configuration affects only the $GIT_DIR/ files\nand not working tree.\n\nSo you found a bug in clone, I think ;-)\n"},{"id":"275664","messageId":"xmqqtwmkxcz3.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"xmqqio30yur4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-11T17:06:24Z","receivedAt":"2016-01-11T17:06:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> On Fri, 8 Jan 2016, Junio C Hamano wrote:\n>>\n>>> I think it is correct not to touch this codepath in this patch,\n>>> because of the above two reasons, but more simply and generally, it\n>>> is correct not to touch this codepath because core.sharedRepository\n>>> is not about working tree files, and .rej is a file you use in your\n>>> working tree.\n>>\n>> I am happy to adjust the log message, but I am pretty certain that the\n>> `core.sharedRepository` setting actually also affects the working tree. At\n>> least in my hands, calling\n>>\n>> \tgit clone -c core.sharedRepository=group . test-shared\n>>\n>> results in all of the working tree files being group-writable.\n>\n> Interesting. I have a suspicion that \"clone\" does not honor the\n> configuration given that way, though.\n>\n> $ umask 077\n> $ git clone -c core.sharedRepository=group ~/w/git.git sharedtest\n> $ cd sharedtest\n> $ ls -l COPYING .git/index\n> -rw------- 1 jch eng 18765 Jan 11 07:43 COPYING\n> -rw------- 1 jch eng 272037 Jan 11 07:43 .git/index\n>\n> Notice that the permission bits in the working tree is correct, but\n> in the resulting .git/ they are bogus, so from this we cannot\n> clearly see the reason why COPYING is not group-readable is because\n> the checkout codepath (write_entry(), I think) is correctly omitting\n> the call to adjust_perm(), or simply the configuration is ignored\n> during the clone.\n>\n> With a workaround to ensure that checkout happens definitely after\n> the configuration gets in effect by doing config and pull/checkout\n> as two separate steps:\n>\n> $ rm -fr sharedtest\n> $ umask 077\n> $ git init sharedtest && cd sharedtest\n> $ git config core.sharedRepository group\n> $ git pull ~/w/git.git/ master\n> $ ls -l COPYING .git/index\n> -rw------- 1 jch eng 18765 Jan 11 07:48 COPYING\n> -rw-rw---- 1 jch eng 272037 Jan 11 07:48 .git/index\n>\n> we can see that the configuration affects only the $GIT_DIR/ files\n> and not working tree.\n>\n> So you found a bug in clone, I think ;-)\n\nHaving said all that, the above does not mean that I'll refuse to\nconsider changing the semantics of core.sharedRepository in a future\nmajor version bump by doing adjust_perm() for working tree files,\nwhich we have deliberately chosen not to do in the current code.\n\nBut that is not within the scope of the patch we are discussing, and\nI am not convinced it is a good idea (I haven't heard either sides\nof arguments), so based on the current design, I think \"we don't do\nfopen_for_writing() for working tree files\" is a valid justification\nthat is short-and-sweet for this patch.\n"},{"id":"275685","messageId":"cover.1452537321.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":"cover.1452085713.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-11T18:35:42Z","receivedAt":"2016-01-11T18:35:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Transient files, e.g. commit messages, are writable only by the owner,\neven in shared repositories, to avoid interference between competing\nusers working on the same files.\n\nThese files are typically not deleted after use. As a consequence, we\nhave to delete such files before writing when they are owned by someone\nelse than the current user.\n\nThe only change relative to v2 is that the second commit message\nclarifies why apply, fsck and fast-import are left unchanged.\n\nReported-by: Yaroslav Halchenko \n\n\nJohannes Schindelin (2):\n commit: allow editing the commit message even in shared repos\n Handle more file writes correctly in shared repos\n\n builtin/commit.c | 2 +-\n builtin/fast-export.c | 2 +-\n builtin/fetch.c | 2 +-\n git-compat-util.h | 1 +\n wrapper.c | 13 +++++++++++++\n 5 files changed, 17 insertions(+), 3 deletions(-)\n\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275686","messageId":"38701d9358765af334ce405eb4a29f0063dae670.1452537321.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":"cover.1452537321.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/2] commit: allow editing the commit message even in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-11T18:35:49Z","receivedAt":"2016-01-11T18:35:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was pointed out by Yaroslav Halchenko that the file containing the\ncommit message is writable only by the owner, which means that we have\nto rewrite it from scratch in a shared repository.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/commit.c | 2 +-\n git-compat-util.h | 1 +\n wrapper.c | 13 +++++++++++++\n 3 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d054f84..89bf6ad 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -761,7 +761,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg2 = \"\";\n \t}\n \n-\ts->fp = fopen(git_path(commit_editmsg), \"w\");\n+\ts->fp = fopen_for_writing(git_path(commit_editmsg));\n \tif (s->fp == NULL)\n \t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2da0a75..e8f2867 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -733,6 +733,7 @@ extern int xmkstemp_mode(char *template, int mode);\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n extern char *xgetcwd(void);\n+extern FILE *fopen_for_writing(const char *path);\n \n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))\n \ndiff --git a/wrapper.c b/wrapper.c\nindex b43d437..29a45d2 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)\n \treturn stream;\n }\n \n+FILE *fopen_for_writing(const char *path)\n+{\n+\tFILE *ret = fopen(path, \"w\");\n+\n+\tif (!ret && errno == EPERM) {\n+\t\tif (!unlink(path))\n+\t\t\tret = fopen(path, \"w\");\n+\t\telse\n+\t\t\terrno = EPERM;\n+\t}\n+\treturn ret;\n+}\n+\n int xmkstemp(char *template)\n {\n \tint fd;\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275687","messageId":"cfac7c2f2bfc8ae72c003899619d47fe856fd1e8.1452537321.git.johannes.schindelin@gmx.de","threadId":"41041","inReplyTo":"cover.1452537321.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/2] Handle more file writes correctly in shared repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-11T18:35:54Z","receivedAt":"2016-01-11T18:35:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In shared repositories, we have to be careful when writing files whose\npermissions do not allow users other than the owner to write them.\n\nIn particular, we force the marks file of fast-export and the FETCH_HEAD\nwhen fetching to be rewritten from scratch.\n\nThis commit does not touch the following users of fopen() that want to\nwrite files:\n\n- git am, when splitting mails (git-am correctly cleans up its directory\n after finishing, so there is no need to share those files between users)\n\n- git apply, when writing rejected hunks (to be conservative, as it is not\n clear whether to write those files in shared mode or not)\n\n- git fsck, when writing lost&found blobs (to be conservative, as it is\n not clear whether to write those files in shared mode or not)\n\n- git merge-file, when writing merged files (when Git itself calls\n merge-file, the file in question was already there, with shared\n permissions).\n\n- git submodule clone, when writing the .git file, because the file will\n not be overwritten\n\n- git_terminal_prompt() in compat/terminal.c, because it is not writing to\n a file at all\n\n- git diff --output, because the output file is clearly not intended to be\n shared between the users of the current repository\n\n- git fast-import, when writing a crash report, because the reports' file\n names are unique due to an embedded process ID\n\n- mailinfo() in mailinfo.c, because the output is clearly not intended to\n be shared between the users of the current repository\n\n- check_or_regenerate_marks() in remote-testsvn.c, because this is only\n used for Git's internal testing\n\n- git rerere, when writing resolved files, because the files in question\n were already written with the correct permissions\n\nNote that this patch does not touch callers of write_file() and\nwrite_file_gently(), which would benefit from the same scrutiny as to\nusage in shared repositories. Most notable users: branch, daemon,\nsubmodule & worktree, and a worrisome call in transport.c when updating\none ref (which ignores the shared flag).\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fast-export.c | 2 +-\n builtin/fetch.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d9ac5d8..2471297 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -880,7 +880,7 @@ static void export_marks(char *file)\n \tFILE *f;\n \tint e = 0;\n \n-\tf = fopen(file, \"w\");\n+\tf = fopen_for_writing(file);\n \tif (!f)\n \t\tdie_errno(\"Unable to open marks file %s for writing.\", file);\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 586840d..33f04c1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -840,7 +840,7 @@ static void check_not_current_branch(struct ref *ref_map)\n static int truncate_fetch_head(void)\n {\n \tconst char *filename = git_path_fetch_head();\n-\tFILE *fp = fopen(filename, \"w\");\n+\tFILE *fp = fopen_for_writing(filename);\n \n \tif (!fp)\n \t\treturn error(_(\"cannot open %s: %s\\n\"), filename, strerror(errno));\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275703","messageId":"20160111202243.GA14289@sigill.intra.peff.net","threadId":"41041","inReplyTo":"cover.1452537321.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T20:22:43Z","receivedAt":"2016-01-11T20:22:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 11, 2016 at 07:35:42PM +0100, Johannes Schindelin wrote:\n\n> Transient files, e.g. commit messages, are writable only by the owner,\n> even in shared repositories, to avoid interference between competing\n> users working on the same files.\n> \n> These files are typically not deleted after use. As a consequence, we\n> have to delete such files before writing when they are owned by someone\n> else than the current user.\n> \n> The only change relative to v2 is that the second commit message\n> clarifies why apply, fsck and fast-import are left unchanged.\n\nThis looks fine to me. Even if if there are sites that could also use\nconversion, this is a strict improvement, and we can revisit the other\nones in the future.\n\nThanks.\n\n-Peff\n"},{"id":"275710","messageId":"xmqqa8obx1ll.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"cover.1452537321.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-11T21:12:06Z","receivedAt":"2016-01-11T21:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Transient files, e.g. commit messages, are writable only by the owner,\n> even in shared repositories, to avoid interference between competing\n> users working on the same files.\n>\n> These files are typically not deleted after use. As a consequence, we\n> have to delete such files before writing when they are owned by someone\n> else than the current user.\n>\n> The only change relative to v2 is that the second commit message\n> clarifies why apply, fsck and fast-import are left unchanged.\n\nI do not think it clarifies to make \"fsck\" whose lost and found are\nwritten to GIT_DIR and \"apply\"'s \".rej\" share the same \"we dunno\"\nreasoning, though.\n\nFor \"apply\"'s .rej and \"rerere\"'s resolution, the reason why we\nshould not touch these codepaths is because we know we should not\nmuck with permission bits of working tree files (i.e. checkout\nrefrains from doing so, and these codepaths should be consistent),\nnot because it is not clear. It _is_ clear we should not, and that\nis because we don't.\n\nI said \"I am not convinced\" but that is for future discussion of\npossibly changing the behaviour of checkout and other things that\ntouch the working tree files, which hasn't even started.\n\nOther than that, the resulting tree looks good to me, though.\n\nThanks.\n\n> Reported-by: Yaroslav Halchenko \n>\n>\n> Johannes Schindelin (2):\n> commit: allow editing the commit message even in shared repos\n> Handle more file writes correctly in shared repos\n>\n> builtin/commit.c | 2 +-\n> builtin/fast-export.c | 2 +-\n> builtin/fetch.c | 2 +-\n> git-compat-util.h | 1 +\n> wrapper.c | 13 +++++++++++++\n> 5 files changed, 17 insertions(+), 3 deletions(-)\n"},{"id":"275711","messageId":"xmqq60yzx14w.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"xmqqa8obx1ll.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-11T21:22:07Z","receivedAt":"2016-01-11T21:22:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> The only change relative to v2 is that the second commit message\n>> clarifies why apply, fsck and fast-import are left unchanged.\n>\n> I do not think it clarifies to make \"fsck\" whose lost and found are\n> written to GIT_DIR and \"apply\"'s \".rej\" share the same \"we dunno\"\n> reasoning, though.\n\nI'd say we should go with this one. I think the reasoning for\n\"fsck\" should be a lot clearer this way.\n\n-- >8 --\nAuthor: Johannes Schindelin \nDate: Mon Jan 11 19:35:54 2016 +0100\n\n Handle more file writes correctly in shared repos\n \n In shared repositories, we have to be careful when writing files whose\n permissions do not allow users other than the owner to write them.\n \n In particular, we force the marks file of fast-export and the FETCH_HEAD\n when fetching to be rewritten from scratch.\n \n This commit does not touch other calls to fopen() that want to\n write files:\n \n - commands that write to working tree files (core.sharedRepository\n does not affect permission bits of working tree files),\n e.g. .rej file created by \"apply --reject\", result of applying a\n previous conflict resolution by \"rerere\", \"git merge-file\".\n \n - git am, when splitting mails (git-am correctly cleans up its directory\n after finishing, so there is no need to share those files between users)\n \n - git fsck, when writing lost&found blobs (they are written in the\n file under its object name, so an existing file with tighter\n permission that you cannot write into is OK, because what you are\n failing to write is the same contents that the file already has\n anyway).\n \n - git submodule clone, when writing the .git file, because the file\n will not be overwritten\n \n - git_terminal_prompt() in compat/terminal.c, because it is not writing to\n a file at all\n \n - git diff --output, because the output file is clearly not intended to be\n shared between the users of the current repository\n \n - git fast-import, when writing a crash report, because the reports' file\n names are unique due to an embedded process ID\n \n - mailinfo() in mailinfo.c, because the output is clearly not intended to\n be shared between the users of the current repository\n \n - check_or_regenerate_marks() in remote-testsvn.c, because this is only\n used for Git's internal testing\n \n Note that this patch does not touch callers of write_file() and\n write_file_gently(), which would benefit from the same scrutiny as\n to usage in shared repositories. Most notable users are branch,\n daemon, submodule & worktree, and a worrisome call in transport.c\n when updating one ref (which ignores the shared flag).\n \n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n"},{"id":"275716","messageId":"20160111213801.GB21131@sigill.intra.peff.net","threadId":"41041","inReplyTo":"xmqq60yzx14w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T21:38:02Z","receivedAt":"2016-01-11T21:38:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 11, 2016 at 01:22:07PM -0800, Junio C Hamano wrote:\n\n> - git fsck, when writing lost&found blobs (they are written in the\n> file under its object name, so an existing file with tighter\n> permission that you cannot write into is OK, because what you are\n> failing to write is the same contents that the file already has\n> anyway).\n\nI'm not sure I buy this argument. Yes, you should not be writing\nanything else, but that does not change the fact that \"fsck\" will\nunceremoniously abort:\n\n $ git init\n $ echo foo | git hash-object -w --stdin\n $ git fsck --lost-found\n notice: HEAD points to an unborn branch (master)\n Checking object directories: 100% (256/256), done.\n notice: No default references\n dangling blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99\n\n $ chmod -w .git/lost-found/other/257cc5642cb1a054f08cc83f2d943e56fd3ebe99 \n $ $ git fsck --lost-found\n notice: HEAD points to an unborn branch (master)\n Checking object directories: 100% (256/256), done.\n notice: No default references\n dangling blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99\n fatal: Could not open '.git/lost-found/other/257cc5642cb1a054f08cc83f2d943e56fd3ebe99': Permission denied\n\nSo I think this would be a reasonable candidate (or alternatively, to\ntreat EPERM on an existing file as a soft error). I am totally fine not\nto address it as part of this series, though.\n\n-Peff\n"},{"id":"275718","messageId":"xmqq1t9nwznm.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"20160111213801.GB21131@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-11T21:54:05Z","receivedAt":"2016-01-11T21:54:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure I buy this argument. Yes, you should not be writing\n> anything else, but that does not change the fact that \"fsck\" will\n> unceremoniously abort:\n> ...\n> So I think this would be a reasonable candidate (or alternatively, to\n> treat EPERM on an existing file as a soft error). I am totally fine not\n> to address it as part of this series, though.\n\nYeah, that crossed my mind (and I agree with the conclusion).\n\nListing what is left deliberately and why is still a good idea, as\nthat would force people to think twice before wasting effort to\nconvert blindly without thinking. Listing what is left behind like\n\"git fsck\" that we know we shouldn't leave behind is even better to\nmark low-hanging fruits. How do you like this one instead?\n\n - git fsck, when writing lost&found blobs (this probably should\n be changed, but left as a low-hanging fruit for future\n contributors).\n"},{"id":"275719","messageId":"20160111220603.GD21131@sigill.intra.peff.net","threadId":"41041","inReplyTo":"xmqq1t9nwznm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T22:06:03Z","receivedAt":"2016-01-11T22:06:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 11, 2016 at 01:54:05PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm not sure I buy this argument. Yes, you should not be writing\n> > anything else, but that does not change the fact that \"fsck\" will\n> > unceremoniously abort:\n> > ...\n> > So I think this would be a reasonable candidate (or alternatively, to\n> > treat EPERM on an existing file as a soft error). I am totally fine not\n> > to address it as part of this series, though.\n> \n> Yeah, that crossed my mind (and I agree with the conclusion).\n> \n> Listing what is left deliberately and why is still a good idea, as\n> that would force people to think twice before wasting effort to\n> convert blindly without thinking. Listing what is left behind like\n> \"git fsck\" that we know we shouldn't leave behind is even better to\n> mark low-hanging fruits. How do you like this one instead?\n> \n> - git fsck, when writing lost&found blobs (this probably should\n> be changed, but left as a low-hanging fruit for future\n> contributors).\n\nI think that's more accurate. :)\n\n-Peff\n"},{"id":"275767","messageId":"alpine.DEB.2.20.1601120905000.2964@virtualbox","threadId":"41041","inReplyTo":"20160111220603.GD21131@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-12T08:05:12Z","receivedAt":"2016-01-12T08:05:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 11 Jan 2016, Jeff King wrote:\n\n> On Mon, Jan 11, 2016 at 01:54:05PM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > I'm not sure I buy this argument. Yes, you should not be writing\n> > > anything else, but that does not change the fact that \"fsck\" will\n> > > unceremoniously abort:\n> > > ...\n> > > So I think this would be a reasonable candidate (or alternatively, to\n> > > treat EPERM on an existing file as a soft error). I am totally fine not\n> > > to address it as part of this series, though.\n> > \n> > Yeah, that crossed my mind (and I agree with the conclusion).\n> > \n> > Listing what is left deliberately and why is still a good idea, as\n> > that would force people to think twice before wasting effort to\n> > convert blindly without thinking. Listing what is left behind like\n> > \"git fsck\" that we know we shouldn't leave behind is even better to\n> > mark low-hanging fruits. How do you like this one instead?\n> > \n> > - git fsck, when writing lost&found blobs (this probably should\n> > be changed, but left as a low-hanging fruit for future\n> > contributors).\n> \n> I think that's more accurate. :)\n\nFine by me!\n\nCiao,\nDscho\n"},{"id":"276116","messageId":"20160115021237.Horde.J6jJsRTdZiG5URajzd8kKsn@webmail.informatik.kit.edu","threadId":"41041","inReplyTo":"xmqq7fk8io3f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-01-15T01:12:37Z","receivedAt":"2016-01-15T01:12:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> Actually, we do not even _need_ a sharedness for this ephemeral\n> file. The additional \"adjust-shared-perm\" is merely a workaround\n> for the fact the next person cannot write into it when it is left\n> behind, and because we do not want to remove it when we are done.\n\nDo we really want to keep such a file after we are done?\n\n From all the cases where $GIT_EDITOR is run as part of an operation \nto edit a temporary file, that temporary file is deleted on success \nmost of the time, except the already mentioned COMMIT_EDITMSG, \nREPLACE_EDITOBJ for 'git replace --edit', and BRANCH_DESCRIPTION for \n'git branch --edit-description'. I don't see why these files should \nbe handled differently than the others and why should they be kept \nafter the operation finished, since at that point the contents is \nalready stored in its proper place (in the commit object, in the \nreplacement object, or in the config file), making these files \nredundant and unnecessary.\n\nIf the operation fails for whatever reason, then that's a different \nmatter, of course. In that case we definitely want to keep these \nfiles to avoid losing whatever the user has written.\n\nGábor\n"},{"id":"276118","messageId":"xmqqegdjk4ud.fsf@gitster.mtv.corp.google.com","threadId":"41041","inReplyTo":"20160115021237.Horde.J6jJsRTdZiG5URajzd8kKsn@webmail.informatik.kit.edu","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-15T01:29:30Z","receivedAt":"2016-01-15T01:29:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Quoting Junio C Hamano :\n>\n>> Actually, we do not even _need_ a sharedness for this ephemeral\n>> file. The additional \"adjust-shared-perm\" is merely a workaround\n>> for the fact the next person cannot write into it when it is left\n>> behind, and because we do not want to remove it when we are done.\n>\n> Do we really want to keep such a file after we are done?\n\nThere is no strong reason we should want to remove them, either.\n\nAs long as the lazy garbage collection works, there is no incentive\nto change things. I do not have strong objection against such a\nchange, as long as you do not break people's existing practices,\neither, though.\n\nWhoever is designing such a change must carefully define what \"after\nwe are done\" exactly means. Removing such a file immediately after\nthe command read from it is likely to be a bad idea, for example, if\nthe command supports hooks that are invoked after the command reads\nand uses the contents of it, as the hooks may be depending on the\npresence of it and the ability to read from it.\n"},{"id":"276126","messageId":"alpine.DEB.2.20.1601150750340.2964@virtualbox","threadId":"41041","inReplyTo":"xmqqegdjk4ud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-15T06:51:49Z","receivedAt":"2016-01-15T06:51:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 14 Jan 2016, Junio C Hamano wrote:\n\n> SZEDER Gábor writes:\n> \n> > Quoting Junio C Hamano :\n> >\n> >> Actually, we do not even _need_ a sharedness for this ephemeral file.\n> >> The additional \"adjust-shared-perm\" is merely a workaround for the\n> >> fact the next person cannot write into it when it is left behind, and\n> >> because we do not want to remove it when we are done.\n> >\n> > Do we really want to keep such a file after we are done?\n> \n> There is no strong reason we should want to remove them, either.\n> \n> As long as the lazy garbage collection works, there is no incentive\n> to change things. I do not have strong objection against such a\n> change, as long as you do not break people's existing practices,\n> either, though.\n> \n> Whoever is designing such a change must carefully define what \"after\n> we are done\" exactly means. Removing such a file immediately after\n> the command read from it is likely to be a bad idea, for example, if\n> the command supports hooks that are invoked after the command reads\n> and uses the contents of it, as the hooks may be depending on the\n> presence of it and the ability to read from it.\n\n... and do not forget scripts that call `git commit`. They can expect the\npresence of that file after a successful operation right now. You will\nhave to warn any potential user of such a script that you are going to\nbreak their setup.\n\nCiao,\nDscho"},{"id":"276146","messageId":"20160115115100.Horde.QlNwKM_-9TZQXszPUgP2qz3@webmail.informatik.kit.edu","threadId":"41041","inReplyTo":"xmqqegdjk4ud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-01-15T10:51:00Z","receivedAt":"2016-01-15T10:51:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> Quoting Junio C Hamano :\n>>\n>>> Actually, we do not even _need_ a sharedness for this ephemeral\n>>> file. The additional \"adjust-shared-perm\" is merely a workaround\n>>> for the fact the next person cannot write into it when it is left\n>>> behind, and because we do not want to remove it when we are done.\n>>\n>> Do we really want to keep such a file after we are done?\n>\n> There is no strong reason we should want to remove them, either.\n>\n> As long as the lazy garbage collection works, there is no incentive\n> to change things.\n\nI'm not sure what you mean by \"lazy garbage collection\"; I suppose not \nthat I sometimes delete a stale COMMIT_EDITMSG or two by hand...\n"},{"id":"276150","messageId":"alpine.DEB.2.20.1601151317360.2964@virtualbox","threadId":"41041","inReplyTo":"20160115115100.Horde.QlNwKM_-9TZQXszPUgP2qz3@webmail.informatik.kit.edu","subject":"Re: [PATCH] commit: ensure correct permissions of the commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-15T12:18:47Z","receivedAt":"2016-01-15T12:18:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Fri, 15 Jan 2016, SZEDER Gábor wrote:\n\n> Quoting Junio C Hamano :\n> \n> >SZEDER Gábor writes:\n> >\n> > >Quoting Junio C Hamano :\n> > >\n> > > >Actually, we do not even _need_ a sharedness for this ephemeral\n> > > >file. The additional \"adjust-shared-perm\" is merely a workaround\n> > > >for the fact the next person cannot write into it when it is left\n> > > >behind, and because we do not want to remove it when we are done.\n> > >\n> > >Do we really want to keep such a file after we are done?\n> >\n> >There is no strong reason we should want to remove them, either.\n> >\n> >As long as the lazy garbage collection works, there is no incentive\n> >to change things.\n> \n> I'm not sure what you mean by \"lazy garbage collection\"; I suppose not\n> that I sometimes delete a stale COMMIT_EDITMSG or two by hand...\n\nYou do not delete a stale COMMIT_EDITMSG *by hand*, but essentially it is\ndeleted the next time you commit.\n\nCiao,\nDscho"}]}