Re: Add "git-update-ref" to update the HEAD (or other) ref

[Linus Torvalds <torvalds@osdl.org>]

On Sun, 25 Sep 2005, Junio C Hamano wrote:
> 
> Is something like the one at the end acceptable?

Looking at the patch closer, no, that's incorrect.

"oldsha" doesn't necessarily exist, since there has to be some way to 
force the new one. So if "oldval" is NULL, we shouldn't re-verify 
anything.

Also, independently of that your patch is buggy because calling
"resolve_ref()" again will overwrite the lockpath, since it's re-used by
the static buffer in git_path(). That's why the "strdup()" is there.

Yeah, yeah, static buffers are evil, but they are also simple and 
efficient. 

But something like this (on top of my original one) might work.

		Linus
----
diff --git a/update-ref.c b/update-ref.c
--- a/update-ref.c
+++ b/update-ref.c
@@ -63,6 +63,19 @@ const char *resolve_ref(const char *path
 	return path;
 }
 
+static int re_verify(const char *path, unsigned char *oldsha1, unsigned char *currsha1)
+{
+	char buf[40];
+	int fd = open(path, O_RDONLY), nr;
+	if (fd < 0)
+		return -1;
+	nr = read(fd, buf, 40);
+	close(fd);
+	if (nr != 40 || get_sha1_hex(buf, currsha1) < 0)
+		return -1;
+	return memcmp(oldsha1, currsha1, 20) ? -1 : 0;
+}
+
 int main(int argc, char **argv)
 {
 	char *hex;
@@ -108,14 +121,18 @@ int main(int argc, char **argv)
 		unlink(lockpath);
 		die("Unable to write to %s", lockpath);
 	}
-		
+
 	/*
-	 * FIXME!
-	 *
-	 * We should re-read the old ref here, and re-verify that it
-	 * matches "oldsha1". Otherwise there's a small race.
+	 * Re-read the ref after getting the lock to verify
 	 */
+	if (oldval && re_verify(path, oldsha1, currsha1) < 0) {
+		unlink(lockpath);
+		die("Ref lock failed");
+	}
 
+	/*
+	 * Finally, replace the old ref with the new one
+	 */
 	if (rename(lockpath, path) < 0) {
 		unlink(lockpath);
 		die("Unable to create %s", path);