[PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Derrick Stolee via GitGitGadget]

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 works for most cases, but I found one nagging test case that was causing problems. This led to the commit "commit-graph: close_commit_graph before shallow walk" and is the patch I am least confident about. Please take a close look at that one and suggest alternatives.

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

While building this series, I got some test failures in the non-the_repository tests. These issues are related to missing references to an arbitrary repository (instead of the_repository) and some uninitialized values in the tests. Stefan already sent a patch to address this [2], and I've included those commits (along with a small tweak [3]). These are only included for convenience.

Thanks,
-Stolee

[1] https://public-inbox.org/git/20180531174024.124488-1-dstolee@microsoft.com/
     [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo

[2] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#t
    [PATCH 0/2] RFC ref store to repository migration

[3] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#m966eac85fd58c66523654ddaf0bec72877d3295a
    [PATCH] TO-SQUASH: replace the_repository with arbitrary r

Based-On: jt/commit-graph-per-object-store
Cc: jonathantanmy@google.com
Cc: sbeller@google.com

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
-- 
gitgitgadget

[PATCH 1/8] refs.c: migrate internal ref iteration to pass thru repository argument

[Stefan Beller via GitGitGadget]

From: Stefan Beller <sbeller@google.com>

In 60ce76d3581 (refs: add repository argument to for_each_replace_ref,
2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle
arbitrary repositories, 2018-04-11), for_each_replace_ref learned how
to iterate over refs by a given arbitrary repository.
New attempts in the object store conversion have shown that it is useful
to have the repository handle available that the refs iteration is
currently iterating over.

To achieve this goal we will need to add a repository argument to
each_ref_fn in refs.h. However as many callers rely on the signature
such a patch would be too large.

So convert the internals of the ref subsystem first to pass through a
repository argument without exposing the change to the user. Assume
the_repository for the passed through repository, although it is not
used anywhere yet.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 refs.c               | 39 +++++++++++++++++++++++++++++++++++++--
 refs.h               | 10 ++++++++++
 refs/iterator.c      |  6 +++---
 refs/refs-internal.h |  5 +++--
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index fcfd3171e..2513f77ac 100644
--- a/refs.c
+++ b/refs.c
@@ -1390,17 +1390,50 @@ struct ref_iterator *refs_ref_iterator_begin(
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
  */
+static int do_for_each_repo_ref(struct repository *r, const char *prefix,
+				each_repo_ref_fn fn, int trim, int flags,
+				void *cb_data)
+{
+	struct ref_iterator *iter;
+	struct ref_store *refs = get_main_ref_store(r);
+
+	if (!refs)
+		return 0;
+
+	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
+
+	return do_for_each_repo_ref_iterator(r, iter, fn, cb_data);
+}
+
+struct do_for_each_ref_help {
+	each_ref_fn *fn;
+	void *cb_data;
+};
+
+static int do_for_each_ref_helper(struct repository *r,
+				  const char *refname,
+				  const struct object_id *oid,
+				  int flags,
+				  void *cb_data)
+{
+	struct do_for_each_ref_help *hp = cb_data;
+
+	return hp->fn(refname, oid, flags, hp->cb_data);
+}
+
 static int do_for_each_ref(struct ref_store *refs, const char *prefix,
 			   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
 	struct ref_iterator *iter;
+	struct do_for_each_ref_help hp = { fn, cb_data };
 
 	if (!refs)
 		return 0;
 
 	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
 
-	return do_for_each_ref_iterator(iter, fn, cb_data);
+	return do_for_each_repo_ref_iterator(the_repository, iter,
+					do_for_each_ref_helper, &hp);
 }
 
 int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
@@ -2029,10 +2062,12 @@ cleanup:
 int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	struct ref_iterator *iter;
+	struct do_for_each_ref_help hp = { fn, cb_data };
 
 	iter = refs->be->reflog_iterator_begin(refs);
 
-	return do_for_each_ref_iterator(iter, fn, cb_data);
+	return do_for_each_repo_ref_iterator(the_repository, iter,
+					     do_for_each_ref_helper, &hp);
 }
 
 int for_each_reflog(each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index cc2fb4c68..80eec8bbc 100644
--- a/refs.h
+++ b/refs.h
@@ -274,6 +274,16 @@ struct ref_transaction;
 typedef int each_ref_fn(const char *refname,
 			const struct object_id *oid, int flags, void *cb_data);
 
+/*
+ * The same as each_ref_fn, but also with a repository argument that
+ * contains the repository associated with the callback.
+ */
+typedef int each_repo_ref_fn(struct repository *r,
+			     const char *refname,
+			     const struct object_id *oid,
+			     int flags,
+			     void *cb_data);
+
 /*
  * The following functions invoke the specified callback function for
  * each reference indicated.  If the function ever returns a nonzero
diff --git a/refs/iterator.c b/refs/iterator.c
index 2ac91ac34..629e00a12 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
 
 struct ref_iterator *current_ref_iter = NULL;
 
-int do_for_each_ref_iterator(struct ref_iterator *iter,
-			     each_ref_fn fn, void *cb_data)
+int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter,
+				  each_repo_ref_fn fn, void *cb_data)
 {
 	int retval = 0, ok;
 	struct ref_iterator *old_ref_iter = current_ref_iter;
 
 	current_ref_iter = iter;
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
-		retval = fn(iter->refname, iter->oid, iter->flags, cb_data);
+		retval = fn(r, iter->refname, iter->oid, iter->flags, cb_data);
 		if (retval) {
 			/*
 			 * If ref_iterator_abort() returns ITER_ERROR,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314b..5c7414bf0 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -473,8 +473,9 @@ extern struct ref_iterator *current_ref_iter;
  * adapter between the callback style of reference iteration and the
  * iterator style.
  */
-int do_for_each_ref_iterator(struct ref_iterator *iter,
-			     each_ref_fn fn, void *cb_data);
+int do_for_each_repo_ref_iterator(struct repository *r,
+				  struct ref_iterator *iter,
+				  each_repo_ref_fn fn, void *cb_data);
 
 /*
  * Only include per-worktree refs in a do_for_each_ref*() iteration.
-- 
gitgitgadget

[PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

[Stefan Beller via GitGitGadget]

From: Stefan Beller <sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/replace.c | 8 ++++----
 refs.c            | 9 ++++-----
 refs.h            | 2 +-
 replace-object.c  | 5 +++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index ef22d724b..d0b1cdb06 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -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));
diff --git a/refs.c b/refs.c
index 2513f77ac..5700cd468 100644
--- a/refs.c
+++ b/refs.c
@@ -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)
diff --git a/refs.h b/refs.h
index 80eec8bbc..a0a18223a 100644
--- a/refs.h
+++ b/refs.h
@@ -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);
 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);
diff --git a/replace-object.c b/replace-object.c
index 801b5c167..017f02f8e 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -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;
-- 
gitgitgadget

Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

[Junio C Hamano]

"Stefan Beller via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Stefan Beller <sbeller@google.com>
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/replace.c | 8 ++++----
>  refs.c            | 9 ++++-----
>  refs.h            | 2 +-
>  replace-object.c  | 5 +++--
>  4 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index ef22d724b..d0b1cdb06 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -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));
> diff --git a/refs.c b/refs.c
> index 2513f77ac..5700cd468 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -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)
> diff --git a/refs.h b/refs.h
> index 80eec8bbc..a0a18223a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -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.

>  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);
> diff --git a/replace-object.c b/replace-object.c
> index 801b5c167..017f02f8e 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -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;

Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

[Stefan Beller]

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

Yes, this series is not finished; we need to convert/upgrade for_each_tag
et al, too.

>  - 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:

    While building this series, I got some test failures in the
    non-the_repository tests. These issues are related to missing
    references to an arbitrary repository (instead of the_repository)
    and some uninitialized values in the tests. Stefan already sent
    a patch to address this [2], and I've included those commits
    (along with a small tweak [3]). These are only included for
    convenience.

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

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

Thanks,
Stefan

Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Try to rebuild 'pu' on top of 'master' yoruself to realize how many
in-flight topics you are hurting and causing unnecessary load on the
maintainer with the "let's add the 'r' parameter without changing
the function names; compiler would catch and cause breakages"
approach.  

Until that happens, I won't waste any more time trying to educate
you on this further.

[PATCH 3/8] commit-graph: update design document

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 Documentation/technical/commit-graph.txt | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt
index c664acbd7..18948f11a 100644
--- a/Documentation/technical/commit-graph.txt
+++ b/Documentation/technical/commit-graph.txt
@@ -112,12 +112,24 @@ Design Details
 - The file format includes parameters for the object ID hash function,
   so a future change of hash algorithm does not require a change in format.
 
+- Commit grafts and replace objects can change the shape of the commit
+  history. These can also be enabled/disabled on the fly using
+  `--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
-- 
gitgitgadget

Re: [PATCH 3/8] commit-graph: update design document

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  Documentation/technical/commit-graph.txt | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

It would be nice to have this information not only in the technical
documentation, but also in user-facing documentation, for example
git-commit-graph manpage.

>
> diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt
> index c664acbd7..18948f11a 100644
> --- a/Documentation/technical/commit-graph.txt
> +++ b/Documentation/technical/commit-graph.txt
> @@ -112,12 +112,24 @@ Design Details
>  - The file format includes parameters for the object ID hash function,
>    so a future change of hash algorithm does not require a change in format.
>  
> +- Commit grafts and replace objects can change the shape of the commit
> +  history. These can also be enabled/disabled on the fly using

I think you meant "The latter can..." instead of "These can...", as
grafts cannot be disabled with `--no-replace-objects` option.

> +  `--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

[PATCH 4/8] test-repository: properly init repo

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/helper/test-repository.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
index 2762ca656..6a84a53ef 100644
--- a/t/helper/test-repository.c
+++ b/t/helper/test-repository.c
@@ -15,7 +15,10 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
 	struct commit *c;
 	struct commit_list *parent;
 
-	repo_init(&r, gitdir, worktree);
+	setup_git_env(gitdir);
+
+	if (repo_init(&r, gitdir, worktree))
+		die("Couldn't init repo");
 
 	c = lookup_commit(&r, commit_oid);
 
@@ -38,7 +41,10 @@ static void test_get_commit_tree_in_graph(const char *gitdir,
 	struct commit *c;
 	struct tree *tree;
 
-	repo_init(&r, gitdir, worktree);
+	setup_git_env(gitdir);
+
+	if (repo_init(&r, gitdir, worktree))
+		die("Couldn't init repo");
 
 	c = lookup_commit(&r, commit_oid);
 
-- 
gitgitgadget

Re: [PATCH 4/8] test-repository: properly init repo

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/helper/test-repository.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
> index 2762ca656..6a84a53ef 100644
> --- a/t/helper/test-repository.c
> +++ b/t/helper/test-repository.c
> @@ -15,7 +15,10 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
>  	struct commit *c;
>  	struct commit_list *parent;
>  
> -	repo_init(&r, gitdir, worktree);
> +	setup_git_env(gitdir);
> +
> +	if (repo_init(&r, gitdir, worktree))
> +		die("Couldn't init repo");

Shouldn't this message be marked for translation?

  +		die(_("Couldn't init repo"));

Perhaps we could provide also more detail about error, if possible.

>  
>  	c = lookup_commit(&r, commit_oid);
>  
> @@ -38,7 +41,10 @@ static void test_get_commit_tree_in_graph(const char *gitdir,
>  	struct commit *c;
>  	struct tree *tree;
>  
> -	repo_init(&r, gitdir, worktree);
> +	setup_git_env(gitdir);
> +
> +	if (repo_init(&r, gitdir, worktree))
> +		die("Couldn't init repo");

I wonder if it would be worth it to extract this idiom into separate
function, called for exaple repo_init_or_die(), to avoid code
duplication (and maybe use it in other places?).

>  
>  	c = lookup_commit(&r, commit_oid);

[PATCH 5/8] commit-graph: not compatible with replace objects

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 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(-)

diff --git a/commit-graph.c b/commit-graph.c
index b0a55ad12..711099858 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,15 @@ static struct commit_graph *alloc_commit_graph(void)
 	return g;
 }
 
+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;
+}
+
 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);
@@ -693,6 +707,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 017f02f8e..4d2d84cf4 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 f996de3d6..c7a99fc35 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 4f17d7701..c90626f5d 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_missing .git/objects/info/commit-graph
+	)
+'
+
 # 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
-- 
gitgitgadget

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Jeff King]

On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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:

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

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Jeff King]

On Wed, Jul 18, 2018 at 03:46:57PM -0400, Jeff King wrote:

> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:
> 
> > From: Derrick Stolee <dstolee@microsoft.com>
> > 
> > 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.

Er, this should be "punt _on_ cases we can't handle". Sorry for my
belated proof-reading.

-Peff

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Derrick Stolee]

On 7/18/2018 3:46 PM, Jeff King wrote:
> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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:
>
>> +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.

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,

-Stolee

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Derrick Stolee]

On 7/18/2018 3:52 PM, Derrick Stolee wrote:
> On 7/18/2018 3:46 PM, Jeff King wrote:
>> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via 
>> GitGitGadget wrote:
>>
>>> From: Derrick Stolee <dstolee@microsoft.com>
>>>
>>> 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:
>>
>>> +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.
>
> 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.

If there are more comments about how I'm handling these cases, then 
please jump in and tell me. I'll revisit this topic in a few weeks.

Thanks,

-Stolee

[1] 
https://public-inbox.org/git/20180718204449.GA31816@sigill.intra.peff.net/T/#u

      [PATCH 1/3] check_replace_refs: fix outdated comment

[2] 
https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#u

     [PATCH 0/2] RFC ref store to repository migration

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Stefan Beller]

> 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) ?

Stefan

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Derrick Stolee]

On 7/20/2018 12:49 PM, Stefan Beller wrote:
>> 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) ?

OK. Thanks for letting me know.

Perhaps I'll give it a try to pass the repository struct in the cbdata 
for the function pointers, as a minimum amount of change option.

Thanks,

-Stolee

Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

All right, looks sensible.  Did you start with the history-"altering"
feature with simplest detection mechanism?

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

I wonder if we should test the implementation, or just that you get
correct answer when history view is altered after writing commit-graph
file... oh, wait, you do that (well, except for testing that
`git commit-graph write` does not create commit-graph file instead of
testing that the commit-graph file stores correct information even after
the feature is removed (with `git replace --delete`) or turned off (with
`--no-replace-objects` or equivalent).  Good.

Sidenote: the inconsistency between command options and
subcommands... ehh... compare e.g. git-replace and git-remote.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  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(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index b0a55ad12..711099858 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,15 @@ static struct commit_graph *alloc_commit_graph(void)
>  	return g;
>  }
>  
> +static int commit_graph_compatible(struct repository *r)
> +{
> +	prepare_replace_object(r);
> +	if (hashmap_get_size(&r->objects->replace_map->map))

A very minor nitpick, feel free to ignore.  I think that the condition
would be more readable written as

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

Another issue is that the condition may be too strict (but it is also
the easiest to test for).  You might have replacement objects for
non-commit objects (e.g. replacing contents of configuration files with
one with API keys and/or other secrets - not that I know any tool that
does that), or replacing commit-objects without changing their history
(e.g. to fix some annoying error in demo repository, where you want to
show reflog).  But I think that would be rare, so easier if maybe
over-eager test is fine for me.

> +		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);
> @@ -693,6 +707,9 @@ void write_commit_graph(const char *obj_dir,
>  	int num_extra_edges;
>  	struct commit_list *parent;
>  
> +	if (!commit_graph_compatible(the_repository))
> +		return;
> +

Hmmm... we might want to write commit-graph for immutable history; but
not all history-"altering" feature can be easily turned off.  So if it
is to stay universal, without aadditional complications, it is good
enough.

>  	oids.nr = 0;
>  	oids.alloc = approximate_object_count() / 4;
>  
> diff --git a/replace-object.c b/replace-object.c
> index 017f02f8e..4d2d84cf4 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 f996de3d6..c7a99fc35 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);
> +

Hmmm... how it is that this function didn't need to be exported before,
I do wonder?

>  /*
>   * 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 4f17d7701..c90626f5d 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 &&

All right, this is preparation / setup; create commit-graph file, and
check that it was created.

> +		git replace HEAD~1 HEAD~2 &&

All right, so before replacement the history looks like this:

     ... <--- A <--- B <--- C <--- D     <--- master  <=== HEAD

             ~3     ~2     ~1    HEAD

While after the replacement the history view looks like this, if I
understand it correctly:

     ... <--- A <--- B
               \
                \---[B] <--- D    <--- master  <=== HEAD


Wouldn't it be better (and more similar to the grafts test) to use
`git replace --graft`, e.g.:

  +		git replace --graft HEAD~1 HEAD~3 &&

But that might be just a matter of taste.

> +		git -c core.commitGraph=false log >expect &&
> +		git -c core.commitGraph=true log >actual &&
> +		test_cmp expect actual &&

All right, so instead of testing if the output is what is expected, we
test that the commit-graph feature did not change it.  Good.

> +		git commit-graph write --reachable &&

Is this necessary [see below]?

> +		git -c core.commitGraph=false --no-replace-objects log >expect &&
> +		git -c core.commitGraph=true --no-replace-objects log >actual &&

All right, and here we test the same for `--no-replace-objects` case.
Good.

By the way, is this `git commit-graph write --reachable` preceding this
part of test necessary?

> +		test_cmp expect actual &&
> +		rm -rf .git/objects/info/commit-graph &&
> +		git commit-graph write --reachable &&
> +		test_path_is_missing .git/objects/info/commit-graph

All right, so here we test that we wont write commit-graph file with
altered view of history; which would store wrong information if the
replace feature is turned off or if replace object is deleted (or if
commit graph gets transferred in the furure, but the replacement refs do
not).

Checking that the commit-graph file ws not created might be seen as
testing implementation details, but full test would require testing with
`--no-replace-objects` and after `git replace -d`; the above is simpler,
so good enough.

> +	)
> +'
> +
>  # 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

[PATCH 6/8] commit-graph: not compatible with grafts

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 commit-graph.c          |  6 ++++++
 commit.c                |  2 +-
 commit.h                |  1 +
 t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 711099858..5097c7c12 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -60,6 +60,12 @@ static struct commit_graph *alloc_commit_graph(void)
 
 static int commit_graph_compatible(struct repository *r)
 {
+	prepare_commit_graft(r);
+	if (r->parsed_objects && r->parsed_objects->grafts_nr)
+		return 0;
+	if (is_repository_shallow(r))
+		return 0;
+
 	prepare_replace_object(r);
 	if (hashmap_get_size(&r->objects->replace_map->map))
 		return 0;
diff --git a/commit.c b/commit.c
index 39b80bd21..609adaf8a 100644
--- a/commit.c
+++ b/commit.c
@@ -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;
 
diff --git a/commit.h b/commit.h
index da0db36eb..5459e279f 100644
--- a/commit.h
+++ b/commit.h
@@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(struct strbuf *line);
 int register_commit_graft(struct repository *r, struct commit_graft *, int);
+void prepare_commit_graft(struct repository *r);
 struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c90626f5d..1d9f49af5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '
 	)
 '
 
+test_expect_success 'commit grafts invalidate commit-graph' '
+	cd "$TRASH_DIRECTORY" &&
+	test_when_finished rm -rf graft &&
+	git clone full graft &&
+	(
+		cd graft &&
+		git commit-graph write --reachable &&
+		test_path_is_file .git/objects/info/commit-graph &&
+		git replace --graft HEAD~1 HEAD~3 &&
+		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_missing .git/objects/info/commit-graph
+	)
+'
+
+test_expect_success 'replace-objects invalidates commit-graph' '
+	cd "$TRASH_DIRECTORY" &&
+	test_when_finished rm -rf shallow &&
+	git clone --depth 2 "file://$TRASH_DIRECTORY/full" shallow &&
+	(
+		cd shallow &&
+		git commit-graph write --reachable &&
+		test_path_is_missing .git/objects/info/commit-graph &&
+		git fetch origin --unshallow &&
+		git commit-graph write --reachable &&
+		test_path_is_file .git/objects/info/commit-graph
+	)
+'
+
 # 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
-- 
gitgitgadget

Re: [PATCH 6/8] commit-graph: not compatible with grafts

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

Tests for grafts are wrong, as they test replace objects.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c          |  6 ++++++
>  commit.c                |  2 +-
>  commit.h                |  1 +
>  t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 711099858..5097c7c12 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -60,6 +60,12 @@ static struct commit_graph *alloc_commit_graph(void)
>  
>  static int commit_graph_compatible(struct repository *r)
>  {
> +	prepare_commit_graft(r);
> +	if (r->parsed_objects && r->parsed_objects->grafts_nr)
> +		return 0;
> +	if (is_repository_shallow(r))
> +		return 0;

Do I assume correctly that is_repository_shallow(r) needs
prepare_commit_graph(r) done earlier?  If it is so, all right.

If it is not so, then why put two separate history-"altering" features,
namely grafts and shallow clones in single commit instead of in separate
commits?

> +
>  	prepare_replace_object(r);
>  	if (hashmap_get_size(&r->objects->replace_map->map))
>  		return 0;
> diff --git a/commit.c b/commit.c
> index 39b80bd21..609adaf8a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -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;
>  
> diff --git a/commit.h b/commit.h
> index da0db36eb..5459e279f 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
>  
>  struct commit_graft *read_graft_line(struct strbuf *line);
>  int register_commit_graft(struct repository *r, struct commit_graft *, int);
> +void prepare_commit_graft(struct repository *r);
>  struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
>

I guess that prepare_commit_graft() was not needed outside commit.c
before.

>  extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index c90626f5d..1d9f49af5 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '
>  	)
>  '
>  
> +test_expect_success 'commit grafts invalidate commit-graph' '
> +	cd "$TRASH_DIRECTORY" &&
> +	test_when_finished rm -rf graft &&
> +	git clone full graft &&
> +	(
> +		cd graft &&
> +		git commit-graph write --reachable &&
> +		test_path_is_file .git/objects/info/commit-graph &&
> +		git replace --graft HEAD~1 HEAD~3 &&

Errr... this uses *replace objects* mechanism, not graft file!!  The
`--graft` option of git-remote command is here to make it easier to use
replace objects to alter the view of history, and make if easy to
convert from deprected grafts feature to replace objects.

Grafts file structure is described in gitrepository-layout(5) manpage.
You would need to do something like the following:

  +		H1=$(git rev-parse --verify HEAD~1) &&
  +		H3=$(git rev-parse --verify HEAD~3) &&
  +		echo '$H1 $H3' >.git/info/grafts &&


You could have used "git replace --graft HEAD~1 HEAD~3" in previous
commit test, like I wrote in reply to it.

> +		git -c core.commitGraph=false log >expect &&
> +		git -c core.commitGraph=true log >actual &&
> +		test_cmp expect actual &&

All right, we test that we get the same result (when graft-file is
written before altering the view of history) with and without the
commit-graph feature.

Sidenote: shouldn't we use rev-list instead?  Though here git-log is
actually safe to use...

> +		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 &&

The `--no-replace-objects` does not affect the graph file!!  You would
want to remove graft file instead:

  +		git commit-graph write --reachable &&
  +		rm -f .git/info/grafts &&
  +		git -c core.commitGraph=false log >expect &&
  +		git -c core.commitGraph=true log >actual &&

And then check the results.

> +		test_cmp expect actual &&
> +		rm -rf .git/objects/info/commit-graph &&
> +		git commit-graph write --reachable &&
> +		test_path_is_missing .git/objects/info/commit-graph

All right.

Though, if it is the same for grafts and for replacements... nah.
Adding a feature and deleting a feature is different, and it is only a
single repetition.

> +	)
> +'
> +
> +test_expect_success 'replace-objects invalidates commit-graph' '
> +	cd "$TRASH_DIRECTORY" &&
> +	test_when_finished rm -rf shallow &&
> +	git clone --depth 2 "file://$TRASH_DIRECTORY/full" shallow &&

Why do we need to use "file://..." URL, instead of simply "full"?

> +	(
> +		cd shallow &&
> +		git commit-graph write --reachable &&
> +		test_path_is_missing .git/objects/info/commit-graph &&
> +		git fetch origin --unshallow &&
> +		git commit-graph write --reachable &&
> +		test_path_is_file .git/objects/info/commit-graph

All right, so in this case because you need to start (clone) with
shallow feature, the test could be simplified.

> +	)
> +'
> +
>  # 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

[PATCH 7/8] commit-graph: not compatible with uninitialized repo

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 5097c7c12..233958e10 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -60,6 +60,9 @@ static struct commit_graph *alloc_commit_graph(void)
 
 static int commit_graph_compatible(struct repository *r)
 {
+	if (!r->gitdir)
+		return 0;
+
 	prepare_commit_graft(r);
 	if (r->parsed_objects && r->parsed_objects->grafts_nr)
 		return 0;
-- 
gitgitgadget

Re: [PATCH 7/8] commit-graph: not compatible with uninitialized repo

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

[I hope that the rest of replies would make it all right through
GitGitGadget]

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 5097c7c12..233958e10 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -60,6 +60,9 @@ static struct commit_graph *alloc_commit_graph(void)
>  
>  static int commit_graph_compatible(struct repository *r)
>  {
> +	if (!r->gitdir)
> +		return 0;
> +

Nice catch.

I wonder if this is the problem because of the following tests (in which
case this test should probably be earlier in this series), or not (and
it doesn't need to).

I wonder how this issue was caught.

>  	prepare_commit_graft(r);
>  	if (r->parsed_objects && r->parsed_objects->grafts_nr)
>  		return 0;

No tests?

[PATCH 8/8] commit-graph: close_commit_graph before shallow walk

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 commit-graph.c | 8 ++++----
 commit-graph.h | 1 +
 upload-pack.c  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 233958e10..237d4e7d1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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);
 
diff --git a/commit-graph.h b/commit-graph.h
index 76e098934..13d736cdd 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -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
diff --git a/upload-pack.c b/upload-pack.c
index 4ca052d0b..52ad6c8e5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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);
-- 
gitgitgadget

Re: [PATCH 8/8] commit-graph: close_commit_graph before shallow walk

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Make close_commit_graph() work for arbitrary repositories.

The first line (above) does not match the rest of the commit message.

> 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 <dstolee@microsoft.com>
> ---
>  commit-graph.c | 8 ++++----
>  commit-graph.h | 1 +
>  upload-pack.c  | 2 ++
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 233958e10..237d4e7d1 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -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);
>  
> diff --git a/commit-graph.h b/commit-graph.h
> index 76e098934..13d736cdd 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -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
> diff --git a/upload-pack.c b/upload-pack.c
> index 4ca052d0b..52ad6c8e5 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -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);

[PATCH] DO-NOT-MERGE: write and read commit-graph always

[Derrick Stolee]

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 <dstolee@microsoft.com>
---
 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(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 158e3f843a..acc31252a9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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>..."),
@@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
 	rerere(0);
+	write_commit_graph_reachable(get_object_directory(), 1);
 	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
diff --git a/builtin/gc.c b/builtin/gc.c
index e103f0f85d..60ab773087 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -41,7 +41,7 @@ static int aggressive_depth = 50;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
-static int gc_write_commit_graph;
+static int gc_write_commit_graph = 1;
 static int detach_auto = 1;
 static timestamp_t gc_log_expire_time;
 static const char *gc_log_expire = "1.day.ago";
@@ -131,7 +131,6 @@ static void gc_config(void)
 	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
 	git_config_get_int("gc.auto", &gc_auto_threshold);
 	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
-	git_config_get_bool("gc.writecommitgraph", &gc_write_commit_graph);
 	git_config_get_bool("gc.autodetach", &detach_auto);
 	git_config_get_expiry("gc.pruneexpire", &prune_expire);
 	git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);
diff --git a/commit-graph.c b/commit-graph.c
index 237d4e7d1b..ed0d27c12e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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)
-		/*
-		 * This repository is not configured to use commit graphs, so
-		 * do not load one. (But report commit_graph_attempted anyway
-		 * so that commit graph loading is not attempted again for this
-		 * repository.)
-		 */
-		return 0;
-
 	if (!commit_graph_compatible(r))
 		return 0;
 
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 4984ca583d..c235672b03 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -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
diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh
index ae52a1882d..0bb54ae227 100755
--- a/t/t5307-pack-missing-commit.sh
+++ b/t/t5307-pack-missing-commit.sh
@@ -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
 '
 
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index e51eb41f4b..9c9cc4d540 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -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
    '
 
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 3f59e58dfb..cec10983cd 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -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

Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

[Jakub Narebski]

Derrick Stolee <dstolee@microsoft.com> writes:

> 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 <dstolee@microsoft.com>
> ---
>  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(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 158e3f843a..acc31252a9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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>..."),
> @@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		     "not exceeded, and then \"git reset HEAD\" to recover."));
>  
>  	rerere(0);
> +	write_commit_graph_reachable(get_object_directory(), 1);
>  	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>  	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
>  	if (amend && !no_post_rewrite) {

This is certainly not for merging.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index e103f0f85d..60ab773087 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -41,7 +41,7 @@ static int aggressive_depth = 50;
>  static int aggressive_window = 250;
>  static int gc_auto_threshold = 6700;
>  static int gc_auto_pack_limit = 50;
> -static int gc_write_commit_graph;
> +static int gc_write_commit_graph = 1;
>  static int detach_auto = 1;
>  static timestamp_t gc_log_expire_time;
>  static const char *gc_log_expire = "1.day.ago";

This is switching from default to off to default to on.  I think with
this feature we would default to off for a long time (wishful thinking:
maybe automatically enabling it for large repositories, or if
commit-graph file exists already?).

> @@ -131,7 +131,6 @@ static void gc_config(void)
>  	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
>  	git_config_get_int("gc.auto", &gc_auto_threshold);
>  	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> -	git_config_get_bool("gc.writecommitgraph", &gc_write_commit_graph);
>  	git_config_get_bool("gc.autodetach", &detach_auto);
>  	git_config_get_expiry("gc.pruneexpire", &prune_expire);
>  	git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);

This is certainly not for merging, as it disables gc.writeCommitGraph
config option entirely.

> diff --git a/commit-graph.c b/commit-graph.c
> index 237d4e7d1b..ed0d27c12e 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -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)
> -		/*
> -		 * This repository is not configured to use commit graphs, so
> -		 * do not load one. (But report commit_graph_attempted anyway
> -		 * so that commit graph loading is not attempted again for this
> -		 * repository.)
> -		 */
> -		return 0;
> -
>  	if (!commit_graph_compatible(r))
>  		return 0;
>

