[PATCH v2 5/8] commit-graph: not compatible with replace objects
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
---
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(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 0bf0c48657..da737df321 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)
return 0;
}
+extern int read_replace_refs;
+
static int graph_write(int argc, const char **argv)
{
struct string_list *pack_indexes = NULL;
@@ -150,6 +152,8 @@ static int graph_write(int argc, const char **argv)
if (!opts.obj_dir)
opts.obj_dir = get_object_directory();
+ read_replace_refs = 0;
+
if (opts.reachable) {
write_commit_graph_reachable(opts.obj_dir, opts.append);
return 0;
diff --git a/commit-graph.c b/commit-graph.c
index b0a55ad128..2c01fa433f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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" */
@@ -56,6 +58,19 @@ static struct commit_graph *alloc_commit_graph(void)
return g;
}
+extern int read_replace_refs;
+
+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;
+}
+
struct commit_graph *load_commit_graph_one(const char *graph_file)
{
void *graph_map;
@@ -223,6 +238,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);
@@ -693,6 +711,9 @@ void write_commit_graph(const char *obj_dir,
int num_extra_edges;
struct commit_list *parent;
+ if (!commit_graph_compatible(the_repository))
+ return;
+
oids.nr = 0;
oids.alloc = approximate_object_count() / 4;
diff --git a/replace-object.c b/replace-object.c
index 3c17864eb7..9821f1477e 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -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;
diff --git a/replace-object.h b/replace-object.h
index 9345e105dd..16528df942 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -10,6 +10,8 @@ struct replace_object {
struct object_id replacement;
};
+void prepare_replace_object(struct repository *r);
+
/*
* This internal function is only declared here for the benefit of
* lookup_replace_object(). Please do not call it directly.
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 4f17d7701e..e0c3c60f66 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '
test_cmp commit-graph-after-gc $objdir/info/commit-graph
'
+test_expect_success 'replace-objects invalidates commit-graph' '
+ cd "$TRASH_DIRECTORY" &&
+ test_when_finished rm -rf replace &&
+ git clone full replace &&
+ (
+ cd replace &&
+ git commit-graph write --reachable &&
+ test_path_is_file .git/objects/info/commit-graph &&
+ git replace HEAD~1 HEAD~2 &&
+ git -c core.commitGraph=false log >expect &&
+ git -c core.commitGraph=true log >actual &&
+ test_cmp expect actual &&
+ git commit-graph write --reachable &&
+ git -c core.commitGraph=false --no-replace-objects log >expect
&&
+ git -c core.commitGraph=true --no-replace-objects log >actual &&
+ test_cmp expect actual &&
+ rm -rf .git/objects/info/commit-graph &&
+ git commit-graph write --reachable &&
+ test_path_is_file .git/objects/info/commit-graph
+ )
Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects
Derrick Stolee writes:
> 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
> ---
> 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(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 0bf0c48657..da737df321 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)
> return 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.
> +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;
> +}
> diff --git a/replace-object.c b/replace-object.c
> index 3c17864eb7..9821f1477e 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -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.
Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects
On 8/21/2018 1:45 PM, Junio C Hamano wrote:
Derrick Stolee writes:
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
---
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(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 0bf0c48657..da737df321 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)
return 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.
You're right. I don't know how I missed that.
+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;
+}
diff --git a/replace-object.c b/replace-object.c
index 3c17864eb7..9821f1477e 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -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);
}
This is a good idea. I can incorporate it into v3.
Then, the caller side can simply become something like:
#define cgraph_compat(r) (prepare_replace_object(r) <= 0)
With the next two patches that add more conditions to
commit_graph_compatible(), I'd prefer keeping it a method instead of a
macro.
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.
I will incorporate this into v3 as well.
Thanks,
-Stolee
