{"thread":{"id":"39797","subject":"Git force push fails after a rejected push (unpack failed)?","startedAt":"2015-07-07T13:45:50Z","lastAt":"2015-07-13T19:58:21Z","messageCount":17,"participants":["X H","Jeff King","Eric Sunshine","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"265671","messageId":"DUB120-W5049F72955243F44BB2511F6920@phx.gbl","threadId":"39797","inReplyTo":null,"subject":"Git force push fails after a rejected push (unpack failed)?","fromName":"X H","fromEmail":"music_is_live_lg@hotmail.com","sentAt":"2015-07-07T13:45:50Z","receivedAt":"2015-07-07T13:45:50Z","isPatch":false,"sender":{"key":"music_is_live_lg@hotmail.com","avatar":null},"body":"Hi,\n\nI have a remote repository on a network share configured as a file remote.\nI have pushed the HEAD of a branch to the remote, then amend the local HEAD. I then try to push the HEAD again but it was rejected.\nI made multiple attempts and then try to force the push with: $ git push -f origin branch_name but it failed with message:\n\nremote: error: unable to write sha1 filename objects/d9/4bfb39cd0be7497e493bd4045111a7b1158134: Permission denied\nremote: fatal: failed to write object\nerror: unpack failed: unpack-objects abnormal exit\nTo file://xxx\n ! [remote rejected] branch_name -> branch_name (unpacker error)\nerror: failed to push some refs to 'file://xxx'\n\nSometimes the error is due to a commit sha, sometimes due to a tree sha.\nIt seems that after a rejected push some files are not deleted and since they are read only on the remote, the next push cannot overwrite them.\n\nIs it the intended behaviour? I try to reproduce this and it seems it doesn't happen always.\n\ngit v2.3.5 (git-for-windows)\n\nThank you. \t\t \t   \t\t  "},{"id":"265686","messageId":"20150707141305.GA629@peff.net","threadId":"39797","inReplyTo":"DUB120-W5049F72955243F44BB2511F6920@phx.gbl","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-07T14:13:06Z","receivedAt":"2015-07-07T14:13:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2015 at 03:45:50PM +0200, X H wrote:\n\n> remote: error: unable to write sha1 filename objects/d9/4bfb39cd0be7497e493bd4045111a7b1158134: Permission denied\n> remote: fatal: failed to write object\n> error: unpack failed: unpack-objects abnormal exit\n\nIt looks like the permissions of one or more of the .git/objects/*\ndirectories on the server is not right. Just a guess, but this may be\nbecause you have multiple accounts pushing to it, and need to set umasks\nor use git's core.sharedrepository config to set the permissions for\neach object.\n\nYou'll probably have to ssh to the server and \"chown\" or \"chmod\" some\ndirectories to fix it in the first place, though.\n\n> Is it the intended behaviour? I try to reproduce this and it seems it\n> doesn't happen always.\n\nProbably only some object directories have the problem, so it would\ndepend on the set of exact objects you are sending, and possibly even\nthe number of objects (if you send a large number of objects, git will\nstore them as a pack, not as loose objects).\n\n-Peff\n"},{"id":"265763","messageId":"20150707194956.GA13792@peff.net","threadId":"39797","inReplyTo":"DUB120-W36B78FEE6DC80BDCB05D7FF6920@phx.gbl","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-07T19:49:56Z","receivedAt":"2015-07-07T19:49:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:\n\n> For the moment, I'm the only one pushing to the remote, always with\n> the same user (second user is planned). I use git-for-windows which is\n> based on MSYS2. I have mounted the network share with noacl option so\n> permissions should be handled by the Windows share. I'm in a group\n> which has read/write access. I have not configured\n> core.sharedrepository, I don't think it is useful with noacl since\n> unix group are not used in this case. The permission for the folder\n> above the file with permission denied is rw, but this file is read\n> only so if git try to modify it it won't work.\n\nAh, so this is not a push to a server, but to a share mounted on the\nlocal box?\n\nThat is leaving my realm of expertise. I'm not sure if it could be a\nmisconfiguration in your share setup, or that git is trying to do\nsomething that would work on a Unix machine, but not on a Windows share.\nYou might want to ask on the msysgit list:\n\n  https://groups.google.com/forum/#!forum/msysgit\n\n> Why does git try to write a file with the same name? If I amend a\n> commit isn't the sha modified?\n\nYes, but remember that git stores all of the objects for all of the\ncommits. So for some reason your push is perhaps trying to send an\nobject that the other side already has. Usually this does not happen\n(the receiver says \"I already have these commits, do not bother sending\ntheir objects\"), but it's possible that you have an object that is not\nreferenced by any commit, or a similar situation. It's hard to say\nwithout looking at the repository.\n\n-Peff\n"},{"id":"265783","messageId":"CAPig+cTMqjrM=EQFaTU+9OcC=-bSPLXoMR-NEH9Q-w782KiSmA@mail.gmail.com","threadId":"39797","inReplyTo":"20150707194956.GA13792@peff.net","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-07T23:05:11Z","receivedAt":"2015-07-07T23:05:11Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 7, 2015 at 3:49 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:\n>\n>> For the moment, I'm the only one pushing to the remote, always with\n>> the same user (second user is planned). I use git-for-windows which is\n>> based on MSYS2. I have mounted the network share with noacl option so\n>> permissions should be handled by the Windows share. I'm in a group\n>> which has read/write access. I have not configured\n>> core.sharedrepository, I don't think it is useful with noacl since\n>> unix group are not used in this case. The permission for the folder\n>> above the file with permission denied is rw, but this file is read\n>> only so if git try to modify it it won't work.\n>\n> Ah, so this is not a push to a server, but to a share mounted on the\n> local box?\n>\n> That is leaving my realm of expertise. I'm not sure if it could be a\n> misconfiguration in your share setup, or that git is trying to do\n> something that would work on a Unix machine, but not on a Windows share.\n> You might want to ask on the msysgit list:\n>\n>   https://groups.google.com/forum/#!forum/msysgit\n\nIs this possibly another case of Windows virus scanner interference?\nThat could account for its variable nature.\n"},{"id":"265864","messageId":"559D60DC.4010304@kdbg.org","threadId":"39797","inReplyTo":"20150707194956.GA13792@peff.net","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-07-08T17:41:48Z","receivedAt":"2015-07-08T17:41:48Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.07.2015 um 21:49 schrieb Jeff King:\n> On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:\n>\n>> For the moment, I'm the only one pushing to the remote, always with\n>> the same user (second user is planned). I use git-for-windows which is\n>> based on MSYS2. I have mounted the network share with noacl option so\n>> permissions should be handled by the Windows share. I'm in a group\n>> which has read/write access. I have not configured\n>> core.sharedrepository, I don't think it is useful with noacl since\n>> unix group are not used in this case. The permission for the folder\n>> above the file with permission denied is rw, but this file is read\n>> only so if git try to modify it it won't work.\n>\n> Ah, so this is not a push to a server, but to a share mounted on the\n> local box?\n>\n> That is leaving my realm of expertise. I'm not sure if it could be a\n> misconfiguration in your share setup, or that git is trying to do\n> something that would work on a Unix machine, but not on a Windows share.\n> You might want to ask on the msysgit list:\n>\n>    https://groups.google.com/forum/#!forum/msysgit\n>\n>> Why does git try to write a file with the same name? If I amend a\n>> commit isn't the sha modified?\n>\n> Yes, but remember that git stores all of the objects for all of the\n> commits. So for some reason your push is perhaps trying to send an\n> object that the other side already has. Usually this does not happen\n> (the receiver says \"I already have these commits, do not bother sending\n> their objects\"), but it's possible that you have an object that is not\n> referenced by any commit, or a similar situation. It's hard to say\n> without looking at the repository.\n\nAfter a non-fast-forward push fails, a subsequent forced push sends the \nsame set of objects, which are already present at the server side, but \nare dangling objects.\n\nApparently, Git for Windows fails to replace the read-only files that \nlive on the network file system.\n\nI have observed this recently, as well. I haven't dug into the detailed \nfailure mode, yet. In my case, I have a daily repack running on the \nserver side (it's a Samba share on a Linux box), that garbage-collects \nthe dangling objects. Usually, the next day the forced push is \nsuccessful. I know this is not very helpful if you can't wait a day for \nthe next push attempt...\n\n-- Hannes\n"},{"id":"265866","messageId":"20150708180539.GA12353@peff.net","threadId":"39797","inReplyTo":"559D60DC.4010304@kdbg.org","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-08T18:05:39Z","receivedAt":"2015-07-08T18:05:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2015 at 07:41:48PM +0200, Johannes Sixt wrote:\n\n> >Yes, but remember that git stores all of the objects for all of the\n> >commits. So for some reason your push is perhaps trying to send an\n> >object that the other side already has. Usually this does not happen\n> >(the receiver says \"I already have these commits, do not bother sending\n> >their objects\"), but it's possible that you have an object that is not\n> >referenced by any commit, or a similar situation. It's hard to say\n> >without looking at the repository.\n> \n> After a non-fast-forward push fails, a subsequent forced push sends the same\n> set of objects, which are already present at the server side, but are\n> dangling objects.\n> \n> Apparently, Git for Windows fails to replace the read-only files that live\n> on the network file system.\n\nI left one bit out from my original explanation, which is that\nwe generally prefer existing objects to new ones. So we would generally\nwant to throw out the new object rather than try to write it out. I'm\nnot sure why unpack-objects would try to write an object we already\nhave.\n\nWe also don't write objects directly, of course; we write to a temporary\nfile and try to link them into place. It really sounds more like the\n\"objects/d9\" directory is where the permission problems are. But, hmm...\n\nThe code path should be unpack-objects.c:write_object, which calls\nsha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These\ndays it uses the freshen_* functions instead of the latter, which does a\nsimilar check.  But it does report failure if we cannot call utime() on\nthe file, preferring to write it out instead (this is the safer choice\nfrom a preventing-prune-corruption perspective).\n\nSo it's possible that the sequence is:\n\n  - unpack-objects tries to write object 1234abcd...\n\n  - write_sha1_file calls freshen_loose_object\n\n  - we call access(\"12/34abcd...\", F_OK) and see that it does indeed\n    exist\n\n  - we call utime(\"12/34abcd...\") which fails (presumably due to EPERM);\n    we return failure and assume we must write out the object\n\n  - write_sha1_file then writes to a temporary file, and tries to link\n    it into place. Now what? If we get EEXIST, we say \"OK, somebody else\n    beat us here\", and we consider that a success. But presumably we get\n    some other error here (which may even be a Windows-ism), fall back\n    to rename(), and that fails with EPERM, which we then report.\n\nIf that's the case, then one solution is to have the\ntimestamp-freshening code silently report success, and skip writing out\nthe object. I'm not entirely comfortable with that, just because it is\nloosening a safety mechanism. But perhaps we could loosen it _only_ in\nthe case of checking the loose object, and when we get EPERM. We know\nthat the next step is going to be writing out that same loose object,\nwhich is almost certainly going to fail for the same reason.\n\nI dunno. The whole concept of trying to write to an object database for\nwhich you do not have permissions seems a little bit weird. This would\ndefinitely be a workaround. But I suspect it did work prior to v2.2.0.\n\n-Peff\n"},{"id":"265867","messageId":"20150708183331.GA16138@peff.net","threadId":"39797","inReplyTo":"20150708180539.GA12353@peff.net","subject":"[PATCH] check_and_freshen_file: fix reversed success-check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-08T18:33:31Z","receivedAt":"2015-07-08T18:33:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2015 at 02:05:39PM -0400, Jeff King wrote:\n\n> The code path should be unpack-objects.c:write_object, which calls\n> sha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These\n> days it uses the freshen_* functions instead of the latter, which does a\n> similar check.  But it does report failure if we cannot call utime() on\n> the file, preferring to write it out instead (this is the safer choice\n> from a preventing-prune-corruption perspective).\n> \n> So it's possible that the sequence is:\n> \n>   - unpack-objects tries to write object 1234abcd...\n> \n>   - write_sha1_file calls freshen_loose_object\n> \n>   - we call access(\"12/34abcd...\", F_OK) and see that it does indeed\n>     exist\n> \n>   - we call utime(\"12/34abcd...\") which fails (presumably due to EPERM);\n>     we return failure and assume we must write out the object\n\n...or maybe in the utime() step there is actually a bug, and we report\nfailure for no good reason. Ugh.\n\n-- >8 --\nSubject: check_and_freshen_file: fix reversed success-check\n\nWhen we want to write out a loose object file, we have\nalways first made sure we don't already have the object\nsomewhere. Since 33d4221 (write_sha1_file: freshen existing\nobjects, 2014-10-15), we also update the timestamp on the\nfile, so that a simultaneous prune knows somebody is\nlikely to reference it soon.\n\nIf our utime() call fails, we treat this the same as not\nhaving the object in the first place; the safe thing to do\nis write out another copy. However, the loose-object check\naccidentally inverst the utime() check; it returns failure\n_only_ when the utime() call actually succeeded. Thus it was\nfailing to protect us there, and in the normal case where\nutime() succeeds, it caused us to pointlessly write out and\nlink the object.\n\nThis passed our freshening tests, because writing out the\nnew object is certainly _one_ way of updating its utime. So\nthe normal case of a successful utime() was inefficient, but\nnot wrong.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe worst part of this is that I had the _same_ bug in the pack code\npath when I initially posted what became 33d4221. René noticed during\nreview, and my fix was to invert the return value from freshen_file to\nmatch the other functions. But of course doing that without fixing the\nother caller meant I introduced the same bug there.\n\nI'll be curious if this fixes the problem the OP is seeing. If not, then\nwe can dig deeper into the weird EPERM problems around this particular\nobject database.\n\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 77cd81d..721eadc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -454,7 +454,7 @@ static int check_and_freshen_file(const char *fn, int freshen)\n {\n \tif (access(fn, F_OK))\n \t\treturn 0;\n-\tif (freshen && freshen_file(fn))\n+\tif (freshen && !freshen_file(fn))\n \t\treturn 0;\n \treturn 1;\n }\n-- \n2.5.0.rc1.340.ge59e3eb\n"},{"id":"265870","messageId":"xmqqegkixxja.fsf@gitster.dls.corp.google.com","threadId":"39797","inReplyTo":"20150708183331.GA16138@peff.net","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-08T19:24:41Z","receivedAt":"2015-07-08T19:24:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: check_and_freshen_file: fix reversed success-check\n>\n> When we want to write out a loose object file, we have\n> always first made sure we don't already have the object\n> somewhere. Since 33d4221 (write_sha1_file: freshen existing\n> objects, 2014-10-15), we also update the timestamp on the\n> file, so that a simultaneous prune knows somebody is\n> likely to reference it soon.\n>\n> If our utime() call fails, we treat this the same as not\n> having the object in the first place; the safe thing to do\n> is write out another copy. However, the loose-object check\n> accidentally inverst the utime() check; it returns failure\n\ns/inverst/invert/?\n\n> _only_ when the utime() call actually succeeded. Thus it was\n> failing to protect us there, and in the normal case where\n> utime() succeeds, it caused us to pointlessly write out and\n> link the object.\n>\n> This passed our freshening tests, because writing out the\n> new object is certainly _one_ way of updating its utime. So\n> the normal case of a successful utime() was inefficient, but\n> not wrong.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The worst part of this is that I had the _same_ bug in the pack code\n> path when I initially posted what became 33d4221. René noticed during\n> review, and my fix was to invert the return value from freshen_file to\n> match the other functions. But of course doing that without fixing the\n> other caller meant I introduced the same bug there.\n\nI think each of the functions in the check_and_freshen_* callchain\ncan at least have a comment in front of it, saying what the returned\nvalue means, to unconfuse readers.  \"Return 1 when the thing exists\nand no further action is necessary; return 0 when the thing does not\nexist or not in a good state and should be overwritten (if the\ncaller has something to overwrite it with, that is)\" or something?\n\nTheir returning \"1\" instead of \"-1\" could be taken as a hint that\nsays \"this non-zero return does not signal a _failure_\", but it is a\nrather weak hint.\n\n>\n> I'll be curious if this fixes the problem the OP is seeing. If not, then\n> we can dig deeper into the weird EPERM problems around this particular\n> object database.\n>\n>  sha1_file.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 77cd81d..721eadc 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -454,7 +454,7 @@ static int check_and_freshen_file(const char *fn, int freshen)\n>  {\n>  \tif (access(fn, F_OK))\n>  \t\treturn 0;\n> -\tif (freshen && freshen_file(fn))\n> +\tif (freshen && !freshen_file(fn))\n>  \t\treturn 0;\n>  \treturn 1;\n>  }\n"},{"id":"265874","messageId":"BLU436-SMTP166B786EBE483D3F47F666F6910@phx.gbl","threadId":"39797","inReplyTo":"20150708180539.GA12353@peff.net","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"X H","fromEmail":"music_is_live_lg@hotmail.com","sentAt":"2015-07-08T20:28:51Z","receivedAt":"2015-07-08T20:28:51Z","isPatch":false,"sender":{"key":"music_is_live_lg@hotmail.com","avatar":null},"body":"Le 8/07/2015 20:05, Jeff King a écrit :\n> On Wed, Jul 08, 2015 at 07:41:48PM +0200, Johannes Sixt wrote:\n>\n>>> Yes, but remember that git stores all of the objects for all of the\n>>> commits. So for some reason your push is perhaps trying to send an\n>>> object that the other side already has. Usually this does not happen\n>>> (the receiver says \"I already have these commits, do not bother sending\n>>> their objects\"), but it's possible that you have an object that is not\n>>> referenced by any commit, or a similar situation. It's hard to say\n>>> without looking at the repository.\n>>\n>> After a non-fast-forward push fails, a subsequent forced push sends the same\n>> set of objects, which are already present at the server side, but are\n>> dangling objects.\n>>\n>> Apparently, Git for Windows fails to replace the read-only files that live\n>> on the network file system.\n>\n> I left one bit out from my original explanation, which is that\n> we generally prefer existing objects to new ones. So we would generally\n> want to throw out the new object rather than try to write it out. I'm\n> not sure why unpack-objects would try to write an object we already\n> have.\n>\n> We also don't write objects directly, of course; we write to a temporary\n> file and try to link them into place. It really sounds more like the\n> \"objects/d9\" directory is where the permission problems are. But, hmm...\n>\n> The code path should be unpack-objects.c:write_object, which calls\n> sha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These\n> days it uses the freshen_* functions instead of the latter, which does a\n> similar check.  But it does report failure if we cannot call utime() on\n> the file, preferring to write it out instead (this is the safer choice\n> from a preventing-prune-corruption perspective).\n>\n> So it's possible that the sequence is:\n>\n>    - unpack-objects tries to write object 1234abcd...\n>\n>    - write_sha1_file calls freshen_loose_object\n>\n>    - we call access(\"12/34abcd...\", F_OK) and see that it does indeed\n>      exist\n>\n>    - we call utime(\"12/34abcd...\") which fails (presumably due to EPERM);\n>      we return failure and assume we must write out the object\n>\n>    - write_sha1_file then writes to a temporary file, and tries to link\n>      it into place. Now what? If we get EEXIST, we say \"OK, somebody else\n>      beat us here\", and we consider that a success. But presumably we get\n>      some other error here (which may even be a Windows-ism), fall back\n>      to rename(), and that fails with EPERM, which we then report.\n>\n> If that's the case, then one solution is to have the\n> timestamp-freshening code silently report success, and skip writing out\n> the object. I'm not entirely comfortable with that, just because it is\n> loosening a safety mechanism. But perhaps we could loosen it _only_ in\n> the case of checking the loose object, and when we get EPERM. We know\n> that the next step is going to be writing out that same loose object,\n> which is almost certainly going to fail for the same reason.\n>\n> I dunno. The whole concept of trying to write to an object database for\n> which you do not have permissions seems a little bit weird. This would\n> definitely be a workaround. But I suspect it did work prior to v2.2.0.\n>\n> -Peff\n>\n\nI've made some tests:\n1) Linux Mint:\n- ntfs share mounted (with noacl) since\n$ chacl -l /the/mountpoint gives\nchacl: cannot get access ACL on '/the/mountpoint': Operation not supported\n- Create a remote repository on the ntfs share\n- Push some test files\n- ls -al for files in the objects folder gives:\n-rw-r--r--\n- with subsequent amend, push the permissions stays the same.\n\n2) On Windows:\n- ntfs share mapped as drive by windows\n- remote repository created by git on windows\n- Push some test files\n- ls -al for files in the objects folder gives:\n-r--r--r--\n- chmod all files to add w permission\n- then amend commit and push, push is successful but all files rewritten \nby the push are again:\n-r--r--r--\nbut all folders under objects are:\ndrwxr-xr-x\n\nSo I suppose it some issue with Windows or MSYS2. I'm not sure it's a \nproblem related to your explanations, I will read it whit fresh mind.\n"},{"id":"265875","messageId":"20150708203352.GA23901@peff.net","threadId":"39797","inReplyTo":"xmqqegkixxja.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] check_and_freshen_file: fix reversed success-check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-08T20:33:52Z","receivedAt":"2015-07-08T20:33:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2015 at 12:24:41PM -0700, Junio C Hamano wrote:\n\n> > If our utime() call fails, we treat this the same as not\n> > having the object in the first place; the safe thing to do\n> > is write out another copy. However, the loose-object check\n> > accidentally inverst the utime() check; it returns failure\n> \n> s/inverst/invert/?\n\nWhoops, should be \"inverts\".\n\n> I think each of the functions in the check_and_freshen_* callchain\n> can at least have a comment in front of it, saying what the returned\n> value means, to unconfuse readers.  \"Return 1 when the thing exists\n> and no further action is necessary; return 0 when the thing does not\n> exist or not in a good state and should be overwritten (if the\n> caller has something to overwrite it with, that is)\" or something?\n> \n> Their returning \"1\" instead of \"-1\" could be taken as a hint that\n> says \"this non-zero return does not signal a _failure_\", but it is a\n> rather weak hint.\n\nFair enough. Here's a re-roll with some extra comments, and the typo-fix\nfrom above.\n\n-- >8 --\nSubject: check_and_freshen_file: fix reversed success-check\n\nWhen we want to write out a loose object file, we have\nalways first made sure we don't already have the object\nsomewhere. Since 33d4221 (write_sha1_file: freshen existing\nobjects, 2014-10-15), we also update the timestamp on the\nfile, so that a simultaneous prune knows somebody is\nlikely to reference it soon.\n\nIf our utime() call fails, we treat this the same as not\nhaving the object in the first place; the safe thing to do\nis write out another copy. However, the loose-object check\naccidentally inverts the utime() check; it returns failure\n_only_ when the utime() call actually succeeded. Thus it was\nfailing to protect us there, and in the normal case where\nutime() succeeds, it caused us to pointlessly write out and\nlink the object.\n\nThis passed our freshening tests, because writing out the\nnew object is certainly _one_ way of updating its utime. So\nthe normal case was inefficient, but not wrong.\n\nWhile we're here, let's also drop a comment in front of the\ncheck_and_freshen functions, making a note of their return\ntype (since it is not our usual \"0 for success, -1 for\nerror\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 77cd81d..e435289 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -443,6 +443,7 @@ void prepare_alt_odb(void)\n \tread_info_alternates(get_object_directory(), 0);\n }\n \n+/* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n static int freshen_file(const char *fn)\n {\n \tstruct utimbuf t;\n@@ -450,11 +451,18 @@ static int freshen_file(const char *fn)\n \treturn !utime(fn, &t);\n }\n \n+/*\n+ * All of the check_and_freshen functions return 1 if the file exists and was\n+ * freshened (if freshening was requested), 0 otherwise. If they return\n+ * 0, you should not assume that it is safe to skip a write of the object (it\n+ * either does not exist on disk, or has a stale mtime and may be subject to\n+ * pruning).\n+ */\n static int check_and_freshen_file(const char *fn, int freshen)\n {\n \tif (access(fn, F_OK))\n \t\treturn 0;\n-\tif (freshen && freshen_file(fn))\n+\tif (freshen && !freshen_file(fn))\n \t\treturn 0;\n \treturn 1;\n }\n-- \n2.5.0.rc1.340.ge59e3eb\n"},{"id":"265878","messageId":"559D8E77.60401@kdbg.org","threadId":"39797","inReplyTo":"20150708180539.GA12353@peff.net","subject":"Re: Git force push fails after a rejected push (unpack failed)?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-07-08T20:56:23Z","receivedAt":"2015-07-08T20:56:23Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.07.2015 um 20:05 schrieb Jeff King:\n> We also don't write objects directly, of course; we write to a temporary\n> file and try to link them into place. It really sounds more like the\n> \"objects/d9\" directory is where the permission problems are. But, hmm...\n\nNot on Windows: A read-only file cannot be overwritten or removed, \nregardless of the permissions of the directory. We do treat this case \nmingw_rename, but I have a slight suspicion that this does not work \nsufficiently reliably on networked file systems.\n\n-- Hannes\n"},{"id":"265879","messageId":"559D9006.20102@kdbg.org","threadId":"39797","inReplyTo":"20150708183331.GA16138@peff.net","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-07-08T21:03:02Z","receivedAt":"2015-07-08T21:03:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.07.2015 um 20:33 schrieb Jeff King:\n> ...or maybe in the utime() step there is actually a bug, and we report\n> failure for no good reason. Ugh.\n\nAh! That code is less than a year old. When I began to adopt a workflow \nrequiring force-pushes lately, I wondered why I haven't seen these \nfailures earlier, because I did do force pushes in the past, but not \nthat frequently. I thought that I had just been lucky. But this would \nexplain it.\n\n-- Hannes\n"},{"id":"265944","messageId":"559EDEE6.1040807@kdbg.org","threadId":"39797","inReplyTo":"559D9006.20102@kdbg.org","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-07-09T20:51:50Z","receivedAt":"2015-07-09T20:51:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.07.2015 um 23:03 schrieb Johannes Sixt:\n> Am 08.07.2015 um 20:33 schrieb Jeff King:\n>> ...or maybe in the utime() step there is actually a bug, and we report\n>> failure for no good reason. Ugh.\n>\n> Ah! That code is less than a year old. When I began to adopt a workflow\n> requiring force-pushes lately, I wondered why I haven't seen these\n> failures earlier, because I did do force pushes in the past, but not\n> that frequently. I thought that I had just been lucky. But this would\n> explain it.\n\nAnd, in fact, with this patch these particular failures are gone! Thank \nyou so much!\n\n-- Hannes\n"},{"id":"265952","messageId":"20150709224830.GA24998@peff.net","threadId":"39797","inReplyTo":"559EDEE6.1040807@kdbg.org","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-09T22:48:30Z","receivedAt":"2015-07-09T22:48:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 09, 2015 at 10:51:50PM +0200, Johannes Sixt wrote:\n\n> >Ah! That code is less than a year old. When I began to adopt a workflow\n> >requiring force-pushes lately, I wondered why I haven't seen these\n> >failures earlier, because I did do force pushes in the past, but not\n> >that frequently. I thought that I had just been lucky. But this would\n> >explain it.\n> \n> And, in fact, with this patch these particular failures are gone! Thank you\n> so much!\n\nGreat, thanks for testing. You can temper your appreciation by noticing\nthat I introduced the bug in the first place. ;)\n\n-Peff\n"},{"id":"266044","messageId":"BLU437-SMTP311678316C434857657D6BF69E0@phx.gbl","threadId":"39797","inReplyTo":"20150709224830.GA24998@peff.net","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"X H","fromEmail":"music_is_live_lg@hotmail.com","sentAt":"2015-07-11T22:21:33Z","receivedAt":"2015-07-11T22:21:33Z","isPatch":true,"sender":{"key":"music_is_live_lg@hotmail.com","avatar":null},"body":"Le 10/07/2015 0:48, Jeff King a écrit :\n> On Thu, Jul 09, 2015 at 10:51:50PM +0200, Johannes Sixt wrote:\n>\n>>> Ah! That code is less than a year old. When I began to adopt a workflow\n>>> requiring force-pushes lately, I wondered why I haven't seen these\n>>> failures earlier, because I did do force pushes in the past, but not\n>>> that frequently. I thought that I had just been lucky. But this would\n>>> explain it.\n>>\n>> And, in fact, with this patch these particular failures are gone! Thank you\n>> so much!\n>\n> Great, thanks for testing. You can temper your appreciation by noticing\n> that I introduced the bug in the first place. ;)\n>\n> -Peff\n>\nHi,\n\nThank you for the patch. I hope it will solve the problem and permit to \nhave a second user using the same repository.\n\nHow are the permission handled, is it git that is asking to create a \nfile read only or rw on the remote or is it the environment with umask \nans so on that decides it, or Windows when the drive is mounted with noacl?\n"},{"id":"266072","messageId":"20150713035224.GA3315@peff.net","threadId":"39797","inReplyTo":"BLU437-SMTP311678316C434857657D6BF69E0@phx.gbl","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-13T03:52:24Z","receivedAt":"2015-07-13T03:52:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 12, 2015 at 12:21:33AM +0200, X H wrote:\n\n> How are the permission handled, is it git that is asking to create a file\n> read only or rw on the remote or is it the environment with umask ans so on\n> that decides it, or Windows when the drive is mounted with noacl?\n\nGenerally, git follows the umask when creating most files. However, for\nthe object files in the object database, it does drop the \"w\" bit, as\nonce written, they should never be changed (after all, the filename is a\nhash of the contents). We don't ever open those files for writing, but\nwe may try to rename another file over them; that might behave\ndifferently on Unix versus Windows (or even differently on Windows\nbetween local and remote-mounted filesystems).\n\n-Peff\n"},{"id":"266089","messageId":"BLU437-SMTP9300461B7BF5F0DBDBE51BF69C0@phx.gbl","threadId":"39797","inReplyTo":"20150713035224.GA3315@peff.net","subject":"Re: [PATCH] check_and_freshen_file: fix reversed success-check","fromName":"X H","fromEmail":"music_is_live_lg@hotmail.com","sentAt":"2015-07-13T19:58:21Z","receivedAt":"2015-07-13T19:58:21Z","isPatch":true,"sender":{"key":"music_is_live_lg@hotmail.com","avatar":null},"body":"Le 13/07/2015 5:52, Jeff King a écrit :\n> On Sun, Jul 12, 2015 at 12:21:33AM +0200, X H wrote:\n>\n>> How are the permission handled, is it git that is asking to create a file\n>> read only or rw on the remote or is it the environment with umask ans so on\n>> that decides it, or Windows when the drive is mounted with noacl?\n>\n> Generally, git follows the umask when creating most files. However, for\n> the object files in the object database, it does drop the \"w\" bit, as\n> once written, they should never be changed (after all, the filename is a\n> hash of the contents). We don't ever open those files for writing, but\n> we may try to rename another file over them; that might behave\n> differently on Unix versus Windows (or even differently on Windows\n> between local and remote-mounted filesystems).\n>\n> -Peff\n>\n\nHi,\n\nWhen mounted on Linux the object files were created rw on the share, but \nwhen mounted on Windows the temporary files are created with read-only \nattributes.\n\nThank you for your patch, forced push are now working from \ngit-on-windows to smb folders shared from another Windows machine. I've \nnot yet tested to push to the share with another user, but I think it \nshould makes no diff.\n"}]}