Re: [PATCH 1/1] commit-graph: add --[no-]progress to write and verify.

[Junio C Hamano <gitster@pobox.com>]

"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Garima Singh <garima.singh@microsoft.com>
>
> Add --[no-]progress to git commit-graph write and verify.
> The progress feature was introduced in 7b0f229
> ("commit-graph write: add progress output", 2018-09-17) but
> the ability to opt-out was overlooked.

Nicely described.

> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 38027b83d9..71796910fc 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -6,17 +6,18 @@
>  #include "repository.h"
>  #include "commit-graph.h"
>  #include "object-store.h"
> +#include "unistd.h"

Please do not contaminate *.c files with #include of system headers.

Often, various platforms require system include files in specific
order, and the project convention is to include them in
git-compat-util.h in the right order (with #ifdef and friends as
necessary).  *.c files are required to include git-compat-util.h (or
one of the well known headers that include git-compat-util.h as the
first one) as the first file.

In fact, "builtin.h" includes "git-compat-util.h" as the first
thing, and "git-compat-util.h" in turn includes unistd reasonably
early.  Do you really need to include it again here?

> @@ -48,16 +50,20 @@ static int graph_verify(int argc, const char **argv)
>  	int fd;
>  	struct stat st;
>  	int flags = 0;
> -
> +	int defaultProgressState = isatty(2);

As you can see from the naming of other variables, we do not do
camelCase variable names.

In fact you do not need this variable, do you?

>  	static struct option builtin_commit_graph_verify_options[] = {
>  		OPT_STRING(0, "object-dir", &opts.obj_dir,
>  			   N_("dir"),
>  			   N_("The object directory to store the graph")),
>  		OPT_BOOL(0, "shallow", &opts.shallow,
>  			 N_("if the commit-graph is split, only verify the tip file")),
> +		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
>  		OPT_END(),
>  	};
>  
> +	opts.progress = defaultProgressState;

... as you can assign isatty(2) to opts.progress here directly.

> @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv)
>  	struct string_list *commit_hex = NULL;
>  	struct string_list lines;
>  	int result = 0;
> -	unsigned int flags = COMMIT_GRAPH_PROGRESS;
> -
> +	unsigned int flags = 0;
> +	int defaultProgressState = isatty(2);

Likewise.

> diff --git a/commit-graph.c b/commit-graph.c
> index fe954ab5f8..b10d47f99a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1986,14 +1986,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
>  	if (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)
>  		return verify_commit_graph_error;
>  
> -	progress = start_progress(_("Verifying commits in commit graph"),
> -				  g->num_commits);
> +	if (flags & COMMIT_GRAPH_PROGRESS)
> +		progress = start_progress(_("Verifying commits in commit graph"),
> +					g->num_commits);

Makes sense.

>  	for (i = 0; i < g->num_commits; i++) {
>  		struct commit *graph_commit, *odb_commit;
>  		struct commit_list *graph_parents, *odb_parents;
>  		uint32_t max_generation = 0;
>  
>  		display_progress(progress, i + 1);
> +
>  		hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);

Drop this change---I do not see a reason for the extra blank line here.