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

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

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

When adding an alternate ODB, we check if the alternate has the same
path as the object dir, and if so, we do nothing. However, that
comparison does not resolve symlinks. This makes it possible to add the
object dir as an alternate, which may result in bad behavior. For
example, it can trick "git repack -a -l -d" (possibly run by "git gc")
into thinking that all packs come from an alternate and delete all
objects.

	rm -rf test &&
	git clone https://github.com/git/git test &&
	(
	cd test &&
	ln -s objects .git/alt-objects &&
	# -c repack.updateserverinfo=false silences a warning about not
	# being able to update "info/refs", it isn't needed to show the
	# bad behavior
	GIT_ALTERNATE_OBJECT_DIRECTORIES=".git/alt-objects" git \
		-c repack.updateserverinfo=false repack -a -l -d  &&
	# It's broken!
	git status
	# Because there are no more objects!
	ls .git/objects/pack
	)

Fix this by resolving symlinks before comparing the alternate and object
dir.

Signed-off-by: Glen Choo <chooglen@google.com>
---
    object-file: use real paths when adding alternates
    
    Here's a bug that got uncovered because of some oddities in how "repo"
    [1] manages its object directories. With some tracing, I'm quite certain
    that the mechanism is that the packs are treated as non-local, but I
    don't understand "git repack" extremely well, so e.g. the test I added
    seems pretty crude.
    
    [1] https://gerrit.googlesource.com/git-repo

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1382%2Fchooglen%2Fobject-file%2Fcheck-alternate-real-path-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1382/chooglen/object-file/check-alternate-real-path-v1
Pull-Request: https://github.com/git/git/pull/1382

 object-file.c     | 17 ++++++++++++-----
 t/t7700-repack.sh | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/object-file.c b/object-file.c
index 957790098fa..f901dd272d1 100644
--- a/object-file.c
+++ b/object-file.c
@@ -455,14 +455,16 @@ static int alt_odb_usable(struct raw_object_store *o,
 			  struct strbuf *path,
 			  const char *normalized_objdir, khiter_t *pos)
 {
+	int ret = 0;
 	int r;
+	struct strbuf real_path = STRBUF_INIT;
 
 	/* Detect cases where alternate disappeared */
 	if (!is_directory(path->buf)) {
 		error(_("object directory %s does not exist; "
 			"check .git/objects/info/alternates"),
 		      path->buf);
-		return 0;
+		goto cleanup;
 	}
 
 	/*
@@ -478,11 +480,16 @@ static int alt_odb_usable(struct raw_object_store *o,
 		assert(r == 1); /* never used */
 		kh_value(o->odb_by_path, p) = o->odb;
 	}
-	if (fspatheq(path->buf, normalized_objdir))
-		return 0;
+
+	strbuf_realpath(&real_path, path->buf, 1);
+	if (fspatheq(real_path.buf, normalized_objdir))
+		goto cleanup;
 	*pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r);
 	/* r: 0 = exists, 1 = never used, 2 = deleted */
-	return r == 0 ? 0 : 1;
+	ret = r == 0 ? 0 : 1;
+ cleanup:
+	strbuf_release(&real_path);
+	return ret;
 }
 
 /*
@@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
 		return;
 	}
 
-	strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);
+	strbuf_realpath(&objdirbuf, r->objects->odb->path, 1);
 	if (strbuf_normalize_path(&objdirbuf) < 0)
 		die(_("unable to normalize object directory: %s"),
 		    objdirbuf.buf);
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
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
+'
+
 test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '
 	mkdir alt_objects/pack &&
 	mv .git/objects/pack/* alt_objects/pack &&

base-commit: 319605f8f00e402f3ea758a02c63534ff800a711
-- 
gitgitgadget

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

[Jeff King]

On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:

> When adding an alternate ODB, we check if the alternate has the same
> path as the object dir, and if so, we do nothing. However, that
> comparison does not resolve symlinks. This makes it possible to add the
> object dir as an alternate, which may result in bad behavior. For
> example, it can trick "git repack -a -l -d" (possibly run by "git gc")
> into thinking that all packs come from an alternate and delete all
> objects.

I think we do attempt to normalize the names. In link_alt_odb_entries(),
we call strbuf_normalize_path() on the base object directory, and then
in link_alt_odb_entry(), we similarly normalize the proposed odb names.
And then we drop duplicates (either of other alternates, or of the main
object directory).

So it seems like the problem is that "normalize" is not enough here,
because it only normalizes the text. We need to be using
strbuf_realpath() which will actually evaluate symbolic links.

> diff --git a/object-file.c b/object-file.c
> index 957790098fa..f901dd272d1 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -455,14 +455,16 @@ static int alt_odb_usable(struct raw_object_store *o,
>  			  struct strbuf *path,
>  			  const char *normalized_objdir, khiter_t *pos)
>  {
> +	int ret = 0;
>  	int r;
> +	struct strbuf real_path = STRBUF_INIT;
>  
>  	/* Detect cases where alternate disappeared */
>  	if (!is_directory(path->buf)) {
>  		error(_("object directory %s does not exist; "
>  			"check .git/objects/info/alternates"),
>  		      path->buf);
> -		return 0;
> +		goto cleanup;
>  	}

This goto seems unnecessary; we know we haven't touched real_path yet,
so there is nothing to clean. But...

