Clarify commit-graph and grafts/replace/shallow incompatibilities

From: Stefan Beller <[email protected]>

Signed-off-by Stefan Beller <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/replace.c | 8 ++++----
 refs.c            | 9 ++++-----
 refs.h            | 2 +-
 replace-object.c  | 5 +++--
 4 files changed, 12 insertions(+), 12 deletions(-)

builtin/replace.c +4 -4

index ef22d724b..d0b1cdb06 100644
@@ -39,7 +39,8 @@ struct show_data {
 	enum replace_format format;
 };
 
-static int show_reference(const char *refname, const struct object_id *oid,
+static int show_reference(struct repository *r, const char *refname,
+			  const struct object_id *oid,
 			  int flag, void *cb_data)
 {
 	struct show_data *data = cb_data;

@@ -56,9 +57,8 @@ static int show_reference(const char *refname, const struct object_id *oid,
 			if (get_oid(refname, &object))
 				return error("Failed to resolve '%s' as a valid ref.", refname);
 
-			obj_type = oid_object_info(the_repository, &object,
-						   NULL);
-			repl_type = oid_object_info(the_repository, oid, NULL);
+			obj_type = oid_object_info(r, &object, NULL);
+			repl_type = oid_object_info(r, oid, NULL);
 
 			printf("%s (%s) -> %s (%s)\n", refname, type_name(obj_type),
 			       oid_to_hex(oid), type_name(repl_type));

refs.c +4 -5

index 2513f77ac..5700cd468 100644
@@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
 	return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_main_ref_store(r),
-			       git_replace_ref_base, fn,
-			       strlen(git_replace_ref_base),
-			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+	return do_for_each_repo_ref(r, git_replace_ref_base, fn,
+				    strlen(git_replace_ref_base),
+				    DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
 int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)

refs.h +0 -0

index 80eec8bbc..a0a18223a 100644

refs.h +0 -0

 int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 			 const char *prefix, void *cb_data);

replace-object.c +3 -2

index 801b5c167..017f02f8e 100644
@@ -6,7 +6,8 @@
 #include "repository.h"
 #include "commit.h"
 
-static int register_replace_ref(const char *refname,
+static int register_replace_ref(struct repository *r,
+				const char *refname,
 				const struct object_id *oid,
 				int flag, void *cb_data)
 {

@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,
 	oidcpy(&repl_obj->replacement, oid);
 
 	/* Register new object */
-	if (oidmap_put(the_repository->objects->replace_map, repl_obj))
+	if (oidmap_put(r->objects->replace_map, repl_obj))
 		die("duplicate replace ref: %s", refname);
 
 	return 0;

refs.h int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, · line 320

Quoted 6 lines

refs.h +1 -1

@@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
 int for_each_tag_ref(each_ref_fn fn, void *cb_data);
 int for_each_branch_ref(each_ref_fn fn, void *cb_data);
 int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);
+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);

With a signature change like this, any change that introduces new call to for_each_replace_ref() using eac_ref_fn() would get compilation error, so this is minimally correct.

Two things that bothersome are that

- for_each_tag/branch/remote_ref() and for_each_replace_ref() now work and look quite differently.

- existing users of for_each_replace_ref() who were all happy working in the_repository have to pass it explicitly, even thought they do not have any need to.

In this case, even if you introduced for_each_replace_ref_in_repo(), making for_each_replace_ref() a thin wrapper that always uses the_repository is a bit more cumbersome than just a simple macro.

But it *is* doable (you'd need to use a wrapping structure around cb_data), and a developer who case about maintainability during API transition would have taken pains to do so. A bit dissapointing.

Quoted 3 lines

- existing users of for_each_replace_ref() who were all happy working in the_repository have to pass it explicitly, even thought they do not have any need to.

All callbacks that are passed to for_each_replace_ref now operate on 'r' instead of the_repository, and that actually fixes a bug (commit message is lacking), but the cover letter hints at:
...

Quoted 3 lines

In this case, even if you introduced for_each_replace_ref_in_repo(), making for_each_replace_ref() a thin wrapper that always uses the_repository is a bit more cumbersome than just a simple macro.

Yes, but such a callback would do the *wrong* subtle thing in some cases as you really want to work in the correct repository for e.g. looking up commits.

Quoted 3 lines

But it *is* doable (you'd need to use a wrapping structure around cb_data), and a developer who case about maintainability during API transition would have taken pains to do so. A bit dissapointing.

My original patches were RFC-ish and a trade off as for the reflog only there is nothing in flight to care about.

Given that we would want to upgrade all the ref callbacks, we have to take this route, I think.

From: Derrick Stolee <[email protected]>

As it exists right now, the commit-graph feature may provide inconsistent results when combined with commit grafts, replace objects, and shallow clones. Update the design document to discuss why these interactions are difficult to reconcile and how we will avoid errors by preventing updates to and reads from the commit-graph file when these other features exist.

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/technical/commit-graph.txt | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Documentation/technical/commit-graph.txt +13 -3

+  `--no-replace-objects`. This leads to difficultly storing both possible
+  interpretations of a commit id, especially when computing generation
+  numbers. The commit-graph will not be read or written when
+  replace-objects or grafts are present.
+
+- Shallow clones create grafts of commits by dropping their parents. This
+  leads the commit-graph to think those commits have generation number 1.
+  If and when those commits are made unshallow, those generation numbers
+  become invalid. Since shallow clones are intended to restrict the commit
+  history to a very small set of commits, the commit-graph feature is less
+  helpful for these clones, anyway. The commit-graph will not be read or
+  written when shallow commits are present.
+
 Future Work
 -----------
 
-- The commit graph feature currently does not honor commit grafts. This can
-  be remedied by duplicating or refactoring the current graft logic.
-
 - After computing and storing generation numbers, we must make graph
   walks aware of generation numbers to gain the performance benefits they
   enable. This will mostly be accomplished by swapping a commit-date-ordered

From: Derrick Stolee <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 t/helper/test-repository.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

t/helper/test-repository.c +0 -0

index 2762ca656..6a84a53ef 100644

t/helper/test-repository.c +0 -0

 	c = lookup_commit(&r, commit_oid);

From: Derrick Stolee <[email protected]>

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist. Test this interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph.

Quoted 2 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Quoted 5 lines

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist. Test this interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph.

I think this approach is sensible. These are optimizations, and it's not a big deal to just punt no cases we can't handle.

Quoted 7 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist. Test this interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph.

I think this approach is sensible. These are optimizations, and it's not a big deal to just punt no cases we can't handle.

I do have one complaint about the implementation, though:

commit-graph.c static struct commit_graph *alloc_commit_graph(void) · line 68

Quoted 8 lines

commit-graph.c +8 -0

+static int commit_graph_compatible(struct repository *r)
+{
+	prepare_replace_object(r);
+	if (hashmap_get_size(&r->objects->replace_map->map))
+		return 0;
+
+	return 1;
+}

If I'm reading the code correctly, this will predicate the decision entirely on the presence of refs in refs/replace/. But you may have a situation where those refs exist, but you are not respecting them in this run.

For instance, imagine[1] a server that hosts arbitrary repositories, but wants to use commit graphs to speed up server-side operations (e.g., serving fetches, but also perhaps a web interface doing --contains, etc). If it runs all of the server-side commands with GIT_NO_REPLACE_OBJECTS=1, then there should be no problem. But if a user pushes anything to refs/replace (because they _do_ use replace refs locally, and want to share them with other clients), that would disable graphs on the server.

So I think this should at least be loosened to:

   if (check_replace_refs) {
	prepare_replace_object(r);
	if (...)
   }

which would make this case work. I'd even go so far as to say that for writing, we should just always ignore replace refs and generate the full graph (just like pack-objects does so when writing a pack). Then the resulting graph can be used selectively by disabling replace refs for particular commands. But for the scenario I described above, the distinction is mostly academic, as replacements would be disabled anyway during the write command anyway.

-Peff

[1] You can probably guess that this is how GitHub handles replace refs. We ran into this long ago because replacements and grafts mess up any other caches external to Git that rely on the immutability of the hash.

We do it with a config option, though, which requires a trivial patch. I'll post that momentarily.

Quoted 58 lines · Jeff King

Quoted 7 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist. Test this interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph.

I think this approach is sensible. These are optimizations, and it's not a big deal to just punt no cases we can't handle.

I do have one complaint about the implementation, though:

commit-graph.c static struct commit_graph *alloc_commit_graph(void) · line 68

Quoted 8 lines

commit-graph.c +8 -0

+static int commit_graph_compatible(struct repository *r)
+{
+    prepare_replace_object(r);
+    if (hashmap_get_size(&r->objects->replace_map->map))
+        return 0;
+
+    return 1;
+}

If I'm reading the code correctly, this will predicate the decision entirely on the presence of refs in refs/replace/. But you may have a situation where those refs exist, but you are not respecting them in this run.

For instance, imagine[1] a server that hosts arbitrary repositories, but wants to use commit graphs to speed up server-side operations (e.g., serving fetches, but also perhaps a web interface doing --contains, etc). If it runs all of the server-side commands with GIT_NO_REPLACE_OBJECTS=1, then there should be no problem. But if a user pushes anything to refs/replace (because they _do_ use replace refs locally, and want to share them with other clients), that would disable graphs on the server.

So I think this should at least be loosened to:

   if (check_replace_refs) {
    prepare_replace_object(r);
    if (...)
   }

which would make this case work. I'd even go so far as to say that for writing, we should just always ignore replace refs and generate the full graph (just like pack-objects does so when writing a pack). Then the resulting graph can be used selectively by disabling replace refs for particular commands. But for the scenario I described above, the distinction is mostly academic, as replacements would be disabled anyway during the write command anyway.

-Peff

[1] You can probably guess that this is how GitHub handles replace refs. We ran into this long ago because replacements and grafts mess up any other caches external to Git that rely on the immutability of the hash.

Quoted 2 lines

     We do it with a config option, though, which requires a trivial patch. I'll post that momentarily.

Thanks for the details! I never considered that someone would have these refs around, but would ignore them most of the time.

The biggest reason I wanted to punt here was that it is easy to toggle between using replace refs and not using them. Writing and reading as long as we are ignoring those refs is a good idea, and I'll use that approach in v2.

Thanks, Peff, for improving the check_replace_refs interaction [1].

Since this series now has two dependencies (including Stefan's ref-store fix [2] that I had included in my v1), I'll let those topics settle before I send a new v2.

Quoted 5 lines

Thanks, Peff, for improving the check_replace_refs interaction [1].

Since this series now has two dependencies (including Stefan's ref-store fix [2] that I had included in my v1), I'll let those topics settle before I send a new v2.

FYI: I have not been working on, nor plan to work on (in the short term) on the ref store fix series, which needs another abstraction layer IIUC to make it easier to integrate such a thing as well as extend the series to all functions in the refstore.

Feel free to take over that series partially (and defer the extension for all functions in the ref store until later) ?

From: Derrick Stolee <[email protected]>

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist.

Test this
interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph.

Signed-off-by Derrick Stolee <[email protected]>

---
 commit-graph.c          | 17 +++++++++++++++++
 replace-object.c        |  2 +-
 replace-object.h        |  2 ++
 t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
 4 files changed, 42 insertions(+), 1 deletion(-)

commit-graph.c +2 -0

index b0a55ad12..711099858 100644
@@ -13,6 +13,8 @@
 #include "commit-graph.h"
 #include "object-store.h"
 #include "alloc.h"
+#include "hashmap.h"
+#include "replace-object.h"
 
 #define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
 #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */

commit-graph.c +8 -0

+		return 0;
+
+	return 1;
+}
+
 struct commit_graph *load_commit_graph_one(const char *graph_file)
 {
 	void *graph_map;

@@ -223,6 +234,9 @@ static int prepare_commit_graph(struct repository *r)
 		 */
 		return 0;
 
+	if (!commit_graph_compatible(r))
+		return 0;
+
 	obj_dir = r->objects->objectdir;
 	prepare_commit_graph_one(r, obj_dir);
 	prepare_alt_odb(r);

commit-graph.c +0 -0

 	oids.nr = 0;
 	oids.alloc = approximate_object_count() / 4;
 

replace-object.c +1 -1

index 017f02f8e..4d2d84cf4 100644
@@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,
 	return 0;
 }
 
-static void prepare_replace_object(struct repository *r)
+void prepare_replace_object(struct repository *r)
 {
 	if (r->objects->replace_map)
 		return;

replace-object.h +0 -0

index f996de3d6..c7a99fc35 100644

replace-object.h +0 -0

 /*
  * This internal function is only declared here for the benefit of
  * lookup_replace_object().  Please do not call it directly.

t/t5318-commit-graph.sh +0 -0

index 4f17d7701..c90626f5d 100755
@@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '
 	test_cmp commit-graph-after-gc $objdir/info/commit-graph

t/t5318-commit-graph.sh +3 -0

+	)
+'
+
 # the verify tests below expect the commit-graph to contain
 # exactly the commits reachable from the commits/8 branch.
 # If the file changes the set of commits in the list, then the

From: Derrick Stolee <[email protected]>

Augment commit_graph_compatible(r) to return false when the given repository r has commit grafts or is a shallow clone. Test that in these situations we ignore existing commit-graph files and we do not write new commit-graph files.

Signed-off-by Derrick Stolee <[email protected]>

---
 commit-graph.c          |  6 ++++++
 commit.c                |  2 +-
 commit.h                |  1 +
 t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 1 deletion(-)

commit-graph.c +0 -0

index 711099858..5097c7c12 100644

commit-graph.c +1 -0

+
 	prepare_replace_object(r);
 	if (hashmap_get_size(&r->objects->replace_map->map))
 		return 0;

commit.c +1 -1

index 39b80bd21..609adaf8a 100644
@@ -209,7 +209,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
 	return 0;
 }
 
-static void prepare_commit_graft(struct repository *r)
+void prepare_commit_graft(struct repository *r)
 {
 	char *graft_file;
 

commit.h +0 -0

index da0db36eb..5459e279f 100644

commit.h +0 -0

 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);

t/t5318-commit-graph.sh +0 -0

index c90626f5d..1d9f49af5 100755
@@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '
 	)
 '

t/t5318-commit-graph.sh +3 -0

+	)
+'
+
 # the verify tests below expect the commit-graph to contain
 # exactly the commits reachable from the commits/8 branch.
 # If the file changes the set of commits in the list, then the

From: Derrick Stolee <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 commit-graph.c | 3 +++
 1 file changed, 3 insertions(+)

commit-graph.c +0 -0

index 5097c7c12..233958e10 100644

From: Derrick Stolee <[email protected]>

Make close_commit_graph() work for arbitrary repositories.

Call close_commit_graph() when about to start a rev-list walk that includes shallow commits. This is necessary in code paths that "fake" shallow commits for the sake of fetch. Specifically, test 351 in t5500-fetch-pack.sh runs

	git fetch --shallow-exclude one origin

with a file-based transfer. When the "remote" has a commit-graph, we do not prevent the commit-graph from being loaded, but then the commits are intended to be dynamically transferred into shallow commits during get_shallow_commits_by_rev_list(). By closing the commit-graph before this call, we prevent this interaction.

Signed-off-by Derrick Stolee <[email protected]>

---
 commit-graph.c | 8 ++++----
 commit-graph.h | 1 +
 upload-pack.c  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

commit-graph.c +4 -4

index 233958e10..237d4e7d1 100644
@@ -256,10 +256,10 @@ static int prepare_commit_graph(struct repository *r)
 	return !!r->objects->commit_graph;
 }
 
-static void close_commit_graph(void)
+void close_commit_graph(struct repository *r)
 {
-	free_commit_graph(the_repository->objects->commit_graph);
-	the_repository->objects->commit_graph = NULL;
+	free_commit_graph(r->objects->commit_graph);
+	r->objects->commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)

@@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,
 	write_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);
 	write_graph_chunk_large_edges(f, commits.list, commits.nr);
 
-	close_commit_graph();
+	close_commit_graph(the_repository);
 	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
 	commit_lock_file(&lk);
 

commit-graph.h +1 -0

index 76e098934..13d736cdd 100644
@@ -59,6 +59,7 @@ void write_commit_graph(const char *obj_dir,
 
 int verify_commit_graph(struct repository *r, struct commit_graph *g);
 
+void close_commit_graph(struct repository *);
 void free_commit_graph(struct commit_graph *);
 
 #endif

upload-pack.c +2 -0

index 4ca052d0b..52ad6c8e5 100644
@@ -24,6 +24,7 @@
 #include "quote.h"
 #include "upload-pack.h"
 #include "serve.h"
+#include "commit-graph.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)

@@ -739,6 +740,7 @@ static void deepen_by_rev_list(int ac, const char **av,
 {
 	struct commit_list *result;
 
+	close_commit_graph(the_repository);
 	result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
 	send_shallow(result);
 	free_commit_list(result);

This commit is not intended to be merged, but is only to create a test environment to see what works with the commit-graph feature and what does not. The tests that fail are primarily related to corrupting the object store to remove a commit from visibility and testing that rev-list fails -- except it doesn't when there is a commit-graph that prevents parsing from the object database. The following tests will fail with this commit, but are not "real" bugs:

t0410-partial-clone.sh, Test 9
t5307-pack-missing-commit.sh, Tests 3-4
t6011-rev-list-with-bad-commit.sh, Test 4

The following test fails because the repo has ambiguous merge-bases, and the commit-graph changes the walk order so we select a different one. This alters the resulting merge from the expected result.

t6024-recursive-merge.sh, Test 4

The tests above are made to pass by deleting the commit-graph file before the necessary steps.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/commit.c                    |  2 ++
 builtin/gc.c                        |  3 +--
 commit-graph.c                      | 11 -----------
 t/t0410-partial-clone.sh            |  1 +
 t/t5307-pack-missing-commit.sh      |  2 ++
 t/t6011-rev-list-with-bad-commit.sh |  1 +
 t/t6024-recursive-merge.sh          |  1 +
 7 files changed, 8 insertions(+), 13 deletions(-)

builtin/commit.c +1 -0

index 158e3f843a..acc31252a9 100644
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "mailmap.h"
 #include "help.h"
+#include "commit-graph.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),

commit-graph.c +0 -4

index 237d4e7d1b..ed0d27c12e 100644
@@ -227,22 +227,11 @@ static int prepare_commit_graph(struct repository *r)
 {
 	struct alternate_object_database *alt;
 	char *obj_dir;
-	int config_value;
 
 	if (r->objects->commit_graph_attempted)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
 
-	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
-	    !config_value)
-		/*

t/t0410-partial-clone.sh +1 -0

index 4984ca583d..c235672b03 100755
@@ -181,6 +181,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "arbitrary string" &&
+	rm -rf repo/.git/objects/info/commit-graph &&
 	git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
 	grep $(git -C repo rev-parse bar) out &&
 	! grep $FOO out

t/t5307-pack-missing-commit.sh +2 -0

index ae52a1882d..0bb54ae227 100755
@@ -24,10 +24,12 @@ test_expect_success 'check corruption' '
 '
 
 test_expect_success 'rev-list notices corruption (1)' '
+	rm -rf .git/objects/info/commit-graph &&
 	test_must_fail git rev-list HEAD
 '
 
 test_expect_success 'rev-list notices corruption (2)' '
+	rm -rf .git/objects/info/commit-graph &&
 	test_must_fail git rev-list --objects HEAD
 '
 

t/t6011-rev-list-with-bad-commit.sh +1 -0

index e51eb41f4b..9c9cc4d540 100755
@@ -43,6 +43,7 @@ test_expect_success 'corrupt second commit object' \
 
 test_expect_success 'rev-list should fail' \
    '
+   rm -rf .git/objects/info/commit-graph &&
    test_must_fail git rev-list --all > /dev/null
    '
 

t/t6024-recursive-merge.sh +0 -0

index 3f59e58dfb..cec10983cd 100755

I wonder though if all those changes to the testsuite shouldn't be merged.

Quoted 2 lines

I wonder though if all those changes to the testsuite shouldn't be merged.

I think Stolee doesn't want this to be merged after rereading subject and the commit message.

The following test fails because the repo has ambiguous merge-bases, and the commit-graph changes the walk order so we select a different one. This alters the resulting merge from the expected result.

t6024-recursive-merge.sh, Test 4

The tests above are made to pass by deleting the commit-graph file before the necessary steps.

t/t6024-recursive-merge.sh +1 -1

index 3f59e58dfb..cec10983cd 100755
@@ -61,6 +61,7 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 '
 
 test_expect_success "combined merge conflicts" "
+       rm -rf .git/objects/info/commit-graph &&
        test_must_fail git merge -m final G
 "
 
--

2.18.0.118.gd4f65b8d14

One unresolved issue with the commit-graph feature is that it can cause issues when combined with replace objects, commit grafts, or shallow clones. These are not 100% incompatible, as one could be reasonably successful writing a commit-graph after replacing some objects and not have issues. The problems happen when commits that are already in the commit-graph file are replaced, or when git is run with the --no-replace-objects option; this can cause incorrect parents or incorrect generation numbers. Similar things occur with commit grafts and shallow clones, especially when running git fetch --unshallow in a shallow repo.

Instead of trying (and probably failing) to make these features work together, default to making the commit-graph feature unavailable in these situations. Create a new method 'commit_graph_compatible(r)' that checks if the repository 'r' has any of these features enabled.

I will send a follow-up patch that shows how I tested these interactions by computing the commit-graph on every 'git commit'.

This approach is very different from the previous RFC on the subject [1].

Thanks,
-Stolee

[1] https://public-inbox.org/git/[email protected]/
[RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo

[2] https://public-inbox.org/git/[email protected]/T/#t
[PATCH 0/2] RFC ref store to repository migration

[3] https://public-inbox.org/git/[email protected]/T/#m966eac85fd58c66523654ddaf0bec72877d3295a
[PATCH] TO-SQUASH: replace the_repository with arbitrary r

Based-On: jt/commit-graph-per-object-store
Cc: [email protected]
Cc: [email protected]

Derrick Stolee (6):
commit-graph: update design document
test-repository: properly init repo
commit-graph: not compatible with replace objects
commit-graph: not compatible with grafts
commit-graph: not compatible with uninitialized repo
commit-graph: close_commit_graph before shallow walk

Stefan Beller (2):
refs.c: migrate internal ref iteration to pass thru repository
argument
refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

 Documentation/technical/commit-graph.txt | 18 ++++++--
 builtin/replace.c                        |  8 ++--
 commit-graph.c                           | 34 ++++++++++++--
 commit-graph.h                           |  1 +
 commit.c                                 |  2 +-
 commit.h                                 |  1 +
 refs.c                                   | 48 +++++++++++++++++---
 refs.h                                   | 12 ++++-
 refs/iterator.c                          |  6 +--
 refs/refs-internal.h                     |  5 +-
 replace-object.c                         |  7 +--
 replace-object.h                         |  2 +
 t/helper/test-repository.c               | 10 +++-
 t/t5318-commit-graph.sh                  | 58 ++++++++++++++++++++++++
 upload-pack.c                            |  2 +
 15 files changed, 184 insertions(+), 30 deletions(-)

base-commit: dade47c06cf849b0ca180a8e6383b55ea6f75812
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-11%2Fderrickstolee%2Fshallow%2Fupstream-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-11/derrickstolee/shallow/upstream-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/11

Sidenote: the GitGitGadget doesn't fold/wrap lines properly in the cover-letter. Not that it is much of a problem.

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist. Test this interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph. However, we do ensure that we write a new commit-graph by setting read_replace_refs to 0, thereby ignoring the replace refs.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/commit-graph.c  |  4 ++++
 commit-graph.c          | 21 +++++++++++++++++++++
 replace-object.c        |  2 +-
 replace-object.h        |  2 ++
 t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
 5 files changed, 50 insertions(+), 1 deletion(-)

builtin/commit-graph.c +0 -0

index 0bf0c48657..da737df321 100644

builtin/commit-graph.c +10 -0

+static int commit_graph_compatible(struct repository *r)
+{
+	if (read_replace_refs) {
+		prepare_replace_object(r);
+		if (hashmap_get_size(&r->objects->replace_map->map))
+			return 0;
+	}
+
+	return 1;
+}

replace-object.c +0 -0

index 3c17864eb7..9821f1477e 100644

Quoted 25 lines · Derrick Stolee

Create new method commit_graph_compatible(r) to check if a given repository r is compatible with the commit-graph feature. Fill the method with a check to see if replace-objects exist. Test this interaction succeeds, including ignoring an existing commit-graph and failing to write a new commit-graph. However, we do ensure that we write a new commit-graph by setting read_replace_refs to 0, thereby ignoring the replace refs.

Signed-off-by Derrick Stolee <[email protected]>

---
  builtin/commit-graph.c  |  4 ++++
  commit-graph.c          | 21 +++++++++++++++++++++
  replace-object.c        |  2 +-
  replace-object.h        |  2 ++
  t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
  5 files changed, 50 insertions(+), 1 deletion(-)

builtin/commit-graph.c +0 -0

index 0bf0c48657..da737df321 100644
@@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)
  	return 0;
  }
 

builtin/commit-graph.c static int graph_read(int argc, const char **argv) · line 124

Quoted 2 lines

builtin/commit-graph.c +2 -0

+extern int read_replace_refs;
+

Why do you need this (and also in commit-graph.c)? I thought cache.h includes it, which you can just make use of it.

Quoted 15 lines

builtin/commit-graph.c +10 -0

+static int commit_graph_compatible(struct repository *r)
+{
+	if (read_replace_refs) {
+		prepare_replace_object(r);
+		if (hashmap_get_size(&r->objects->replace_map->map))
+			return 0;
+	}
+
+	return 1;
+}

replace-object.c +0 -0

index 3c17864eb7..9821f1477e 100644

replace-object.c static int register_replace_ref(struct repository *r, · line 38

Quoted 8 lines

replace-object.c +1 -1

@@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,
  	return 0;
  }
 
-static void prepare_replace_object(struct repository *r)
+void prepare_replace_object(struct repository *r)
  {
  	if (r->objects->replace_map)
  		return;

The way the new caller is written, I am wondering if this function should return "we are (or, are not) using the replace object feature", making it unnecessary for callers on the reading side to know about "read-replace-refs" external variable, for example.

	/*
	 * To be called on-demand from codepaths that want to make
	 * sure that replacement objects can be found as necessary.
	 *
	 * Return number of replacement defined for the repository, or
	 * -1 when !read_replace_refs tells us not to use replacement
	 * mechanism at all.
	 */
	int prepare_replace_object(struct repository *r)
	{
		if (!read_replace_refs)
			return -1;

		if (!r->objects->replace_map) {
			r->objects->replace_map =
				xmalloc(...);
			oidmap_init(r->objects->replace_map, 0);
			for_each_refplace_ref(r, register_...);
		}
		return hashmap_get_size(&r->objects->replace_map->map);
	}

Then, the caller side can simply become something like:

	#define cgraph_compat(r) (prepare_replace_object(r) <= 0)

There are various writers to read_replace_refs variable, but I think they should first be replaced with calls to something like:

	void use_replace_refs(struct repository *r, int enable);

which allows us to hide the global variable and later make it per repository if we wanted to.

Because of the change of base, it is hard to provide a side-by-side diff from v1.

Quoted 2 lines · Derrick Stolee

Because of the change of base, it is hard to provide a side-by-side diff from v1.

I thought that was the whole point of range-diff. ;-)

Quoted 4 lines · Stefan Beller

Quoted 2 lines · Derrick Stolee

Because of the change of base, it is hard to provide a side-by-side diff from v1.

I thought that was the whole point of range-diff. ;-)

I thought so, too. Was there any change that confused range-diff machinery?

One unresolved issue with the commit-graph feature is that it can cause issues when combined with replace objects, commit grafts, or shallow clones. These are not 100% incompatible, as one could be reasonably successful writing a commit-graph after replacing some objects and not have issues. The problems happen when commits that are already in the commit-graph file are replaced, or when git is run with the --no-replace-objects option; this can cause incorrect parents or incorrect generation numbers. Similar things occur with commit grafts and shallow clones, especially when running git fetch --unshallow in a shallow repo.

Instead of trying (and probably failing) to make these features work together, default to making the commit-graph feature unavailable in these situations. Create a new method 'commit_graph_compatible(r)' that checks if the repository 'r' has any of these features enabled.

CHANGES IN V2:

* The first two commits regarding the ref iterators are unchanged, despite a lot of discussion on the subject [1].

* I included Peff's changes in jk/core-use-replace-refs, changing the base commit for the series to 1689c22c1c328e9135ed51458e9f9a5d224c5057 (the merge that brought that topic into 'msater').

* I fixed the tests for the interactions with the graft feature.

Because of the change of base, it is hard to provide a side-by-side diff from v1.

Thanks,
-Stolee

[1] https://public-inbox.org/git/CAGZ79kZ3PzqpGzXWcmxjzi98gA+LT2MBOf8KaA89hOa-Qig=Og@mail.gmail.com/
Stefan's response recommending we keep the first two commits.

Quoted 27 lines · Derrick Stolee

One unresolved issue with the commit-graph feature is that it can cause issues when combined with replace objects, commit grafts, or shallow clones. These are not 100% incompatible, as one could be reasonably successful writing a commit-graph after replacing some objects and not have issues. The problems happen when commits that are already in the commit-graph file are replaced, or when git is run with the --no-replace-objects option; this can cause incorrect parents or incorrect generation numbers. Similar things occur with commit grafts and shallow clones, especially when running git fetch --unshallow in a shallow repo.

Instead of trying (and probably failing) to make these features work together, default to making the commit-graph feature unavailable in these situations. Create a new method 'commit_graph_compatible(r)' that checks if the repository 'r' has any of these features enabled.

CHANGES IN V2:

* The first two commits regarding the ref iterators are unchanged, despite a lot of discussion on the subject [1].

* I included Peff's changes in jk/core-use-replace-refs, changing the base commit for the series to 1689c22c1c328e9135ed51458e9f9a5d224c5057 (the merge that brought that topic into 'msater').

* I fixed the tests for the interactions with the graft feature.

Quoted 9 lines

Because of the change of base, it is hard to provide a side-by-side diff from v1.

Thanks,
-Stolee

[1] https://public-inbox.org/git/CAGZ79kZ3PzqpGzXWcmxjzi98gA+LT2MBOf8KaA89hOa-Qig=Og@mail.gmail.com/
Stefan's response recommending we keep the first two commits.

After reviewing my own patches, I flipped again (Sorry!) and would rather not see my patches be merged, but the very original solution by you, that you proposed at [1]. That said, I will not insist on it, and if this is merged as is, we can fix it up later.

With that said, I just read through the remaining patches, I think they are a valuable addition to Git and could be merged as-is.

[1] https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1

Thanks,
Stefan