Re: [PATCH] check_and_freshen_file: fix reversed success-check
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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check_and_freshen_file: fix reversed success-check
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check_and_freshen_file: fix reversed success-check
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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check_and_freshen_file: fix reversed success-check
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check_and_freshen_file: fix reversed success-check
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] check_and_freshen_file: fix reversed success-check
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 p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check_and_freshen_file: fix reversed success-check
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check_and_freshen_file: fix reversed success-check
Jeff King p...@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 p...@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;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
