create_symref: use existing ref-lock code

The create_symref() function predates the existence of "struct lock_file", let alone the more recent "struct ref_lock". Instead, it just does its own manual dot-locking. Besides being more code, this has a few downsides: - if git is interrupted while holding the lock, we don't clean up the lockfile - we don't do the usual directory/filename conflict check. So you can sometimes create a symref "refs/heads/foo/bar", even if "refs/heads/foo" exists (namely, if the refs are packed and we do not hit the d/f conflict in the filesystem). This patch refactors create_symref() to use the "struct ref_lock" interface, which handles both of these things. There are a few bonus cleanups that come along with it: - we leaked ref_path in some error cases - the symref contents were stored in a fixed-size buffer, putting an artificial (albeit large) limitation on the length of the refname. We now write through fprintf, and handle refnames of any size. - we called adjust_shared_perm only after the file was renamed into place, creating a potential race with readers in a shared repository. The lockfile code now handles this when creating the lockfile, making it atomic. - the legacy prefer_symlink_refs path did not do any locking at all. Admittedly, it is not atomic from a reader's perspective (as it unlinks and re-creates the symlink to overwrite), but at least it cannot conflict with other writers now. - the result of this patch is hopefully more readable. It eliminates three goto labels. Two were for error checking that is now simplified, and the third was to reach shared code that has been pulled into its own function. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Dec 29, 2015 at 00:57 UTC 370e5ad65e8878989eecbae28a479b1a4d6a841b
2 files changed +62 -55
refs/files-backend.c
+54 -55
@@ -2811,74 +2811,73 @@ static int commit_ref_update(struct ref_lock *lock,
2811 return 0;
2812 }
2813
2814 -int create_symref(const char *refname, const char *target, const char *logmsg)
2814 +static int create_ref_symlink(struct ref_lock *lock, const char *target)
2815 {
2816 - char *lockpath = NULL;
2817 - char buf[1000];
2818 - int fd, len, written;
2819 - char *ref_path = git_pathdup("%s", refname);
2820 - unsigned char old_sha1[20], new_sha1[20];
2821 - struct strbuf err = STRBUF_INIT;
2822 -
2823 - if (logmsg && read_ref(refname, old_sha1))
2824 - hashclr(old_sha1);
2825 -
2826 - if (safe_create_leading_directories(ref_path) < 0)
2827 - return error("unable to create directory for %s", ref_path);
2828 -
2816 + int ret = -1;
2817 #ifndef NO_SYMLINK_HEAD
2830 - if (prefer_symlink_refs) {
2831 - unlink(ref_path);
2832 - if (!symlink(target, ref_path))
2833 - goto done;
2818 + char *ref_path = get_locked_file_path(lock->lk);
2819 + unlink(ref_path);
2820 + ret = symlink(target, ref_path);
2821 + free(ref_path);
2822 +
2823 + if (ret)
2824 fprintf(stderr, "no symlink - falling back to symbolic ref\n");
2835 - }
2825 #endif
2826 + return ret;
2827 +}
2828
2838 - len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
2839 - if (sizeof(buf) <= len) {
2840 - error("refname too long: %s", target);
2841 - goto error_free_return;
2842 - }
2843 - lockpath = mkpathdup("%s.lock", ref_path);
2844 - fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
2845 - if (fd < 0) {
2846 - error("Unable to open %s for writing", lockpath);
2847 - goto error_free_return;
2848 - }
2849 - written = write_in_full(fd, buf, len);
2850 - if (close(fd) != 0 || written != len) {
2851 - error("Unable to write to %s", lockpath);
2852 - goto error_unlink_return;
2853 - }
2854 - if (rename(lockpath, ref_path) < 0) {
2855 - error("Unable to create %s", ref_path);
2856 - goto error_unlink_return;
2857 - }
2858 - if (adjust_shared_perm(ref_path)) {
2859 - error("Unable to fix permissions on %s", lockpath);
2860 - error_unlink_return:
2861 - unlink_or_warn(lockpath);
2862 - error_free_return:
2863 - free(lockpath);
2864 - free(ref_path);
2865 - return -1;
2866 - }
2867 - free(lockpath);
2868 -
2869 -#ifndef NO_SYMLINK_HEAD
2870 - done:
2871 -#endif
2829 +static void update_symref_reflog(struct ref_lock *lock, const char *refname,
2830 + const char *target, const char *logmsg)
2831 +{
2832 + struct strbuf err = STRBUF_INIT;
2833 + unsigned char new_sha1[20];
2834 if (logmsg && !read_ref(target, new_sha1) &&
2873 - log_ref_write(refname, old_sha1, new_sha1, logmsg, 0, &err)) {
2835 + log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
2836 error("%s", err.buf);
2837 strbuf_release(&err);
2838 }
2839 +}
2840
2878 - free(ref_path);
2841 +static int create_symref_locked(struct ref_lock *lock, const char *refname,
2842 + const char *target, const char *logmsg)
2843 +{
2844 + if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
2845 + update_symref_reflog(lock, refname, target, logmsg);
2846 + return 0;
2847 + }
2848 +
2849 + if (!fdopen_lock_file(lock->lk, "w"))
2850 + return error("unable to fdopen %s: %s",
2851 + lock->lk->tempfile.filename.buf, strerror(errno));
2852 +
2853 + /* no error check; commit_ref will check ferror */
2854 + fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
2855 + if (commit_ref(lock) < 0)
2856 + return error("unable to write symref for %s: %s", refname,
2857 + strerror(errno));
2858 + update_symref_reflog(lock, refname, target, logmsg);
2859 return 0;
2860 }
2861
2862 +int create_symref(const char *refname, const char *target, const char *logmsg)
2863 +{
2864 + struct strbuf err = STRBUF_INIT;
2865 + struct ref_lock *lock;
2866 + int ret;
2867 +
2868 + lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
2869 + &err);
2870 + if (!lock) {
2871 + error("%s", err.buf);
2872 + strbuf_release(&err);
2873 + return -1;
2874 + }
2875 +
2876 + ret = create_symref_locked(lock, refname, target, logmsg);
2877 + unlock_ref(lock);
2878 + return ret;
2879 +}
2880 +
2881 int reflog_exists(const char *refname)
2882 {
2883 struct stat st;
t/t1401-symbolic-ref.sh
+8
@@ -114,4 +114,12 @@ test_expect_success 'symbolic-ref writes reflog entry' '
114 test_cmp expect actual
115 '
116
117 +test_expect_success 'symbolic-ref does not create ref d/f conflicts' '
118 + git checkout -b df &&
119 + test_commit df &&
120 + test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df &&
121 + git pack-refs --all --prune &&
122 + test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df
123 +'
124 +
125 test_done