{"thread":{"id":"58811","subject":"[PATCH] object-file: use real paths when adding alternates","startedAt":"2022-11-17T17:31:37Z","lastAt":"2022-11-25T06:51:08Z","messageCount":17,"participants":["Glen Choo via GitGitGadget","Jeff King","Ævar Arnfjörð Bjarmason","Taylor Blau","Glen Choo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467453","messageId":"pull.1382.git.git.1668706274099.gitgitgadget@gmail.com","threadId":"58811","inReplyTo":null,"subject":"[PATCH] object-file: use real paths when adding alternates","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-17T17:31:13Z","receivedAt":"2022-11-17T17:31:37Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nWhen adding an alternate ODB, we check if the alternate has the same\npath as the object dir, and if so, we do nothing. However, that\ncomparison does not resolve symlinks. This makes it possible to add the\nobject dir as an alternate, which may result in bad behavior. For\nexample, it can trick \"git repack -a -l -d\" (possibly run by \"git gc\")\ninto thinking that all packs come from an alternate and delete all\nobjects.\n\n\trm -rf test &&\n\tgit clone https://github.com/git/git test &&\n\t(\n\tcd test &&\n\tln -s objects .git/alt-objects &&\n\t# -c repack.updateserverinfo=false silences a warning about not\n\t# being able to update \"info/refs\", it isn't needed to show the\n\t# bad behavior\n\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\".git/alt-objects\" git \\\n\t\t-c repack.updateserverinfo=false repack -a -l -d  &&\n\t# It's broken!\n\tgit status\n\t# Because there are no more objects!\n\tls .git/objects/pack\n\t)\n\nFix this by resolving symlinks before comparing the alternate and object\ndir.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    object-file: use real paths when adding alternates\n    \n    Here's a bug that got uncovered because of some oddities in how \"repo\"\n    [1] manages its object directories. With some tracing, I'm quite certain\n    that the mechanism is that the packs are treated as non-local, but I\n    don't understand \"git repack\" extremely well, so e.g. the test I added\n    seems pretty crude.\n    \n    [1] https://gerrit.googlesource.com/git-repo\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1382%2Fchooglen%2Fobject-file%2Fcheck-alternate-real-path-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1382/chooglen/object-file/check-alternate-real-path-v1\nPull-Request: https://github.com/git/git/pull/1382\n\n object-file.c     | 17 ++++++++++++-----\n t/t7700-repack.sh | 18 ++++++++++++++++++\n 2 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 957790098fa..f901dd272d1 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -455,14 +455,16 @@ static int alt_odb_usable(struct raw_object_store *o,\n \t\t\t  struct strbuf *path,\n \t\t\t  const char *normalized_objdir, khiter_t *pos)\n {\n+\tint ret = 0;\n \tint r;\n+\tstruct strbuf real_path = STRBUF_INIT;\n \n \t/* Detect cases where alternate disappeared */\n \tif (!is_directory(path->buf)) {\n \t\terror(_(\"object directory %s does not exist; \"\n \t\t\t\"check .git/objects/info/alternates\"),\n \t\t      path->buf);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n \t/*\n@@ -478,11 +480,16 @@ static int alt_odb_usable(struct raw_object_store *o,\n \t\tassert(r == 1); /* never used */\n \t\tkh_value(o->odb_by_path, p) = o->odb;\n \t}\n-\tif (fspatheq(path->buf, normalized_objdir))\n-\t\treturn 0;\n+\n+\tstrbuf_realpath(&real_path, path->buf, 1);\n+\tif (fspatheq(real_path.buf, normalized_objdir))\n+\t\tgoto cleanup;\n \t*pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r);\n \t/* r: 0 = exists, 1 = never used, 2 = deleted */\n-\treturn r == 0 ? 0 : 1;\n+\tret = r == 0 ? 0 : 1;\n+ cleanup:\n+\tstrbuf_release(&real_path);\n+\treturn ret;\n }\n \n /*\n@@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n \t\treturn;\n \t}\n \n-\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n+\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n \tif (strbuf_normalize_path(&objdirbuf) < 0)\n \t\tdie(_(\"unable to normalize object directory: %s\"),\n \t\t    objdirbuf.buf);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 5be483bf887..ce1954d0977 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n \ttest_has_duplicate_object false\n '\n \n+test_expect_success '--local keeps packs when alternate is objectdir ' '\n+\tgit init alt_symlink &&\n+\t(\n+\t\tcd alt_symlink &&\n+\t\tgit init &&\n+\t\techo content >file4 &&\n+\t\tgit add file4 &&\n+\t\tgit commit -m commit_file4 &&\n+\t\tgit repack -a &&\n+\t\tls .git/objects/pack/*.pack >../expect &&\n+\t\tln -s objects .git/alt_objects &&\n+\t\techo \"$(pwd)/.git/alt_objects\" >.git/objects/info/alternates &&\n+\t\tgit repack -a -d -l &&\n+\t\tls .git/objects/pack/*.pack >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '\n \tmkdir alt_objects/pack &&\n \tmv .git/objects/pack/* alt_objects/pack &&\n\nbase-commit: 319605f8f00e402f3ea758a02c63534ff800a711\n-- \ngitgitgadget\n"},{"id":"467458","messageId":"Y3aBzbzub7flQyca@coredump.intra.peff.net","threadId":"58811","inReplyTo":"pull.1382.git.git.1668706274099.gitgitgadget@gmail.com","subject":"Re: [PATCH] object-file: use real paths when adding alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T18:47:41Z","receivedAt":"2022-11-17T18:47:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:\n\n> When adding an alternate ODB, we check if the alternate has the same\n> path as the object dir, and if so, we do nothing. However, that\n> comparison does not resolve symlinks. This makes it possible to add the\n> object dir as an alternate, which may result in bad behavior. For\n> example, it can trick \"git repack -a -l -d\" (possibly run by \"git gc\")\n> into thinking that all packs come from an alternate and delete all\n> objects.\n\nI think we do attempt to normalize the names. In link_alt_odb_entries(),\nwe call strbuf_normalize_path() on the base object directory, and then\nin link_alt_odb_entry(), we similarly normalize the proposed odb names.\nAnd then we drop duplicates (either of other alternates, or of the main\nobject directory).\n\nSo it seems like the problem is that \"normalize\" is not enough here,\nbecause it only normalizes the text. We need to be using\nstrbuf_realpath() which will actually evaluate symbolic links.\n\n> diff --git a/object-file.c b/object-file.c\n> index 957790098fa..f901dd272d1 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -455,14 +455,16 @@ static int alt_odb_usable(struct raw_object_store *o,\n>  \t\t\t  struct strbuf *path,\n>  \t\t\t  const char *normalized_objdir, khiter_t *pos)\n>  {\n> +\tint ret = 0;\n>  \tint r;\n> +\tstruct strbuf real_path = STRBUF_INIT;\n>  \n>  \t/* Detect cases where alternate disappeared */\n>  \tif (!is_directory(path->buf)) {\n>  \t\terror(_(\"object directory %s does not exist; \"\n>  \t\t\t\"check .git/objects/info/alternates\"),\n>  \t\t      path->buf);\n> -\t\treturn 0;\n> +\t\tgoto cleanup;\n>  \t}\n\nThis goto seems unnecessary; we know we haven't touched real_path yet,\nso there is nothing to clean. But...\n\n> @@ -478,11 +480,16 @@ static int alt_odb_usable(struct raw_object_store *o,\n>  \t\tassert(r == 1); /* never used */\n>  \t\tkh_value(o->odb_by_path, p) = o->odb;\n>  \t}\n> -\tif (fspatheq(path->buf, normalized_objdir))\n> -\t\treturn 0;\n> +\n> +\tstrbuf_realpath(&real_path, path->buf, 1);\n> +\tif (fspatheq(real_path.buf, normalized_objdir))\n> +\t\tgoto cleanup;\n>  \t*pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r);\n>  \t/* r: 0 = exists, 1 = never used, 2 = deleted */\n> -\treturn r == 0 ? 0 : 1;\n> +\tret = r == 0 ? 0 : 1;\n> + cleanup:\n> +\tstrbuf_release(&real_path);\n> +\treturn ret;\n>  }\n\nThis seems like the wrong place to be doing realpath. We've already\nnormalized earlier in link_alt_odb_entry(). Why not use realpath there?\n\nIt does mean we'd end up with the realpath in the object_directory\nstruct, but I don't see that as a bad thing (in fact, it may be slightly\nfaster if it saves the kernel crossing a symlink boundary).\n\nI do suspect it would change the error message for missing intermediate\ndirectories (since realpath would complain, before we even hit the \"does\nthis even exist\"), but it's an error in both cases. Which brings up\nanother point here: I think right now we treat those errors as warnings,\nand cnotinue on without the alternate available (because we ignore the\nresult of link_alt_odb_entry). But with your patch, a failure in this\nrealpath would cause us to die immediately.\n\n> @@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n>  \t\treturn;\n>  \t}\n>  \n> -\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n> +\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n>  \tif (strbuf_normalize_path(&objdirbuf) < 0)\n>  \t\tdie(_(\"unable to normalize object directory: %s\"),\n>  \t\t    objdirbuf.buf);\n\nSimilarly here, I think we'd want to _replace_ the normalize with a\nrealpath. There's no point in doing both. It's OK to die in this one\nbecause we assume the object directory can be normalized/realpath'd.\n\nSo I'd have expected the code portion of your patch to be more like:\n\ndiff --git a/object-file.c b/object-file.c\nindex 957790098f..c6a195c6dd 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n {\n \tstruct object_directory *ent;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n \tkhiter_t pos;\n \n \tif (!is_absolute_path(entry->buf) && relative_base) {\n@@ -516,12 +517,18 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n \t}\n \tstrbuf_addbuf(&pathbuf, entry);\n \n-\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n-\t\terror(_(\"unable to normalize alternate object path: %s\"),\n-\t\t      pathbuf.buf);\n-\t\tstrbuf_release(&pathbuf);\n-\t\treturn -1;\n+\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n+\t\tif (relative_base) {\n+\t\t\terror(_(\"unable to normalize alternate object path: %s\"),\n+\t\t\t      pathbuf.buf);\n+\t\t\tstrbuf_release(&pathbuf);\n+\t\t\treturn -1;\n+\t\t}\n+\t\t/* allow broken paths from env per 37a95862c625 */\n+\t\tstrbuf_addstr(&tmp, pathbuf.buf);\n \t}\n+\tstrbuf_swap(&pathbuf, &tmp);\n+\tstrbuf_release(&tmp);\n \n \t/*\n \t * The trailing slash after the directory name is given by\n@@ -596,10 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n \t\treturn;\n \t}\n \n-\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n-\tif (strbuf_normalize_path(&objdirbuf) < 0)\n-\t\tdie(_(\"unable to normalize object directory: %s\"),\n-\t\t    objdirbuf.buf);\n+\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n \n \twhile (*alt) {\n \t\talt = parse_alt_odb_entry(alt, sep, &entry);\n\nThe \"tmp\" swapping in link_alt_odb_entry is kind of unfortunate. It\nwould be nice if there were an in-place version of strbuf_realpath, even\nif it was using two buffers under the hood (which is how the normalize\ncode does it). And then the patch really would be s/normalize/realpath/,\nwhich is easier to understand.\n\nPossibly this should also be using the \"forgiving\" version. We\neventually error out on missing entries later on, so it's not a big deal\nto error here. But it would let us keep the error message the same. I\ndon't know that it matters much in practice.\n\n-Peff\n"},{"id":"467461","messageId":"221117.86h6yxgy7b.gmgdl@evledraar.gmail.com","threadId":"58811","inReplyTo":"Y3aBzbzub7flQyca@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: use real paths when adding alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-17T19:41:44Z","receivedAt":"2022-11-17T19:47:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 17 2022, Jeff King wrote:\n\n> On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:\n> [...]\n>> @@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n>>  \t\treturn;\n>>  \t}\n>>  \n>> -\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n>> +\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n>>  \tif (strbuf_normalize_path(&objdirbuf) < 0)\n>>  \t\tdie(_(\"unable to normalize object directory: %s\"),\n>>  \t\t    objdirbuf.buf);\n>\n> Similarly here, I think we'd want to _replace_ the normalize with a\n> realpath. There's no point in doing both. It's OK to die in this one\n> because we assume the object directory can be normalized/realpath'd.\n>\n> So I'd have expected the code portion of your patch to be more like:\n>\n> diff --git a/object-file.c b/object-file.c\n> index 957790098f..c6a195c6dd 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>  {\n>  \tstruct object_directory *ent;\n>  \tstruct strbuf pathbuf = STRBUF_INIT;\n> +\tstruct strbuf tmp = STRBUF_INIT;\n>  \tkhiter_t pos;\n>  \n>  \tif (!is_absolute_path(entry->buf) && relative_base) {\n> @@ -516,12 +517,18 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>  \t}\n>  \tstrbuf_addbuf(&pathbuf, entry);\n>  \n> -\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n> -\t\terror(_(\"unable to normalize alternate object path: %s\"),\n> -\t\t      pathbuf.buf);\n> -\t\tstrbuf_release(&pathbuf);\n> -\t\treturn -1;\n> +\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n> +\t\tif (relative_base) {\n> +\t\t\terror(_(\"unable to normalize alternate object path: %s\"),\n> +\t\t\t      pathbuf.buf);\n> +\t\t\tstrbuf_release(&pathbuf);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\t/* allow broken paths from env per 37a95862c625 */\n> +\t\tstrbuf_addstr(&tmp, pathbuf.buf);\n>  \t}\n> +\tstrbuf_swap(&pathbuf, &tmp);\n> +\tstrbuf_release(&tmp);\n>  \n>  \t/*\n>  \t * The trailing slash after the directory name is given by\n> @@ -596,10 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n>  \t\treturn;\n>  \t}\n>  \n> -\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n> -\tif (strbuf_normalize_path(&objdirbuf) < 0)\n> -\t\tdie(_(\"unable to normalize object directory: %s\"),\n> -\t\t    objdirbuf.buf);\n> +\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n>  \n>  \twhile (*alt) {\n>  \t\talt = parse_alt_odb_entry(alt, sep, &entry);\n>\n> The \"tmp\" swapping in link_alt_odb_entry is kind of unfortunate. It\n> would be nice if there were an in-place version of strbuf_realpath, even\n> if it was using two buffers under the hood (which is how the normalize\n> code does it). And then the patch really would be s/normalize/realpath/,\n> which is easier to understand.\n>\n> Possibly this should also be using the \"forgiving\" version. We\n> eventually error out on missing entries later on, so it's not a big deal\n> to error here. But it would let us keep the error message the same. I\n> don't know that it matters much in practice.\n\nThis probably isn't worth it, but I wondered if this wouldn't be easier\nif we pulled that memory management into the caller, it's not\nperformance sensitive (or maybe, how many alternatives do people have\n:)), but an advantage of this is that we avoid the free()/malloc() if we\nonly get partway through, i.e. return early and keep looping.\n\nIn terms of general code smell & how we manage the \"return\" here, as\nadding \"RESULT_MUST_BE_USED\" to this shows we never use the \"0\" or \"-1\"\n(or any other...) return value.\n\nThat's been the case since this was added in c2f493a4ae1 (Transitively\nread alternatives, 2006-05-07), so we can probably just make this a\n\"void\" and ditch the returns if we're finding ourselves juggling these\nreturn values...\n\n object-file.c | 44 ++++++++++++++++++++++----------------------\n 1 file changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex c6a195c6dd2..1a94d98e0c7 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -504,47 +504,43 @@ static void read_info_alternates(struct repository *r,\n \t\t\t\t const char *relative_base,\n \t\t\t\t int depth);\n static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n-\tconst char *relative_base, int depth, const char *normalized_objdir)\n+\t\t\t      const char *relative_base, int depth,\n+\t\t\t      const char *normalized_objdir,\n+\t\t\t      struct strbuf *pathbuf)\n {\n \tstruct object_directory *ent;\n-\tstruct strbuf pathbuf = STRBUF_INIT;\n \tstruct strbuf tmp = STRBUF_INIT;\n \tkhiter_t pos;\n \n \tif (!is_absolute_path(entry->buf) && relative_base) {\n-\t\tstrbuf_realpath(&pathbuf, relative_base, 1);\n-\t\tstrbuf_addch(&pathbuf, '/');\n+\t\tstrbuf_realpath(pathbuf, relative_base, 1);\n+\t\tstrbuf_addch(pathbuf, '/');\n \t}\n-\tstrbuf_addbuf(&pathbuf, entry);\n+\tstrbuf_addbuf(pathbuf, entry);\n \n-\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n-\t\tif (relative_base) {\n-\t\t\terror(_(\"unable to normalize alternate object path: %s\"),\n-\t\t\t      pathbuf.buf);\n-\t\t\tstrbuf_release(&pathbuf);\n-\t\t\treturn -1;\n-\t\t}\n+\tif (!strbuf_realpath(&tmp, pathbuf->buf, 0)) {\n+\t\tif (relative_base)\n+\t\t\treturn error(_(\"unable to normalize alternate object path: %s\"),\n+\t\t\t\t     pathbuf->buf);\n \t\t/* allow broken paths from env per 37a95862c625 */\n-\t\tstrbuf_addstr(&tmp, pathbuf.buf);\n+\t\tstrbuf_addstr(&tmp, pathbuf->buf);\n \t}\n-\tstrbuf_swap(&pathbuf, &tmp);\n+\tstrbuf_swap(pathbuf, &tmp);\n \tstrbuf_release(&tmp);\n \n \t/*\n \t * The trailing slash after the directory name is given by\n \t * this function at the end. Remove duplicates.\n \t */\n-\twhile (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')\n-\t\tstrbuf_setlen(&pathbuf, pathbuf.len - 1);\n+\twhile (pathbuf->len && pathbuf->buf[pathbuf->len - 1] == '/')\n+\t\tstrbuf_setlen(pathbuf, pathbuf->len - 1);\n \n-\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {\n-\t\tstrbuf_release(&pathbuf);\n+\tif (!alt_odb_usable(r->objects, pathbuf, normalized_objdir, &pos))\n \t\treturn -1;\n-\t}\n \n \tCALLOC_ARRAY(ent, 1);\n-\t/* pathbuf.buf is already in r->objects->odb_by_path */\n-\tent->path = strbuf_detach(&pathbuf, NULL);\n+\t/* pathbuf->buf is already in r->objects->odb_by_path */\n+\tent->path = strbuf_detach(pathbuf, NULL);\n \n \t/* add the alternate entry */\n \t*r->objects->odb_tail = ent;\n@@ -593,6 +589,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n {\n \tstruct strbuf objdirbuf = STRBUF_INIT;\n \tstruct strbuf entry = STRBUF_INIT;\n+\tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (!alt || !*alt)\n \t\treturn;\n@@ -610,8 +607,11 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n \t\tif (!entry.len)\n \t\t\tcontinue;\n \t\tlink_alt_odb_entry(r, &entry,\n-\t\t\t\t   relative_base, depth, objdirbuf.buf);\n+\t\t\t\t   relative_base, depth, objdirbuf.buf,\n+\t\t\t\t   &pathbuf);\n+\t\tstrbuf_reset(&pathbuf);\n \t}\n+\tstrbuf_release(&pathbuf);\n \tstrbuf_release(&entry);\n \tstrbuf_release(&objdirbuf);\n }\n\n"},{"id":"467465","messageId":"Y3atneCCAA4fMFKL@nand.local","threadId":"58811","inReplyTo":"pull.1382.git.git.1668706274099.gitgitgadget@gmail.com","subject":"Re: [PATCH] object-file: use real paths when adding alternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-17T21:54:37Z","receivedAt":"2022-11-17T21:54:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:\n> From: Glen Choo <chooglen@google.com>\n>\n> When adding an alternate ODB, we check if the alternate has the same\n> path as the object dir, and if so, we do nothing. However, that\n> comparison does not resolve symlinks. This makes it possible to add the\n> object dir as an alternate, which may result in bad behavior. For\n> example, it can trick \"git repack -a -l -d\" (possibly run by \"git gc\")\n> into thinking that all packs come from an alternate and delete all\n> objects.\n\nNice find and fix. Looks like Peff had a couple of good suggestions\nwhich I'd like to see incorporated into another version before we pick\nthis up.\n\nOtherwise, looking at the patch myself, it seems obviously good. Thanks\nfor working on it.\n\n\nThanks,\nTaylor\n"},{"id":"467468","messageId":"Y3auRnJqHq3pMKAe@coredump.intra.peff.net","threadId":"58811","inReplyTo":"221117.86h6yxgy7b.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] object-file: use real paths when adding alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T21:57:26Z","receivedAt":"2022-11-17T21:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> This probably isn't worth it, but I wondered if this wouldn't be easier\n> if we pulled that memory management into the caller, it's not\n> performance sensitive (or maybe, how many alternatives do people have\n> :)), but an advantage of this is that we avoid the free()/malloc() if we\n> only get partway through, i.e. return early and keep looping.\n\nI agree with your \"probably\". This isn't worth it to save a malloc in a\nvery non-hot code path. If the two bits of code were otherwise equal\n(say, reset-ing a buffer used directly in a loop) I might say \"why\nnot?\". But crossing a function boundary to me introduces way too many\nquestions in somebody reading the code (like \"is pathbuf supposed to\nhave something in it?\") to make it worth doing here.\n\nBut even if we did want to do it, see below.\n\n> In terms of general code smell & how we manage the \"return\" here, as\n> adding \"RESULT_MUST_BE_USED\" to this shows we never use the \"0\" or \"-1\"\n> (or any other...) return value.\n> \n> That's been the case since this was added in c2f493a4ae1 (Transitively\n> read alternatives, 2006-05-07), so we can probably just make this a\n> \"void\" and ditch the returns if we're finding ourselves juggling these\n> return values...\n\nYeah, we could ditch the return values. In a sense they are at least\ndocumenting how link_alt_odb_entry() sees the world, but if nobody looks\nat them, I'd be OK dropping them to make it clear that we don't intend\nto ever act on them.\n\nThat said, both of these are orthogonal to what Glen's patches are\ndoing. If you want to submit a series later to deal with them, OK, but\nlet's try not to hijack the conversation for patches that are fixing a\nreal bug.\n\n-Peff\n"},{"id":"467470","messageId":"Y3avm0ZgkVWr2qX7@nand.local","threadId":"58811","inReplyTo":"Y3auRnJqHq3pMKAe@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: use real paths when adding alternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-17T22:03:07Z","receivedAt":"2022-11-17T22:05:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 17, 2022 at 04:57:26PM -0500, Jeff King wrote:\n> That said, both of these are orthogonal to what Glen's patches are\n> doing. If you want to submit a series later to deal with them, OK, but\n> let's try not to hijack the conversation for patches that are fixing a\n> real bug.\n\nThanks for keeping us on track.\n\nThanks,\nTaylor\n"},{"id":"467476","messageId":"kl6lsfihgmib.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58811","inReplyTo":"Y3auRnJqHq3pMKAe@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: use real paths when adding alternates","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-18T00:00:12Z","receivedAt":"2022-11-18T00:00:19Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This probably isn't worth it, but I wondered if this wouldn't be easier\n>> if we pulled that memory management into the caller [...]\n> [......] But crossing a function boundary to me introduces way too many\n> questions in somebody reading the code (like \"is pathbuf supposed to\n> have something in it?\") to make it worth doing here.\n\nThanks, both :)\n\nI think the loss of readability is enough for me to hold off on this\nsuggestion, but I don't mind reviewing cleanup patches on top of this.\n"},{"id":"467728","messageId":"pull.1382.v2.git.git.1669074557348.gitgitgadget@gmail.com","threadId":"58811","inReplyTo":"pull.1382.git.git.1668706274099.gitgitgadget@gmail.com","subject":"[PATCH v2] object-file: use real paths when adding alternates","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-21T23:49:17Z","receivedAt":"2022-11-21T23:49:24Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nWhen adding an alternate ODB, we check if the alternate has the same\npath as the object dir, and if so, we do nothing. However, that\ncomparison does not resolve symlinks. This makes it possible to add the\nobject dir as an alternate, which may result in bad behavior. For\nexample, it can trick \"git repack -a -l -d\" (possibly run by \"git gc\")\ninto thinking that all packs come from an alternate and delete all\nobjects.\n\n\trm -rf test &&\n\tgit clone https://github.com/git/git test &&\n\t(\n\tcd test &&\n\tln -s objects .git/alt-objects &&\n\t# -c repack.updateserverinfo=false silences a warning about not\n\t# being able to update \"info/refs\", it isn't needed to show the\n\t# bad behavior\n\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\".git/alt-objects\" git \\\n\t\t-c repack.updateserverinfo=false repack -a -l -d  &&\n\t# It's broken!\n\tgit status\n\t# Because there are no more objects!\n\tls .git/objects/pack\n\t)\n\nFix this by resolving symlinks and relative paths before comparing the\nalternate and object dir. This lets us clean up a number of issues noted\nin 37a95862c6 (alternates: re-allow relative paths from environment,\n2016-11-07):\n\n- Now that we compare the real paths, duplicate detection is no longer\n  foiled by relative paths.\n- Using strbuf_realpath() allows us to \"normalize\" paths that\n  strbuf_normalize_path() can't, so we can stop silently ignoring errors\n  when \"normalizing\" paths from the environment.\n- We now store an absolute path based on getcwd() (the \"future\n  direction\" named in 37a95862c6), so chdir()-ing in the process no\n  longer changes the directory pointed to by the alternate. This is a\n  change in behavior, but a desirable one.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    object-file: use real paths when adding alternates\n    \n    Thanks for the feedback on v1. This version takes nearly all of Peff's\n    patch [1] except for the comment about making an exception for relative\n    paths in the environment. My reading of the commit [2] is that it was a\n    workaround for strbuf_normalize_path() not being able to handle relative\n    paths, so the only reason to special-case the environment is to preserve\n    the behavior of respecting broken paths, which (unlike relative paths) I\n    don't think will be missed.\n    \n    Changes in v2:\n    \n     * Do realpath when storing the alternate's directory entry instead of\n       only during the usability check.\n     * Update commit message to reflect the relationship to [2]\n    \n    [1] https://lore.kernel.org/git/Y3aBzbzub7flQyca@coredump.intra.peff.net\n    [2] 37a95862c6 (alternates: re-allow relative paths from environment,\n    2016-11-07)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1382%2Fchooglen%2Fobject-file%2Fcheck-alternate-real-path-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1382/chooglen/object-file/check-alternate-real-path-v2\nPull-Request: https://github.com/git/git/pull/1382\n\nRange-diff vs v1:\n\n 1:  9392725ad01 ! 1:  ed9e12c0051 object-file: use real paths when adding alternates\n     @@ Commit message\n                  ls .git/objects/pack\n                  )\n      \n     -    Fix this by resolving symlinks before comparing the alternate and object\n     -    dir.\n     +    Fix this by resolving symlinks and relative paths before comparing the\n     +    alternate and object dir. This lets us clean up a number of issues noted\n     +    in 37a95862c6 (alternates: re-allow relative paths from environment,\n     +    2016-11-07):\n     +\n     +    - Now that we compare the real paths, duplicate detection is no longer\n     +      foiled by relative paths.\n     +    - Using strbuf_realpath() allows us to \"normalize\" paths that\n     +      strbuf_normalize_path() can't, so we can stop silently ignoring errors\n     +      when \"normalizing\" paths from the environment.\n     +    - We now store an absolute path based on getcwd() (the \"future\n     +      direction\" named in 37a95862c6), so chdir()-ing in the process no\n     +      longer changes the directory pointed to by the alternate. This is a\n     +      change in behavior, but a desirable one.\n      \n          Signed-off-by: Glen Choo <chooglen@google.com>\n      \n       ## object-file.c ##\n     -@@ object-file.c: static int alt_odb_usable(struct raw_object_store *o,\n     - \t\t\t  struct strbuf *path,\n     - \t\t\t  const char *normalized_objdir, khiter_t *pos)\n     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n       {\n     -+\tint ret = 0;\n     - \tint r;\n     -+\tstruct strbuf real_path = STRBUF_INIT;\n     + \tstruct object_directory *ent;\n     + \tstruct strbuf pathbuf = STRBUF_INIT;\n     ++\tstruct strbuf tmp = STRBUF_INIT;\n     + \tkhiter_t pos;\n       \n     - \t/* Detect cases where alternate disappeared */\n     - \tif (!is_directory(path->buf)) {\n     - \t\terror(_(\"object directory %s does not exist; \"\n     - \t\t\t\"check .git/objects/info/alternates\"),\n     - \t\t      path->buf);\n     --\t\treturn 0;\n     -+\t\tgoto cleanup;\n     + \tif (!is_absolute_path(entry->buf) && relative_base) {\n     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n       \t}\n     + \tstrbuf_addbuf(&pathbuf, entry);\n       \n     - \t/*\n     -@@ object-file.c: static int alt_odb_usable(struct raw_object_store *o,\n     - \t\tassert(r == 1); /* never used */\n     - \t\tkh_value(o->odb_by_path, p) = o->odb;\n     +-\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n     ++\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n     + \t\terror(_(\"unable to normalize alternate object path: %s\"),\n     +-\t\t      pathbuf.buf);\n     ++\t\t\tpathbuf.buf);\n     + \t\tstrbuf_release(&pathbuf);\n     + \t\treturn -1;\n       \t}\n     --\tif (fspatheq(path->buf, normalized_objdir))\n     --\t\treturn 0;\n     -+\n     -+\tstrbuf_realpath(&real_path, path->buf, 1);\n     -+\tif (fspatheq(real_path.buf, normalized_objdir))\n     -+\t\tgoto cleanup;\n     - \t*pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r);\n     - \t/* r: 0 = exists, 1 = never used, 2 = deleted */\n     --\treturn r == 0 ? 0 : 1;\n     -+\tret = r == 0 ? 0 : 1;\n     -+ cleanup:\n     -+\tstrbuf_release(&real_path);\n     -+\treturn ret;\n     - }\n     ++\tstrbuf_swap(&pathbuf, &tmp);\n     ++\tstrbuf_release(&tmp);\n       \n     - /*\n     + \t/*\n     + \t * The trailing slash after the directory name is given by\n      @@ object-file.c: static void link_alt_odb_entries(struct repository *r, const char *alt,\n       \t\treturn;\n       \t}\n       \n      -\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n     +-\tif (strbuf_normalize_path(&objdirbuf) < 0)\n     +-\t\tdie(_(\"unable to normalize object directory: %s\"),\n     +-\t\t    objdirbuf.buf);\n      +\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n     - \tif (strbuf_normalize_path(&objdirbuf) < 0)\n     - \t\tdie(_(\"unable to normalize object directory: %s\"),\n     - \t\t    objdirbuf.buf);\n     + \n     + \twhile (*alt) {\n     + \t\talt = parse_alt_odb_entry(alt, sep, &entry);\n      \n       ## t/t7700-repack.sh ##\n      @@ t/t7700-repack.sh: test_expect_success 'loose objects in alternate ODB are not repacked' '\n\n\n object-file.c     | 12 ++++++------\n t/t7700-repack.sh | 18 ++++++++++++++++++\n 2 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 957790098fa..ef2b762234d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n {\n \tstruct object_directory *ent;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n \tkhiter_t pos;\n \n \tif (!is_absolute_path(entry->buf) && relative_base) {\n@@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n \t}\n \tstrbuf_addbuf(&pathbuf, entry);\n \n-\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n+\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n \t\terror(_(\"unable to normalize alternate object path: %s\"),\n-\t\t      pathbuf.buf);\n+\t\t\tpathbuf.buf);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn -1;\n \t}\n+\tstrbuf_swap(&pathbuf, &tmp);\n+\tstrbuf_release(&tmp);\n \n \t/*\n \t * The trailing slash after the directory name is given by\n@@ -596,10 +599,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n \t\treturn;\n \t}\n \n-\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n-\tif (strbuf_normalize_path(&objdirbuf) < 0)\n-\t\tdie(_(\"unable to normalize object directory: %s\"),\n-\t\t    objdirbuf.buf);\n+\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n \n \twhile (*alt) {\n \t\talt = parse_alt_odb_entry(alt, sep, &entry);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 5be483bf887..ce1954d0977 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n \ttest_has_duplicate_object false\n '\n \n+test_expect_success '--local keeps packs when alternate is objectdir ' '\n+\tgit init alt_symlink &&\n+\t(\n+\t\tcd alt_symlink &&\n+\t\tgit init &&\n+\t\techo content >file4 &&\n+\t\tgit add file4 &&\n+\t\tgit commit -m commit_file4 &&\n+\t\tgit repack -a &&\n+\t\tls .git/objects/pack/*.pack >../expect &&\n+\t\tln -s objects .git/alt_objects &&\n+\t\techo \"$(pwd)/.git/alt_objects\" >.git/objects/info/alternates &&\n+\t\tgit repack -a -d -l &&\n+\t\tls .git/objects/pack/*.pack >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '\n \tmkdir alt_objects/pack &&\n \tmv .git/objects/pack/* alt_objects/pack &&\n\nbase-commit: 319605f8f00e402f3ea758a02c63534ff800a711\n-- \ngitgitgadget\n"},{"id":"467738","messageId":"221122.868rk3bxbb.gmgdl@evledraar.gmail.com","threadId":"58811","inReplyTo":"pull.1382.v2.git.git.1669074557348.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] object-file: use real paths when adding alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-22T00:56:09Z","receivedAt":"2022-11-22T01:19:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 21 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo <chooglen@google.com>\n\nAside from some small nits this looks good to me.\n\n>  object-file.c     | 12 ++++++------\n>  t/t7700-repack.sh | 18 ++++++++++++++++++\n>  2 files changed, 24 insertions(+), 6 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 957790098fa..ef2b762234d 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>  {\n>  \tstruct object_directory *ent;\n>  \tstruct strbuf pathbuf = STRBUF_INIT;\n> +\tstruct strbuf tmp = STRBUF_INIT;\n>  \tkhiter_t pos;\n>  \n>  \tif (!is_absolute_path(entry->buf) && relative_base) {\n> @@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>  \t}\n>  \tstrbuf_addbuf(&pathbuf, entry);\n>  \n> -\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n> +\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n>  \t\terror(_(\"unable to normalize alternate object path: %s\"),\n> -\t\t      pathbuf.buf);\n> +\t\t\tpathbuf.buf);\n\nThis is a mis-indentation, it was OK in the pre-image, not now.\n\n>  \t\tstrbuf_release(&pathbuf);\n\nDoesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the\n\"REALPATH_MANY_MISSING\" case it'll have allocated the \"resolved\" (the\n&tmp you pass in here) and then \"does a \"goto error_out\".\n\nIt then *resets* the strbuf, but doesn't release it, assuming that\nyou're going to pass it in again. So in that case we'd leak here, no?\n\nI.e. a NULL return value from strbuf_realpath() doesn't mean that it\ndidn't allocate in the scratch area passed to it, so we need to\nstrbuf_release(&tmp) here too.\n\nPerhaps this on top is simpler (but also see below)?:\n\t\n\tdiff --git a/object-file.c b/object-file.c\n\tindex ef2b762234d..d5d502504bb 100644\n\t--- a/object-file.c\n\t+++ b/object-file.c\n\t@@ -510,6 +510,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n\t \tstruct strbuf pathbuf = STRBUF_INIT;\n\t \tstruct strbuf tmp = STRBUF_INIT;\n\t \tkhiter_t pos;\n\t+\tint ret = -1;\n\t \n\t \tif (!is_absolute_path(entry->buf) && relative_base) {\n\t \t\tstrbuf_realpath(&pathbuf, relative_base, 1);\n\t@@ -520,11 +521,9 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n\t \tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n\t \t\terror(_(\"unable to normalize alternate object path: %s\"),\n\t \t\t\tpathbuf.buf);\n\t-\t\tstrbuf_release(&pathbuf);\n\t-\t\treturn -1;\n\t+\t\tgoto error;\n\t \t}\n\t \tstrbuf_swap(&pathbuf, &tmp);\n\t-\tstrbuf_release(&tmp);\n\t \n\t \t/*\n\t \t * The trailing slash after the directory name is given by\n\t@@ -533,10 +532,8 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n\t \twhile (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')\n\t \t\tstrbuf_setlen(&pathbuf, pathbuf.len - 1);\n\t \n\t-\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {\n\t-\t\tstrbuf_release(&pathbuf);\n\t-\t\treturn -1;\n\t-\t}\n\t+\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))\n\t+\t\tgoto error;\n\t \n\t \tCALLOC_ARRAY(ent, 1);\n\t \t/* pathbuf.buf is already in r->objects->odb_by_path */\n\t@@ -552,7 +549,11 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n\t \t/* recursively add alternates */\n\t \tread_info_alternates(r, ent->path, depth + 1);\n\t \n\t-\treturn 0;\n\t+\tret = 0;\n\t+error:\n\t+\tstrbuf_release(&tmp);\n\t+\tstrbuf_release(&pathbuf);\n\t+\treturn ret;\n\t }\n\t \n\t static const char *parse_alt_odb_entry(const char *string,\n\t\n\t\n\n>  \t\treturn -1;\n>  \t}\n> +\tstrbuf_swap(&pathbuf, &tmp);\n> +\tstrbuf_release(&tmp);\n>  \n>  \t/*\n>  \t * The trailing slash after the directory name is given by\n> @@ -596,10 +599,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n>  \t\treturn;\n>  \t}\n>  \n> -\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n> -\tif (strbuf_normalize_path(&objdirbuf) < 0)\n> -\t\tdie(_(\"unable to normalize object directory: %s\"),\n> -\t\t    objdirbuf.buf);\n> +\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n>  \n>  \twhile (*alt) {\n>  \t\talt = parse_alt_odb_entry(alt, sep, &entry);\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 5be483bf887..ce1954d0977 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n>  \ttest_has_duplicate_object false\n>  '\n>  \n> +test_expect_success '--local keeps packs when alternate is objectdir ' '\n> +\tgit init alt_symlink &&\n> +\t(\n> +\t\tcd alt_symlink &&\n> +\t\tgit init &&\n\nThe tests pass without this re-\"git init\", left over from development?\n\n> +\t\techo content >file4 &&\n> +\t\tgit add file4 &&\n> +\t\tgit commit -m commit_file4 &&\n> +\t\tgit repack -a &&\n> +\t\tls .git/objects/pack/*.pack >../expect &&\n> +\t\tln -s objects .git/alt_objects &&\n> +\t\techo \"$(pwd)/.git/alt_objects\" >.git/objects/info/alternates &&\n> +\t\tgit repack -a -d -l &&\n> +\t\tls .git/objects/pack/*.pack >../actual\n> +\t) &&\n> +\ttest_cmp expect actual\n> +'\n> +\n\nI think this is better squashed in:\n\t\n\tdiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n\tindex ce1954d0977..79eef5b4aa7 100755\n\t--- a/t/t7700-repack.sh\n\t+++ b/t/t7700-repack.sh\n\t@@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n\t '\n\t \n\t test_expect_success '--local keeps packs when alternate is objectdir ' '\n\t-\tgit init alt_symlink &&\n\t+\ttest_when_finished \"rm -rf repo\" &&\n\t+\tgit init repo &&\n\t+\ttest_commit -C repo A &&\n\t \t(\n\t-\t\tcd alt_symlink &&\n\t-\t\tgit init &&\n\t-\t\techo content >file4 &&\n\t-\t\tgit add file4 &&\n\t-\t\tgit commit -m commit_file4 &&\n\t+\t\tcd repo &&\n\t \t\tgit repack -a &&\n\t \t\tls .git/objects/pack/*.pack >../expect &&\n\t \t\tln -s objects .git/alt_objects &&\n\nBecause:\n\n * If it's not a setup for a later test let's call it \"repo\" and clean\n   it up at the end.\n\n * The \"file4\" you're creating doesn't go with the existing pattern, the\n   file{1..3} are created in the top-level .git, here you're making a\n   file4 in another repo.\n\n   This just calls it \"A.t\", and makes it with test_commit, since all\n   you need is a dummy commit.\n\n * I think we typically use \"find .. -type f\", not \"ls\", see\n   e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think\n   aside from that test there's some other \"let's compare the packed\n   before & after\" in the test suite, but I can't remember offhand...\n"},{"id":"467804","messageId":"Y30ls8yD7WES0pq9@coredump.intra.peff.net","threadId":"58811","inReplyTo":"pull.1382.v2.git.git.1669074557348.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] object-file: use real paths when adding alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-22T19:40:35Z","receivedAt":"2022-11-22T19:40:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2022 at 11:49:17PM +0000, Glen Choo via GitGitGadget wrote:\n\n>     Thanks for the feedback on v1. This version takes nearly all of Peff's\n>     patch [1] except for the comment about making an exception for relative\n>     paths in the environment. My reading of the commit [2] is that it was a\n>     workaround for strbuf_normalize_path() not being able to handle relative\n>     paths, so the only reason to special-case the environment is to preserve\n>     the behavior of respecting broken paths, which (unlike relative paths) I\n>     don't think will be missed.\n\nYeah, that makes sense. If realpath fails because a path isn't present,\nthen we would throw it away anyway. So we don't need to quietly\ntolerate, unless we really care about the difference between reporting\n\"this directory doesn't seem to exist\" versus \"I couldn't run realpath\non this directory\". One is a subset of the other.\n\n> diff --git a/object-file.c b/object-file.c\n> index 957790098fa..ef2b762234d 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>  {\n>  \tstruct object_directory *ent;\n>  \tstruct strbuf pathbuf = STRBUF_INIT;\n> +\tstruct strbuf tmp = STRBUF_INIT;\n>  \tkhiter_t pos;\n>  \n>  \tif (!is_absolute_path(entry->buf) && relative_base) {\n> @@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>  \t}\n>  \tstrbuf_addbuf(&pathbuf, entry);\n>  \n> -\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n> +\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n>  \t\terror(_(\"unable to normalize alternate object path: %s\"),\n> -\t\t      pathbuf.buf);\n> +\t\t\tpathbuf.buf);\n>  \t\tstrbuf_release(&pathbuf);\n>  \t\treturn -1;\n>  \t}\n> +\tstrbuf_swap(&pathbuf, &tmp);\n> +\tstrbuf_release(&tmp);\n\nSo here we are looking at an alternates entry (either from a file or\nfrom the environment). We do note all errors, even in relative ones from\nthe environment, but we don't die, so we'll just ignore the failed\nalternate. Good.\n\n> @@ -596,10 +599,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n>  \t\treturn;\n>  \t}\n>  \n> -\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n> -\tif (strbuf_normalize_path(&objdirbuf) < 0)\n> -\t\tdie(_(\"unable to normalize object directory: %s\"),\n> -\t\t    objdirbuf.buf);\n> +\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n\nAnd here we are resolving the actual object directory, and we always\ndied if that couldn't be normalized. And we'll continue to do so by\nrealpath. Good.\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n\nAnd then that's all we needed in the C code, since we already do\nduplicate checks. Good. :)\n\n> index 5be483bf887..ce1954d0977 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n>  \ttest_has_duplicate_object false\n>  '\n>  \n> +test_expect_success '--local keeps packs when alternate is objectdir ' '\n> +\tgit init alt_symlink &&\n> +\t(\n> +\t\tcd alt_symlink &&\n> +\t\tgit init &&\n> +\t\techo content >file4 &&\n> +\t\tgit add file4 &&\n> +\t\tgit commit -m commit_file4 &&\n> +\t\tgit repack -a &&\n> +\t\tls .git/objects/pack/*.pack >../expect &&\n> +\t\tln -s objects .git/alt_objects &&\n> +\t\techo \"$(pwd)/.git/alt_objects\" >.git/objects/info/alternates &&\n> +\t\tgit repack -a -d -l &&\n> +\t\tls .git/objects/pack/*.pack >../actual\n> +\t) &&\n> +\ttest_cmp expect actual\n> +'\n\nThis probably needs to be protected with a SYMLINKS prereq.\n\n-Peff\n"},{"id":"467806","messageId":"Y30onDTUFmAezkSl@coredump.intra.peff.net","threadId":"58811","inReplyTo":"221122.868rk3bxbb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] object-file: use real paths when adding alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-22T19:53:00Z","receivedAt":"2022-11-22T19:53:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2022 at 01:56:09AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > @@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n> >  \t}\n> >  \tstrbuf_addbuf(&pathbuf, entry);\n> >  \n> > -\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n> > +\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n> >  \t\terror(_(\"unable to normalize alternate object path: %s\"),\n> > -\t\t      pathbuf.buf);\n> > +\t\t\tpathbuf.buf);\n> \n> This is a mis-indentation, it was OK in the pre-image, not now.\n> \n> >  \t\tstrbuf_release(&pathbuf);\n> \n> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the\n> \"REALPATH_MANY_MISSING\" case it'll have allocated the \"resolved\" (the\n> &tmp you pass in here) and then \"does a \"goto error_out\".\n> \n> It then *resets* the strbuf, but doesn't release it, assuming that\n> you're going to pass it in again. So in that case we'd leak here, no?\n> \n> I.e. a NULL return value from strbuf_realpath() doesn't mean that it\n> didn't allocate in the scratch area passed to it, so we need to\n> strbuf_release(&tmp) here too.\n\nWe don't use MANY_MISSING in this code path, but I didn't read\nstrbuf_realpath_1() carefully enough to see if that is the only case.\nBut regardless, I think it is a bug in strbuf_realpath(). All of the\nstrbuf functions generally try to leave a buffer untouched on error.\n\nSo IMHO we would want a preparatory patch with s/reset/release/ in that\nfunction, which better matches the intent (we might be freeing an\nallocated buffer, but that's OK from the caller perspective). In theory\nit ought to just roll back the length for whatever it put into the\nbuffer, but it looks like the rest of the function is happy to clobber\nwhat's in the buf, even on non-error. That's why we have\nstrbuf_add_real_path(), but of course it doesn't allow for setting the\ndie_on_error flag.\n\n-Peff\n"},{"id":"467889","messageId":"kl6lzgchcieo.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58811","inReplyTo":"221122.868rk3bxbb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] object-file: use real paths when adding alternates","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-24T00:20:31Z","receivedAt":"2022-11-24T00:20:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>  object-file.c     | 12 ++++++------\n>>  t/t7700-repack.sh | 18 ++++++++++++++++++\n>>  2 files changed, 24 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/object-file.c b/object-file.c\n>> index 957790098fa..ef2b762234d 100644\n>> --- a/object-file.c\n>> +++ b/object-file.c\n>> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>>  {\n>>  \tstruct object_directory *ent;\n>>  \tstruct strbuf pathbuf = STRBUF_INIT;\n>> +\tstruct strbuf tmp = STRBUF_INIT;\n>>  \tkhiter_t pos;\n>>  \n>>  \tif (!is_absolute_path(entry->buf) && relative_base) {\n>> @@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n>>  \t}\n>>  \tstrbuf_addbuf(&pathbuf, entry);\n>>  \n>> -\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n>> +\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n>>  \t\terror(_(\"unable to normalize alternate object path: %s\"),\n>> -\t\t      pathbuf.buf);\n>> +\t\t\tpathbuf.buf);\n>\n> This is a mis-indentation, it was OK in the pre-image, not now.\n\nStrange, this came from \"make style\", and in the GitHub web UI, it shows\nthe next line as aligned with the opening \". Meh, I'll undo it.\n\n> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the\n> \"REALPATH_MANY_MISSING\" case it'll have allocated the \"resolved\" (the\n> &tmp you pass in here) and then \"does a \"goto error_out\".\n>\n> It then *resets* the strbuf, but doesn't release it, assuming that\n> you're going to pass it in again. So in that case we'd leak here, no?\n>\n> I.e. a NULL return value from strbuf_realpath() doesn't mean that it\n> didn't allocate in the scratch area passed to it, so we need to\n> strbuf_release(&tmp) here too.\n\nYeah, you're right. At any rate, it's a lot of cognitive overload to\ncheck if strbuf_realpath() will or won't allocate, so free()-ing in the\ncaller makes sense.\n\nSeparately, Peff mentioned that strbuf_realpath() not free()-ing is a\nreal bug, but I'll leave that for a future cleanup.\n\n>> +\t\techo content >file4 &&\n>> +\t\tgit add file4 &&\n>> +\t\tgit commit -m commit_file4 &&\n>> +\t\tgit repack -a &&\n>> +\t\tls .git/objects/pack/*.pack >../expect &&\n>> +\t\tln -s objects .git/alt_objects &&\n>> +\t\techo \"$(pwd)/.git/alt_objects\" >.git/objects/info/alternates &&\n>> +\t\tgit repack -a -d -l &&\n>> +\t\tls .git/objects/pack/*.pack >../actual\n>> +\t) &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>\n> I think this is better squashed in:\n> \t\n> \tdiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> \tindex ce1954d0977..79eef5b4aa7 100755\n> \t--- a/t/t7700-repack.sh\n> \t+++ b/t/t7700-repack.sh\n> \t@@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n> \t '\n> \t \n> \t test_expect_success '--local keeps packs when alternate is objectdir ' '\n> \t-\tgit init alt_symlink &&\n> \t+\ttest_when_finished \"rm -rf repo\" &&\n> \t+\tgit init repo &&\n> \t+\ttest_commit -C repo A &&\n> \t \t(\n> \t-\t\tcd alt_symlink &&\n> \t-\t\tgit init &&\n> \t-\t\techo content >file4 &&\n> \t-\t\tgit add file4 &&\n> \t-\t\tgit commit -m commit_file4 &&\n> \t+\t\tcd repo &&\n> \t \t\tgit repack -a &&\n> \t \t\tls .git/objects/pack/*.pack >../expect &&\n> \t \t\tln -s objects .git/alt_objects &&\n>\n> Because:\n>\n>  * If it's not a setup for a later test let's call it \"repo\" and clean\n>    it up at the end.\n>\n>  * The \"file4\" you're creating doesn't go with the existing pattern, the\n>    file{1..3} are created in the top-level .git, here you're making a\n>    file4 in another repo.\n>\n>    This just calls it \"A.t\", and makes it with test_commit, since all\n>    you need is a dummy commit.\n>\n>  * I think we typically use \"find .. -type f\", not \"ls\", see\n>    e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think\n>    aside from that test there's some other \"let's compare the packed\n>    before & after\" in the test suite, but I can't remember offhand...\n\nIt seems like t7700-repack.sh itself isn't consistent either (which is\nprobably how I ended up with \"ls\"). I'll also leave it alone unless\nsomeone has strong opinions.\n"},{"id":"467891","messageId":"kl6lwn7lch1h.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58811","inReplyTo":"Y30onDTUFmAezkSl@coredump.intra.peff.net","subject":"Re: [PATCH v2] object-file: use real paths when adding alternates","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-24T00:50:02Z","receivedAt":"2022-11-24T00:51:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the\n>> \"REALPATH_MANY_MISSING\" case it'll have allocated the \"resolved\" (the\n>> &tmp you pass in here) and then \"does a \"goto error_out\".\n>> \n>> It then *resets* the strbuf, but doesn't release it, assuming that\n>> you're going to pass it in again. So in that case we'd leak here, no?\n>> \n>> I.e. a NULL return value from strbuf_realpath() doesn't mean that it\n>> didn't allocate in the scratch area passed to it, so we need to\n>> strbuf_release(&tmp) here too.\n>\n> We don't use MANY_MISSING in this code path, but I didn't read\n> strbuf_realpath_1() carefully enough to see if that is the only case.\n> But regardless, I think it is a bug in strbuf_realpath(). All of the\n> strbuf functions generally try to leave a buffer untouched on error.\n>\n> So IMHO we would want a preparatory patch with s/reset/release/ in that\n> function, which better matches the intent (we might be freeing an\n> allocated buffer, but that's OK from the caller perspective).\n\nIs that always OK? I would think that we'd do something closer to\nstrbuf_getcwd():\n\n  int strbuf_getcwd(struct strbuf *sb)\n  {\n    size_t oldalloc = sb->alloc;\n    /* ... */\n    if (oldalloc == 0)\n      strbuf_release(sb);\n    else\n      strbuf_reset(sb);\n  }\n\ni.e. if the caller passed in a strbuf with allocated contents, they're\nresponsible for free()-ing it, otherwise we free() it. That does fix the\nleak in this patch, but I don't feel strongly enough about changing\nstrbuf_realpath() to do it now, so I'll do without the change for now.\n"},{"id":"467892","messageId":"pull.1382.v3.git.git.1669251331340.gitgitgadget@gmail.com","threadId":"58811","inReplyTo":"pull.1382.v2.git.git.1669074557348.gitgitgadget@gmail.com","subject":"[PATCH v3] object-file: use real paths when adding alternates","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-24T00:55:31Z","receivedAt":"2022-11-24T00:55:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nWhen adding an alternate ODB, we check if the alternate has the same\npath as the object dir, and if so, we do nothing. However, that\ncomparison does not resolve symlinks. This makes it possible to add the\nobject dir as an alternate, which may result in bad behavior. For\nexample, it can trick \"git repack -a -l -d\" (possibly run by \"git gc\")\ninto thinking that all packs come from an alternate and delete all\nobjects.\n\n\trm -rf test &&\n\tgit clone https://github.com/git/git test &&\n\t(\n\tcd test &&\n\tln -s objects .git/alt-objects &&\n\t# -c repack.updateserverinfo=false silences a warning about not\n\t# being able to update \"info/refs\", it isn't needed to show the\n\t# bad behavior\n\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\".git/alt-objects\" git \\\n\t\t-c repack.updateserverinfo=false repack -a -l -d  &&\n\t# It's broken!\n\tgit status\n\t# Because there are no more objects!\n\tls .git/objects/pack\n\t)\n\nFix this by resolving symlinks and relative paths before comparing the\nalternate and object dir. This lets us clean up a number of issues noted\nin 37a95862c6 (alternates: re-allow relative paths from environment,\n2016-11-07):\n\n- Now that we compare the real paths, duplicate detection is no longer\n  foiled by relative paths.\n- Using strbuf_realpath() allows us to \"normalize\" paths that\n  strbuf_normalize_path() can't, so we can stop silently ignoring errors\n  when \"normalizing\" paths from the environment.\n- We now store an absolute path based on getcwd() (the \"future\n  direction\" named in 37a95862c6), so chdir()-ing in the process no\n  longer changes the directory pointed to by the alternate. This is a\n  change in behavior, but a desirable one.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    object-file: use real paths when adding alternates\n    \n    Thanks all for the feedback on v2. Once again, this version takes nearly\n    all of Ævar's fixup patches [1] :)\n    \n    (It seems like the linux-* CI jobs are broken? I saw this tree pass\n    previously, but linux-* is failing to build all of a sudden.)\n    \n    Changes in v3:\n    \n     * strbuf_release() all strbufs\n     * Remove unnecessary details from the test (since it's a one-off, not\n       reused)\n    \n    [1]\n    https://lore.kernel.org/git/221122.868rk3bxbb.gmgdl@evledraar.gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1382%2Fchooglen%2Fobject-file%2Fcheck-alternate-real-path-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1382/chooglen/object-file/check-alternate-real-path-v3\nPull-Request: https://github.com/git/git/pull/1382\n\nRange-diff vs v2:\n\n 1:  ed9e12c0051 ! 1:  9bc04174be6 object-file: use real paths when adding alternates\n     @@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct\n       \tstruct strbuf pathbuf = STRBUF_INIT;\n      +\tstruct strbuf tmp = STRBUF_INIT;\n       \tkhiter_t pos;\n     ++\tint ret = -1;\n       \n       \tif (!is_absolute_path(entry->buf) && relative_base) {\n     + \t\tstrbuf_realpath(&pathbuf, relative_base, 1);\n      @@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n       \t}\n       \tstrbuf_addbuf(&pathbuf, entry);\n     @@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct\n      -\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n      +\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n       \t\terror(_(\"unable to normalize alternate object path: %s\"),\n     --\t\t      pathbuf.buf);\n     -+\t\t\tpathbuf.buf);\n     - \t\tstrbuf_release(&pathbuf);\n     - \t\treturn -1;\n     + \t\t      pathbuf.buf);\n     +-\t\tstrbuf_release(&pathbuf);\n     +-\t\treturn -1;\n     ++\t\tgoto error;\n       \t}\n      +\tstrbuf_swap(&pathbuf, &tmp);\n     -+\tstrbuf_release(&tmp);\n       \n       \t/*\n       \t * The trailing slash after the directory name is given by\n     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n     + \twhile (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')\n     + \t\tstrbuf_setlen(&pathbuf, pathbuf.len - 1);\n     + \n     +-\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {\n     +-\t\tstrbuf_release(&pathbuf);\n     +-\t\treturn -1;\n     +-\t}\n     ++\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))\n     ++\t\tgoto error;\n     + \n     + \tCALLOC_ARRAY(ent, 1);\n     + \t/* pathbuf.buf is already in r->objects->odb_by_path */\n     +@@ object-file.c: static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n     + \n     + \t/* recursively add alternates */\n     + \tread_info_alternates(r, ent->path, depth + 1);\n     +-\n     +-\treturn 0;\n     ++\tret = 0;\n     ++ error:\n     ++\tstrbuf_release(&tmp);\n     ++\tstrbuf_release(&pathbuf);\n     ++\treturn ret;\n     + }\n     + \n     + static const char *parse_alt_odb_entry(const char *string,\n      @@ object-file.c: static void link_alt_odb_entries(struct repository *r, const char *alt,\n       \t\treturn;\n       \t}\n     @@ t/t7700-repack.sh: test_expect_success 'loose objects in alternate ODB are not r\n       \ttest_has_duplicate_object false\n       '\n       \n     -+test_expect_success '--local keeps packs when alternate is objectdir ' '\n     -+\tgit init alt_symlink &&\n     ++test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir ' '\n     ++\ttest_when_finished \"rm -rf repo\" &&\n     ++\tgit init repo &&\n     ++\ttest_commit -C repo A &&\n      +\t(\n     -+\t\tcd alt_symlink &&\n     -+\t\tgit init &&\n     -+\t\techo content >file4 &&\n     -+\t\tgit add file4 &&\n     -+\t\tgit commit -m commit_file4 &&\n     ++\t\tcd repo &&\n      +\t\tgit repack -a &&\n      +\t\tls .git/objects/pack/*.pack >../expect &&\n      +\t\tln -s objects .git/alt_objects &&\n\n\n object-file.c     | 26 +++++++++++++-------------\n t/t7700-repack.sh | 16 ++++++++++++++++\n 2 files changed, 29 insertions(+), 13 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 957790098fa..26290554bb4 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -508,7 +508,9 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n {\n \tstruct object_directory *ent;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n \tkhiter_t pos;\n+\tint ret = -1;\n \n \tif (!is_absolute_path(entry->buf) && relative_base) {\n \t\tstrbuf_realpath(&pathbuf, relative_base, 1);\n@@ -516,12 +518,12 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n \t}\n \tstrbuf_addbuf(&pathbuf, entry);\n \n-\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n+\tif (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {\n \t\terror(_(\"unable to normalize alternate object path: %s\"),\n \t\t      pathbuf.buf);\n-\t\tstrbuf_release(&pathbuf);\n-\t\treturn -1;\n+\t\tgoto error;\n \t}\n+\tstrbuf_swap(&pathbuf, &tmp);\n \n \t/*\n \t * The trailing slash after the directory name is given by\n@@ -530,10 +532,8 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n \twhile (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')\n \t\tstrbuf_setlen(&pathbuf, pathbuf.len - 1);\n \n-\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {\n-\t\tstrbuf_release(&pathbuf);\n-\t\treturn -1;\n-\t}\n+\tif (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))\n+\t\tgoto error;\n \n \tCALLOC_ARRAY(ent, 1);\n \t/* pathbuf.buf is already in r->objects->odb_by_path */\n@@ -548,8 +548,11 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,\n \n \t/* recursively add alternates */\n \tread_info_alternates(r, ent->path, depth + 1);\n-\n-\treturn 0;\n+\tret = 0;\n+ error:\n+\tstrbuf_release(&tmp);\n+\tstrbuf_release(&pathbuf);\n+\treturn ret;\n }\n \n static const char *parse_alt_odb_entry(const char *string,\n@@ -596,10 +599,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,\n \t\treturn;\n \t}\n \n-\tstrbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);\n-\tif (strbuf_normalize_path(&objdirbuf) < 0)\n-\t\tdie(_(\"unable to normalize object directory: %s\"),\n-\t\t    objdirbuf.buf);\n+\tstrbuf_realpath(&objdirbuf, r->objects->odb->path, 1);\n \n \twhile (*alt) {\n \t\talt = parse_alt_odb_entry(alt, sep, &entry);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 5be483bf887..599b4499b8e 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -90,6 +90,22 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '\n \ttest_has_duplicate_object false\n '\n \n+test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir ' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo A &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit repack -a &&\n+\t\tls .git/objects/pack/*.pack >../expect &&\n+\t\tln -s objects .git/alt_objects &&\n+\t\techo \"$(pwd)/.git/alt_objects\" >.git/objects/info/alternates &&\n+\t\tgit repack -a -d -l &&\n+\t\tls .git/objects/pack/*.pack >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '\n \tmkdir alt_objects/pack &&\n \tmv .git/objects/pack/* alt_objects/pack &&\n\nbase-commit: 319605f8f00e402f3ea758a02c63534ff800a711\n-- \ngitgitgadget\n"},{"id":"467895","messageId":"Y37DprbgD2Wg1PMZ@coredump.intra.peff.net","threadId":"58811","inReplyTo":"kl6lwn7lch1h.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] object-file: use real paths when adding alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-24T01:06:46Z","receivedAt":"2022-11-24T01:06:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 23, 2022 at 04:50:02PM -0800, Glen Choo wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the\n> >> \"REALPATH_MANY_MISSING\" case it'll have allocated the \"resolved\" (the\n> >> &tmp you pass in here) and then \"does a \"goto error_out\".\n> >> \n> >> It then *resets* the strbuf, but doesn't release it, assuming that\n> >> you're going to pass it in again. So in that case we'd leak here, no?\n> >> \n> >> I.e. a NULL return value from strbuf_realpath() doesn't mean that it\n> >> didn't allocate in the scratch area passed to it, so we need to\n> >> strbuf_release(&tmp) here too.\n> >\n> > We don't use MANY_MISSING in this code path, but I didn't read\n> > strbuf_realpath_1() carefully enough to see if that is the only case.\n> > But regardless, I think it is a bug in strbuf_realpath(). All of the\n> > strbuf functions generally try to leave a buffer untouched on error.\n> >\n> > So IMHO we would want a preparatory patch with s/reset/release/ in that\n> > function, which better matches the intent (we might be freeing an\n> > allocated buffer, but that's OK from the caller perspective).\n> \n> Is that always OK? I would think that we'd do something closer to\n> strbuf_getcwd():\n> \n>   int strbuf_getcwd(struct strbuf *sb)\n>   {\n>     size_t oldalloc = sb->alloc;\n>     /* ... */\n>     if (oldalloc == 0)\n>       strbuf_release(sb);\n>     else\n>       strbuf_reset(sb);\n>   }\n> \n> i.e. if the caller passed in a strbuf with allocated contents, they're\n> responsible for free()-ing it, otherwise we free() it. That does fix the\n> leak in this patch, but I don't feel strongly enough about changing\n> strbuf_realpath() to do it now, so I'll do without the change for now.\n\nThat's what I was getting at with \"that's OK from the caller\nperspective\". strbuf_realpath() is also unlike other strbuf functions in\nthat it clobbers the contents of the buffer, even on success (rather\nthan adding on success and rolling back to the original state on error).\n\nSince the caller is OK with the buffer being clobbered anyway, it should\nnot matter to it whether we clobbered an allocated buffer back to an\nunallocated one on error. The confusing thing (and the current behavior)\nis when we do the opposite: change an unallocated one to an allocated\none.\n\n-Peff\n"},{"id":"467896","messageId":"Y37EEiJembUN9PmL@coredump.intra.peff.net","threadId":"58811","inReplyTo":"pull.1382.v3.git.git.1669251331340.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] object-file: use real paths when adding alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-24T01:08:34Z","receivedAt":"2022-11-24T01:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 24, 2022 at 12:55:31AM +0000, Glen Choo via GitGitGadget wrote:\n\n>     object-file: use real paths when adding alternates\n>     \n>     Thanks all for the feedback on v2. Once again, this version takes nearly\n>     all of Ævar's fixup patches [1] :)\n\nThis looks fine to me. I'd probably have done the strbuf_realpath()\nchange I suggested, but I don't mind if you want to punt on it for now.\n\n-Peff\n"},{"id":"467973","messageId":"xmqqcz9bedd3.fsf@gitster.g","threadId":"58811","inReplyTo":"Y37EEiJembUN9PmL@coredump.intra.peff.net","subject":"Re: [PATCH v3] object-file: use real paths when adding alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-25T06:51:04Z","receivedAt":"2022-11-25T06:51:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Nov 24, 2022 at 12:55:31AM +0000, Glen Choo via GitGitGadget wrote:\n>\n>>     object-file: use real paths when adding alternates\n>>     \n>>     Thanks all for the feedback on v2. Once again, this version takes nearly\n>>     all of Ævar's fixup patches [1] :)\n>\n> This looks fine to me. I'd probably have done the strbuf_realpath()\n> change I suggested, but I don't mind if you want to punt on it for now.\n\nThanks, both.\n\nWhen we designed the alternates long time ago, the \"avoid placing\nduplicate directories on list\" was done merely as an optimization\nand not as a correctness measure, as there was no plan to add\nanything destructive like \"delete all non-local objects\".  But now\nthis seems to matter X-<.\n\n"}]}