> @@ -478,11 +480,16 @@ static int alt_odb_usable(struct raw_object_store *o,
>  		assert(r == 1); /* never used */
>  		kh_value(o->odb_by_path, p) = o->odb;
>  	}
> -	if (fspatheq(path->buf, normalized_objdir))
> -		return 0;
> +
> +	strbuf_realpath(&real_path, path->buf, 1);
> +	if (fspatheq(real_path.buf, normalized_objdir))
> +		goto cleanup;
>  	*pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r);
>  	/* r: 0 = exists, 1 = never used, 2 = deleted */
> -	return r == 0 ? 0 : 1;
> +	ret = r == 0 ? 0 : 1;
> + cleanup:
> +	strbuf_release(&real_path);
> +	return ret;
>  }

This seems like the wrong place to be doing realpath. We've already
normalized earlier in link_alt_odb_entry(). Why not use realpath there?

It does mean we'd end up with the realpath in the object_directory
struct, but I don't see that as a bad thing (in fact, it may be slightly
faster if it saves the kernel crossing a symlink boundary).

I do suspect it would change the error message for missing intermediate
directories (since realpath would complain, before we even hit the "does
this even exist"), but it's an error in both cases. Which brings up
another point here: I think right now we treat those errors as warnings,
and cnotinue on without the alternate available (because we ignore the
result of link_alt_odb_entry). But with your patch, a failure in this
realpath would cause us to die immediately.

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

Similarly here, I think we'd want to _replace_ the normalize with a
realpath. There's no point in doing both. It's OK to die in this one
because we assume the object directory can be normalized/realpath'd.

So I'd have expected the code portion of your patch to be more like:

diff --git a/object-file.c b/object-file.c
index 957790098f..c6a195c6dd 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,18 @@ 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) {
-		error(_("unable to normalize alternate object path: %s"),
-		      pathbuf.buf);
-		strbuf_release(&pathbuf);
-		return -1;
+	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
+		if (relative_base) {
+			error(_("unable to normalize alternate object path: %s"),
+			      pathbuf.buf);
+			strbuf_release(&pathbuf);
+			return -1;
+		}
+		/* allow broken paths from env per 37a95862c625 */
+		strbuf_addstr(&tmp, pathbuf.buf);
 	}
+	strbuf_swap(&pathbuf, &tmp);
+	strbuf_release(&tmp);
 
 	/*
 	 * The trailing slash after the directory name is given by
@@ -596,10 +603,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);
 
 	while (*alt) {
 		alt = parse_alt_odb_entry(alt, sep, &entry);

The "tmp" swapping in link_alt_odb_entry is kind of unfortunate. It
would be nice if there were an in-place version of strbuf_realpath, even
if it was using two buffers under the hood (which is how the normalize
code does it). And then the patch really would be s/normalize/realpath/,
which is easier to understand.

Possibly this should also be using the "forgiving" version. We
eventually error out on missing entries later on, so it's not a big deal
to error here. But it would let us keep the error message the same. I
don't know that it matters much in practice.

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 17 2022, Jeff King wrote:

> On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:
> [...]
>> @@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
>>  		return;
>>  	}
>>  
>> -	strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);
>> +	strbuf_realpath(&objdirbuf, r->objects->odb->path, 1);
>>  	if (strbuf_normalize_path(&objdirbuf) < 0)
>>  		die(_("unable to normalize object directory: %s"),
>>  		    objdirbuf.buf);
>
> Similarly here, I think we'd want to _replace_ the normalize with a
> realpath. There's no point in doing both. It's OK to die in this one
> because we assume the object directory can be normalized/realpath'd.
>
> So I'd have expected the code portion of your patch to be more like:
>
> diff --git a/object-file.c b/object-file.c
> index 957790098f..c6a195c6dd 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,18 @@ 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) {
> -		error(_("unable to normalize alternate object path: %s"),
> -		      pathbuf.buf);
> -		strbuf_release(&pathbuf);
> -		return -1;
> +	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
> +		if (relative_base) {
> +			error(_("unable to normalize alternate object path: %s"),
> +			      pathbuf.buf);
> +			strbuf_release(&pathbuf);
> +			return -1;
> +		}
> +		/* allow broken paths from env per 37a95862c625 */
> +		strbuf_addstr(&tmp, pathbuf.buf);
>  	}
> +	strbuf_swap(&pathbuf, &tmp);
> +	strbuf_release(&tmp);
>  
>  	/*
>  	 * The trailing slash after the directory name is given by
> @@ -596,10 +603,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);
>  
>  	while (*alt) {
>  		alt = parse_alt_odb_entry(alt, sep, &entry);
>
> The "tmp" swapping in link_alt_odb_entry is kind of unfortunate. It
> would be nice if there were an in-place version of strbuf_realpath, even
> if it was using two buffers under the hood (which is how the normalize
> code does it). And then the patch really would be s/normalize/realpath/,
> which is easier to understand.
>
> Possibly this should also be using the "forgiving" version. We
> eventually error out on missing entries later on, so it's not a big deal
> to error here. But it would let us keep the error message the same. I
> don't know that it matters much in practice.

This probably isn't worth it, but I wondered if this wouldn't be easier
if we pulled that memory management into the caller, it's not
performance sensitive (or maybe, how many alternatives do people have
:)), but an advantage of this is that we avoid the free()/malloc() if we
only get partway through, i.e. return early and keep looping.

In terms of general code smell & how we manage the "return" here, as
adding "RESULT_MUST_BE_USED" to this shows we never use the "0" or "-1"
(or any other...) return value.

That's been the case since this was added in c2f493a4ae1 (Transitively
read alternatives, 2006-05-07), so we can probably just make this a
"void" and ditch the returns if we're finding ourselves juggling these
return values...

 object-file.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/object-file.c b/object-file.c
index c6a195c6dd2..1a94d98e0c7 100644
--- a/object-file.c
+++ b/object-file.c
@@ -504,47 +504,43 @@ static void read_info_alternates(struct repository *r,
 				 const char *relative_base,
 				 int depth);
 static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
-	const char *relative_base, int depth, const char *normalized_objdir)
+			      const char *relative_base, int depth,
+			      const char *normalized_objdir,
+			      struct strbuf *pathbuf)
 {
 	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) {
-		strbuf_realpath(&pathbuf, relative_base, 1);
-		strbuf_addch(&pathbuf, '/');
+		strbuf_realpath(pathbuf, relative_base, 1);
+		strbuf_addch(pathbuf, '/');
 	}
-	strbuf_addbuf(&pathbuf, entry);
+	strbuf_addbuf(pathbuf, entry);
 
-	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
-		if (relative_base) {
-			error(_("unable to normalize alternate object path: %s"),
-			      pathbuf.buf);
-			strbuf_release(&pathbuf);
-			return -1;
-		}
+	if (!strbuf_realpath(&tmp, pathbuf->buf, 0)) {
+		if (relative_base)
+			return error(_("unable to normalize alternate object path: %s"),
+				     pathbuf->buf);
 		/* allow broken paths from env per 37a95862c625 */
