git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH on master v2] revision: use commit graph in get_reference()

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 9, 2018, 01:49 UTC
Message-ID
<xmqqbm5v5v0f.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<xmqqwooj5xpr.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 23 quoted lines
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> When fetching into a repository, a connectivity check is first made by
>> check_exist_and_connected() in builtin/fetch.c that runs:
>> ...
>> Another way to accomplish this effect would be to modify parse_object()
>> to use the commit graph if possible; however, I did not want to change
>> parse_object()'s current behavior of always checking the object
>> signature of the returned object.
>
> Sounds good.
>
>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>> ---
>> This patch is now on master.
>
> OK.  
>
> Obviously that won't apply to the base for v1 without conflicts, and
> it of course applies cleanly on 'master', but the result of doing so
> will cause the same conflicts when sb/more-repo-in-api is merged on
> top, which means that the same conflicts need to be resolved if this
> wants to be merged to 'next' (or 'pu', FWIW).

So,... as I had to do the reverse rebase anyway, here is the difference since the previous round, which I came up with by comparing these two:

 (A) merge 'sb/more-repo-in-api' to 'master' and then merge v1 of
     this topic to the result.
 (B) apply your patch to 'master', and then merge
     'sb/more-repo-in-api' to the result.
diff --git a/commit-graph.c b/commit-graph.c
index f78a8e96b5..74a17789f8 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -286,7 +286,8 @@ void close_commit_graph(struct repository *r)
 	r->objects->commit_graph = NULL;
 }
 
-static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
+static int bsearch_graph(struct commit_graph *g, const struct object_id *oid,
+			 uint32_t *pos)
 {
 	return bsearch_hash(oid->hash, g->chunk_oid_fanout,
 			    g->chunk_oid_lookup, g->hash_len, pos);
@@ -377,26 +378,41 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin
 	}
 }
 
-static int parse_commit_in_graph_one(struct repository *r,
-				     struct commit_graph *g,
-				     struct commit *item)
+static struct commit *parse_commit_in_graph_one(struct repository *r,
+						struct commit_graph *g,
+						struct commit *shell,
+						const struct object_id *oid)
 {
 	uint32_t pos;
 
-	if (item->object.parsed)
-		return 1;
+	if (shell && shell->object.parsed)
+		return shell;
 
-	if (find_commit_in_graph(item, g, &pos))
-		return fill_commit_in_graph(r, item, g, pos);
+	if (shell && shell->graph_pos != COMMIT_NOT_FROM_GRAPH) {
+		pos = shell->graph_pos;
+	} else if (bsearch_graph(g, shell ? &shell->object.oid : oid, &pos)) {
+		/* bsearch_graph sets pos */
+	} else {
+		return NULL;
+	}
 
-	return 0;
+	if (!shell) {
+		shell = lookup_commit(r, oid);
+		if (!shell)
+			return NULL;
+	}
+
+	fill_commit_in_graph(r, shell, g, pos);
+	return shell;
 }
 
-int parse_commit_in_graph(struct repository *r, struct commit *item)
+struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,
+				     const struct object_id *oid)
 {
 	if (!prepare_commit_graph(r))
 		return 0;
-	return parse_commit_in_graph_one(r, r->objects->commit_graph, item);
+	return parse_commit_in_graph_one(r, r->objects->commit_graph, shell,
+					 oid);
 }
 
 void load_commit_graph_info(struct repository *r, struct commit *item)
@@ -1033,7 +1049,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		}
 
 		graph_commit = lookup_commit(r, &cur_oid);
-		if (!parse_commit_in_graph_one(r, g, graph_commit))
+		if (!parse_commit_in_graph_one(r, g, graph_commit, NULL))
 			graph_report("failed to parse %s from commit-graph",
 				     oid_to_hex(&cur_oid));
 	}
diff --git a/commit-graph.h b/commit-graph.h
index 9db40b4d3a..8b7b5985dc 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -13,16 +13,20 @@ struct commit;
 char *get_commit_graph_filename(const char *obj_dir);
 
 /*
- * Given a commit struct, try to fill the commit struct info, including:
+ * If the given commit (identified by shell->object.oid or oid) is in the
+ * commit graph, returns a commit struct (reusing shell if it is not NULL)
+ * including the following info:
  *  1. tree object
  *  2. date
  *  3. parents.
  *
- * Returns 1 if and only if the commit was found in the packed graph.
+ * If not, returns NULL. See parse_commit_buffer() for the fallback after this
+ * call.
  *
- * See parse_commit_buffer() for the fallback after this call.
+ * Either shell or oid must be non-NULL. If both are non-NULL, oid is ignored.
  */
