Problem with git-push

Problem with git-push

[Florent Thoumie]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

Hi list,

I'm having some weird issues with git-push lately (1.4.1 on both ends):

: flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push
updating 'refs/heads/xorg'
  from 6d1e4fce0a22da799175ec8e168dc0767f1131ef
  to   496cbd4c75220c204aac4fd1ef9912e40e0314b2
Generating pack...
Done counting 16 objects.
Result has 11 objects.
Deltifying 11 objects.
 100% (11/11) done
Total 11, written 11 (delta 4), reused 0 (delta 0)
Unpacking 11 objects
unpack unpacker exited with error code
ng refs/heads/xorg n/a (unpacker error)
unable to write sha1 filename ./objects/2d: Is a directory
fatal: failed to write object
: flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push
updating 'refs/heads/xorg'
  from 6d1e4fce0a22da799175ec8e168dc0767f1131ef
  to   496cbd4c75220c204aac4fd1ef9912e40e0314b2
Generating pack...
Done counting 16 objects.
Result has 11 objects.
Deltifying 11 objects.
 100% (11/11) done
Total 11, written 11 (delta 4), reused 0 (delta 0)
Unpacking 11 objects
unpack unpacker exited with error code
ng refs/heads/xorg n/a (unpacker error)
unable to write sha1 filename ./objects/d7: Is a directory
fatal: failed to write object
: flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push
updating 'refs/heads/xorg'
  from 6d1e4fce0a22da799175ec8e168dc0767f1131ef
  to   496cbd4c75220c204aac4fd1ef9912e40e0314b2
Generating pack...
Done counting 16 objects.
Result has 11 objects.
Deltifying 11 objects.
 100% (11/11) done
Total 11, written 11 (delta 4), reused 0 (delta 0)
Unpacking 11 objects
refs/heads/xorg: 6d1e4fce0a22da799175ec8e168dc0767f1131ef ->
496cbd4c75220c204aac4fd1ef9912e40e0314b2

Any idea what that could be?

Cheers.

Florent

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: Problem with git-push

[Junio C Hamano]

Florent Thoumie <flz@xbsd.org> writes:

> I'm having some weird issues with git-push lately (1.4.1 on both ends):

Bear with some basic questions.

 - "lately" means this used to work with earlier git on the same
   machine with same configurations?

 - the three runs seem to be pushing the same ref to the same
   repository.  Did you want to illustrate that it fails upon
   writing out different objects and eventually succeeds?

 - what operating system and what filesystem?

 - are you working with a shared repository?

> : flz@cream:/tinderbox/portstrees/xorg-modular/ports; git push
> updating 'refs/heads/xorg'
>   from 6d1e4fce0a22da799175ec8e168dc0767f1131ef
>   to   496cbd4c75220c204aac4fd1ef9912e40e0314b2
> Generating pack...
> Done counting 16 objects.
> Result has 11 objects.
> Deltifying 11 objects.
>  100% (11/11) done
> Total 11, written 11 (delta 4), reused 0 (delta 0)
> Unpacking 11 objects
> unpack unpacker exited with error code
> ng refs/heads/xorg n/a (unpacker error)
> unable to write sha1 filename ./objects/2d: Is a directory
> fatal: failed to write object

This message comes from sha1_file.c::move_temp_to_file().  The
unpacker wrote the object whose name begins with 2d in a
temporary file .git/obj_XXXXXX by calling write_sha1_file(),
which in turn calls move_temp_to_file() to move it to its final
destination ".git/objects/2d/XXXXXX".

move_temp_to_file() first tries to make a hardlink in
.git/objects/2d/ directory, assuming that most of the time you
already have it.  If it succeeds, then it runs unlink() on the
temporary file ".git/obj_XXXXXX".  But taht is not what is
happening.