This is certainly not for merging, as it disables core.commitGraph
config option entirely.

> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 4984ca583d..c235672b03 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -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
> diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh
> index ae52a1882d..0bb54ae227 100755
> --- a/t/t5307-pack-missing-commit.sh
> +++ b/t/t5307-pack-missing-commit.sh
> @@ -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
>  '
>  
> diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
> index e51eb41f4b..9c9cc4d540 100755
> --- a/t/t6011-rev-list-with-bad-commit.sh
> +++ b/t/t6011-rev-list-with-bad-commit.sh
> @@ -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
>     '
>  
> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
> index 3f59e58dfb..cec10983cd 100755
> --- a/t/t6024-recursive-merge.sh
> +++ b/t/t6024-recursive-merge.sh
> @@ -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
>  "

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

Regards,
-- 
Jakub Narębski

Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

[Stefan Beller]

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

Thanks,
Stefan

Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

[Jakub Narebski]

Stefan Beller <sbeller@google.com> writes:

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

Yes, I understand that, and for the most part I agree with it.  This
commit main purpose is to thoroughly exercise the new feature.

But I think that changes to the testsuite could have been extracted into
separate commit, and merged (and only those changes).  It could serve as
note about the intent of the test.  Well, perhaps after encapsulating it
in some function with a good name... ;-)