-int parse_commit_in_graph(struct repository *r, struct commit *item);
+struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,
+				     const struct object_id *oid);
 
 /*
  * It is possible that we loaded commit contents from the commit buffer,
diff --git a/commit.c b/commit.c
index a5333c7ac6..da7a1d3262 100644
--- a/commit.c
+++ b/commit.c
@@ -462,7 +462,7 @@ int repo_parse_commit_internal(struct repository *r,
 		return -1;
 	if (item->object.parsed)
 		return 0;
-	if (use_commit_graph && parse_commit_in_graph(r, item))
+	if (use_commit_graph && parse_commit_in_graph(r, item, NULL))
 		return 0;
 	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
 	if (!buffer)
diff --git a/revision.c b/revision.c
index 22aa109c14..05fddb5880 100644
--- a/revision.c
+++ b/revision.c
@@ -213,19 +213,9 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
 {
 	struct object *object;
 
-	/*
-	 * If the repository has commit graphs, repo_parse_commit() avoids
-	 * reading the object buffer, so use it whenever possible.
-	 */
-	if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {
-		struct commit *c = lookup_commit(revs->repo, oid);
-		if (!repo_parse_commit(revs->repo, c))
-			object = (struct object *) c;
-		else
-			object = NULL;
-	} else {
+	object = (struct object *) parse_commit_in_graph(revs->repo, NULL, oid);
+	if (!object)
 		object = parse_object(revs->repo, oid);
-	}
 
 	if (!object) {
 		if (revs->ignore_missing)
diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
index f7f8618445..689a0b652e 100644
--- a/t/helper/test-repository.c
+++ b/t/helper/test-repository.c
@@ -27,7 +27,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
 
 	c = lookup_commit(&r, commit_oid);
 
-	if (!parse_commit_in_graph(&r, c))
+	if (!parse_commit_in_graph(&r, c, NULL))
 		die("Couldn't parse commit");
 
 	printf("%"PRItime, c->date);
@@ -62,7 +62,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,
 	 * get_commit_tree_in_graph does not automatically parse the commit, so
 	 * parse it first.
 	 */
-	if (!parse_commit_in_graph(&r, c))
+	if (!parse_commit_in_graph(&r, c, NULL))
 		die("Couldn't parse commit");
 	tree = get_commit_tree_in_graph(&r, c);
 	if (!tree)
Previous: Junio C HamanoNext: Jeff King
Message 11 of 28 in “revision: use commit graph in get_reference()”
  1. revision: use commit graph in get_reference()Jonathan Tan, Dec 4, 2018
  2. Stefan BellerDec 4, 2018
  3. Jonathan TanDec 6, 2018
  4. Derrick StoleeDec 7, 2018
  5. Jeff KingDec 5, 2018
  6. Jonathan TanDec 6, 2018
  7. Jeff KingDec 7, 2018
  8. Junio C HamanoDec 5, 2018
  9. revision: use commit graph in get_reference()Jonathan Tan, Dec 7, 2018
  10. Junio C HamanoDec 9, 2018
  11. Junio C HamanoDec 9, 2018
  12. Jeff KingDec 11, 2018
  13. Jonathan TanDec 12, 2018
  14. Jeff KingDec 13, 2018
  15. Derrick StoleeDec 13, 2018
  16. revision: use commit graph in get_reference()Jonathan Tan, Dec 13, 2018
  17. Junio C HamanoDec 14, 2018
  18. Jeff KingDec 14, 2018
  19. Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()SZEDER Gábor, Jan 25, 2019
  20. Stefan BellerJan 25, 2019
  21. Jonathan TanJan 25, 2019
  22. SZEDER GáborJan 25, 2019
  23. SZEDER GáborJan 25, 2019
  24. object_as_type: initialize commit-graph-related fields of 'struct commit'SZEDER Gábor, Jan 27, 2019
  25. SZEDER GáborJan 27, 2019
  26. Derrick StoleeJan 27, 2019
  27. Jonathan TanJan 28, 2019
  28. Jeff KingJan 28, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.