[PATCH 0/4] improve symbolic-ref robustness

[PATCH 0/4] improve symbolic-ref robustness

[Jeff King]

I noticed that an interrupt "git symbolic-ref" will not clean up
"HEAD.lock". So I started this series as an attempt to convert
create_symref() to "struct lock_file" to get the usual tempfile cleanup.

But I found a few other points of interest. The biggest is that
git-symbolic-ref does not actually propagate errors in its exit code.
Whoops. That's fixed in the first patch, which could go separately to
"maint".

The rest of it is fairly dependent on master because of the
refs/files-backend.c code movement. I can backport it if we really want
it for maint.

  [1/4]: symbolic-ref: propagate error code from create_symref()
  [2/4]: t1401: test reflog creation for git-symbolic-ref
  [3/4]: create_symref: modernize variable names
  [4/4]: create_symref: use existing ref-lock code

-Peff

[PATCH 1/4] symbolic-ref: propagate error code from create_symref()

[Jeff King]

If create_symref() fails, git-symbolic-ref will still exit
with code 0, and our caller has no idea that the command did
nothing.

This appears to have been broken since the beginning of time
(e.g., it is not a regression where create_symref() stopped
calling die() or something similar).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/symbolic-ref.c  | 2 +-
 t/t1401-symbolic-ref.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index ce0fde7..9c29a64 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -67,7 +67,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		if (!strcmp(argv[0], "HEAD") &&
 		    !starts_with(argv[1], "refs/"))
 			die("Refusing to point HEAD outside of refs/");
-		create_symref(argv[0], argv[1], msg);
+		ret = !!create_symref(argv[0], argv[1], msg);
 		break;
 	default:
 		usage_with_options(git_symbolic_ref_usage, options);
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 20b022a..fbb4835 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -92,4 +92,10 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '
 	test_cmp expect actual
 '
 
+test_expect_success 'symbolic-ref reports failure in exit code' '
+	test_when_finished "rm -f .git/HEAD.lock" &&
+	>.git/HEAD.lock &&
+	test_must_fail git symbolic-ref HEAD refs/heads/whatever
+'
+
 test_done
-- 
2.7.0.rc1.350.g9acc0f4

[PATCH 2/4] t1401: test reflog creation for git-symbolic-ref

[Jeff King]

The current code writes a reflog entry whenever we update a
symbolic ref, but we never test that this is so. Let's add a
test to make sure upcoming refactoring doesn't cause a
regression.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1401-symbolic-ref.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index fbb4835..1f0dff3 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -98,4 +98,20 @@ test_expect_success 'symbolic-ref reports failure in exit code' '
 	test_must_fail git symbolic-ref HEAD refs/heads/whatever
 '
 
+test_expect_success 'symbolic-ref writes reflog entry' '
+	git checkout -b log1 &&
+	test_commit one &&
+	git checkout -b log2  &&
+	test_commit two &&
+	git checkout --orphan orphan &&
+	git symbolic-ref -m create HEAD refs/heads/log1 &&
+	git symbolic-ref -m update HEAD refs/heads/log2 &&
+	cat >expect <<-\EOF &&
+	update
+	create
+	EOF
+	git log --format=%gs -g >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.7.0.rc1.350.g9acc0f4

[PATCH 3/4] create_symref: modernize variable names

[Jeff King]

Once upon a time, create_symref() was used only to point
HEAD at a branch name, and the variable names reflect that
(e.g., calling the path git_HEAD). However, it is much more
generic these days (and has been for some time). Let's
update the variable names to make it easier to follow:

  - `ref_target` is now just `ref`, matching the declaration
    in `cache.h` (and hopefully making it clear that it is
    the symref itself, and not the target).

  - `git_HEAD` is now `ref_path`; the on-disk path
    corresponding to `ref`.

  - `refs_heads_master` is now just `target`; i.e., what the
    symref points at. This term also matches what is in
    the symlink(2) manpage (at least on Linux).

  - the buffer to hold the symref file's contents was simply
    called `ref`. It's now `buf` (admittedly also generic,
    but at least not actively introducing confusion with the
    other variable holding the refname).

Signed-off-by: Jeff King <peff@peff.net>
---
This could actually be squashed in with patch 4, as most of the changed
instances just go away. I did find the new names easier to work with,
though, so maybe they make the diff for that patch easier.

 refs.h               |  2 +-
 refs/files-backend.c | 41 ++++++++++++++++++++---------------------
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/refs.h b/refs.h
index 7a04077..96a25ad 100644
--- a/refs.h
+++ b/refs.h
@@ -292,7 +292,7 @@ extern char *shorten_unambiguous_ref(const char *refname, int strict);
 /** rename ref, return 0 on success **/
 extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);
 
-extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
+extern int create_symref(const char *ref, const char *target, const char *logmsg);
 
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c648b5e..6bfa139 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2811,58 +2811,57 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-int create_symref(const char *ref_target, const char *refs_heads_master,
-		  const char *logmsg)
+int create_symref(const char *ref, const char *target, const char *logmsg)
 {
 	char *lockpath = NULL;
-	char ref[1000];
+	char buf[1000];
 	int fd, len, written;
-	char *git_HEAD = git_pathdup("%s", ref_target);
+	char *ref_path = git_pathdup("%s", ref);
 	unsigned char old_sha1[20], new_sha1[20];
 	struct strbuf err = STRBUF_INIT;
 
-	if (logmsg && read_ref(ref_target, old_sha1))
+	if (logmsg && read_ref(ref, old_sha1))
 		hashclr(old_sha1);
 
-	if (safe_create_leading_directories(git_HEAD) < 0)
-		return error("unable to create directory for %s", git_HEAD);
+	if (safe_create_leading_directories(ref_path) < 0)
+		return error("unable to create directory for %s", ref_path);
 
 #ifndef NO_SYMLINK_HEAD
 	if (prefer_symlink_refs) {
-		unlink(git_HEAD);
-		if (!symlink(refs_heads_master, git_HEAD))
+		unlink(ref_path);
+		if (!symlink(target, ref_path))
 			goto done;
 		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
 	}
 #endif
 
-	len = snprintf(ref, sizeof(ref), "ref: %s\n", refs_heads_master);
-	if (sizeof(ref) <= len) {
-		error("refname too long: %s", refs_heads_master);
+	len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
+	if (sizeof(buf) <= len) {
+		error("refname too long: %s", target);
 		goto error_free_return;
 	}
-	lockpath = mkpathdup("%s.lock", git_HEAD);
+	lockpath = mkpathdup("%s.lock", ref_path);
 	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
 	if (fd < 0) {
 		error("Unable to open %s for writing", lockpath);
 		goto error_free_return;
 	}
-	written = write_in_full(fd, ref, len);
+	written = write_in_full(fd, buf, len);
 	if (close(fd) != 0 || written != len) {
 		error("Unable to write to %s", lockpath);
 		goto error_unlink_return;
 	}
-	if (rename(lockpath, git_HEAD) < 0) {
-		error("Unable to create %s", git_HEAD);
+	if (rename(lockpath, ref_path) < 0) {
+		error("Unable to create %s", ref_path);
 		goto error_unlink_return;
 	}
-	if (adjust_shared_perm(git_HEAD)) {
+	if (adjust_shared_perm(ref_path)) {
 		error("Unable to fix permissions on %s", lockpath);
 	error_unlink_return:
 		unlink_or_warn(lockpath);
 	error_free_return:
 		free(lockpath);
-		free(git_HEAD);
+		free(ref_path);
 		return -1;
 	}
 	free(lockpath);
@@ -2870,13 +2869,13 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 #ifndef NO_SYMLINK_HEAD
 	done:
 #endif
-	if (logmsg && !read_ref(refs_heads_master, new_sha1) &&
-		log_ref_write(ref_target, old_sha1, new_sha1, logmsg, 0, &err)) {
+	if (logmsg && !read_ref(target, new_sha1) &&
+		log_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {
 		error("%s", err.buf);
 		strbuf_release(&err);
 	}
 
-	free(git_HEAD);
+	free(ref_path);
 	return 0;
 }
 
-- 
2.7.0.rc1.350.g9acc0f4

[PATCH 4/4] create_symref: use existing ref-lock code

[Jeff King]

The create_symref() function predates the existence of
"struct lock_file", let alone the more recent "struct
ref_lock". Instead, it just does its own manual dot-locking.
Besides being more code, this has a few downsides:

 - if git is interrupted while holding the lock, we don't
   clean up the lockfile

 - we don't do the usual directory/filename conflict check.
   So you can sometimes create a symref "refs/heads/foo/bar",
   even if "refs/heads/foo" exists (namely, if the refs are
   packed and we do not hit the d/f conflict in the
   filesystem).

This patch refactors create_symref() to use the "struct
ref_lock" interface, which handles both of these things.
There are a few bonus cleanups that come along with it:

 - we leaked ref_path in some error cases

 - the symref contents were stored in a fixed-size buffer,
   putting an artificial (albeit large) limitation on the
   length of the refname. We now write through fprintf, and
   handle refnames of any size.

 - we called adjust_shared_perm only after the file was
   renamed into place, creating a potential race with
   readers in a shared repository. Now we fix the
   permissions first, and commit only if that succeeded.
   This also makes the update atomic with respect to our
   exit code (whereas previously, we might report failure
   even though we updated the ref).

 - the legacy prefer_symlink_refs path did not do any
   locking at all. Admittedly, it is not atomic from a
   reader's perspective (and it cannot be; it has to unlink
   and then symlink, creating a race), but at least it
   cannot conflict with other writers now.

 - the result of this patch is hopefully more readable. It
   eliminates three goto labels. Two were for error checking
   that is now simplified, and the third was to reach shared
   code that has been pulled into its own function.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c    | 113 +++++++++++++++++++++++++-----------------------
 t/t1401-symbolic-ref.sh |   8 ++++
 2 files changed, 66 insertions(+), 55 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6bfa139..3d53c42 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2811,74 +2811,77 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-int create_symref(const char *ref, const char *target, const char *logmsg)
+static int create_ref_symlink(struct ref_lock *lock, const char *target)
 {
-	char *lockpath = NULL;
-	char buf[1000];
-	int fd, len, written;
-	char *ref_path = git_pathdup("%s", ref);
-	unsigned char old_sha1[20], new_sha1[20];
-	struct strbuf err = STRBUF_INIT;
-
-	if (logmsg && read_ref(ref, old_sha1))
-		hashclr(old_sha1);
-
-	if (safe_create_leading_directories(ref_path) < 0)
-		return error("unable to create directory for %s", ref_path);
-
+	int ret = -1;
 #ifndef NO_SYMLINK_HEAD
-	if (prefer_symlink_refs) {
-		unlink(ref_path);
-		if (!symlink(target, ref_path))
-			goto done;
+	char *ref_path = get_locked_file_path(lock->lk);
+	unlink(ref_path);
+	ret = symlink(target, ref_path);
+	free(ref_path);
+
+	if (ret)
 		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
-	}
 #endif
+	return ret;
+}
 
-	len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
-	if (sizeof(buf) <= len) {
-		error("refname too long: %s", target);
-		goto error_free_return;
-	}
-	lockpath = mkpathdup("%s.lock", ref_path);
-	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
-	if (fd < 0) {
-		error("Unable to open %s for writing", lockpath);
-		goto error_free_return;
-	}
-	written = write_in_full(fd, buf, len);
-	if (close(fd) != 0 || written != len) {
-		error("Unable to write to %s", lockpath);
-		goto error_unlink_return;
-	}
-	if (rename(lockpath, ref_path) < 0) {
-		error("Unable to create %s", ref_path);
-		goto error_unlink_return;
-	}
-	if (adjust_shared_perm(ref_path)) {
-		error("Unable to fix permissions on %s", lockpath);
-	error_unlink_return:
-		unlink_or_warn(lockpath);
-	error_free_return:
-		free(lockpath);
-		free(ref_path);
-		return -1;
-	}
-	free(lockpath);
-
-#ifndef NO_SYMLINK_HEAD
-	done:
-#endif
+static void update_symref_reflog(struct ref_lock *lock, const char *ref,
+				 const char *target, const char *logmsg)
+{
+	struct strbuf err = STRBUF_INIT;
+	unsigned char new_sha1[20];
 	if (logmsg && !read_ref(target, new_sha1) &&
-		log_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {
+	    log_ref_write(ref, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
 		error("%s", err.buf);
 		strbuf_release(&err);
 	}
+}
 
-	free(ref_path);
+static int create_symref_locked(struct ref_lock *lock, const char *ref,
+				const char *target, const char *logmsg)
+{
+	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
+		update_symref_reflog(lock, ref, target, logmsg);
+		return 0;
+	}
+
+	if (!fdopen_lock_file(lock->lk, "w"))
+		return error("unable to fdopen %s: %s",
+			     lock->lk->tempfile.filename.buf, strerror(errno));
+
+	if (adjust_shared_perm(lock->lk->tempfile.filename.buf))
+		return error("unable to fix permissions on %s: %s",
+			     lock->lk->tempfile.filename.buf, strerror(errno));
+
+	/* no error check; commit_ref will check ferror */
+	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
+	if (commit_ref(lock) < 0)
+		return error("unable to write symref for %s: %s", ref,
+			     strerror(errno));
+	update_symref_reflog(lock, ref, target, logmsg);
 	return 0;
 }
 
+int create_symref(const char *ref, const char *target, const char *logmsg)
+{
+	struct strbuf err = STRBUF_INIT;
+	struct ref_lock *lock;
+	int ret;
+
+	lock = lock_ref_sha1_basic(ref, NULL, NULL, NULL, REF_NODEREF, NULL,
+				   &err);
+	if (!lock) {
+		error("%s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
+
+	ret = create_symref_locked(lock, ref, target, logmsg);
+	unlock_ref(lock);
+	return ret;
+}
+
 int reflog_exists(const char *refname)
 {
 	struct stat st;
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 1f0dff3..5db876c 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -114,4 +114,12 @@ test_expect_success 'symbolic-ref writes reflog entry' '
 	test_cmp expect actual
 '
 
+test_expect_success 'symbolic-ref does not create ref d/f conflicts' '
+	git checkout -b df &&
+	test_commit df &&
+	test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df &&
+	git pack-refs --all --prune &&
+	test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df
+'
+
 test_done
-- 
2.7.0.rc1.350.g9acc0f4

Re: [PATCH 4/4] create_symref: use existing ref-lock code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>  #ifndef NO_SYMLINK_HEAD
> -	if (prefer_symlink_refs) {
> -		unlink(ref_path);
> -		if (!symlink(target, ref_path))
> -			goto done;

I see that the original was sloppy (most certainly my bad) ...

> +	char *ref_path = get_locked_file_path(lock->lk);
> +	unlink(ref_path);

... and you inherited that.  I see a few seemingly related helpers
in wrapper.c, but none looks useful in this context X-<.

    if (unlink_or_warn(ref_path))
    	return -1;

is close enough, but it still lets the caller fallback to textual
symref.

> +	ret = symlink(target, ref_path);
> +	free(ref_path);
> +
> +	if (ret)
>  		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
> -	}
>  #endif
> +	return ret;
> +}
>  
> -	len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
> -...
> -	free(lockpath);
> -
> -#ifndef NO_SYMLINK_HEAD
> -	done:
> -#endif

This huge block is worth removing ;-)  Thanks.

Re: [PATCH 4/4] create_symref: use existing ref-lock code

[Jeff King]

On Mon, Dec 21, 2015 at 12:50:28PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >  #ifndef NO_SYMLINK_HEAD
> > -	if (prefer_symlink_refs) {
> > -		unlink(ref_path);
> > -		if (!symlink(target, ref_path))
> > -			goto done;
> 
> I see that the original was sloppy (most certainly my bad) ...
> 
> > +	char *ref_path = get_locked_file_path(lock->lk);
> > +	unlink(ref_path);
> 
> ... and you inherited that.  I see a few seemingly related helpers
> in wrapper.c, but none looks useful in this context X-<.
> 
>     if (unlink_or_warn(ref_path))
>     	return -1;
> 
> is close enough, but it still lets the caller fallback to textual
> symref.

I don't think the original is _wrong_; it's meant to be "unlink if
needed", and the symlink call is what we really care about (if our
unlink failed for anything but ENOENT, the symlink will, too). But I
agree unlink_or_warn should do that and give us a nice warning (and
cover up ENOENT). To fall through, I think we just want (in the
original):

  if (!unlink_or_warn(ref_path) && !symlink(target, ref_path))
	goto done;

and in mine that becomes:

  int ret = -1;
  ...
  if (!unlink_or_warn(ref_path) && !symlink(target, ref_path))
	ret = 0;

I must confess I considered a follow-on patch to drop the
prefer_symlink_refs code path completely. I'm surprised it all still
works with packed-refs, etc, but resolve_ref does take special care to
use readlink().

-Peff

Re: [PATCH 3/4] create_symref: modernize variable names

[Michael Haggerty]

On 12/20/2015 08:29 AM, Jeff King wrote:
> Once upon a time, create_symref() was used only to point
> HEAD at a branch name, and the variable names reflect that
> (e.g., calling the path git_HEAD). However, it is much more
> generic these days (and has been for some time). Let's
> update the variable names to make it easier to follow:
> 
>   - `ref_target` is now just `ref`, matching the declaration
>     in `cache.h` (and hopefully making it clear that it is
>     the symref itself, and not the target).

I've been trying to name variables that hold reference names "refname"
to distinguish them clearly from other representations of references,
like "struct ref_entry *".

> [...]

Otherwise LGTM.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 4/4] create_symref: use existing ref-lock code

[Michael Haggerty]

On 12/20/2015 08:34 AM, Jeff King wrote:
> The create_symref() function predates the existence of
> "struct lock_file", let alone the more recent "struct
> ref_lock". Instead, it just does its own manual dot-locking.
> Besides being more code, this has a few downsides:
> 
>  - if git is interrupted while holding the lock, we don't
>    clean up the lockfile
> 
>  - we don't do the usual directory/filename conflict check.
>    So you can sometimes create a symref "refs/heads/foo/bar",
>    even if "refs/heads/foo" exists (namely, if the refs are
>    packed and we do not hit the d/f conflict in the
>    filesystem).
> 
> This patch refactors create_symref() to use the "struct
> ref_lock" interface, which handles both of these things.
> There are a few bonus cleanups that come along with it:
> 
>  - we leaked ref_path in some error cases
> 
>  - the symref contents were stored in a fixed-size buffer,
>    putting an artificial (albeit large) limitation on the
>    length of the refname. We now write through fprintf, and
>    handle refnames of any size.
> 
>  - we called adjust_shared_perm only after the file was
>    renamed into place, creating a potential race with
>    readers in a shared repository. Now we fix the
>    permissions first, and commit only if that succeeded.
>    This also makes the update atomic with respect to our
>    exit code (whereas previously, we might report failure
>    even though we updated the ref).
> 
>  - the legacy prefer_symlink_refs path did not do any
>    locking at all. Admittedly, it is not atomic from a
>    reader's perspective (and it cannot be; it has to unlink
>    and then symlink, creating a race), but at least it
>    cannot conflict with other writers now.
> 
>  - the result of this patch is hopefully more readable. It
>    eliminates three goto labels. Two were for error checking
>    that is now simplified, and the third was to reach shared
>    code that has been pulled into its own function.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  refs/files-backend.c    | 113 +++++++++++++++++++++++++-----------------------
>  t/t1401-symbolic-ref.sh |   8 ++++
>  2 files changed, 66 insertions(+), 55 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6bfa139..3d53c42 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2811,74 +2811,77 @@ static int commit_ref_update(struct ref_lock *lock,
>  	return 0;
>  }
>  
> -int create_symref(const char *ref, const char *target, const char *logmsg)
> +static int create_ref_symlink(struct ref_lock *lock, const char *target)
>  {
> -	char *lockpath = NULL;
> -	char buf[1000];
> -	int fd, len, written;
> -	char *ref_path = git_pathdup("%s", ref);
> -	unsigned char old_sha1[20], new_sha1[20];
> -	struct strbuf err = STRBUF_INIT;
> -
> -	if (logmsg && read_ref(ref, old_sha1))
> -		hashclr(old_sha1);
> -
> -	if (safe_create_leading_directories(ref_path) < 0)
> -		return error("unable to create directory for %s", ref_path);
> -
> +	int ret = -1;
>  #ifndef NO_SYMLINK_HEAD
> -	if (prefer_symlink_refs) {
> -		unlink(ref_path);
> -		if (!symlink(target, ref_path))
> -			goto done;
> +	char *ref_path = get_locked_file_path(lock->lk);
> +	unlink(ref_path);
> +	ret = symlink(target, ref_path);
> +	free(ref_path);
> +
> +	if (ret)
>  		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
> -	}
>  #endif
> +	return ret;
> +}

This function is racy. A reader might see no reference at all in the
moment between the `unlink()` and the `symlink()`. Moreover, if this
process is killed at that moment, the symbolic ref would be gone forever.

I think that the semantics of `rename()` would allow this race to be
fixed, though, since `symlink()` doesn't have the analogue of
`O_CREAT|O_EXCL`, one would need a lockfile *and* a second temporary
filename under which the new symlink is originally created.

However, this race has always been here, and symlink-based symrefs are
obsolete, so it's probably not worth fixing.

> -	len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
> -	if (sizeof(buf) <= len) {
> -		error("refname too long: %s", target);
> -		goto error_free_return;
> -	}
> -	lockpath = mkpathdup("%s.lock", ref_path);
> -	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
> -	if (fd < 0) {
> -		error("Unable to open %s for writing", lockpath);
> -		goto error_free_return;
> -	}
> -	written = write_in_full(fd, buf, len);
> -	if (close(fd) != 0 || written != len) {
> -		error("Unable to write to %s", lockpath);
> -		goto error_unlink_return;
> -	}
> -	if (rename(lockpath, ref_path) < 0) {
> -		error("Unable to create %s", ref_path);
> -		goto error_unlink_return;
> -	}
> -	if (adjust_shared_perm(ref_path)) {
> -		error("Unable to fix permissions on %s", lockpath);
> -	error_unlink_return:
> -		unlink_or_warn(lockpath);
> -	error_free_return:
> -		free(lockpath);
> -		free(ref_path);
> -		return -1;
> -	}
> -	free(lockpath);
> -
> -#ifndef NO_SYMLINK_HEAD
> -	done:
> -#endif
> +static void update_symref_reflog(struct ref_lock *lock, const char *ref,
> +				 const char *target, const char *logmsg)
> +{
> +	struct strbuf err = STRBUF_INIT;
> +	unsigned char new_sha1[20];
>  	if (logmsg && !read_ref(target, new_sha1) &&
> -		log_ref_write(ref, old_sha1, new_sha1, logmsg, 0, &err)) {
> +	    log_ref_write(ref, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
>  		error("%s", err.buf);
>  		strbuf_release(&err);
>  	}
> +}
>  
> -	free(ref_path);
> +static int create_symref_locked(struct ref_lock *lock, const char *ref,
> +				const char *target, const char *logmsg)
> +{
> +	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
> +		update_symref_reflog(lock, ref, target, logmsg);
> +		return 0;
> +	}
> +
> +	if (!fdopen_lock_file(lock->lk, "w"))
> +		return error("unable to fdopen %s: %s",
> +			     lock->lk->tempfile.filename.buf, strerror(errno));
> +
> +	if (adjust_shared_perm(lock->lk->tempfile.filename.buf))
> +		return error("unable to fix permissions on %s: %s",
> +			     lock->lk->tempfile.filename.buf, strerror(errno));

You can skip this step. lock_file() already calls adjust_shared_perm().

> +	/* no error check; commit_ref will check ferror */
> +	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
> +	if (commit_ref(lock) < 0)
> +		return error("unable to write symref for %s: %s", ref,
> +			     strerror(errno));
> +	update_symref_reflog(lock, ref, target, logmsg);

Here is another problem that didn't originate with your changes:

The reflog should be written while holding the reference lock, to
prevent two processes' trying to write new entries at the same time.

I think the problem would be solved if you move the call to
update_symref_reflog() above the call to commit_ref().

Granted, this could case a reflog entry to be written for a reference
update whose commit fails, but that's also a risk for non-symbolic
references. Fixing this residual problem would require the ability to
roll back reflog changes.

>  	return 0;
>  }
> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 4/4] create_symref: use existing ref-lock code

[Jeff King]

On Mon, Dec 28, 2015 at 10:45:19AM +0100, Michael Haggerty wrote:

> > +static int create_ref_symlink(struct ref_lock *lock, const char *target)
> [...]
> > +	char *ref_path = get_locked_file_path(lock->lk);
> > +	unlink(ref_path);
> > +	ret = symlink(target, ref_path);
> [...]
> 
> This function is racy. A reader might see no reference at all in the
> moment between the `unlink()` and the `symlink()`. Moreover, if this
> process is killed at that moment, the symbolic ref would be gone forever.
> 
> I think that the semantics of `rename()` would allow this race to be
> fixed, though, since `symlink()` doesn't have the analogue of
> `O_CREAT|O_EXCL`, one would need a lockfile *and* a second temporary
> filename under which the new symlink is originally created.
> 
> However, this race has always been here, and symlink-based symrefs are
> obsolete, so it's probably not worth fixing.

Yeah. In the commit message I wrote:

> >  - the legacy prefer_symlink_refs path did not do any
> >    locking at all. Admittedly, it is not atomic from a
> >    reader's perspective (and it cannot be; it has to unlink
> >    and then symlink, creating a race), but at least it
> >    cannot conflict with other writers now.

but I think you are right that we could do tricks with rename() on the
symlink. That is in POSIX, though I wouldn't be surprised if it
misbehaves on some obscure systems. Given that using symlinks is only
triggered by an undocumented (!) option that presumably very few people
use, I'm inclined to leave it as-is.

I was actually tempted to rip it out, as the option is mostly a hack
from 2006:

  http://article.gmane.org/gmane.comp.version-control.git/19402

It was done to let people bisect old kernel trees whose build system
_depends_ on .git/HEAD being a symlink. My thought is:

  - people are a lot less likely to bisect back that far anymore

  - ...and if they do, their build system will be totally broken by the
    new ref-backend stuff

  - ...and the right solution is not to put a hack in git, but for the
    bisecting user to "fix up" the tree before testing (by munging the
    symlink themselves, or applying a patch to the build system to use
    `git rev-parse`). This matches what people have to do for every
    other type of weird "the old version doesn't quite build"
    incompatibility that has nothing to do with git.

But that should probably come as a separate patch (it affects this only
in that it makes this patch simpler to do that cleanup first :) ).

> > +	if (adjust_shared_perm(lock->lk->tempfile.filename.buf))
> > +		return error("unable to fix permissions on %s: %s",
> > +			     lock->lk->tempfile.filename.buf, strerror(errno));
> 
> You can skip this step. lock_file() already calls adjust_shared_perm().

Thanks, fixed.

> > +	/* no error check; commit_ref will check ferror */
> > +	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
> > +	if (commit_ref(lock) < 0)
> > +		return error("unable to write symref for %s: %s", ref,
> > +			     strerror(errno));
> > +	update_symref_reflog(lock, ref, target, logmsg);
> 
> Here is another problem that didn't originate with your changes:
> 
> The reflog should be written while holding the reference lock, to
> prevent two processes' trying to write new entries at the same time.
> 
> I think the problem would be solved if you move the call to
> update_symref_reflog() above the call to commit_ref().
> 
> Granted, this could case a reflog entry to be written for a reference
> update whose commit fails, but that's also a risk for non-symbolic
> references. Fixing this residual problem would require the ability to
> roll back reflog changes.

Thanks, I agree with your reasoning (and especially that we should err
on the same side that normal ref-writing does). I'll do it in a
follow-on patch, though, to make it more clear what is going on.

-Peff

Re: [PATCH 3/4] create_symref: modernize variable names

[Jeff King]

On Mon, Dec 28, 2015 at 09:20:42AM +0100, Michael Haggerty wrote:

> On 12/20/2015 08:29 AM, Jeff King wrote:
> > Once upon a time, create_symref() was used only to point
> > HEAD at a branch name, and the variable names reflect that
> > (e.g., calling the path git_HEAD). However, it is much more
> > generic these days (and has been for some time). Let's
> > update the variable names to make it easier to follow:
> > 
> >   - `ref_target` is now just `ref`, matching the declaration
> >     in `cache.h` (and hopefully making it clear that it is
> >     the symref itself, and not the target).
> 
> I've been trying to name variables that hold reference names "refname"
> to distinguish them clearly from other representations of references,
> like "struct ref_entry *".

That makes sense. I've tweaked this patch to use "refname", and carried
through the change to the following patch.

-Peff

Re: [PATCH 4/4] create_symref: use existing ref-lock code

[Jeff King]

On Tue, Dec 29, 2015 at 12:02:30AM -0500, Jeff King wrote:

> Given that using symlinks is only
> triggered by an undocumented (!) option that presumably very few people
> use, I'm inclined to leave it as-is.

Sorry, I am wrong here. We do document core.preferSymlinkRefs. I missed
it because my grep was case-sensitive. :-/

-Peff

[PATCH v2 0/3] improve symbolic-ref robustness

[Jeff King]

On Sun, Dec 20, 2015 at 02:26:37AM -0500, Jeff King wrote:

> I noticed that an interrupt "git symbolic-ref" will not clean up
> "HEAD.lock". So I started this series as an attempt to convert
> create_symref() to "struct lock_file" to get the usual tempfile cleanup.

Here's version 2, based on comments from Michael. The first two patches
were picked out separately for jk/symbolic-ref-maint, so I've dropped
them here (so 1+2 here are the original 3+4).

The other differences from v1 are:

  - use "refname" instead of "ref" to match surrounding code

  - drop adjust_shared_perm, as lockfile does it for us

  - adjust reflog writing order (done in a new patch)

The patches are:

  [1/3]: create_symref: modernize variable names
  [2/3]: create_symref: use existing ref-lock code
  [3/3]: create_symref: write reflog while holding lock

-Peff

[PATCH 1/3] create_symref: modernize variable names

[Jeff King]

Once upon a time, create_symref() was used only to point
HEAD at a branch name, and the variable names reflect that
(e.g., calling the path git_HEAD). However, it is much more
generic these days (and has been for some time). Let's
update the variable names to make it easier to follow:

  - `ref_target` is now just `refname`. This is closer to
    the `ref` that is already in `cache.h`, but with the
    extra twist that "name" makes it clear this is the name
    and not a ref struct. Dropping "target" hopefully makes
    it clear that we are talking about the symref itself,
    not what it points to.

  - `git_HEAD` is now `ref_path`; the on-disk path
    corresponding to `ref`.

  - `refs_heads_master` is now just `target`; i.e., what the
    symref points at. This term also matches what is in
    the symlink(2) manpage (at least on Linux).

  - the buffer to hold the symref file's contents was simply
    called `ref`. It's now `buf` (admittedly also generic,
    but at least not actively introducing confusion with the
    other variable holding the refname).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.h               |  2 +-
 refs/files-backend.c | 41 ++++++++++++++++++++---------------------
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/refs.h b/refs.h
index 7a04077..3c3da29 100644
--- a/refs.h
+++ b/refs.h
@@ -292,7 +292,7 @@ extern char *shorten_unambiguous_ref(const char *refname, int strict);
 /** rename ref, return 0 on success **/
 extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);
 
-extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
+extern int create_symref(const char *refname, const char *target, const char *logmsg);
 
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c648b5e..d6f9123 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2811,58 +2811,57 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-int create_symref(const char *ref_target, const char *refs_heads_master,
-		  const char *logmsg)
+int create_symref(const char *refname, const char *target, const char *logmsg)
 {
 	char *lockpath = NULL;
-	char ref[1000];
+	char buf[1000];
 	int fd, len, written;
-	char *git_HEAD = git_pathdup("%s", ref_target);
+	char *ref_path = git_pathdup("%s", refname);
 	unsigned char old_sha1[20], new_sha1[20];
 	struct strbuf err = STRBUF_INIT;
 
-	if (logmsg && read_ref(ref_target, old_sha1))
+	if (logmsg && read_ref(refname, old_sha1))
 		hashclr(old_sha1);
 
-	if (safe_create_leading_directories(git_HEAD) < 0)
-		return error("unable to create directory for %s", git_HEAD);
+	if (safe_create_leading_directories(ref_path) < 0)
+		return error("unable to create directory for %s", ref_path);
 
 #ifndef NO_SYMLINK_HEAD
 	if (prefer_symlink_refs) {
-		unlink(git_HEAD);
-		if (!symlink(refs_heads_master, git_HEAD))
+		unlink(ref_path);
+		if (!symlink(target, ref_path))
 			goto done;
 		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
 	}
 #endif
 
-	len = snprintf(ref, sizeof(ref), "ref: %s\n", refs_heads_master);
-	if (sizeof(ref) <= len) {
-		error("refname too long: %s", refs_heads_master);
+	len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
+	if (sizeof(buf) <= len) {
+		error("refname too long: %s", target);
 		goto error_free_return;
 	}
-	lockpath = mkpathdup("%s.lock", git_HEAD);
+	lockpath = mkpathdup("%s.lock", ref_path);
 	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
 	if (fd < 0) {
 		error("Unable to open %s for writing", lockpath);
 		goto error_free_return;
 	}
-	written = write_in_full(fd, ref, len);
+	written = write_in_full(fd, buf, len);
 	if (close(fd) != 0 || written != len) {
 		error("Unable to write to %s", lockpath);
 		goto error_unlink_return;
 	}
-	if (rename(lockpath, git_HEAD) < 0) {
-		error("Unable to create %s", git_HEAD);
+	if (rename(lockpath, ref_path) < 0) {
+		error("Unable to create %s", ref_path);
 		goto error_unlink_return;
 	}
-	if (adjust_shared_perm(git_HEAD)) {
+	if (adjust_shared_perm(ref_path)) {
 		error("Unable to fix permissions on %s", lockpath);
 	error_unlink_return:
 		unlink_or_warn(lockpath);
 	error_free_return:
 		free(lockpath);
-		free(git_HEAD);
+		free(ref_path);
 		return -1;
 	}
 	free(lockpath);
@@ -2870,13 +2869,13 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 #ifndef NO_SYMLINK_HEAD
 	done:
 #endif
-	if (logmsg && !read_ref(refs_heads_master, new_sha1) &&
-		log_ref_write(ref_target, old_sha1, new_sha1, logmsg, 0, &err)) {
+	if (logmsg && !read_ref(target, new_sha1) &&
+		log_ref_write(refname, old_sha1, new_sha1, logmsg, 0, &err)) {
 		error("%s", err.buf);
 		strbuf_release(&err);
 	}
 
-	free(git_HEAD);
+	free(ref_path);
 	return 0;
 }
 
-- 
2.7.0.rc2.368.g1cbb535

[PATCH 2/3] create_symref: use existing ref-lock code

[Jeff King]

The create_symref() function predates the existence of
"struct lock_file", let alone the more recent "struct
ref_lock". Instead, it just does its own manual dot-locking.
Besides being more code, this has a few downsides:

 - if git is interrupted while holding the lock, we don't
   clean up the lockfile

 - we don't do the usual directory/filename conflict check.
   So you can sometimes create a symref "refs/heads/foo/bar",
   even if "refs/heads/foo" exists (namely, if the refs are
   packed and we do not hit the d/f conflict in the
   filesystem).

This patch refactors create_symref() to use the "struct
ref_lock" interface, which handles both of these things.
There are a few bonus cleanups that come along with it:

 - we leaked ref_path in some error cases

 - the symref contents were stored in a fixed-size buffer,
   putting an artificial (albeit large) limitation on the
   length of the refname. We now write through fprintf, and
   handle refnames of any size.

 - we called adjust_shared_perm only after the file was
   renamed into place, creating a potential race with
   readers in a shared repository. The lockfile code now
   handles this when creating the lockfile, making it
   atomic.

 - the legacy prefer_symlink_refs path did not do any
   locking at all. Admittedly, it is not atomic from a
   reader's perspective (as it unlinks and re-creates the
   symlink to overwrite), but at least it cannot conflict
   with other writers now.

 - the result of this patch is hopefully more readable. It
   eliminates three goto labels. Two were for error checking
   that is now simplified, and the third was to reach shared
   code that has been pulled into its own function.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c    | 109 ++++++++++++++++++++++++------------------------
 t/t1401-symbolic-ref.sh |   8 ++++
 2 files changed, 62 insertions(+), 55 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d6f9123..3d1994d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2811,74 +2811,73 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-int create_symref(const char *refname, const char *target, const char *logmsg)
+static int create_ref_symlink(struct ref_lock *lock, const char *target)
 {
-	char *lockpath = NULL;
-	char buf[1000];
-	int fd, len, written;
-	char *ref_path = git_pathdup("%s", refname);
-	unsigned char old_sha1[20], new_sha1[20];
-	struct strbuf err = STRBUF_INIT;
-
-	if (logmsg && read_ref(refname, old_sha1))
-		hashclr(old_sha1);
-
-	if (safe_create_leading_directories(ref_path) < 0)
-		return error("unable to create directory for %s", ref_path);
-
+	int ret = -1;
 #ifndef NO_SYMLINK_HEAD
-	if (prefer_symlink_refs) {
-		unlink(ref_path);
-		if (!symlink(target, ref_path))
-			goto done;
+	char *ref_path = get_locked_file_path(lock->lk);
+	unlink(ref_path);
+	ret = symlink(target, ref_path);
+	free(ref_path);
+
+	if (ret)
 		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
-	}
 #endif
+	return ret;
+}
 
-	len = snprintf(buf, sizeof(buf), "ref: %s\n", target);
-	if (sizeof(buf) <= len) {
-		error("refname too long: %s", target);
-		goto error_free_return;
-	}
-	lockpath = mkpathdup("%s.lock", ref_path);
-	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
-	if (fd < 0) {
-		error("Unable to open %s for writing", lockpath);
-		goto error_free_return;
-	}
-	written = write_in_full(fd, buf, len);
-	if (close(fd) != 0 || written != len) {
-		error("Unable to write to %s", lockpath);
-		goto error_unlink_return;
-	}
-	if (rename(lockpath, ref_path) < 0) {
-		error("Unable to create %s", ref_path);
-		goto error_unlink_return;
-	}
-	if (adjust_shared_perm(ref_path)) {
-		error("Unable to fix permissions on %s", lockpath);
-	error_unlink_return:
-		unlink_or_warn(lockpath);
-	error_free_return:
-		free(lockpath);
-		free(ref_path);
-		return -1;
-	}
-	free(lockpath);
-
-#ifndef NO_SYMLINK_HEAD
-	done:
-#endif
+static void update_symref_reflog(struct ref_lock *lock, const char *refname,
+				 const char *target, const char *logmsg)
+{
+	struct strbuf err = STRBUF_INIT;
+	unsigned char new_sha1[20];
 	if (logmsg && !read_ref(target, new_sha1) &&
-		log_ref_write(refname, old_sha1, new_sha1, logmsg, 0, &err)) {
+	    log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
 		error("%s", err.buf);
 		strbuf_release(&err);
 	}
+}
 
-	free(ref_path);
+static int create_symref_locked(struct ref_lock *lock, const char *refname,
+				const char *target, const char *logmsg)
+{
+	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
+		update_symref_reflog(lock, refname, target, logmsg);
+		return 0;
+	}
+
+	if (!fdopen_lock_file(lock->lk, "w"))
+		return error("unable to fdopen %s: %s",
+			     lock->lk->tempfile.filename.buf, strerror(errno));
+
+	/* no error check; commit_ref will check ferror */
+	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
+	if (commit_ref(lock) < 0)
+		return error("unable to write symref for %s: %s", refname,
+			     strerror(errno));
+	update_symref_reflog(lock, refname, target, logmsg);
 	return 0;
 }
 
+int create_symref(const char *refname, const char *target, const char *logmsg)
+{
+	struct strbuf err = STRBUF_INIT;
+	struct ref_lock *lock;
+	int ret;
+
+	lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
+				   &err);
+	if (!lock) {
+		error("%s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
+
+	ret = create_symref_locked(lock, refname, target, logmsg);
+	unlock_ref(lock);
+	return ret;
+}
+
 int reflog_exists(const char *refname)
 {
 	struct stat st;
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 1f0dff3..5db876c 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -114,4 +114,12 @@ test_expect_success 'symbolic-ref writes reflog entry' '
 	test_cmp expect actual
 '
 
+test_expect_success 'symbolic-ref does not create ref d/f conflicts' '
+	git checkout -b df &&
+	test_commit df &&
+	test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df &&
+	git pack-refs --all --prune &&
+	test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df
+'
+
 test_done
-- 
2.7.0.rc2.368.g1cbb535

[PATCH 3/3] create_symref: write reflog while holding lock

[Jeff King]

We generally hold a lock on the matching ref while writing
to its reflog; this prevents two simultaneous writers from
clobbering each other's reflog lines (it does not even have
to be two symref updates; because we don't hold the lock, we
could race with somebody writing to the pointed-to ref via
HEAD, for example).

We can fix this by writing the reflog before we commit the
lockfile. This runs the risk of writing the reflog but
failing the final rename(), but at least we now err on the
same side as the rest of the ref code.

Noticed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3d1994d..180c837 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2850,12 +2850,13 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,
 		return error("unable to fdopen %s: %s",
 			     lock->lk->tempfile.filename.buf, strerror(errno));
 
+	update_symref_reflog(lock, refname, target, logmsg);
+
 	/* no error check; commit_ref will check ferror */
 	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
 	if (commit_ref(lock) < 0)
 		return error("unable to write symref for %s: %s", refname,
 			     strerror(errno));
-	update_symref_reflog(lock, refname, target, logmsg);
 	return 0;
 }
 
-- 
2.7.0.rc2.368.g1cbb535

[RFC/PATCH 4/3] create_symref: drop support for writing symbolic links

[Jeff King]

I'm iffy on this one, just because it drops a user-visible feature with
no deprecation. For the reasons given below, I doubt anybody is using it
for the original intended purpose, but maybe there is some crazy person
whose workflow revolves around core.preferSymlinkRefs.

A conservative choice would probably be to issue a deprecation warning
when we see it defined, wait a few versions, and then apply the patch
below.

-- >8 --
Long ago, in 2fabd21 (Disable USE_SYMLINK_HEAD by default,
2005-11-15), we switched git's default behavior to writing
text symrefs instead of symbolic links. Any scripts
accustomed to looking directly at .git/HEAD were updated to
use `rev-parse` instead. The Linux kernel's setlocalversion
script was one, and it was fixed in 117a93d (kbuild: Use git
in scripts/setlocalversion, 2006-01-04).

However, the problem still happened when bisecting the
kernel; pre-117a93d kernels would not build properly with a
newer git, because they wanted to look directly at HEAD. To
solve this, we added 9f0bb90 (core.prefersymlinkrefs: use
symlinks for .git/HEAD, 2006-05-02), which lets the user
turn on the old behavior, theoretically letting you bisect
older kernel history.

But there are a few complications with this solution:

  - packed-refs means you are limited in what you can do
    with .git/HEAD. If it is a symlink, you may `readlink`
    it to see where it points, but you cannot necessarily
    `cat .git/HEAD` to get the sha1, as the pointed-to ref
    may be packed.

    In particular, this means that the pre-117a93d kbuild
    script would sometimes work and sometimes not.

  - These days, we bisect on a detached HEAD. So during
    bisection, .git/HEAD _is_ a regular file with the
    sha1, and it works to `cat` it, whether or not you set
    core.preferSymlinkRefs.

Such scripts will all be broken again if we move to
alternate ref backends. They should have learned to use
`rev-parse` long ago, and it is only bisecting ancient
history that is a problem.

Now that almost ten years have passed, it seems less likely
that developers will bisect so far back in history. And
moreover, this is but one of many possible problems
developers run into when trying to build versions. The
standard solution is to apply a "fixup" patch or other
workaround while test-building the project, and that would
work here, too.

This patch therefore drops core.preferSymlinkRefs
completely. There are a few reasons to want to do so:

  1. It drops some code that is probably exercised very
     rarely.

  2. The symlink code is not up to the same standards as the
     rest of the ref code. In particular, it is racy with
     respect to simultaneous readers and writers.

  3. If we want to eventually drop the symlink-reading code,
     this is a good first step to deprecating it.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt               |  6 ------
 cache.h                                |  1 -
 config.c                               |  5 -----
 contrib/completion/git-completion.bash |  1 -
 environment.c                          |  1 -
 refs/files-backend.c                   | 20 --------------------
 t/t7201-co.sh                          | 12 ------------
 t/t9903-bash-prompt.sh                 |  9 ---------
 8 files changed, 55 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..06a2f2a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -433,12 +433,6 @@ CIFS/Microsoft Windows.
 +
 False by default.
 
-core.preferSymlinkRefs::
-	Instead of the default "symref" format for HEAD
-	and other symbolic reference files, use symbolic links.
-	This is sometimes needed to work with old scripts that
-	expect HEAD to be a symbolic link.
-
 core.bare::
 	If true this repository is assumed to be 'bare' and has no
 	working directory associated with it.  If this is the case a
diff --git a/cache.h b/cache.h
index c63fcc1..5ff7df2 100644
--- a/cache.h
+++ b/cache.h
@@ -625,7 +625,6 @@ extern int has_symlinks;
 extern int minimum_abbrev, default_abbrev;
 extern int ignore_case;
 extern int assume_unchanged;
-extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
diff --git a/config.c b/config.c
index 86a5eb2..f479eaa 100644
--- a/config.c
+++ b/config.c
@@ -726,11 +726,6 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
-	if (!strcmp(var, "core.prefersymlinkrefs")) {
-		prefer_symlink_refs = git_config_bool(var, value);
-		return 0;
-	}
-
 	if (!strcmp(var, "core.logallrefupdates")) {
 		log_all_ref_updates = git_config_bool(var, value);
 		return 0;
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6956807..31d517d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2046,7 +2046,6 @@ _git_config ()
 		core.packedGitLimit
 		core.packedGitWindowSize
 		core.pager
-		core.preferSymlinkRefs
 		core.preloadindex
 		core.quotepath
 		core.repositoryFormatVersion
diff --git a/environment.c b/environment.c
index 2da7fe2..80a1c0c 100644
--- a/environment.c
+++ b/environment.c
@@ -19,7 +19,6 @@ int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;
 int ignore_case;
 int assume_unchanged;
-int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 180c837..6e19953 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-static int create_ref_symlink(struct ref_lock *lock, const char *target)
-{
-	int ret = -1;
-#ifndef NO_SYMLINK_HEAD
-	char *ref_path = get_locked_file_path(lock->lk);
-	unlink(ref_path);
-	ret = symlink(target, ref_path);
-	free(ref_path);
-
-	if (ret)
-		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
-#endif
-	return ret;
-}
-
 static void update_symref_reflog(struct ref_lock *lock, const char *refname,
 				 const char *target, const char *logmsg)
 {
@@ -2841,11 +2826,6 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,
 static int create_symref_locked(struct ref_lock *lock, const char *refname,
 				const char *target, const char *logmsg)
 {
-	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
-		update_symref_reflog(lock, refname, target, logmsg);
-		return 0;
-	}
-
 	if (!fdopen_lock_file(lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
 			     lock->lk->tempfile.filename.buf, strerror(errno));
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 8859236..715dc00 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -427,18 +427,6 @@ test_expect_success 'checkout w/--track from tag fails' '
     test "z$(git rev-parse master^0)" = "z$(git rev-parse HEAD)"
 '
 
-test_expect_success 'detach a symbolic link HEAD' '
-    git checkout master &&
-    git config --bool core.prefersymlinkrefs yes &&
-    git checkout side &&
-    git checkout master &&
-    it=$(git symbolic-ref HEAD) &&
-    test "z$it" = zrefs/heads/master &&
-    here=$(git rev-parse --verify refs/heads/master) &&
-    git checkout side^ &&
-    test "z$(git rev-parse --verify refs/heads/master)" = "z$here"
-'
-
 test_expect_success \
     'checkout with --track fakes a sensible -b <name>' '
     git config remote.origin.fetch "+refs/heads/*:refs/remotes/origin/*" &&
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index af82049..25066f35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -46,15 +46,6 @@ test_expect_success 'prompt - branch name' '
 	test_cmp expected "$actual"
 '
 
-test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '
-	printf " (master)" >expected &&
-	test_when_finished "git checkout master" &&
-	test_config core.preferSymlinkRefs true &&
-	git checkout master &&
-	__git_ps1 >"$actual" &&
-	test_cmp expected "$actual"
-'
-
 test_expect_success 'prompt - unborn branch' '
 	printf " (unborn)" >expected &&
 	git checkout --orphan unborn &&
-- 
2.7.0.rc2.368.g1cbb535

Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links

[Jeff King]

On Tue, Dec 29, 2015 at 01:00:55AM -0500, Jeff King wrote:

> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,
>  	return 0;
>  }
>  
> -static int create_ref_symlink(struct ref_lock *lock, const char *target)
> -{
> -	int ret = -1;
> -#ifndef NO_SYMLINK_HEAD
> -	char *ref_path = get_locked_file_path(lock->lk);
> -	unlink(ref_path);
> -	ret = symlink(target, ref_path);
> -	free(ref_path);
> -
> -	if (ret)
> -		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
> -#endif

I forgot the build-time knob, which becomes a noop after this patch. If
we choose to apply this patch, we'd want to squash this in, too:

diff --git a/Makefile b/Makefile
index fd19b54..05ffd60 100644
--- a/Makefile
+++ b/Makefile
@@ -113,9 +113,6 @@ all::
 #
 # Define NO_SYS_SELECT_H if you don't have sys/select.h.
 #
-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
-# Enable it on Windows.  By default, symrefs are still used.
-#
 # Define NO_SVN_TESTS if you want to skip time-consuming SVN interoperability
 # tests.  These tests take up a significant amount of the total test time
 # but are not needed unless you plan to talk to SVN repos.
@@ -1200,9 +1197,6 @@ ifdef FREAD_READS_DIRECTORIES
 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
 	COMPAT_OBJS += compat/fopen.o
 endif
-ifdef NO_SYMLINK_HEAD
-	BASIC_CFLAGS += -DNO_SYMLINK_HEAD
-endif
 ifdef GETTEXT_POISON
 	BASIC_CFLAGS += -DGETTEXT_POISON
 endif
diff --git a/config.mak.uname b/config.mak.uname
index f34dcaa..9b77e2c 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -169,7 +169,6 @@ ifeq ($(uname_O),Cygwin)
 		NO_STRCASESTR = YesPlease
 		NO_MEMMEM = YesPlease
 		NO_MKSTEMPS = YesPlease
-		NO_SYMLINK_HEAD = YesPlease
 		NO_IPV6 = YesPlease
 		OLD_ICONV = UnfortunatelyYes
 		# There are conflicting reports about this.
@@ -338,7 +337,6 @@ ifeq ($(uname_S),Windows)
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
-	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
 	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
@@ -491,7 +489,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
-	NO_SYMLINK_HEAD = YesPlease
 	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
 	NO_STRCASESTR = YesPlease
diff --git a/configure.ac b/configure.ac
index 89e2590..cad5418 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1096,9 +1096,6 @@ GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
 # Define USE_PIC if you need the main git objects to be built with -fPIC
 # in order to build and link perl/Git.so.  x86-64 seems to need this.
 #
-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
-# Enable it on Windows.  By default, symrefs are still used.
-#
 # Define NO_PTHREADS if we do not have pthreads.
 #
 # Define PTHREAD_LIBS to the linker flag used for Pthread support.

Re: [PATCH v2 0/3] improve symbolic-ref robustness

[Michael Haggerty]

On 12/29/2015 06:55 AM, Jeff King wrote:
> On Sun, Dec 20, 2015 at 02:26:37AM -0500, Jeff King wrote:
> 
>> I noticed that an interrupt "git symbolic-ref" will not clean up
>> "HEAD.lock". So I started this series as an attempt to convert
>> create_symref() to "struct lock_file" to get the usual tempfile cleanup.
> 
> Here's version 2, based on comments from Michael. The first two patches
> were picked out separately for jk/symbolic-ref-maint, so I've dropped
> them here (so 1+2 here are the original 3+4).
> 
> The other differences from v1 are:
> 
>   - use "refname" instead of "ref" to match surrounding code
> 
>   - drop adjust_shared_perm, as lockfile does it for us
> 
>   - adjust reflog writing order (done in a new patch)
> 
> The patches are:
> 
>   [1/3]: create_symref: modernize variable names
>   [2/3]: create_symref: use existing ref-lock code
>   [3/3]: create_symref: write reflog while holding lock

Thanks, Peff. The whole series is

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> A conservative choice would probably be to issue a deprecation warning
> when we see it defined, wait a few versions, and then apply the patch
> below.

I agree with the analysis below.  And I agree that in the ideal
world, it would have been better not to add "prefer symlink refs"
configuration in the first place.  But we do not live in the ideal
world, and we already have it, so deprecation would need the usual
multi-step process.

Thanks.

> -- >8 --
> Long ago, in 2fabd21 (Disable USE_SYMLINK_HEAD by default,
> 2005-11-15), we switched git's default behavior to writing
> text symrefs instead of symbolic links. Any scripts
> accustomed to looking directly at .git/HEAD were updated to
> use `rev-parse` instead. The Linux kernel's setlocalversion
> script was one, and it was fixed in 117a93d (kbuild: Use git
> in scripts/setlocalversion, 2006-01-04).
>
> However, the problem still happened when bisecting the
> kernel; pre-117a93d kernels would not build properly with a
> newer git, because they wanted to look directly at HEAD. To
> solve this, we added 9f0bb90 (core.prefersymlinkrefs: use
> symlinks for .git/HEAD, 2006-05-02), which lets the user
> turn on the old behavior, theoretically letting you bisect
> older kernel history.
>
> But there are a few complications with this solution:
>
>   - packed-refs means you are limited in what you can do
>     with .git/HEAD. If it is a symlink, you may `readlink`
>     it to see where it points, but you cannot necessarily
>     `cat .git/HEAD` to get the sha1, as the pointed-to ref
>     may be packed.
>
>     In particular, this means that the pre-117a93d kbuild
>     script would sometimes work and sometimes not.
>
>   - These days, we bisect on a detached HEAD. So during
>     bisection, .git/HEAD _is_ a regular file with the
>     sha1, and it works to `cat` it, whether or not you set
>     core.preferSymlinkRefs.
>
> Such scripts will all be broken again if we move to
> alternate ref backends. They should have learned to use
> `rev-parse` long ago, and it is only bisecting ancient
> history that is a problem.
>
> Now that almost ten years have passed, it seems less likely
> that developers will bisect so far back in history. And
> moreover, this is but one of many possible problems
> developers run into when trying to build versions. The
> standard solution is to apply a "fixup" patch or other
> workaround while test-building the project, and that would
> work here, too.
>
> This patch therefore drops core.preferSymlinkRefs
> completely. There are a few reasons to want to do so:
>
>   1. It drops some code that is probably exercised very
>      rarely.
>
>   2. The symlink code is not up to the same standards as the
>      rest of the ref code. In particular, it is racy with
>      respect to simultaneous readers and writers.
>
>   3. If we want to eventually drop the symlink-reading code,
>      this is a good first step to deprecating it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/config.txt               |  6 ------
>  cache.h                                |  1 -
>  config.c                               |  5 -----
>  contrib/completion/git-completion.bash |  1 -
>  environment.c                          |  1 -
>  refs/files-backend.c                   | 20 --------------------
>  t/t7201-co.sh                          | 12 ------------
>  t/t9903-bash-prompt.sh                 |  9 ---------
>  8 files changed, 55 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f617886..06a2f2a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -433,12 +433,6 @@ CIFS/Microsoft Windows.
>  +
>  False by default.
>  
> -core.preferSymlinkRefs::
> -	Instead of the default "symref" format for HEAD
> -	and other symbolic reference files, use symbolic links.
> -	This is sometimes needed to work with old scripts that
> -	expect HEAD to be a symbolic link.
> -
>  core.bare::
>  	If true this repository is assumed to be 'bare' and has no
>  	working directory associated with it.  If this is the case a
> diff --git a/cache.h b/cache.h
> index c63fcc1..5ff7df2 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -625,7 +625,6 @@ extern int has_symlinks;
>  extern int minimum_abbrev, default_abbrev;
>  extern int ignore_case;
>  extern int assume_unchanged;
> -extern int prefer_symlink_refs;
>  extern int log_all_ref_updates;
>  extern int warn_ambiguous_refs;
>  extern int warn_on_object_refname_ambiguity;
> diff --git a/config.c b/config.c
> index 86a5eb2..f479eaa 100644
> --- a/config.c
> +++ b/config.c
> @@ -726,11 +726,6 @@ static int git_default_core_config(const char *var, const char *value)
>  		return 0;
>  	}
>  
> -	if (!strcmp(var, "core.prefersymlinkrefs")) {
> -		prefer_symlink_refs = git_config_bool(var, value);
> -		return 0;
> -	}
> -
>  	if (!strcmp(var, "core.logallrefupdates")) {
>  		log_all_ref_updates = git_config_bool(var, value);
>  		return 0;
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 6956807..31d517d 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2046,7 +2046,6 @@ _git_config ()
>  		core.packedGitLimit
>  		core.packedGitWindowSize
>  		core.pager
> -		core.preferSymlinkRefs
>  		core.preloadindex
>  		core.quotepath
>  		core.repositoryFormatVersion
> diff --git a/environment.c b/environment.c
> index 2da7fe2..80a1c0c 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -19,7 +19,6 @@ int has_symlinks = 1;
>  int minimum_abbrev = 4, default_abbrev = 7;
>  int ignore_case;
>  int assume_unchanged;
> -int prefer_symlink_refs;
>  int is_bare_repository_cfg = -1; /* unspecified */
>  int log_all_ref_updates = -1; /* unspecified */
>  int warn_ambiguous_refs = 1;
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 180c837..6e19953 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,
>  	return 0;
>  }
>  
> -static int create_ref_symlink(struct ref_lock *lock, const char *target)
> -{
> -	int ret = -1;
> -#ifndef NO_SYMLINK_HEAD
> -	char *ref_path = get_locked_file_path(lock->lk);
> -	unlink(ref_path);
> -	ret = symlink(target, ref_path);
> -	free(ref_path);
> -
> -	if (ret)
> -		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
> -#endif
> -	return ret;
> -}
> -
>  static void update_symref_reflog(struct ref_lock *lock, const char *refname,
>  				 const char *target, const char *logmsg)
>  {
> @@ -2841,11 +2826,6 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,
>  static int create_symref_locked(struct ref_lock *lock, const char *refname,
>  				const char *target, const char *logmsg)
>  {
> -	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
> -		update_symref_reflog(lock, refname, target, logmsg);
> -		return 0;
> -	}
> -
>  	if (!fdopen_lock_file(lock->lk, "w"))
>  		return error("unable to fdopen %s: %s",
>  			     lock->lk->tempfile.filename.buf, strerror(errno));
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 8859236..715dc00 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -427,18 +427,6 @@ test_expect_success 'checkout w/--track from tag fails' '
>      test "z$(git rev-parse master^0)" = "z$(git rev-parse HEAD)"
>  '
>  
> -test_expect_success 'detach a symbolic link HEAD' '
> -    git checkout master &&
> -    git config --bool core.prefersymlinkrefs yes &&
> -    git checkout side &&
> -    git checkout master &&
> -    it=$(git symbolic-ref HEAD) &&
> -    test "z$it" = zrefs/heads/master &&
> -    here=$(git rev-parse --verify refs/heads/master) &&
> -    git checkout side^ &&
> -    test "z$(git rev-parse --verify refs/heads/master)" = "z$here"
> -'
> -
>  test_expect_success \
>      'checkout with --track fakes a sensible -b <name>' '
>      git config remote.origin.fetch "+refs/heads/*:refs/remotes/origin/*" &&
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index af82049..25066f35 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -46,15 +46,6 @@ test_expect_success 'prompt - branch name' '
>  	test_cmp expected "$actual"
>  '
>  
> -test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '
> -	printf " (master)" >expected &&
> -	test_when_finished "git checkout master" &&
> -	test_config core.preferSymlinkRefs true &&
> -	git checkout master &&
> -	__git_ps1 >"$actual" &&
> -	test_cmp expected "$actual"
> -'
> -
>  test_expect_success 'prompt - unborn branch' '
>  	printf " (unborn)" >expected &&
>  	git checkout --orphan unborn &&

Re: [PATCH v2 0/3] improve symbolic-ref robustness

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 12/29/2015 06:55 AM, Jeff King wrote:
>
>> The patches are:
>> 
>>   [1/3]: create_symref: modernize variable names
>>   [2/3]: create_symref: use existing ref-lock code
>>   [3/3]: create_symref: write reflog while holding lock
>
> Thanks, Peff. The whole series is
>
> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
>
> Michael

Thanks, both.

Re: [PATCH v2 0/3] improve symbolic-ref robustness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Dec 20, 2015 at 02:26:37AM -0500, Jeff King wrote:
>
>> I noticed that an interrupt "git symbolic-ref" will not clean up
>> "HEAD.lock". So I started this series as an attempt to convert
>> create_symref() to "struct lock_file" to get the usual tempfile cleanup.
>
> Here's version 2, based on comments from Michael. The first two patches
> were picked out separately for jk/symbolic-ref-maint, so I've dropped
> them here (so 1+2 here are the original 3+4).
>
> The other differences from v1 are:
>
>   - use "refname" instead of "ref" to match surrounding code
>
>   - drop adjust_shared_perm, as lockfile does it for us
>
>   - adjust reflog writing order (done in a new patch)
>
> The patches are:
>
>   [1/3]: create_symref: modernize variable names
>   [2/3]: create_symref: use existing ref-lock code
>   [3/3]: create_symref: write reflog while holding lock

This is queued as an early part of 'pu', and some refactoring in
David's refs-backend-lmdb topic conflicts with it when merged to
'pu'.  I think I resolved the conflicts correctly, but please double
check the result when I push it out later today.

Thanks.

Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links

[Jeff King]

On Tue, Dec 29, 2015 at 10:32:04AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > A conservative choice would probably be to issue a deprecation warning
> > when we see it defined, wait a few versions, and then apply the patch
> > below.
> 
> I agree with the analysis below.  And I agree that in the ideal
> world, it would have been better not to add "prefer symlink refs"
> configuration in the first place.  But we do not live in the ideal
> world, and we already have it, so deprecation would need the usual
> multi-step process.

Here's the first step of that multi-step process. The commit message
will look familiar, as the rationale for deprecating is the same as for
dropping.

This is on top of what I posted earlier (it obviously could be
independent, but I assume we're planning to merge the other bits, and I
don't mind holding this topic hostage for a little while to save some
annoying merging).

-- >8 --
Subject: [PATCH] create_symref: deprecate support for writing symbolic links

Long ago, in 2fabd21 (Disable USE_SYMLINK_HEAD by default,
2005-11-15), we switched git's default behavior to writing
text symrefs instead of symbolic links. Any scripts
accustomed to looking directly at .git/HEAD were updated to
use `rev-parse` instead. The Linux kernel's setlocalversion
script was one, and it was fixed in 117a93d (kbuild: Use git
in scripts/setlocalversion, 2006-01-04).

However, the problem still happened when bisecting the
kernel; pre-117a93d would not build properly with a newer
git, because they wanted to look directly at HEAD. To solve
this, we added 9f0bb90 (core.prefersymlinkrefs: use symlinks
for .git/HEAD, 2006-05-02), which lets the user turn on the
old behavior, theoretically letting you bisect older kernel
history.

But there are a few complications with this solution:

  - packed-refs means you are limited in what you can do
    with .git/HEAD. If it is a symlink, you may `readlink`
    it to see where it points, but you cannot necessarily
    `cat .git/HEAD` to get the sha1, as the pointed-to ref
    may be packed.

    In particular, this means that the pre-117a93d kbuild
    script would sometimes work and sometimes not.

  - These days, we bisect on a detached HEAD. So during
    bisection, .git/HEAD _is_ a regular file with the
    sha1, and it works to `cat` it, whether or not you set
    core.preferSymlinkRefs.

Such scripts will all be broken again if we move to
alternate ref backends. They should have learned to use
`rev-parse` long ago, and it is only bisecting ancient
history that is a problem.

Now that almost ten years have passed, it seems less likely
that developers will bisect so far back in history. And
moreover, this is but one of many possible problems
developers run into when trying to build versions. The
standard solution is to apply a "fixup" patch or other
workaround while test-building the project, and that would
work here, too.

This patch therefore deprecates core.preferSymlinkRefs
completely. There are a few reasons to want to do so:

  1. It drops some code that is probably exercised very
     rarely.

  2. The symlink code is not up to the same standards as the
     rest of the ref code. In particular, it is racy with
     respect to simultaneous readers and writers.

  3. If we want to eventually drop the symlink-reading code,
     this is a good first step to deprecating it.

We print a deprecation warning anytime a symlink is created
using this code. That prevents us from spamming the user
with multiple warnings from read-only operations, but
catches major operations like init, clone, and checkout.

Signed-off-by: Jeff King <peff@peff.net>
---
There's no advice.* safety valve here. The solution is "fix your
script", and the last-ditch effort is "come to the mailing list and tell
us why it would be a bad thing to remove the feature".

 refs/files-backend.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 180c837..22b7c11 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2860,12 +2860,27 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,
 	return 0;
 }
 
+static const char symlink_deprecation_warning[] =
+"The core.preferSymlinkRefs configuration option has been\n"
+"deprecated and will be removed in a future version of Git. If your\n"
+"workflow or script depends on '.git/HEAD' being a symbolic link,\n"
+"it should be adjusted to use:\n"
+"\n"
+"        git rev-parse HEAD\n"
+"\n"
+"        git rev-parse --symbolic-full-name HEAD\n"
+"\n"
+"to get the sha1 or branch name, respectively.";
+
 int create_symref(const char *refname, const char *target, const char *logmsg)
 {
 	struct strbuf err = STRBUF_INIT;
 	struct ref_lock *lock;
 	int ret;
 
+	if (prefer_symlink_refs)
+		warning("%s", symlink_deprecation_warning);
+
 	lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
 				   &err);
 	if (!lock) {
-- 
2.7.0.rc3.367.g09631da

Re: [RFC/PATCH 4/3] create_symref: drop support for writing symbolic links

[Jeff King]

On Wed, Dec 30, 2015 at 01:53:43AM -0500, Jeff King wrote:

> On Tue, Dec 29, 2015 at 10:32:04AM -0800, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > A conservative choice would probably be to issue a deprecation warning
> > > when we see it defined, wait a few versions, and then apply the patch
> > > below.
> > 
> > I agree with the analysis below.  And I agree that in the ideal
> > world, it would have been better not to add "prefer symlink refs"
> > configuration in the first place.  But we do not live in the ideal
> > world, and we already have it, so deprecation would need the usual
> > multi-step process.
> 
> Here's the first step of that multi-step process. The commit message
> will look familiar, as the rationale for deprecating is the same as for
> dropping.

And here's the second step, which would obviously want to wait a while
before being merged.

A third possible step would be removing the reading side from
resolve_ref(). But that would want to wait a few more cycles past this
one. And I'm hesitant to do it while the ref code is in flux. It will be
easier to just write the patch later on. :)

-- >8 --
Subject: [PATCH] create_symref: drop support for writing symbolic links

The parent commit deprecated the writing of symbolic links
for symrefs.  Now that some time has passed, we can follow
through by dropping the code.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt               |  6 ------
 Makefile                               |  6 ------
 cache.h                                |  1 -
 config.c                               |  5 -----
 config.mak.uname                       |  3 ---
 configure.ac                           |  3 ---
 contrib/completion/git-completion.bash |  1 -
 environment.c                          |  1 -
 refs/files-backend.c                   | 35 ----------------------------------
 t/t7201-co.sh                          | 12 ------------
 t/t9903-bash-prompt.sh                 |  9 ---------
 11 files changed, 82 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..06a2f2a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -433,12 +433,6 @@ CIFS/Microsoft Windows.
 +
 False by default.
 
-core.preferSymlinkRefs::
-	Instead of the default "symref" format for HEAD
-	and other symbolic reference files, use symbolic links.
-	This is sometimes needed to work with old scripts that
-	expect HEAD to be a symbolic link.
-
 core.bare::
 	If true this repository is assumed to be 'bare' and has no
 	working directory associated with it.  If this is the case a
diff --git a/Makefile b/Makefile
index fd19b54..05ffd60 100644
--- a/Makefile
+++ b/Makefile
@@ -113,9 +113,6 @@ all::
 #
 # Define NO_SYS_SELECT_H if you don't have sys/select.h.
 #
-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
-# Enable it on Windows.  By default, symrefs are still used.
-#
 # Define NO_SVN_TESTS if you want to skip time-consuming SVN interoperability
 # tests.  These tests take up a significant amount of the total test time
 # but are not needed unless you plan to talk to SVN repos.
@@ -1200,9 +1197,6 @@ ifdef FREAD_READS_DIRECTORIES
 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
 	COMPAT_OBJS += compat/fopen.o
 endif
-ifdef NO_SYMLINK_HEAD
-	BASIC_CFLAGS += -DNO_SYMLINK_HEAD
-endif
 ifdef GETTEXT_POISON
 	BASIC_CFLAGS += -DGETTEXT_POISON
 endif
diff --git a/cache.h b/cache.h
index c63fcc1..5ff7df2 100644
--- a/cache.h
+++ b/cache.h
@@ -625,7 +625,6 @@ extern int has_symlinks;
 extern int minimum_abbrev, default_abbrev;
 extern int ignore_case;
 extern int assume_unchanged;
-extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
diff --git a/config.c b/config.c
index 86a5eb2..f479eaa 100644
--- a/config.c
+++ b/config.c
@@ -726,11 +726,6 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
-	if (!strcmp(var, "core.prefersymlinkrefs")) {
-		prefer_symlink_refs = git_config_bool(var, value);
-		return 0;
-	}
-
 	if (!strcmp(var, "core.logallrefupdates")) {
 		log_all_ref_updates = git_config_bool(var, value);
 		return 0;
diff --git a/config.mak.uname b/config.mak.uname
index f34dcaa..9b77e2c 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -169,7 +169,6 @@ ifeq ($(uname_O),Cygwin)
 		NO_STRCASESTR = YesPlease
 		NO_MEMMEM = YesPlease
 		NO_MKSTEMPS = YesPlease
-		NO_SYMLINK_HEAD = YesPlease
 		NO_IPV6 = YesPlease
 		OLD_ICONV = UnfortunatelyYes
 		# There are conflicting reports about this.
@@ -338,7 +337,6 @@ ifeq ($(uname_S),Windows)
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
-	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
 	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
@@ -491,7 +489,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
-	NO_SYMLINK_HEAD = YesPlease
 	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
 	NO_STRCASESTR = YesPlease
diff --git a/configure.ac b/configure.ac
index 89e2590..cad5418 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1096,9 +1096,6 @@ GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
 # Define USE_PIC if you need the main git objects to be built with -fPIC
 # in order to build and link perl/Git.so.  x86-64 seems to need this.
 #
-# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
-# Enable it on Windows.  By default, symrefs are still used.
-#
 # Define NO_PTHREADS if we do not have pthreads.
 #
 # Define PTHREAD_LIBS to the linker flag used for Pthread support.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6956807..31d517d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2046,7 +2046,6 @@ _git_config ()
 		core.packedGitLimit
 		core.packedGitWindowSize
 		core.pager
-		core.preferSymlinkRefs
 		core.preloadindex
 		core.quotepath
 		core.repositoryFormatVersion
diff --git a/environment.c b/environment.c
index 2da7fe2..80a1c0c 100644
--- a/environment.c
+++ b/environment.c
@@ -19,7 +19,6 @@ int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;
 int ignore_case;
 int assume_unchanged;
-int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 22b7c11..6e19953 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2811,21 +2811,6 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-static int create_ref_symlink(struct ref_lock *lock, const char *target)
-{
-	int ret = -1;
-#ifndef NO_SYMLINK_HEAD
-	char *ref_path = get_locked_file_path(lock->lk);
-	unlink(ref_path);
-	ret = symlink(target, ref_path);
-	free(ref_path);
-
-	if (ret)
-		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
-#endif
-	return ret;
-}
-
 static void update_symref_reflog(struct ref_lock *lock, const char *refname,
 				 const char *target, const char *logmsg)
 {
@@ -2841,11 +2826,6 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,
 static int create_symref_locked(struct ref_lock *lock, const char *refname,
 				const char *target, const char *logmsg)
 {
-	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
-		update_symref_reflog(lock, refname, target, logmsg);
-		return 0;
-	}
-
 	if (!fdopen_lock_file(lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
 			     lock->lk->tempfile.filename.buf, strerror(errno));
@@ -2860,27 +2840,12 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,
 	return 0;
 }
 
-static const char symlink_deprecation_warning[] =
-"The core.preferSymlinkRefs configuration option has been\n"
-"deprecated and will be removed in a future version of Git. If your\n"
-"workflow or script depends on '.git/HEAD' being a symbolic link,\n"
-"it should be adjusted to use:\n"
-"\n"
-"        git rev-parse HEAD\n"
-"\n"
-"        git rev-parse --symbolic-full-name HEAD\n"
-"\n"
-"to get the sha1 or branch name, respectively.";
-
 int create_symref(const char *refname, const char *target, const char *logmsg)
 {
 	struct strbuf err = STRBUF_INIT;
 	struct ref_lock *lock;
 	int ret;
 
-	if (prefer_symlink_refs)
-		warning("%s", symlink_deprecation_warning);
-
 	lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
 				   &err);
 	if (!lock) {
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 8859236..715dc00 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -427,18 +427,6 @@ test_expect_success 'checkout w/--track from tag fails' '
     test "z$(git rev-parse master^0)" = "z$(git rev-parse HEAD)"
 '
 
-test_expect_success 'detach a symbolic link HEAD' '
-    git checkout master &&
-    git config --bool core.prefersymlinkrefs yes &&
-    git checkout side &&
-    git checkout master &&
-    it=$(git symbolic-ref HEAD) &&
-    test "z$it" = zrefs/heads/master &&
-    here=$(git rev-parse --verify refs/heads/master) &&
-    git checkout side^ &&
-    test "z$(git rev-parse --verify refs/heads/master)" = "z$here"
-'
-
 test_expect_success \
     'checkout with --track fakes a sensible -b <name>' '
     git config remote.origin.fetch "+refs/heads/*:refs/remotes/origin/*" &&
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index af82049..25066f35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -46,15 +46,6 @@ test_expect_success 'prompt - branch name' '
 	test_cmp expected "$actual"
 '
 
-test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '
-	printf " (master)" >expected &&
-	test_when_finished "git checkout master" &&
-	test_config core.preferSymlinkRefs true &&
-	git checkout master &&
-	__git_ps1 >"$actual" &&
-	test_cmp expected "$actual"
-'
-
 test_expect_success 'prompt - unborn branch' '
 	printf " (unborn)" >expected &&
 	git checkout --orphan unborn &&
-- 
2.7.0.rc3.367.g09631da

Re: [PATCH v2 0/3] improve symbolic-ref robustness

[Jeff King]

On Tue, Dec 29, 2015 at 01:24:42PM -0800, Junio C Hamano wrote:

> > The patches are:
> >
> >   [1/3]: create_symref: modernize variable names
> >   [2/3]: create_symref: use existing ref-lock code
> >   [3/3]: create_symref: write reflog while holding lock
> 
> This is queued as an early part of 'pu', and some refactoring in
> David's refs-backend-lmdb topic conflicts with it when merged to
> 'pu'.  I think I resolved the conflicts correctly, but please double
> check the result when I push it out later today.

Looks sane to me. It seems like the change was just the renaming of the
functions. We _could_ match the renaming of the static-local functions
(e.g., create_symref_locked would become files_create_symref_locked),
but I don't think it's necessary for file-local functions.

-Peff