{"thread":{"id":"34611","subject":"About close() in commit_lock_file()","startedAt":"2013-08-05T14:23:38Z","lastAt":"2013-08-06T08:36:52Z","messageCount":4,"participants":["Duy Nguyen","Junio C Hamano","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"224577","messageId":"CACsJy8By1cpPZ5QyVd6VhKSkd-y_E6pTYdDimK9P0wXia-uMqg@mail.gmail.com","threadId":"34611","inReplyTo":null,"subject":"About close() in commit_lock_file()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-05T14:23:38Z","receivedAt":"2013-08-05T14:23:38Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"close() is added in commit_lock_file(), before rename(), by 4723ee9\n(Close files opened by lock_file() before unlinking. - 2007-11-13),\nwhich is needed by Windows. But doesn't that create a gap between\nclose() and rename() on other platforms where another process can\nreplace .lock file with something else before rename() is executed?\nShould we enclose close() in #ifdef __MINGW32__ (and maybe\n__CYGWIN__)?\n--\nDuy\n"},{"id":"224605","messageId":"7vli4gkrf0.fsf@alter.siamese.dyndns.org","threadId":"34611","inReplyTo":"CACsJy8By1cpPZ5QyVd6VhKSkd-y_E6pTYdDimK9P0wXia-uMqg@mail.gmail.com","subject":"Re: About close() in commit_lock_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T16:56:19Z","receivedAt":"2013-08-05T16:56:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> close() is added in commit_lock_file(), before rename(), by 4723ee9\n> (Close files opened by lock_file() before unlinking. - 2007-11-13),\n> which is needed by Windows. But doesn't that create a gap between\n> close() and rename() on other platforms where another process can\n> replace .lock file with something else before rename() is executed?\n\nInteresting.\n\n> Should we enclose close() in #ifdef __MINGW32__ (and maybe\n> __CYGWIN__)?\n\nOr just have \"close and retry\" code after seeing rename() fails with\nsome known errno, without singling out a particular platform?\n"},{"id":"224650","messageId":"52009A90.7050701@viscovery.net","threadId":"34611","inReplyTo":"CACsJy8By1cpPZ5QyVd6VhKSkd-y_E6pTYdDimK9P0wXia-uMqg@mail.gmail.com","subject":"Re: About close() in commit_lock_file()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-08-06T06:41:20Z","receivedAt":"2013-08-06T06:41:20Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 8/5/2013 16:23, schrieb Duy Nguyen:\n> close() is added in commit_lock_file(), before rename(), by 4723ee9\n> (Close files opened by lock_file() before unlinking. - 2007-11-13),\n> which is needed by Windows. But doesn't that create a gap between\n> close() and rename() on other platforms where another process can\n> replace .lock file with something else before rename() is executed?\n\nFirst, files manipulated by commit_lock_file() are to be opened only using\nlock_file() by definition. Opening such a file in with open() or fopen()\nor renaming it via rename() without using the lockfile.[ch] API is\npossible regardless of whether commit_lock_file() has closed the file or\nnot. Such manipulation is already undefined behavior (from Git's point of\nview), and there is nothing we can do about \"misbehaving\" processes.\n\nSecond, lock_file() uses O_CREAT|O_EXCL, which fails when the file exists,\nregardless of whether it is open or not.\n\nConclusion: There is no problem.\n\n-- Hannes\n"},{"id":"224656","messageId":"CACsJy8DquJj1qVawKfrbZY+2WLC54Gskizs1sxZ_j1yutBkbdA@mail.gmail.com","threadId":"34611","inReplyTo":"52009A90.7050701@viscovery.net","subject":"Re: About close() in commit_lock_file()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-06T08:36:52Z","receivedAt":"2013-08-06T08:36:52Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 6, 2013 at 1:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 8/5/2013 16:23, schrieb Duy Nguyen:\n>> close() is added in commit_lock_file(), before rename(), by 4723ee9\n>> (Close files opened by lock_file() before unlinking. - 2007-11-13),\n>> which is needed by Windows. But doesn't that create a gap between\n>> close() and rename() on other platforms where another process can\n>> replace .lock file with something else before rename() is executed?\n>\n> First, files manipulated by commit_lock_file() are to be opened only using\n> lock_file() by definition. Opening such a file in with open() or fopen()\n> or renaming it via rename() without using the lockfile.[ch] API is\n> possible regardless of whether commit_lock_file() has closed the file or\n> not. Such manipulation is already undefined behavior (from Git's point of\n> view), and there is nothing we can do about \"misbehaving\" processes.\n>\n> Second, lock_file() uses O_CREAT|O_EXCL, which fails when the file exists,\n> regardless of whether it is open or not.\n\nThe second process could unlink(.lock file) first, then open it, even\nusing lock_file(), but I think that falls into your \"first\" and is an\ninvalid case from git point of view, so..\n\n> Conclusion: There is no problem.\n\nAgreed. False alarm.\n-- \nDuy\n"}]}