All of lore.kernel.org
 help / color / mirror / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: peff@peff.net, dstolee@microsoft.com, gitster@pobox.com
Subject: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'
Date: Thu, 30 Jan 2020 15:00:50 -0800	[thread overview]
Message-ID: <890e0e7136204f5ca47f0703f32b4adb99ad8d7e.1580424766.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1580424766.git.me@ttaylorr.com>

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

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

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/commit-graph.c | 13 +++++++---
 builtin/commit.c       |  1 +
 commit-graph.c         | 59 ++++++++++++++++++++++++++++++------------
 commit-graph.h         |  8 ++++--
 4 files changed, 58 insertions(+), 23 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index e0c6fc4bbf..3edac318e8 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -76,8 +76,11 @@ 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);
+	}
 
 	/* Return failure if open_ok predicted success */
 	if (!graph)
@@ -97,6 +100,7 @@ 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,
@@ -145,9 +149,10 @@ 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;
 	}
@@ -169,7 +174,7 @@ 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,
diff --git a/builtin/commit.c b/builtin/commit.c
index aa1332308a..bd071169d7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -36,6 +36,7 @@
 #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>..."),
diff --git a/commit-graph.c b/commit-graph.c
index b205e65ed1..2c06876b26 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -75,6 +75,26 @@ 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));
@@ -327,14 +347,15 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)
 	return g;
 }
 
-static struct commit_graph *load_commit_graph_v1(struct repository *r, const char *obj_dir)
+static struct commit_graph *load_commit_graph_v1(struct repository *r,
+						 struct object_directory *odb)
 {
-	char *graph_name = get_commit_graph_filename(obj_dir);
+	char *graph_name = get_commit_graph_filename(odb->path);
 	struct commit_graph *g = load_commit_graph_one(graph_name);
 	free(graph_name);
 
 	if (g)
-		g->obj_dir = obj_dir;
+		g->odb = odb;
 
 	return g;
 }
@@ -372,14 +393,15 @@ static int add_graph_to_chain(struct commit_graph *g,
 	return 1;
 }
 
