Re: [RFC PATCH 10/12] commit-graph: add '--reachable' option

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <dstolee@microsoft.com> writes:

> When writing commit-graph files, it can be convenient to ask for all
> reachable commits (starting at the ref set) in the resulting file. This
> is particularly helpful when writing to stdin is complicated, such as a
> future integration with 'git gc' which will call
> 'git commit-graph write --reachable' after performing cleanup of the
> object database.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

For what it is worth, it looks good to me.

> ---
>  Documentation/git-commit-graph.txt |  8 ++++--
>  builtin/commit-graph.c             | 41 +++++++++++++++++++++++++++---
>  t/t5318-commit-graph.sh            | 10 ++++++++
>  3 files changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index 93c7841ba2..1b14d40590 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -37,12 +37,16 @@ Write a commit graph file based on the commits found in packfiles.
>  +
>  With the `--stdin-packs` option, generate the new commit graph by
>  walking objects only in the specified pack-indexes. (Cannot be combined
> -with --stdin-commits.)
> +with --stdin-commits or --reachable.)
>  +
>  With the `--stdin-commits` option, generate the new commit graph by
>  walking commits starting at the commits specified in stdin as a list
>  of OIDs in hex, one OID per line. (Cannot be combined with
> ---stdin-packs.)
> +--stdin-packs or --reachable.)
> ++
> +With the `--reachable` option, generate the new commit graph by walking
> +commits starting at all refs. (Cannot be combined with --stdin-commits
> +or --stind-packs.)

I wonder if this "cannot be combined" is sustainable... ;-)


[...]
> @@ -113,6 +115,25 @@ static int graph_read(int argc, const char **argv)
>  	return 0;
>  }
>  
> +struct hex_list {
> +	char **hex_strs;
> +	int hex_nr;
> +	int hex_alloc;
> +};
> +
> +static int add_ref_to_list(const char *refname,
> +			   const struct object_id *oid,
> +			   int flags, void *cb_data)
> +{
> +	struct hex_list *list = (struct hex_list*)cb_data;
> +
> +	ALLOC_GROW(list->hex_strs, list->hex_nr + 1, list->hex_alloc);
> +	list->hex_strs[list->hex_nr] = xcalloc(GIT_MAX_HEXSZ + 1, 1);
> +	strcpy(list->hex_strs[list->hex_nr], oid_to_hex(oid));
> +	list->hex_nr++;
> +	return 0;
> +}
> +
>  static int graph_write(int argc, const char **argv)
>  {
>  	const char **pack_indexes = NULL;
> @@ -127,6 +148,8 @@ static int graph_write(int argc, const char **argv)
>  		OPT_STRING(0, "object-dir", &opts.obj_dir,
>  			N_("dir"),
>  			N_("The object directory to store the graph")),
> +		OPT_BOOL(0, "reachable", &opts.reachable,
> +			N_("start walk at all refs")),
>  		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,
>  			N_("scan pack-indexes listed by stdin for commits")),
>  		OPT_BOOL(0, "stdin-commits", &opts.stdin_commits,
> @@ -140,8 +163,8 @@ static int graph_write(int argc, const char **argv)
>  			     builtin_commit_graph_write_options,
>  			     builtin_commit_graph_write_usage, 0);
>  
> -	if (opts.stdin_packs && opts.stdin_commits)
> -		die(_("cannot use both --stdin-commits and --stdin-packs"));
> +	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
> +		die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));

Nice trick, but is it worth it (in place of boolean expression)?  Though
it lines up with the error message, though...

>  	if (!opts.obj_dir)
>  		opts.obj_dir = get_object_directory();
>  
> @@ -164,6 +187,16 @@ static int graph_write(int argc, const char **argv)
>  			commit_hex = lines;
>  			commits_nr = lines_nr;
>  		}
> +	} else if (opts.reachable) {
> +		struct hex_list list;
> +		list.hex_nr = 0;
> +		list.hex_alloc = 128;
> +		ALLOC_ARRAY(list.hex_strs, list.hex_alloc);
> +
> +		for_each_ref(add_ref_to_list, &list);
> +
> +		commit_hex = (const char **)list.hex_strs;
> +		commits_nr = list.hex_nr;

Nice trick!

>  	}
>  
>  	write_commit_graph(opts.obj_dir,
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index e91053271a..ccadd22f57 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -200,6 +200,16 @@ test_expect_success 'build graph from commits with append' '
>  graph_git_behavior 'append graph, commit 8 vs merge 1' full commits/8 merge/1
>  graph_git_behavior 'append graph, commit 8 vs merge 2' full commits/8 merge/2
>  
> +test_expect_success 'build graph using --reachable' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	git commit-graph write --reachable &&
> +	test_path_is_file $objdir/info/commit-graph &&
> +	graph_read_expect "11" "large_edges"
> +'
> +
> +graph_git_behavior 'append graph, commit 8 vs merge 1' full commits/8 merge/1
> +graph_git_behavior 'append graph, commit 8 vs merge 2' full commits/8 merge/2
> +
>  test_expect_success 'setup bare repo' '
>  	cd "$TRASH_DIRECTORY" &&
>  	git clone --bare --no-local full bare &&