Linus Torvalds [off-list ref] writes:
+ * FIXME!
Is something like the one at the end acceptable?
I'd like to take these patches in two stages (I am not asking
you for a resend):
- Drop the emulated symlink part from the update-ref.c; have it
graduate to "master" branch and use it in existing scripts.
- Take the read_ref() change, along with a patch to re-add the
emulated symlink part to update-ref.c (after making its
interpretation to match that of read_ref() -- which requires
the prefix to be exactly "ref: " five bytes); keep it in "pu"
branch a bit longer.
---
diff --git a/update-ref.c b/update-ref.c
--- a/update-ref.c
+++ b/update-ref.c
@@ -97,11 +97,13 @@ int main(int argc, char **argv)
}
/*
- * FIXME!
- *
- * We should re-read the old ref here, and re-verify that it
+ * We re-read the old ref here, and re-verify that it
* matches "oldsha1". Otherwise there's a small race.
*/
+ if (!resolve_ref(git_path("%s", refname), oldsha1))
+ die("Cannot verify ref: %s", refname);
+ if (memcmp(oldsha1, currsha1, 20))
+ die("Ref %s changed to %s", refname, sha1_to_hex(oldsha1));
if (rename(lockpath, path) < 0) {
unlink(lockpath);
On Sun, 25 Sep 2005, Junio C Hamano wrote:
I'd like to take these patches in two stages (I am not asking
you for a resend):
- Drop the emulated symlink part from the update-ref.c; have it
graduate to "master" branch and use it in existing scripts.
Sure.
- Take the read_ref() change, along with a patch to re-add the
emulated symlink part to update-ref.c (after making its
interpretation to match that of read_ref() -- which requires
the prefix to be exactly "ref: " five bytes); keep it in "pu"
branch a bit longer.
I was actually thinking of maybe entirely replacing "read_ref()" with the
more powerful "resolve_ref()" - moving resolve_ref() into refs.c.
That way there's only one place that knows about the "ref:" thing.
But yes, forcing the format to be "ref: " instead of "ref:<whitespace>*"
sounds fine.
Linus
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);