Re: [PATCH v2 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:

> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index eb5e7865f0..ca0b1a683f 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,8 +10,8 @@ SYNOPSIS
>  --------
>  [verse]
>  'git commit-graph read' [--object-dir <dir>]
> -'git commit-graph verify' [--object-dir <dir>] [--shallow]
> -'git commit-graph write' <options> [--object-dir <dir>]
> +'git commit-graph verify' [--object-dir <dir>] [--shallow] [--[no-]progress]
> +'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress]

This is not a problem with this patch, but it is disturbing to see
<options> and other concrete "--option" listed explicitly.  It could
be that "--object-dir <dir>" is so important an option that deserves
to be singled out while other random options can be left to individual
option's description, but in that case, would "--progress" be equally
important (if anything, as an option that is purely about appearance,
I would expect it to be with a lot lower importance)?

I guess with a preparatory clean-up patch to deal with the <options>
part, the result of applying this patch would not look so bad.
Perhaps renaming <options> to <write-specific-options> and moving it
to the end of the line might be sufficient.  I dunno.  At least we'd
need to make sure that it is clear to readers what options are
allowed where we wrote <options> above.

> @@ -29,6 +29,9 @@ OPTIONS
>  	commit-graph file is expected to be in the `<dir>/info` directory and
>  	the packfiles are expected to be in `<dir>/pack`.
>  
> +--[no-]progress::
> +	Turn progress on/off explicitly. If neither is specified, progress is 

Trailing whitespace.

> +	shown if standard error is connected to a terminal.
>   ...
> +	if (opts.progress)
> +		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
> +	

Trailing whitespace.

> diff --git a/commit-graph.c b/commit-graph.c
> index f2888c203b..2802f2ade6 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1992,8 +1992,10 @@ 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_WRITE_PROGRESS)
> +		progress = start_progress(_("Verifying commits in commit graph"),
> +					g->num_commits);
> +

This is correct, but it feels funny that it is sufficient to
castrate start_progress() and we do not have to muck with existing
calls to show and stop progress output.  We rely on progress being
NULL for that to work, and existing code initializes the variable
to NULL, so we are OK.