Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Michael Haggerty wrote:

> One of the tricks that `contrib/workdir/git-new-workdir` plays is to
> making `packed-refs` in the new workdir a symlink to the `packed-refs`
> file in the original repository. Before
> 42dfa7ecef ("commit_packed_refs(): use a staging file separate from
> the lockfile", 2017-06-23), a lockfile was used as the staging file,
> and because the `LOCK_NO_DEREF` was not used, the pointed-to file was
> locked and modified.
>
> But after that commit, the staging file was created using a tempfile,
> with the end result that rewriting the `packed-refs` file in the
> workdir overwrote the symlink rather than the original `packed-refs`
> file.
>
> Change `commit_packed_refs()` to use `get_locked_file_path()` to find
> the path of the file that it should overwrite. Since that path was
> properly resolved when the lockfile was created, this restores the
> pre-42dfa7ecef behavior.
>
> Also add a test case to document this use case and prevent a
> regression like this from recurring.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Reported-by: Dave Walker <dawalker@google.com>

[...]
>  refs/packed-backend.c | 24 ++++++++++++++++++------
>  t/t3210-pack-refs.sh  | 15 +++++++++++++++
>  2 files changed, 33 insertions(+), 6 deletions(-)

The patch looks good, except for one nit marked below (*).  I'll apply
it locally and ask people to test it, probably tomorrow morning.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for writing it.  Patch left unsnipped for reference.

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index a28befbfa3..59e7d1a509 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
>  	struct packed_ref_cache *packed_ref_cache =
>  		get_packed_ref_cache(refs);
>  	int ok;
> +	int ret = -1;
>  	struct strbuf sb = STRBUF_INIT;
>  	FILE *out;
>  	struct ref_iterator *iter;
> +	char *packed_refs_path;
>  
>  	if (!is_lock_file_locked(&refs->lock))
>  		die("BUG: commit_packed_refs() called when unlocked");
>  
> -	strbuf_addf(&sb, "%s.new", refs->path);
> +	/*
> +	 * If packed-refs is a symlink, we want to overwrite the
> +	 * symlinked-to file, not the symlink itself. Also, put the
> +	 * staging file next to it:
> +	 */
> +	packed_refs_path = get_locked_file_path(&refs->lock);
> +	strbuf_addf(&sb, "%s.new", packed_refs_path);
>  	if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
>  		strbuf_addf(err, "unable to create file %s: %s",
>  			    sb.buf, strerror(errno));
>  		strbuf_release(&sb);
> -		return -1;
> +		goto out;
>  	}
>  	strbuf_release(&sb);
>  
> @@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
>  		goto error;
>  	}
>  
> -	if (rename_tempfile(&refs->tempfile, refs->path)) {
> +	if (rename_tempfile(&refs->tempfile, packed_refs_path)) {
>  		strbuf_addf(err, "error replacing %s: %s",
>  			    refs->path, strerror(errno));
> -		return -1;
> +		goto out;
>  	}
>  
> -	return 0;
> +	ret = 0;
> +	goto out;
>  
>  error:
>  	delete_tempfile(&refs->tempfile);
> -	return -1;
> +
> +out:
> +	free(packed_refs_path);
> +	return ret;
>  }
>  
>  /*
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 2bb4b25ed9..0d8a03e2a9 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '
>  	git -c core.packedrefstimeout=3000 pack-refs --all --prune
>  '
>  
> +test_expect_success 'pack symlinked packed-refs' '

(*) Does this need a SYMLINKS prereq to avoid trouble on Windows?

> +	# First make sure that symlinking works when reading:
> +	git update-ref refs/heads/loosy refs/heads/master &&
> +	git for-each-ref >all-refs-before &&
> +	mv .git/packed-refs .git/my-deviant-packed-refs &&
> +	ln -s my-deviant-packed-refs .git/packed-refs &&
> +	git for-each-ref >all-refs-linked &&
> +	test_cmp all-refs-before all-refs-linked &&
> +	git pack-refs --all --prune &&
> +	git for-each-ref >all-refs-packed &&
> +	test_cmp all-refs-before all-refs-packed &&
> +	test -h .git/packed-refs &&
> +	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +'
> +
>  test_done