-		strbuf_addstr(&tmp, pathbuf.buf);
+		strbuf_addstr(&tmp, pathbuf->buf);
 	}
-	strbuf_swap(&pathbuf, &tmp);
+	strbuf_swap(pathbuf, &tmp);
 	strbuf_release(&tmp);
 
 	/*
 	 * The trailing slash after the directory name is given by
 	 * this function at the end. Remove duplicates.
 	 */
-	while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
-		strbuf_setlen(&pathbuf, pathbuf.len - 1);
+	while (pathbuf->len && pathbuf->buf[pathbuf->len - 1] == '/')
+		strbuf_setlen(pathbuf, pathbuf->len - 1);
 
-	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {
-		strbuf_release(&pathbuf);
+	if (!alt_odb_usable(r->objects, pathbuf, normalized_objdir, &pos))
 		return -1;
-	}
 
 	CALLOC_ARRAY(ent, 1);
-	/* pathbuf.buf is already in r->objects->odb_by_path */
-	ent->path = strbuf_detach(&pathbuf, NULL);
+	/* pathbuf->buf is already in r->objects->odb_by_path */
+	ent->path = strbuf_detach(pathbuf, NULL);
 
 	/* add the alternate entry */
 	*r->objects->odb_tail = ent;
@@ -593,6 +589,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
 {
 	struct strbuf objdirbuf = STRBUF_INIT;
 	struct strbuf entry = STRBUF_INIT;
+	struct strbuf pathbuf = STRBUF_INIT;
 
 	if (!alt || !*alt)
 		return;
@@ -610,8 +607,11 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
 		if (!entry.len)
 			continue;
 		link_alt_odb_entry(r, &entry,
-				   relative_base, depth, objdirbuf.buf);
+				   relative_base, depth, objdirbuf.buf,
+				   &pathbuf);
+		strbuf_reset(&pathbuf);
 	}
+	strbuf_release(&pathbuf);
 	strbuf_release(&entry);
 	strbuf_release(&objdirbuf);
 }

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

[Taylor Blau]

On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:
> From: Glen Choo <chooglen@google.com>
>
> When adding an alternate ODB, we check if the alternate has the same
> path as the object dir, and if so, we do nothing. However, that
> comparison does not resolve symlinks. This makes it possible to add the
> object dir as an alternate, which may result in bad behavior. For
> example, it can trick "git repack -a -l -d" (possibly run by "git gc")
> into thinking that all packs come from an alternate and delete all
> objects.

Nice find and fix. Looks like Peff had a couple of good suggestions
which I'd like to see incorporated into another version before we pick
this up.

Otherwise, looking at the patch myself, it seems obviously good. Thanks
for working on it.


Thanks,
Taylor

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

[Jeff King]

On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:

> This probably isn't worth it, but I wondered if this wouldn't be easier
> if we pulled that memory management into the caller, it's not
> performance sensitive (or maybe, how many alternatives do people have
> :)), but an advantage of this is that we avoid the free()/malloc() if we
> only get partway through, i.e. return early and keep looping.

I agree with your "probably". This isn't worth it to save a malloc in a
very non-hot code path. If the two bits of code were otherwise equal
(say, reset-ing a buffer used directly in a loop) I might say "why
not?". But crossing a function boundary to me introduces way too many
questions in somebody reading the code (like "is pathbuf supposed to
have something in it?") to make it worth doing here.

But even if we did want to do it, see below.

> In terms of general code smell & how we manage the "return" here, as
> adding "RESULT_MUST_BE_USED" to this shows we never use the "0" or "-1"
> (or any other...) return value.
> 
> That's been the case since this was added in c2f493a4ae1 (Transitively
> read alternatives, 2006-05-07), so we can probably just make this a
> "void" and ditch the returns if we're finding ourselves juggling these
> return values...

Yeah, we could ditch the return values. In a sense they are at least
documenting how link_alt_odb_entry() sees the world, but if nobody looks
at them, I'd be OK dropping them to make it clear that we don't intend
to ever act on them.

