git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* Git force push fails after a rejected push (unpack failed)?
@ 2015-07-07 13:45 X H
  2015-07-07 14:13 ` Jeff King
  0 siblings, 1 reply; 17+ messages in thread
From: X H @ 2015-07-07 13:45 UTC (permalink / raw)
  To: git@vger.kernel.org

Hi,

I have a remote repository on a network share configured as a file remote.
I 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.
I made multiple attempts and then try to force the push with: $ git push -f origin branch_name but it failed with message:

remote: error: unable to write sha1 filename objects/d9/4bfb39cd0be7497e493bd4045111a7b1158134: Permission denied
remote: fatal: failed to write object
error: unpack failed: unpack-objects abnormal exit
To file://xxx
 ! [remote rejected] branch_name -> branch_name (unpacker error)
error: failed to push some refs to 'file://xxx'

Sometimes the error is due to a commit sha, sometimes due to a tree sha.
It 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.

Is it the intended behaviour? I try to reproduce this and it seems it doesn't happen always.

git v2.3.5 (git-for-windows)

Thank you. 		 	   		  

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
  2015-07-07 13:45 Git force push fails after a rejected push (unpack failed)? X H
@ 2015-07-07 14:13 ` Jeff King
       [not found]   ` <DUB120-W36B78FEE6DC80BDCB05D7FF6920@phx.gbl>
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2015-07-07 14:13 UTC (permalink / raw)
  To: X H; +Cc: git@vger.kernel.org

On Tue, Jul 07, 2015 at 03:45:50PM +0200, X H wrote:

> remote: error: unable to write sha1 filename objects/d9/4bfb39cd0be7497e493bd4045111a7b1158134: Permission denied
> remote: fatal: failed to write object
> error: unpack failed: unpack-objects abnormal exit

It looks like the permissions of one or more of the .git/objects/*
directories on the server is not right. Just a guess, but this may be
because you have multiple accounts pushing to it, and need to set umasks
or use git's core.sharedrepository config to set the permissions for
each object.

You'll probably have to ssh to the server and "chown" or "chmod" some
directories to fix it in the first place, though.

> Is it the intended behaviour? I try to reproduce this and it seems it
> doesn't happen always.

Probably only some object directories have the problem, so it would
depend on the set of exact objects you are sending, and possibly even
the number of objects (if you send a large number of objects, git will
store them as a pack, not as loose objects).

-Peff

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
       [not found]   ` <DUB120-W36B78FEE6DC80BDCB05D7FF6920@phx.gbl>
@ 2015-07-07 19:49     ` Jeff King
  2015-07-07 23:05       ` Eric Sunshine
  2015-07-08 17:41       ` Johannes Sixt
  0 siblings, 2 replies; 17+ messages in thread
From: Jeff King @ 2015-07-07 19:49 UTC (permalink / raw)
  To: X H; +Cc: git@vger.kernel.org

On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:

> For the moment, I'm the only one pushing to the remote, always with
> the same user (second user is planned). I use git-for-windows which is
> based on MSYS2. I have mounted the network share with noacl option so
> permissions should be handled by the Windows share. I'm in a group
> which has read/write access. I have not configured
> core.sharedrepository, I don't think it is useful with noacl since
> unix group are not used in this case. The permission for the folder
> above the file with permission denied is rw, but this file is read
> only so if git try to modify it it won't work.

Ah, so this is not a push to a server, but to a share mounted on the
local box?

That is leaving my realm of expertise. I'm not sure if it could be a
misconfiguration in your share setup, or that git is trying to do
something that would work on a Unix machine, but not on a Windows share.
You might want to ask on the msysgit list:

  https://groups.google.com/forum/#!forum/msysgit

> Why does git try to write a file with the same name? If I amend a
> commit isn't the sha modified?

Yes, but remember that git stores all of the objects for all of the
commits. So for some reason your push is perhaps trying to send an
object that the other side already has. Usually this does not happen
(the receiver says "I already have these commits, do not bother sending
their objects"), but it's possible that you have an object that is not
referenced by any commit, or a similar situation. It's hard to say
without looking at the repository.

-Peff

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
  2015-07-07 19:49     ` Jeff King