Anyway, this is a minor insignificant thing.
-- 
Jakub Narębski

Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

[Elijah Newren]

On Wed, Jul 18, 2018 at 8:22 AM, Derrick Stolee <dstolee@microsoft.com> wrote:
> 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.

I know you meant for these to not be merged, but perhaps the test in
t6024 could be made to be less stringent about order of merge bases.
In particular, instead of expecting a certain sha1 to be at stage 2
and a different one to be at stage 3, it could just check that both
shas appear in the `git ls-files --stage` output.

> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
> index 3f59e58dfb..cec10983cd 100755
> --- a/t/t6024-recursive-merge.sh
> +++ b/t/t6024-recursive-merge.sh
> @@ -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

Re: [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Jakub Narebski]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

This means that these features can change the view of the history, and
if the commit-graph was created before the change, it can have stale
incorrect information -- which would lead to incorrect results if the
commit-graph feature is used.

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

Yes, the alternative would be trying to keep the commit-graph file
up-to-date with the current view of history.  But there are problems
with this approach.


With grafts (which is a deprecated feature), there is no automated entry
point.  Best what one could do is to store some kind of fingerprint of
graft file, and invalidate / recreate commit-graft file if it does not
match what is in graft file at the time of running git command that may
use commit-graft feature.

With replace objects we have two separate entry points: the git-replace
command and fetching references in refs/replace/* namespace (and
equivalent).  While the first should be not difficult to do, the second
one seems to be harder.  Additionally, there is '--no-replace-objects'
option to turn off the feature (fetch and push ignore replacement
objects automatically); so we might want to have two versions of
commit-graph: one with replacement objects feature and one without (or
something like that).

The shallow clone seems easiest, with only one automated entry points
for changing the view of history this way, and no option to disable this
feature -- but it is also the least interesting, as the intent of
shallow clone is to have less history, so the commit-graph is less
necessary, as you wrote.


We might want to revisit it later, but I agree that starting from this
simple approach would be best.

One thing I would like to see added is for the user to know when
commit-graph is not available (in manpages), and maybe even a way to see
if it is on (e.g. with 'git commit-graph verify' and/or maybe in
'git status' output).  But this is a separate issue.

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

Wouldn't it be enough to create commit-graph file, change the view of
the history (via grafts, via replace objects, via shallow clone), and
check that you still get correct results?

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

The previous RFC was in my opinion needlessly complicated.  This one is
much simpler, and better.

[...]
> Thanks,
> -Stolee
>
> [1] https://public-inbox.org/git/20180531174024.124488-1-dstolee@microsoft.com/
>      [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo
>
> [2] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#t
>     [PATCH 0/2] RFC ref store to repository migration
>
> [3] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#m966eac85fd58c66523654ddaf0bec72877d3295a
>     [PATCH] TO-SQUASH: replace the_repository with arbitrary r
>
> Based-On: jt/commit-graph-per-object-store
> Cc: jonathantanmy@google.com
> Cc: sbeller@google.com
>
> 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

Re: [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Eric Sunshine]

On Sun, Jul 29, 2018 at 9:13 AM Jakub Narebski <jnareb@gmail.com> wrote:
> Sidenote: the GitGitGadget doesn't fold/wrap lines properly in the
> cover-letter.  Not that it is much of a problem.

Stefan opened a ticket at the gitgitgadget project:
https://github.com/gitgitgadget/gitgitgadget/issues/26

[PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

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

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.

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/commit-graph.c                   |  4 ++
 builtin/replace.c                        |  8 ++--
 commit-graph.c                           | 38 +++++++++++++--
 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                  | 60 ++++++++++++++++++++++++
 upload-pack.c                            |  2 +
 16 files changed, 194 insertions(+), 30 deletions(-)


base-commit: 1689c22c1c328e9135ed51458e9f9a5d224c5057
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 1/8] refs.c: migrate internal ref iteration to pass thru repository argument

[Derrick Stolee]

From: Stefan Beller <sbeller@google.com>

In 60ce76d3581 (refs: add repository argument to for_each_replace_ref,
2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle
arbitrary repositories, 2018-04-11), for_each_replace_ref learned how
to iterate over refs by a given arbitrary repository.
New attempts in the object store conversion have shown that it is useful
to have the repository handle available that the refs iteration is
currently iterating over.

To achieve this goal we will need to add a repository argument to
each_ref_fn in refs.h. However as many callers rely on the signature
such a patch would be too large.

So convert the internals of the ref subsystem first to pass through a
repository argument without exposing the change to the user. Assume
the_repository for the passed through repository, although it is not
used anywhere yet.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 refs.c               | 39 +++++++++++++++++++++++++++++++++++++--
 refs.h               | 10 ++++++++++
 refs/iterator.c      |  6 +++---
 refs/refs-internal.h |  5 +++--
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 457fb78057..7cd76f72d2 100644
--- a/refs.c
+++ b/refs.c
@@ -1386,17 +1386,50 @@ struct ref_iterator *refs_ref_iterator_begin(
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
  */
