object-file: use real paths when adding alternates

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>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/object-file.c b/object-file.c
index 957790098fab7e9344d34ebc5a2ef066c24147bc..26290554bb4c5f1d679194fe7d3f2b5c78a273e2 100644 (file)
--- 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 5be483bf887cc60a1c967196ccfa7584f363bc6e..599b4499b8ef87f9f67b0237fa6c06fa305513e0 100755 (executable)
--- 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 &&