Re: [PATCH v2] object-file: use real paths when adding alternates

[Jeff King <peff@peff.net>]

On Mon, Nov 21, 2022 at 11:49:17PM +0000, Glen Choo via GitGitGadget wrote:

>     Thanks for the feedback on v1. This version takes nearly all of Peff's
>     patch [1] except for the comment about making an exception for relative
>     paths in the environment. My reading of the commit [2] is that it was a
>     workaround for strbuf_normalize_path() not being able to handle relative
>     paths, so the only reason to special-case the environment is to preserve
>     the behavior of respecting broken paths, which (unlike relative paths) I
>     don't think will be missed.

Yeah, that makes sense. If realpath fails because a path isn't present,
then we would throw it away anyway. So we don't need to quietly
tolerate, unless we really care about the difference between reporting
"this directory doesn't seem to exist" versus "I couldn't run realpath
on this directory". One is a subset of the other.

> diff --git a/object-file.c b/object-file.c
> index 957790098fa..ef2b762234d 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
>  {
>  	struct object_directory *ent;
>  	struct strbuf pathbuf = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
>  	khiter_t pos;
>  
>  	if (!is_absolute_path(entry->buf) && relative_base) {
> @@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
>  	}
>  	strbuf_addbuf(&pathbuf, entry);
>  
> -	if (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {
> +	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
>  		error(_("unable to normalize alternate object path: %s"),
> -		      pathbuf.buf);
> +			pathbuf.buf);
>  		strbuf_release(&pathbuf);
>  		return -1;
>  	}
> +	strbuf_swap(&pathbuf, &tmp);
> +	strbuf_release(&tmp);

So here we are looking at an alternates entry (either from a file or
from the environment). We do note all errors, even in relative ones from
the environment, but we don't die, so we'll just ignore the failed
alternate. Good.

> @@ -596,10 +599,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
>  		return;
>  	}
>  
> -	strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);
> -	if (strbuf_normalize_path(&objdirbuf) < 0)
> -		die(_("unable to normalize object directory: %s"),
> -		    objdirbuf.buf);
> +	strbuf_realpath(&objdirbuf, r->objects->odb->path, 1);

And here we are resolving the actual object directory, and we always
died if that couldn't be normalized. And we'll continue to do so by
realpath. Good.

> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh

And then that's all we needed in the C code, since we already do
duplicate checks. Good. :)

> index 5be483bf887..ce1954d0977 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '
>  	test_has_duplicate_object false
>  '
>  
> +test_expect_success '--local keeps packs when alternate is objectdir ' '
> +	git init alt_symlink &&
> +	(
> +		cd alt_symlink &&
> +		git init &&
> +		echo content >file4 &&
> +		git add file4 &&
> +		git commit -m commit_file4 &&
> +		git repack -a &&
> +		ls .git/objects/pack/*.pack >../expect &&
> +		ln -s objects .git/alt_objects &&
> +		echo "$(pwd)/.git/alt_objects" >.git/objects/info/alternates &&
> +		git repack -a -d -l &&
> +		ls .git/objects/pack/*.pack >../actual
> +	) &&
> +	test_cmp expect actual
> +'

This probably needs to be protected with a SYMLINKS prereq.

-Peff