+static int do_for_each_repo_ref(struct repository *r, const char *prefix,
+				each_repo_ref_fn fn, int trim, int flags,
+				void *cb_data)
+{
+	struct ref_iterator *iter;
+	struct ref_store *refs = get_main_ref_store(r);
+
+	if (!refs)
+		return 0;
+
+	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
+
+	return do_for_each_repo_ref_iterator(r, iter, fn, cb_data);
+}
+
+struct do_for_each_ref_help {
+	each_ref_fn *fn;
+	void *cb_data;
+};
+
+static int do_for_each_ref_helper(struct repository *r,
+				  const char *refname,
+				  const struct object_id *oid,
+				  int flags,
+				  void *cb_data)
+{
+	struct do_for_each_ref_help *hp = cb_data;
+
+	return hp->fn(refname, oid, flags, hp->cb_data);
+}
+
 static int do_for_each_ref(struct ref_store *refs, const char *prefix,
 			   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
 	struct ref_iterator *iter;
+	struct do_for_each_ref_help hp = { fn, cb_data };
 
 	if (!refs)
 		return 0;
 
 	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
 
-	return do_for_each_ref_iterator(iter, fn, cb_data);
+	return do_for_each_repo_ref_iterator(the_repository, iter,
+					do_for_each_ref_helper, &hp);
 }
 
 int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