That said, both of these are orthogonal to what Glen's patches are
doing. If you want to submit a series later to deal with them, OK, but
let's try not to hijack the conversation for patches that are fixing a
real bug.

-Peff

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

[Taylor Blau]

On Thu, Nov 17, 2022 at 04:57:26PM -0500, Jeff King wrote:
> That said, both of these are orthogonal to what Glen's patches are
> doing. If you want to submit a series later to deal with them, OK, but
> let's try not to hijack the conversation for patches that are fixing a
> real bug.

Thanks for keeping us on track.

Thanks,
Taylor

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

[Glen Choo]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> This probably isn't worth it, but I wondered if this wouldn't be easier
>> if we pulled that memory management into the caller [...]
> [......] But crossing a function boundary to me introduces way too many
> questions in somebody reading the code (like "is pathbuf supposed to
> have something in it?") to make it worth doing here.

Thanks, both :)

I think the loss of readability is enough for me to hold off on this
suggestion, but I don't mind reviewing cleanup patches on top of this.

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

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

When adding an alternate ODB, we check if the alternate has the same
path as the object dir, and if so, we do nothing. However, that
comparison does not resolve symlinks. This makes it possible to add the
object dir as an alternate, which may result in bad behavior. For
example, it can trick "git repack -a -l -d" (possibly run by "git gc")
into thinking that all packs come from an alternate and delete all
objects.

	rm -rf test &&
	git clone https://github.com/git/git test &&
	(
	cd test &&
	ln -s objects .git/alt-objects &&
	# -c repack.updateserverinfo=false silences a warning about not
	# being able to update "info/refs", it isn't needed to show the
	# bad behavior
	GIT_ALTERNATE_OBJECT_DIRECTORIES=".git/alt-objects" git \
		-c repack.updateserverinfo=false repack -a -l -d  &&
	# It's broken!
	git status
	# Because there are no more objects!
	ls .git/objects/pack
	)

Fix this by resolving symlinks and relative paths before comparing the
alternate and object dir. This lets us clean up a number of issues noted
in 37a95862c6 (alternates: re-allow relative paths from environment,
2016-11-07):

- Now that we compare the real paths, duplicate detection is no longer
  foiled by relative paths.
- Using strbuf_realpath() allows us to "normalize" paths that
  strbuf_normalize_path() can't, so we can stop silently ignoring errors
  when "normalizing" paths from the environment.
