Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 12, 2019, 20:40 UTC
- Message-ID
- <xmqqef0lfdt4.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <47cc99bd151db67fe2ee0f91bb98b3eb7e55786d.1566836997.git.gitgitgadget@gmail.com>
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 12 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.
Show 5 quoted lines
> + shown if standard error is connected to a terminal. > ... > + if (opts.progress) > + flags |= COMMIT_GRAPH_WRITE_PROGRESS; > +
Trailing whitespace.
Show 14 quoted lines
> 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.