{"thread":{"id":"4178","subject":"[RFC 2/5] Improve abstraction of ref lock/write.","startedAt":"2006-05-17T09:55:02Z","lastAt":"2006-05-17T10:46:50Z","messageCount":2,"participants":["Shawn Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"20124","messageId":"20060517095502.GC28529@spearce.org","threadId":"4178","inReplyTo":null,"subject":"[RFC 2/5] Improve abstraction of ref lock/write.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-05-17T09:55:02Z","receivedAt":"2006-05-17T09:55:02Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Created 'struct ref_lock' to contain the data necessary to perform\na ref update.  This change improves writing a ref as the file names\nare generated only once (rather than twice) and supports following\nsymrefs (up to the maximum depth).  Further the ref_lock structure\nprovides room to extend the update API with ref logging.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n\n---\n\n fetch.c |   32 +++++----\n refs.c  |  236 ++++++++++++++++++++++++++++++---------------------------------\n refs.h  |   24 ++++--\n 3 files changed, 144 insertions(+), 148 deletions(-)\n\nd38945e0c759f8739da4c24485578a8193d54c59\ndiff --git a/fetch.c b/fetch.c\nindex 73bde07..8bdaacb 100644\n--- a/fetch.c\n+++ b/fetch.c\n@@ -204,14 +204,14 @@ static int mark_complete(const char *pat\n \n int pull(char *target)\n {\n+\tstruct ref_lock *lock;\n \tunsigned char sha1[20];\n-\tint fd = -1;\n \n \tsave_commit_buffer = 0;\n \ttrack_object_refs = 0;\n-\tif (write_ref && current_ref) {\n-\t\tfd = lock_ref_sha1(write_ref, current_ref);\n-\t\tif (fd < 0)\n+\tif (write_ref) {\n+\t\tlock = lock_ref_sha1(write_ref, current_ref, 1);\n+\t\tif (!lock)\n \t\t\treturn -1;\n \t}\n \n@@ -219,20 +219,22 @@ int pull(char *target)\n \t\tfor_each_ref(mark_complete);\n \t}\n \n-\tif (interpret_target(target, sha1))\n-\t\treturn error(\"Could not interpret %s as something to pull\",\n-\t\t\t     target);\n-\tif (process(lookup_unknown_object(sha1)))\n+\tif (interpret_target(target, sha1)) {\n+\t\terror(\"Could not interpret %s as something to pull\", target);\n+\t\tunlock_ref(lock);\n+\t\treturn -1;\n+\t}\n+\tif (process(lookup_unknown_object(sha1))) {\n+\t\tunlock_ref(lock);\n \t\treturn -1;\n-\tif (loop())\n+\t}\n+\tif (loop()) {\n+\t\tunlock_ref(lock);\n \t\treturn -1;\n-\t\n+\t}\n+\n \tif (write_ref) {\n-\t\tif (current_ref) {\n-\t\t\twrite_ref_sha1(write_ref, fd, sha1);\n-\t\t} else {\n-\t\t\twrite_ref_sha1_unlocked(write_ref, sha1);\n-\t\t}\n+\t\treturn write_ref_sha1(lock, sha1, \"git fetch\");\n \t}\n \treturn 0;\n }\ndiff --git a/refs.c b/refs.c\nindex 0f3491f..91c8c44 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -198,26 +198,6 @@ int for_each_remote_ref(int (*fn)(const \n \treturn do_for_each_ref(\"refs/remotes\", fn, 13);\n }\n \n-static char *ref_file_name(const char *ref)\n-{\n-\tchar *base = get_refs_directory();\n-\tint baselen = strlen(base);\n-\tint reflen = strlen(ref);\n-\tchar *ret = xmalloc(baselen + 2 + reflen);\n-\tsprintf(ret, \"%s/%s\", base, ref);\n-\treturn ret;\n-}\n-\n-static char *ref_lock_file_name(const char *ref)\n-{\n-\tchar *base = get_refs_directory();\n-\tint baselen = strlen(base);\n-\tint reflen = strlen(ref);\n-\tchar *ret = xmalloc(baselen + 7 + reflen);\n-\tsprintf(ret, \"%s/%s.lock\", base, ref);\n-\treturn ret;\n-}\n-\n int get_ref_sha1(const char *ref, unsigned char *sha1)\n {\n \tif (check_ref_format(ref))\n@@ -225,94 +205,6 @@ int get_ref_sha1(const char *ref, unsign\n \treturn read_ref(git_path(\"refs/%s\", ref), sha1);\n }\n \n-static int lock_ref_file(const char *filename, const char *lock_filename,\n-\t\t\t const unsigned char *old_sha1)\n-{\n-\tint fd = open(lock_filename, O_WRONLY | O_CREAT | O_EXCL, 0666);\n-\tunsigned char current_sha1[20];\n-\tint retval;\n-\tif (fd < 0) {\n-\t\treturn error(\"Couldn't open lock file for %s: %s\",\n-\t\t\t     filename, strerror(errno));\n-\t}\n-\tretval = read_ref(filename, current_sha1);\n-\tif (old_sha1) {\n-\t\tif (retval) {\n-\t\t\tclose(fd);\n-\t\t\tunlink(lock_filename);\n-\t\t\treturn error(\"Could not read the current value of %s\",\n-\t\t\t\t     filename);\n-\t\t}\n-\t\tif (memcmp(current_sha1, old_sha1, 20)) {\n-\t\t\tclose(fd);\n-\t\t\tunlink(lock_filename);\n-\t\t\terror(\"The current value of %s is %s\",\n-\t\t\t      filename, sha1_to_hex(current_sha1));\n-\t\t\treturn error(\"Expected %s\",\n-\t\t\t\t     sha1_to_hex(old_sha1));\n-\t\t}\n-\t} else {\n-\t\tif (!retval) {\n-\t\t\tclose(fd);\n-\t\t\tunlink(lock_filename);\n-\t\t\treturn error(\"Unexpectedly found a value of %s for %s\",\n-\t\t\t\t     sha1_to_hex(current_sha1), filename);\n-\t\t}\n-\t}\n-\treturn fd;\n-}\n-\n-int lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n-{\n-\tchar *filename;\n-\tchar *lock_filename;\n-\tint retval;\n-\tif (check_ref_format(ref))\n-\t\treturn -1;\n-\tfilename = ref_file_name(ref);\n-\tlock_filename = ref_lock_file_name(ref);\n-\tretval = lock_ref_file(filename, lock_filename, old_sha1);\n-\tfree(filename);\n-\tfree(lock_filename);\n-\treturn retval;\n-}\n-\n-static int write_ref_file(const char *filename,\n-\t\t\t  const char *lock_filename, int fd,\n-\t\t\t  const unsigned char *sha1)\n-{\n-\tchar *hex = sha1_to_hex(sha1);\n-\tchar term = '\\n';\n-\tif (write(fd, hex, 40) < 40 ||\n-\t    write(fd, &term, 1) < 1) {\n-\t\terror(\"Couldn't write %s\", filename);\n-\t\tclose(fd);\n-\t\treturn -1;\n-\t}\n-\tclose(fd);\n-\trename(lock_filename, filename);\n-\treturn 0;\n-}\n-\n-int write_ref_sha1(const char *ref, int fd, const unsigned char *sha1)\n-{\n-\tchar *filename;\n-\tchar *lock_filename;\n-\tint retval;\n-\tif (fd < 0)\n-\t\treturn -1;\n-\tif (check_ref_format(ref))\n-\t\treturn -1;\n-\tfilename = ref_file_name(ref);\n-\tlock_filename = ref_lock_file_name(ref);\n-\tif (safe_create_leading_directories(filename))\n-\t\tdie(\"unable to create leading directory for %s\", filename);\n-\tretval = write_ref_file(filename, lock_filename, fd, sha1);\n-\tfree(filename);\n-\tfree(lock_filename);\n-\treturn retval;\n-}\n-\n /*\n  * Make sure \"ref\" is something reasonable to have under \".git/refs/\";\n  * We do not like it if:\n@@ -365,25 +257,119 @@ int check_ref_format(const char *ref)\n \t}\n }\n \n-int write_ref_sha1_unlocked(const char *ref, const unsigned char *sha1)\n+static struct ref_lock* verify_lock(struct ref_lock *lock,\n+\tconst unsigned char *old_sha1, int mustexist)\n+{\n+\tchar buf[40];\n+\tint nr, fd = open(lock->ref_file, O_RDONLY);\n+\tif (fd < 0 && (mustexist || errno != ENOENT)) {\n+\t\terror(\"Can't verify ref %s\", lock->ref_file);\n+\t\tunlock_ref(lock);\n+\t\treturn NULL;\n+\t}\n+\tnr = read(fd, buf, 40);\n+\tclose(fd);\n+\tif (nr != 40 || get_sha1_hex(buf, lock->old_sha1) < 0) {\n+\t\terror(\"Can't verify ref %s\", lock->ref_file);\n+\t\tunlock_ref(lock);\n+\t\treturn NULL;\n+\t}\n+\tif (memcmp(lock->old_sha1, old_sha1, 20)) {\n+\t\terror(\"Ref %s is at %s but expected %s\", lock->ref_file,\n+\t\t\tsha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));\n+\t\tunlock_ref(lock);\n+\t\treturn NULL;\n+\t}\n+\treturn lock;\n+}\n+\n+static struct ref_lock* lock_ref_sha1_basic(const char *path,\n+\tint plen,\n+\tconst unsigned char *old_sha1, int mustexist)\n+{\n+\tstruct ref_lock *lock;\n+\n+\tlock = xcalloc(1, sizeof(struct ref_lock));\n+\tlock->lock_fd = -1;\n+\n+\tplen = strlen(path) - plen;\n+\tpath = resolve_ref(path, lock->old_sha1, mustexist);\n+\tif (!path) {\n+\t\terror(\"Can't read ref %s\", path);\n+\t\tunlock_ref(lock);\n+\t\treturn NULL;\n+\t}\n+\n+\tlock->ref_file = strdup(path);\n+\tlock->lock_file = strdup(mkpath(\"%s.lock\", lock->ref_file));\n+\n+\tif (safe_create_leading_directories(lock->lock_file))\n+\t\tdie(\"unable to create directory for %s\", lock->lock_file);\n+\tlock->lock_fd = open(lock->lock_file,\n+\t\tO_WRONLY | O_CREAT | O_EXCL, 0666);\n+\tif (lock->lock_fd < 0) {\n+\t\terror(\"Couldn't open lock file %s: %s\",\n+\t\t\tlock->lock_file, strerror(errno));\n+\t\tunlock_ref(lock);\n+\t\treturn NULL;\n+\t}\n+\n+\treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n+}\n+\n+struct ref_lock* lock_ref_sha1(const char *ref,\n+\tconst unsigned char *old_sha1, int mustexist)\n {\n-\tchar *filename;\n-\tchar *lock_filename;\n-\tint fd;\n-\tint retval;\n \tif (check_ref_format(ref))\n+\t\treturn NULL;\n+\treturn lock_ref_sha1_basic(git_path(\"refs/%s\", ref),\n+\t\tstrlen(ref), old_sha1, mustexist);\n+}\n+\n+struct ref_lock* lock_any_ref_for_update(const char *ref,\n+\tconst unsigned char *old_sha1, int mustexist)\n+{\n+\treturn lock_ref_sha1_basic(git_path(\"%s\", ref),\n+\t\tstrlen(ref), old_sha1, mustexist);\n+}\n+\n+void unlock_ref (struct ref_lock *lock)\n+{\n+\tif (lock->lock_fd >= 0) {\n+\t\tclose(lock->lock_fd);\n+\t\tunlink(lock->lock_file);\n+\t}\n+\tif (lock->ref_file)\n+\t\tfree(lock->ref_file);\n+\tif (lock->lock_file)\n+\t\tfree(lock->lock_file);\n+\tfree(lock);\n+}\n+\n+int write_ref_sha1(struct ref_lock *lock,\n+\tconst unsigned char *sha1, const char *logmsg)\n+{\n+\tstatic char term = '\\n';\n+\n+\tif (!lock)\n \t\treturn -1;\n-\tfilename = ref_file_name(ref);\n-\tlock_filename = ref_lock_file_name(ref);\n-\tif (safe_create_leading_directories(filename))\n-\t\tdie(\"unable to create leading directory for %s\", filename);\n-\tfd = open(lock_filename, O_WRONLY | O_CREAT | O_EXCL, 0666);\n-\tif (fd < 0) {\n-\t\terror(\"Writing %s\", lock_filename);\n-\t\tperror(\"Open\");\n+\tif (!memcmp(lock->old_sha1, sha1, 20)) {\n+\t\tunlock_ref(lock);\n+\t\treturn 0;\n \t}\n-\tretval = write_ref_file(filename, lock_filename, fd, sha1);\n-\tfree(filename);\n-\tfree(lock_filename);\n-\treturn retval;\n+\tif (write(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n+\t    write(lock->lock_fd, &term, 1) != 1\n+\t\t|| close(lock->lock_fd) < 0) {\n+\t\terror(\"Couldn't write %s\", lock->lock_file);\n+\t\tunlock_ref(lock);\n+\t\treturn -1;\n+\t}\n+\tif (rename(lock->lock_file, lock->ref_file) < 0) {\n+\t\terror(\"Couldn't set %s\", lock->ref_file);\n+\t\tunlock_ref(lock);\n+\t\treturn -1;\n+\t}\n+\tlock->lock_fd = -1;\n+\tunlock_ref(lock);\n+\treturn 0;\n }\ndiff --git a/refs.h b/refs.h\nindex fa816c1..b7e9df2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -1,6 +1,13 @@\n #ifndef REFS_H\n #define REFS_H\n \n+struct ref_lock {\n+\tchar *ref_file;\n+\tchar *lock_file;\n+\tunsigned char old_sha1[20];\n+\tint lock_fd;\n+};\n+\n /*\n  * Calls the specified function for each ref file until it returns nonzero,\n  * and returns the value\n@@ -14,16 +21,17 @@ extern int for_each_remote_ref(int (*fn)\n /** Reads the refs file specified into sha1 **/\n extern int get_ref_sha1(const char *ref, unsigned char *sha1);\n \n-/** Locks ref and returns the fd to give to write_ref_sha1() if the ref\n- * has the given value currently; otherwise, returns -1.\n- **/\n-extern int lock_ref_sha1(const char *ref, const unsigned char *old_sha1);\n+/** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n+extern struct ref_lock* lock_ref_sha1(const char *ref, const unsigned char *old_sha1, int mustexist);\n+\n+/** Locks any ref (for 'HEAD' type refs). */\n+extern struct ref_lock* lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int mustexist);\n \n-/** Writes sha1 into the refs file specified, locked with the given fd. **/\n-extern int write_ref_sha1(const char *ref, int fd, const unsigned char *sha1);\n+/** Release any lock taken but not written. **/\n+extern void unlock_ref (struct ref_lock *lock);\n \n-/** Writes sha1 into the refs file specified. **/\n-extern int write_ref_sha1_unlocked(const char *ref, const unsigned char *sha1);\n+/** Writes sha1 into the ref specified by the lock. **/\n+extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n /** Returns 0 if target has the right format for a ref. **/\n extern int check_ref_format(const char *target);\n-- \n1.3.2.g7278\n"},{"id":"20130","messageId":"7vk68lgc85.fsf@assigned-by-dhcp.cox.net","threadId":"4178","inReplyTo":"20060517095502.GC28529@spearce.org","subject":"Re: [RFC 2/5] Improve abstraction of ref lock/write.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-17T10:46:50Z","receivedAt":"2006-05-17T10:46:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce <spearce@spearce.org> writes:\n\n> Created 'struct ref_lock' to contain the data necessary to perform\n> a ref update...\n>\n>  fetch.c |   32 +++++----\n>  refs.c  |  236 ++++++++++++++++++++++++++++++---------------------------------\n>  refs.h  |   24 ++++--\n>  3 files changed, 144 insertions(+), 148 deletions(-)\n\nLooks a bit intrusive probably necessary so not necessarily bad\nbut I'll postpone looking at this for now; maybe tomorrow or\nmore likely over the weekend.\n\nEyeballs from people touched the affected files in the past are\ngreatly appreciated.\n"}]}