- We now store an absolute path based on getcwd() (the "future
  direction" named in 37a95862c6), so chdir()-ing in the process no
  longer changes the directory pointed to by the alternate. This is a
  change in behavior, but a desirable one.

Signed-off-by: Glen Choo <chooglen@google.com>
---
    object-file: use real paths when adding alternates
    
    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.
    
    Changes in v2:
    
     * Do realpath when storing the alternate's directory entry instead of
       only during the usability check.
     * Update commit message to reflect the relationship to [2]
    
    [1] https://lore.kernel.org/git/Y3aBzbzub7flQyca@coredump.intra.peff.net
    [2] 37a95862c6 (alternates: re-allow relative paths from environment,
    2016-11-07)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1382%2Fchooglen%2Fobject-file%2Fcheck-alternate-real-path-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1382/chooglen/object-file/check-alternate-real-path-v2
Pull-Request: https://github.com/git/git/pull/1382

Range-diff vs v1:

 1:  9392725ad01 ! 1:  ed9e12c0051 object-file: use real paths when adding alternates
     @@ Commit message
                  ls .git/objects/pack
                  )
      
     -    Fix this by resolving symlinks before comparing the alternate and object
     -    dir.
     +    Fix this by resolving symlinks and relative paths before comparing the
     +    alternate and object dir. This lets us clean up a number of issues noted
     +    in 37a95862c6 (alternates: re-allow relative paths from environment,
     +    2016-11-07):
     +
     +    - Now that we compare the real paths, duplicate detection is no longer
     +      foiled by relative paths.
     +    - Using strbuf_realpath() allows us to "normalize" paths that
     +      strbuf_normalize_path() can't, so we can stop silently ignoring errors
     +      when "normalizing" paths from the environment.
     +    - We now store an absolute path based on getcwd() (the "future
     +      direction" named in 37a95862c6), so chdir()-ing in the process no
     +      longer changes the directory pointed to by the alternate. This is a
     +      change in behavior, but a desirable one.
      
          Signed-off-by: Glen Choo <chooglen@google.com>
      
       ## object-file.c ##
     -@@ object-file.c: static int alt_odb_usable(struct raw_object_store *o,
     - 			  struct strbuf *path,
     - 			  const char *normalized_objdir, khiter_t *pos)
     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
       {
     -+	int ret = 0;
     - 	int r;
     -+	struct strbuf real_path = STRBUF_INIT;
     + 	struct object_directory *ent;
     + 	struct strbuf pathbuf = STRBUF_INIT;
     ++	struct strbuf tmp = STRBUF_INIT;
     + 	khiter_t pos;
       
     - 	/* Detect cases where alternate disappeared */
     - 	if (!is_directory(path->buf)) {
     - 		error(_("object directory %s does not exist; "
     - 			"check .git/objects/info/alternates"),
     - 		      path->buf);
     --		return 0;
     -+		goto cleanup;
     + 	if (!is_absolute_path(entry->buf) && relative_base) {
     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
       	}
     + 	strbuf_addbuf(&pathbuf, entry);
       
     - 	/*
     -@@ object-file.c: static int alt_odb_usable(struct raw_object_store *o,
     - 		assert(r == 1); /* never used */
     - 		kh_value(o->odb_by_path, p) = o->odb;
     +-	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;
       	}
     --	if (fspatheq(path->buf, normalized_objdir))
     --		return 0;
     -+
     -+	strbuf_realpath(&real_path, path->buf, 1);
     -+	if (fspatheq(real_path.buf, normalized_objdir))
     -+		goto cleanup;
     - 	*pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r);
     - 	/* r: 0 = exists, 1 = never used, 2 = deleted */
     --	return r == 0 ? 0 : 1;
     -+	ret = r == 0 ? 0 : 1;
     -+ cleanup:
     -+	strbuf_release(&real_path);
     -+	return ret;
     - }
     ++	strbuf_swap(&pathbuf, &tmp);
     ++	strbuf_release(&tmp);
       
     - /*
     + 	/*
     + 	 * The trailing slash after the directory name is given by
      @@ object-file.c: 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);
     - 	if (strbuf_normalize_path(&objdirbuf) < 0)
     - 		die(_("unable to normalize object directory: %s"),
     - 		    objdirbuf.buf);
     + 
     + 	while (*alt) {
     + 		alt = parse_alt_odb_entry(alt, sep, &entry);
      
       ## t/t7700-repack.sh ##
      @@ t/t7700-repack.sh: test_expect_success 'loose objects in alternate ODB are not repacked' '


 object-file.c     | 12 ++++++------
 t/t7700-repack.sh | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

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);
 
 	/*
 	 * The trailing slash after the directory name is given by
@@ -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);
 
 	while (*alt) {
 		alt = parse_alt_odb_entry(alt, sep, &entry);
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
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
+'
+
 test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '
 	mkdir alt_objects/pack &&
 	mv .git/objects/pack/* alt_objects/pack &&

base-commit: 319605f8f00e402f3ea758a02c63534ff800a711
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 21 2022, Glen Choo via GitGitGadget wrote:

> From: Glen Choo <chooglen@google.com>

Aside from some small nits this looks good to me.

>  object-file.c     | 12 ++++++------
>  t/t7700-repack.sh | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+), 6 deletions(-)
>
> 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);

This is a mis-indentation, it was OK in the pre-image, not now.

>  		strbuf_release(&pathbuf);

Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
"REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
&tmp you pass in here) and then "does a "goto error_out".

It then *resets* the strbuf, but doesn't release it, assuming that
you're going to pass it in again. So in that case we'd leak here, no?

I.e. a NULL return value from strbuf_realpath() doesn't mean that it
didn't allocate in the scratch area passed to it, so we need to
strbuf_release(&tmp) here too.

Perhaps this on top is simpler (but also see below)?:
	
	diff --git a/object-file.c b/object-file.c
	index ef2b762234d..d5d502504bb 100644
	--- a/object-file.c
	+++ b/object-file.c
	@@ -510,6 +510,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	struct strbuf pathbuf = STRBUF_INIT;
	 	struct strbuf tmp = STRBUF_INIT;
	 	khiter_t pos;
	+	int ret = -1;
	 
	 	if (!is_absolute_path(entry->buf) && relative_base) {
	 		strbuf_realpath(&pathbuf, relative_base, 1);
	@@ -520,11 +521,9 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
	 		error(_("unable to normalize alternate object path: %s"),
	 			pathbuf.buf);
	-		strbuf_release(&pathbuf);
	-		return -1;
	+		goto error;
	 	}
	 	strbuf_swap(&pathbuf, &tmp);
	-	strbuf_release(&tmp);
	 
	 	/*
	 	 * The trailing slash after the directory name is given by
	@@ -533,10 +532,8 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
	 		strbuf_setlen(&pathbuf, pathbuf.len - 1);
	 
	-	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {
	-		strbuf_release(&pathbuf);
	-		return -1;
	-	}
	+	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))
	+		goto error;
	 
	 	CALLOC_ARRAY(ent, 1);
	 	/* pathbuf.buf is already in r->objects->odb_by_path */
	@@ -552,7 +549,11 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	/* recursively add alternates */
	 	read_info_alternates(r, ent->path, depth + 1);
	 
	-	return 0;
	+	ret = 0;
	+error:
	+	strbuf_release(&tmp);
	+	strbuf_release(&pathbuf);
	+	return ret;
	 }
	 
	 static const char *parse_alt_odb_entry(const char *string,
	
	

>  		return -1;
>  	}
> +	strbuf_swap(&pathbuf, &tmp);
> +	strbuf_release(&tmp);
>  
>  	/*
>  	 * The trailing slash after the directory name is given by
> @@ -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);
>  
>  	while (*alt) {
>  		alt = parse_alt_odb_entry(alt, sep, &entry);
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> 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 &&

The tests pass without this re-"git init", left over from development?

> +		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
> +'
> +

I think this is better squashed in:
	
	diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
	index ce1954d0977..79eef5b4aa7 100755
	--- a/t/t7700-repack.sh
	+++ b/t/t7700-repack.sh
	@@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '
	 '
	 
	 test_expect_success '--local keeps packs when alternate is objectdir ' '
	-	git init alt_symlink &&
	+	test_when_finished "rm -rf repo" &&
	+	git init repo &&
	+	test_commit -C repo A &&
	 	(
	-		cd alt_symlink &&
	-		git init &&
	-		echo content >file4 &&
	-		git add file4 &&
	-		git commit -m commit_file4 &&
	+		cd repo &&
	 		git repack -a &&
	 		ls .git/objects/pack/*.pack >../expect &&
	 		ln -s objects .git/alt_objects &&

Because:

 * If it's not a setup for a later test let's call it "repo" and clean
   it up at the end.

 * The "file4" you're creating doesn't go with the existing pattern, the
   file{1..3} are created in the top-level .git, here you're making a
   file4 in another repo.

   This just calls it "A.t", and makes it with test_commit, since all
   you need is a dummy commit.

 * I think we typically use "find .. -type f", not "ls", see
   e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think
   aside from that test there's some other "let's compare the packed
   before & after" in the test suite, but I can't remember offhand...

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

[Jeff King]

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

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

[Jeff King]

On Tue, Nov 22, 2022 at 01:56:09AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > @@ -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);
> 
> This is a mis-indentation, it was OK in the pre-image, not now.
> 
> >  		strbuf_release(&pathbuf);
> 
> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
> &tmp you pass in here) and then "does a "goto error_out".
> 
> It then *resets* the strbuf, but doesn't release it, assuming that
> you're going to pass it in again. So in that case we'd leak here, no?
> 
> I.e. a NULL return value from strbuf_realpath() doesn't mean that it
> didn't allocate in the scratch area passed to it, so we need to
> strbuf_release(&tmp) here too.

We don't use MANY_MISSING in this code path, but I didn't read
strbuf_realpath_1() carefully enough to see if that is the only case.
But regardless, I think it is a bug in strbuf_realpath(). All of the
strbuf functions generally try to leave a buffer untouched on error.

So IMHO we would want a preparatory patch with s/reset/release/ in that
function, which better matches the intent (we might be freeing an
allocated buffer, but that's OK from the caller perspective). In theory
it ought to just roll back the length for whatever it put into the
buffer, but it looks like the rest of the function is happy to clobber
what's in the buf, even on non-error. That's why we have
strbuf_add_real_path(), but of course it doesn't allow for setting the
die_on_error flag.

-Peff

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

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>  object-file.c     | 12 ++++++------
>>  t/t7700-repack.sh | 18 ++++++++++++++++++
>>  2 files changed, 24 insertions(+), 6 deletions(-)
>>
>> 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);
>
> This is a mis-indentation, it was OK in the pre-image, not now.

Strange, this came from "make style", and in the GitHub web UI, it shows
the next line as aligned with the opening ". Meh, I'll undo it.

> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
> &tmp you pass in here) and then "does a "goto error_out".
>
> It then *resets* the strbuf, but doesn't release it, assuming that
> you're going to pass it in again. So in that case we'd leak here, no?
>
> I.e. a NULL return value from strbuf_realpath() doesn't mean that it
> didn't allocate in the scratch area passed to it, so we need to
> strbuf_release(&tmp) here too.

Yeah, you're right. At any rate, it's a lot of cognitive overload to
check if strbuf_realpath() will or won't allocate, so free()-ing in the
caller makes sense.

Separately, Peff mentioned that strbuf_realpath() not free()-ing is a
real bug, but I'll leave that for a future cleanup.

>> +		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
>> +'
>> +
>
> I think this is better squashed in:
> 	
> 	diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> 	index ce1954d0977..79eef5b4aa7 100755
> 	--- a/t/t7700-repack.sh
> 	+++ b/t/t7700-repack.sh
> 	@@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '
> 	 '
> 	 
> 	 test_expect_success '--local keeps packs when alternate is objectdir ' '
> 	-	git init alt_symlink &&
> 	+	test_when_finished "rm -rf repo" &&
> 	+	git init repo &&
> 	+	test_commit -C repo A &&
> 	 	(
> 	-		cd alt_symlink &&
> 	-		git init &&
> 	-		echo content >file4 &&
> 	-		git add file4 &&
> 	-		git commit -m commit_file4 &&
> 	+		cd repo &&
> 	 		git repack -a &&
> 	 		ls .git/objects/pack/*.pack >../expect &&
> 	 		ln -s objects .git/alt_objects &&
>
> Because:
>
>  * If it's not a setup for a later test let's call it "repo" and clean
>    it up at the end.
>
>  * The "file4" you're creating doesn't go with the existing pattern, the
>    file{1..3} are created in the top-level .git, here you're making a
>    file4 in another repo.
>
>    This just calls it "A.t", and makes it with test_commit, since all
>    you need is a dummy commit.
>
>  * I think we typically use "find .. -type f", not "ls", see
>    e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think
>    aside from that test there's some other "let's compare the packed
>    before & after" in the test suite, but I can't remember offhand...

It seems like t7700-repack.sh itself isn't consistent either (which is
probably how I ended up with "ls"). I'll also leave it alone unless
someone has strong opinions.

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

[Glen Choo]

Jeff King <peff@peff.net> writes:

>> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
>> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
>> &tmp you pass in here) and then "does a "goto error_out".
>> 
>> It then *resets* the strbuf, but doesn't release it, assuming that
>> you're going to pass it in again. So in that case we'd leak here, no?
>> 
>> I.e. a NULL return value from strbuf_realpath() doesn't mean that it
>> didn't allocate in the scratch area passed to it, so we need to
>> strbuf_release(&tmp) here too.
>
> We don't use MANY_MISSING in this code path, but I didn't read
> strbuf_realpath_1() carefully enough to see if that is the only case.
> But regardless, I think it is a bug in strbuf_realpath(). All of the
> strbuf functions generally try to leave a buffer untouched on error.
>
> So IMHO we would want a preparatory patch with s/reset/release/ in that
> function, which better matches the intent (we might be freeing an
> allocated buffer, but that's OK from the caller perspective).

Is that always OK? I would think that we'd do something closer to
strbuf_getcwd():

  int strbuf_getcwd(struct strbuf *sb)
  {
    size_t oldalloc = sb->alloc;
    /* ... */
    if (oldalloc == 0)
      strbuf_release(sb);
    else
      strbuf_reset(sb);
  }

