[RFC][PATCH] Rewriting revs in place in push target repository

[RFC][PATCH] Rewriting revs in place in push target repository

[Petr Baudis]

Rewrite refs in place in receive-pack & friends

When updating a ref, it would write a new file with the new ref and
then rename it, overwriting the original file. The problem is that
this destroys permissions and ownership of the original file, which is
troublesome especially in multiuser environment, like the one I live in.

This might be controversial, but it's a showbreaker for me wrt. pushing
now. Some alternative solution barely solving my particular situation
might be surely worked out, but this is more general. The question is:

* Does this break atomicity?

	I think it does not in real setups, since thanks to O_RDWR the
	file should be overwritten only when the write() happens.
	Can a 41-byte write() be non-atomic in any real conditions?

* Does this break with full disk/quota?

	I'm not sure - we are substituting 41 bytes by another 41
	bytes; will the system ever be evil enough to truncate the
	file, then decide the user is over his quota and not write
	the new contents?

Signed-off-by: Petr Baudis <pasky@suse.cz>

diff --git a/receive-pack.c b/receive-pack.c
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -92,13 +92,7 @@ static int run_update_hook(const char *r
 static int update(const char *name,
 		  unsigned char *old_sha1, unsigned char *new_sha1)
 {
-	char new_hex[60], *old_hex, *lock_name;
-	int newfd, namelen, written;
-
-	namelen = strlen(name);
-	lock_name = xmalloc(namelen + 10);
-	memcpy(lock_name, name, namelen);
-	memcpy(lock_name + namelen, ".lock", 6);
+	char new_hex[60], *old_hex;
 
 	strcpy(new_hex, sha1_to_hex(new_sha1));
 	old_hex = sha1_to_hex(old_sha1);
@@ -106,38 +100,38 @@ static int update(const char *name,
 		return error("unpack should have generated %s, "
 			     "but I can't find it!", new_hex);
 
-	safe_create_leading_directories(lock_name);
-
-	newfd = open(lock_name, O_CREAT | O_EXCL | O_WRONLY, 0666);
-	if (newfd < 0)
-		return error("unable to create %s (%s)",
-			     lock_name, strerror(errno));
-
-	/* Write the ref with an ending '\n' */
-	new_hex[40] = '\n';
-	new_hex[41] = 0;
-	written = write(newfd, new_hex, 41);
-	/* Remove the '\n' again */
-	new_hex[40] = 0;
-
-	close(newfd);
-	if (written != 41) {
-		unlink(lock_name);
-		return error("unable to write %s", lock_name);
-	}
 	if (verify_old_ref(name, old_hex) < 0) {
-		unlink(lock_name);
 		return error("%s changed during push", name);
 	}
 	if (run_update_hook(name, old_hex, new_hex)) {
-		unlink(lock_name);
 		return error("hook declined to update %s\n", name);
 	}
-	else if (rename(lock_name, name) < 0) {
-		unlink(lock_name);
-		return error("unable to replace %s", name);
-	}
 	else {
+		char *name2;
+		int newfd, written;
+
+		name2 = strdup(name);
+		safe_create_leading_directories(name2);
+		free(name2);
+
+		newfd = open(name, O_CREAT | O_RDWR, 0666);
+		if (newfd < 0)
+			return error("unable to create %s (%s)",
+				     name, strerror(errno));
+
+		/* Write the ref with an ending '\n' */
+		new_hex[40] = '\n';
+		new_hex[41] = 0;
+		written = write(newfd, new_hex, 41);
+		/* Remove the '\n' again */
+		new_hex[40] = 0;
+
+		close(newfd);
+		if (written != 41) {
+			unlink(name);
+			return error("unable to write %s", name);
+		}
+
 		fprintf(stderr, "%s: %s -> %s\n", name, old_hex, new_hex);
 		return 0;
 	}

Re: [RFC][PATCH] Rewriting revs in place in push target repository

[Linus Torvalds]

On Sat, 13 Aug 2005, Petr Baudis wrote:
> 
> * Does this break atomicity?
> 
> 	I think it does not in real setups, since thanks to O_RDWR the
> 	file should be overwritten only when the write() happens.
> 	Can a 41-byte write() be non-atomic in any real conditions?

That's not the problem.

The problem is that your change means that there is no locking, and you 
now can have two writers that both update the same file, and they _both_ 
think that they succeed. They'll both read the old contents, decide that 
it still is the one from before the push, and then they'll both do the 
write.

And yes, in most (all?) sane filesystems, the end result is that one of 
them "wins", and the end result is a nice 41-byte file. But the problem is 
that the other write just totally got lost, and the person doing the push 
_thought_ he had updated the thing, but never did.

To make things worse, with NFS and client-side caching, different clients 
that look at the tree at around that time can literally see _different_ 
heads winning the race. One of the writers wrote "first", and that client 
(and other NFS clients doing a read at that time) will see it succeed. But 
then the other pusher writes, and now people will see _that_ one succeed.

Confusion reigns.

In contrast, with the "create lock-file and rename" thing, if there is a
race, somebody will win, and the loser will hopefully know they lost.

> * Does this break with full disk/quota?
> 
> 	I'm not sure - we are substituting 41 bytes by another 41
> 	bytes; will the system ever be evil enough to truncate the
> 	file, then decide the user is over his quota and not write
> 	the new contents?

Probably not.

But how about you just try to copy the permission/group of the original
file before you do the rename? I assume that if you're depending on 
permissions, it's either a shared group or by having the thing writable by 
others, so doing a 

	if (!fstat(oldfd, &st)) {
		fchown(fd, (uid_t) -1, st.st_gid);
		fchmod(fd, st.st_mode & ALLPERMS);
	}
	.. do rename here ..

which should get you where you want, no?

		Linus

Re: [RFC][PATCH] Rewriting revs in place in push target repository

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Rewrite refs in place in receive-pack & friends
>
> When updating a ref, it would write a new file with the new ref and
> then rename it, overwriting the original file. The problem is that
> this destroys permissions and ownership of the original file, which is
> troublesome especially in multiuser environment, like the one I live in.

Hmph.  If a repo is _really_ used multiuser then you should not
have to care about ownership.  If you can write into a
repository for a project (implying that you are a member of that
project group), and if your umask is set up correctly (meaning
it is 002 or looser), and with g+s bit on the directory at the
repository root level when it was created, shouldn't your newly
created ref file be also writable by others in that project?

Re: [RFC][PATCH] Rewriting revs in place in push target repository

[Linus Torvalds]

On Sat, 13 Aug 2005, Junio C Hamano wrote:
>
> Petr Baudis <pasky@suse.cz> writes: 
> > Rewrite refs in place in receive-pack & friends
> >
> > When updating a ref, it would write a new file with the new ref and
> > then rename it, overwriting the original file. The problem is that
> > this destroys permissions and ownership of the original file, which is
> > troublesome especially in multiuser environment, like the one I live in.
> 
> Hmph.  If a repo is _really_ used multiuser then you should not
> have to care about ownership.

I think Pasky's usage is that different heads are owned by different
groups and/or users, and he wants to use the filesystem permissions to
determine who gets to update which branch. Which is reasonable in a way.

On the other hand, I don't think filesystem permissions are really very 
useful. I think it's more appropriate to use triggers to say something 
like "only allow people in the 'xyz' group to write to this head".

Obviously, triggers aren't about _security_ - somebody who has write 
permissions to the tree can always screw up others. But triggers are fine 
for things like branch ownership, where you trust your users, but you just 
want to avoid mistakes.

So a trigger might be something like

	#!/bin/sh
	. git-sh-setup-script
	branch="$1"
	old="$2"
	new="$3"
	if [ -e $GIT_DIR/permissions/$branch ]; then
		id=$(id -un)
		grep -q "^$id$" $GIT_DIR/permissions/$branch ||
			die "You're not allowed to write to $branch"
	fi
	true

and that would allow you to list all users that are allowed to write to 
the branch in $GIT_DIR/permissions/<branchname>.

Totally untested, of course. But the concept should work.

		Linus

Re: [RFC][PATCH] Rewriting revs in place in push target repository

[Petr Baudis]

Dear diary, on Sun, Aug 14, 2005 at 02:55:16AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Petr Baudis <pasky@suse.cz> writes:
> 
> > Rewrite refs in place in receive-pack & friends
> >
> > When updating a ref, it would write a new file with the new ref and
> > then rename it, overwriting the original file. The problem is that
> > this destroys permissions and ownership of the original file, which is
> > troublesome especially in multiuser environment, like the one I live in.
> 
> Hmph.  If a repo is _really_ used multiuser then you should not
> have to care about ownership.  If you can write into a
> repository for a project (implying that you are a member of that
> project group), and if your umask is set up correctly (meaning
> it is 002 or looser), and with g+s bit on the directory at the
> repository root level when it was created, shouldn't your newly
> created ref file be also writable by others in that project?

Hmm, but how do you actually set the umask correctly just for git
pushing? I'm sorry but it doesn't occur to me.

I like Linus' solution, but have no time to do the patch now, so I did
just a quick dirty fix and added a chmod to hooks/update. ;-)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
If you want the holes in your knowledge showing up try teaching
someone.  -- Alan Cox

Re: [RFC][PATCH] Rewriting revs in place in push target repository

[Chris Wedgwood]

On Sat, Aug 13, 2005 at 11:47:25PM +0200, Petr Baudis wrote:

> 	I think it does not in real setups, since thanks to O_RDWR the
> 	file should be overwritten only when the write() happens.
> 	Can a 41-byte write() be non-atomic in any real conditions?

yes

if you journal metadata only you can see a file extended w/o having
the block flushed

Re: [RFC][PATCH] Rewriting revs in place in push target repository

[Matthias Urlichs]

Hi, Chris Wedgwood wrote:

> On Sat, Aug 13, 2005 at 11:47:25PM +0200, Petr Baudis wrote:
> 
>> 	I think it does not in real setups, since thanks to O_RDWR the
>> 	file should be overwritten only when the write() happens.
>> 	Can a 41-byte write() be non-atomic in any real conditions?
> 
> if you journal metadata only you can see a file extended w/o having
> the block flushed

??? but the file is *not* extended. Also, whether or not a block is
flushed should only matter if the machine crashes ..?

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
CONS [from LISP] 1. v. To add a new element to a list.	2. CONS UP:
   v. To synthesize from smaller pieces: "to cons up an example".
				-- From the AI Hackers' Dictionary