-static struct commit_graph *load_commit_graph_chain(struct repository *r, const char *obj_dir)
+static struct commit_graph *load_commit_graph_chain(struct repository *r,
+						    struct object_directory *odb)
 {
 	struct commit_graph *graph_chain = NULL;
 	struct strbuf line = STRBUF_INIT;
 	struct stat st;
 	struct object_id *oids;
 	int i = 0, valid = 1, count;
-	char *chain_name = get_chain_filename(obj_dir);
+	char *chain_name = get_chain_filename(odb->path);
 	FILE *fp;
 	int stat_res;
 
@@ -418,7 +440,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, const
 			free(graph_name);
 
 			if (g) {
-				g->obj_dir = odb->path;
+				g->odb = odb;
 
 				if (add_graph_to_chain(g, graph_chain, oids, i)) {
 					graph_chain = g;
@@ -442,23 +464,25 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r, const
 	return graph_chain;
 }
 
-struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir)
+struct commit_graph *read_commit_graph_one(struct repository *r,
+					   struct object_directory *odb)
 {
-	struct commit_graph *g = load_commit_graph_v1(r, obj_dir);
+	struct commit_graph *g = load_commit_graph_v1(r, odb);
 
 	if (!g)
-		g = load_commit_graph_chain(r, obj_dir);
+		g = load_commit_graph_chain(r, odb);
 
 	return g;
 }
 
-static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
+static void prepare_commit_graph_one(struct repository *r,
+				     struct object_directory *odb)
 {
 
 	if (r->objects->commit_graph)
 		return;
 
-	r->objects->commit_graph = read_commit_graph_one(r, obj_dir);
+	r->objects->commit_graph = read_commit_graph_one(r, odb);
 }
 
 /*
@@ -505,7 +529,7 @@ static int prepare_commit_graph(struct repository *r)
 	for (odb = r->objects->odb;
 	     !r->objects->commit_graph && odb;
 	     odb = odb->next)
-		prepare_commit_graph_one(r, odb->path);
+		prepare_commit_graph_one(r, odb);
 	return !!r->objects->commit_graph;
 }
 
@@ -1470,7 +1494,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 	if (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {
 		char *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));
-		char *new_base_name = get_split_graph_filename(ctx->new_base_graph->obj_dir, new_base_hash);
+		char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);
 
 		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);
 		free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);
@@ -1553,7 +1577,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 
 	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))
 			break;
 
 		num_commits += g->num_commits;
@@ -1565,10 +1589,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 	ctx->new_base_graph = g;
 
 	if (ctx->num_commit_graphs_after == 2) {
-		char *old_graph_name = get_commit_graph_filename(g->obj_dir);
+		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)) {
 			ctx->num_commit_graphs_after = 1;
 			ctx->new_base_graph = NULL;
 		}
@@ -1824,7 +1848,8 @@ int write_commit_graph(const char *obj_dir,
 		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);
 		if (ctx->r->objects->commit_graph)
 			ctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;
 	}
diff --git a/commit-graph.h b/commit-graph.h
index 7f5c933fa2..9700a6c7c2 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -5,6 +5,7 @@
 #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"
@@ -14,6 +15,8 @@ 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
@@ -48,7 +51,7 @@ struct commit_graph {
 	uint32_t num_commits;
 	struct object_id oid;
 	char *filename;
-	const char *obj_dir;
+	struct object_directory *odb;
 
 	uint32_t num_commits_in_base;
 	struct commit_graph *base_graph;
@@ -61,7 +64,8 @@ struct commit_graph {
 };
 
 struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);
-struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir);
+struct commit_graph *read_commit_graph_one(struct repository *r,
+					   struct object_directory *odb);
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);
 
-- 
2.25.0.dirty


  parent reply	other threads:[~2020-01-30 23:00 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-30 23:00 [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere Taylor Blau
2020-01-30 23:00 ` [PATCH 1/6] t5318: don't pass non-object directory to '--object-dir' Taylor Blau
2020-01-30 23:00 ` [PATCH 5/6] commit-graph.c: remove path normalization, comparison Taylor Blau
2020-01-30 23:00 ` [PATCH 6/6] commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Taylor Blau
2020-01-30 23:00 ` Taylor Blau [this message]
2020-01-31  6:52   ` [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph' Martin Ågren
2020-01-31 10:20     ` Jeff King
2020-01-31 19:19       ` Martin Ågren
2020-02-03  4:36       ` Taylor Blau
2020-02-03  8:36         ` Jeff King
2020-01-31 20:49     ` Junio C Hamano
2020-02-03  3:58     ` Taylor Blau
2020-01-30 23:00 ` [PATCH 4/6] commit-graph.h: store an odb in 'struct write_commit_graph_context' Taylor Blau
2020-01-30 23:00 ` [PATCH 3/6] builtin/commit-graph.c: die() with unknown '--object-dir' Taylor Blau
2020-01-31 10:30 ` [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere Jeff King
2020-01-31 13:22   ` Derrick Stolee
2020-02-03  4:38     ` Taylor Blau
2020-02-03 21:17 ` [PATCH v2 0/5] " Taylor Blau
2020-02-03 21:17   ` [PATCH v2 1/5] t5318: don't pass non-object directory to '--object-dir' Taylor Blau
2020-02-03 21:17   ` [PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context' Taylor Blau
2020-02-04  5:51     ` Taylor Blau
2020-02-04 19:54       ` Junio C Hamano
2020-02-04 21:28         ` Taylor Blau
2020-02-04 21:44           ` Junio C Hamano
2020-02-03 21:18   ` [PATCH v2 3/5] commit-graph.h: store object directory in 'struct commit_graph' Taylor Blau
2020-02-03 21:18   ` [PATCH v2 4/5] commit-graph.c: remove path normalization, comparison Taylor Blau
2020-02-03 21:18   ` [PATCH v2 5/5] commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Taylor Blau
2020-02-05 12:30   ` [PATCH v2 0/5] commit-graph: use 'struct object_directory *' everywhere Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=890e0e7136204f5ca47f0703f32b4adb99ad8d7e.1580424766.git.me@ttaylorr.com \
    --to=me@ttaylorr.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.