Re: [PATCH v8 09/14] commit-graph: add core.commitGraph setting
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Apr 14, 2018, 18:33 UTC
- Message-ID
- <86h8odljso.fsf@gmail.com>
- In-Reply-To
- <20180410125608.39443-10-dstolee@microsoft.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 7 quoted lines
> 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'.
Show 20 quoted lines
> 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
Show 43 quoted lines
> 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