{"thread":{"id":"52724","subject":"[PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere","startedAt":"2020-01-30T23:00:44Z","lastAt":"2020-02-05T12:30:21Z","messageCount":28,"participants":["Taylor Blau","Martin Ågren","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"390865","messageId":"cover.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":null,"subject":"[PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:40Z","receivedAt":"2020-01-30T23:00:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nWhile working on deploying layered commit-graphs at GitHub, I noticed\nthat I was able to trigger the mechanism to prevent merging\ncross-alternate commit-graph layers in funny ways, like claiming that\n\"./objects\" and \"objects\" were in different directories.\n\nThis resulted in removing all of the places in the commit-graph.c code\nthat use 'char *object_dir's with 'struct object_directory *'s instead.\nThis allows us to replace brittle string-based path comparisons with raw\npointer comparisons to check whether two commit-graphs belong to the\nsame object store or not.\n\nThis has a pleasant side effect in PATCH 5/6 of fixing an uninitialized\nread as described in [1].\n\nThis series became a little bit longer than I was expecting it to be, so\nhere is the high-level structure:\n\n  - 1/6 fixes a bug in a test that would cause a subsequent failure if\n    unaddressed.\n\n  - 2/6 does the first half of the removal by using 'struct\n    object_directory *'s within the 'commit_graph' structure.\n\n  - 4/6 does the second half by removing 'char *object_dir' usage in the\n    'write_commit_graph_context' structure.\n\n  - 5/6 ties 2/6 and 4/6 together by removing all path normalization\n    completely, fixing the uninitialized read bug.\n\n  - And 6/6 cleans up.\n\nWe've been running a version of this series based on our fork (which is\nin turn based on 2.24) for a few hours without issue.\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/20191027042116.GA5801@sigill.intra.peff.net/\n\nTaylor Blau (6):\n  t5318: don't pass non-object directory to '--object-dir'\n  commit-graph.h: store object directory in 'struct commit_graph'\n  builtin/commit-graph.c: die() with unknown '--object-dir'\n  commit-graph.h: store an odb in 'struct write_commit_graph_context'\n  commit-graph.c: remove path normalization, comparison\n  commit-graph.h: use odb in 'load_commit_graph_one_fd_st'\n\n Documentation/git-commit-graph.txt |   5 +-\n builtin/commit-graph.c             |  25 ++++--\n builtin/commit.c                   |   3 +-\n builtin/fetch.c                    |   2 +-\n builtin/gc.c                       |   2 +-\n commit-graph.c                     | 135 +++++++++++++++--------------\n commit-graph.h                     |  17 ++--\n t/helper/test-read-graph.c         |   8 +-\n t/t5318-commit-graph.sh            |   4 +-\n 9 files changed, 114 insertions(+), 87 deletions(-)\n\n--\n2.25.0.dirty\n"},{"id":"390866","messageId":"7b22efb0c61046a5864afeb052332a3a53331533.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH 1/6] t5318: don't pass non-object directory to '--object-dir'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:43Z","receivedAt":"2020-01-30T23:00:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In f237c8b6fe (commit-graph: implement git-commit-graph write,\n2018-04-02) the test t5318.3 was introduced to ensure that calling 'git\ncommit-graph write' in a repository with no packfiles does not write any\ncommit-graph file(s).\n\nTo exercise more paths in 'builtin/commit-graph.c', this test passes\n'--object-dir' to 'git commit-graph write', but the given argument\nrefers to the working copy, not the object directory.\n\nSince the commit-graph sub-commands currently swallow these errors, this\ndoes not result in a test failure. But, it is only lucky that the test\nends with no commit-graphs, since there were none to begin with.\n\nIn preparation for a future commit where an '--object-dir' argument that\ndoes not match a known object directory will print out a failure, let's\nfix the test to still use '--object-dir', but pass the correct location\nto the object store instead of '.'.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5318-commit-graph.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 3f03de6018..0bf98b56ec 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -19,8 +19,8 @@ test_expect_success 'verify graph with no graph file' '\n \n test_expect_success 'write graph with no packs' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-\tgit commit-graph write --object-dir . &&\n-\ttest_path_is_missing info/commit-graph\n+\tgit commit-graph write --object-dir $objdir &&\n+\ttest_path_is_missing $objdir/info/commit-graph\n '\n \n test_expect_success 'exit with correct error on bad input to --stdin-packs' '\n-- \n2.25.0.dirty\n\n"},{"id":"390867","messageId":"be415874f5e5021922a0f2d4e8171ed6ebad67b1.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH 5/6] commit-graph.c: remove path normalization, comparison","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:45Z","receivedAt":"2020-01-30T23:00:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of the previous patch, all calls to 'commit-graph.c' functions which\nperform path normalization (for e.g., 'get_commit_graph_filename()') are\nof the form 'ctx->odb->path', which is always in normalized form.\n\nNow that there are no callers passing non-normalized paths to these\nfunctions, ensure that future callers are bound by the same restrictions\nby making these functions take a 'struct object_directory *' instead of\na 'const char *'. To match, replace all calls with arguments of the form\n'ctx->odb->path' with 'ctx->odb' To recover the path, functions that\nperform path manipulation simply use 'odb->path'.\n\nFurther, avoid string comparisons with arguments of the form\n'odb->path', and instead prefer raw pointer comparisons, which\naccomplish the same effect, but are far less brittle.\n\nThis has a pleasant side-effect of making these functions much more\nrobust to paths that cannot be normalized by 'normalize_path_copy()',\ni.e., because they are outside of the current working directory.\n\nFor example, prior to this patch, Valgrind reports that the following\nuninitialized memory read [1]:\n\n  $ ( cd t && GIT_DIR=../.git valgrind git rev-parse HEAD^ )\n\nbecause 'normalize_path_copy()' can't normalize '../.git' (since it's\nrelative to but above of the current working directory) [2].\n\nBy using a 'struct object_directory *' directly,\n'get_commit_graph_filename()' does not need to normalize, because all\npaths are relative to the current working directory since they are\nalways read from the '->path' of an object directory.\n\n[1]: https://lore.kernel.org/git/20191027042116.GA5801@sigill.intra.peff.net.\n[2]: The bug here is that 'get_commit_graph_filename()' returns the\n     result of 'normalize_path_copy()' without checking the return\n     value.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c     | 11 ++++-----\n commit-graph.c             | 47 +++++++++++++++-----------------------\n commit-graph.h             |  2 +-\n t/helper/test-read-graph.c |  6 ++---\n 4 files changed, 28 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 9b1148a60a..4ab045395e 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -46,6 +46,7 @@ static struct object_directory *find_odb_or_die(struct repository *r,\n static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n+\tstruct object_directory *odb = NULL;\n \tchar *graph_name;\n \tint open_ok;\n \tint fd;\n@@ -76,7 +77,8 @@ static int graph_verify(int argc, const char **argv)\n \tif (opts.progress)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n-\tgraph_name = get_commit_graph_filename(opts.obj_dir);\n+\todb = find_odb_or_die(the_repository, opts.obj_dir);\n+\tgraph_name = get_commit_graph_filename(odb);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok && errno != ENOENT)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n@@ -85,11 +87,8 @@ static int graph_verify(int argc, const char **argv)\n \n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n-\telse {\n-\t\tstruct object_directory *odb;\n-\t\tif ((odb = find_odb_or_die(the_repository, opts.obj_dir)))\n-\t\t\tgraph = read_commit_graph_one(the_repository, odb);\n-\t}\n+\telse\n+\t\tgraph = read_commit_graph_one(the_repository, odb);\n \n \t/* Return failure if open_ok predicted success */\n \tif (!graph)\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 19889e5fea..09316240f0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -44,30 +44,21 @@\n /* Remember to update object flag allocation in object.h */\n #define REACHABLE       (1u<<15)\n \n-char *get_commit_graph_filename(const char *obj_dir)\n+char *get_commit_graph_filename(struct object_directory *odb)\n {\n-\tchar *filename = xstrfmt(\"%s/info/commit-graph\", obj_dir);\n-\tchar *normalized = xmalloc(strlen(filename) + 1);\n-\tnormalize_path_copy(normalized, filename);\n-\tfree(filename);\n-\treturn normalized;\n+\treturn xstrfmt(\"%s/info/commit-graph\", odb->path);\n }\n \n-static char *get_split_graph_filename(const char *obj_dir,\n+static char *get_split_graph_filename(struct object_directory *odb,\n \t\t\t\t      const char *oid_hex)\n {\n-\tchar *filename = xstrfmt(\"%s/info/commit-graphs/graph-%s.graph\",\n-\t\t\t\t obj_dir,\n-\t\t\t\t oid_hex);\n-\tchar *normalized = xmalloc(strlen(filename) + 1);\n-\tnormalize_path_copy(normalized, filename);\n-\tfree(filename);\n-\treturn normalized;\n+\treturn xstrfmt(\"%s/info/commit-graphs/graph-%s.graph\", odb->path,\n+\t\t       oid_hex);\n }\n \n-static char *get_chain_filename(const char *obj_dir)\n+static char *get_chain_filename(struct object_directory *odb)\n {\n-\treturn xstrfmt(\"%s/info/commit-graphs/commit-graph-chain\", obj_dir);\n+\treturn xstrfmt(\"%s/info/commit-graphs/commit-graph-chain\", odb->path);\n }\n \n static uint8_t oid_version(void)\n@@ -350,7 +341,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb)\n {\n-\tchar *graph_name = get_commit_graph_filename(odb->path);\n+\tchar *graph_name = get_commit_graph_filename(odb);\n \tstruct commit_graph *g = load_commit_graph_one(graph_name);\n \tfree(graph_name);\n \n@@ -401,7 +392,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n-\tchar *chain_name = get_chain_filename(odb->path);\n+\tchar *chain_name = get_chain_filename(odb);\n \tFILE *fp;\n \tint stat_res;\n \n@@ -434,7 +425,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \n \t\tvalid = 0;\n \t\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n-\t\t\tchar *graph_name = get_split_graph_filename(odb->path, line.buf);\n+\t\t\tchar *graph_name = get_split_graph_filename(odb, line.buf);\n \t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n \n \t\t\tfree(graph_name);\n@@ -1395,7 +1386,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t    ctx->odb->path);\n \t\tctx->graph_name = strbuf_detach(&tmp_file, NULL);\n \t} else {\n-\t\tctx->graph_name = get_commit_graph_filename(ctx->odb->path);\n+\t\tctx->graph_name = get_commit_graph_filename(ctx->odb);\n \t}\n \n \tif (safe_create_leading_directories(ctx->graph_name)) {\n@@ -1406,7 +1397,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n \n \tif (ctx->split) {\n-\t\tchar *lock_name = get_chain_filename(ctx->odb->path);\n+\t\tchar *lock_name = get_chain_filename(ctx->odb);\n \n \t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n \n@@ -1494,7 +1485,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \tif (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {\n \t\tchar *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));\n-\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);\n+\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb, new_base_hash);\n \n \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);\n \t\tfree(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);\n@@ -1530,12 +1521,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb->path);\n+\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb);\n \t\t\tunlink(graph_name);\n \t\t}\n \n \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));\n-\t\tfinal_graph_name = get_split_graph_filename(ctx->odb->path,\n+\t\tfinal_graph_name = get_split_graph_filename(ctx->odb,\n \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n \n@@ -1577,7 +1568,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->odb->path, ctx->odb->path))\n+\t\tif (g->odb != ctx->odb)\n \t\t\tbreak;\n \n \t\tnum_commits += g->num_commits;\n@@ -1589,10 +1580,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \tctx->new_base_graph = g;\n \n \tif (ctx->num_commit_graphs_after == 2) {\n-\t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n+\t\tchar *old_graph_name = get_commit_graph_filename(g->odb);\n \n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->odb->path, ctx->odb->path)) {\n+\t\t    g->odb != ctx->odb) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1743,7 +1734,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n \t\texpire_time -= ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n-\t\tchar *chain_file_name = get_chain_filename(ctx->odb->path);\n+\t\tchar *chain_file_name = get_chain_filename(ctx->odb);\n \t\tunlink(chain_file_name);\n \t\tfree(chain_file_name);\n \t\tctx->num_commit_graphs_after = 0;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 39c0c0e51c..97e2bce313 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -12,7 +12,7 @@\n \n struct commit;\n \n-char *get_commit_graph_filename(const char *obj_dir);\n+char *get_commit_graph_filename(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n \n struct object_directory *find_odb(struct repository *r, const char *obj_dir);\ndiff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\nindex d2884efe0a..2c2f65f06c 100644\n--- a/t/helper/test-read-graph.c\n+++ b/t/helper/test-read-graph.c\n@@ -11,12 +11,12 @@ int cmd__read_graph(int argc, const char **argv)\n \tint open_ok;\n \tint fd;\n \tstruct stat st;\n-\tconst char *object_dir;\n+\tstruct object_directory *odb;\n \n \tsetup_git_directory();\n-\tobject_dir = get_object_directory();\n+\todb = the_repository->objects->odb;\n \n-\tgraph_name = get_commit_graph_filename(object_dir);\n+\tgraph_name = get_commit_graph_filename(odb);\n \n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok)\n-- \n2.25.0.dirty\n\n"},{"id":"390868","messageId":"15c74d0f65545d7ceb258712e27c388b58cd54d9.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH 6/6] commit-graph.h: use odb in 'load_commit_graph_one_fd_st'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:47Z","receivedAt":"2020-01-30T23:00:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply a similar treatment as in the previous patch to pass a 'struct\nobject_directory *' through the 'load_commit_graph_one_fd_st'\ninitializer, too.\n\nThis prevents a potential bug where a pointer comparison is made to a\nNULL 'g->odb', which would cause the commit-graph machinery to think\nthat a pair of commit-graphs belonged to different alternates when in\nfact they do not (i.e., in the case of no '--object-dir').\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c     |  2 +-\n commit-graph.c             | 21 ++++++++++-----------\n commit-graph.h             |  3 ++-\n t/helper/test-read-graph.c |  2 +-\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 4ab045395e..de321c71ad 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -86,7 +86,7 @@ static int graph_verify(int argc, const char **argv)\n \tFREE_AND_NULL(graph_name);\n \n \tif (open_ok)\n-\t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n+\t\tgraph = load_commit_graph_one_fd_st(fd, &st, odb);\n \telse\n \t\tgraph = read_commit_graph_one(the_repository, odb);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 09316240f0..6d34829f57 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -128,7 +128,8 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n+\t\t\t\t\t\t struct object_directory *odb)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n@@ -144,7 +145,9 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tret = parse_commit_graph(graph_map, fd, graph_size);\n \n-\tif (!ret) {\n+\tif (ret)\n+\t\tret->odb = odb;\n+\telse {\n \t\tmunmap(graph_map, graph_size);\n \t\tclose(fd);\n \t}\n@@ -319,7 +322,8 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \treturn graph;\n }\n \n-static struct commit_graph *load_commit_graph_one(const char *graph_file)\n+static struct commit_graph *load_commit_graph_one(const char *graph_file,\n+\t\t\t\t\t\t  struct object_directory *odb)\n {\n \n \tstruct stat st;\n@@ -330,7 +334,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!open_ok)\n \t\treturn NULL;\n \n-\tg = load_commit_graph_one_fd_st(fd, &st);\n+\tg = load_commit_graph_one_fd_st(fd, &st, odb);\n \n \tif (g)\n \t\tg->filename = xstrdup(graph_file);\n@@ -342,12 +346,9 @@ static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb)\n {\n \tchar *graph_name = get_commit_graph_filename(odb);\n-\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n+\tstruct commit_graph *g = load_commit_graph_one(graph_name, odb);\n \tfree(graph_name);\n \n-\tif (g)\n-\t\tg->odb = odb;\n-\n \treturn g;\n }\n \n@@ -426,13 +427,11 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \t\tvalid = 0;\n \t\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n \t\t\tchar *graph_name = get_split_graph_filename(odb, line.buf);\n-\t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n+\t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name, odb);\n \n \t\t\tfree(graph_name);\n \n \t\t\tif (g) {\n-\t\t\t\tg->odb = odb;\n-\n \t\t\t\tif (add_graph_to_chain(g, graph_chain, oids, i)) {\n \t\t\t\t\tgraph_chain = g;\n \t\t\t\t\tvalid = 1;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 97e2bce313..7d9fc9c16a 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -63,7 +63,8 @@ struct commit_graph {\n \tconst unsigned char *chunk_base_graphs;\n };\n \n-struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n+\t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\ndiff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\nindex 2c2f65f06c..f8a461767c 100644\n--- a/t/helper/test-read-graph.c\n+++ b/t/helper/test-read-graph.c\n@@ -22,7 +22,7 @@ int cmd__read_graph(int argc, const char **argv)\n \tif (!open_ok)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n-\tgraph = load_commit_graph_one_fd_st(fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st, odb);\n \tif (!graph)\n \t\treturn 1;\n \n-- \n2.25.0.dirty\n"},{"id":"390869","messageId":"890e0e7136204f5ca47f0703f32b4adb99ad8d7e.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:50Z","receivedAt":"2020-01-30T23:00:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"There are lots of places in 'commit-graph.h' where a function either has\n(or almost has) a full 'struct object_directory *', accesses '->path',\nand then throws away the rest of the struct.\n\nThis can cause headaches when comparing the locations of object\ndirectories across alternates (e.g., in the case of deciding if two\ncommit-graph layers can be merged). These paths are normalized with\n'normalize_path_copy()' which mitigates some comparison issues, but not\nall [1].\n\nInstead of getting rid of the 'struct object_directory *', store that\ninsead of a 'char *odb' in 'struct commit_graph'. Once the 'struct\nwrite_commit_graph_context' has an object_directory pointer, too, this\nwill allow calling code to replace these error-prone path comparisons\nwith raw pointer comparisons, thereby circumventing any\nnormalization-related errors. This will be introduced in a subsequent\npatch.\n\n[1]: In my testing, for example, I can get one side of the commit-graph\ncode to fill object_dir with \"./objects\" and the other with just\n\"objects\".\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c | 13 +++++++---\n builtin/commit.c       |  1 +\n commit-graph.c         | 59 ++++++++++++++++++++++++++++++------------\n commit-graph.h         |  8 ++++--\n 4 files changed, 58 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex e0c6fc4bbf..3edac318e8 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -76,8 +76,11 @@ static int graph_verify(int argc, const char **argv)\n \n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n-\t else\n-\t\tgraph = read_commit_graph_one(the_repository, opts.obj_dir);\n+\telse {\n+\t\tstruct object_directory *odb;\n+\t\tif ((odb = find_odb(the_repository, opts.obj_dir)))\n+\t\t\tgraph = read_commit_graph_one(the_repository, odb);\n+\t}\n \n \t/* Return failure if open_ok predicted success */\n \tif (!graph)\n@@ -97,6 +100,7 @@ static int graph_write(int argc, const char **argv)\n \tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n+\tstruct object_directory *odb = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -145,9 +149,10 @@ static int graph_write(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \tread_replace_refs = 0;\n+\todb = find_odb(the_repository, opts.obj_dir);\n \n \tif (opts.reachable) {\n-\t\tif (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))\n+\t\tif (write_commit_graph_reachable(odb->path, flags, &split_opts))\n \t\t\treturn 1;\n \t\treturn 0;\n \t}\n@@ -169,7 +174,7 @@ static int graph_write(int argc, const char **argv)\n \t\tUNLEAK(buf);\n \t}\n \n-\tif (write_commit_graph(opts.obj_dir,\n+\tif (write_commit_graph(odb->path,\n \t\t\t       pack_indexes,\n \t\t\t       commit_hex,\n \t\t\t       flags,\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex aa1332308a..bd071169d7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -36,6 +36,7 @@\n #include \"help.h\"\n #include \"commit-reach.h\"\n #include \"commit-graph.h\"\n+#include \"object-store.h\"\n \n static const char * const builtin_commit_usage[] = {\n \tN_(\"git commit [<options>] [--] <pathspec>...\"),\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b205e65ed1..2c06876b26 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -75,6 +75,26 @@ static uint8_t oid_version(void)\n \treturn 1;\n }\n \n+struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n+{\n+\tstruct object_directory *odb;\n+\tchar *obj_dir_real = real_pathdup(obj_dir, 1);\n+\tint cmp = -1;\n+\n+\tprepare_alt_odb(r);\n+\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n+\t\tcmp = strcmp(obj_dir_real, real_path(odb->path));\n+\t\tif (!cmp)\n+\t\t\tbreak;\n+\t}\n+\n+\tfree(obj_dir_real);\n+\n+\tif (cmp)\n+\t\todb = NULL;\n+\treturn odb;\n+}\n+\n static struct commit_graph *alloc_commit_graph(void)\n {\n \tstruct commit_graph *g = xcalloc(1, sizeof(*g));\n@@ -327,14 +347,15 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n \treturn g;\n }\n \n-static struct commit_graph *load_commit_graph_v1(struct repository *r, const char *obj_dir)\n+static struct commit_graph *load_commit_graph_v1(struct repository *r,\n+\t\t\t\t\t\t struct object_directory *odb)\n {\n-\tchar *graph_name = get_commit_graph_filename(obj_dir);\n+\tchar *graph_name = get_commit_graph_filename(odb->path);\n \tstruct commit_graph *g = load_commit_graph_one(graph_name);\n \tfree(graph_name);\n \n \tif (g)\n-\t\tg->obj_dir = obj_dir;\n+\t\tg->odb = odb;\n \n \treturn g;\n }\n@@ -372,14 +393,15 @@ static int add_graph_to_chain(struct commit_graph *g,\n \treturn 1;\n }\n \n-static struct commit_graph *load_commit_graph_chain(struct repository *r, const char *obj_dir)\n+static struct commit_graph *load_commit_graph_chain(struct repository *r,\n+\t\t\t\t\t\t    struct object_directory *odb)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n-\tchar *chain_name = get_chain_filename(obj_dir);\n+\tchar *chain_name = get_chain_filename(odb->path);\n \tFILE *fp;\n \tint stat_res;\n \n@@ -418,7 +440,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, const\n \t\t\tfree(graph_name);\n \n \t\t\tif (g) {\n-\t\t\t\tg->obj_dir = odb->path;\n+\t\t\t\tg->odb = odb;\n \n \t\t\t\tif (add_graph_to_chain(g, graph_chain, oids, i)) {\n \t\t\t\t\tgraph_chain = g;\n@@ -442,23 +464,25 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, const\n \treturn graph_chain;\n }\n \n-struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir)\n+struct commit_graph *read_commit_graph_one(struct repository *r,\n+\t\t\t\t\t   struct object_directory *odb)\n {\n-\tstruct commit_graph *g = load_commit_graph_v1(r, obj_dir);\n+\tstruct commit_graph *g = load_commit_graph_v1(r, odb);\n \n \tif (!g)\n-\t\tg = load_commit_graph_chain(r, obj_dir);\n+\t\tg = load_commit_graph_chain(r, odb);\n \n \treturn g;\n }\n \n-static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n+static void prepare_commit_graph_one(struct repository *r,\n+\t\t\t\t     struct object_directory *odb)\n {\n \n \tif (r->objects->commit_graph)\n \t\treturn;\n \n-\tr->objects->commit_graph = read_commit_graph_one(r, obj_dir);\n+\tr->objects->commit_graph = read_commit_graph_one(r, odb);\n }\n \n /*\n@@ -505,7 +529,7 @@ static int prepare_commit_graph(struct repository *r)\n \tfor (odb = r->objects->odb;\n \t     !r->objects->commit_graph && odb;\n \t     odb = odb->next)\n-\t\tprepare_commit_graph_one(r, odb->path);\n+\t\tprepare_commit_graph_one(r, odb);\n \treturn !!r->objects->commit_graph;\n }\n \n@@ -1470,7 +1494,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \tif (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {\n \t\tchar *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));\n-\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->obj_dir, new_base_hash);\n+\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);\n \n \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);\n \t\tfree(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);\n@@ -1553,7 +1577,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->obj_dir, ctx->obj_dir))\n+\t\tif (strcmp(g->odb->path, ctx->obj_dir))\n \t\t\tbreak;\n \n \t\tnum_commits += g->num_commits;\n@@ -1565,10 +1589,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \tctx->new_base_graph = g;\n \n \tif (ctx->num_commit_graphs_after == 2) {\n-\t\tchar *old_graph_name = get_commit_graph_filename(g->obj_dir);\n+\t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n \n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->obj_dir, ctx->obj_dir)) {\n+\t\t    strcmp(g->odb->path, ctx->obj_dir)) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1824,7 +1848,8 @@ int write_commit_graph(const char *obj_dir,\n \t\tctx->oids.alloc = split_opts->max_commits;\n \n \tif (ctx->append) {\n-\t\tprepare_commit_graph_one(ctx->r, ctx->obj_dir);\n+\t\tstruct object_directory *odb = find_odb(ctx->r, ctx->obj_dir);\n+\t\tprepare_commit_graph_one(ctx->r, odb);\n \t\tif (ctx->r->objects->commit_graph)\n \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 7f5c933fa2..9700a6c7c2 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -5,6 +5,7 @@\n #include \"repository.h\"\n #include \"string-list.h\"\n #include \"cache.h\"\n+#include \"object-store.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n@@ -14,6 +15,8 @@ struct commit;\n char *get_commit_graph_filename(const char *obj_dir);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n \n+struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n+\n /*\n  * Given a commit struct, try to fill the commit struct info, including:\n  *  1. tree object\n@@ -48,7 +51,7 @@ struct commit_graph {\n \tuint32_t num_commits;\n \tstruct object_id oid;\n \tchar *filename;\n-\tconst char *obj_dir;\n+\tstruct object_directory *odb;\n \n \tuint32_t num_commits_in_base;\n \tstruct commit_graph *base_graph;\n@@ -61,7 +64,8 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n-struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir);\n+struct commit_graph *read_commit_graph_one(struct repository *r,\n+\t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\n \n-- \n2.25.0.dirty\n\n"},{"id":"390870","messageId":"3ddec36ba3f8929e5099fd9476de2cdfd06db4d7.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH 4/6] commit-graph.h: store an odb in 'struct write_commit_graph_context'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:52Z","receivedAt":"2020-01-30T23:00:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a previous patch, the 'char *object_dir' in 'struct commit_graph' was\nreplaced with a 'struct object_directory'. This patch applies the same\ntreatement to 'struct write_commit_graph_context', which is an\nintermediate step towards getting rid of all path normalization in\n'commit-graph.c'.\n\nInstead of taking a 'char *object_dir', functions that construct a\n'struct write_commit_graph_context' now take a 'struct object_directory\n*'. Any code that needs an object directory path use '->path' instead.\n\nThis ensures that all calls to functions that perform path normalization\nare given arguments which do not themselves require normalization. This\nprepares those functions to drop their normalization entirely, which\nwill occur in the subsequent patch.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c |  4 ++--\n builtin/commit.c       |  2 +-\n builtin/fetch.c        |  2 +-\n builtin/gc.c           |  2 +-\n commit-graph.c         | 42 ++++++++++++++++--------------------------\n commit-graph.h         |  4 ++--\n 6 files changed, 23 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 93ff90d73b..9b1148a60a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -161,7 +161,7 @@ static int graph_write(int argc, const char **argv)\n \todb = find_odb_or_die(the_repository, opts.obj_dir);\n \n \tif (opts.reachable) {\n-\t\tif (write_commit_graph_reachable(odb->path, flags, &split_opts))\n+\t\tif (write_commit_graph_reachable(odb, flags, &split_opts))\n \t\t\treturn 1;\n \t\treturn 0;\n \t}\n@@ -183,7 +183,7 @@ static int graph_write(int argc, const char **argv)\n \t\tUNLEAK(buf);\n \t}\n \n-\tif (write_commit_graph(odb->path,\n+\tif (write_commit_graph(odb,\n \t\t\t       pack_indexes,\n \t\t\t       commit_hex,\n \t\t\t       flags,\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex bd071169d7..9df3733e96 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1689,7 +1689,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t      \"not exceeded, and then \\\"git restore --staged :/\\\" to recover.\"));\n \n \tif (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&\n-\t    write_commit_graph_reachable(get_object_directory(), 0, NULL))\n+\t    write_commit_graph_reachable(the_repository->objects->odb, 0, NULL))\n \t\treturn 1;\n \n \trepo_rerere(the_repository, 0);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d0..1ce16c96e9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1870,7 +1870,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (progress)\n \t\t\tcommit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n-\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     commit_graph_flags,\n \t\t\t\t\t     NULL);\n \t}\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3f76bf4aa7..8e0b9cf41b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -686,7 +686,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tprepare_repo_settings(the_repository);\n \tif (the_repository->settings.gc_write_commit_graph == 1)\n-\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n \t\t\t\t\t     NULL);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2c06876b26..19889e5fea 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -796,7 +796,7 @@ struct packed_oid_list {\n \n struct write_commit_graph_context {\n \tstruct repository *r;\n-\tchar *obj_dir;\n+\tstruct object_directory *odb;\n \tchar *graph_name;\n \tstruct packed_oid_list oids;\n \tstruct packed_commit_list commits;\n@@ -1173,7 +1173,7 @@ static int add_ref_to_list(const char *refname,\n \treturn 0;\n }\n \n-int write_commit_graph_reachable(const char *obj_dir,\n+int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n@@ -1181,7 +1181,7 @@ int write_commit_graph_reachable(const char *obj_dir,\n \tint result;\n \n \tfor_each_ref(add_ref_to_list, &list);\n-\tresult = write_commit_graph(obj_dir, NULL, &list,\n+\tresult = write_commit_graph(odb, NULL, &list,\n \t\t\t\t    flags, split_opts);\n \n \tstring_list_clear(&list, 0);\n@@ -1196,7 +1196,7 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \tstruct strbuf packname = STRBUF_INIT;\n \tint dirlen;\n \n-\tstrbuf_addf(&packname, \"%s/pack/\", ctx->obj_dir);\n+\tstrbuf_addf(&packname, \"%s/pack/\", ctx->odb->path);\n \tdirlen = packname.len;\n \tif (ctx->report_progress) {\n \t\tstrbuf_addf(&progress_title,\n@@ -1392,10 +1392,10 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \t\tstrbuf_addf(&tmp_file,\n \t\t\t    \"%s/info/commit-graphs/tmp_graph_XXXXXX\",\n-\t\t\t    ctx->obj_dir);\n+\t\t\t    ctx->odb->path);\n \t\tctx->graph_name = strbuf_detach(&tmp_file, NULL);\n \t} else {\n-\t\tctx->graph_name = get_commit_graph_filename(ctx->obj_dir);\n+\t\tctx->graph_name = get_commit_graph_filename(ctx->odb->path);\n \t}\n \n \tif (safe_create_leading_directories(ctx->graph_name)) {\n@@ -1406,7 +1406,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n \n \tif (ctx->split) {\n-\t\tchar *lock_name = get_chain_filename(ctx->obj_dir);\n+\t\tchar *lock_name = get_chain_filename(ctx->odb->path);\n \n \t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n \n@@ -1530,12 +1530,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-\t\t\tchar *graph_name = get_commit_graph_filename(ctx->obj_dir);\n+\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb->path);\n \t\t\tunlink(graph_name);\n \t\t}\n \n \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));\n-\t\tfinal_graph_name = get_split_graph_filename(ctx->obj_dir,\n+\t\tfinal_graph_name = get_split_graph_filename(ctx->odb->path,\n \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n \n@@ -1577,7 +1577,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->odb->path, ctx->obj_dir))\n+\t\tif (strcmp(g->odb->path, ctx->odb->path))\n \t\t\tbreak;\n \n \t\tnum_commits += g->num_commits;\n@@ -1592,7 +1592,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n \n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->odb->path, ctx->obj_dir)) {\n+\t\t    strcmp(g->odb->path, ctx->odb->path)) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1743,13 +1743,13 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n \t\texpire_time -= ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n-\t\tchar *chain_file_name = get_chain_filename(ctx->obj_dir);\n+\t\tchar *chain_file_name = get_chain_filename(ctx->odb->path);\n \t\tunlink(chain_file_name);\n \t\tfree(chain_file_name);\n \t\tctx->num_commit_graphs_after = 0;\n \t}\n \n-\tstrbuf_addstr(&path, ctx->obj_dir);\n+\tstrbuf_addstr(&path, ctx->odb->path);\n \tstrbuf_addstr(&path, \"/info/commit-graphs\");\n \tdir = opendir(path.buf);\n \n@@ -1788,7 +1788,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tstrbuf_release(&path);\n }\n \n-int write_commit_graph(const char *obj_dir,\n+int write_commit_graph(struct object_directory *odb,\n \t\t       struct string_list *pack_indexes,\n \t\t       struct string_list *commit_hex,\n \t\t       enum commit_graph_write_flags flags,\n@@ -1796,7 +1796,6 @@ int write_commit_graph(const char *obj_dir,\n {\n \tstruct write_commit_graph_context *ctx;\n \tuint32_t i, count_distinct = 0;\n-\tsize_t len;\n \tint res = 0;\n \n \tif (!commit_graph_compatible(the_repository))\n@@ -1804,14 +1803,7 @@ int write_commit_graph(const char *obj_dir,\n \n \tctx = xcalloc(1, sizeof(struct write_commit_graph_context));\n \tctx->r = the_repository;\n-\n-\t/* normalize object dir with no trailing slash */\n-\tctx->obj_dir = xmallocz(strlen(obj_dir) + 1);\n-\tnormalize_path_copy(ctx->obj_dir, obj_dir);\n-\tlen = strlen(ctx->obj_dir);\n-\tif (len && ctx->obj_dir[len - 1] == '/')\n-\t\tctx->obj_dir[len - 1] = 0;\n-\n+\tctx->odb = odb;\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n@@ -1848,8 +1840,7 @@ int write_commit_graph(const char *obj_dir,\n \t\tctx->oids.alloc = split_opts->max_commits;\n \n \tif (ctx->append) {\n-\t\tstruct object_directory *odb = find_odb(ctx->r, ctx->obj_dir);\n-\t\tprepare_commit_graph_one(ctx->r, odb);\n+\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n \t\tif (ctx->r->objects->commit_graph)\n \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n \t}\n@@ -1923,7 +1914,6 @@ int write_commit_graph(const char *obj_dir,\n \tfree(ctx->graph_name);\n \tfree(ctx->commits.list);\n \tfree(ctx->oids.list);\n-\tfree(ctx->obj_dir);\n \n \tif (ctx->commit_graph_filenames_after) {\n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9700a6c7c2..39c0c0e51c 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -95,10 +95,10 @@ struct split_commit_graph_opts {\n  * is not compatible with the commit-graph feature, then the\n  * methods will return 0 without writing a commit-graph.\n  */\n-int write_commit_graph_reachable(const char *obj_dir,\n+int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts);\n-int write_commit_graph(const char *obj_dir,\n+int write_commit_graph(struct object_directory *odb,\n \t\t       struct string_list *pack_indexes,\n \t\t       struct string_list *commit_hex,\n \t\t       enum commit_graph_write_flags flags,\n-- \n2.25.0.dirty\n\n"},{"id":"390871","messageId":"bff635973073f4c496213e0416483985d7a4d12e.1580424766.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH 3/6] builtin/commit-graph.c: die() with unknown '--object-dir'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-01-30T23:00:54Z","receivedAt":"2020-01-30T23:00:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The commit-graph sub-commands 'write', 'verify' both take an\n'--object-dir' argument, which is used to specify the location of an\nobject directory containing commit-graphs.\n\nHowever, there was no verification that the '--object-dir' argument\nwas an object directory. In the case of an '--object-dir' argument that\neither (1) doesn't exist, or (2) isn't an object directory, 'git\ncommit-graph ...' would exit silently with status zero.\n\nThis can clearly lead to unintended behavior, such as verifying\ncommit-graphs that aren't in a repository's own object store (or one of\nits alternates), or causing a typo to mask a legitimate commit-graph\nverification failure.\n\nTo remedy this, let's wrap 'find_odb()' with 'find_odb_or_die()' and\ncause the above such errors to produce an error and non-zero exit code.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-commit-graph.txt |  5 ++++-\n builtin/commit-graph.c             | 13 +++++++++++--\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex bcd85c1976..28d1fee505 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -26,7 +26,10 @@ OPTIONS\n \tfile. This parameter exists to specify the location of an alternate\n \tthat only has the objects directory, not a full `.git` directory. The\n \tcommit-graph file is expected to be in the `<dir>/info` directory and\n-\tthe packfiles are expected to be in `<dir>/pack`.\n+\tthe packfiles are expected to be in `<dir>/pack`. If the directory\n+\tcould not be made into an absolute path, or does not match any known\n+\tobject directory, `git commit-graph ...` will exit with non-zero\n+\tstatus.\n \n --[no-]progress::\n \tTurn progress on/off explicitly. If neither is specified, progress is\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 3edac318e8..93ff90d73b 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -34,6 +34,15 @@ static struct opts_commit_graph {\n \tint progress;\n } opts;\n \n+static struct object_directory *find_odb_or_die(struct repository *r,\n+\t\t\t\t\t\tconst char *obj_dir)\n+{\n+\tstruct object_directory *odb = find_odb(r, obj_dir);\n+\tif (!odb)\n+\t\tdie(_(\"could not find object directory matching %s\"), obj_dir);\n+\treturn odb;\n+}\n+\n static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n@@ -78,7 +87,7 @@ static int graph_verify(int argc, const char **argv)\n \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \telse {\n \t\tstruct object_directory *odb;\n-\t\tif ((odb = find_odb(the_repository, opts.obj_dir)))\n+\t\tif ((odb = find_odb_or_die(the_repository, opts.obj_dir)))\n \t\t\tgraph = read_commit_graph_one(the_repository, odb);\n \t}\n \n@@ -149,7 +158,7 @@ static int graph_write(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \tread_replace_refs = 0;\n-\todb = find_odb(the_repository, opts.obj_dir);\n+\todb = find_odb_or_die(the_repository, opts.obj_dir);\n \n \tif (opts.reachable) {\n \t\tif (write_commit_graph_reachable(odb->path, flags, &split_opts))\n-- \n2.25.0.dirty\n\n"},{"id":"390884","messageId":"CAN0heSrrrWjBKnzjMfzEkTMVTge2AfVdwsp6D5Mx==5S8-ZLJQ@mail.gmail.com","threadId":"52724","inReplyTo":"890e0e7136204f5ca47f0703f32b4adb99ad8d7e.1580424766.git.me@ttaylorr.com","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-01-31T06:52:02Z","receivedAt":"2020-01-31T06:52:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 31 Jan 2020 at 00:03, Taylor Blau <me@ttaylorr.com> wrote:\n> Instead of getting rid of the 'struct object_directory *', store that\n> insead of a 'char *odb' in 'struct commit_graph'. Once the 'struct\n\ns/insead/instead/\n\n>         if (open_ok)\n>                 graph = load_commit_graph_one_fd_st(fd, &st);\n> -        else\n> -               graph = read_commit_graph_one(the_repository, opts.obj_dir);\n> +       else {\n> +               struct object_directory *odb;\n> +               if ((odb = find_odb(the_repository, opts.obj_dir)))\n> +                       graph = read_commit_graph_one(the_repository, odb);\n> +       }\n\nI'm a tiny bit allergic to this assignment-within-if. It's wrapped by\nanother pair of parentheses, which both compilers and humans know to\ninterpret as \"trust me, this is not a mistake\", but I still find this\neasier to read:\n\n  odb = find_odb(...);\n  if (odb)\n          ....\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n\n> +#include \"object-store.h\"\n\nThis is the only change in this file, which looks a bit odd. I haven't\nactually applied your patches, to be honest, but is this inclusion\nreally needed?\n\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n\n> +struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n\nThis doesn't look commit-graph related -- could/should it go somewhere\nelse?\n\n> +{\n> +       struct object_directory *odb;\n> +       char *obj_dir_real = real_pathdup(obj_dir, 1);\n> +       int cmp = -1;\n> +\n> +       prepare_alt_odb(r);\n> +       for (odb = r->objects->odb; odb; odb = odb->next) {\n> +               cmp = strcmp(obj_dir_real, real_path(odb->path));\n> +               if (!cmp)\n> +                       break;\n> +       }\n\nAt this point, either odb is NULL or cmp is zero. Those are the only two\nways out of the loop.\n\n> +       free(obj_dir_real);\n> +\n> +       if (cmp)\n> +               odb = NULL;\n\nMeaning that this doesn't do much? If the most recent comparison failed,\nit's because we didn't find anything, so odb will be NULL.\n\n> +       return odb;\n> +}\n\nI think you could drop `cmp` and that final check, and write the loop\nbody as \"if (!strcmp(...)) break\". You could also have an empty loop\nbody, but I wouldn't go there -- I'd find that less readable. (Maybe\nthat's just me.)\n\nMartin\n"},{"id":"390894","messageId":"20200131102049.GC2916051@coredump.intra.peff.net","threadId":"52724","inReplyTo":"CAN0heSrrrWjBKnzjMfzEkTMVTge2AfVdwsp6D5Mx==5S8-ZLJQ@mail.gmail.com","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-31T10:20:49Z","receivedAt":"2020-01-31T10:20:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote:\n\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> \n> > +struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n> \n> This doesn't look commit-graph related -- could/should it go somewhere\n> else?\n\nI think the right place is actually as a static inside\nbuiltin/commit-graph.c, as this is really about handling its weird\n--object-dir options.\n\nBut it can't go there in this patch, because there's a caller in\ncommit-graph.c. In patch 4, we convert write_commit_graph() to take an\nodb, too, and that call goes away. At that point, we could move it into\nthe builtin as a static.\n\nIdeally we could flip the order of this patch and patch 4, but that\ndoesn't work either: by switching to an odb we lose our path\nnormalization, but if the other side hasn't switched either, then we\ncan't just compare odb pointers. It would be a temporary regression.\n\nSo there's a circular dependency between the two patches. I think we\nought to do done of:\n\n  - move find_odb() to a static as a cleanup on top\n\n  - squash those two patches together into a single\n\n  - swap the patch order, but have write_commit_graph_ctx store both the\n    \"odb\" _and_ the normalized copy of the path we do now. That leaves\n    it correct, and then it can be cleaned up in favor of an odb pointer\n    comparison in patch 5, along with the rest of the normalized bits.\n\nI'm OK with any of those. The second two have the added bonus that we\ncould introduce the die() behavior into find_odb() immediately, and\nexplain it (there's another temporary weirdness in this patch where\nspecifying an --object-dir outside of the repository becomes a silent\nnoop, and then the next patch turns it into an error, but that could all\nbe done in a single step when we introduce find_odb()).\n\n> I think you could drop `cmp` and that final check, and write the loop\n> body as \"if (!strcmp(...)) break\". You could also have an empty loop\n> body, but I wouldn't go there -- I'd find that less readable. (Maybe\n> that's just me.)\n\nYeah, I believe you are correct (and this is a nice simplification worth\ndoing).\n\n-Peff\n"},{"id":"390896","messageId":"20200131103008.GD2916051@coredump.intra.peff.net","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"Re: [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-31T10:30:08Z","receivedAt":"2020-01-31T10:30:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote:\n\n> This series became a little bit longer than I was expecting it to be, so\n> here is the high-level structure:\n> \n>   - 1/6 fixes a bug in a test that would cause a subsequent failure if\n>     unaddressed.\n> \n>   - 2/6 does the first half of the removal by using 'struct\n>     object_directory *'s within the 'commit_graph' structure.\n> \n>   - 4/6 does the second half by removing 'char *object_dir' usage in the\n>     'write_commit_graph_context' structure.\n> \n>   - 5/6 ties 2/6 and 4/6 together by removing all path normalization\n>     completely, fixing the uninitialized read bug.\n> \n>   - And 6/6 cleans up.\n\nWith the exception of the patch-ordering discussion in the sub-thread\nwith Martin, this looks good to me.\n\nPatch 3 is a change in user-visible behavior, as it restricts how\n--object-dir can be used (it must be the main object-dir or an alternate\nwithin the repository). I don't _think_ anybody would care, as the\nsemantics of those options seemed kind of ill-defined to me in the first\nplace. But it's worth calling out as a potential risk. I suppose the\nalternative is to make a one-off fake \"struct object_directory\" within\nthe process that isn't connected to the repository. But if nobody cares,\nI'd just as soon avoid that.\n\nOne other funny thing with this series: the Date headers of your patches\nseem out of order. They ordering in your cover letter here is fine and\npresumably reflects the commit topology:\n\n> Taylor Blau (6):\n>   t5318: don't pass non-object directory to '--object-dir'\n>   commit-graph.h: store object directory in 'struct commit_graph'\n>   builtin/commit-graph.c: die() with unknown '--object-dir'\n>   commit-graph.h: store an odb in 'struct write_commit_graph_context'\n>   commit-graph.c: remove path normalization, comparison\n>   commit-graph.h: use odb in 'load_commit_graph_one_fd_st'\n\nbut the Date headers in order of 1-6 are:\n\n  Date:   Thu, 30 Jan 2020 15:00:43 -0800\n  Date:   Thu, 30 Jan 2020 15:00:50 -0800\n  Date:   Thu, 30 Jan 2020 15:00:54 -0800\n  Date:   Thu, 30 Jan 2020 15:00:52 -0800\n  Date:   Thu, 30 Jan 2020 15:00:45 -0800\n  Date:   Thu, 30 Jan 2020 15:00:47 -0800\n\nIt's like your sending script rewrites the Date header and puts a\n2-second bump between each one (which is good, and what git-send-email\ndoes), but got fed the patches in the wrong order (perhaps their\n_original_ date order, if there was clean-up rebasing).\n\n-Peff\n"},{"id":"390901","messageId":"60a4d129-c6a9-1e5f-a467-7db0babbcffa@gmail.com","threadId":"52724","inReplyTo":"20200131103008.GD2916051@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-31T13:22:42Z","receivedAt":"2020-01-31T13:22:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/31/2020 5:30 AM, Jeff King wrote:\n> On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote:\n> \n>> This series became a little bit longer than I was expecting it to be, so\n>> here is the high-level structure:\n>>\n>>   - 1/6 fixes a bug in a test that would cause a subsequent failure if\n>>     unaddressed.\n>>\n>>   - 2/6 does the first half of the removal by using 'struct\n>>     object_directory *'s within the 'commit_graph' structure.\n>>\n>>   - 4/6 does the second half by removing 'char *object_dir' usage in the\n>>     'write_commit_graph_context' structure.\n>>\n>>   - 5/6 ties 2/6 and 4/6 together by removing all path normalization\n>>     completely, fixing the uninitialized read bug.\n>>\n>>   - And 6/6 cleans up.\n> \n> With the exception of the patch-ordering discussion in the sub-thread\n> with Martin, this looks good to me.\n\nI agree. Martin's comment is a good one. I can't find anything else\nto improve the series.\n\n> Patch 3 is a change in user-visible behavior, as it restricts how\n> --object-dir can be used (it must be the main object-dir or an alternate\n> within the repository). I don't _think_ anybody would care, as the\n> semantics of those options seemed kind of ill-defined to me in the first\n> place. But it's worth calling out as a potential risk. I suppose the\n> alternative is to make a one-off fake \"struct object_directory\" within\n> the process that isn't connected to the repository. But if nobody cares,\n> I'd just as soon avoid that.\n\nI think that this change of behavior is fine, especially because if\nsomeone writes a commit-graph to an --object-dir that is not an\nalternate, then that repo will not discover the new commit-graph\nanyway.\n\nI do like that you state a possible work-around in case someone shows\nup with a legitimate use case for a non-alternate object-dir.\n\nThanks,\n-Stolee\n\n"},{"id":"390934","messageId":"CAN0heSo1a3s5chz=FD-XnLncLwH_N59BPZzqtEUC_nJ8sLJtdA@mail.gmail.com","threadId":"52724","inReplyTo":"20200131102049.GC2916051@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-01-31T19:19:12Z","receivedAt":"2020-01-31T19:19:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 31 Jan 2020 at 11:20, Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote:\n>\n> > > --- a/commit-graph.c\n> > > +++ b/commit-graph.c\n> >\n> > > +struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n> >\n> > This doesn't look commit-graph related -- could/should it go somewhere\n> > else?\n>\n> I think the right place is actually as a static inside\n> builtin/commit-graph.c, as this is really about handling its weird\n> --object-dir options.\n>\n> But it can't go there in this patch, because there's a caller in\n> commit-graph.c. In patch 4, we convert write_commit_graph() to take an\n> odb, too, and that call goes away. At that point, we could move it into\n> the builtin as a static.\n\nThanks for explaining the issue.\n\n> Ideally we could flip the order of this patch and patch 4, but that\n> doesn't work either: by switching to an odb we lose our path\n> normalization, but if the other side hasn't switched either, then we\n> can't just compare odb pointers. It would be a temporary regression.\n>\n> So there's a circular dependency between the two patches. I think we\n> ought to do done of:\n>\n>   - move find_odb() to a static as a cleanup on top\n>\n>   - squash those two patches together into a single\n>\n>   - swap the patch order, but have write_commit_graph_ctx store both the\n>     \"odb\" _and_ the normalized copy of the path we do now. That leaves\n>     it correct, and then it can be cleaned up in favor of an odb pointer\n>     comparison in patch 5, along with the rest of the normalized bits.\n>\n> I'm OK with any of those. The second two have the added bonus that we\n> could introduce the die() behavior into find_odb() immediately, and\n> explain it (there's another temporary weirdness in this patch where\n> specifying an --object-dir outside of the repository becomes a silent\n> noop, and then the next patch turns it into an error, but that could all\n> be done in a single step when we introduce find_odb()).\n\n... and these ways of addressing it.\n\nMartin\n"},{"id":"390960","messageId":"xmqqftfv9wri.fsf@gitster-ct.c.googlers.com","threadId":"52724","inReplyTo":"CAN0heSrrrWjBKnzjMfzEkTMVTge2AfVdwsp6D5Mx==5S8-ZLJQ@mail.gmail.com","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-31T20:49:37Z","receivedAt":"2020-01-31T20:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n>> +               struct object_directory *odb;\n>> +               if ((odb = find_odb(the_repository, opts.obj_dir)))\n>> +                       graph = read_commit_graph_one(the_repository, odb);\n>> +       }\n>\n> I'm a tiny bit allergic to this assignment-within-if. It's wrapped by\n> another pair of parentheses, which both compilers and humans know to\n> interpret as \"trust me, this is not a mistake\", but I still find this\n> easier to read:\n>\n>   odb = find_odb(...);\n>   if (odb)\n>           ....\n\nYup.\n"},{"id":"391019","messageId":"20200203035819.GA23625@syl.local","threadId":"52724","inReplyTo":"CAN0heSrrrWjBKnzjMfzEkTMVTge2AfVdwsp6D5Mx==5S8-ZLJQ@mail.gmail.com","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T03:58:19Z","receivedAt":"2020-02-03T03:58:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Martin,\n\nThanks for your review! Your comments were all quite helpful, and I\napplied all of your suggested changes.\n\nOn Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote:\n> On Fri, 31 Jan 2020 at 00:03, Taylor Blau <me@ttaylorr.com> wrote:\n> > Instead of getting rid of the 'struct object_directory *', store that\n> > insead of a 'char *odb' in 'struct commit_graph'. Once the 'struct\n>\n> s/insead/instead/\n\nTypo. Thanks for noticing. I fixed this in my local copy of this branch.\n\n> >         if (open_ok)\n> >                 graph = load_commit_graph_one_fd_st(fd, &st);\n> > -        else\n> > -               graph = read_commit_graph_one(the_repository, opts.obj_dir);\n> > +       else {\n> > +               struct object_directory *odb;\n> > +               if ((odb = find_odb(the_repository, opts.obj_dir)))\n> > +                       graph = read_commit_graph_one(the_repository, odb);\n> > +       }\n>\n> I'm a tiny bit allergic to this assignment-within-if. It's wrapped by\n> another pair of parentheses, which both compilers and humans know to\n> interpret as \"trust me, this is not a mistake\", but I still find this\n> easier to read:\n>\n>   odb = find_odb(...);\n>   if (odb)\n>           ....\n\nTo be honest, I'm not such a fan of this style myself, but it seemed odd\nto me to write:\n\n  struct object_directory *odb;\n  odb = ...;\n  if (odb) {\n  }\n\nwhen we were really only trying to call 'find_odb()' and do something\nwith its result, but only if it was non-NULL. I counted 152 of these\nassign-if's laying around with:\n\n  $ git grep 'if ((.* =[^=]' | wc -l\n\nbut it seems like they are in poor style (as evidenced by your and\nJunio's response later in the thread). So, I removed this and instead\npromoted 'odb' to a local variable at the function level, since we\ndo that promotion anyway in a couple of patches later. This reduces the\nchurn, and avoids either an assign-if, or a define/assign/check.\n\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n>\n> > +#include \"object-store.h\"\n\nNo; this is a stray left over from some development on this branch. I'll\nremove it.\n\n> This is the only change in this file, which looks a bit odd. I haven't\n> actually applied your patches, to be honest, but is this inclusion\n> really needed?\n>\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n>\n> > +struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n>\n> This doesn't look commit-graph related -- could/should it go somewhere\n> else?\n\nI'll respond in more complete detail further down in the thread, but the\nshort answer is \"yes, this should go in builtin/commit-graph.c\".\n\n> > +{\n> > +       struct object_directory *odb;\n> > +       char *obj_dir_real = real_pathdup(obj_dir, 1);\n> > +       int cmp = -1;\n> > +\n> > +       prepare_alt_odb(r);\n> > +       for (odb = r->objects->odb; odb; odb = odb->next) {\n> > +               cmp = strcmp(obj_dir_real, real_path(odb->path));\n> > +               if (!cmp)\n> > +                       break;\n> > +       }\n>\n> At this point, either odb is NULL or cmp is zero. Those are the only two\n> ways out of the loop.\n>\n> > +       free(obj_dir_real);\n> > +\n> > +       if (cmp)\n> > +               odb = NULL;\n>\n> Meaning that this doesn't do much? If the most recent comparison failed,\n> it's because we didn't find anything, so odb will be NULL.\n>\n> > +       return odb;\n> > +}\n>\n> I think you could drop `cmp` and that final check, and write the loop\n> body as \"if (!strcmp(...)) break\". You could also have an empty loop\n> body, but I wouldn't go there -- I'd find that less readable. (Maybe\n> that's just me.)\n\nThanks, I changed this to remove the 'cmp' check outside of the loop,\nwhich I agree is unnecessary.\n\n> Martin\n\nThanks,\nTaylor\n"},{"id":"391021","messageId":"20200203043646.GB23625@syl.local","threadId":"52724","inReplyTo":"20200131102049.GC2916051@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T04:36:46Z","receivedAt":"2020-02-03T04:36:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 31, 2020 at 05:20:49AM -0500, Jeff King wrote:\n> On Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote:\n>\n> > > --- a/commit-graph.c\n> > > +++ b/commit-graph.c\n> >\n> > > +struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n> >\n> > This doesn't look commit-graph related -- could/should it go somewhere\n> > else?\n>\n> I think the right place is actually as a static inside\n> builtin/commit-graph.c, as this is really about handling its weird\n> --object-dir options.\n>\n> But it can't go there in this patch, because there's a caller in\n> commit-graph.c. In patch 4, we convert write_commit_graph() to take an\n> odb, too, and that call goes away. At that point, we could move it into\n> the builtin as a static.\n>\n> Ideally we could flip the order of this patch and patch 4, but that\n> doesn't work either: by switching to an odb we lose our path\n> normalization, but if the other side hasn't switched either, then we\n> can't just compare odb pointers. It would be a temporary regression.\n>\n> So there's a circular dependency between the two patches. I think we\n> ought to do done of:\n>\n>   - move find_odb() to a static as a cleanup on top\n>\n>   - squash those two patches together into a single\n>\n>   - swap the patch order, but have write_commit_graph_ctx store both the\n>     \"odb\" _and_ the normalized copy of the path we do now. That leaves\n>     it correct, and then it can be cleaned up in favor of an odb pointer\n>     comparison in patch 5, along with the rest of the normalized bits.\n\nThanks for describing the problem, and for laying out a few options!\n\nI went with this option, which was to swap patches 2/6 and 4/6, and then\nsquashing 3/6 into what _was_ 4/6 (but is now 2/5).\n\nThat all said, I am not sure that I follow the explanation that we need\nto have 'write_commit_graph_ctx' store the normalized object_dir, since\nwe can use 'odb->path' for this instead. I double checked, and we\nalready do the same loose normalization in\n'sha1-file.c:link_alt_odb_entry()', where we remove trailing slashes.\n\nThis isn't the exact same normalization strictly speaking. It differs\nonly in that we remove more than one trailing slash if it exists, but\nthis seems acceptable for the 2 patches that this behavior changes lives\non for.\n\nI double checked that the 'odb->path' is normalized relative to the $PWD,\nin which case I think that we can rely on it as a stand-in for\n'object_dir', and don't have to store both.\n\n> I'm OK with any of those. The second two have the added bonus that we\n> could introduce the die() behavior into find_odb() immediately, and\n> explain it (there's another temporary weirdness in this patch where\n> specifying an --object-dir outside of the repository becomes a silent\n> noop, and then the next patch turns it into an error, but that could all\n> be done in a single step when we introduce find_odb()).\n>\n> > I think you could drop `cmp` and that final check, and write the loop\n> > body as \"if (!strcmp(...)) break\". You could also have an empty loop\n> > body, but I wouldn't go there -- I'd find that less readable. (Maybe\n> > that's just me.)\n>\n> Yeah, I believe you are correct (and this is a nice simplification worth\n> doing).\n>\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"391022","messageId":"20200203043803.GC23625@syl.local","threadId":"52724","inReplyTo":"60a4d129-c6a9-1e5f-a467-7db0babbcffa@gmail.com","subject":"Re: [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T04:38:03Z","receivedAt":"2020-02-03T04:38:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 31, 2020 at 08:22:42AM -0500, Derrick Stolee wrote:\n> On 1/31/2020 5:30 AM, Jeff King wrote:\n> > On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote:\n> >\n> >> This series became a little bit longer than I was expecting it to be, so\n> >> here is the high-level structure:\n> >>\n> >>   - 1/6 fixes a bug in a test that would cause a subsequent failure if\n> >>     unaddressed.\n> >>\n> >>   - 2/6 does the first half of the removal by using 'struct\n> >>     object_directory *'s within the 'commit_graph' structure.\n> >>\n> >>   - 4/6 does the second half by removing 'char *object_dir' usage in the\n> >>     'write_commit_graph_context' structure.\n> >>\n> >>   - 5/6 ties 2/6 and 4/6 together by removing all path normalization\n> >>     completely, fixing the uninitialized read bug.\n> >>\n> >>   - And 6/6 cleans up.\n> >\n> > With the exception of the patch-ordering discussion in the sub-thread\n> > with Martin, this looks good to me.\n>\n> I agree. Martin's comment is a good one. I can't find anything else\n> to improve the series.\n\nThanks for your review!\n\n> > Patch 3 is a change in user-visible behavior, as it restricts how\n> > --object-dir can be used (it must be the main object-dir or an alternate\n> > within the repository). I don't _think_ anybody would care, as the\n> > semantics of those options seemed kind of ill-defined to me in the first\n> > place. But it's worth calling out as a potential risk. I suppose the\n> > alternative is to make a one-off fake \"struct object_directory\" within\n> > the process that isn't connected to the repository. But if nobody cares,\n> > I'd just as soon avoid that.\n>\n> I think that this change of behavior is fine, especially because if\n> someone writes a commit-graph to an --object-dir that is not an\n> alternate, then that repo will not discover the new commit-graph\n> anyway.\n\nAnd thanks for the ack. I would be somewhat surprised if someone were\nreally relying on this behavior in practice.\n\n> I do like that you state a possible work-around in case someone shows\n> up with a legitimate use case for a non-alternate object-dir.\n>\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"391026","messageId":"20200203083616.GA2164@coredump.intra.peff.net","threadId":"52724","inReplyTo":"20200203043646.GB23625@syl.local","subject":"Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-03T08:36:16Z","receivedAt":"2020-02-03T08:36:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 02, 2020 at 08:36:46PM -0800, Taylor Blau wrote:\n\n> That all said, I am not sure that I follow the explanation that we need\n> to have 'write_commit_graph_ctx' store the normalized object_dir, since\n> we can use 'odb->path' for this instead. I double checked, and we\n> already do the same loose normalization in\n> 'sha1-file.c:link_alt_odb_entry()', where we remove trailing slashes.\n\nI was assuming that the normalization in write_commit_graph_ctx was\nnecessary (and I think there are some tests around this), but I admit I\ndidn't actually try it. If it's not failing tests, I'm OK with comparing\nthe non-normalized paths as an intermediate step, since we end up\ncomparing pointers after the final patches.\n\n-Peff\n"},{"id":"391059","messageId":"cover.1580764494.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580424766.git.me@ttaylorr.com","subject":"[PATCH v2 0/5] commit-graph: use 'struct object_directory *' everywhere","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T21:17:53Z","receivedAt":"2020-02-03T21:17:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere is a re-roll of the series in this thread to replace string-based\npath comparison in 'commit-graph.c' code with raw pointer comparison of\n'struct object_directory *'s.\n\nThe only thing that has changed substantially since v1 is that I swapped\nthe order of patches 2/6 and 4/6. What was patch 3/6 got folded into\nwhat is now patch 2/5. This resolves an inconvenience where we had to\ndefine a helper function in 'commit-graph.c', when it really belonged in\n'builtin/commit-graph.c'.\n\nI took a few other suggestions from Martin in what is now patch 4/5, and\nnoticed a few other small things along the way. A range-diff since v1 is\nincluded below.\n\nThanks as always,\nTaylor\n\nTaylor Blau (5):\n  t5318: don't pass non-object directory to '--object-dir'\n  commit-graph.h: store an odb in 'struct write_commit_graph_context'\n  commit-graph.h: store object directory in 'struct commit_graph'\n  commit-graph.c: remove path normalization, comparison\n  commit-graph.h: use odb in 'load_commit_graph_one_fd_st'\n\n Documentation/git-commit-graph.txt |   5 +-\n builtin/commit-graph.c             |  34 +++++++--\n builtin/commit.c                   |   2 +-\n builtin/fetch.c                    |   2 +-\n builtin/gc.c                       |   2 +-\n commit-graph.c                     | 115 +++++++++++++----------------\n commit-graph.h                     |  15 ++--\n t/helper/test-read-graph.c         |   8 +-\n t/t5318-commit-graph.sh            |   4 +-\n 9 files changed, 100 insertions(+), 87 deletions(-)\n\nRange-diff against v1:\n1:  09ba72c85a = 1:  84a8709ad1 t5318: don't pass non-object directory to '--object-dir'\n4:  9784a5db3c ! 2:  d9819cfb33 commit-graph.h: store an odb in 'struct write_commit_graph_context'\n    @@ Metadata\n      ## Commit message ##\n         commit-graph.h: store an odb in 'struct write_commit_graph_context'\n\n    -    In a previous patch, the 'char *object_dir' in 'struct commit_graph' was\n    -    replaced with a 'struct object_directory'. This patch applies the same\n    -    treatement to 'struct write_commit_graph_context', which is an\n    -    intermediate step towards getting rid of all path normalization in\n    -    'commit-graph.c'.\n    -\n    -    Instead of taking a 'char *object_dir', functions that construct a\n    -    'struct write_commit_graph_context' now take a 'struct object_directory\n    -    *'. Any code that needs an object directory path use '->path' instead.\n    -\n    -    This ensures that all calls to functions that perform path normalization\n    -    are given arguments which do not themselves require normalization. This\n    -    prepares those functions to drop their normalization entirely, which\n    -    will occur in the subsequent patch.\n    +    There are lots of places in 'commit-graph.h' where a function either has\n    +    (or almost has) a full 'struct object_directory *', accesses '->path',\n    +    and then throws away the rest of the struct.\n    +\n    +    This can cause headaches when comparing the locations of object\n    +    directories across alternates (e.g., in the case of deciding if two\n    +    commit-graph layers can be merged). These paths are normalized with\n    +    'normalize_path_copy()' which mitigates some comparison issues, but not\n    +    all [1].\n    +\n    +    Replace usage of 'char *object_dir' with 'odb->path' by storing a\n    +    'struct object_directory *' in the 'write_commit_graph_context'\n    +    structure. This is an intermediate step towards getting rid of all path\n    +    normalization in 'commit-graph.c'.\n    +\n    +    Resolving a user-provided '--object-dir' argument now requires that we\n    +    compare it to the known alternates for equality.  Prior to this patch,\n    +    an unknown '--object-dir' argument would silently exit with status zero.\n    +\n    +    This can clearly lead to unintended behavior, such as verifying\n    +    commit-graphs that aren't in a repository's own object store (or one of\n    +    its alternates), or causing a typo to mask a legitimate commit-graph\n    +    verification failure. Make this error non-silent by 'die()'-ing when the\n    +    given '--object-dir' does not match any known alternate object store.\n    +\n    +    [1]: In my testing, for example, I can get one side of the commit-graph\n    +    code to fill object_dir with \"./objects\" and the other with just\n    +    \"objects\".\n\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n\n    + ## Documentation/git-commit-graph.txt ##\n    +@@ Documentation/git-commit-graph.txt: OPTIONS\n    + \tfile. This parameter exists to specify the location of an alternate\n    + \tthat only has the objects directory, not a full `.git` directory. The\n    + \tcommit-graph file is expected to be in the `<dir>/info` directory and\n    +-\tthe packfiles are expected to be in `<dir>/pack`.\n    ++\tthe packfiles are expected to be in `<dir>/pack`. If the directory\n    ++\tcould not be made into an absolute path, or does not match any known\n    ++\tobject directory, `git commit-graph ...` will exit with non-zero\n    ++\tstatus.\n    +\n    + --[no-]progress::\n    + \tTurn progress on/off explicitly. If neither is specified, progress is\n    +\n      ## builtin/commit-graph.c ##\n    +@@ builtin/commit-graph.c: static struct opts_commit_graph {\n    + \tint progress;\n    + } opts;\n    +\n    ++struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n    ++{\n    ++\tstruct object_directory *odb;\n    ++\tchar *obj_dir_real = real_pathdup(obj_dir, 1);\n    ++\n    ++\tprepare_alt_odb(r);\n    ++\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n    ++\t\tif (!strcmp(obj_dir_real, real_path(odb->path)))\n    ++\t\t\tbreak;\n    ++\t}\n    ++\n    ++\tfree(obj_dir_real);\n    ++\n    ++\tif (!odb)\n    ++\t\tdie(_(\"could not find object directory matching %s\"), obj_dir);\n    ++\treturn odb;\n    ++}\n    ++\n    + static int graph_verify(int argc, const char **argv)\n    + {\n    + \tstruct commit_graph *graph = NULL;\n    ++\tstruct object_directory *odb = NULL;\n    + \tchar *graph_name;\n    + \tint open_ok;\n    + \tint fd;\n    +@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n    + \tif (opts.progress)\n    + \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n    +\n    +-\tgraph_name = get_commit_graph_filename(opts.obj_dir);\n    ++\todb = find_odb(the_repository, opts.obj_dir);\n    ++\tgraph_name = get_commit_graph_filename(odb->path);\n    + \topen_ok = open_commit_graph(graph_name, &fd, &st);\n    + \tif (!open_ok && errno != ENOENT)\n    + \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n    +@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n    +\n    + \tif (open_ok)\n    + \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n    +-\t else\n    +-\t\tgraph = read_commit_graph_one(the_repository, opts.obj_dir);\n    ++\telse\n    ++\t\tgraph = read_commit_graph_one(the_repository, odb->path);\n    +\n    + \t/* Return failure if open_ok predicted success */\n    + \tif (!graph)\n     @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n    - \todb = find_odb_or_die(the_repository, opts.obj_dir);\n    + {\n    + \tstruct string_list *pack_indexes = NULL;\n    + \tstruct string_list *commit_hex = NULL;\n    ++\tstruct object_directory *odb = NULL;\n    + \tstruct string_list lines;\n    + \tint result = 0;\n    + \tenum commit_graph_write_flags flags = 0;\n    +@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n    + \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n    +\n    + \tread_replace_refs = 0;\n    ++\todb = find_odb(the_repository, opts.obj_dir);\n\n      \tif (opts.reachable) {\n    --\t\tif (write_commit_graph_reachable(odb->path, flags, &split_opts))\n    +-\t\tif (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))\n     +\t\tif (write_commit_graph_reachable(odb, flags, &split_opts))\n      \t\t\treturn 1;\n      \t\treturn 0;\n    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n      \t\tUNLEAK(buf);\n      \t}\n\n    --\tif (write_commit_graph(odb->path,\n    +-\tif (write_commit_graph(opts.obj_dir,\n     +\tif (write_commit_graph(odb,\n      \t\t\t       pack_indexes,\n      \t\t\t       commit_hex,\n    @@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph\n\n      \twhile (g && (g->num_commits <= size_mult * num_commits ||\n      \t\t    (max_commits && num_commits > max_commits))) {\n    --\t\tif (strcmp(g->odb->path, ctx->obj_dir))\n    -+\t\tif (strcmp(g->odb->path, ctx->odb->path))\n    +-\t\tif (strcmp(g->obj_dir, ctx->obj_dir))\n    ++\t\tif (strcmp(g->obj_dir, ctx->odb->path))\n      \t\t\tbreak;\n\n      \t\tnum_commits += g->num_commits;\n     @@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n    - \t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n    + \t\tchar *old_graph_name = get_commit_graph_filename(g->obj_dir);\n\n      \t\tif (!strcmp(g->filename, old_graph_name) &&\n    --\t\t    strcmp(g->odb->path, ctx->obj_dir)) {\n    -+\t\t    strcmp(g->odb->path, ctx->odb->path)) {\n    +-\t\t    strcmp(g->obj_dir, ctx->obj_dir)) {\n    ++\t\t    strcmp(g->obj_dir, ctx->odb->path)) {\n      \t\t\tctx->num_commit_graphs_after = 1;\n      \t\t\tctx->new_base_graph = NULL;\n      \t\t}\n    @@ commit-graph.c: int write_commit_graph(const char *obj_dir,\n      \t\tctx->oids.alloc = split_opts->max_commits;\n\n      \tif (ctx->append) {\n    --\t\tstruct object_directory *odb = find_odb(ctx->r, ctx->obj_dir);\n    --\t\tprepare_commit_graph_one(ctx->r, odb);\n    -+\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n    +-\t\tprepare_commit_graph_one(ctx->r, ctx->obj_dir);\n    ++\t\tprepare_commit_graph_one(ctx->r, ctx->odb->path);\n      \t\tif (ctx->r->objects->commit_graph)\n      \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n      \t}\n    @@ commit-graph.c: int write_commit_graph(const char *obj_dir,\n      \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n\n      ## commit-graph.h ##\n    +@@\n    + #include \"repository.h\"\n    + #include \"string-list.h\"\n    + #include \"cache.h\"\n    ++#include \"object-store.h\"\n    +\n    + #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n    + #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n     @@ commit-graph.h: struct split_commit_graph_opts {\n       * is not compatible with the commit-graph feature, then the\n       * methods will return 0 without writing a commit-graph.\n2:  328884f5f6 ! 3:  5fd5cfca6e commit-graph.h: store object directory in 'struct commit_graph'\n    @@ Metadata\n      ## Commit message ##\n         commit-graph.h: store object directory in 'struct commit_graph'\n\n    -    There are lots of places in 'commit-graph.h' where a function either has\n    -    (or almost has) a full 'struct object_directory *', accesses '->path',\n    -    and then throws away the rest of the struct.\n    +    In a previous patch, the 'char *object_dir' in 'struct commit_graph' was\n    +    replaced with a 'struct object_directory'. This patch applies the same\n    +    treatment to 'struct commit_graph', which is another intermediate step\n    +    towards getting rid of all path normalization in 'commit-graph.c'.\n\n    -    This can cause headaches when comparing the locations of object\n    -    directories across alternates (e.g., in the case of deciding if two\n    -    commit-graph layers can be merged). These paths are normalized with\n    -    'normalize_path_copy()' which mitigates some comparison issues, but not\n    -    all [1].\n    +    Instead of taking a 'char *object_dir', functions that construct a\n    +    'struct commit_graph' now take a 'struct object_directory *'. Any code\n    +    that needs an object directory path use '->path' instead.\n\n    -    Instead of getting rid of the 'struct object_directory *', store that\n    -    insead of a 'char *odb' in 'struct commit_graph'. Once the 'struct\n    -    write_commit_graph_context' has an object_directory pointer, too, this\n    -    will allow calling code to replace these error-prone path comparisons\n    -    with raw pointer comparisons, thereby circumventing any\n    -    normalization-related errors. This will be introduced in a subsequent\n    -    patch.\n    -\n    -    [1]: In my testing, for example, I can get one side of the commit-graph\n    -    code to fill object_dir with \"./objects\" and the other with just\n    -    \"objects\".\n    +    This ensures that all calls to functions that perform path normalization\n    +    are given arguments which do not themselves require normalization. This\n    +    prepares those functions to drop their normalization entirely, which\n    +    will occur in the subsequent patch.\n\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n\n      ## builtin/commit-graph.c ##\n     @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n    -\n      \tif (open_ok)\n      \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n    --\t else\n    --\t\tgraph = read_commit_graph_one(the_repository, opts.obj_dir);\n    -+\telse {\n    -+\t\tstruct object_directory *odb;\n    -+\t\tif ((odb = find_odb(the_repository, opts.obj_dir)))\n    -+\t\t\tgraph = read_commit_graph_one(the_repository, odb);\n    -+\t}\n    + \telse\n    +-\t\tgraph = read_commit_graph_one(the_repository, odb->path);\n    ++\t\tgraph = read_commit_graph_one(the_repository, odb);\n\n      \t/* Return failure if open_ok predicted success */\n      \tif (!graph)\n    -@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n    - \tstruct string_list lines;\n    - \tint result = 0;\n    - \tenum commit_graph_write_flags flags = 0;\n    -+\tstruct object_directory *odb = NULL;\n    -\n    - \tstatic struct option builtin_commit_graph_write_options[] = {\n    - \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n    -@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n    - \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n    -\n    - \tread_replace_refs = 0;\n    -+\todb = find_odb(the_repository, opts.obj_dir);\n    -\n    - \tif (opts.reachable) {\n    --\t\tif (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))\n    -+\t\tif (write_commit_graph_reachable(odb->path, flags, &split_opts))\n    - \t\t\treturn 1;\n    - \t\treturn 0;\n    - \t}\n    -@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n    - \t\tUNLEAK(buf);\n    - \t}\n    -\n    --\tif (write_commit_graph(opts.obj_dir,\n    -+\tif (write_commit_graph(odb->path,\n    - \t\t\t       pack_indexes,\n    - \t\t\t       commit_hex,\n    - \t\t\t       flags,\n    -\n    - ## builtin/commit.c ##\n    -@@\n    - #include \"help.h\"\n    - #include \"commit-reach.h\"\n    - #include \"commit-graph.h\"\n    -+#include \"object-store.h\"\n    -\n    - static const char * const builtin_commit_usage[] = {\n    - \tN_(\"git commit [<options>] [--] <pathspec>...\"),\n\n      ## commit-graph.c ##\n    -@@ commit-graph.c: static uint8_t oid_version(void)\n    - \treturn 1;\n    - }\n    -\n    -+struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n    -+{\n    -+\tstruct object_directory *odb;\n    -+\tchar *obj_dir_real = real_pathdup(obj_dir, 1);\n    -+\tint cmp = -1;\n    -+\n    -+\tprepare_alt_odb(r);\n    -+\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n    -+\t\tcmp = strcmp(obj_dir_real, real_path(odb->path));\n    -+\t\tif (!cmp)\n    -+\t\t\tbreak;\n    -+\t}\n    -+\n    -+\tfree(obj_dir_real);\n    -+\n    -+\tif (cmp)\n    -+\t\todb = NULL;\n    -+\treturn odb;\n    -+}\n    -+\n    - static struct commit_graph *alloc_commit_graph(void)\n    - {\n    - \tstruct commit_graph *g = xcalloc(1, sizeof(*g));\n     @@ commit-graph.c: static struct commit_graph *load_commit_graph_one(const char *graph_file)\n      \treturn g;\n      }\n    @@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph\n\n      \twhile (g && (g->num_commits <= size_mult * num_commits ||\n      \t\t    (max_commits && num_commits > max_commits))) {\n    --\t\tif (strcmp(g->obj_dir, ctx->obj_dir))\n    -+\t\tif (strcmp(g->odb->path, ctx->obj_dir))\n    +-\t\tif (strcmp(g->obj_dir, ctx->odb->path))\n    ++\t\tif (strcmp(g->odb->path, ctx->odb->path))\n      \t\t\tbreak;\n\n      \t\tnum_commits += g->num_commits;\n    @@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph\n     +\t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n\n      \t\tif (!strcmp(g->filename, old_graph_name) &&\n    --\t\t    strcmp(g->obj_dir, ctx->obj_dir)) {\n    -+\t\t    strcmp(g->odb->path, ctx->obj_dir)) {\n    +-\t\t    strcmp(g->obj_dir, ctx->odb->path)) {\n    ++\t\t    strcmp(g->odb->path, ctx->odb->path)) {\n      \t\t\tctx->num_commit_graphs_after = 1;\n      \t\t\tctx->new_base_graph = NULL;\n      \t\t}\n    -@@ commit-graph.c: int write_commit_graph(const char *obj_dir,\n    +@@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n      \t\tctx->oids.alloc = split_opts->max_commits;\n\n      \tif (ctx->append) {\n    --\t\tprepare_commit_graph_one(ctx->r, ctx->obj_dir);\n    -+\t\tstruct object_directory *odb = find_odb(ctx->r, ctx->obj_dir);\n    -+\t\tprepare_commit_graph_one(ctx->r, odb);\n    +-\t\tprepare_commit_graph_one(ctx->r, ctx->odb->path);\n    ++\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n      \t\tif (ctx->r->objects->commit_graph)\n      \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n      \t}\n\n      ## commit-graph.h ##\n    -@@\n    - #include \"repository.h\"\n    - #include \"string-list.h\"\n    - #include \"cache.h\"\n    -+#include \"object-store.h\"\n    -\n    - #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n    - #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n    -@@ commit-graph.h: struct commit;\n    - char *get_commit_graph_filename(const char *obj_dir);\n    - int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n    -\n    -+struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n    -+\n    - /*\n    -  * Given a commit struct, try to fill the commit struct info, including:\n    -  *  1. tree object\n     @@ commit-graph.h: struct commit_graph {\n      \tuint32_t num_commits;\n      \tstruct object_id oid;\n3:  ce884d7742 < -:  ---------- builtin/commit-graph.c: die() with unknown '--object-dir'\n5:  c9b2ba46ab ! 4:  f14e95aa7e commit-graph.c: remove path normalization, comparison\n    @@ Commit message\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n\n      ## builtin/commit-graph.c ##\n    -@@ builtin/commit-graph.c: static struct object_directory *find_odb_or_die(struct repository *r,\n    - static int graph_verify(int argc, const char **argv)\n    - {\n    - \tstruct commit_graph *graph = NULL;\n    -+\tstruct object_directory *odb = NULL;\n    - \tchar *graph_name;\n    - \tint open_ok;\n    - \tint fd;\n     @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n    - \tif (opts.progress)\n      \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n\n    --\tgraph_name = get_commit_graph_filename(opts.obj_dir);\n    -+\todb = find_odb_or_die(the_repository, opts.obj_dir);\n    + \todb = find_odb(the_repository, opts.obj_dir);\n    +-\tgraph_name = get_commit_graph_filename(odb->path);\n     +\tgraph_name = get_commit_graph_filename(odb);\n      \topen_ok = open_commit_graph(graph_name, &fd, &st);\n      \tif (!open_ok && errno != ENOENT)\n      \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n    -@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n    -\n    - \tif (open_ok)\n    - \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n    --\telse {\n    --\t\tstruct object_directory *odb;\n    --\t\tif ((odb = find_odb_or_die(the_repository, opts.obj_dir)))\n    --\t\t\tgraph = read_commit_graph_one(the_repository, odb);\n    --\t}\n    -+\telse\n    -+\t\tgraph = read_commit_graph_one(the_repository, odb);\n    -\n    - \t/* Return failure if open_ok predicted success */\n    - \tif (!graph)\n\n      ## commit-graph.c ##\n     @@\n    @@ commit-graph.h\n     +char *get_commit_graph_filename(struct object_directory *odb);\n      int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n\n    - struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n    + /*\n\n      ## t/helper/test-read-graph.c ##\n     @@ t/helper/test-read-graph.c: int cmd__read_graph(int argc, const char **argv)\n6:  e70483f771 = 5:  aa12b7378b commit-graph.h: use odb in 'load_commit_graph_one_fd_st'\n--\n2.25.0.119.gaa12b7378b\n"},{"id":"391060","messageId":"84a8709ad14c8731e394d21b9ddee1372e0ec436.1580764494.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580764494.git.me@ttaylorr.com","subject":"[PATCH v2 1/5] t5318: don't pass non-object directory to '--object-dir'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T21:17:56Z","receivedAt":"2020-02-03T21:18:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In f237c8b6fe (commit-graph: implement git-commit-graph write,\n2018-04-02) the test t5318.3 was introduced to ensure that calling 'git\ncommit-graph write' in a repository with no packfiles does not write any\ncommit-graph file(s).\n\nTo exercise more paths in 'builtin/commit-graph.c', this test passes\n'--object-dir' to 'git commit-graph write', but the given argument\nrefers to the working copy, not the object directory.\n\nSince the commit-graph sub-commands currently swallow these errors, this\ndoes not result in a test failure. But, it is only lucky that the test\nends with no commit-graphs, since there were none to begin with.\n\nIn preparation for a future commit where an '--object-dir' argument that\ndoes not match a known object directory will print out a failure, let's\nfix the test to still use '--object-dir', but pass the correct location\nto the object store instead of '.'.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5318-commit-graph.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 3f03de6018..0bf98b56ec 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -19,8 +19,8 @@ test_expect_success 'verify graph with no graph file' '\n \n test_expect_success 'write graph with no packs' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-\tgit commit-graph write --object-dir . &&\n-\ttest_path_is_missing info/commit-graph\n+\tgit commit-graph write --object-dir $objdir &&\n+\ttest_path_is_missing $objdir/info/commit-graph\n '\n \n test_expect_success 'exit with correct error on bad input to --stdin-packs' '\n-- \n2.25.0.119.gaa12b7378b\n\n"},{"id":"391061","messageId":"d9819cfb33ad95d4206dd1bbf4b38b7fdf69130f.1580764494.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580764494.git.me@ttaylorr.com","subject":"[PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T21:17:59Z","receivedAt":"2020-02-03T21:18:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"There are lots of places in 'commit-graph.h' where a function either has\n(or almost has) a full 'struct object_directory *', accesses '->path',\nand then throws away the rest of the struct.\n\nThis can cause headaches when comparing the locations of object\ndirectories across alternates (e.g., in the case of deciding if two\ncommit-graph layers can be merged). These paths are normalized with\n'normalize_path_copy()' which mitigates some comparison issues, but not\nall [1].\n\nReplace usage of 'char *object_dir' with 'odb->path' by storing a\n'struct object_directory *' in the 'write_commit_graph_context'\nstructure. This is an intermediate step towards getting rid of all path\nnormalization in 'commit-graph.c'.\n\nResolving a user-provided '--object-dir' argument now requires that we\ncompare it to the known alternates for equality.  Prior to this patch,\nan unknown '--object-dir' argument would silently exit with status zero.\n\nThis can clearly lead to unintended behavior, such as verifying\ncommit-graphs that aren't in a repository's own object store (or one of\nits alternates), or causing a typo to mask a legitimate commit-graph\nverification failure. Make this error non-silent by 'die()'-ing when the\ngiven '--object-dir' does not match any known alternate object store.\n\n[1]: In my testing, for example, I can get one side of the commit-graph\ncode to fill object_dir with \"./objects\" and the other with just\n\"objects\".\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-commit-graph.txt |  5 +++-\n builtin/commit-graph.c             | 32 +++++++++++++++++++----\n builtin/commit.c                   |  2 +-\n builtin/fetch.c                    |  2 +-\n builtin/gc.c                       |  2 +-\n commit-graph.c                     | 41 ++++++++++++------------------\n commit-graph.h                     |  5 ++--\n 7 files changed, 53 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex bcd85c1976..28d1fee505 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -26,7 +26,10 @@ OPTIONS\n \tfile. This parameter exists to specify the location of an alternate\n \tthat only has the objects directory, not a full `.git` directory. The\n \tcommit-graph file is expected to be in the `<dir>/info` directory and\n-\tthe packfiles are expected to be in `<dir>/pack`.\n+\tthe packfiles are expected to be in `<dir>/pack`. If the directory\n+\tcould not be made into an absolute path, or does not match any known\n+\tobject directory, `git commit-graph ...` will exit with non-zero\n+\tstatus.\n \n --[no-]progress::\n \tTurn progress on/off explicitly. If neither is specified, progress is\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex e0c6fc4bbf..31b57e4e1d 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -34,9 +34,28 @@ static struct opts_commit_graph {\n \tint progress;\n } opts;\n \n+struct object_directory *find_odb(struct repository *r, const char *obj_dir)\n+{\n+\tstruct object_directory *odb;\n+\tchar *obj_dir_real = real_pathdup(obj_dir, 1);\n+\n+\tprepare_alt_odb(r);\n+\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n+\t\tif (!strcmp(obj_dir_real, real_path(odb->path)))\n+\t\t\tbreak;\n+\t}\n+\n+\tfree(obj_dir_real);\n+\n+\tif (!odb)\n+\t\tdie(_(\"could not find object directory matching %s\"), obj_dir);\n+\treturn odb;\n+}\n+\n static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n+\tstruct object_directory *odb = NULL;\n \tchar *graph_name;\n \tint open_ok;\n \tint fd;\n@@ -67,7 +86,8 @@ static int graph_verify(int argc, const char **argv)\n \tif (opts.progress)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n-\tgraph_name = get_commit_graph_filename(opts.obj_dir);\n+\todb = find_odb(the_repository, opts.obj_dir);\n+\tgraph_name = get_commit_graph_filename(odb->path);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok && errno != ENOENT)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n@@ -76,8 +96,8 @@ static int graph_verify(int argc, const char **argv)\n \n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n-\t else\n-\t\tgraph = read_commit_graph_one(the_repository, opts.obj_dir);\n+\telse\n+\t\tgraph = read_commit_graph_one(the_repository, odb->path);\n \n \t/* Return failure if open_ok predicted success */\n \tif (!graph)\n@@ -94,6 +114,7 @@ static int graph_write(int argc, const char **argv)\n {\n \tstruct string_list *pack_indexes = NULL;\n \tstruct string_list *commit_hex = NULL;\n+\tstruct object_directory *odb = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n@@ -145,9 +166,10 @@ static int graph_write(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \tread_replace_refs = 0;\n+\todb = find_odb(the_repository, opts.obj_dir);\n \n \tif (opts.reachable) {\n-\t\tif (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))\n+\t\tif (write_commit_graph_reachable(odb, flags, &split_opts))\n \t\t\treturn 1;\n \t\treturn 0;\n \t}\n@@ -169,7 +191,7 @@ static int graph_write(int argc, const char **argv)\n \t\tUNLEAK(buf);\n \t}\n \n-\tif (write_commit_graph(opts.obj_dir,\n+\tif (write_commit_graph(odb,\n \t\t\t       pack_indexes,\n \t\t\t       commit_hex,\n \t\t\t       flags,\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 646e84547d..9e124aaa7b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1689,7 +1689,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t      \"not exceeded, and then \\\"git restore --staged :/\\\" to recover.\"));\n \n \tif (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&\n-\t    write_commit_graph_reachable(get_object_directory(), 0, NULL))\n+\t    write_commit_graph_reachable(the_repository->objects->odb, 0, NULL))\n \t\treturn 1;\n \n \trepo_rerere(the_repository, 0);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d0..1ce16c96e9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1870,7 +1870,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (progress)\n \t\t\tcommit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n-\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     commit_graph_flags,\n \t\t\t\t\t     NULL);\n \t}\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3f76bf4aa7..8e0b9cf41b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -686,7 +686,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tprepare_repo_settings(the_repository);\n \tif (the_repository->settings.gc_write_commit_graph == 1)\n-\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n \t\t\t\t\t     NULL);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b205e65ed1..cbfeece112 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -772,7 +772,7 @@ struct packed_oid_list {\n \n struct write_commit_graph_context {\n \tstruct repository *r;\n-\tchar *obj_dir;\n+\tstruct object_directory *odb;\n \tchar *graph_name;\n \tstruct packed_oid_list oids;\n \tstruct packed_commit_list commits;\n@@ -1149,7 +1149,7 @@ static int add_ref_to_list(const char *refname,\n \treturn 0;\n }\n \n-int write_commit_graph_reachable(const char *obj_dir,\n+int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n@@ -1157,7 +1157,7 @@ int write_commit_graph_reachable(const char *obj_dir,\n \tint result;\n \n \tfor_each_ref(add_ref_to_list, &list);\n-\tresult = write_commit_graph(obj_dir, NULL, &list,\n+\tresult = write_commit_graph(odb, NULL, &list,\n \t\t\t\t    flags, split_opts);\n \n \tstring_list_clear(&list, 0);\n@@ -1172,7 +1172,7 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \tstruct strbuf packname = STRBUF_INIT;\n \tint dirlen;\n \n-\tstrbuf_addf(&packname, \"%s/pack/\", ctx->obj_dir);\n+\tstrbuf_addf(&packname, \"%s/pack/\", ctx->odb->path);\n \tdirlen = packname.len;\n \tif (ctx->report_progress) {\n \t\tstrbuf_addf(&progress_title,\n@@ -1368,10 +1368,10 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \t\tstrbuf_addf(&tmp_file,\n \t\t\t    \"%s/info/commit-graphs/tmp_graph_XXXXXX\",\n-\t\t\t    ctx->obj_dir);\n+\t\t\t    ctx->odb->path);\n \t\tctx->graph_name = strbuf_detach(&tmp_file, NULL);\n \t} else {\n-\t\tctx->graph_name = get_commit_graph_filename(ctx->obj_dir);\n+\t\tctx->graph_name = get_commit_graph_filename(ctx->odb->path);\n \t}\n \n \tif (safe_create_leading_directories(ctx->graph_name)) {\n@@ -1382,7 +1382,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n \n \tif (ctx->split) {\n-\t\tchar *lock_name = get_chain_filename(ctx->obj_dir);\n+\t\tchar *lock_name = get_chain_filename(ctx->odb->path);\n \n \t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n \n@@ -1506,12 +1506,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-\t\t\tchar *graph_name = get_commit_graph_filename(ctx->obj_dir);\n+\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb->path);\n \t\t\tunlink(graph_name);\n \t\t}\n \n \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));\n-\t\tfinal_graph_name = get_split_graph_filename(ctx->obj_dir,\n+\t\tfinal_graph_name = get_split_graph_filename(ctx->odb->path,\n \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n \n@@ -1553,7 +1553,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->obj_dir, ctx->obj_dir))\n+\t\tif (strcmp(g->obj_dir, ctx->odb->path))\n \t\t\tbreak;\n \n \t\tnum_commits += g->num_commits;\n@@ -1568,7 +1568,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t\tchar *old_graph_name = get_commit_graph_filename(g->obj_dir);\n \n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->obj_dir, ctx->obj_dir)) {\n+\t\t    strcmp(g->obj_dir, ctx->odb->path)) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1719,13 +1719,13 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n \t\texpire_time -= ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n-\t\tchar *chain_file_name = get_chain_filename(ctx->obj_dir);\n+\t\tchar *chain_file_name = get_chain_filename(ctx->odb->path);\n \t\tunlink(chain_file_name);\n \t\tfree(chain_file_name);\n \t\tctx->num_commit_graphs_after = 0;\n \t}\n \n-\tstrbuf_addstr(&path, ctx->obj_dir);\n+\tstrbuf_addstr(&path, ctx->odb->path);\n \tstrbuf_addstr(&path, \"/info/commit-graphs\");\n \tdir = opendir(path.buf);\n \n@@ -1764,7 +1764,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tstrbuf_release(&path);\n }\n \n-int write_commit_graph(const char *obj_dir,\n+int write_commit_graph(struct object_directory *odb,\n \t\t       struct string_list *pack_indexes,\n \t\t       struct string_list *commit_hex,\n \t\t       enum commit_graph_write_flags flags,\n@@ -1772,7 +1772,6 @@ int write_commit_graph(const char *obj_dir,\n {\n \tstruct write_commit_graph_context *ctx;\n \tuint32_t i, count_distinct = 0;\n-\tsize_t len;\n \tint res = 0;\n \n \tif (!commit_graph_compatible(the_repository))\n@@ -1780,14 +1779,7 @@ int write_commit_graph(const char *obj_dir,\n \n \tctx = xcalloc(1, sizeof(struct write_commit_graph_context));\n \tctx->r = the_repository;\n-\n-\t/* normalize object dir with no trailing slash */\n-\tctx->obj_dir = xmallocz(strlen(obj_dir) + 1);\n-\tnormalize_path_copy(ctx->obj_dir, obj_dir);\n-\tlen = strlen(ctx->obj_dir);\n-\tif (len && ctx->obj_dir[len - 1] == '/')\n-\t\tctx->obj_dir[len - 1] = 0;\n-\n+\tctx->odb = odb;\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n@@ -1824,7 +1816,7 @@ int write_commit_graph(const char *obj_dir,\n \t\tctx->oids.alloc = split_opts->max_commits;\n \n \tif (ctx->append) {\n-\t\tprepare_commit_graph_one(ctx->r, ctx->obj_dir);\n+\t\tprepare_commit_graph_one(ctx->r, ctx->odb->path);\n \t\tif (ctx->r->objects->commit_graph)\n \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n \t}\n@@ -1898,7 +1890,6 @@ int write_commit_graph(const char *obj_dir,\n \tfree(ctx->graph_name);\n \tfree(ctx->commits.list);\n \tfree(ctx->oids.list);\n-\tfree(ctx->obj_dir);\n \n \tif (ctx->commit_graph_filenames_after) {\n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 7f5c933fa2..2a6251bd3d 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -5,6 +5,7 @@\n #include \"repository.h\"\n #include \"string-list.h\"\n #include \"cache.h\"\n+#include \"object-store.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n@@ -91,10 +92,10 @@ struct split_commit_graph_opts {\n  * is not compatible with the commit-graph feature, then the\n  * methods will return 0 without writing a commit-graph.\n  */\n-int write_commit_graph_reachable(const char *obj_dir,\n+int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts);\n-int write_commit_graph(const char *obj_dir,\n+int write_commit_graph(struct object_directory *odb,\n \t\t       struct string_list *pack_indexes,\n \t\t       struct string_list *commit_hex,\n \t\t       enum commit_graph_write_flags flags,\n-- \n2.25.0.119.gaa12b7378b\n\n"},{"id":"391062","messageId":"aa12b7378b90abff5cb288a64fbe4a1b335cf73b.1580764494.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580764494.git.me@ttaylorr.com","subject":"[PATCH v2 5/5] commit-graph.h: use odb in 'load_commit_graph_one_fd_st'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T21:18:04Z","receivedAt":"2020-02-03T21:18:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply a similar treatment as in the previous patch to pass a 'struct\nobject_directory *' through the 'load_commit_graph_one_fd_st'\ninitializer, too.\n\nThis prevents a potential bug where a pointer comparison is made to a\nNULL 'g->odb', which would cause the commit-graph machinery to think\nthat a pair of commit-graphs belonged to different alternates when in\nfact they do not (i.e., in the case of no '--object-dir').\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c     |  2 +-\n commit-graph.c             | 21 ++++++++++-----------\n commit-graph.h             |  3 ++-\n t/helper/test-read-graph.c |  2 +-\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex b16eba2a7a..dd6ab84be8 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -95,7 +95,7 @@ static int graph_verify(int argc, const char **argv)\n \tFREE_AND_NULL(graph_name);\n \n \tif (open_ok)\n-\t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n+\t\tgraph = load_commit_graph_one_fd_st(fd, &st, odb);\n \telse\n \t\tgraph = read_commit_graph_one(the_repository, odb);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 49541760b5..656dd647d5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -108,7 +108,8 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n+\t\t\t\t\t\t struct object_directory *odb)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n@@ -124,7 +125,9 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tret = parse_commit_graph(graph_map, fd, graph_size);\n \n-\tif (!ret) {\n+\tif (ret)\n+\t\tret->odb = odb;\n+\telse {\n \t\tmunmap(graph_map, graph_size);\n \t\tclose(fd);\n \t}\n@@ -299,7 +302,8 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \treturn graph;\n }\n \n-static struct commit_graph *load_commit_graph_one(const char *graph_file)\n+static struct commit_graph *load_commit_graph_one(const char *graph_file,\n+\t\t\t\t\t\t  struct object_directory *odb)\n {\n \n \tstruct stat st;\n@@ -310,7 +314,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!open_ok)\n \t\treturn NULL;\n \n-\tg = load_commit_graph_one_fd_st(fd, &st);\n+\tg = load_commit_graph_one_fd_st(fd, &st, odb);\n \n \tif (g)\n \t\tg->filename = xstrdup(graph_file);\n@@ -322,12 +326,9 @@ static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb)\n {\n \tchar *graph_name = get_commit_graph_filename(odb);\n-\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n+\tstruct commit_graph *g = load_commit_graph_one(graph_name, odb);\n \tfree(graph_name);\n \n-\tif (g)\n-\t\tg->odb = odb;\n-\n \treturn g;\n }\n \n@@ -406,13 +407,11 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \t\tvalid = 0;\n \t\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n \t\t\tchar *graph_name = get_split_graph_filename(odb, line.buf);\n-\t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n+\t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name, odb);\n \n \t\t\tfree(graph_name);\n \n \t\t\tif (g) {\n-\t\t\t\tg->odb = odb;\n-\n \t\t\t\tif (add_graph_to_chain(g, graph_chain, oids, i)) {\n \t\t\t\t\tgraph_chain = g;\n \t\t\t\t\tvalid = 1;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 5a690723b0..e87a6f6360 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -61,7 +61,8 @@ struct commit_graph {\n \tconst unsigned char *chunk_base_graphs;\n };\n \n-struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n+\t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\ndiff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\nindex 2c2f65f06c..f8a461767c 100644\n--- a/t/helper/test-read-graph.c\n+++ b/t/helper/test-read-graph.c\n@@ -22,7 +22,7 @@ int cmd__read_graph(int argc, const char **argv)\n \tif (!open_ok)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n-\tgraph = load_commit_graph_one_fd_st(fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st, odb);\n \tif (!graph)\n \t\treturn 1;\n \n-- \n2.25.0.119.gaa12b7378b\n"},{"id":"391063","messageId":"f14e95aa7eb2fa4dce2c3e1a1d81dd022efede0b.1580764494.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580764494.git.me@ttaylorr.com","subject":"[PATCH v2 4/5] commit-graph.c: remove path normalization, comparison","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T21:18:02Z","receivedAt":"2020-02-03T21:18:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of the previous patch, all calls to 'commit-graph.c' functions which\nperform path normalization (for e.g., 'get_commit_graph_filename()') are\nof the form 'ctx->odb->path', which is always in normalized form.\n\nNow that there are no callers passing non-normalized paths to these\nfunctions, ensure that future callers are bound by the same restrictions\nby making these functions take a 'struct object_directory *' instead of\na 'const char *'. To match, replace all calls with arguments of the form\n'ctx->odb->path' with 'ctx->odb' To recover the path, functions that\nperform path manipulation simply use 'odb->path'.\n\nFurther, avoid string comparisons with arguments of the form\n'odb->path', and instead prefer raw pointer comparisons, which\naccomplish the same effect, but are far less brittle.\n\nThis has a pleasant side-effect of making these functions much more\nrobust to paths that cannot be normalized by 'normalize_path_copy()',\ni.e., because they are outside of the current working directory.\n\nFor example, prior to this patch, Valgrind reports that the following\nuninitialized memory read [1]:\n\n  $ ( cd t && GIT_DIR=../.git valgrind git rev-parse HEAD^ )\n\nbecause 'normalize_path_copy()' can't normalize '../.git' (since it's\nrelative to but above of the current working directory) [2].\n\nBy using a 'struct object_directory *' directly,\n'get_commit_graph_filename()' does not need to normalize, because all\npaths are relative to the current working directory since they are\nalways read from the '->path' of an object directory.\n\n[1]: https://lore.kernel.org/git/20191027042116.GA5801@sigill.intra.peff.net.\n[2]: The bug here is that 'get_commit_graph_filename()' returns the\n     result of 'normalize_path_copy()' without checking the return\n     value.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c     |  2 +-\n commit-graph.c             | 47 +++++++++++++++-----------------------\n commit-graph.h             |  2 +-\n t/helper/test-read-graph.c |  6 ++---\n 4 files changed, 24 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 3501d9077b..b16eba2a7a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -87,7 +87,7 @@ static int graph_verify(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \todb = find_odb(the_repository, opts.obj_dir);\n-\tgraph_name = get_commit_graph_filename(odb->path);\n+\tgraph_name = get_commit_graph_filename(odb);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok && errno != ENOENT)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 3af4a721ee..49541760b5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -44,30 +44,21 @@\n /* Remember to update object flag allocation in object.h */\n #define REACHABLE       (1u<<15)\n \n-char *get_commit_graph_filename(const char *obj_dir)\n+char *get_commit_graph_filename(struct object_directory *odb)\n {\n-\tchar *filename = xstrfmt(\"%s/info/commit-graph\", obj_dir);\n-\tchar *normalized = xmalloc(strlen(filename) + 1);\n-\tnormalize_path_copy(normalized, filename);\n-\tfree(filename);\n-\treturn normalized;\n+\treturn xstrfmt(\"%s/info/commit-graph\", odb->path);\n }\n \n-static char *get_split_graph_filename(const char *obj_dir,\n+static char *get_split_graph_filename(struct object_directory *odb,\n \t\t\t\t      const char *oid_hex)\n {\n-\tchar *filename = xstrfmt(\"%s/info/commit-graphs/graph-%s.graph\",\n-\t\t\t\t obj_dir,\n-\t\t\t\t oid_hex);\n-\tchar *normalized = xmalloc(strlen(filename) + 1);\n-\tnormalize_path_copy(normalized, filename);\n-\tfree(filename);\n-\treturn normalized;\n+\treturn xstrfmt(\"%s/info/commit-graphs/graph-%s.graph\", odb->path,\n+\t\t       oid_hex);\n }\n \n-static char *get_chain_filename(const char *obj_dir)\n+static char *get_chain_filename(struct object_directory *odb)\n {\n-\treturn xstrfmt(\"%s/info/commit-graphs/commit-graph-chain\", obj_dir);\n+\treturn xstrfmt(\"%s/info/commit-graphs/commit-graph-chain\", odb->path);\n }\n \n static uint8_t oid_version(void)\n@@ -330,7 +321,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb)\n {\n-\tchar *graph_name = get_commit_graph_filename(odb->path);\n+\tchar *graph_name = get_commit_graph_filename(odb);\n \tstruct commit_graph *g = load_commit_graph_one(graph_name);\n \tfree(graph_name);\n \n@@ -381,7 +372,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n-\tchar *chain_name = get_chain_filename(odb->path);\n+\tchar *chain_name = get_chain_filename(odb);\n \tFILE *fp;\n \tint stat_res;\n \n@@ -414,7 +405,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \n \t\tvalid = 0;\n \t\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n-\t\t\tchar *graph_name = get_split_graph_filename(odb->path, line.buf);\n+\t\t\tchar *graph_name = get_split_graph_filename(odb, line.buf);\n \t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n \n \t\t\tfree(graph_name);\n@@ -1375,7 +1366,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t    ctx->odb->path);\n \t\tctx->graph_name = strbuf_detach(&tmp_file, NULL);\n \t} else {\n-\t\tctx->graph_name = get_commit_graph_filename(ctx->odb->path);\n+\t\tctx->graph_name = get_commit_graph_filename(ctx->odb);\n \t}\n \n \tif (safe_create_leading_directories(ctx->graph_name)) {\n@@ -1386,7 +1377,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n \n \tif (ctx->split) {\n-\t\tchar *lock_name = get_chain_filename(ctx->odb->path);\n+\t\tchar *lock_name = get_chain_filename(ctx->odb);\n \n \t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n \n@@ -1474,7 +1465,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \tif (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {\n \t\tchar *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));\n-\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);\n+\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb, new_base_hash);\n \n \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);\n \t\tfree(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);\n@@ -1510,12 +1501,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb->path);\n+\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb);\n \t\t\tunlink(graph_name);\n \t\t}\n \n \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));\n-\t\tfinal_graph_name = get_split_graph_filename(ctx->odb->path,\n+\t\tfinal_graph_name = get_split_graph_filename(ctx->odb,\n \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n \n@@ -1557,7 +1548,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->odb->path, ctx->odb->path))\n+\t\tif (g->odb != ctx->odb)\n \t\t\tbreak;\n \n \t\tnum_commits += g->num_commits;\n@@ -1569,10 +1560,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \tctx->new_base_graph = g;\n \n \tif (ctx->num_commit_graphs_after == 2) {\n-\t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n+\t\tchar *old_graph_name = get_commit_graph_filename(g->odb);\n \n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->odb->path, ctx->odb->path)) {\n+\t\t    g->odb != ctx->odb) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1723,7 +1714,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n \t\texpire_time -= ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n-\t\tchar *chain_file_name = get_chain_filename(ctx->odb->path);\n+\t\tchar *chain_file_name = get_chain_filename(ctx->odb);\n \t\tunlink(chain_file_name);\n \t\tfree(chain_file_name);\n \t\tctx->num_commit_graphs_after = 0;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 2448134378..5a690723b0 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -12,7 +12,7 @@\n \n struct commit;\n \n-char *get_commit_graph_filename(const char *obj_dir);\n+char *get_commit_graph_filename(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n \n /*\ndiff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\nindex d2884efe0a..2c2f65f06c 100644\n--- a/t/helper/test-read-graph.c\n+++ b/t/helper/test-read-graph.c\n@@ -11,12 +11,12 @@ int cmd__read_graph(int argc, const char **argv)\n \tint open_ok;\n \tint fd;\n \tstruct stat st;\n-\tconst char *object_dir;\n+\tstruct object_directory *odb;\n \n \tsetup_git_directory();\n-\tobject_dir = get_object_directory();\n+\todb = the_repository->objects->odb;\n \n-\tgraph_name = get_commit_graph_filename(object_dir);\n+\tgraph_name = get_commit_graph_filename(odb);\n \n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok)\n-- \n2.25.0.119.gaa12b7378b\n\n"},{"id":"391064","messageId":"5fd5cfca6e9a59c0bbf62501e6064106cf87df49.1580764494.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"cover.1580764494.git.me@ttaylorr.com","subject":"[PATCH v2 3/5] commit-graph.h: store object directory in 'struct commit_graph'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-03T21:18:00Z","receivedAt":"2020-02-03T21:18:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a previous patch, the 'char *object_dir' in 'struct commit_graph' was\nreplaced with a 'struct object_directory'. This patch applies the same\ntreatment to 'struct commit_graph', which is another intermediate step\ntowards getting rid of all path normalization in 'commit-graph.c'.\n\nInstead of taking a 'char *object_dir', functions that construct a\n'struct commit_graph' now take a 'struct object_directory *'. Any code\nthat needs an object directory path use '->path' instead.\n\nThis ensures that all calls to functions that perform path normalization\nare given arguments which do not themselves require normalization. This\nprepares those functions to drop their normalization entirely, which\nwill occur in the subsequent patch.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c |  2 +-\n commit-graph.c         | 38 +++++++++++++++++++++-----------------\n commit-graph.h         |  5 +++--\n 3 files changed, 25 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 31b57e4e1d..3501d9077b 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -97,7 +97,7 @@ static int graph_verify(int argc, const char **argv)\n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \telse\n-\t\tgraph = read_commit_graph_one(the_repository, odb->path);\n+\t\tgraph = read_commit_graph_one(the_repository, odb);\n \n \t/* Return failure if open_ok predicted success */\n \tif (!graph)\ndiff --git a/commit-graph.c b/commit-graph.c\nindex cbfeece112..3af4a721ee 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -327,14 +327,15 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n \treturn g;\n }\n \n-static struct commit_graph *load_commit_graph_v1(struct repository *r, const char *obj_dir)\n+static struct commit_graph *load_commit_graph_v1(struct repository *r,\n+\t\t\t\t\t\t struct object_directory *odb)\n {\n-\tchar *graph_name = get_commit_graph_filename(obj_dir);\n+\tchar *graph_name = get_commit_graph_filename(odb->path);\n \tstruct commit_graph *g = load_commit_graph_one(graph_name);\n \tfree(graph_name);\n \n \tif (g)\n-\t\tg->obj_dir = obj_dir;\n+\t\tg->odb = odb;\n \n \treturn g;\n }\n@@ -372,14 +373,15 @@ static int add_graph_to_chain(struct commit_graph *g,\n \treturn 1;\n }\n \n-static struct commit_graph *load_commit_graph_chain(struct repository *r, const char *obj_dir)\n+static struct commit_graph *load_commit_graph_chain(struct repository *r,\n+\t\t\t\t\t\t    struct object_directory *odb)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n-\tchar *chain_name = get_chain_filename(obj_dir);\n+\tchar *chain_name = get_chain_filename(odb->path);\n \tFILE *fp;\n \tint stat_res;\n \n@@ -418,7 +420,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, const\n \t\t\tfree(graph_name);\n \n \t\t\tif (g) {\n-\t\t\t\tg->obj_dir = odb->path;\n+\t\t\t\tg->odb = odb;\n \n \t\t\t\tif (add_graph_to_chain(g, graph_chain, oids, i)) {\n \t\t\t\t\tgraph_chain = g;\n@@ -442,23 +444,25 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, const\n \treturn graph_chain;\n }\n \n-struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir)\n+struct commit_graph *read_commit_graph_one(struct repository *r,\n+\t\t\t\t\t   struct object_directory *odb)\n {\n-\tstruct commit_graph *g = load_commit_graph_v1(r, obj_dir);\n+\tstruct commit_graph *g = load_commit_graph_v1(r, odb);\n \n \tif (!g)\n-\t\tg = load_commit_graph_chain(r, obj_dir);\n+\t\tg = load_commit_graph_chain(r, odb);\n \n \treturn g;\n }\n \n-static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n+static void prepare_commit_graph_one(struct repository *r,\n+\t\t\t\t     struct object_directory *odb)\n {\n \n \tif (r->objects->commit_graph)\n \t\treturn;\n \n-\tr->objects->commit_graph = read_commit_graph_one(r, obj_dir);\n+\tr->objects->commit_graph = read_commit_graph_one(r, odb);\n }\n \n /*\n@@ -505,7 +509,7 @@ static int prepare_commit_graph(struct repository *r)\n \tfor (odb = r->objects->odb;\n \t     !r->objects->commit_graph && odb;\n \t     odb = odb->next)\n-\t\tprepare_commit_graph_one(r, odb->path);\n+\t\tprepare_commit_graph_one(r, odb);\n \treturn !!r->objects->commit_graph;\n }\n \n@@ -1470,7 +1474,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \tif (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {\n \t\tchar *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));\n-\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->obj_dir, new_base_hash);\n+\t\tchar *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);\n \n \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);\n \t\tfree(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);\n@@ -1553,7 +1557,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->obj_dir, ctx->odb->path))\n+\t\tif (strcmp(g->odb->path, ctx->odb->path))\n \t\t\tbreak;\n \n \t\tnum_commits += g->num_commits;\n@@ -1565,10 +1569,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \tctx->new_base_graph = g;\n \n \tif (ctx->num_commit_graphs_after == 2) {\n-\t\tchar *old_graph_name = get_commit_graph_filename(g->obj_dir);\n+\t\tchar *old_graph_name = get_commit_graph_filename(g->odb->path);\n \n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->obj_dir, ctx->odb->path)) {\n+\t\t    strcmp(g->odb->path, ctx->odb->path)) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1816,7 +1820,7 @@ int write_commit_graph(struct object_directory *odb,\n \t\tctx->oids.alloc = split_opts->max_commits;\n \n \tif (ctx->append) {\n-\t\tprepare_commit_graph_one(ctx->r, ctx->odb->path);\n+\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n \t\tif (ctx->r->objects->commit_graph)\n \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 2a6251bd3d..2448134378 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -49,7 +49,7 @@ struct commit_graph {\n \tuint32_t num_commits;\n \tstruct object_id oid;\n \tchar *filename;\n-\tconst char *obj_dir;\n+\tstruct object_directory *odb;\n \n \tuint32_t num_commits_in_base;\n \tstruct commit_graph *base_graph;\n@@ -62,7 +62,8 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n-struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir);\n+struct commit_graph *read_commit_graph_one(struct repository *r,\n+\t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\n \n-- \n2.25.0.119.gaa12b7378b\n\n"},{"id":"391078","messageId":"5d3819180dbc9bc33a8fe4354e2320f497151fb4.1580795403.git.me@ttaylorr.com","threadId":"52724","inReplyTo":"d9819cfb33ad95d4206dd1bbf4b38b7fdf69130f.1580764494.git.me@ttaylorr.com","subject":"[PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-04T05:51:50Z","receivedAt":"2020-02-04T05:51:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Whoops. In v2, this patch introduces 'find_odb()' as a function in\n'builtin/commit-graph.c', but does not declare it static. This causes\nbreakage in gcc with '-Wmissing-prototypes'. Here is a correct version\nof the patch that does not cause such breakage.\n\n-- 8< --\n\nSubject: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct\n\nThere are lots of places in 'commit-graph.h' where a function either has\n(or almost has) a full 'struct object_directory *', accesses '->path',\nand then throws away the rest of the struct.\n\nThis can cause headaches when comparing the locations of object\ndirectories across alternates (e.g., in the case of deciding if two\ncommit-graph layers can be merged). These paths are normalized with\n'normalize_path_copy()' which mitigates some comparison issues, but not\nall [1].\n\nReplace usage of 'char *object_dir' with 'odb->path' by storing a\n'struct object_directory *' in the 'write_commit_graph_context'\nstructure. This is an intermediate step towards getting rid of all path\nnormalization in 'commit-graph.c'.\n\nResolving a user-provided '--object-dir' argument now requires that we\ncompare it to the known alternates for equality.  Prior to this patch,\nan unknown '--object-dir' argument would silently exit with status zero.\n\nThis can clearly lead to unintended behavior, such as verifying\ncommit-graphs that aren't in a repository's own object store (or one of\nits alternates), or causing a typo to mask a legitimate commit-graph\nverification failure. Make this error non-silent by 'die()'-ing when the\ngiven '--object-dir' does not match any known alternate object store.\n\n[1]: In my testing, for example, I can get one side of the commit-graph\ncode to fill object_dir with \"./objects\" and the other with just\n\"objects\".\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-commit-graph.txt |  5 +++-\n builtin/commit-graph.c             | 33 ++++++++++++++++++++----\n builtin/commit.c                   |  2 +-\n builtin/fetch.c                    |  2 +-\n builtin/gc.c                       |  2 +-\n commit-graph.c                     | 41 ++++++++++++------------------\n commit-graph.h                     |  5 ++--\n 7 files changed, 54 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex bcd85c1976..28d1fee505 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -26,7 +26,10 @@ OPTIONS\n \tfile. This parameter exists to specify the location of an alternate\n \tthat only has the objects directory, not a full `.git` directory. The\n \tcommit-graph file is expected to be in the `<dir>/info` directory and\n-\tthe packfiles are expected to be in `<dir>/pack`.\n+\tthe packfiles are expected to be in `<dir>/pack`. If the directory\n+\tcould not be made into an absolute path, or does not match any known\n+\tobject directory, `git commit-graph ...` will exit with non-zero\n+\tstatus.\n\n --[no-]progress::\n \tTurn progress on/off explicitly. If neither is specified, progress is\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex e0c6fc4bbf..20a3d31b76 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -34,9 +34,29 @@ static struct opts_commit_graph {\n \tint progress;\n } opts;\n\n+static struct object_directory *find_odb(struct repository *r,\n+\t\t\t\t\t const char *obj_dir)\n+{\n+\tstruct object_directory *odb;\n+\tchar *obj_dir_real = real_pathdup(obj_dir, 1);\n+\n+\tprepare_alt_odb(r);\n+\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n+\t\tif (!strcmp(obj_dir_real, real_path(odb->path)))\n+\t\t\tbreak;\n+\t}\n+\n+\tfree(obj_dir_real);\n+\n+\tif (!odb)\n+\t\tdie(_(\"could not find object directory matching %s\"), obj_dir);\n+\treturn odb;\n+}\n+\n static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n+\tstruct object_directory *odb = NULL;\n \tchar *graph_name;\n \tint open_ok;\n \tint fd;\n@@ -67,7 +87,8 @@ static int graph_verify(int argc, const char **argv)\n \tif (opts.progress)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n\n-\tgraph_name = get_commit_graph_filename(opts.obj_dir);\n+\todb = find_odb(the_repository, opts.obj_dir);\n+\tgraph_name = get_commit_graph_filename(odb->path);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok && errno != ENOENT)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n@@ -76,8 +97,8 @@ static int graph_verify(int argc, const char **argv)\n\n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(fd, &st);\n-\t else\n-\t\tgraph = read_commit_graph_one(the_repository, opts.obj_dir);\n+\telse\n+\t\tgraph = read_commit_graph_one(the_repository, odb->path);\n\n \t/* Return failure if open_ok predicted success */\n \tif (!graph)\n@@ -94,6 +115,7 @@ static int graph_write(int argc, const char **argv)\n {\n \tstruct string_list *pack_indexes = NULL;\n \tstruct string_list *commit_hex = NULL;\n+\tstruct object_directory *odb = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n@@ -145,9 +167,10 @@ static int graph_write(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n\n \tread_replace_refs = 0;\n+\todb = find_odb(the_repository, opts.obj_dir);\n\n \tif (opts.reachable) {\n-\t\tif (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))\n+\t\tif (write_commit_graph_reachable(odb, flags, &split_opts))\n \t\t\treturn 1;\n \t\treturn 0;\n \t}\n@@ -169,7 +192,7 @@ static int graph_write(int argc, const char **argv)\n \t\tUNLEAK(buf);\n \t}\n\n-\tif (write_commit_graph(opts.obj_dir,\n+\tif (write_commit_graph(odb,\n \t\t\t       pack_indexes,\n \t\t\t       commit_hex,\n \t\t\t       flags,\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 646e84547d..9e124aaa7b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1689,7 +1689,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t      \"not exceeded, and then \\\"git restore --staged :/\\\" to recover.\"));\n\n \tif (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&\n-\t    write_commit_graph_reachable(get_object_directory(), 0, NULL))\n+\t    write_commit_graph_reachable(the_repository->objects->odb, 0, NULL))\n \t\treturn 1;\n\n \trepo_rerere(the_repository, 0);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d0..1ce16c96e9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1870,7 +1870,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (progress)\n \t\t\tcommit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;\n\n-\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     commit_graph_flags,\n \t\t\t\t\t     NULL);\n \t}\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3f76bf4aa7..8e0b9cf41b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -686,7 +686,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n\n \tprepare_repo_settings(the_repository);\n \tif (the_repository->settings.gc_write_commit_graph == 1)\n-\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n \t\t\t\t\t     NULL);\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b205e65ed1..cbfeece112 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -772,7 +772,7 @@ struct packed_oid_list {\n\n struct write_commit_graph_context {\n \tstruct repository *r;\n-\tchar *obj_dir;\n+\tstruct object_directory *odb;\n \tchar *graph_name;\n \tstruct packed_oid_list oids;\n \tstruct packed_commit_list commits;\n@@ -1149,7 +1149,7 @@ static int add_ref_to_list(const char *refname,\n \treturn 0;\n }\n\n-int write_commit_graph_reachable(const char *obj_dir,\n+int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n@@ -1157,7 +1157,7 @@ int write_commit_graph_reachable(const char *obj_dir,\n \tint result;\n\n \tfor_each_ref(add_ref_to_list, &list);\n-\tresult = write_commit_graph(obj_dir, NULL, &list,\n+\tresult = write_commit_graph(odb, NULL, &list,\n \t\t\t\t    flags, split_opts);\n\n \tstring_list_clear(&list, 0);\n@@ -1172,7 +1172,7 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \tstruct strbuf packname = STRBUF_INIT;\n \tint dirlen;\n\n-\tstrbuf_addf(&packname, \"%s/pack/\", ctx->obj_dir);\n+\tstrbuf_addf(&packname, \"%s/pack/\", ctx->odb->path);\n \tdirlen = packname.len;\n \tif (ctx->report_progress) {\n \t\tstrbuf_addf(&progress_title,\n@@ -1368,10 +1368,10 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n\n \t\tstrbuf_addf(&tmp_file,\n \t\t\t    \"%s/info/commit-graphs/tmp_graph_XXXXXX\",\n-\t\t\t    ctx->obj_dir);\n+\t\t\t    ctx->odb->path);\n \t\tctx->graph_name = strbuf_detach(&tmp_file, NULL);\n \t} else {\n-\t\tctx->graph_name = get_commit_graph_filename(ctx->obj_dir);\n+\t\tctx->graph_name = get_commit_graph_filename(ctx->odb->path);\n \t}\n\n \tif (safe_create_leading_directories(ctx->graph_name)) {\n@@ -1382,7 +1382,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n\n \tif (ctx->split) {\n-\t\tchar *lock_name = get_chain_filename(ctx->obj_dir);\n+\t\tchar *lock_name = get_chain_filename(ctx->odb->path);\n\n \t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n\n@@ -1506,12 +1506,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-\t\t\tchar *graph_name = get_commit_graph_filename(ctx->obj_dir);\n+\t\t\tchar *graph_name = get_commit_graph_filename(ctx->odb->path);\n \t\t\tunlink(graph_name);\n \t\t}\n\n \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));\n-\t\tfinal_graph_name = get_split_graph_filename(ctx->obj_dir,\n+\t\tfinal_graph_name = get_split_graph_filename(ctx->odb->path,\n \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n\n@@ -1553,7 +1553,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n\n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n \t\t    (max_commits && num_commits > max_commits))) {\n-\t\tif (strcmp(g->obj_dir, ctx->obj_dir))\n+\t\tif (strcmp(g->obj_dir, ctx->odb->path))\n \t\t\tbreak;\n\n \t\tnum_commits += g->num_commits;\n@@ -1568,7 +1568,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t\tchar *old_graph_name = get_commit_graph_filename(g->obj_dir);\n\n \t\tif (!strcmp(g->filename, old_graph_name) &&\n-\t\t    strcmp(g->obj_dir, ctx->obj_dir)) {\n+\t\t    strcmp(g->obj_dir, ctx->odb->path)) {\n \t\t\tctx->num_commit_graphs_after = 1;\n \t\t\tctx->new_base_graph = NULL;\n \t\t}\n@@ -1719,13 +1719,13 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n \t\texpire_time -= ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n-\t\tchar *chain_file_name = get_chain_filename(ctx->obj_dir);\n+\t\tchar *chain_file_name = get_chain_filename(ctx->odb->path);\n \t\tunlink(chain_file_name);\n \t\tfree(chain_file_name);\n \t\tctx->num_commit_graphs_after = 0;\n \t}\n\n-\tstrbuf_addstr(&path, ctx->obj_dir);\n+\tstrbuf_addstr(&path, ctx->odb->path);\n \tstrbuf_addstr(&path, \"/info/commit-graphs\");\n \tdir = opendir(path.buf);\n\n@@ -1764,7 +1764,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \tstrbuf_release(&path);\n }\n\n-int write_commit_graph(const char *obj_dir,\n+int write_commit_graph(struct object_directory *odb,\n \t\t       struct string_list *pack_indexes,\n \t\t       struct string_list *commit_hex,\n \t\t       enum commit_graph_write_flags flags,\n@@ -1772,7 +1772,6 @@ int write_commit_graph(const char *obj_dir,\n {\n \tstruct write_commit_graph_context *ctx;\n \tuint32_t i, count_distinct = 0;\n-\tsize_t len;\n \tint res = 0;\n\n \tif (!commit_graph_compatible(the_repository))\n@@ -1780,14 +1779,7 @@ int write_commit_graph(const char *obj_dir,\n\n \tctx = xcalloc(1, sizeof(struct write_commit_graph_context));\n \tctx->r = the_repository;\n-\n-\t/* normalize object dir with no trailing slash */\n-\tctx->obj_dir = xmallocz(strlen(obj_dir) + 1);\n-\tnormalize_path_copy(ctx->obj_dir, obj_dir);\n-\tlen = strlen(ctx->obj_dir);\n-\tif (len && ctx->obj_dir[len - 1] == '/')\n-\t\tctx->obj_dir[len - 1] = 0;\n-\n+\tctx->odb = odb;\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n@@ -1824,7 +1816,7 @@ int write_commit_graph(const char *obj_dir,\n \t\tctx->oids.alloc = split_opts->max_commits;\n\n \tif (ctx->append) {\n-\t\tprepare_commit_graph_one(ctx->r, ctx->obj_dir);\n+\t\tprepare_commit_graph_one(ctx->r, ctx->odb->path);\n \t\tif (ctx->r->objects->commit_graph)\n \t\t\tctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;\n \t}\n@@ -1898,7 +1890,6 @@ int write_commit_graph(const char *obj_dir,\n \tfree(ctx->graph_name);\n \tfree(ctx->commits.list);\n \tfree(ctx->oids.list);\n-\tfree(ctx->obj_dir);\n\n \tif (ctx->commit_graph_filenames_after) {\n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 7f5c933fa2..2a6251bd3d 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -5,6 +5,7 @@\n #include \"repository.h\"\n #include \"string-list.h\"\n #include \"cache.h\"\n+#include \"object-store.h\"\n\n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n@@ -91,10 +92,10 @@ struct split_commit_graph_opts {\n  * is not compatible with the commit-graph feature, then the\n  * methods will return 0 without writing a commit-graph.\n  */\n-int write_commit_graph_reachable(const char *obj_dir,\n+int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t enum commit_graph_write_flags flags,\n \t\t\t\t const struct split_commit_graph_opts *split_opts);\n-int write_commit_graph(const char *obj_dir,\n+int write_commit_graph(struct object_directory *odb,\n \t\t       struct string_list *pack_indexes,\n \t\t       struct string_list *commit_hex,\n \t\t       enum commit_graph_write_flags flags,\n--\n2.25.0.119.gaa12b7378b\n"},{"id":"391096","messageId":"xmqqzhdy6sbx.fsf@gitster-ct.c.googlers.com","threadId":"52724","inReplyTo":"5d3819180dbc9bc33a8fe4354e2320f497151fb4.1580795403.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-04T19:54:58Z","receivedAt":"2020-02-04T19:55:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Whoops. In v2, this patch introduces 'find_odb()' as a function in\n> 'builtin/commit-graph.c', but does not declare it static. This causes\n> breakage in gcc with '-Wmissing-prototypes'. Here is a correct version\n> of the patch that does not cause such breakage.\n>\n> -- 8< --\n>\n> Subject: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct\n\nWhat happened to the rest of the line ;-)?\n"},{"id":"391125","messageId":"20200204212833.GA42626@syl.local","threadId":"52724","inReplyTo":"xmqqzhdy6sbx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-04T21:28:33Z","receivedAt":"2020-02-04T21:28:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 04, 2020 at 11:54:58AM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > Whoops. In v2, this patch introduces 'find_odb()' as a function in\n> > 'builtin/commit-graph.c', but does not declare it static. This causes\n> > breakage in gcc with '-Wmissing-prototypes'. Here is a correct version\n> > of the patch that does not cause such breakage.\n> >\n> > -- 8< --\n> >\n> > Subject: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct\n>\n> What happened to the rest of the line ;-)?\n\nHeh. Your email made me chuckle since this wasn't the first time I've\nbeen asked about this today ;-). I manually yanked the 'Subject:' line,\nbut not the continuation below it.\n\nI'd be happy to re-send a version of this patch to fix this, but if you\ndon't mind slicing it up, that works too.\n\nThanks,\nTaylor\n"},{"id":"391128","messageId":"xmqqa75y6n8y.fsf@gitster-ct.c.googlers.com","threadId":"52724","inReplyTo":"20200204212833.GA42626@syl.local","subject":"Re: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-04T21:44:45Z","receivedAt":"2020-02-04T21:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Tue, Feb 04, 2020 at 11:54:58AM -0800, Junio C Hamano wrote:\n>> Taylor Blau <me@ttaylorr.com> writes:\n>>\n>> > Whoops. In v2, this patch introduces 'find_odb()' as a function in\n>> > 'builtin/commit-graph.c', but does not declare it static. This causes\n>> > breakage in gcc with '-Wmissing-prototypes'. Here is a correct version\n>> > of the patch that does not cause such breakage.\n>> >\n>> > -- 8< --\n>> >\n>> > Subject: [PATCH v2 2/5] commit-graph.h: store an odb in 'struct\n>>\n>> What happened to the rest of the line ;-)?\n>\n> Heh. Your email made me chuckle since this wasn't the first time I've\n> been asked about this today ;-). I manually yanked the 'Subject:' line,\n> but not the continuation below it.\n>\n> I'd be happy to re-send a version of this patch to fix this, but if you\n> don't mind slicing it up, that works too.\n>\n> Thanks,\n> Taylor\n\nNoneedtoresend. Thanks.\n"},{"id":"391162","messageId":"20200205123018.GB55322@coredump.intra.peff.net","threadId":"52724","inReplyTo":"cover.1580764494.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/5] commit-graph: use 'struct object_directory *' everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-05T12:30:18Z","receivedAt":"2020-02-05T12:30:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 03, 2020 at 01:17:53PM -0800, Taylor Blau wrote:\n\n> Here is a re-roll of the series in this thread to replace string-based\n> path comparison in 'commit-graph.c' code with raw pointer comparison of\n> 'struct object_directory *'s.\n> \n> The only thing that has changed substantially since v1 is that I swapped\n> the order of patches 2/6 and 4/6. What was patch 3/6 got folded into\n> what is now patch 2/5. This resolves an inconvenience where we had to\n> define a helper function in 'commit-graph.c', when it really belonged in\n> 'builtin/commit-graph.c'.\n> \n> I took a few other suggestions from Martin in what is now patch 4/5, and\n> noticed a few other small things along the way. A range-diff since v1 is\n> included below.\n\nThanks, I read over this and the new layout looks quite good.\n\n-Peff\n"}]}