i.e. if the caller passed in a strbuf with allocated contents, they're
responsible for free()-ing it, otherwise we free() it. That does fix the
leak in this patch, but I don't feel strongly enough about changing
strbuf_realpath() to do it now, so I'll do without the change for now.

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

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

When adding an alternate ODB, we check if the alternate has the same
path as the object dir, and if so, we do nothing. However, that
comparison does not resolve symlinks. This makes it possible to add the
object dir as an alternate, which may result in bad behavior. For
example, it can trick "git repack -a -l -d" (possibly run by "git gc")
into thinking that all packs come from an alternate and delete all
objects.

	rm -rf test &&
	git clone https://github.com/git/git test &&
	(
	cd test &&
	ln -s objects .git/alt-objects &&
	# -c repack.updateserverinfo=false silences a warning about not
	# being able to update "info/refs", it isn't needed to show the
	# bad behavior
	GIT_ALTERNATE_OBJECT_DIRECTORIES=".git/alt-objects" git \
		-c repack.updateserverinfo=false repack -a -l -d  &&
	# It's broken!
	git status
	# Because there are no more objects!
	ls .git/objects/pack
	)

Fix this by resolving symlinks and relative paths before comparing the
alternate and object dir. This lets us clean up a number of issues noted
in 37a95862c6 (alternates: re-allow relative paths from environment,
2016-11-07):