@ 2015-07-07 23:05       ` Eric Sunshine
  2015-07-08 17:41       ` Johannes Sixt
  1 sibling, 0 replies; 17+ messages in thread
From: Eric Sunshine @ 2015-07-07 23:05 UTC (permalink / raw)
  To: Jeff King; +Cc: X H, git@vger.kernel.org

On Tue, Jul 7, 2015 at 3:49 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:
>
>> For the moment, I'm the only one pushing to the remote, always with
>> the same user (second user is planned). I use git-for-windows which is
>> based on MSYS2. I have mounted the network share with noacl option so
>> permissions should be handled by the Windows share. I'm in a group
>> which has read/write access. I have not configured
>> core.sharedrepository, I don't think it is useful with noacl since
>> unix group are not used in this case. The permission for the folder
>> above the file with permission denied is rw, but this file is read
>> only so if git try to modify it it won't work.
>
> Ah, so this is not a push to a server, but to a share mounted on the
> local box?
>
> That is leaving my realm of expertise. I'm not sure if it could be a
> misconfiguration in your share setup, or that git is trying to do
> something that would work on a Unix machine, but not on a Windows share.
> You might want to ask on the msysgit list:
>
>   https://groups.google.com/forum/#!forum/msysgit

Is this possibly another case of Windows virus scanner interference?
That could account for its variable nature.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
  2015-07-07 19:49     ` Jeff King
  2015-07-07 23:05       ` Eric Sunshine
@ 2015-07-08 17:41       ` Johannes Sixt
  2015-07-08 18:05         ` Jeff King
  1 sibling, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2015-07-08 17:41 UTC (permalink / raw)
  To: Jeff King, X H; +Cc: git@vger.kernel.org

