Re: [PATCH v2 2/2] commit-graph: fix writing first commit-graph during fetch

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The previous commit includes a failing test for an issue around
> fetch.writeCommitGraph and fetching in a repo with a submodule. Here, we
> fix that bug and set the test to "test_expect_success".
> 
> The prolem arises with this set of commands when the remote repo at

> <url> has a submodule. Note that --recurse-submodules is not needed to
> demonstrate the bug.
> 
> 	$ git clone <url> test
> 	$ cd test
> 	$ git -c fetch.writeCommitGraph=true fetch origin
> 	Computing commit graph generation numbers: 100% (12/12), done.
> 	BUG: commit-graph.c:886: missing parent <hash1> for commit <hash2>
> 	Aborted (core dumped)
> 
> As an initial fix, I converted the code in builtin/fetch.c that calls
> write_commit_graph_reachable() to instead launch a "git commit-graph
> write --reachable --split" process. That code worked, but is not how we
> want the feature to work long-term.
> 
> That test did demonstrate that the issue must be something to do with
> internal state of the 'git fetch' process.
> 
> The write_commit_graph() method in commit-graph.c ensures the commits we
> plan to write are "closed under reachability" using close_reachable().
> This method walks from the input commits, and uses the UNINTERESTING
> flag to mark which commits have already been visited. This allows the
> walk to take O(N) time, where N is the number of commits, instead of
> O(P) time, where P is the number of paths. (The number of paths can be
> exponential in the number of commits.)
> 
> However, the UNINTERESTING flag is used in lots of places in the
> codebase. This flag usually means some barrier to stop a commit walk,
> such as in revision-walking to compare histories. It is not often
> cleared after the walk completes because the starting points of those
> walks do not have the UNINTERESTING flag, and clear_commit_marks() would
> stop immediately.
> 
> This is happening during a 'git fetch' call with a remote. The fetch
> negotiation is comparing the remote refs with the local refs and marking
> some commits as UNINTERESTING.
> 
> You may ask: did this feature ever work at all? Yes, it did, as long as
> you had a commit-graph covering all of your local refs. My testing was
> unfortunately limited to this scenario. The UNINTERESTING commits are
> always part of the "old" commit-graph, and when we add new commits to a
> top layer of the commit-graph chain those are not needed. If we happen
> to merge layers of the chain, then the commits are added as a list, not
> using a commit walk. Further, the test added for this config option in
> t5510-fetch.sh uses local filesystem clones, which somehow avoids this
> logic.

> I tested running clear_commit_marks_many() to clear the UNINTERESTING
> flag inside close_reachable(), but the tips did not have the flag, so
> that did nothing.
> 
> It turns out that the calculate_changed_submodule_paths() method is at
> fault. Thanks, Peff, for pointing out this detail! More specifically,
> for each submodule, the collect_changed_submodules() runs a revision
> walk to essentially do file-history on the list of submodules. That
> revision walk marks commits UNININTERESTING if they are simiplified away

> by not changing the submodule.
> 
> Instead, I finally arrived on the conclusion that I should use a flag
> that is not used in any other part of the code. In commit-reach.c, a
> number of flags were defined for commit walk algorithms. The REACHABLE
> flag seemed like it made the most sense, and it seems it was not
> actually used in the file. The REACHABLE flag was used in early versions
> of commit-reach.c, but was removed by 4fbcca4 (commit-reach: make
> can_all_from_reach... linear, 2018-07-20).
> 
> Add the REACHABLE flag to commit-graph.c and use it instead of
> UNINTERESTING in close_reachable(). This fixes the bug in manual
> testing.

> Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Szeder Gábor <szeder.dev@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c   | 11 +++++++----
>  commit-reach.c   |  1 -
>  object.h         |  3 ++-
>  t/t5510-fetch.sh |  2 +-
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/commit-graph.c b/commit-graph.c
> index fc4a43b8d6..0aea7b2ae5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -41,6 +41,9 @@
>  #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
>  			+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)
>  
> +/* Remember to update object flag allocation in object.h */
> +#define REACHABLE       (1u<<15)
> +
>  char *get_commit_graph_filename(const char *obj_dir)
>  {
>  	char *filename = xstrfmt("%s/info/commit-graph", obj_dir);
> @@ -1030,11 +1033,11 @@ static void add_missing_parents(struct write_commit_graph_context *ctx, struct c
>  {
>  	struct commit_list *parent;
>  	for (parent = commit->parents; parent; parent = parent->next) {
> -		if (!(parent->item->object.flags & UNINTERESTING)) {
> +		if (!(parent->item->object.flags & REACHABLE)) {
>  			ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
>  			oidcpy(&ctx->oids.list[ctx->oids.nr], &(parent->item->object.oid));
>  			ctx->oids.nr++;
> -			parent->item->object.flags |= UNINTERESTING;
> +			parent->item->object.flags |= REACHABLE;
>  		}
>  	}
>  }
> @@ -1052,7 +1055,7 @@ static void close_reachable(struct write_commit_graph_context *ctx)
>  		display_progress(ctx->progress, i + 1);
>  		commit = lookup_commit(ctx->r, &ctx->oids.list[i]);
>  		if (commit)
> -			commit->object.flags |= UNINTERESTING;
> +			commit->object.flags |= REACHABLE;
>  	}
>  	stop_progress(&ctx->progress);
>  
> @@ -1089,7 +1092,7 @@ static void close_reachable(struct write_commit_graph_context *ctx)
>  		commit = lookup_commit(ctx->r, &ctx->oids.list[i]);
>  
>  		if (commit)
> -			commit->object.flags &= ~UNINTERESTING;
> +			commit->object.flags &= ~REACHABLE;
>  	}
>  	stop_progress(&ctx->progress);
>  }
> diff --git a/commit-reach.c b/commit-reach.c
> index 3ea174788a..4ca7e706a1 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -10,7 +10,6 @@
>  #include "commit-reach.h"
>  
>  /* Remember to update object flag allocation in object.h */
> -#define REACHABLE       (1u<<15)
>  #define PARENT1		(1u<<16)
>  #define PARENT2		(1u<<17)
>  #define STALE		(1u<<18)
> diff --git a/object.h b/object.h
> index 0120892bbd..25f5ab3d54 100644
> --- a/object.h
> +++ b/object.h
> @@ -68,7 +68,8 @@ struct object_array {
>   * bisect.c:                                        16
>   * bundle.c:                                        16
>   * http-push.c:                                     16-----19
> - * commit-reach.c:                                15-------19
> + * commit-graph.c:                                15
> + * commit-reach.c:                                  16-----19
>   * sha1-name.c:                                              20
>   * list-objects-filter.c:                                      21
>   * builtin/fsck.c:           0--3
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e8ae3af0b6..7bfbcc2036 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -583,7 +583,7 @@ test_expect_success 'fetch.writeCommitGraph' '
>  	)
>  '
>  
> -test_expect_failure 'fetch.writeCommitGraph with submodules' '
> +test_expect_success 'fetch.writeCommitGraph with submodules' '
>  	pwd="$(pwd)" &&
>  	git clone dups super &&
>  	(
> -- 
> gitgitgadget