link_temp_to_file() function first tries to make a hardlink with
link(".git/obj_XXXXXX", ".git/objects/2d/XXXXXX").  This call
must be failing for you.  Then it tries to create the leading
directory ".git/objects/2d", and does adjust_shared_perm and
when it fails it returns (but forgets to restore the slash it
removed earlier -- a patch to fix this is attached at the end).

This explains why the final filename is truncated at the last
slash, hence ./objects/2d in the error message.  Also it
explains why repeated tries would succeed; the first run creates
.git/objects/2d/ directory so the second run will try hardlink
and will succeed.

So the next thing to see is why adjust_shared_perm() is failing
for you.

Oops.

I think this function is broken at the concept level.  This is
to work around people's broken umask (when you are working with
other people, your umask should not be stronger than 007) and
tries to flip group writability bit on.

First of all, if the necessary bits are already on, it is not
necessary to run chmod(), but more importantly, if the directory
in question was created by somebody else, I do not think this
chmod() would succeed even if you are in the same group as the
owner (i.e. previous creator) of the directory.

[jc: I am CC'ing Johannes to blame him on commit 457f06d6;
git-pickaxe is turning out to be quite handy ;-)

	git pickaxe -C -f L246,277 v1.4.1 -- path.c

]

A quick workaround until we sort this out would be:

 - make sure all your developers have umask suitable for group
   work (i.e. second octal-digit from the right should be zero
   to give group members the same rights as you do).

 - run "chmod g+w .git/objects/??" in the shared repository.

-- >8 --

