Hi,
Here is a re-roll of the series in this thread to replace string-based path comparison in 'commit-graph.c' code with raw pointer comparison of 'struct object_directory *'s.
The only thing that has changed substantially since v1 is that I swapped the order of patches 2/6 and 4/6. What was patch 3/6 got folded into what is now patch 2/5. This resolves an inconvenience where we had to define a helper function in 'commit-graph.c', when it really belonged in 'builtin/commit-graph.c'.
I took a few other suggestions from Martin in what is now patch 4/5, and noticed a few other small things along the way. A range-diff since v1 is included below.
Thanks as always, Taylor
Taylor Blau (5):
t5318: don't pass non-object directory to '--object-dir'
commit-graph.h: store an odb in 'struct write_commit_graph_context'
commit-graph.h: store object directory in 'struct commit_graph'
commit-graph.c: remove path normalization, comparison
commit-graph.h: use odb in 'load_commit_graph_one_fd_st'
Documentation/git-commit-graph.txt | 5 +-
builtin/commit-graph.c | 34 +++++++--
builtin/commit.c | 2 +-
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
commit-graph.c | 115 +++++++++++++----------------
commit-graph.h | 15 ++--
t/helper/test-read-graph.c | 8 +-
t/t5318-commit-graph.sh | 4 +-
9 files changed, 100 insertions(+), 87 deletions(-)
Range-diff against v1:
1: 09ba72c85a = 1: 84a8709ad1 t5318: don't pass non-object directory to '--object-dir'
4: 9784a5db3c ! 2: d9819cfb33 commit-graph.h: store an odb in 'struct write_commit_graph_context'
@@ Metadata
## Commit message ##
commit-graph.h: store an odb in 'struct write_commit_graph_context' - In a previous patch, the 'char *object_dir' in 'struct commit_graph' was
- replaced with a 'struct object_directory'. This patch applies the same
- treatement to 'struct write_commit_graph_context', which is an
- intermediate step towards getting rid of all path normalization in
- 'commit-graph.c'.
-
- Instead of taking a 'char *object_dir', functions that construct a
- 'struct write_commit_graph_context' now take a 'struct object_directory
- *'. Any code that needs an object directory path use '->path' instead.
-
- This ensures that all calls to functions that perform path normalization
- are given arguments which do not themselves require normalization. This
- prepares those functions to drop their normalization entirely, which
- will occur in the subsequent patch.
+ There are lots of places in 'commit-graph.h' where a function either has
+ (or almost has) a full 'struct object_directory *', accesses '->path',
+ and then throws away the rest of the struct.
+
+ This can cause headaches when comparing the locations of object
+ directories across alternates (e.g., in the case of deciding if two
+ commit-graph layers can be merged). These paths are normalized with
+ 'normalize_path_copy()' which mitigates some comparison issues, but not
+ all [1].
+
+ Replace usage of 'char *object_dir' with 'odb->path' by storing a
+ 'struct object_directory *' in the 'write_commit_graph_context'
+ structure. This is an intermediate step towards getting rid of all path
+ normalization in 'commit-graph.c'.
+
+ Resolving a user-provided '--object-dir' argument now requires that we
+ compare it to the known alternates for equality. Prior to this patch,
+ an unknown '--object-dir' argument would silently exit with status zero.
+
+ This can clearly lead to unintended behavior, such as verifying
+ commit-graphs that aren't in a repository's own object store (or one of
+ its alternates), or causing a typo to mask a legitimate commit-graph
+ verification failure. Make this error non-silent by 'die()'-ing when the
+ given '--object-dir' does not match any known alternate object store.
+
+ [1]: In my testing, for example, I can get one side of the commit-graph
+ code to fill object_dir with "./objects" and the other with just
+ "objects". Signed-off-by: Taylor Blau <me@ttaylorr.com>
+ ## Documentation/git-commit-graph.txt ##
+@@ Documentation/git-commit-graph.txt: OPTIONS
+ file. This parameter exists to specify the location of an alternate
+ that only has the objects directory, not a full `.git` directory. The
+ commit-graph file is expected to be in the `<dir>/info` directory and
+- the packfiles are expected to be in `<dir>/pack`.
++ the packfiles are expected to be in `<dir>/pack`. If the directory
++ could not be made into an absolute path, or does not match any known
++ object directory, `git commit-graph ...` will exit with non-zero
++ status.
+
+ --[no-]progress::
+ Turn progress on/off explicitly. If neither is specified, progress is
+
## builtin/commit-graph.c ##
+@@ builtin/commit-graph.c: static struct opts_commit_graph {
+ int progress;
+ } opts;
+
++struct object_directory *find_odb(struct repository *r, const char *obj_dir)
++{
++ struct object_directory *odb;
++ char *obj_dir_real = real_pathdup(obj_dir, 1);
++
++ prepare_alt_odb(r);
++ for (odb = r->objects->odb; odb; odb = odb->next) {
++ if (!strcmp(obj_dir_real, real_path(odb->path)))
++ break;
++ }
++
++ free(obj_dir_real);
++
++ if (!odb)
++ die(_("could not find object directory matching %s"), obj_dir);
++ return odb;
++}
++
+ static int graph_verify(int argc, const char **argv)
+ {
+ struct commit_graph *graph = NULL;
++ struct object_directory *odb = NULL;
+ char *graph_name;
+ int open_ok;
+ int fd;
+@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
+ if (opts.progress)
+ flags |= COMMIT_GRAPH_WRITE_PROGRESS;
+
+- graph_name = get_commit_graph_filename(opts.obj_dir);
++ odb = find_odb(the_repository, opts.obj_dir);
++ graph_name = get_commit_graph_filename(odb->path);
+ open_ok = open_commit_graph(graph_name, &fd, &st);
+ if (!open_ok && errno != ENOENT)
+ die_errno(_("Could not open commit-graph '%s'"), graph_name);
+@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
+
+ if (open_ok)
+ graph = load_commit_graph_one_fd_st(fd, &st);
+- else
+- graph = read_commit_graph_one(the_repository, opts.obj_dir);
++ else
++ graph = read_commit_graph_one(the_repository, odb->path);
+
+ /* Return failure if open_ok predicted success */
+ if (!graph)
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
- odb = find_odb_or_die(the_repository, opts.obj_dir);
+ {
+ struct string_list *pack_indexes = NULL;
+ struct string_list *commit_hex = NULL;
++ struct object_directory *odb = NULL;
+ struct string_list lines;
+ int result = 0;
+ enum commit_graph_write_flags flags = 0;
+@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
+ flags |= COMMIT_GRAPH_WRITE_PROGRESS;
+
+ read_replace_refs = 0;
++ odb = find_odb(the_repository, opts.obj_dir); if (opts.reachable) {
-- if (write_commit_graph_reachable(odb->path, flags, &split_opts))
+- if (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))
+ if (write_commit_graph_reachable(odb, flags, &split_opts))
return 1;
return 0;
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
UNLEAK(buf);
} -- if (write_commit_graph(odb->path,
+- if (write_commit_graph(opts.obj_dir,
+ if (write_commit_graph(odb,
pack_indexes,
commit_hex,
@@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph while (g && (g->num_commits <= size_mult * num_commits ||
(max_commits && num_commits > max_commits))) {
-- if (strcmp(g->odb->path, ctx->obj_dir))
-+ if (strcmp(g->odb->path, ctx->odb->path))
+- if (strcmp(g->obj_dir, ctx->obj_dir))
++ if (strcmp(g->obj_dir, ctx->odb->path))
break; num_commits += g->num_commits;
@@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
- char *old_graph_name = get_commit_graph_filename(g->odb->path);
+ char *old_graph_name = get_commit_graph_filename(g->obj_dir); if (!strcmp(g->filename, old_graph_name) &&
-- strcmp(g->odb->path, ctx->obj_dir)) {
-+ strcmp(g->odb->path, ctx->odb->path)) {
+- strcmp(g->obj_dir, ctx->obj_dir)) {
++ strcmp(g->obj_dir, ctx->odb->path)) {
ctx->num_commit_graphs_after = 1;
ctx->new_base_graph = NULL;
}
@@ commit-graph.c: int write_commit_graph(const char *obj_dir,
ctx->oids.alloc = split_opts->max_commits; if (ctx->append) {
-- struct object_directory *odb = find_odb(ctx->r, ctx->obj_dir);
-- prepare_commit_graph_one(ctx->r, odb);
-+ prepare_commit_graph_one(ctx->r, ctx->odb);
+- prepare_commit_graph_one(ctx->r, ctx->obj_dir);
++ prepare_commit_graph_one(ctx->r, ctx->odb->path);
if (ctx->r->objects->commit_graph)
ctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;
}
@@ commit-graph.c: int write_commit_graph(const char *obj_dir,
for (i = 0; i < ctx->num_commit_graphs_after; i++) { ## commit-graph.h ##
+@@
+ #include "repository.h"
+ #include "string-list.h"
+ #include "cache.h"
++#include "object-store.h"
+
+ #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+ #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
@@ commit-graph.h: struct split_commit_graph_opts {
* is not compatible with the commit-graph feature, then the
* methods will return 0 without writing a commit-graph.
2: 328884f5f6 ! 3: 5fd5cfca6e commit-graph.h: store object directory in 'struct commit_graph'
@@ Metadata
## Commit message ##
commit-graph.h: store object directory in 'struct commit_graph' - There are lots of places in 'commit-graph.h' where a function either has
- (or almost has) a full 'struct object_directory *', accesses '->path',
- and then throws away the rest of the struct.
+ In a previous patch, the 'char *object_dir' in 'struct commit_graph' was
+ replaced with a 'struct object_directory'. This patch applies the same
+ treatment to 'struct commit_graph', which is another intermediate step
+ towards getting rid of all path normalization in 'commit-graph.c'. - This can cause headaches when comparing the locations of object
- directories across alternates (e.g., in the case of deciding if two
- commit-graph layers can be merged). These paths are normalized with
- 'normalize_path_copy()' which mitigates some comparison issues, but not
- all [1].
+ Instead of taking a 'char *object_dir', functions that construct a
+ 'struct commit_graph' now take a 'struct object_directory *'. Any code
+ that needs an object directory path use '->path' instead. - Instead of getting rid of the 'struct object_directory *', store that
- insead of a 'char *odb' in 'struct commit_graph'. Once the 'struct
- write_commit_graph_context' has an object_directory pointer, too, this
- will allow calling code to replace these error-prone path comparisons
- with raw pointer comparisons, thereby circumventing any
- normalization-related errors. This will be introduced in a subsequent
- patch.
-
- [1]: In my testing, for example, I can get one side of the commit-graph
- code to fill object_dir with "./objects" and the other with just
- "objects".
+ This ensures that all calls to functions that perform path normalization
+ are given arguments which do not themselves require normalization. This
+ prepares those functions to drop their normalization entirely, which
+ will occur in the subsequent patch. Signed-off-by: Taylor Blau <me@ttaylorr.com>
## builtin/commit-graph.c ##
@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
-
if (open_ok)
graph = load_commit_graph_one_fd_st(fd, &st);
-- else
-- graph = read_commit_graph_one(the_repository, opts.obj_dir);
-+ else {
-+ struct object_directory *odb;
-+ if ((odb = find_odb(the_repository, opts.obj_dir)))
-+ graph = read_commit_graph_one(the_repository, odb);
-+ }
+ else
+- graph = read_commit_graph_one(the_repository, odb->path);
++ graph = read_commit_graph_one(the_repository, odb); /* Return failure if open_ok predicted success */
if (!graph)
-@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
- struct string_list lines;
- int result = 0;
- enum commit_graph_write_flags flags = 0;
-+ struct object_directory *odb = NULL;
-
- static struct option builtin_commit_graph_write_options[] = {
- OPT_STRING(0, "object-dir", &opts.obj_dir,
-@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
- flags |= COMMIT_GRAPH_WRITE_PROGRESS;
-
- read_replace_refs = 0;
-+ odb = find_odb(the_repository, opts.obj_dir);
-
- if (opts.reachable) {
-- if (write_commit_graph_reachable(opts.obj_dir, flags, &split_opts))
-+ if (write_commit_graph_reachable(odb->path, flags, &split_opts))
- return 1;
- return 0;
- }
-@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
- UNLEAK(buf);
- }
-
-- if (write_commit_graph(opts.obj_dir,
-+ if (write_commit_graph(odb->path,
- pack_indexes,
- commit_hex,
- flags,
-
- ## builtin/commit.c ##
-@@
- #include "help.h"
- #include "commit-reach.h"
- #include "commit-graph.h"
-+#include "object-store.h"
-
- static const char * const builtin_commit_usage[] = {
- N_("git commit [<options>] [--] <pathspec>..."), ## commit-graph.c ##
-@@ commit-graph.c: static uint8_t oid_version(void)
- return 1;
- }
-
-+struct object_directory *find_odb(struct repository *r, const char *obj_dir)
-+{
-+ struct object_directory *odb;
-+ char *obj_dir_real = real_pathdup(obj_dir, 1);
-+ int cmp = -1;
-+
-+ prepare_alt_odb(r);
-+ for (odb = r->objects->odb; odb; odb = odb->next) {
-+ cmp = strcmp(obj_dir_real, real_path(odb->path));
-+ if (!cmp)
-+ break;
-+ }
-+
-+ free(obj_dir_real);
-+
-+ if (cmp)
-+ odb = NULL;
-+ return odb;
-+}
-+
- static struct commit_graph *alloc_commit_graph(void)
- {
- struct commit_graph *g = xcalloc(1, sizeof(*g));
@@ commit-graph.c: static struct commit_graph *load_commit_graph_one(const char *graph_file)
return g;
}
@@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph while (g && (g->num_commits <= size_mult * num_commits ||
(max_commits && num_commits > max_commits))) {
-- if (strcmp(g->obj_dir, ctx->obj_dir))
-+ if (strcmp(g->odb->path, ctx->obj_dir))
+- if (strcmp(g->obj_dir, ctx->odb->path))
++ if (strcmp(g->odb->path, ctx->odb->path))
break; num_commits += g->num_commits;
@@ commit-graph.c: static void split_graph_merge_strategy(struct write_commit_graph
+ char *old_graph_name = get_commit_graph_filename(g->odb->path); if (!strcmp(g->filename, old_graph_name) &&
-- strcmp(g->obj_dir, ctx->obj_dir)) {
-+ strcmp(g->odb->path, ctx->obj_dir)) {
+- strcmp(g->obj_dir, ctx->odb->path)) {
++ strcmp(g->odb->path, ctx->odb->path)) {
ctx->num_commit_graphs_after = 1;
ctx->new_base_graph = NULL;
}
-@@ commit-graph.c: int write_commit_graph(const char *obj_dir,
+@@ commit-graph.c: int write_commit_graph(struct object_directory *odb,
ctx->oids.alloc = split_opts->max_commits; if (ctx->append) {
-- prepare_commit_graph_one(ctx->r, ctx->obj_dir);
-+ struct object_directory *odb = find_odb(ctx->r, ctx->obj_dir);
-+ prepare_commit_graph_one(ctx->r, odb);
+- prepare_commit_graph_one(ctx->r, ctx->odb->path);
++ prepare_commit_graph_one(ctx->r, ctx->odb);
if (ctx->r->objects->commit_graph)
ctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;
} ## commit-graph.h ##
-@@
- #include "repository.h"
- #include "string-list.h"
- #include "cache.h"
-+#include "object-store.h"
-
- #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
- #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
-@@ commit-graph.h: struct commit;
- char *get_commit_graph_filename(const char *obj_dir);
- int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
-
-+struct object_directory *find_odb(struct repository *r, const char *obj_dir);
-+
- /*
- * Given a commit struct, try to fill the commit struct info, including:
- * 1. tree object
@@ commit-graph.h: struct commit_graph {
uint32_t num_commits;
struct object_id oid;
3: ce884d7742 < -: ---------- builtin/commit-graph.c: die() with unknown '--object-dir'
5: c9b2ba46ab ! 4: f14e95aa7e commit-graph.c: remove path normalization, comparison
@@ Commit message
Signed-off-by: Taylor Blau <me@ttaylorr.com> ## builtin/commit-graph.c ##
-@@ builtin/commit-graph.c: static struct object_directory *find_odb_or_die(struct repository *r,
- static int graph_verify(int argc, const char **argv)
- {
- struct commit_graph *graph = NULL;
-+ struct object_directory *odb = NULL;
- char *graph_name;
- int open_ok;
- int fd;
@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
- if (opts.progress)
flags |= COMMIT_GRAPH_WRITE_PROGRESS; -- graph_name = get_commit_graph_filename(opts.obj_dir);
-+ odb = find_odb_or_die(the_repository, opts.obj_dir);
+ odb = find_odb(the_repository, opts.obj_dir);
+- graph_name = get_commit_graph_filename(odb->path);
+ graph_name = get_commit_graph_filename(odb);
open_ok = open_commit_graph(graph_name, &fd, &st);
if (!open_ok && errno != ENOENT)
die_errno(_("Could not open commit-graph '%s'"), graph_name);
-@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
-
- if (open_ok)
- graph = load_commit_graph_one_fd_st(fd, &st);
-- else {
-- struct object_directory *odb;
-- if ((odb = find_odb_or_die(the_repository, opts.obj_dir)))
-- graph = read_commit_graph_one(the_repository, odb);
-- }
-+ else
-+ graph = read_commit_graph_one(the_repository, odb);
-
- /* Return failure if open_ok predicted success */
- if (!graph) ## commit-graph.c ##
@@
@@ commit-graph.h
+char *get_commit_graph_filename(struct object_directory *odb);
int open_commit_graph(const char *graph_file, int *fd, struct stat *st); - struct object_directory *find_odb(struct repository *r, const char *obj_dir);
+ /* ## t/helper/test-read-graph.c ##
@@ t/helper/test-read-graph.c: int cmd__read_graph(int argc, const char **argv)
6: e70483f771 = 5: aa12b7378b commit-graph.h: use odb in 'load_commit_graph_one_fd_st'
--
2.25.0.119.gaa12b7378b
Taylor Blau
