{"thread":{"id":"40035","subject":"[PATCH] config.mak.uname: Cygwin: Use renames for creation","startedAt":"2015-08-07T20:30:28Z","lastAt":"2015-08-18T15:44:31Z","messageCount":9,"participants":["Adam Dinwoodie","Mark Levedahl","brian m. carlson","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"267629","messageId":"1438979428-5888-1-git-send-email-adam@dinwoodie.org","threadId":"40035","inReplyTo":null,"subject":"[PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2015-08-07T20:30:28Z","receivedAt":"2015-08-07T20:30:28Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"When generating build options for Cygwin, enable\nOBJECT_CREATION_USES_RENAMES.  This is necessary to use Git on Windows\nshared directories, and is already enabled for the MinGW and plain\nWindows builds.\n\nThis problem was reported on the Cygwin mailing list at\nhttps://cygwin.com/ml/cygwin/2015-08/msg00102.html (amongst others) and\nis being applied as a manual patch to the Cygwin builds until the patch\nis taken here.\n\nReported-by: Peter Rosin <peda@lysator.liu.se>\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n config.mak.uname | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 943c439..be5cbec 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -187,6 +187,7 @@ ifeq ($(uname_O),Cygwin)\n \tX = .exe\n \tUNRELIABLE_FSTAT = UnfortunatelyYes\n \tSPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield\n+\tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n endif\n ifeq ($(uname_S),FreeBSD)\n \tNEEDS_LIBICONV = YesPlease\n-- \n2.4.5\n"},{"id":"267659","messageId":"55C66AF2.3060706@gmail.com","threadId":"40035","inReplyTo":"1438979428-5888-1-git-send-email-adam@dinwoodie.org","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2015-08-08T20:47:46Z","receivedAt":"2015-08-08T20:47:46Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 08/07/2015 04:30 PM, Adam Dinwoodie wrote:\n> When generating build options for Cygwin, enable\n> OBJECT_CREATION_USES_RENAMES.  This is necessary to use Git on Windows\n> shared directories, and is already enabled for the MinGW and plain\n> Windows builds.\n>\n> This problem was reported on the Cygwin mailing list at\n> https://cygwin.com/ml/cygwin/2015-08/msg00102.html (amongst others) and\n> is being applied as a manual patch to the Cygwin builds until the patch\n> is taken here.\n>\n> Reported-by: Peter Rosin <peda@lysator.liu.se>\n> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n> ---\n>   config.mak.uname | 1 +\n>   1 file changed, 1 insertion(+)\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 943c439..be5cbec 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -187,6 +187,7 @@ ifeq ($(uname_O),Cygwin)\n>   \tX = .exe\n>   \tUNRELIABLE_FSTAT = UnfortunatelyYes\n>   \tSPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield\n> +\tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n>   endif\n>   ifeq ($(uname_S),FreeBSD)\n>   \tNEEDS_LIBICONV = YesPlease\nI've been supporting use of git on cygwin since about 2008, this issue \nhas never risen that I know. Whatever issue is being patched around \nhere, if truly repeatable, should be handled by the cygwin dll as that \ncode is focused on providing full linux compatibility. If git on linux \ndoes need this patch, git on cygwin should not, either. So, I vote \nagainst this.\n\nMark\n"},{"id":"267660","messageId":"20150808210627.GB155450@vauxhall.crustytoothpaste.net","threadId":"40035","inReplyTo":"55C66AF2.3060706@gmail.com","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-08-08T21:06:28Z","receivedAt":"2015-08-08T21:06:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Aug 08, 2015 at 04:47:46PM -0400, Mark Levedahl wrote:\n> On 08/07/2015 04:30 PM, Adam Dinwoodie wrote:\n> >When generating build options for Cygwin, enable\n> >OBJECT_CREATION_USES_RENAMES.  This is necessary to use Git on Windows\n> >shared directories, and is already enabled for the MinGW and plain\n> >Windows builds.\n> I've been supporting use of git on cygwin since about 2008, this issue has\n> never risen that I know. Whatever issue is being patched around here, if\n> truly repeatable, should be handled by the cygwin dll as that code is\n> focused on providing full linux compatibility. If git on linux does need\n> this patch, git on cygwin should not, either. So, I vote against this.\n\nWe've gotten a lot of users on the list who ask why their Git\ndirectories on shared drives aren't working (or are broken in some way).\nSince I don't use Windows, let me ask: does the Cygwin DLL handle\nlink(2) properly on shared drives, and if not, would this patch help it\ndo so?  I can imagine that perhaps SMB doesn't support the necessary\noperations to make a POSIX link(2) work properly.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"267664","messageId":"CA+kUOa=KRBSKDqWUj2RiO45PqVYGmN+yqG426jtUoXayxGkduw@mail.gmail.com","threadId":"40035","inReplyTo":"20150808210627.GB155450@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2015-08-09T02:01:06Z","receivedAt":"2015-08-09T02:01:06Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Sat, Aug 08, 2015 at 09:06:28PM +0000, brian m. carlson wrote:\n> On Sat, Aug 08, 2015 at 04:47:46PM -0400, Mark Levedahl wrote:\n> > On 08/07/2015 04:30 PM, Adam Dinwoodie wrote:\n> > >When generating build options for Cygwin, enable\n> > >OBJECT_CREATION_USES_RENAMES.  This is necessary to use Git on Windows\n> > >shared directories, and is already enabled for the MinGW and plain\n> > >Windows builds.\n> >\n> > I've been supporting use of git on cygwin since about 2008, this issue has\n> > never risen that I know. Whatever issue is being patched around here, if\n> > truly repeatable, should be handled by the cygwin dll as that code is\n> > focused on providing full linux compatibility. If git on linux does need\n> > this patch, git on cygwin should not, either. So, I vote against this.\n\nThere has been recent and historical discussion on the Cygwin mailing\nlist about this problem, as well as in other places like Stack Overflow.\nI've put a link to one of those discussions on the Cygwin mailing list\nin the original patch email.  I can also see some discussiions on this\nlist that seem related (search archives for \"failed to read delta-pack\nbase object\" and \"Cygwin\").\n\nIt may be the technically correct approach that the Cygwin library ought\nto fix this, and indeed some improvements have been made in this area.\nHowever given the limited interfaces that Windows offers here, a final\nfix is very unlikely to come any time soon, so this patch is the\npragmatic solution.\n\nI do not see any difference between the situation here and the situation\nfor MinGW, which is fundamentally a Cygwin fork, but which already has\nthis build option set for it in config.mak.uname.\n\n> We've gotten a lot of users on the list who ask why their Git\n> directories on shared drives aren't working (or are broken in some way).\n> Since I don't use Windows, let me ask: does the Cygwin DLL handle\n> link(2) properly on shared drives, and if not, would this patch help it\n> do so?  I can imagine that perhaps SMB doesn't support the necessary\n> operations to make a POSIX link(2) work properly.\n\nI'd need to go back to the Cygwin list to get a definite answer, but as\nI understand it, yes, this is is exactly the problem -- quoting Corinna,\none of the Cygwin project leads, \"The MS NFS is not very reliable in\nkeeping up with changes to metadata.\"\n\nWe have verified that setting `core.createobject rename` resolves the\nproblem for people who are seeing it, which very strongly implies that\nthis build option would solve the problem similarly, but would fix it\nfor all users, not just those who spend enough time investigating the\nproblem to find that setting.\n\nAdam\n"},{"id":"267675","messageId":"058a7756ada2fa5043ca9b910d6e1543@www.dscho.org","threadId":"40035","inReplyTo":"CA+kUOa=KRBSKDqWUj2RiO45PqVYGmN+yqG426jtUoXayxGkduw@mail.gmail.com","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-09T09:01:26Z","receivedAt":"2015-08-09T09:01:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Adam,\n\nOn 2015-08-09 04:01, Adam Dinwoodie wrote:\n\n> I do not see any difference between the situation here and the situation\n> for MinGW, which is fundamentally a Cygwin fork, but which already has\n> this build option set for it in config.mak.uname.\n\nThis is incorrect. MinGW is distinctly *not* a Cygwin fork. MinGW means \"Minimal GNU on Windows\" and that in turn means that it provides an environment to build executables that purely use the Win32 API. Read: no POSIX emulation whatsoever. Most notably, MinGW programs cannot use fork(2); It is simply unavailable.\n\nWhat you *probably* meant is that Git for Windows relies on MSys2 for its shell and Perl scripts, and that MSys2 in turn is a fork of Cygwin. That affects *only* the scripts, though; Git itself (as in `git.exe`) is still a pure MinGW program (and as a consequence, is quite a bit faster than Cygwin Git, at the price of certain quirks that Cygwin Git does not suffer).\n\n>> We've gotten a lot of users on the list who ask why their Git\n>> directories on shared drives aren't working (or are broken in some way).\n>> Since I don't use Windows, let me ask: does the Cygwin DLL handle\n>> link(2) properly on shared drives, and if not, would this patch help it\n>> do so?  I can imagine that perhaps SMB doesn't support the necessary\n>> operations to make a POSIX link(2) work properly.\n> \n> I'd need to go back to the Cygwin list to get a definite answer, but as\n> I understand it, yes, this is is exactly the problem -- quoting Corinna,\n> one of the Cygwin project leads, \"The MS NFS is not very reliable in\n> keeping up with changes to metadata.\"\n> \n> We have verified that setting `core.createobject rename` resolves the\n> problem for people who are seeing it, which very strongly implies that\n> this build option would solve the problem similarly, but would fix it\n> for all users, not just those who spend enough time investigating the\n> problem to find that setting.\n\n>From my experience, it appears that providing Corinna Vinschen (or better put: the Cygwin developers in general) with a sound patch gets things fixed pretty timely.\n\nAnd since `core.createObject = rename` seems to work around the problem, it should be possible to patch the Cygwin runtime accordingly. Sure, it will take a little investigation *what* code should be changed, and how, but the obvious benefit to *all* Cygwin applications should make that effort worth your while.\n\nPlease note that Cygwin's source code itself is in Git now, too: https://cygwin.com/git.html\n\nCiao,\nJohannes\n"},{"id":"267693","messageId":"55C7883C.7040504@dinwoodie.org","threadId":"40035","inReplyTo":"058a7756ada2fa5043ca9b910d6e1543@www.dscho.org","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2015-08-09T17:05:00Z","receivedAt":"2015-08-09T17:05:00Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On 09/08/2015 10:01, Johannes Schindelin wrote:\n> On 2015-08-09 04:01, Adam Dinwoodie wrote:\n>> I do not see any difference between the situation here and the situation\n>> for MinGW, which is fundamentally a Cygwin fork, but which already has\n>> this build option set for it in config.mak.uname.\n> This is incorrect. MinGW is distinctly *not* a Cygwin fork. MinGW means \"Minimal GNU on Windows\" and that in turn means that it provides an environment to build executables that purely use the Win32 API. Read: no POSIX emulation whatsoever. Most notably, MinGW programs cannot use fork(2); It is simply unavailable.\n>\n> What you *probably* meant is that Git for Windows relies on MSys2 for its shell and Perl scripts, and that MSys2 in turn is a fork of Cygwin. That affects *only* the scripts, though; Git itself (as in `git.exe`) is still a pure MinGW program (and as a consequence, is quite a bit faster than Cygwin Git, at the price of certain quirks that Cygwin Git does not suffer).\n\nYou're quite right; I'd misread the MinGW page. Thank you for pointing \nthat out :)\n\n>>> We've gotten a lot of users on the list who ask why their Git\n>>> directories on shared drives aren't working (or are broken in some way).\n>>> Since I don't use Windows, let me ask: does the Cygwin DLL handle\n>>> link(2) properly on shared drives, and if not, would this patch help it\n>>> do so?  I can imagine that perhaps SMB doesn't support the necessary\n>>> operations to make a POSIX link(2) work properly.\n>> I'd need to go back to the Cygwin list to get a definite answer, but as\n>> I understand it, yes, this is is exactly the problem -- quoting Corinna,\n>> one of the Cygwin project leads, \"The MS NFS is not very reliable in\n>> keeping up with changes to metadata.\"\n>>\n>> We have verified that setting `core.createobject rename` resolves the\n>> problem for people who are seeing it, which very strongly implies that\n>> this build option would solve the problem similarly, but would fix it\n>> for all users, not just those who spend enough time investigating the\n>> problem to find that setting.\n>  From my experience, it appears that providing Corinna Vinschen (or better put: the Cygwin developers in general) with a sound patch gets things fixed pretty timely.\n>\n> And since `core.createObject = rename` seems to work around the problem, it should be possible to patch the Cygwin runtime accordingly. Sure, it will take a little investigation *what* code should be changed, and how, but the obvious benefit to *all* Cygwin applications should make that effort worth your while.\n\nHmm. I'm not sure what a Cygwin fix would look like here. As I \nunderstand it, using link(2) will fail if the target exists, while using \nrename(2) will just clobber a target file.\n\nIf the desired goal is that Cygwin's link(2) acts like POSIX link(2) on \nnetwork drives, I'm not convinced that's possible, at least not by \nemulating `core.createObject = rename` in the Cygwin library layer. The \nonly implementations I can think of would introduce a window condition \nbetween checking for the target file's existence and then clobbering it. \nThat'd mean the vast majority of the time the Cygwin command would work \nas expected, but there'd be race conditions where occasionally a file \ngets silently clobbered. Intermittent silent bugs seem much worse than \nthe command reliably failing.\n\nI think, given that, the current behaviour is preferable: calling link \njust errors out in this scenario, because there's no way to map it down \nto something that reliably acts like the POSIX call. It's then up to \nwhatever program is trying to use the call to find an alternative way to \nachieve its desired result, and the consequences of that workaround are \nthe responsibility of the calling application.\n\nIn short, as I understand it, this is exactly the scenario \nOBJECT_CREATION_USES_RENAMES is intended for.\n"},{"id":"267789","messageId":"xmqqwpx3t102.fsf@gitster.dls.corp.google.com","threadId":"40035","inReplyTo":"55C7883C.7040504@dinwoodie.org","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T19:08:29Z","receivedAt":"2015-08-10T19:08:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie <adam@dinwoodie.org> writes:\n\n> If the desired goal is that Cygwin's link(2) acts like POSIX link(2)\n> on network drives, I'm not convinced that's possible, at least not by\n> emulating `core.createObject = rename` in the Cygwin library\n> layer.\n\nThe way core.createObject=rename makes things work is by avoiding\nlink(2) in the first place and using rename(2) instead.  You might\nbe able to emulate rename(2) of A to B by doing a link(2) of A to B\nand then unlink(2) of A, but I do not think it is reasonable for the\nsystem call emulation layer to detect a sequence of link followed by\nunlink and use rename, i.e. emulationg the other way around.\n\nSo I suspect \"fix in Cygwin\" is a non-starter.\n\nBut in the end, I'd prefer the choice of the compiled-in default up\nto the port maintainers.  You earlier said:\n\n    This problem was reported on the Cygwin mailing list at\n    https://cygwin.com/ml/cygwin/2015-08/msg00102.html (amongst others) and\n    is being applied as a manual patch to the Cygwin builds until the patch\n    is taken here.\n\nso my preference is to see Cygwin continue to do so for a couple\nrelease cycles of ours to make sure all Cygwin end-users are happy\nand consider the flip of the default a good change for them, and\nthen the official Cygwin packager of Git sends a patch our way.\n\nhttps://cygwin.com/ml/cygwin/2015-08/msg00102.html seems to indicate\nthat somebody called Adam Dinwoodie is wearing Git maintainer hat,\nso perhaps you may be that \"official Cygwin packager of Git\" ;-)\n\nI agree with everything you said in that message to Peter---the\npatch should be included when you hear reports of `git config\ncore.createObject rename` helping more people.  After versions of\nCygwin Git package with such a change proves good, let's reduce the\nworkload of Cygwin Git maintainer by upstreaming that change to my\ntree.  But not before.\n\nThanks.\n"},{"id":"267820","messageId":"20150811100527.GW14466@dinwoodie.org","threadId":"40035","inReplyTo":"xmqqwpx3t102.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2015-08-11T10:05:27Z","receivedAt":"2015-08-11T10:05:27Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Mon, Aug 10, 2015 at 12:08:29PM -0700, Junio C Hamano wrote:\n> But in the end, I'd prefer the choice of the compiled-in default up\n> to the port maintainers.  You earlier said:\n> \n>     This problem was reported on the Cygwin mailing list at\n>     https://cygwin.com/ml/cygwin/2015-08/msg00102.html (amongst others) and\n>     is being applied as a manual patch to the Cygwin builds until the patch\n>     is taken here.\n> \n> so my preference is to see Cygwin continue to do so for a couple\n> release cycles of ours to make sure all Cygwin end-users are happy\n> and consider the flip of the default a good change for them, and\n> then the official Cygwin packager of Git sends a patch our way.\n\nWait a few releases then resubmit assuming we've not had complaints from\nCygwin user.  Okay!\n\n> https://cygwin.com/ml/cygwin/2015-08/msg00102.html seems to indicate\n> that somebody called Adam Dinwoodie is wearing Git maintainer hat,\n> so perhaps you may be that \"official Cygwin packager of Git\" ;-)\n\nThat would indeed be yours truly :)\n\n> I agree with everything you said in that message to Peter---the\n> patch should be included when you hear reports of `git config\n> core.createObject rename` helping more people.  After versions of\n> Cygwin Git package with such a change proves good, let's reduce the\n> workload of Cygwin Git maintainer by upstreaming that change to my\n> tree.  But not before.\n\nCool.  FWIW, the reason we've started applying this patch to the\ndownstream Cygwin builds is that I've now seen a number of reports\n(primarily on Stack Overflow) of this problem where that config change\nhas fixed things.  I'd been holding off until I had those extra reports,\nbut now I have them I made the patch and figured I'd submit it upstream\nat the same time.  As above, I'll wait a while until we're happy it's\nstable, and resubmit at that point.\n\n(On which note, I should probably submit the patch to the git-gui\nMakefile we've had in our builds since back in v1.7.9, before I took\nover the maintainership, since that one clearly is pretty stable...)\n\nThanks!\n"},{"id":"268255","messageId":"053a445c5b44e4c92dccba4dd5de26b9@www.dscho.org","threadId":"40035","inReplyTo":"55C7883C.7040504@dinwoodie.org","subject":"Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-18T15:44:31Z","receivedAt":"2015-08-18T15:44:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Adam,\n\nOn 2015-08-09 19:05, Adam Dinwoodie wrote:\n> On 09/08/2015 10:01, Johannes Schindelin wrote:\n>> On 2015-08-09 04:01, Adam Dinwoodie wrote:\n>>> \n>>>> We've gotten a lot of users on the list who ask why their Git\n>>>> directories on shared drives aren't working (or are broken in some way).\n>>>> Since I don't use Windows, let me ask: does the Cygwin DLL handle\n>>>> link(2) properly on shared drives, and if not, would this patch help it\n>>>> do so?  I can imagine that perhaps SMB doesn't support the necessary\n>>>> operations to make a POSIX link(2) work properly.\n>>>\n>>> I'd need to go back to the Cygwin list to get a definite answer, but as\n>>> I understand it, yes, this is is exactly the problem -- quoting Corinna,\n>>> one of the Cygwin project leads, \"The MS NFS is not very reliable in\n>>> keeping up with changes to metadata.\"\n>>>\n>>> We have verified that setting `core.createobject rename` resolves the\n>>> problem for people who are seeing it, which very strongly implies that\n>>> this build option would solve the problem similarly, but would fix it\n>>> for all users, not just those who spend enough time investigating the\n>>> problem to find that setting.\n>>\n>>  From my experience, it appears that providing Corinna Vinschen (or better put: the Cygwin developers in general) with a sound patch gets things fixed pretty timely.\n>>\n>> And since `core.createObject = rename` seems to work around the problem, it should be possible to patch the Cygwin runtime accordingly. Sure, it will take a little investigation *what* code should be changed, and how, but the obvious benefit to *all* Cygwin applications should make that effort worth your while.\n> \n> Hmm. I'm not sure what a Cygwin fix would look like here. As I\n> understand it, using link(2) will fail if the target exists, while\n> using rename(2) will just clobber a target file.\n\nThanks to you and Junio for explaining why my idea was bad. Sorry!\n\nCiao,\nJohannes\n"}]}