diff --git a/sha1_file.c b/sha1_file.c
index e89d24c..5707069 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1400,8 +1400,10 @@ static int link_temp_to_file(const char
 	if (dir) {
 		*dir = 0;
 		mkdir(filename, 0777);
-		if (adjust_shared_perm(filename))
+		if (adjust_shared_perm(filename)) {
+			*dir = '/';
 			return -2;
+		}
 		*dir = '/';
 		if (!link(tmpfile, filename))
 			return 0;

Re: Problem with git-push

[Johannes Schindelin]

Hi,

On Tue, 31 Oct 2006, Junio C Hamano wrote:

> So the next thing to see is why adjust_shared_perm() is failing
> for you.
> 
> Oops.
> 
> I think this function is broken at the concept level.  This is
> to work around people's broken umask (when you are working with
> other people, your umask should not be stronger than 007) and
> tries to flip group writability bit on.
> 
> First of all, if the necessary bits are already on, it is not
> necessary to run chmod(),

right

> but more importantly, if the directory in question was created by 
> somebody else, I do not think this chmod() would succeed even if you are 
> in the same group as the owner (i.e. previous creator) of the directory.

But if somebody else created it, it should already have the correct 
permissions in the first place (unless the user played around with them, 
which is not a pilot error, but a willfull pointing of the barrel in the 
general direction of your knee).

> [jc: I am CC'ing Johannes to blame him on commit 457f06d6;
> git-pickaxe is turning out to be quite handy ;-)

I am hating the tool already.

>  	if (dir) {
>  		*dir = 0;
>  		mkdir(filename, 0777);
> -		if (adjust_shared_perm(filename))
> +		if (adjust_shared_perm(filename)) {
> +			*dir = '/';
>  			return -2;
> +		}

How about this instead:

-		mkdir(filename, 0777);
-		if (adjust_shared_perm(filename))
+		if (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {
+			*dir = '/';
 			return -2;
+		}

Ciao,
Dscho

Re: Problem with git-push

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> but more importantly, if the directory in question was created by 
>> somebody else, I do not think this chmod() would succeed even if you are 
>> in the same group as the owner (i.e. previous creator) of the directory.
>
> But if somebody else created it, it should already have the correct 
> permissions in the first place (unless the user played around with them, 
> which is not a pilot error, but a willfull pointing of the barrel in the 
> general direction of your knee).

True; I think the yesterday's analysis is still incomplete.  I
haven't reached the point where I can explain "is a directory".
If the directory was there and mkdir() failed (but we do not
check its return value), it would have set errno to EEXIST not
to EISDIR.  There is something else going on.

>> [jc: I am CC'ing Johannes to blame him on commit 457f06d6;
>> git-pickaxe is turning out to be quite handy ;-)
>
> I am hating the tool already.

Well, we could rename it to "git credit" ;-).

Re: Problem with git-push

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> How about this instead:
>
> -		mkdir(filename, 0777);
> -		if (adjust_shared_perm(filename))
> +		if (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {
> +			*dir = '/';
>  			return -2;
> +		}

Not really.  See the comment above the code you just touched.

Re: Problem with git-push

[Florent Thoumie]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

On Tue, 2006-10-31 at 15:39 -0800, Junio C Hamano wrote:

> A quick workaround until we sort this out would be:
> 
>  - make sure all your developers have umask suitable for group
>    work (i.e. second octal-digit from the right should be zero
>    to give group members the same rights as you do).
> 
>  - run "chmod g+w .git/objects/??" in the shared repository.

I thought I checked on the remote (guess I only looked at my local
repo), but that was it, wrong perms.

Sorry if this is a dumb question but assuming i did 3 successive
git-push's, why is the object name changing from a try to another?

Thanks for the answer.

Florent


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: Problem with git-push

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> but more importantly, if the directory in question was created by 
>>> somebody else, I do not think this chmod() would succeed even if you are 
>>> in the same group as the owner (i.e. previous creator) of the directory.
>>
>> But if somebody else created it, it should already have the correct 
>> permissions in the first place (unless the user played around with them, 
>> which is not a pilot error, but a willfull pointing of the barrel in the 
>> general direction of your knee).
>
> True; I think the yesterday's analysis is still incomplete.  I
> haven't reached the point where I can explain "is a directory".
> If the directory was there and mkdir() failed (but we do not
> check its return value), it would have set errno to EEXIST not
> to EISDIR.  There is something else going on.

Actually, Florent's said the directory permission was screwed up
to begin with, so after following the code a bit more I can see
why it said "is a directory".

mkdir() may have failed because somebody else had a directory
there, perhaps with wrong permission; link_temp_to_file()
correctly fails from adjust_shared_perm() (because the directory
had incorrect permission that it cannot fix), returns -2 with
truncated filename "object/2d", back to move_temp_to_file(), and
-2 is not EEXIST so it tries to rename the temporary file to
that directory, which would also fail and sets ret to errno
(which now is EISDIR).  After unlinking the temporary file, we
notice that ret is EISDIR and reports the failure.

So there is not much more to explain, other than why mkdir()
failed in the first place.  Previous driver error?

I think idempotent chmod() on somebody else's file or directory
would fail, so this may be a safer addition to the codepath in
question.

diff --git a/path.c b/path.c
index bb89fb0..b82f194 100644
--- a/path.c
+++ b/path.c
@@ -279,7 +279,7 @@ int adjust_shared_perm(const char *path)
 			    : 0));
 	if (S_ISDIR(mode))
 		mode |= S_ISGID;
-	if (chmod(path, mode) < 0)
+	if (mode != st.st_mode && chmod(path, mode) < 0)
 		return -2;
 	return 0;
 }

Re: Problem with git-push

[Junio C Hamano]

Florent Thoumie <flz@xbsd.org> writes:

> On Wed, 2006-11-01 at 13:43 -0800, Junio C Hamano wrote:
>> Junio C Hamano <junkio@cox.net> writes:
>> 
>> Actually, Florent's said the directory permission was screwed up
>> to begin with, so after following the code a bit more I can see
>> why it said "is a directory".
>
> I screwed the perms. I have a cron job that does automatic imports and
> it's running as root. I've been lured by the fact that it uses my name
> and email address to do those imports, so I thought it was running under
> my unprivileged account.

