Re: [PATCH v8 09/14] commit-graph: add core.commitGraph setting

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <stolee@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The commit graph feature is controlled by the new core.commitGraph config
> setting. This defaults to 0, so the feature is opt-in.
>
> The intention of core.commitGraph is that a user can always stop checking
> for or parsing commit graph files if core.commitGraph=0.

This is bool-valued setting, so the commit message should talk about
'true' and 'false', not '1' or '0'.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/config.txt | 4 ++++
>  cache.h                  | 1 +
>  config.c                 | 5 +++++
>  environment.c            | 1 +
>  4 files changed, 11 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4e0cff87f6..e5c7013fb0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -898,6 +898,10 @@ core.notesRef::
>  This setting defaults to "refs/notes/commits", and it can be overridden by
>  the `GIT_NOTES_REF` environment variable.  See linkgit:git-notes[1].
>  
> +core.commitGraph::
> +	Enable git commit graph feature. Allows reading from the
> +	commit-graph file.
> +

This is a very minimal description of this config variable.  In my
opionion it lack two things:

1. The reference to the documentation where one can read more, for
   example linkgit:git-commit-graph[1] manpage, like in the description
   of core.sparseCheckout feature described below.

2. The information about restrictions, for example something like the
   following:

  "The feature does not work for shallow clones, neither when
  `git-replace` or grafts file are used."

   Perhaps even with references to each commit-graph disabling feature.

When the feature matures, and it will be on by default (well, the
reading part at least), it would probably acquire wording similar to the
one for `pack.useBitmaps` config option[1], isn't it?

[1]: https://git-scm.com/docs/git-config#git-config-packuseBitmaps

>  core.sparseCheckout::
>  	Enable "sparse checkout" feature. See section "Sparse checkout" in
>  	linkgit:git-read-tree[1] for more information.
> diff --git a/cache.h b/cache.h
> index a61b2d3f0d..8bdbcbbbf7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -805,6 +805,7 @@ extern char *git_replace_ref_base;
>  
>  extern int fsync_object_files;
>  extern int core_preload_index;
> +extern int core_commit_graph;
>  extern int core_apply_sparse_checkout;
>  extern int precomposed_unicode;
>  extern int protect_hfs;
> diff --git a/config.c b/config.c
> index b0c20e6cb8..25ee4a676c 100644
> --- a/config.c
> +++ b/config.c
> @@ -1226,6 +1226,11 @@ static int git_default_core_config(const char *var, const char *value)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "core.commitgraph")) {
> +		core_commit_graph = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "core.sparsecheckout")) {
>  		core_apply_sparse_checkout = git_config_bool(var, value);
>  		return 0;
> diff --git a/environment.c b/environment.c
> index d6dd64662c..8853e2f0dd 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -62,6 +62,7 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
>  enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
>  char *notes_ref_name;
>  int grafts_replace_parents = 1;
> +int core_commit_graph;
>  int core_apply_sparse_checkout;
>  int merge_log_config = -1;
>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */

So this is just a config variable handling.  Nicely short patch to
review; the code part looks good to me.

-- 
Jakub Narębski