Am 07.07.2015 um 21:49 schrieb Jeff King:
> On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:
>
>> For the moment, I'm the only one pushing to the remote, always with
>> the same user (second user is planned). I use git-for-windows which is
>> based on MSYS2. I have mounted the network share with noacl option so
>> permissions should be handled by the Windows share. I'm in a group
>> which has read/write access. I have not configured
>> core.sharedrepository, I don't think it is useful with noacl since
>> unix group are not used in this case. The permission for the folder
>> above the file with permission denied is rw, but this file is read
>> only so if git try to modify it it won't work.
>
> Ah, so this is not a push to a server, but to a share mounted on the
> local box?
>
> That is leaving my realm of expertise. I'm not sure if it could be a
> misconfiguration in your share setup, or that git is trying to do
> something that would work on a Unix machine, but not on a Windows share.
> You might want to ask on the msysgit list:
>
>    https://groups.google.com/forum/#!forum/msysgit
>
>> Why does git try to write a file with the same name? If I amend a
>> commit isn't the sha modified?
>
> Yes, but remember that git stores all of the objects for all of the
> commits. So for some reason your push is perhaps trying to send an
> object that the other side already has. Usually this does not happen
> (the receiver says "I already have these commits, do not bother sending
> their objects"), but it's possible that you have an object that is not
> referenced by any commit, or a similar situation. It's hard to say
> without looking at the repository.

After a non-fast-forward push fails, a subsequent forced push sends the 
same set of objects, which are already present at the server side, but 
are dangling objects.

Apparently, Git for Windows fails to replace the read-only files that 
live on the network file system.

I have observed this recently, as well. I haven't dug into the detailed 
failure mode, yet. In my case, I have a daily repack running on the 
server side (it's a Samba share on a Linux box), that garbage-collects 
the dangling objects. Usually, the next day the forced push is 
successful. I know this is not very helpful if you can't wait a day for 
the next push attempt...

-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
  2015-07-08 17:41       ` Johannes Sixt
@ 2015-07-08 18:05         ` Jeff King
  2015-07-08 18:33           ` [PATCH] check_and_freshen_file: fix reversed success-check Jeff King
                             ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Jeff King @ 2015-07-08 18:05 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: X H, git@vger.kernel.org

On Wed, Jul 08, 2015 at 07:41:48PM +0200, Johannes Sixt wrote:

> >Yes, but remember that git stores all of the objects for all of the
> >commits. So for some reason your push is perhaps trying to send an
> >object that the other side already has. Usually this does not happen
> >(the receiver says "I already have these commits, do not bother sending
> >their objects"), but it's possible that you have an object that is not
> >referenced by any commit, or a similar situation. It's hard to say
> >without looking at the repository.
> 
> After a non-fast-forward push fails, a subsequent forced push sends the same
> set of objects, which are already present at the server side, but are
> dangling objects.
> 
> Apparently, Git for Windows fails to replace the read-only files that live
> on the network file system.

I left one bit out from my original explanation, which is that
we generally prefer existing objects to new ones. So we would generally
want to throw out the new object rather than try to write it out. I'm
not sure why unpack-objects would try to write an object we already
have.

We also don't write objects directly, of course; we write to a temporary
file and try to link them into place. It really sounds more like the
"objects/d9" directory is where the permission problems are. But, hmm...

The code path should be unpack-objects.c:write_object, which calls
sha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These
days it uses the freshen_* functions instead of the latter, which does a
similar check.  But it does report failure if we cannot call utime() on
the file, preferring to write it out instead (this is the safer choice
from a preventing-prune-corruption perspective).

So it's possible that the sequence is:

  - unpack-objects tries to write object 1234abcd...

  - write_sha1_file calls freshen_loose_object

  - we call access("12/34abcd...", F_OK) and see that it does indeed
    exist

  - we call utime("12/34abcd...") which fails (presumably due to EPERM);
    we return failure and assume we must write out the object

  - write_sha1_file then writes to a temporary file, and tries to link
    it into place. Now what? If we get EEXIST, we say "OK, somebody else
    beat us here", and we consider that a success. But presumably we get
    some other error here (which may even be a Windows-ism), fall back
    to rename(), and that fails with EPERM, which we then report.

If that's the case, then one solution is to have the
timestamp-freshening code silently report success, and skip writing out
the object. I'm not entirely comfortable with that, just because it is
loosening a safety mechanism. But perhaps we could loosen it _only_ in
the case of checking the loose object, and when we get EPERM. We know
that the next step is going to be writing out that same loose object,
which is almost certainly going to fail for the same reason.

I dunno. The whole concept of trying to write to an object database for
which you do not have permissions seems a little bit weird. This would
definitely be a workaround. But I suspect it did work prior to v2.2.0.

-Peff

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-08 18:05         ` Jeff King
@ 2015-07-08 18:33           ` Jeff King
  2015-07-08 19:24             ` Junio C Hamano
  2015-07-08 21:03             ` [PATCH] " Johannes Sixt
  2015-07-08 20:28           ` Git force push fails after a rejected push (unpack failed)? X H
  2015-07-08 20:56           ` Johannes Sixt
  2 siblings, 2 replies; 17+ messages in thread
From: Jeff King @ 2015-07-08 18:33 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, René Scharfe, X H, git@vger.kernel.org

On Wed, Jul 08, 2015 at 02:05:39PM -0400, Jeff King wrote:

> The code path should be unpack-objects.c:write_object, which calls
> sha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These
> days it uses the freshen_* functions instead of the latter, which does a
> similar check.  But it does report failure if we cannot call utime() on
> the file, preferring to write it out instead (this is the safer choice
> from a preventing-prune-corruption perspective).
> 
> So it's possible that the sequence is:
> 
>   - unpack-objects tries to write object 1234abcd...
> 
>   - write_sha1_file calls freshen_loose_object
> 
>   - we call access("12/34abcd...", F_OK) and see that it does indeed
>     exist
> 
>   - we call utime("12/34abcd...") which fails (presumably due to EPERM);
>     we return failure and assume we must write out the object

...or maybe in the utime() step there is actually a bug, and we report
failure for no good reason. Ugh.

-- >8 --
Subject: check_and_freshen_file: fix reversed success-check

When we want to write out a loose object file, we have
always first made sure we don't already have the object
somewhere. Since 33d4221 (write_sha1_file: freshen existing
objects, 2014-10-15), we also update the timestamp on the
file, so that a simultaneous prune knows somebody is
likely to reference it soon.

If our utime() call fails, we treat this the same as not
having the object in the first place; the safe thing to do
is write out another copy. However, the loose-object check
accidentally inverst the utime() check; it returns failure
_only_ when the utime() call actually succeeded. Thus it was
failing to protect us there, and in the normal case where
utime() succeeds, it caused us to pointlessly write out and
link the object.

This passed our freshening tests, because writing out the
new object is certainly _one_ way of updating its utime. So
the normal case of a successful utime() was inefficient, but
not wrong.

Signed-off-by: Jeff King <peff@peff.net>
---
The worst part of this is that I had the _same_ bug in the pack code
path when I initially posted what became 33d4221. René noticed during
review, and my fix was to invert the return value from freshen_file to
match the other functions. But of course doing that without fixing the
other caller meant I introduced the same bug there.

I'll be curious if this fixes the problem the OP is seeing. If not, then
we can dig deeper into the weird EPERM problems around this particular
object database.

 sha1_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 77cd81d..721eadc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -454,7 +454,7 @@ static int check_and_freshen_file(const char *fn, int freshen)
 {
 	if (access(fn, F_OK))
 		return 0;
-	if (freshen && freshen_file(fn))
+	if (freshen && !freshen_file(fn))
 		return 0;
 	return 1;
 }
-- 
2.5.0.rc1.340.ge59e3eb

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-08 18:33           ` [PATCH] check_and_freshen_file: fix reversed success-check Jeff King
@ 2015-07-08 19:24             ` Junio C Hamano
  2015-07-08 20:33               ` [PATCH v2] " Jeff King
  2015-07-08 21:03             ` [PATCH] " Johannes Sixt
  1 sibling, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2015-07-08 19:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Sixt, René Scharfe, X H, git@vger.kernel.org

Jeff King <peff@peff.net> writes:

> Subject: check_and_freshen_file: fix reversed success-check
>
> When we want to write out a loose object file, we have
> always first made sure we don't already have the object
> somewhere. Since 33d4221 (write_sha1_file: freshen existing
> objects, 2014-10-15), we also update the timestamp on the
> file, so that a simultaneous prune knows somebody is
> likely to reference it soon.
>
> If our utime() call fails, we treat this the same as not
> having the object in the first place; the safe thing to do
> is write out another copy. However, the loose-object check
> accidentally inverst the utime() check; it returns failure

s/inverst/invert/?

> _only_ when the utime() call actually succeeded. Thus it was
> failing to protect us there, and in the normal case where
> utime() succeeds, it caused us to pointlessly write out and
> link the object.
>
> This passed our freshening tests, because writing out the
> new object is certainly _one_ way of updating its utime. So
> the normal case of a successful utime() was inefficient, but
> not wrong.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The worst part of this is that I had the _same_ bug in the pack code
> path when I initially posted what became 33d4221. René noticed during
> review, and my fix was to invert the return value from freshen_file to
> match the other functions. But of course doing that without fixing the
> other caller meant I introduced the same bug there.

I think each of the functions in the check_and_freshen_* callchain
can at least have a comment in front of it, saying what the returned
value means, to unconfuse readers.  "Return 1 when the thing exists
and no further action is necessary; return 0 when the thing does not
exist or not in a good state and should be overwritten (if the
caller has something to overwrite it with, that is)" or something?

Their returning "1" instead of "-1" could be taken as a hint that
says "this non-zero return does not signal a _failure_", but it is a
rather weak hint.

>
> I'll be curious if this fixes the problem the OP is seeing. If not, then
> we can dig deeper into the weird EPERM problems around this particular
> object database.
>
>  sha1_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 77cd81d..721eadc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -454,7 +454,7 @@ static int check_and_freshen_file(const char *fn, int freshen)
>  {
>  	if (access(fn, F_OK))
>  		return 0;
> -	if (freshen && freshen_file(fn))
> +	if (freshen && !freshen_file(fn))
>  		return 0;
>  	return 1;
>  }

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
  2015-07-08 18:05         ` Jeff King
  2015-07-08 18:33           ` [PATCH] check_and_freshen_file: fix reversed success-check Jeff King
@ 2015-07-08 20:28           ` X H
  2015-07-08 20:56           ` Johannes Sixt
  2 siblings, 0 replies; 17+ messages in thread
From: X H @ 2015-07-08 20:28 UTC (permalink / raw)
  To: Jeff King, Johannes Sixt; +Cc: git@vger.kernel.org

Le 8/07/2015 20:05, Jeff King a écrit :
> On Wed, Jul 08, 2015 at 07:41:48PM +0200, Johannes Sixt wrote:
>
>>> Yes, but remember that git stores all of the objects for all of the
>>> commits. So for some reason your push is perhaps trying to send an
>>> object that the other side already has. Usually this does not happen
>>> (the receiver says "I already have these commits, do not bother sending
>>> their objects"), but it's possible that you have an object that is not
>>> referenced by any commit, or a similar situation. It's hard to say
>>> without looking at the repository.
>>
>> After a non-fast-forward push fails, a subsequent forced push sends the same
>> set of objects, which are already present at the server side, but are
>> dangling objects.
>>
>> Apparently, Git for Windows fails to replace the read-only files that live
>> on the network file system.
>
> I left one bit out from my original explanation, which is that
> we generally prefer existing objects to new ones. So we would generally
> want to throw out the new object rather than try to write it out. I'm
> not sure why unpack-objects would try to write an object we already
> have.
>
> We also don't write objects directly, of course; we write to a temporary
> file and try to link them into place. It really sounds more like the
> "objects/d9" directory is where the permission problems are. But, hmm...
>
> The code path should be unpack-objects.c:write_object, which calls
> sha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These
> days it uses the freshen_* functions instead of the latter, which does a
> similar check.  But it does report failure if we cannot call utime() on
> the file, preferring to write it out instead (this is the safer choice
> from a preventing-prune-corruption perspective).
>
> So it's possible that the sequence is:
>
>    - unpack-objects tries to write object 1234abcd...
>
>    - write_sha1_file calls freshen_loose_object
>
>    - we call access("12/34abcd...", F_OK) and see that it does indeed
>      exist
>
>    - we call utime("12/34abcd...") which fails (presumably due to EPERM);
>      we return failure and assume we must write out the object
>
>    - write_sha1_file then writes to a temporary file, and tries to link
>      it into place. Now what? If we get EEXIST, we say "OK, somebody else
>      beat us here", and we consider that a success. But presumably we get
>      some other error here (which may even be a Windows-ism), fall back
>      to rename(), and that fails with EPERM, which we then report.
>
> If that's the case, then one solution is to have the
> timestamp-freshening code silently report success, and skip writing out
> the object. I'm not entirely comfortable with that, just because it is
> loosening a safety mechanism. But perhaps we could loosen it _only_ in
> the case of checking the loose object, and when we get EPERM. We know
> that the next step is going to be writing out that same loose object,
> which is almost certainly going to fail for the same reason.
>
> I dunno. The whole concept of trying to write to an object database for
> which you do not have permissions seems a little bit weird. This would
> definitely be a workaround. But I suspect it did work prior to v2.2.0.
>
> -Peff
>

I've made some tests:
1) Linux Mint:
- ntfs share mounted (with noacl) since
$ chacl -l /the/mountpoint gives
chacl: cannot get access ACL on '/the/mountpoint': Operation not supported
- Create a remote repository on the ntfs share
- Push some test files
- ls -al for files in the objects folder gives:
-rw-r--r--
- with subsequent amend, push the permissions stays the same.

2) On Windows:
- ntfs share mapped as drive by windows
- remote repository created by git on windows
- Push some test files
- ls -al for files in the objects folder gives:
-r--r--r--
- chmod all files to add w permission
- then amend commit and push, push is successful but all files rewritten 
by the push are again:
-r--r--r--
but all folders under objects are:
drwxr-xr-x

So I suppose it some issue with Windows or MSYS2. I'm not sure it's a 
problem related to your explanations, I will read it whit fresh mind.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] check_and_freshen_file: fix reversed success-check
  2015-07-08 19:24             ` Junio C Hamano
@ 2015-07-08 20:33               ` Jeff King
  0 siblings, 0 replies; 17+ messages in thread
From: Jeff King @ 2015-07-08 20:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, René Scharfe, X H, git@vger.kernel.org

On Wed, Jul 08, 2015 at 12:24:41PM -0700, Junio C Hamano wrote:

> > If our utime() call fails, we treat this the same as not
> > having the object in the first place; the safe thing to do
> > is write out another copy. However, the loose-object check
> > accidentally inverst the utime() check; it returns failure
> 
> s/inverst/invert/?

Whoops, should be "inverts".

> I think each of the functions in the check_and_freshen_* callchain
> can at least have a comment in front of it, saying what the returned
> value means, to unconfuse readers.  "Return 1 when the thing exists
> and no further action is necessary; return 0 when the thing does not
> exist or not in a good state and should be overwritten (if the
> caller has something to overwrite it with, that is)" or something?
> 
> Their returning "1" instead of "-1" could be taken as a hint that
> says "this non-zero return does not signal a _failure_", but it is a
> rather weak hint.

Fair enough. Here's a re-roll with some extra comments, and the typo-fix
from above.

-- >8 --
Subject: check_and_freshen_file: fix reversed success-check

When we want to write out a loose object file, we have
always first made sure we don't already have the object
somewhere. Since 33d4221 (write_sha1_file: freshen existing
objects, 2014-10-15), we also update the timestamp on the
file, so that a simultaneous prune knows somebody is
likely to reference it soon.

If our utime() call fails, we treat this the same as not
having the object in the first place; the safe thing to do
is write out another copy. However, the loose-object check
accidentally inverts the utime() check; it returns failure
_only_ when the utime() call actually succeeded. Thus it was
failing to protect us there, and in the normal case where
utime() succeeds, it caused us to pointlessly write out and
link the object.

This passed our freshening tests, because writing out the
new object is certainly _one_ way of updating its utime. So
the normal case was inefficient, but not wrong.

While we're here, let's also drop a comment in front of the
check_and_freshen functions, making a note of their return
type (since it is not our usual "0 for success, -1 for
error").

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_file.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 77cd81d..e435289 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -443,6 +443,7 @@ void prepare_alt_odb(void)
 	read_info_alternates(get_object_directory(), 0);
 }
 
+/* Returns 1 if we have successfully freshened the file, 0 otherwise. */
 static int freshen_file(const char *fn)
 {
 	struct utimbuf t;
@@ -450,11 +451,18 @@ static int freshen_file(const char *fn)
 	return !utime(fn, &t);
 }
 
+/*
+ * All of the check_and_freshen functions return 1 if the file exists and was
+ * freshened (if freshening was requested), 0 otherwise. If they return
+ * 0, you should not assume that it is safe to skip a write of the object (it
+ * either does not exist on disk, or has a stale mtime and may be subject to
+ * pruning).
+ */
 static int check_and_freshen_file(const char *fn, int freshen)
 {
 	if (access(fn, F_OK))
 		return 0;
-	if (freshen && freshen_file(fn))
+	if (freshen && !freshen_file(fn))
 		return 0;
 	return 1;
 }
-- 
2.5.0.rc1.340.ge59e3eb

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: Git force push fails after a rejected push (unpack failed)?
  2015-07-08 18:05         ` Jeff King
  2015-07-08 18:33           ` [PATCH] check_and_freshen_file: fix reversed success-check Jeff King
  2015-07-08 20:28           ` Git force push fails after a rejected push (unpack failed)? X H
@ 2015-07-08 20:56           ` Johannes Sixt
  2 siblings, 0 replies; 17+ messages in thread
From: Johannes Sixt @ 2015-07-08 20:56 UTC (permalink / raw)
  To: Jeff King; +Cc: X H, git@vger.kernel.org

Am 08.07.2015 um 20:05 schrieb Jeff King:
> We also don't write objects directly, of course; we write to a temporary
> file and try to link them into place. It really sounds more like the
> "objects/d9" directory is where the permission problems are. But, hmm...

Not on Windows: A read-only file cannot be overwritten or removed, 
regardless of the permissions of the directory. We do treat this case 
mingw_rename, but I have a slight suspicion that this does not work 
sufficiently reliably on networked file systems.

-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-08 18:33           ` [PATCH] check_and_freshen_file: fix reversed success-check Jeff King
  2015-07-08 19:24             ` Junio C Hamano
@ 2015-07-08 21:03             ` Johannes Sixt
  2015-07-09 20:51               ` Johannes Sixt
  1 sibling, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2015-07-08 21:03 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, René Scharfe, X H, git@vger.kernel.org

Am 08.07.2015 um 20:33 schrieb Jeff King:
> ...or maybe in the utime() step there is actually a bug, and we report
> failure for no good reason. Ugh.

Ah! That code is less than a year old. When I began to adopt a workflow 
requiring force-pushes lately, I wondered why I haven't seen these 
failures earlier, because I did do force pushes in the past, but not 
that frequently. I thought that I had just been lucky. But this would 
explain it.

-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-08 21:03             ` [PATCH] " Johannes Sixt
@ 2015-07-09 20:51               ` Johannes Sixt
  2015-07-09 22:48                 ` Jeff King
  0 siblings, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2015-07-09 20:51 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, René Scharfe, X H, git@vger.kernel.org

Am 08.07.2015 um 23:03 schrieb Johannes Sixt:
> Am 08.07.2015 um 20:33 schrieb Jeff King:
>> ...or maybe in the utime() step there is actually a bug, and we report
>> failure for no good reason. Ugh.
>
> Ah! That code is less than a year old. When I began to adopt a workflow
> requiring force-pushes lately, I wondered why I haven't seen these
> failures earlier, because I did do force pushes in the past, but not
> that frequently. I thought that I had just been lucky. But this would
> explain it.

And, in fact, with this patch these particular failures are gone! Thank 
you so much!

-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-09 20:51               ` Johannes Sixt
@ 2015-07-09 22:48                 ` Jeff King
  2015-07-11 22:21                   ` X H
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2015-07-09 22:48 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, René Scharfe, X H, git@vger.kernel.org

On Thu, Jul 09, 2015 at 10:51:50PM +0200, Johannes Sixt wrote:

> >Ah! That code is less than a year old. When I began to adopt a workflow
> >requiring force-pushes lately, I wondered why I haven't seen these
> >failures earlier, because I did do force pushes in the past, but not
> >that frequently. I thought that I had just been lucky. But this would
> >explain it.
> 
> And, in fact, with this patch these particular failures are gone! Thank you
> so much!

Great, thanks for testing. You can temper your appreciation by noticing
that I introduced the bug in the first place. ;)

-Peff

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-09 22:48                 ` Jeff King
@ 2015-07-11 22:21                   ` X H
  2015-07-13  3:52                     ` Jeff King
  0 siblings, 1 reply; 17+ messages in thread
From: X H @ 2015-07-11 22:21 UTC (permalink / raw)
  To: Jeff King, Johannes Sixt
  Cc: Junio C Hamano, René Scharfe, git@vger.kernel.org

Le 10/07/2015 0:48, Jeff King a écrit :
> On Thu, Jul 09, 2015 at 10:51:50PM +0200, Johannes Sixt wrote:
>
>>> Ah! That code is less than a year old. When I began to adopt a workflow
>>> requiring force-pushes lately, I wondered why I haven't seen these
>>> failures earlier, because I did do force pushes in the past, but not
>>> that frequently. I thought that I had just been lucky. But this would
>>> explain it.
>>
>> And, in fact, with this patch these particular failures are gone! Thank you
>> so much!
>
> Great, thanks for testing. You can temper your appreciation by noticing
> that I introduced the bug in the first place. ;)
>
> -Peff
>
Hi,

Thank you for the patch. I hope it will solve the problem and permit to 
have a second user using the same repository.

How are the permission handled, is it git that is asking to create a 
file read only or rw on the remote or is it the environment with umask 
ans so on that decides it, or Windows when the drive is mounted with noacl?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-11 22:21                   ` X H
@ 2015-07-13  3:52                     ` Jeff King
  2015-07-13 19:58                       ` X H
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2015-07-13  3:52 UTC (permalink / raw)
  To: X H; +Cc: Johannes Sixt, Junio C Hamano, René Scharfe,
	git@vger.kernel.org

On Sun, Jul 12, 2015 at 12:21:33AM +0200, X H wrote:

> How are the permission handled, is it git that is asking to create a file
> read only or rw on the remote or is it the environment with umask ans so on
> that decides it, or Windows when the drive is mounted with noacl?

Generally, git follows the umask when creating most files. However, for
the object files in the object database, it does drop the "w" bit, as
once written, they should never be changed (after all, the filename is a
hash of the contents). We don't ever open those files for writing, but
we may try to rename another file over them; that might behave
differently on Unix versus Windows (or even differently on Windows
between local and remote-mounted filesystems).

-Peff

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] check_and_freshen_file: fix reversed success-check
  2015-07-13  3:52                     ` Jeff King
@ 2015-07-13 19:58                       ` X H
  0 siblings, 0 replies; 17+ messages in thread
From: X H @ 2015-07-13 19:58 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Sixt, Junio C Hamano, René Scharfe,
	git@vger.kernel.org

Le 13/07/2015 5:52, Jeff King a écrit :
> On Sun, Jul 12, 2015 at 12:21:33AM +0200, X H wrote:
>
>> How are the permission handled, is it git that is asking to create a file
>> read only or rw on the remote or is it the environment with umask ans so on
>> that decides it, or Windows when the drive is mounted with noacl?
>
> Generally, git follows the umask when creating most files. However, for
> the object files in the object database, it does drop the "w" bit, as
> once written, they should never be changed (after all, the filename is a
> hash of the contents). We don't ever open those files for writing, but
> we may try to rename another file over them; that might behave
> differently on Unix versus Windows (or even differently on Windows
> between local and remote-mounted filesystems).
>
> -Peff
>

Hi,

When mounted on Linux the object files were created rw on the share, but 
when mounted on Windows the temporary files are created with read-only 
attributes.

Thank you for your patch, forced push are now working from 
git-on-windows to smb folders shared from another Windows machine. I've 
not yet tested to push to the share with another user, but I think it 
should makes no diff.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-07-13 19:59 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-07 13:45 Git force push fails after a rejected push (unpack failed)? X H
2015-07-07 14:13 ` Jeff King
     [not found]   ` <DUB120-W36B78FEE6DC80BDCB05D7FF6920@phx.gbl>
2015-07-07 19:49     ` Jeff King
2015-07-07 23:05       ` Eric Sunshine
2015-07-08 17:41       ` Johannes Sixt
2015-07-08 18:05         ` Jeff King
2015-07-08 18:33           ` [PATCH] check_and_freshen_file: fix reversed success-check Jeff King
2015-07-08 19:24             ` Junio C Hamano
2015-07-08 20:33               ` [PATCH v2] " Jeff King
2015-07-08 21:03             ` [PATCH] " Johannes Sixt
2015-07-09 20:51               ` Johannes Sixt
2015-07-09 22:48                 ` Jeff King
2015-07-11 22:21                   ` X H
2015-07-13  3:52                     ` Jeff King
2015-07-13 19:58                       ` X H
2015-07-08 20:28           ` Git force push fails after a rejected push (unpack failed)? X H
2015-07-08 20:56           ` Johannes Sixt

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).