@@ -2025,10 +2058,12 @@ int refs_verify_refname_available(struct ref_store *refs,
 int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	struct ref_iterator *iter;
+	struct do_for_each_ref_help hp = { fn, cb_data };
 
 	iter = refs->be->reflog_iterator_begin(refs);
 
-	return do_for_each_ref_iterator(iter, fn, cb_data);
+	return do_for_each_repo_ref_iterator(the_repository, iter,
+					     do_for_each_ref_helper, &hp);
 }
 
 int for_each_reflog(each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index cc2fb4c68c..80eec8bbc6 100644
--- a/refs.h
+++ b/refs.h
@@ -274,6 +274,16 @@ struct ref_transaction;
 typedef int each_ref_fn(const char *refname,
 			const struct object_id *oid, int flags, void *cb_data);
 
+/*
+ * The same as each_ref_fn, but also with a repository argument that
+ * contains the repository associated with the callback.
+ */
+typedef int each_repo_ref_fn(struct repository *r,
+			     const char *refname,
+			     const struct object_id *oid,
+			     int flags,
+			     void *cb_data);
+
 /*
  * The following functions invoke the specified callback function for
  * each reference indicated.  If the function ever returns a nonzero
diff --git a/refs/iterator.c b/refs/iterator.c
index 2ac91ac340..629e00a122 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
 
 struct ref_iterator *current_ref_iter = NULL;
 
-int do_for_each_ref_iterator(struct ref_iterator *iter,
-			     each_ref_fn fn, void *cb_data)
+int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter,
+				  each_repo_ref_fn fn, void *cb_data)
 {
 	int retval = 0, ok;
 	struct ref_iterator *old_ref_iter = current_ref_iter;
 
 	current_ref_iter = iter;
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
-		retval = fn(iter->refname, iter->oid, iter->flags, cb_data);
+		retval = fn(r, iter->refname, iter->oid, iter->flags, cb_data);
 		if (retval) {
 			/*
 			 * If ref_iterator_abort() returns ITER_ERROR,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 04425d6d1e..89d7dd49cd 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -474,8 +474,9 @@ extern struct ref_iterator *current_ref_iter;
  * adapter between the callback style of reference iteration and the
  * iterator style.
  */
-int do_for_each_ref_iterator(struct ref_iterator *iter,
-			     each_ref_fn fn, void *cb_data);
+int do_for_each_repo_ref_iterator(struct repository *r,
+				  struct ref_iterator *iter,
+				  each_repo_ref_fn fn, void *cb_data);
 
 /*
  * Only include per-worktree refs in a do_for_each_ref*() iteration.
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback

[Derrick Stolee]

From: Stefan Beller <sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/replace.c | 8 ++++----
 refs.c            | 9 ++++-----
 refs.h            | 2 +-
 replace-object.c  | 5 +++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index e997a562f0..b5861a0ee9 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -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));
diff --git a/refs.c b/refs.c
index 7cd76f72d2..c5a5f727e8 100644
--- a/refs.c
+++ b/refs.c
@@ -1474,12 +1474,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)
diff --git a/refs.h b/refs.h
index 80eec8bbc6..a0a18223a1 100644
--- a/refs.h
+++ b/refs.h
@@ -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);
 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);
diff --git a/replace-object.c b/replace-object.c
index 4162df6324..3c17864eb7 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -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;
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 3/8] commit-graph: update design document

[Derrick Stolee]

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 <dstolee@microsoft.com>
---
 Documentation/technical/commit-graph.txt | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt
index c664acbd76..001395e950 100644
--- a/Documentation/technical/commit-graph.txt
+++ b/Documentation/technical/commit-graph.txt
@@ -112,12 +112,24 @@ Design Details
 - The file format includes parameters for the object ID hash function,
   so a future change of hash algorithm does not require a change in format.
 
+- Commit grafts and replace objects can change the shape of the commit
+  history. The latter can also be enabled/disabled on the fly using
+  `--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
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 4/8] test-repository: properly init repo

[Derrick Stolee]

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/helper/test-repository.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
index 2762ca6562..6a84a53efb 100644
--- a/t/helper/test-repository.c
+++ b/t/helper/test-repository.c
@@ -15,7 +15,10 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
 	struct commit *c;
 	struct commit_list *parent;
 
-	repo_init(&r, gitdir, worktree);
+	setup_git_env(gitdir);
+
+	if (repo_init(&r, gitdir, worktree))
+		die("Couldn't init repo");
 
 	c = lookup_commit(&r, commit_oid);
 
@@ -38,7 +41,10 @@ static void test_get_commit_tree_in_graph(const char *gitdir,
 	struct commit *c;
 	struct tree *tree;
 
-	repo_init(&r, gitdir, worktree);
+	setup_git_env(gitdir);
+
+	if (repo_init(&r, gitdir, worktree))
+		die("Couldn't init repo");
 
 	c = lookup_commit(&r, commit_oid);
 
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 5/8] commit-graph: not compatible with replace objects

[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 <dstolee@microsoft.com>
---
 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
+	)
+'
+
 # 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
-- 
2.18.0.118.gd4f65b8d14

Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects

[Junio C Hamano]

Derrick Stolee <dstolee@microsoft.com> 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 <dstolee@microsoft.com>
> ---
>  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

[Derrick Stolee]

On 8/21/2018 1:45 PM, Junio C Hamano wrote:
> Derrick Stolee <dstolee@microsoft.com> 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 <dstolee@microsoft.com>
>> ---
>>   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

[PATCH v2 6/8] commit-graph: not compatible with grafts

[Derrick Stolee]

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.

Helped-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c          |  6 ++++++
 commit.c                |  2 +-
 commit.h                |  1 +
 t/t5318-commit-graph.sh | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 2c01fa433f..c4eaedf4e5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -68,6 +68,12 @@ static int commit_graph_compatible(struct repository *r)
 			return 0;
 	}
 
+	prepare_commit_graft(r);
+	if (r->parsed_objects && r->parsed_objects->grafts_nr)
+		return 0;
+	if (is_repository_shallow(r))
+		return 0;
+
 	return 1;
 }
 
diff --git a/commit.c b/commit.c
index 30d1af2b20..ef9a2cbb23 100644
--- a/commit.c
+++ b/commit.c
@@ -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;
 
diff --git a/commit.h b/commit.h
index da0db36eba..5459e279fe 100644
--- a/commit.h
+++ b/commit.h
@@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(struct strbuf *line);
 int register_commit_graft(struct repository *r, struct commit_graft *, int);
+void prepare_commit_graft(struct repository *r);
 struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index e0c3c60f66..6aee861f78 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -281,6 +281,44 @@ test_expect_success 'replace-objects invalidates commit-graph' '
 	)
 '
 
+test_expect_success 'commit grafts invalidate commit-graph' '
+	cd "$TRASH_DIRECTORY" &&
+	test_when_finished rm -rf graft &&
+	git clone full graft &&
+	(
+		cd graft &&
+		git commit-graph write --reachable &&
+		test_path_is_file .git/objects/info/commit-graph &&
+		H1=$(git rev-parse --verify HEAD~1) &&
+		H3=$(git rev-parse --verify HEAD~3) &&
+		echo "$H1 $H3" >.git/info/grafts &&
+		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_missing .git/objects/info/commit-graph
+	)
+'
+
+test_expect_success 'replace-objects invalidates commit-graph' '
+	cd "$TRASH_DIRECTORY" &&
+	test_when_finished rm -rf shallow &&
+	git clone --depth 2 "file://$TRASH_DIRECTORY/full" shallow &&
+	(
+		cd shallow &&
+		git commit-graph write --reachable &&
+		test_path_is_missing .git/objects/info/commit-graph &&
+		git fetch origin --unshallow &&
+		git commit-graph write --reachable &&
+		test_path_is_file .git/objects/info/commit-graph
+	)
+'
+
 # 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
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 7/8] commit-graph: not compatible with uninitialized repo

[Derrick Stolee]

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index c4eaedf4e5..cee2caab5c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -62,6 +62,9 @@ extern int read_replace_refs;
 
 static int commit_graph_compatible(struct repository *r)
 {
+	if (!r->gitdir)
+		return 0;
+
 	if (read_replace_refs) {
 		prepare_replace_object(r);
 		if (hashmap_get_size(&r->objects->replace_map->map))
-- 
2.18.0.118.gd4f65b8d14

[PATCH v2 8/8] commit-graph: close_commit_graph before shallow walk

[Derrick Stolee]

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 <dstolee@microsoft.com>
---
 commit-graph.c | 8 ++++----
 commit-graph.h | 1 +
 upload-pack.c  | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index cee2caab5c..4bd1a4abbf 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -260,10 +260,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)
@@ -875,7 +875,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);
 
diff --git a/commit-graph.h b/commit-graph.h
index 76e098934a..13d736cdde 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -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
diff --git a/upload-pack.c b/upload-pack.c
index 82b393ec31..2ae9d9bb47 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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)
@@ -740,6 +741,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);
-- 
2.18.0.118.gd4f65b8d14

Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Stefan Beller]

On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee <dstolee@microsoft.com> wrote:
> 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'll take a look.

Thanks,
Stefan

Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee <dstolee@microsoft.com> wrote:
>> 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?

Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Derrick Stolee]

On 8/21/2018 1:51 PM, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee <dstolee@microsoft.com> wrote:
>>> 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?

I've just been out of the loop and didn't realize what range-diff did. 
Here is the range-diff:

1:  43ddcc9ef9 = 1:  c8edae4179 refs.c: migrate internal ref iteration 
to pass thru repository argument
2:  22dc9ce836 = 2:  f89451e884 refs.c: upgrade for_each_replace_ref to 
be a each_repo_ref_fn callback
3:  5e90d36f84 ! 3:  d95aa3472c commit-graph: update design document
     @@ -19,7 +19,7 @@
         so a future change of hash algorithm does not require a change 
in format.

      +- Commit grafts and replace objects can change the shape of the 
commit
     -+  history. These can also be enabled/disabled on the fly using
     ++  history. The latter can also be enabled/disabled on the fly using
      +  `--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
4:  88711a3cf4 = 4:  ec89c88e14 test-repository: properly init repo
5:  7f596c1718 ! 5:  0321a3cf10 commit-graph: not compatible with 
replace objects
     @@ -6,10 +6,34 @@
          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.
     +    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 <dstolee@microsoft.com>

     +diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
     +--- a/builtin/commit-graph.c
     ++++ b/builtin/commit-graph.c
     +@@
     +   return 0;
     + }
     +
     ++extern int read_replace_refs;
     ++
     + static int graph_write(int argc, const char **argv)
     + {
     +   struct string_list *pack_indexes = NULL;
     +@@
     +   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
      --- a/commit-graph.c
      +++ b/commit-graph.c
     @@ -26,11 +50,15 @@
         return g;
       }

     ++extern int read_replace_refs;
     ++
      +static int commit_graph_compatible(struct repository *r)
      +{
     -+  prepare_replace_object(r);
     -+  if (hashmap_get_size(&r->objects->replace_map->map))
     -+          return 0;
     ++  if (read_replace_refs) {
     ++          prepare_replace_object(r);
     ++          if (hashmap_get_size(&r->objects->replace_map->map))
     ++                  return 0;
     ++  }
      +
      +  return 1;
      +}
     @@ -110,7 +138,7 @@
      +          test_cmp expect actual &&
      +          rm -rf .git/objects/info/commit-graph &&
      +          git commit-graph write --reachable &&
     -+          test_path_is_missing .git/objects/info/commit-graph
     ++          test_path_is_file .git/objects/info/commit-graph
      +  )
      +'
      +
6:  94dd91ac35 ! 6:  d18ecc0124 commit-graph: not compatible with grafts
     @@ -7,24 +7,25 @@
          situations we ignore existing commit-graph files and we do not 
write new
          commit-graph files.

     +    Helped-by: Jakub Narebski <jnareb@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

      diff --git a/commit-graph.c b/commit-graph.c
      --- a/commit-graph.c
      +++ b/commit-graph.c
      @@
     +                   return 0;
     +   }

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

      diff --git a/commit.c b/commit.c
      --- a/commit.c
     @@ -66,7 +67,9 @@
      +          cd graft &&
      +          git commit-graph write --reachable &&
      +          test_path_is_file .git/objects/info/commit-graph &&
     -+          git replace --graft HEAD~1 HEAD~3 &&
     ++          H1=$(git rev-parse --verify HEAD~1) &&
     ++          H3=$(git rev-parse --verify HEAD~3) &&
     ++          echo "$H1 $H3" >.git/info/grafts &&
      +          git -c core.commitGraph=false log >expect &&
      +          git -c core.commitGraph=true log >actual &&
      +          test_cmp expect actual &&
7:  5314a5a93d ! 7:  87d3397700 commit-graph: not compatible with 
uninitialized repo
     @@ -14,6 +14,6 @@
      +  if (!r->gitdir)
      +          return 0;
      +
     -   prepare_commit_graft(r);
     -   if (r->parsed_objects && r->parsed_objects->grafts_nr)
     -           return 0;
     +   if (read_replace_refs) {
     +           prepare_replace_object(r);
     +           if (hashmap_get_size(&r->objects->replace_map->map))
8:  f4ab234ed2 ! 8:  a26c175897 commit-graph: close_commit_graph before 
shallow walk
     @@ -2,8 +2,6 @@

          commit-graph: close_commit_graph before shallow walk

     -    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

Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Stefan Beller]

On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee <dstolee@microsoft.com> wrote:
>
> 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.
>

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

Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Derrick Stolee]

On 8/20/2018 3:37 PM, Stefan Beller wrote:
> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee <dstolee@microsoft.com> wrote:
>> 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.
>>
> 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

Just to keep things on-list as possible, here is the patch for the 
commit linked above. It would replace patches 1 & 2 from this series.

Junio: I can send a v3 that includes this commit if you need it, or if 
there are other edits to be made in this series.

commit 300db80140dacc927db0d46c804ca0ef4dcc1be1
Author: Derrick Stolee <dstolee@microsoft.com>
Date:   Fri Jul 20 15:39:06 2018 -0400

     replace-objects: use arbitrary repositories

     This is the smallest possible change that makes prepare_replace_objects
     work properly with arbitrary repositories. By supplying the repository
     as the cb_data, we do not need to modify any code in the ref iterator
     logic. We will likely want to do a full replacement of the ref iterator
     logic to provide a repository struct as a concrete parameter.

     Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

diff --git a/replace-object.c b/replace-object.c
index 801b5c1678..e99fcd1ff6 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -14,6 +14,7 @@ static int register_replace_ref(const char *refname,
         const char *slash = strrchr(refname, '/');
         const char *hash = slash ? slash + 1 : refname;
         struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));
+       struct repository *r = (struct repository *)cb_data;

         if (get_oid_hex(hash, &repl_obj->original.oid)) {
                 free(repl_obj);
@@ -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;
@@ -40,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
                 xmalloc(sizeof(*r->objects->replace_map));
         oidmap_init(r->objects->replace_map, 0);

-       for_each_replace_ref(r, register_replace_ref, NULL);
+       for_each_replace_ref(r, register_replace_ref, r);
  }

  /* We allow "recursive" replacement. Only within reason, though */