{"thread":{"id":"41044","subject":"[PATCH 0/4] improve symbolic-ref robustness","startedAt":"2015-12-20T07:26:38Z","lastAt":"2015-12-30T06:57:28Z","messageCount":25,"participants":["Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"274767","messageId":"20151220072637.GA22102@sigill.intra.peff.net","threadId":"41044","inReplyTo":null,"subject":"[PATCH 0/4] improve symbolic-ref robustness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-20T07:26:38Z","receivedAt":"2015-12-20T07:26:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed that an interrupt \"git symbolic-ref\" will not clean up\n\"HEAD.lock\". So I started this series as an attempt to convert\ncreate_symref() to \"struct lock_file\" to get the usual tempfile cleanup.\n\nBut I found a few other points of interest. The biggest is that\ngit-symbolic-ref does not actually propagate errors in its exit code.\nWhoops. That's fixed in the first patch, which could go separately to\n\"maint\".\n\nThe rest of it is fairly dependent on master because of the\nrefs/files-backend.c code movement. I can backport it if we really want\nit for maint.\n\n [1/4]: symbolic-ref: propagate error code from create_symref()\n [2/4]: t1401: test reflog creation for git-symbolic-ref\n [3/4]: create_symref: modernize variable names\n [4/4]: create_symref: use existing ref-lock code\n\n-Peff\n"},{"id":"274768","messageId":"20151220072718.GA30662@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151220072637.GA22102@sigill.intra.peff.net","subject":"[PATCH 1/4] symbolic-ref: propagate error code from create_symref()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-20T07:27:18Z","receivedAt":"2015-12-20T07:27:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If create_symref() fails, git-symbolic-ref will still exit\nwith code 0, and our caller has no idea that the command did\nnothing.\n\nThis appears to have been broken since the beginning of time\n(e.g., it is not a regression where create_symref() stopped\ncalling die() or something similar).\n\nSigned-off-by: Jeff King \n---\n builtin/symbolic-ref.c | 2 +-\n t/t1401-symbolic-ref.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex ce0fde7..9c29a64 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -67,7 +67,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n \t\t !starts_with(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n-\t\tcreate_symref(argv[0], argv[1], msg);\n+\t\tret = !!create_symref(argv[0], argv[1], msg);\n \t\tbreak;\n \tdefault:\n \t\tusage_with_options(git_symbolic_ref_usage, options);\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 20b022a..fbb4835 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -92,4 +92,10 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'symbolic-ref reports failure in exit code' '\n+\ttest_when_finished \"rm -f .git/HEAD.lock\" &&\n+\t>.git/HEAD.lock &&\n+\ttest_must_fail git symbolic-ref HEAD refs/heads/whatever\n+'\n+\n test_done\n-- \n2.7.0.rc1.350.g9acc0f4\n"},{"id":"274769","messageId":"20151220072723.GB30662@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151220072637.GA22102@sigill.intra.peff.net","subject":"[PATCH 2/4] t1401: test reflog creation for git-symbolic-ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-20T07:27:23Z","receivedAt":"2015-12-20T07:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current code writes a reflog entry whenever we update a\nsymbolic ref, but we never test that this is so. Let's add a\ntest to make sure upcoming refactoring doesn't cause a\nregression.\n\nSigned-off-by: Jeff King \n---\n t/t1401-symbolic-ref.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex fbb4835..1f0dff3 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -98,4 +98,20 @@ test_expect_success 'symbolic-ref reports failure in exit code' '\n \ttest_must_fail git symbolic-ref HEAD refs/heads/whatever\n '\n \n+test_expect_success 'symbolic-ref writes reflog entry' '\n+\tgit checkout -b log1 &&\n+\ttest_commit one &&\n+\tgit checkout -b log2 &&\n+\ttest_commit two &&\n+\tgit checkout --orphan orphan &&\n+\tgit symbolic-ref -m create HEAD refs/heads/log1 &&\n+\tgit symbolic-ref -m update HEAD refs/heads/log2 &&\n+\tcat >expect <<-\\EOF &&\n+\tupdate\n+\tcreate\n+\tEOF\n+\tgit log --format=%gs -g >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.rc1.350.g9acc0f4\n"},{"id":"274770","messageId":"20151220072949.GC30662@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151220072637.GA22102@sigill.intra.peff.net","subject":"[PATCH 3/4] create_symref: modernize variable names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-20T07:29:49Z","receivedAt":"2015-12-20T07:29:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, create_symref() was used only to point\nHEAD at a branch name, and the variable names reflect that\n(e.g., calling the path git_HEAD). However, it is much more\ngeneric these days (and has been for some time). Let's\nupdate the variable names to make it easier to follow:\n\n - `ref_target` is now just `ref`, matching the declaration\n in `cache.h` (and hopefully making it clear that it is\n the symref itself, and not the target).\n\n - `git_HEAD` is now `ref_path`; the on-disk path\n corresponding to `ref`.\n\n - `refs_heads_master` is now just `target`; i.e., what the\n symref points at. This term also matches what is in\n the symlink(2) manpage (at least on Linux).\n\n - the buffer to hold the symref file's contents was simply\n called `ref`. It's now `buf` (admittedly also generic,\n but at least not actively introducing confusion with the\n other variable holding the refname).\n\nSigned-off-by: Jeff King \n---\nThis could actually be squashed in with patch 4, as most of the changed\ninstances just go away. I did find the new names easier to work with,\nthough, so maybe they make the diff for that patch easier.\n\n refs.h | 2 +-\n refs/files-backend.c | 41 ++++++++++++++++++++---------------------\n 2 files changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 7a04077..96a25ad 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -292,7 +292,7 @@ extern char *shorten_unambiguous_ref(const char *refname, int strict);\n /** rename ref, return 0 on success **/\n extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);\n \n-extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);\n+extern int create_symref(const char *ref, const char *target, const char *logmsg);\n \n enum action_on_err {\n \tUPDATE_REFS_MSG_ON_ERR,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c648b5e..6bfa139 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2811,58 +2811,57 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-int create_symref(const char *ref_target, const char *refs_heads_master,\n-\t\t const char *logmsg)\n+int create_symref(const char *ref, const char *target, const char *logmsg)\n {\n \tchar *lockpath = NULL;\n-\tchar ref[1000];\n+\tchar buf[1000];\n \tint fd, len, written;\n-\tchar *git_HEAD = git_pathdup(\"%s\", ref_target);\n+\tchar *ref_path = git_pathdup(\"%s\", ref);\n \tunsigned char old_sha1[20], new_sha1[20];\n \tstruct strbuf err = STRBUF_INIT;\n \n-\tif (logmsg && read_ref(ref_target, old_sha1))\n+\tif (logmsg && read_ref(ref, old_sha1))\n \t\thashclr(old_sha1);\n \n-\tif (safe_create_leading_directories(git_HEAD) < 0)\n-\t\treturn error(\"unable to create directory for %s\", git_HEAD);\n+\tif (safe_create_leading_directories(ref_path) < 0)\n+\t\treturn error(\"unable to create directory for %s\", ref_path);\n \n #ifndef NO_SYMLINK_HEAD\n \tif (prefer_symlink_refs) {\n-\t\tunlink(git_HEAD);\n-\t\tif (!symlink(refs_heads_master, git_HEAD))\n+\t\tunlink(ref_path);\n+\t\tif (!symlink(target, ref_path))\n \t\t\tgoto done;\n \t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n \t}\n #endif\n \n-\tlen = snprintf(ref, sizeof(ref), \"ref: %s\\n\", refs_heads_master);\n-\tif (sizeof(ref) <= len) {\n-\t\terror(\"refname too long: %s\", refs_heads_master);\n+\tlen = snprintf(buf, sizeof(buf), \"ref: %s\\n\", target);\n+\tif (sizeof(buf) <= len) {\n+\t\terror(\"refname too long: %s\", target);\n \t\tgoto error_free_return;\n \t}\n-\tlockpath = mkpathdup(\"%s.lock\", git_HEAD);\n+\tlockpath = mkpathdup(\"%s.lock\", ref_path);\n \tfd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \tif (fd < 0) {\n \t\terror(\"Unable to open %s for writing\", lockpath);\n \t\tgoto error_free_return;\n \t}\n-\twritten = write_in_full(fd, ref, len);\n+\twritten = write_in_full(fd, buf, len);\n \tif (close(fd) != 0 || written != len) {\n \t\terror(\"Unable to write to %s\", lockpath);\n \t\tgoto error_unlink_return;\n \t}\n-\tif (rename(lockpath, git_HEAD) < 0) {\n-\t\terror(\"Unable to create %s\", git_HEAD);\n+\tif (rename(lockpath, ref_path) < 0) {\n+\t\terror(\"Unable to create %s\", ref_path);\n \t\tgoto error_unlink_return;\n \t}\n-\tif (adjust_shared_perm(git_HEAD)) {\n+\tif (adjust_shared_perm(ref_path)) {\n \t\terror(\"Unable to fix permissions on %s\", lockpath);\n \terror_unlink_return:\n \t\tunlink_or_warn(lockpath);\n \terror_free_return:\n \t\tfree(lockpath);\n-\t\tfree(git_HEAD);\n+\t\tfree(ref_path);\n \t\treturn -1;\n \t}\n \tfree(lockpath);\n@@ -2870,13 +2869,13 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n #ifndef NO_SYMLINK_HEAD\n \tdone:\n #endif\n-\tif (logmsg && !read_ref(refs_heads_master, new_sha1) &&\n-\t\tlog_ref_write(ref_target, old_sha1, new_sha1, logmsg, 0, &err)) {\n+\tif (logmsg && !read_ref(target, new_sha1) &&\n+\t\tlog_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n \n-\tfree(git_HEAD);\n+\tfree(ref_path);\n \treturn 0;\n }\n \n-- \n2.7.0.rc1.350.g9acc0f4\n"},{"id":"274771","messageId":"20151220073414.GD30662@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151220072637.GA22102@sigill.intra.peff.net","subject":"[PATCH 4/4] create_symref: use existing ref-lock code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-20T07:34:14Z","receivedAt":"2015-12-20T07:34:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The create_symref() function predates the existence of\n\"struct lock_file\", let alone the more recent \"struct\nref_lock\". Instead, it just does its own manual dot-locking.\nBesides being more code, this has a few downsides:\n\n - if git is interrupted while holding the lock, we don't\n clean up the lockfile\n\n - we don't do the usual directory/filename conflict check.\n So you can sometimes create a symref \"refs/heads/foo/bar\",\n even if \"refs/heads/foo\" exists (namely, if the refs are\n packed and we do not hit the d/f conflict in the\n filesystem).\n\nThis patch refactors create_symref() to use the \"struct\nref_lock\" interface, which handles both of these things.\nThere are a few bonus cleanups that come along with it:\n\n - we leaked ref_path in some error cases\n\n - the symref contents were stored in a fixed-size buffer,\n putting an artificial (albeit large) limitation on the\n length of the refname. We now write through fprintf, and\n handle refnames of any size.\n\n - we called adjust_shared_perm only after the file was\n renamed into place, creating a potential race with\n readers in a shared repository. Now we fix the\n permissions first, and commit only if that succeeded.\n This also makes the update atomic with respect to our\n exit code (whereas previously, we might report failure\n even though we updated the ref).\n\n - the legacy prefer_symlink_refs path did not do any\n locking at all. Admittedly, it is not atomic from a\n reader's perspective (and it cannot be; it has to unlink\n and then symlink, creating a race), but at least it\n cannot conflict with other writers now.\n\n - the result of this patch is hopefully more readable. It\n eliminates three goto labels. Two were for error checking\n that is now simplified, and the third was to reach shared\n code that has been pulled into its own function.\n\nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 113 +++++++++++++++++++++++++-----------------------\n t/t1401-symbolic-ref.sh | 8 ++++\n 2 files changed, 66 insertions(+), 55 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6bfa139..3d53c42 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2811,74 +2811,77 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-int create_symref(const char *ref, const char *target, const char *logmsg)\n+static int create_ref_symlink(struct ref_lock *lock, const char *target)\n {\n-\tchar *lockpath = NULL;\n-\tchar buf[1000];\n-\tint fd, len, written;\n-\tchar *ref_path = git_pathdup(\"%s\", ref);\n-\tunsigned char old_sha1[20], new_sha1[20];\n-\tstruct strbuf err = STRBUF_INIT;\n-\n-\tif (logmsg && read_ref(ref, old_sha1))\n-\t\thashclr(old_sha1);\n-\n-\tif (safe_create_leading_directories(ref_path) < 0)\n-\t\treturn error(\"unable to create directory for %s\", ref_path);\n-\n+\tint ret = -1;\n #ifndef NO_SYMLINK_HEAD\n-\tif (prefer_symlink_refs) {\n-\t\tunlink(ref_path);\n-\t\tif (!symlink(target, ref_path))\n-\t\t\tgoto done;\n+\tchar *ref_path = get_locked_file_path(lock->lk);\n+\tunlink(ref_path);\n+\tret = symlink(target, ref_path);\n+\tfree(ref_path);\n+\n+\tif (ret)\n \t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n-\t}\n #endif\n+\treturn ret;\n+}\n \n-\tlen = snprintf(buf, sizeof(buf), \"ref: %s\\n\", target);\n-\tif (sizeof(buf) <= len) {\n-\t\terror(\"refname too long: %s\", target);\n-\t\tgoto error_free_return;\n-\t}\n-\tlockpath = mkpathdup(\"%s.lock\", ref_path);\n-\tfd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n-\tif (fd < 0) {\n-\t\terror(\"Unable to open %s for writing\", lockpath);\n-\t\tgoto error_free_return;\n-\t}\n-\twritten = write_in_full(fd, buf, len);\n-\tif (close(fd) != 0 || written != len) {\n-\t\terror(\"Unable to write to %s\", lockpath);\n-\t\tgoto error_unlink_return;\n-\t}\n-\tif (rename(lockpath, ref_path) < 0) {\n-\t\terror(\"Unable to create %s\", ref_path);\n-\t\tgoto error_unlink_return;\n-\t}\n-\tif (adjust_shared_perm(ref_path)) {\n-\t\terror(\"Unable to fix permissions on %s\", lockpath);\n-\terror_unlink_return:\n-\t\tunlink_or_warn(lockpath);\n-\terror_free_return:\n-\t\tfree(lockpath);\n-\t\tfree(ref_path);\n-\t\treturn -1;\n-\t}\n-\tfree(lockpath);\n-\n-#ifndef NO_SYMLINK_HEAD\n-\tdone:\n-#endif\n+static void update_symref_reflog(struct ref_lock *lock, const char *ref,\n+\t\t\t\t const char *target, const char *logmsg)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tunsigned char new_sha1[20];\n \tif (logmsg && !read_ref(target, new_sha1) &&\n-\t\tlog_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {\n+\t log_ref_write(ref, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n+}\n \n-\tfree(ref_path);\n+static int create_symref_locked(struct ref_lock *lock, const char *ref,\n+\t\t\t\tconst char *target, const char *logmsg)\n+{\n+\tif (prefer_symlink_refs && !create_ref_symlink(lock, target)) {\n+\t\tupdate_symref_reflog(lock, ref, target, logmsg);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!fdopen_lock_file(lock->lk, \"w\"))\n+\t\treturn error(\"unable to fdopen %s: %s\",\n+\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n+\n+\tif (adjust_shared_perm(lock->lk->tempfile.filename.buf))\n+\t\treturn error(\"unable to fix permissions on %s: %s\",\n+\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n+\n+\t/* no error check; commit_ref will check ferror */\n+\tfprintf(lock->lk->tempfile.fp, \"ref: %s\\n\", target);\n+\tif (commit_ref(lock) < 0)\n+\t\treturn error(\"unable to write symref for %s: %s\", ref,\n+\t\t\t strerror(errno));\n+\tupdate_symref_reflog(lock, ref, target, logmsg);\n \treturn 0;\n }\n \n+int create_symref(const char *ref, const char *target, const char *logmsg)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_lock *lock;\n+\tint ret;\n+\n+\tlock = lock_ref_sha1_basic(ref, NULL, NULL, NULL, REF_NODEREF, NULL,\n+\t\t\t\t &err);\n+\tif (!lock) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n+\tret = create_symref_locked(lock, ref, target, logmsg);\n+\tunlock_ref(lock);\n+\treturn ret;\n+}\n+\n int reflog_exists(const char *refname)\n {\n \tstruct stat st;\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 1f0dff3..5db876c 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -114,4 +114,12 @@ test_expect_success 'symbolic-ref writes reflog entry' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n+\tgit checkout -b df &&\n+\ttest_commit df &&\n+\ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df &&\n+\tgit pack-refs --all --prune &&\n+\ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n+'\n+\n test_done\n-- \n2.7.0.rc1.350.g9acc0f4\n"},{"id":"274826","messageId":"xmqq8u4nfrvv.fsf@gitster.mtv.corp.google.com","threadId":"41044","inReplyTo":"20151220073414.GD30662@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] create_symref: use existing ref-lock code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T20:50:28Z","receivedAt":"2015-12-21T20:50:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> #ifndef NO_SYMLINK_HEAD\n> -\tif (prefer_symlink_refs) {\n> -\t\tunlink(ref_path);\n> -\t\tif (!symlink(target, ref_path))\n> -\t\t\tgoto done;\n\nI see that the original was sloppy (most certainly my bad) ...\n\n> +\tchar *ref_path = get_locked_file_path(lock->lk);\n> +\tunlink(ref_path);\n\n... and you inherited that. I see a few seemingly related helpers\nin wrapper.c, but none looks useful in this context X-<.\n\n if (unlink_or_warn(ref_path))\n \treturn -1;\n\nis close enough, but it still lets the caller fallback to textual\nsymref.\n\n> +\tret = symlink(target, ref_path);\n> +\tfree(ref_path);\n> +\n> +\tif (ret)\n> \t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n> -\t}\n> #endif\n> +\treturn ret;\n> +}\n> \n> -\tlen = snprintf(buf, sizeof(buf), \"ref: %s\\n\", target);\n> -...\n> -\tfree(lockpath);\n> -\n> -#ifndef NO_SYMLINK_HEAD\n> -\tdone:\n> -#endif\n\nThis huge block is worth removing ;-) Thanks.\n"},{"id":"274835","messageId":"20151222005824.GB6901@sigill.intra.peff.net","threadId":"41044","inReplyTo":"xmqq8u4nfrvv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] create_symref: use existing ref-lock code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-22T00:58:24Z","receivedAt":"2015-12-22T00:58:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 21, 2015 at 12:50:28PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > #ifndef NO_SYMLINK_HEAD\n> > -\tif (prefer_symlink_refs) {\n> > -\t\tunlink(ref_path);\n> > -\t\tif (!symlink(target, ref_path))\n> > -\t\t\tgoto done;\n> \n> I see that the original was sloppy (most certainly my bad) ...\n> \n> > +\tchar *ref_path = get_locked_file_path(lock->lk);\n> > +\tunlink(ref_path);\n> \n> ... and you inherited that. I see a few seemingly related helpers\n> in wrapper.c, but none looks useful in this context X-<.\n> \n> if (unlink_or_warn(ref_path))\n> \treturn -1;\n> \n> is close enough, but it still lets the caller fallback to textual\n> symref.\n\nI don't think the original is _wrong_; it's meant to be \"unlink if\nneeded\", and the symlink call is what we really care about (if our\nunlink failed for anything but ENOENT, the symlink will, too). But I\nagree unlink_or_warn should do that and give us a nice warning (and\ncover up ENOENT). To fall through, I think we just want (in the\noriginal):\n\n if (!unlink_or_warn(ref_path) && !symlink(target, ref_path))\n\tgoto done;\n\nand in mine that becomes:\n\n int ret = -1;\n ...\n if (!unlink_or_warn(ref_path) && !symlink(target, ref_path))\n\tret = 0;\n\nI must confess I considered a follow-on patch to drop the\nprefer_symlink_refs code path completely. I'm surprised it all still\nworks with packed-refs, etc, but resolve_ref does take special care to\nuse readlink().\n\n-Peff\n"},{"id":"275038","messageId":"5680F0DA.1020301@alum.mit.edu","threadId":"41044","inReplyTo":"20151220072949.GC30662@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] create_symref: modernize variable names","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-12-28T08:20:42Z","receivedAt":"2015-12-28T08:20:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/20/2015 08:29 AM, Jeff King wrote:\n> Once upon a time, create_symref() was used only to point\n> HEAD at a branch name, and the variable names reflect that\n> (e.g., calling the path git_HEAD). However, it is much more\n> generic these days (and has been for some time). Let's\n> update the variable names to make it easier to follow:\n> \n> - `ref_target` is now just `ref`, matching the declaration\n> in `cache.h` (and hopefully making it clear that it is\n> the symref itself, and not the target).\n\nI've been trying to name variables that hold reference names \"refname\"\nto distinguish them clearly from other representations of references,\nlike \"struct ref_entry *\".\n\n> [...]\n\nOtherwise LGTM.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"275039","messageId":"568104AF.102@alum.mit.edu","threadId":"41044","inReplyTo":"20151220073414.GD30662@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] create_symref: use existing ref-lock code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-12-28T09:45:19Z","receivedAt":"2015-12-28T09:45:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/20/2015 08:34 AM, Jeff King wrote:\n> The create_symref() function predates the existence of\n> \"struct lock_file\", let alone the more recent \"struct\n> ref_lock\". Instead, it just does its own manual dot-locking.\n> Besides being more code, this has a few downsides:\n> \n> - if git is interrupted while holding the lock, we don't\n> clean up the lockfile\n> \n> - we don't do the usual directory/filename conflict check.\n> So you can sometimes create a symref \"refs/heads/foo/bar\",\n> even if \"refs/heads/foo\" exists (namely, if the refs are\n> packed and we do not hit the d/f conflict in the\n> filesystem).\n> \n> This patch refactors create_symref() to use the \"struct\n> ref_lock\" interface, which handles both of these things.\n> There are a few bonus cleanups that come along with it:\n> \n> - we leaked ref_path in some error cases\n> \n> - the symref contents were stored in a fixed-size buffer,\n> putting an artificial (albeit large) limitation on the\n> length of the refname. We now write through fprintf, and\n> handle refnames of any size.\n> \n> - we called adjust_shared_perm only after the file was\n> renamed into place, creating a potential race with\n> readers in a shared repository. Now we fix the\n> permissions first, and commit only if that succeeded.\n> This also makes the update atomic with respect to our\n> exit code (whereas previously, we might report failure\n> even though we updated the ref).\n> \n> - the legacy prefer_symlink_refs path did not do any\n> locking at all. Admittedly, it is not atomic from a\n> reader's perspective (and it cannot be; it has to unlink\n> and then symlink, creating a race), but at least it\n> cannot conflict with other writers now.\n> \n> - the result of this patch is hopefully more readable. It\n> eliminates three goto labels. Two were for error checking\n> that is now simplified, and the third was to reach shared\n> code that has been pulled into its own function.\n> \n> Signed-off-by: Jeff King \n> ---\n> refs/files-backend.c | 113 +++++++++++++++++++++++++-----------------------\n> t/t1401-symbolic-ref.sh | 8 ++++\n> 2 files changed, 66 insertions(+), 55 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 6bfa139..3d53c42 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2811,74 +2811,77 @@ static int commit_ref_update(struct ref_lock *lock,\n> \treturn 0;\n> }\n> \n> -int create_symref(const char *ref, const char *target, const char *logmsg)\n> +static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> {\n> -\tchar *lockpath = NULL;\n> -\tchar buf[1000];\n> -\tint fd, len, written;\n> -\tchar *ref_path = git_pathdup(\"%s\", ref);\n> -\tunsigned char old_sha1[20], new_sha1[20];\n> -\tstruct strbuf err = STRBUF_INIT;\n> -\n> -\tif (logmsg && read_ref(ref, old_sha1))\n> -\t\thashclr(old_sha1);\n> -\n> -\tif (safe_create_leading_directories(ref_path) < 0)\n> -\t\treturn error(\"unable to create directory for %s\", ref_path);\n> -\n> +\tint ret = -1;\n> #ifndef NO_SYMLINK_HEAD\n> -\tif (prefer_symlink_refs) {\n> -\t\tunlink(ref_path);\n> -\t\tif (!symlink(target, ref_path))\n> -\t\t\tgoto done;\n> +\tchar *ref_path = get_locked_file_path(lock->lk);\n> +\tunlink(ref_path);\n> +\tret = symlink(target, ref_path);\n> +\tfree(ref_path);\n> +\n> +\tif (ret)\n> \t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n> -\t}\n> #endif\n> +\treturn ret;\n> +}\n\nThis function is racy. A reader might see no reference at all in the\nmoment between the `unlink()` and the `symlink()`. Moreover, if this\nprocess is killed at that moment, the symbolic ref would be gone forever.\n\nI think that the semantics of `rename()` would allow this race to be\nfixed, though, since `symlink()` doesn't have the analogue of\n`O_CREAT|O_EXCL`, one would need a lockfile *and* a second temporary\nfilename under which the new symlink is originally created.\n\nHowever, this race has always been here, and symlink-based symrefs are\nobsolete, so it's probably not worth fixing.\n\n> -\tlen = snprintf(buf, sizeof(buf), \"ref: %s\\n\", target);\n> -\tif (sizeof(buf) <= len) {\n> -\t\terror(\"refname too long: %s\", target);\n> -\t\tgoto error_free_return;\n> -\t}\n> -\tlockpath = mkpathdup(\"%s.lock\", ref_path);\n> -\tfd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n> -\tif (fd < 0) {\n> -\t\terror(\"Unable to open %s for writing\", lockpath);\n> -\t\tgoto error_free_return;\n> -\t}\n> -\twritten = write_in_full(fd, buf, len);\n> -\tif (close(fd) != 0 || written != len) {\n> -\t\terror(\"Unable to write to %s\", lockpath);\n> -\t\tgoto error_unlink_return;\n> -\t}\n> -\tif (rename(lockpath, ref_path) < 0) {\n> -\t\terror(\"Unable to create %s\", ref_path);\n> -\t\tgoto error_unlink_return;\n> -\t}\n> -\tif (adjust_shared_perm(ref_path)) {\n> -\t\terror(\"Unable to fix permissions on %s\", lockpath);\n> -\terror_unlink_return:\n> -\t\tunlink_or_warn(lockpath);\n> -\terror_free_return:\n> -\t\tfree(lockpath);\n> -\t\tfree(ref_path);\n> -\t\treturn -1;\n> -\t}\n> -\tfree(lockpath);\n> -\n> -#ifndef NO_SYMLINK_HEAD\n> -\tdone:\n> -#endif\n> +static void update_symref_reflog(struct ref_lock *lock, const char *ref,\n> +\t\t\t\t const char *target, const char *logmsg)\n> +{\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tunsigned char new_sha1[20];\n> \tif (logmsg && !read_ref(target, new_sha1) &&\n> -\t\tlog_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {\n> +\t log_ref_write(ref, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {\n> \t\terror(\"%s\", err.buf);\n> \t\tstrbuf_release(&err);\n> \t}\n> +}\n> \n> -\tfree(ref_path);\n> +static int create_symref_locked(struct ref_lock *lock, const char *ref,\n> +\t\t\t\tconst char *target, const char *logmsg)\n> +{\n> +\tif (prefer_symlink_refs && !create_ref_symlink(lock, target)) {\n> +\t\tupdate_symref_reflog(lock, ref, target, logmsg);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (!fdopen_lock_file(lock->lk, \"w\"))\n> +\t\treturn error(\"unable to fdopen %s: %s\",\n> +\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n> +\n> +\tif (adjust_shared_perm(lock->lk->tempfile.filename.buf))\n> +\t\treturn error(\"unable to fix permissions on %s: %s\",\n> +\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n\nYou can skip this step. lock_file() already calls adjust_shared_perm().\n\n> +\t/* no error check; commit_ref will check ferror */\n> +\tfprintf(lock->lk->tempfile.fp, \"ref: %s\\n\", target);\n> +\tif (commit_ref(lock) < 0)\n> +\t\treturn error(\"unable to write symref for %s: %s\", ref,\n> +\t\t\t strerror(errno));\n> +\tupdate_symref_reflog(lock, ref, target, logmsg);\n\nHere is another problem that didn't originate with your changes:\n\nThe reflog should be written while holding the reference lock, to\nprevent two processes' trying to write new entries at the same time.\n\nI think the problem would be solved if you move the call to\nupdate_symref_reflog() above the call to commit_ref().\n\nGranted, this could case a reflog entry to be written for a reference\nupdate whose commit fails, but that's also a risk for non-symbolic\nreferences. Fixing this residual problem would require the ability to\nroll back reflog changes.\n\n> \treturn 0;\n> }\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"275067","messageId":"20151229050230.GA13253@sigill.intra.peff.net","threadId":"41044","inReplyTo":"568104AF.102@alum.mit.edu","subject":"Re: [PATCH 4/4] create_symref: use existing ref-lock code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:02:30Z","receivedAt":"2015-12-29T05:02:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 28, 2015 at 10:45:19AM +0100, Michael Haggerty wrote:\n\n> > +static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> [...]\n> > +\tchar *ref_path = get_locked_file_path(lock->lk);\n> > +\tunlink(ref_path);\n> > +\tret = symlink(target, ref_path);\n> [...]\n> \n> This function is racy. A reader might see no reference at all in the\n> moment between the `unlink()` and the `symlink()`. Moreover, if this\n> process is killed at that moment, the symbolic ref would be gone forever.\n> \n> I think that the semantics of `rename()` would allow this race to be\n> fixed, though, since `symlink()` doesn't have the analogue of\n> `O_CREAT|O_EXCL`, one would need a lockfile *and* a second temporary\n> filename under which the new symlink is originally created.\n> \n> However, this race has always been here, and symlink-based symrefs are\n> obsolete, so it's probably not worth fixing.\n\nYeah. In the commit message I wrote:\n\n> > - the legacy prefer_symlink_refs path did not do any\n> > locking at all. Admittedly, it is not atomic from a\n> > reader's perspective (and it cannot be; it has to unlink\n> > and then symlink, creating a race), but at least it\n> > cannot conflict with other writers now.\n\nbut I think you are right that we could do tricks with rename() on the\nsymlink. That is in POSIX, though I wouldn't be surprised if it\nmisbehaves on some obscure systems. Given that using symlinks is only\ntriggered by an undocumented (!) option that presumably very few people\nuse, I'm inclined to leave it as-is.\n\nI was actually tempted to rip it out, as the option is mostly a hack\nfrom 2006:\n\n http://article.gmane.org/gmane.comp.version-control.git/19402\n\nIt was done to let people bisect old kernel trees whose build system\n_depends_ on .git/HEAD being a symlink. My thought is:\n\n - people are a lot less likely to bisect back that far anymore\n\n - ...and if they do, their build system will be totally broken by the\n new ref-backend stuff\n\n - ...and the right solution is not to put a hack in git, but for the\n bisecting user to \"fix up\" the tree before testing (by munging the\n symlink themselves, or applying a patch to the build system to use\n `git rev-parse`). This matches what people have to do for every\n other type of weird \"the old version doesn't quite build\"\n incompatibility that has nothing to do with git.\n\nBut that should probably come as a separate patch (it affects this only\nin that it makes this patch simpler to do that cleanup first :) ).\n\n> > +\tif (adjust_shared_perm(lock->lk->tempfile.filename.buf))\n> > +\t\treturn error(\"unable to fix permissions on %s: %s\",\n> > +\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n> \n> You can skip this step. lock_file() already calls adjust_shared_perm().\n\nThanks, fixed.\n\n> > +\t/* no error check; commit_ref will check ferror */\n> > +\tfprintf(lock->lk->tempfile.fp, \"ref: %s\\n\", target);\n> > +\tif (commit_ref(lock) < 0)\n> > +\t\treturn error(\"unable to write symref for %s: %s\", ref,\n> > +\t\t\t strerror(errno));\n> > +\tupdate_symref_reflog(lock, ref, target, logmsg);\n> \n> Here is another problem that didn't originate with your changes:\n> \n> The reflog should be written while holding the reference lock, to\n> prevent two processes' trying to write new entries at the same time.\n> \n> I think the problem would be solved if you move the call to\n> update_symref_reflog() above the call to commit_ref().\n> \n> Granted, this could case a reflog entry to be written for a reference\n> update whose commit fails, but that's also a risk for non-symbolic\n> references. Fixing this residual problem would require the ability to\n> roll back reflog changes.\n\nThanks, I agree with your reasoning (and especially that we should err\non the same side that normal ref-writing does). I'll do it in a\nfollow-on patch, though, to make it more clear what is going on.\n\n-Peff\n"},{"id":"275068","messageId":"20151229050255.GB13253@sigill.intra.peff.net","threadId":"41044","inReplyTo":"5680F0DA.1020301@alum.mit.edu","subject":"Re: [PATCH 3/4] create_symref: modernize variable names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:02:56Z","receivedAt":"2015-12-29T05:02:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 28, 2015 at 09:20:42AM +0100, Michael Haggerty wrote:\n\n> On 12/20/2015 08:29 AM, Jeff King wrote:\n> > Once upon a time, create_symref() was used only to point\n> > HEAD at a branch name, and the variable names reflect that\n> > (e.g., calling the path git_HEAD). However, it is much more\n> > generic these days (and has been for some time). Let's\n> > update the variable names to make it easier to follow:\n> > \n> > - `ref_target` is now just `ref`, matching the declaration\n> > in `cache.h` (and hopefully making it clear that it is\n> > the symref itself, and not the target).\n> \n> I've been trying to name variables that hold reference names \"refname\"\n> to distinguish them clearly from other representations of references,\n> like \"struct ref_entry *\".\n\nThat makes sense. I've tweaked this patch to use \"refname\", and carried\nthrough the change to the following patch.\n\n-Peff\n"},{"id":"275069","messageId":"20151229054116.GA14551@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151229050230.GA13253@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] create_symref: use existing ref-lock code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:41:16Z","receivedAt":"2015-12-29T05:41:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 29, 2015 at 12:02:30AM -0500, Jeff King wrote:\n\n> Given that using symlinks is only\n> triggered by an undocumented (!) option that presumably very few people\n> use, I'm inclined to leave it as-is.\n\nSorry, I am wrong here. We do document core.preferSymlinkRefs. I missed\nit because my grep was case-sensitive. :-/\n\n-Peff\n"},{"id":"275070","messageId":"20151229055558.GA12848@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151220072637.GA22102@sigill.intra.peff.net","subject":"[PATCH v2 0/3] improve symbolic-ref robustness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:55:58Z","receivedAt":"2015-12-29T05:55:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 20, 2015 at 02:26:37AM -0500, Jeff King wrote:\n\n> I noticed that an interrupt \"git symbolic-ref\" will not clean up\n> \"HEAD.lock\". So I started this series as an attempt to convert\n> create_symref() to \"struct lock_file\" to get the usual tempfile cleanup.\n\nHere's version 2, based on comments from Michael. The first two patches\nwere picked out separately for jk/symbolic-ref-maint, so I've dropped\nthem here (so 1+2 here are the original 3+4).\n\nThe other differences from v1 are:\n\n - use \"refname\" instead of \"ref\" to match surrounding code\n\n - drop adjust_shared_perm, as lockfile does it for us\n\n - adjust reflog writing order (done in a new patch)\n\nThe patches are:\n\n [1/3]: create_symref: modernize variable names\n [2/3]: create_symref: use existing ref-lock code\n [3/3]: create_symref: write reflog while holding lock\n\n-Peff\n"},{"id":"275071","messageId":"20151229055644.GA17012@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151229055558.GA12848@sigill.intra.peff.net","subject":"[PATCH 1/3] create_symref: modernize variable names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:56:44Z","receivedAt":"2015-12-29T05:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, create_symref() was used only to point\nHEAD at a branch name, and the variable names reflect that\n(e.g., calling the path git_HEAD). However, it is much more\ngeneric these days (and has been for some time). Let's\nupdate the variable names to make it easier to follow:\n\n - `ref_target` is now just `refname`. This is closer to\n the `ref` that is already in `cache.h`, but with the\n extra twist that \"name\" makes it clear this is the name\n and not a ref struct. Dropping \"target\" hopefully makes\n it clear that we are talking about the symref itself,\n not what it points to.\n\n - `git_HEAD` is now `ref_path`; the on-disk path\n corresponding to `ref`.\n\n - `refs_heads_master` is now just `target`; i.e., what the\n symref points at. This term also matches what is in\n the symlink(2) manpage (at least on Linux).\n\n - the buffer to hold the symref file's contents was simply\n called `ref`. It's now `buf` (admittedly also generic,\n but at least not actively introducing confusion with the\n other variable holding the refname).\n\nSigned-off-by: Jeff King \n---\n refs.h | 2 +-\n refs/files-backend.c | 41 ++++++++++++++++++++---------------------\n 2 files changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 7a04077..3c3da29 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -292,7 +292,7 @@ extern char *shorten_unambiguous_ref(const char *refname, int strict);\n /** rename ref, return 0 on success **/\n extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);\n \n-extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);\n+extern int create_symref(const char *refname, const char *target, const char *logmsg);\n \n enum action_on_err {\n \tUPDATE_REFS_MSG_ON_ERR,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c648b5e..d6f9123 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2811,58 +2811,57 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-int create_symref(const char *ref_target, const char *refs_heads_master,\n-\t\t const char *logmsg)\n+int create_symref(const char *refname, const char *target, const char *logmsg)\n {\n \tchar *lockpath = NULL;\n-\tchar ref[1000];\n+\tchar buf[1000];\n \tint fd, len, written;\n-\tchar *git_HEAD = git_pathdup(\"%s\", ref_target);\n+\tchar *ref_path = git_pathdup(\"%s\", refname);\n \tunsigned char old_sha1[20], new_sha1[20];\n \tstruct strbuf err = STRBUF_INIT;\n \n-\tif (logmsg && read_ref(ref_target, old_sha1))\n+\tif (logmsg && read_ref(refname, old_sha1))\n \t\thashclr(old_sha1);\n \n-\tif (safe_create_leading_directories(git_HEAD) < 0)\n-\t\treturn error(\"unable to create directory for %s\", git_HEAD);\n+\tif (safe_create_leading_directories(ref_path) < 0)\n+\t\treturn error(\"unable to create directory for %s\", ref_path);\n \n #ifndef NO_SYMLINK_HEAD\n \tif (prefer_symlink_refs) {\n-\t\tunlink(git_HEAD);\n-\t\tif (!symlink(refs_heads_master, git_HEAD))\n+\t\tunlink(ref_path);\n+\t\tif (!symlink(target, ref_path))\n \t\t\tgoto done;\n \t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n \t}\n #endif\n \n-\tlen = snprintf(ref, sizeof(ref), \"ref: %s\\n\", refs_heads_master);\n-\tif (sizeof(ref) <= len) {\n-\t\terror(\"refname too long: %s\", refs_heads_master);\n+\tlen = snprintf(buf, sizeof(buf), \"ref: %s\\n\", target);\n+\tif (sizeof(buf) <= len) {\n+\t\terror(\"refname too long: %s\", target);\n \t\tgoto error_free_return;\n \t}\n-\tlockpath = mkpathdup(\"%s.lock\", git_HEAD);\n+\tlockpath = mkpathdup(\"%s.lock\", ref_path);\n \tfd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \tif (fd < 0) {\n \t\terror(\"Unable to open %s for writing\", lockpath);\n \t\tgoto error_free_return;\n \t}\n-\twritten = write_in_full(fd, ref, len);\n+\twritten = write_in_full(fd, buf, len);\n \tif (close(fd) != 0 || written != len) {\n \t\terror(\"Unable to write to %s\", lockpath);\n \t\tgoto error_unlink_return;\n \t}\n-\tif (rename(lockpath, git_HEAD) < 0) {\n-\t\terror(\"Unable to create %s\", git_HEAD);\n+\tif (rename(lockpath, ref_path) < 0) {\n+\t\terror(\"Unable to create %s\", ref_path);\n \t\tgoto error_unlink_return;\n \t}\n-\tif (adjust_shared_perm(git_HEAD)) {\n+\tif (adjust_shared_perm(ref_path)) {\n \t\terror(\"Unable to fix permissions on %s\", lockpath);\n \terror_unlink_return:\n \t\tunlink_or_warn(lockpath);\n \terror_free_return:\n \t\tfree(lockpath);\n-\t\tfree(git_HEAD);\n+\t\tfree(ref_path);\n \t\treturn -1;\n \t}\n \tfree(lockpath);\n@@ -2870,13 +2869,13 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n #ifndef NO_SYMLINK_HEAD\n \tdone:\n #endif\n-\tif (logmsg && !read_ref(refs_heads_master, new_sha1) &&\n-\t\tlog_ref_write(ref_target, old_sha1, new_sha1, logmsg, 0, &err)) {\n+\tif (logmsg && !read_ref(target, new_sha1) &&\n+\t\tlog_ref_write(refname, old_sha1, new_sha1, logmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n \n-\tfree(git_HEAD);\n+\tfree(ref_path);\n \treturn 0;\n }\n \n-- \n2.7.0.rc2.368.g1cbb535\n"},{"id":"275073","messageId":"20151229055701.GB17012@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151229055558.GA12848@sigill.intra.peff.net","subject":"[PATCH 2/3] create_symref: use existing ref-lock code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:57:01Z","receivedAt":"2015-12-29T05:57:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The create_symref() function predates the existence of\n\"struct lock_file\", let alone the more recent \"struct\nref_lock\". Instead, it just does its own manual dot-locking.\nBesides being more code, this has a few downsides:\n\n - if git is interrupted while holding the lock, we don't\n clean up the lockfile\n\n - we don't do the usual directory/filename conflict check.\n So you can sometimes create a symref \"refs/heads/foo/bar\",\n even if \"refs/heads/foo\" exists (namely, if the refs are\n packed and we do not hit the d/f conflict in the\n filesystem).\n\nThis patch refactors create_symref() to use the \"struct\nref_lock\" interface, which handles both of these things.\nThere are a few bonus cleanups that come along with it:\n\n - we leaked ref_path in some error cases\n\n - the symref contents were stored in a fixed-size buffer,\n putting an artificial (albeit large) limitation on the\n length of the refname. We now write through fprintf, and\n handle refnames of any size.\n\n - we called adjust_shared_perm only after the file was\n renamed into place, creating a potential race with\n readers in a shared repository. The lockfile code now\n handles this when creating the lockfile, making it\n atomic.\n\n - the legacy prefer_symlink_refs path did not do any\n locking at all. Admittedly, it is not atomic from a\n reader's perspective (as it unlinks and re-creates the\n symlink to overwrite), but at least it cannot conflict\n with other writers now.\n\n - the result of this patch is hopefully more readable. It\n eliminates three goto labels. Two were for error checking\n that is now simplified, and the third was to reach shared\n code that has been pulled into its own function.\n\nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 109 ++++++++++++++++++++++++------------------------\n t/t1401-symbolic-ref.sh | 8 ++++\n 2 files changed, 62 insertions(+), 55 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d6f9123..3d1994d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2811,74 +2811,73 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-int create_symref(const char *refname, const char *target, const char *logmsg)\n+static int create_ref_symlink(struct ref_lock *lock, const char *target)\n {\n-\tchar *lockpath = NULL;\n-\tchar buf[1000];\n-\tint fd, len, written;\n-\tchar *ref_path = git_pathdup(\"%s\", refname);\n-\tunsigned char old_sha1[20], new_sha1[20];\n-\tstruct strbuf err = STRBUF_INIT;\n-\n-\tif (logmsg && read_ref(refname, old_sha1))\n-\t\thashclr(old_sha1);\n-\n-\tif (safe_create_leading_directories(ref_path) < 0)\n-\t\treturn error(\"unable to create directory for %s\", ref_path);\n-\n+\tint ret = -1;\n #ifndef NO_SYMLINK_HEAD\n-\tif (prefer_symlink_refs) {\n-\t\tunlink(ref_path);\n-\t\tif (!symlink(target, ref_path))\n-\t\t\tgoto done;\n+\tchar *ref_path = get_locked_file_path(lock->lk);\n+\tunlink(ref_path);\n+\tret = symlink(target, ref_path);\n+\tfree(ref_path);\n+\n+\tif (ret)\n \t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n-\t}\n #endif\n+\treturn ret;\n+}\n \n-\tlen = snprintf(buf, sizeof(buf), \"ref: %s\\n\", target);\n-\tif (sizeof(buf) <= len) {\n-\t\terror(\"refname too long: %s\", target);\n-\t\tgoto error_free_return;\n-\t}\n-\tlockpath = mkpathdup(\"%s.lock\", ref_path);\n-\tfd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n-\tif (fd < 0) {\n-\t\terror(\"Unable to open %s for writing\", lockpath);\n-\t\tgoto error_free_return;\n-\t}\n-\twritten = write_in_full(fd, buf, len);\n-\tif (close(fd) != 0 || written != len) {\n-\t\terror(\"Unable to write to %s\", lockpath);\n-\t\tgoto error_unlink_return;\n-\t}\n-\tif (rename(lockpath, ref_path) < 0) {\n-\t\terror(\"Unable to create %s\", ref_path);\n-\t\tgoto error_unlink_return;\n-\t}\n-\tif (adjust_shared_perm(ref_path)) {\n-\t\terror(\"Unable to fix permissions on %s\", lockpath);\n-\terror_unlink_return:\n-\t\tunlink_or_warn(lockpath);\n-\terror_free_return:\n-\t\tfree(lockpath);\n-\t\tfree(ref_path);\n-\t\treturn -1;\n-\t}\n-\tfree(lockpath);\n-\n-#ifndef NO_SYMLINK_HEAD\n-\tdone:\n-#endif\n+static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n+\t\t\t\t const char *target, const char *logmsg)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tunsigned char new_sha1[20];\n \tif (logmsg && !read_ref(target, new_sha1) &&\n-\t\tlog_ref_write(refname, old_sha1, new_sha1, logmsg, 0, &err)) {\n+\t log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n+}\n \n-\tfree(ref_path);\n+static int create_symref_locked(struct ref_lock *lock, const char *refname,\n+\t\t\t\tconst char *target, const char *logmsg)\n+{\n+\tif (prefer_symlink_refs && !create_ref_symlink(lock, target)) {\n+\t\tupdate_symref_reflog(lock, refname, target, logmsg);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!fdopen_lock_file(lock->lk, \"w\"))\n+\t\treturn error(\"unable to fdopen %s: %s\",\n+\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n+\n+\t/* no error check; commit_ref will check ferror */\n+\tfprintf(lock->lk->tempfile.fp, \"ref: %s\\n\", target);\n+\tif (commit_ref(lock) < 0)\n+\t\treturn error(\"unable to write symref for %s: %s\", refname,\n+\t\t\t strerror(errno));\n+\tupdate_symref_reflog(lock, refname, target, logmsg);\n \treturn 0;\n }\n \n+int create_symref(const char *refname, const char *target, const char *logmsg)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_lock *lock;\n+\tint ret;\n+\n+\tlock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,\n+\t\t\t\t &err);\n+\tif (!lock) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n+\tret = create_symref_locked(lock, refname, target, logmsg);\n+\tunlock_ref(lock);\n+\treturn ret;\n+}\n+\n int reflog_exists(const char *refname)\n {\n \tstruct stat st;\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 1f0dff3..5db876c 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -114,4 +114,12 @@ test_expect_success 'symbolic-ref writes reflog entry' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n+\tgit checkout -b df &&\n+\ttest_commit df &&\n+\ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df &&\n+\tgit pack-refs --all --prune &&\n+\ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n+'\n+\n test_done\n-- \n2.7.0.rc2.368.g1cbb535\n"},{"id":"275072","messageId":"20151229055725.GC17012@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151229055558.GA12848@sigill.intra.peff.net","subject":"[PATCH 3/3] create_symref: write reflog while holding lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T05:57:25Z","receivedAt":"2015-12-29T05:57:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally hold a lock on the matching ref while writing\nto its reflog; this prevents two simultaneous writers from\nclobbering each other's reflog lines (it does not even have\nto be two symref updates; because we don't hold the lock, we\ncould race with somebody writing to the pointed-to ref via\nHEAD, for example).\n\nWe can fix this by writing the reflog before we commit the\nlockfile. This runs the risk of writing the reflog but\nfailing the final rename(), but at least we now err on the\nsame side as the rest of the ref code.\n\nNoticed-by: Michael Haggerty \nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3d1994d..180c837 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2850,12 +2850,13 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,\n \t\treturn error(\"unable to fdopen %s: %s\",\n \t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n \n+\tupdate_symref_reflog(lock, refname, target, logmsg);\n+\n \t/* no error check; commit_ref will check ferror */\n \tfprintf(lock->lk->tempfile.fp, \"ref: %s\\n\", target);\n \tif (commit_ref(lock) < 0)\n \t\treturn error(\"unable to write symref for %s: %s\", refname,\n \t\t\t strerror(errno));\n-\tupdate_symref_reflog(lock, refname, target, logmsg);\n \treturn 0;\n }\n \n-- \n2.7.0.rc2.368.g1cbb535\n"},{"id":"275074","messageId":"20151229060055.GA17047@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151229055558.GA12848@sigill.intra.peff.net","subject":"[RFC/PATCH 4/3] create_symref: drop support for writing symbolic links","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T06:00:55Z","receivedAt":"2015-12-29T06:00:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I'm iffy on this one, just because it drops a user-visible feature with\nno deprecation. For the reasons given below, I doubt anybody is using it\nfor the original intended purpose, but maybe there is some crazy person\nwhose workflow revolves around core.preferSymlinkRefs.\n\nA conservative choice would probably be to issue a deprecation warning\nwhen we see it defined, wait a few versions, and then apply the patch\nbelow.\n\n-- >8 --\nLong ago, in 2fabd21 (Disable USE_SYMLINK_HEAD by default,\n2005-11-15), we switched git's default behavior to writing\ntext symrefs instead of symbolic links. Any scripts\naccustomed to looking directly at .git/HEAD were updated to\nuse `rev-parse` instead. The Linux kernel's setlocalversion\nscript was one, and it was fixed in 117a93d (kbuild: Use git\nin scripts/setlocalversion, 2006-01-04).\n\nHowever, the problem still happened when bisecting the\nkernel; pre-117a93d kernels would not build properly with a\nnewer git, because they wanted to look directly at HEAD. To\nsolve this, we added 9f0bb90 (core.prefersymlinkrefs: use\nsymlinks for .git/HEAD, 2006-05-02), which lets the user\nturn on the old behavior, theoretically letting you bisect\nolder kernel history.\n\nBut there are a few complications with this solution:\n\n - packed-refs means you are limited in what you can do\n with .git/HEAD. If it is a symlink, you may `readlink`\n it to see where it points, but you cannot necessarily\n `cat .git/HEAD` to get the sha1, as the pointed-to ref\n may be packed.\n\n In particular, this means that the pre-117a93d kbuild\n script would sometimes work and sometimes not.\n\n - These days, we bisect on a detached HEAD. So during\n bisection, .git/HEAD _is_ a regular file with the\n sha1, and it works to `cat` it, whether or not you set\n core.preferSymlinkRefs.\n\nSuch scripts will all be broken again if we move to\nalternate ref backends. They should have learned to use\n`rev-parse` long ago, and it is only bisecting ancient\nhistory that is a problem.\n\nNow that almost ten years have passed, it seems less likely\nthat developers will bisect so far back in history. And\nmoreover, this is but one of many possible problems\ndevelopers run into when trying to build versions. The\nstandard solution is to apply a \"fixup\" patch or other\nworkaround while test-building the project, and that would\nwork here, too.\n\nThis patch therefore drops core.preferSymlinkRefs\ncompletely. There are a few reasons to want to do so:\n\n 1. It drops some code that is probably exercised very\n rarely.\n\n 2. The symlink code is not up to the same standards as the\n rest of the ref code. In particular, it is racy with\n respect to simultaneous readers and writers.\n\n 3. If we want to eventually drop the symlink-reading code,\n this is a good first step to deprecating it.\n\nSigned-off-by: Jeff King \n---\n Documentation/config.txt | 6 ------\n cache.h | 1 -\n config.c | 5 -----\n contrib/completion/git-completion.bash | 1 -\n environment.c | 1 -\n refs/files-backend.c | 20 --------------------\n t/t7201-co.sh | 12 ------------\n t/t9903-bash-prompt.sh | 9 ---------\n 8 files changed, 55 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f617886..06a2f2a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -433,12 +433,6 @@ CIFS/Microsoft Windows.\n +\n False by default.\n \n-core.preferSymlinkRefs::\n-\tInstead of the default \"symref\" format for HEAD\n-\tand other symbolic reference files, use symbolic links.\n-\tThis is sometimes needed to work with old scripts that\n-\texpect HEAD to be a symbolic link.\n-\n core.bare::\n \tIf true this repository is assumed to be 'bare' and has no\n \tworking directory associated with it. If this is the case a\ndiff --git a/cache.h b/cache.h\nindex c63fcc1..5ff7df2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -625,7 +625,6 @@ extern int has_symlinks;\n extern int minimum_abbrev, default_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n-extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\ndiff --git a/config.c b/config.c\nindex 86a5eb2..f479eaa 100644\n--- a/config.c\n+++ b/config.c\n@@ -726,11 +726,6 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.prefersymlinkrefs\")) {\n-\t\tprefer_symlink_refs = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"core.logallrefupdates\")) {\n \t\tlog_all_ref_updates = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 6956807..31d517d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2046,7 +2046,6 @@ _git_config ()\n \t\tcore.packedGitLimit\n \t\tcore.packedGitWindowSize\n \t\tcore.pager\n-\t\tcore.preferSymlinkRefs\n \t\tcore.preloadindex\n \t\tcore.quotepath\n \t\tcore.repositoryFormatVersion\ndiff --git a/environment.c b/environment.c\nindex 2da7fe2..80a1c0c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -19,7 +19,6 @@ int has_symlinks = 1;\n int minimum_abbrev = 4, default_abbrev = 7;\n int ignore_case;\n int assume_unchanged;\n-int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 180c837..6e19953 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-static int create_ref_symlink(struct ref_lock *lock, const char *target)\n-{\n-\tint ret = -1;\n-#ifndef NO_SYMLINK_HEAD\n-\tchar *ref_path = get_locked_file_path(lock->lk);\n-\tunlink(ref_path);\n-\tret = symlink(target, ref_path);\n-\tfree(ref_path);\n-\n-\tif (ret)\n-\t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n-#endif\n-\treturn ret;\n-}\n-\n static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n \t\t\t\t const char *target, const char *logmsg)\n {\n@@ -2841,11 +2826,6 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n static int create_symref_locked(struct ref_lock *lock, const char *refname,\n \t\t\t\tconst char *target, const char *logmsg)\n {\n-\tif (prefer_symlink_refs && !create_ref_symlink(lock, target)) {\n-\t\tupdate_symref_reflog(lock, refname, target, logmsg);\n-\t\treturn 0;\n-\t}\n-\n \tif (!fdopen_lock_file(lock->lk, \"w\"))\n \t\treturn error(\"unable to fdopen %s: %s\",\n \t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 8859236..715dc00 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -427,18 +427,6 @@ test_expect_success 'checkout w/--track from tag fails' '\n test \"z$(git rev-parse master^0)\" = \"z$(git rev-parse HEAD)\"\n '\n \n-test_expect_success 'detach a symbolic link HEAD' '\n- git checkout master &&\n- git config --bool core.prefersymlinkrefs yes &&\n- git checkout side &&\n- git checkout master &&\n- it=$(git symbolic-ref HEAD) &&\n- test \"z$it\" = zrefs/heads/master &&\n- here=$(git rev-parse --verify refs/heads/master) &&\n- git checkout side^ &&\n- test \"z$(git rev-parse --verify refs/heads/master)\" = \"z$here\"\n-'\n-\n test_expect_success \\\n 'checkout with --track fakes a sensible -b ' '\n git config remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/*\" &&\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex af82049..25066f35 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -46,15 +46,6 @@ test_expect_success 'prompt - branch name' '\n \ttest_cmp expected \"$actual\"\n '\n \n-test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '\n-\tprintf \" (master)\" >expected &&\n-\ttest_when_finished \"git checkout master\" &&\n-\ttest_config core.preferSymlinkRefs true &&\n-\tgit checkout master &&\n-\t__git_ps1 >\"$actual\" &&\n-\ttest_cmp expected \"$actual\"\n-'\n-\n test_expect_success 'prompt - unborn branch' '\n \tprintf \" (unborn)\" >expected &&\n \tgit checkout --orphan unborn &&\n-- \n2.7.0.rc2.368.g1cbb535\n"},{"id":"275075","messageId":"20151229060358.GB12848@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151229060055.GA17047@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T06:03:58Z","receivedAt":"2015-12-29T06:03:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 29, 2015 at 01:00:55AM -0500, Jeff King wrote:\n\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,\n> \treturn 0;\n> }\n> \n> -static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> -{\n> -\tint ret = -1;\n> -#ifndef NO_SYMLINK_HEAD\n> -\tchar *ref_path = get_locked_file_path(lock->lk);\n> -\tunlink(ref_path);\n> -\tret = symlink(target, ref_path);\n> -\tfree(ref_path);\n> -\n> -\tif (ret)\n> -\t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n> -#endif\n\nI forgot the build-time knob, which becomes a noop after this patch. If\nwe choose to apply this patch, we'd want to squash this in, too:\n\ndiff --git a/Makefile b/Makefile\nindex fd19b54..05ffd60 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -113,9 +113,6 @@ all::\n #\n # Define NO_SYS_SELECT_H if you don't have sys/select.h.\n #\n-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.\n-# Enable it on Windows. By default, symrefs are still used.\n-#\n # Define NO_SVN_TESTS if you want to skip time-consuming SVN interoperability\n # tests. These tests take up a significant amount of the total test time\n # but are not needed unless you plan to talk to SVN repos.\n@@ -1200,9 +1197,6 @@ ifdef FREAD_READS_DIRECTORIES\n \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n \tCOMPAT_OBJS += compat/fopen.o\n endif\n-ifdef NO_SYMLINK_HEAD\n-\tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n-endif\n ifdef GETTEXT_POISON\n \tBASIC_CFLAGS += -DGETTEXT_POISON\n endif\ndiff --git a/config.mak.uname b/config.mak.uname\nindex f34dcaa..9b77e2c 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -169,7 +169,6 @@ ifeq ($(uname_O),Cygwin)\n \t\tNO_STRCASESTR = YesPlease\n \t\tNO_MEMMEM = YesPlease\n \t\tNO_MKSTEMPS = YesPlease\n-\t\tNO_SYMLINK_HEAD = YesPlease\n \t\tNO_IPV6 = YesPlease\n \t\tOLD_ICONV = UnfortunatelyYes\n \t\t# There are conflicting reports about this.\n@@ -338,7 +337,6 @@ ifeq ($(uname_S),Windows)\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n-\tNO_SYMLINK_HEAD = YesPlease\n \tNO_IPV6 = YesPlease\n \tNO_UNIX_SOCKETS = YesPlease\n \tNO_SETENV = YesPlease\n@@ -491,7 +489,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n-\tNO_SYMLINK_HEAD = YesPlease\n \tNO_UNIX_SOCKETS = YesPlease\n \tNO_SETENV = YesPlease\n \tNO_STRCASESTR = YesPlease\ndiff --git a/configure.ac b/configure.ac\nindex 89e2590..cad5418 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -1096,9 +1096,6 @@ GIT_CONF_SUBST([HAVE_BSD_SYSCTL])\n # Define USE_PIC if you need the main git objects to be built with -fPIC\n # in order to build and link perl/Git.so. x86-64 seems to need this.\n #\n-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.\n-# Enable it on Windows. By default, symrefs are still used.\n-#\n # Define NO_PTHREADS if we do not have pthreads.\n #\n # Define PTHREAD_LIBS to the linker flag used for Pthread support.\n"},{"id":"275111","messageId":"56824362.30903@alum.mit.edu","threadId":"41044","inReplyTo":"20151229055558.GA12848@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] improve symbolic-ref robustness","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-12-29T08:25:06Z","receivedAt":"2015-12-29T08:25:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/29/2015 06:55 AM, Jeff King wrote:\n> On Sun, Dec 20, 2015 at 02:26:37AM -0500, Jeff King wrote:\n> \n>> I noticed that an interrupt \"git symbolic-ref\" will not clean up\n>> \"HEAD.lock\". So I started this series as an attempt to convert\n>> create_symref() to \"struct lock_file\" to get the usual tempfile cleanup.\n> \n> Here's version 2, based on comments from Michael. The first two patches\n> were picked out separately for jk/symbolic-ref-maint, so I've dropped\n> them here (so 1+2 here are the original 3+4).\n> \n> The other differences from v1 are:\n> \n> - use \"refname\" instead of \"ref\" to match surrounding code\n> \n> - drop adjust_shared_perm, as lockfile does it for us\n> \n> - adjust reflog writing order (done in a new patch)\n> \n> The patches are:\n> \n> [1/3]: create_symref: modernize variable names\n> [2/3]: create_symref: use existing ref-lock code\n> [3/3]: create_symref: write reflog while holding lock\n\nThanks, Peff. The whole series is\n\nReviewed-by: Michael Haggerty \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"275142","messageId":"xmqqbn99hzrv.fsf@gitster.mtv.corp.google.com","threadId":"41044","inReplyTo":"20151229060055.GA17047@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T18:32:04Z","receivedAt":"2015-12-29T18:32:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> A conservative choice would probably be to issue a deprecation warning\n> when we see it defined, wait a few versions, and then apply the patch\n> below.\n\nI agree with the analysis below. And I agree that in the ideal\nworld, it would have been better not to add \"prefer symlink refs\"\nconfiguration in the first place. But we do not live in the ideal\nworld, and we already have it, so deprecation would need the usual\nmulti-step process.\n\nThanks.\n\n> -- >8 --\n> Long ago, in 2fabd21 (Disable USE_SYMLINK_HEAD by default,\n> 2005-11-15), we switched git's default behavior to writing\n> text symrefs instead of symbolic links. Any scripts\n> accustomed to looking directly at .git/HEAD were updated to\n> use `rev-parse` instead. The Linux kernel's setlocalversion\n> script was one, and it was fixed in 117a93d (kbuild: Use git\n> in scripts/setlocalversion, 2006-01-04).\n>\n> However, the problem still happened when bisecting the\n> kernel; pre-117a93d kernels would not build properly with a\n> newer git, because they wanted to look directly at HEAD. To\n> solve this, we added 9f0bb90 (core.prefersymlinkrefs: use\n> symlinks for .git/HEAD, 2006-05-02), which lets the user\n> turn on the old behavior, theoretically letting you bisect\n> older kernel history.\n>\n> But there are a few complications with this solution:\n>\n> - packed-refs means you are limited in what you can do\n> with .git/HEAD. If it is a symlink, you may `readlink`\n> it to see where it points, but you cannot necessarily\n> `cat .git/HEAD` to get the sha1, as the pointed-to ref\n> may be packed.\n>\n> In particular, this means that the pre-117a93d kbuild\n> script would sometimes work and sometimes not.\n>\n> - These days, we bisect on a detached HEAD. So during\n> bisection, .git/HEAD _is_ a regular file with the\n> sha1, and it works to `cat` it, whether or not you set\n> core.preferSymlinkRefs.\n>\n> Such scripts will all be broken again if we move to\n> alternate ref backends. They should have learned to use\n> `rev-parse` long ago, and it is only bisecting ancient\n> history that is a problem.\n>\n> Now that almost ten years have passed, it seems less likely\n> that developers will bisect so far back in history. And\n> moreover, this is but one of many possible problems\n> developers run into when trying to build versions. The\n> standard solution is to apply a \"fixup\" patch or other\n> workaround while test-building the project, and that would\n> work here, too.\n>\n> This patch therefore drops core.preferSymlinkRefs\n> completely. There are a few reasons to want to do so:\n>\n> 1. It drops some code that is probably exercised very\n> rarely.\n>\n> 2. The symlink code is not up to the same standards as the\n> rest of the ref code. In particular, it is racy with\n> respect to simultaneous readers and writers.\n>\n> 3. If we want to eventually drop the symlink-reading code,\n> this is a good first step to deprecating it.\n>\n> Signed-off-by: Jeff King \n> ---\n> Documentation/config.txt | 6 ------\n> cache.h | 1 -\n> config.c | 5 -----\n> contrib/completion/git-completion.bash | 1 -\n> environment.c | 1 -\n> refs/files-backend.c | 20 --------------------\n> t/t7201-co.sh | 12 ------------\n> t/t9903-bash-prompt.sh | 9 ---------\n> 8 files changed, 55 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index f617886..06a2f2a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -433,12 +433,6 @@ CIFS/Microsoft Windows.\n> +\n> False by default.\n> \n> -core.preferSymlinkRefs::\n> -\tInstead of the default \"symref\" format for HEAD\n> -\tand other symbolic reference files, use symbolic links.\n> -\tThis is sometimes needed to work with old scripts that\n> -\texpect HEAD to be a symbolic link.\n> -\n> core.bare::\n> \tIf true this repository is assumed to be 'bare' and has no\n> \tworking directory associated with it. If this is the case a\n> diff --git a/cache.h b/cache.h\n> index c63fcc1..5ff7df2 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -625,7 +625,6 @@ extern int has_symlinks;\n> extern int minimum_abbrev, default_abbrev;\n> extern int ignore_case;\n> extern int assume_unchanged;\n> -extern int prefer_symlink_refs;\n> extern int log_all_ref_updates;\n> extern int warn_ambiguous_refs;\n> extern int warn_on_object_refname_ambiguity;\n> diff --git a/config.c b/config.c\n> index 86a5eb2..f479eaa 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -726,11 +726,6 @@ static int git_default_core_config(const char *var, const char *value)\n> \t\treturn 0;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.prefersymlinkrefs\")) {\n> -\t\tprefer_symlink_refs = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> \tif (!strcmp(var, \"core.logallrefupdates\")) {\n> \t\tlog_all_ref_updates = git_config_bool(var, value);\n> \t\treturn 0;\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 6956807..31d517d 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2046,7 +2046,6 @@ _git_config ()\n> \t\tcore.packedGitLimit\n> \t\tcore.packedGitWindowSize\n> \t\tcore.pager\n> -\t\tcore.preferSymlinkRefs\n> \t\tcore.preloadindex\n> \t\tcore.quotepath\n> \t\tcore.repositoryFormatVersion\n> diff --git a/environment.c b/environment.c\n> index 2da7fe2..80a1c0c 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -19,7 +19,6 @@ int has_symlinks = 1;\n> int minimum_abbrev = 4, default_abbrev = 7;\n> int ignore_case;\n> int assume_unchanged;\n> -int prefer_symlink_refs;\n> int is_bare_repository_cfg = -1; /* unspecified */\n> int log_all_ref_updates = -1; /* unspecified */\n> int warn_ambiguous_refs = 1;\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 180c837..6e19953 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,\n> \treturn 0;\n> }\n> \n> -static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> -{\n> -\tint ret = -1;\n> -#ifndef NO_SYMLINK_HEAD\n> -\tchar *ref_path = get_locked_file_path(lock->lk);\n> -\tunlink(ref_path);\n> -\tret = symlink(target, ref_path);\n> -\tfree(ref_path);\n> -\n> -\tif (ret)\n> -\t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n> -#endif\n> -\treturn ret;\n> -}\n> -\n> static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n> \t\t\t\t const char *target, const char *logmsg)\n> {\n> @@ -2841,11 +2826,6 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n> static int create_symref_locked(struct ref_lock *lock, const char *refname,\n> \t\t\t\tconst char *target, const char *logmsg)\n> {\n> -\tif (prefer_symlink_refs && !create_ref_symlink(lock, target)) {\n> -\t\tupdate_symref_reflog(lock, refname, target, logmsg);\n> -\t\treturn 0;\n> -\t}\n> -\n> \tif (!fdopen_lock_file(lock->lk, \"w\"))\n> \t\treturn error(\"unable to fdopen %s: %s\",\n> \t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n> diff --git a/t/t7201-co.sh b/t/t7201-co.sh\n> index 8859236..715dc00 100755\n> --- a/t/t7201-co.sh\n> +++ b/t/t7201-co.sh\n> @@ -427,18 +427,6 @@ test_expect_success 'checkout w/--track from tag fails' '\n> test \"z$(git rev-parse master^0)\" = \"z$(git rev-parse HEAD)\"\n> '\n> \n> -test_expect_success 'detach a symbolic link HEAD' '\n> - git checkout master &&\n> - git config --bool core.prefersymlinkrefs yes &&\n> - git checkout side &&\n> - git checkout master &&\n> - it=$(git symbolic-ref HEAD) &&\n> - test \"z$it\" = zrefs/heads/master &&\n> - here=$(git rev-parse --verify refs/heads/master) &&\n> - git checkout side^ &&\n> - test \"z$(git rev-parse --verify refs/heads/master)\" = \"z$here\"\n> -'\n> -\n> test_expect_success \\\n> 'checkout with --track fakes a sensible -b ' '\n> git config remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/*\" &&\n> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n> index af82049..25066f35 100755\n> --- a/t/t9903-bash-prompt.sh\n> +++ b/t/t9903-bash-prompt.sh\n> @@ -46,15 +46,6 @@ test_expect_success 'prompt - branch name' '\n> \ttest_cmp expected \"$actual\"\n> '\n> \n> -test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '\n> -\tprintf \" (master)\" >expected &&\n> -\ttest_when_finished \"git checkout master\" &&\n> -\ttest_config core.preferSymlinkRefs true &&\n> -\tgit checkout master &&\n> -\t__git_ps1 >\"$actual\" &&\n> -\ttest_cmp expected \"$actual\"\n> -'\n> -\n> test_expect_success 'prompt - unborn branch' '\n> \tprintf \" (unborn)\" >expected &&\n> \tgit checkout --orphan unborn &&\n"},{"id":"275143","messageId":"xmqq7fjxhzms.fsf@gitster.mtv.corp.google.com","threadId":"41044","inReplyTo":"56824362.30903@alum.mit.edu","subject":"Re: [PATCH v2 0/3] improve symbolic-ref robustness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T18:35:07Z","receivedAt":"2015-12-29T18:35:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 12/29/2015 06:55 AM, Jeff King wrote:\n>\n>> The patches are:\n>> \n>> [1/3]: create_symref: modernize variable names\n>> [2/3]: create_symref: use existing ref-lock code\n>> [3/3]: create_symref: write reflog while holding lock\n>\n> Thanks, Peff. The whole series is\n>\n> Reviewed-by: Michael Haggerty \n>\n> Michael\n\nThanks, both.\n"},{"id":"275151","messageId":"xmqqpoxpgd7p.fsf@gitster.mtv.corp.google.com","threadId":"41044","inReplyTo":"20151229055558.GA12848@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] improve symbolic-ref robustness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T21:24:42Z","receivedAt":"2015-12-29T21:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Dec 20, 2015 at 02:26:37AM -0500, Jeff King wrote:\n>\n>> I noticed that an interrupt \"git symbolic-ref\" will not clean up\n>> \"HEAD.lock\". So I started this series as an attempt to convert\n>> create_symref() to \"struct lock_file\" to get the usual tempfile cleanup.\n>\n> Here's version 2, based on comments from Michael. The first two patches\n> were picked out separately for jk/symbolic-ref-maint, so I've dropped\n> them here (so 1+2 here are the original 3+4).\n>\n> The other differences from v1 are:\n>\n> - use \"refname\" instead of \"ref\" to match surrounding code\n>\n> - drop adjust_shared_perm, as lockfile does it for us\n>\n> - adjust reflog writing order (done in a new patch)\n>\n> The patches are:\n>\n> [1/3]: create_symref: modernize variable names\n> [2/3]: create_symref: use existing ref-lock code\n> [3/3]: create_symref: write reflog while holding lock\n\nThis is queued as an early part of 'pu', and some refactoring in\nDavid's refs-backend-lmdb topic conflicts with it when merged to\n'pu'. I think I resolved the conflicts correctly, but please double\ncheck the result when I push it out later today.\n\nThanks.\n"},{"id":"275165","messageId":"20151230065343.GA26964@sigill.intra.peff.net","threadId":"41044","inReplyTo":"xmqqbn99hzrv.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-30T06:53:44Z","receivedAt":"2015-12-30T06:53:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 29, 2015 at 10:32:04AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > A conservative choice would probably be to issue a deprecation warning\n> > when we see it defined, wait a few versions, and then apply the patch\n> > below.\n> \n> I agree with the analysis below. And I agree that in the ideal\n> world, it would have been better not to add \"prefer symlink refs\"\n> configuration in the first place. But we do not live in the ideal\n> world, and we already have it, so deprecation would need the usual\n> multi-step process.\n\nHere's the first step of that multi-step process. The commit message\nwill look familiar, as the rationale for deprecating is the same as for\ndropping.\n\nThis is on top of what I posted earlier (it obviously could be\nindependent, but I assume we're planning to merge the other bits, and I\ndon't mind holding this topic hostage for a little while to save some\nannoying merging).\n\n-- >8 --\nSubject: [PATCH] create_symref: deprecate support for writing symbolic links\n\nLong ago, in 2fabd21 (Disable USE_SYMLINK_HEAD by default,\n2005-11-15), we switched git's default behavior to writing\ntext symrefs instead of symbolic links. Any scripts\naccustomed to looking directly at .git/HEAD were updated to\nuse `rev-parse` instead. The Linux kernel's setlocalversion\nscript was one, and it was fixed in 117a93d (kbuild: Use git\nin scripts/setlocalversion, 2006-01-04).\n\nHowever, the problem still happened when bisecting the\nkernel; pre-117a93d would not build properly with a newer\ngit, because they wanted to look directly at HEAD. To solve\nthis, we added 9f0bb90 (core.prefersymlinkrefs: use symlinks\nfor .git/HEAD, 2006-05-02), which lets the user turn on the\nold behavior, theoretically letting you bisect older kernel\nhistory.\n\nBut there are a few complications with this solution:\n\n - packed-refs means you are limited in what you can do\n with .git/HEAD. If it is a symlink, you may `readlink`\n it to see where it points, but you cannot necessarily\n `cat .git/HEAD` to get the sha1, as the pointed-to ref\n may be packed.\n\n In particular, this means that the pre-117a93d kbuild\n script would sometimes work and sometimes not.\n\n - These days, we bisect on a detached HEAD. So during\n bisection, .git/HEAD _is_ a regular file with the\n sha1, and it works to `cat` it, whether or not you set\n core.preferSymlinkRefs.\n\nSuch scripts will all be broken again if we move to\nalternate ref backends. They should have learned to use\n`rev-parse` long ago, and it is only bisecting ancient\nhistory that is a problem.\n\nNow that almost ten years have passed, it seems less likely\nthat developers will bisect so far back in history. And\nmoreover, this is but one of many possible problems\ndevelopers run into when trying to build versions. The\nstandard solution is to apply a \"fixup\" patch or other\nworkaround while test-building the project, and that would\nwork here, too.\n\nThis patch therefore deprecates core.preferSymlinkRefs\ncompletely. There are a few reasons to want to do so:\n\n 1. It drops some code that is probably exercised very\n rarely.\n\n 2. The symlink code is not up to the same standards as the\n rest of the ref code. In particular, it is racy with\n respect to simultaneous readers and writers.\n\n 3. If we want to eventually drop the symlink-reading code,\n this is a good first step to deprecating it.\n\nWe print a deprecation warning anytime a symlink is created\nusing this code. That prevents us from spamming the user\nwith multiple warnings from read-only operations, but\ncatches major operations like init, clone, and checkout.\n\nSigned-off-by: Jeff King \n---\nThere's no advice.* safety valve here. The solution is \"fix your\nscript\", and the last-ditch effort is \"come to the mailing list and tell\nus why it would be a bad thing to remove the feature\".\n\n refs/files-backend.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 180c837..22b7c11 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2860,12 +2860,27 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,\n \treturn 0;\n }\n \n+static const char symlink_deprecation_warning[] =\n+\"The core.preferSymlinkRefs configuration option has been\\n\"\n+\"deprecated and will be removed in a future version of Git. If your\\n\"\n+\"workflow or script depends on '.git/HEAD' being a symbolic link,\\n\"\n+\"it should be adjusted to use:\\n\"\n+\"\\n\"\n+\" git rev-parse HEAD\\n\"\n+\"\\n\"\n+\" git rev-parse --symbolic-full-name HEAD\\n\"\n+\"\\n\"\n+\"to get the sha1 or branch name, respectively.\";\n+\n int create_symref(const char *refname, const char *target, const char *logmsg)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_lock *lock;\n \tint ret;\n \n+\tif (prefer_symlink_refs)\n+\t\twarning(\"%s\", symlink_deprecation_warning);\n+\n \tlock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,\n \t\t\t\t &err);\n \tif (!lock) {\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275166","messageId":"20151230065614.GA29782@sigill.intra.peff.net","threadId":"41044","inReplyTo":"20151230065343.GA26964@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-30T06:56:15Z","receivedAt":"2015-12-30T06:56:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 30, 2015 at 01:53:43AM -0500, Jeff King wrote:\n\n> On Tue, Dec 29, 2015 at 10:32:04AM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > A conservative choice would probably be to issue a deprecation warning\n> > > when we see it defined, wait a few versions, and then apply the patch\n> > > below.\n> > \n> > I agree with the analysis below. And I agree that in the ideal\n> > world, it would have been better not to add \"prefer symlink refs\"\n> > configuration in the first place. But we do not live in the ideal\n> > world, and we already have it, so deprecation would need the usual\n> > multi-step process.\n> \n> Here's the first step of that multi-step process. The commit message\n> will look familiar, as the rationale for deprecating is the same as for\n> dropping.\n\nAnd here's the second step, which would obviously want to wait a while\nbefore being merged.\n\nA third possible step would be removing the reading side from\nresolve_ref(). But that would want to wait a few more cycles past this\none. And I'm hesitant to do it while the ref code is in flux. It will be\neasier to just write the patch later on. :)\n\n-- >8 --\nSubject: [PATCH] create_symref: drop support for writing symbolic links\n\nThe parent commit deprecated the writing of symbolic links\nfor symrefs. Now that some time has passed, we can follow\nthrough by dropping the code.\n\nSigned-off-by: Jeff King \n---\n Documentation/config.txt | 6 ------\n Makefile | 6 ------\n cache.h | 1 -\n config.c | 5 -----\n config.mak.uname | 3 ---\n configure.ac | 3 ---\n contrib/completion/git-completion.bash | 1 -\n environment.c | 1 -\n refs/files-backend.c | 35 ----------------------------------\n t/t7201-co.sh | 12 ------------\n t/t9903-bash-prompt.sh | 9 ---------\n 11 files changed, 82 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f617886..06a2f2a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -433,12 +433,6 @@ CIFS/Microsoft Windows.\n +\n False by default.\n \n-core.preferSymlinkRefs::\n-\tInstead of the default \"symref\" format for HEAD\n-\tand other symbolic reference files, use symbolic links.\n-\tThis is sometimes needed to work with old scripts that\n-\texpect HEAD to be a symbolic link.\n-\n core.bare::\n \tIf true this repository is assumed to be 'bare' and has no\n \tworking directory associated with it. If this is the case a\ndiff --git a/Makefile b/Makefile\nindex fd19b54..05ffd60 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -113,9 +113,6 @@ all::\n #\n # Define NO_SYS_SELECT_H if you don't have sys/select.h.\n #\n-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.\n-# Enable it on Windows. By default, symrefs are still used.\n-#\n # Define NO_SVN_TESTS if you want to skip time-consuming SVN interoperability\n # tests. These tests take up a significant amount of the total test time\n # but are not needed unless you plan to talk to SVN repos.\n@@ -1200,9 +1197,6 @@ ifdef FREAD_READS_DIRECTORIES\n \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n \tCOMPAT_OBJS += compat/fopen.o\n endif\n-ifdef NO_SYMLINK_HEAD\n-\tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n-endif\n ifdef GETTEXT_POISON\n \tBASIC_CFLAGS += -DGETTEXT_POISON\n endif\ndiff --git a/cache.h b/cache.h\nindex c63fcc1..5ff7df2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -625,7 +625,6 @@ extern int has_symlinks;\n extern int minimum_abbrev, default_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n-extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\ndiff --git a/config.c b/config.c\nindex 86a5eb2..f479eaa 100644\n--- a/config.c\n+++ b/config.c\n@@ -726,11 +726,6 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.prefersymlinkrefs\")) {\n-\t\tprefer_symlink_refs = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"core.logallrefupdates\")) {\n \t\tlog_all_ref_updates = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/config.mak.uname b/config.mak.uname\nindex f34dcaa..9b77e2c 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -169,7 +169,6 @@ ifeq ($(uname_O),Cygwin)\n \t\tNO_STRCASESTR = YesPlease\n \t\tNO_MEMMEM = YesPlease\n \t\tNO_MKSTEMPS = YesPlease\n-\t\tNO_SYMLINK_HEAD = YesPlease\n \t\tNO_IPV6 = YesPlease\n \t\tOLD_ICONV = UnfortunatelyYes\n \t\t# There are conflicting reports about this.\n@@ -338,7 +337,6 @@ ifeq ($(uname_S),Windows)\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n-\tNO_SYMLINK_HEAD = YesPlease\n \tNO_IPV6 = YesPlease\n \tNO_UNIX_SOCKETS = YesPlease\n \tNO_SETENV = YesPlease\n@@ -491,7 +489,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n-\tNO_SYMLINK_HEAD = YesPlease\n \tNO_UNIX_SOCKETS = YesPlease\n \tNO_SETENV = YesPlease\n \tNO_STRCASESTR = YesPlease\ndiff --git a/configure.ac b/configure.ac\nindex 89e2590..cad5418 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -1096,9 +1096,6 @@ GIT_CONF_SUBST([HAVE_BSD_SYSCTL])\n # Define USE_PIC if you need the main git objects to be built with -fPIC\n # in order to build and link perl/Git.so. x86-64 seems to need this.\n #\n-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.\n-# Enable it on Windows. By default, symrefs are still used.\n-#\n # Define NO_PTHREADS if we do not have pthreads.\n #\n # Define PTHREAD_LIBS to the linker flag used for Pthread support.\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 6956807..31d517d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2046,7 +2046,6 @@ _git_config ()\n \t\tcore.packedGitLimit\n \t\tcore.packedGitWindowSize\n \t\tcore.pager\n-\t\tcore.preferSymlinkRefs\n \t\tcore.preloadindex\n \t\tcore.quotepath\n \t\tcore.repositoryFormatVersion\ndiff --git a/environment.c b/environment.c\nindex 2da7fe2..80a1c0c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -19,7 +19,6 @@ int has_symlinks = 1;\n int minimum_abbrev = 4, default_abbrev = 7;\n int ignore_case;\n int assume_unchanged;\n-int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 22b7c11..6e19953 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-static int create_ref_symlink(struct ref_lock *lock, const char *target)\n-{\n-\tint ret = -1;\n-#ifndef NO_SYMLINK_HEAD\n-\tchar *ref_path = get_locked_file_path(lock->lk);\n-\tunlink(ref_path);\n-\tret = symlink(target, ref_path);\n-\tfree(ref_path);\n-\n-\tif (ret)\n-\t\tfprintf(stderr, \"no symlink - falling back to symbolic ref\\n\");\n-#endif\n-\treturn ret;\n-}\n-\n static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n \t\t\t\t const char *target, const char *logmsg)\n {\n@@ -2841,11 +2826,6 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n static int create_symref_locked(struct ref_lock *lock, const char *refname,\n \t\t\t\tconst char *target, const char *logmsg)\n {\n-\tif (prefer_symlink_refs && !create_ref_symlink(lock, target)) {\n-\t\tupdate_symref_reflog(lock, refname, target, logmsg);\n-\t\treturn 0;\n-\t}\n-\n \tif (!fdopen_lock_file(lock->lk, \"w\"))\n \t\treturn error(\"unable to fdopen %s: %s\",\n \t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n@@ -2860,27 +2840,12 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,\n \treturn 0;\n }\n \n-static const char symlink_deprecation_warning[] =\n-\"The core.preferSymlinkRefs configuration option has been\\n\"\n-\"deprecated and will be removed in a future version of Git. If your\\n\"\n-\"workflow or script depends on '.git/HEAD' being a symbolic link,\\n\"\n-\"it should be adjusted to use:\\n\"\n-\"\\n\"\n-\" git rev-parse HEAD\\n\"\n-\"\\n\"\n-\" git rev-parse --symbolic-full-name HEAD\\n\"\n-\"\\n\"\n-\"to get the sha1 or branch name, respectively.\";\n-\n int create_symref(const char *refname, const char *target, const char *logmsg)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_lock *lock;\n \tint ret;\n \n-\tif (prefer_symlink_refs)\n-\t\twarning(\"%s\", symlink_deprecation_warning);\n-\n \tlock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,\n \t\t\t\t &err);\n \tif (!lock) {\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 8859236..715dc00 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -427,18 +427,6 @@ test_expect_success 'checkout w/--track from tag fails' '\n test \"z$(git rev-parse master^0)\" = \"z$(git rev-parse HEAD)\"\n '\n \n-test_expect_success 'detach a symbolic link HEAD' '\n- git checkout master &&\n- git config --bool core.prefersymlinkrefs yes &&\n- git checkout side &&\n- git checkout master &&\n- it=$(git symbolic-ref HEAD) &&\n- test \"z$it\" = zrefs/heads/master &&\n- here=$(git rev-parse --verify refs/heads/master) &&\n- git checkout side^ &&\n- test \"z$(git rev-parse --verify refs/heads/master)\" = \"z$here\"\n-'\n-\n test_expect_success \\\n 'checkout with --track fakes a sensible -b ' '\n git config remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/*\" &&\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex af82049..25066f35 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -46,15 +46,6 @@ test_expect_success 'prompt - branch name' '\n \ttest_cmp expected \"$actual\"\n '\n \n-test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '\n-\tprintf \" (master)\" >expected &&\n-\ttest_when_finished \"git checkout master\" &&\n-\ttest_config core.preferSymlinkRefs true &&\n-\tgit checkout master &&\n-\t__git_ps1 >\"$actual\" &&\n-\ttest_cmp expected \"$actual\"\n-'\n-\n test_expect_success 'prompt - unborn branch' '\n \tprintf \" (unborn)\" >expected &&\n \tgit checkout --orphan unborn &&\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275167","messageId":"20151230065728.GB26964@sigill.intra.peff.net","threadId":"41044","inReplyTo":"xmqqpoxpgd7p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/3] improve symbolic-ref robustness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-30T06:57:28Z","receivedAt":"2015-12-30T06:57:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 29, 2015 at 01:24:42PM -0800, Junio C Hamano wrote:\n\n> > The patches are:\n> >\n> > [1/3]: create_symref: modernize variable names\n> > [2/3]: create_symref: use existing ref-lock code\n> > [3/3]: create_symref: write reflog while holding lock\n> \n> This is queued as an early part of 'pu', and some refactoring in\n> David's refs-backend-lmdb topic conflicts with it when merged to\n> 'pu'. I think I resolved the conflicts correctly, but please double\n> check the result when I push it out later today.\n\nLooks sane to me. It seems like the change was just the renaming of the\nfunctions. We _could_ match the renaming of the static-local functions\n(e.g., create_symref_locked would become files_create_symref_locked),\nbut I don't think it's necessary for file-local functions.\n\n-Peff\n"}]}