Thanks for a honest clarification.  I was starting to worry if
this is a BSD vs Linux filesystem gotcha that is harder to debug
without having an actual box.

Re: Problem with git-push

[Florent Thoumie]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

On Wed, 2006-11-01 at 13:43 -0800, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >>> but more importantly, if the directory in question was created by 
> >>> somebody else, I do not think this chmod() would succeed even if you are 
> >>> in the same group as the owner (i.e. previous creator) of the directory.
> >>
> >> But if somebody else created it, it should already have the correct 
> >> permissions in the first place (unless the user played around with them, 
> >> which is not a pilot error, but a willfull pointing of the barrel in the 
> >> general direction of your knee).
> >
> > True; I think the yesterday's analysis is still incomplete.  I
> > haven't reached the point where I can explain "is a directory".
> > If the directory was there and mkdir() failed (but we do not
> > check its return value), it would have set errno to EEXIST not
> > to EISDIR.  There is something else going on.
> 
> Actually, Florent's said the directory permission was screwed up
> to begin with, so after following the code a bit more I can see
> why it said "is a directory".

I screwed the perms. I have a cron job that does automatic imports and
it's running as root. I've been lured by the fact that it uses my name
and email address to do those imports, so I thought it was running under
my unprivileged account.

Florent

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: Problem with git-push

[Johannes Schindelin]

Hi,

On Wed, 1 Nov 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > How about this instead:
> >
> > -		mkdir(filename, 0777);
> > -		if (adjust_shared_perm(filename))
> > +		if (!mkdir(filename, 0777) && adjust_shared_perm(filename)) {
> > +			*dir = '/';
> >  			return -2;
> > +		}
> 
> Not really.  See the comment above the code you just touched.

 /*
  * Try to mkdir the last path component if that failed.
  *
  * Re-try the "link()" regardless of whether the mkdir
  * succeeds, since a race might mean that somebody
  * else succeeded.
  */

What I proposed does not break that. It only means that you only try to 
adjust the permissions if the mkdir succeeded. If somebody else created 
the directory in the mean time, it is not our job to adjust the 
permissions. Worse, if the other mkdir was run by a different user, the 
permission adjusting _cannot_ succeed, like you pointed out.

So I think that my patch is correct, but does not matter very much.

The other thing you asked for: adjust_shared_permissions should only try 
to adjust the permissions only if they are not already correct. Here is my 
attempt to implement that:

Totally untested.

Ciao,
Dscho

-- snip --
adjust_shared_perm(): Avoid unnecessary chmod()s

Sighed-off-by: Johannes E. Schindelin <johannes.schindelin@gmx.de>
---
diff --git a/path.c b/path.c
index bb89fb0..0a2bc51 100644
--- a/path.c
+++ b/path.c
@@ -279,7 +279,7 @@ int adjust_shared_perm(const char *path)
 			    : 0));
 	if (S_ISDIR(mode))
 		mode |= S_ISGID;
-	if (chmod(path, mode) < 0)
+	if ((mode & st.st_mode != mode) && chmod(path, mode) < 0)
 		return -2;
 	return 0;
 }

Re: Problem with git-push

[Johannes Schindelin]

Hi,

On Wed, 1 Nov 2006, Junio C Hamano wrote:

> -	if (chmod(path, mode) < 0)
> +	if (mode != st.st_mode && chmod(path, mode) < 0)

Oops. Somehow I missed this mail before replying. I'd like to point out 
that it's safer to check for (mode & st.st_mode != mode), since we should 
leave broader permissions as are.

Ciao,
Dscho

Re: Problem with git-push

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> What I proposed does not break that. It only means that you only try to 
> adjust the permissions if the mkdir succeeded. If somebody else created 
> the directory in the mean time, it is not our job to adjust the 
> permissions.
> So I think that my patch is correct, but does not matter very much.

Ok.  I misread the patch.  Will apply with a one-liner log.

Thanks.