object-file: use real paths when adding alternates

From: Glen Choo <[email protected]>

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.

---
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(-)

index 957790098fa..f901dd272d1 100644
@@ -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);

index 5be483bf887..ce1954d0977 100755
@@ -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

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.

index 957790098f..c6a195c6dd 100644
@@ -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);

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(-)

index c6a195c6dd2..1a94d98e0c7 100644
@@ -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);
 }

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.

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

Thanks for keeping us on track.

Thanks,
Taylor

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.

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

From: Glen Choo <[email protected]>

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.

---
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/[email protected]
[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.

## 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(-)

index 957790098fa..ef2b762234d 100644
@@ -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);

index 5be483bf887..ce1954d0977 100755
@@ -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

Aside from some small nits this looks good to me.

	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,

	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 &&

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

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

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.

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.

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.

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.

From: Glen Choo <[email protected]>

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.

---
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/[email protected]

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(-)

index 957790098fa..26290554bb4 100644
@@ -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);

index 5be483bf887..599b4499b8e 100755
@@ -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

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

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-<.