- Now that we compare the real paths, duplicate detection is no longer
  foiled by relative paths.
- Using strbuf_realpath() allows us to "normalize" paths that
  strbuf_normalize_path() can't, so we can stop silently ignoring errors
  when "normalizing" paths from the environment.
- We now store an absolute path based on getcwd() (the "future
  direction" named in 37a95862c6), so chdir()-ing in the process no
  longer changes the directory pointed to by the alternate. This is a
  change in behavior, but a desirable one.

Signed-off-by: Glen Choo <chooglen@google.com>
---
    object-file: use real paths when adding alternates
    
    Thanks all for the feedback on v2. Once again, this version takes nearly
    all of Ævar's fixup patches [1] :)
    
    (It seems like the linux-* CI jobs are broken? I saw this tree pass
    previously, but linux-* is failing to build all of a sudden.)
    
    Changes in v3:
    
     * strbuf_release() all strbufs
     * Remove unnecessary details from the test (since it's a one-off, not
       reused)
    
    [1]
    https://lore.kernel.org/git/221122.868rk3bxbb.gmgdl@evledraar.gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1382%2Fchooglen%2Fobject-file%2Fcheck-alternate-real-path-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1382/chooglen/object-file/check-alternate-real-path-v3
Pull-Request: https://github.com/git/git/pull/1382

Range-diff vs v2:

 1:  ed9e12c0051 ! 1:  9bc04174be6 object-file: use real paths when adding alternates
     @@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct
       	struct strbuf pathbuf = STRBUF_INIT;
      +	struct strbuf tmp = STRBUF_INIT;
       	khiter_t pos;
     ++	int ret = -1;
       
       	if (!is_absolute_path(entry->buf) && relative_base) {
     + 		strbuf_realpath(&pathbuf, relative_base, 1);
      @@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
       	}
       	strbuf_addbuf(&pathbuf, entry);
     @@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct
      -	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;
     + 		      pathbuf.buf);
     +-		strbuf_release(&pathbuf);
     +-		return -1;
     ++		goto error;
       	}
      +	strbuf_swap(&pathbuf, &tmp);
     -+	strbuf_release(&tmp);
       
       	/*
       	 * The trailing slash after the directory name is given by
     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
     + 	while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
     + 		strbuf_setlen(&pathbuf, pathbuf.len - 1);
     + 
     +-	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {
     +-		strbuf_release(&pathbuf);
     +-		return -1;
     +-	}
     ++	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))
     ++		goto error;
     + 
     + 	CALLOC_ARRAY(ent, 1);
     + 	/* pathbuf.buf is already in r->objects->odb_by_path */
     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
     + 
     + 	/* recursively add alternates */
     + 	read_info_alternates(r, ent->path, depth + 1);
     +-
     +-	return 0;
     ++	ret = 0;
     ++ error:
     ++	strbuf_release(&tmp);
     ++	strbuf_release(&pathbuf);
     ++	return ret;
     + }
     + 
     + static const char *parse_alt_odb_entry(const char *string,
      @@ object-file.c: static void link_alt_odb_entries(struct repository *r, const char *alt,
       		return;
       	}
     @@ t/t7700-repack.sh: test_expect_success 'loose objects in alternate ODB are not r
       	test_has_duplicate_object false
       '
       
     -+test_expect_success '--local keeps packs when alternate is objectdir ' '
     -+	git init alt_symlink &&
     ++test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir ' '
     ++	test_when_finished "rm -rf repo" &&
     ++	git init repo &&
     ++	test_commit -C repo A &&
      +	(
     -+		cd alt_symlink &&
     -+		git init &&
     -+		echo content >file4 &&
     -+		git add file4 &&
     -+		git commit -m commit_file4 &&
     ++		cd repo &&
      +		git repack -a &&
      +		ls .git/objects/pack/*.pack >../expect &&
      +		ln -s objects .git/alt_objects &&


 object-file.c     | 26 +++++++++++++-------------
 t/t7700-repack.sh | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/object-file.c b/object-file.c
index 957790098fa..26290554bb4 100644
--- a/object-file.c
+++ b/object-file.c
@@ -508,7 +508,9 @@ 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;
+	int ret = -1;
 
 	if (!is_absolute_path(entry->buf) && relative_base) {
 		strbuf_realpath(&pathbuf, relative_base, 1);
@@ -516,12 +518,12 @@ 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);
-		strbuf_release(&pathbuf);
-		return -1;
+		goto error;
 	}
+	strbuf_swap(&pathbuf, &tmp);
 
 	/*
 	 * The trailing slash after the directory name is given by
@@ -530,10 +532,8 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
 	while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
 		strbuf_setlen(&pathbuf, pathbuf.len - 1);
 
-	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {
-		strbuf_release(&pathbuf);
-		return -1;
-	}
+	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))
+		goto error;
 
 	CALLOC_ARRAY(ent, 1);
 	/* pathbuf.buf is already in r->objects->odb_by_path */
@@ -548,8 +548,11 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
 
 	/* recursively add alternates */
 	read_info_alternates(r, ent->path, depth + 1);
-
-	return 0;
+	ret = 0;
+ error:
+	strbuf_release(&tmp);
+	strbuf_release(&pathbuf);
+	return ret;
 }
 
 static const char *parse_alt_odb_entry(const char *string,
@@ -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);
 
 	while (*alt) {
 		alt = parse_alt_odb_entry(alt, sep, &entry);
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 5be483bf887..599b4499b8e 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -90,6 +90,22 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '
 	test_has_duplicate_object false
 '
 
+test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir ' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo A &&
+	(
+		cd repo &&
+		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
+'
+
 test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '
 	mkdir alt_objects/pack &&
 	mv .git/objects/pack/* alt_objects/pack &&

base-commit: 319605f8f00e402f3ea758a02c63534ff800a711
-- 
gitgitgadget

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

[Jeff King]

On Wed, Nov 23, 2022 at 04:50:02PM -0800, Glen Choo wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
> >> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
> >> &tmp you pass in here) and then "does a "goto error_out".
> >> 
> >> It then *resets* the strbuf, but doesn't release it, assuming that
> >> you're going to pass it in again. So in that case we'd leak here, no?
> >> 
> >> I.e. a NULL return value from strbuf_realpath() doesn't mean that it
> >> didn't allocate in the scratch area passed to it, so we need to
> >> strbuf_release(&tmp) here too.
> >
> > We don't use MANY_MISSING in this code path, but I didn't read
> > strbuf_realpath_1() carefully enough to see if that is the only case.
> > But regardless, I think it is a bug in strbuf_realpath(). All of the
> > strbuf functions generally try to leave a buffer untouched on error.
> >
> > So IMHO we would want a preparatory patch with s/reset/release/ in that
> > function, which better matches the intent (we might be freeing an
> > allocated buffer, but that's OK from the caller perspective).
> 
> Is that always OK? I would think that we'd do something closer to
> strbuf_getcwd():
> 
>   int strbuf_getcwd(struct strbuf *sb)
>   {
>     size_t oldalloc = sb->alloc;
>     /* ... */
>     if (oldalloc == 0)
>       strbuf_release(sb);
>     else
>       strbuf_reset(sb);
>   }
> 
> i.e. if the caller passed in a strbuf with allocated contents, they're
> responsible for free()-ing it, otherwise we free() it. That does fix the
> leak in this patch, but I don't feel strongly enough about changing
> strbuf_realpath() to do it now, so I'll do without the change for now.

That's what I was getting at with "that's OK from the caller
perspective". strbuf_realpath() is also unlike other strbuf functions in
that it clobbers the contents of the buffer, even on success (rather
than adding on success and rolling back to the original state on error).

Since the caller is OK with the buffer being clobbered anyway, it should
not matter to it whether we clobbered an allocated buffer back to an
unallocated one on error. The confusing thing (and the current behavior)
is when we do the opposite: change an unallocated one to an allocated
one.

-Peff

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

[Jeff King]

On Thu, Nov 24, 2022 at 12:55:31AM +0000, Glen Choo via GitGitGadget wrote:

>     object-file: use real paths when adding alternates
>     
>     Thanks all for the feedback on v2. Once again, this version takes nearly
>     all of Ævar's fixup patches [1] :)

This looks fine to me. I'd probably have done the strbuf_realpath()
change I suggested, but I don't mind if you want to punt on it for now.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 24, 2022 at 12:55:31AM +0000, Glen Choo via GitGitGadget wrote:
>
>>     object-file: use real paths when adding alternates
>>     
>>     Thanks all for the feedback on v2. Once again, this version takes nearly
>>     all of Ævar's fixup patches [1] :)
>
> This looks fine to me. I'd probably have done the strbuf_realpath()
> change I suggested, but I don't mind if you want to punt on it for now.

Thanks, both.

When we designed the alternates long time ago, the "avoid placing
duplicate directories on list" was done merely as an optimization
and not as a correctness measure, as there was no plan to add
anything destructive like "delete all non-local objects".  But now
this seems to matter X-<.