Re: [PATCH 1/1] commit-graph: add --[no-]progress to write and verify.
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Aug 20, 2019, 21:13 UTC
- Message-ID
- <CAPig+cSR-ab_8AeZ9fJX2G8h6x_V_NUG01pXWQAgF+_pgmR2fQ@mail.gmail.com>
- In-Reply-To
- <da89f7dadb0be2d4ada22dd3e2d1f5524c73f70d.1566326275.git.gitgitgadget@gmail.com>
On Tue, Aug 20, 2019 at 2:38 PM Garima Singh via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 13 quoted lines
> 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.
>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
> 'git commit-graph read' [--object-dir <dir>]
> -'git commit-graph verify' [--object-dir <dir>] [--shallow]
> +'git commit-graph verify' [--object-dir <dir>] [--shallow] [--[no-]progress]
> 'git commit-graph write' <options> [--object-dir <dir>]This synopsis shows only 'verify' accepting --[no-]progress, however, the "usage" message in commit-graph.c itself shows 'verify' and 'write' accepting the option.
> @@ -29,6 +29,8 @@ OPTIONS > +--[no-]progress:: > + Toggle whether to show progress or not.
This is misleading. The --progress option does not _toggle_ the setting. The positive form explicitly enables it, and the negated form explicitly disables it. Try to take inspiration for the wording of this description by consulting other existing documentation. For instance, from Documentation/merge-options.txt:
Turn progress on/off explicitly. If neither is specified,
progress is shown if standard error is connected to a terminal.Show 7 quoted lines
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> @@ -6,17 +6,18 @@
> static char const * const builtin_commit_graph_usage[] = {
> - N_("git commit-graph verify [--object-dir <objdir>] [--shallow]"),
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] <split options>"),
> + N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),This disagrees with the synopsis in the documentation, as mentioned above.
Show 8 quoted lines
> static int graph_verify(int argc, const char **argv) > @@ -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); > +
You have stray whitespace at the end of the "blank" line following the new variable declaration, hence the odd-looking diff.
Show 6 quoted lines
> @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv) > - unsigned int flags = COMMIT_GRAPH_PROGRESS; > - > + unsigned int flags = 0; > + int defaultProgressState = isatty(2); > +
Ditto.
Show 10 quoted lines
> diff --git 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);The progress reporting functions can safely handle a NULL pointer for 'progress'. In the original code 'progress' was assigned explicitly, thus could not be NULL, However, in the revised code, it's not clear from this snippet what the value of 'progress' is if COMMIT_GRAPH_PROGRESS is not in 'flags'. If 'progress' is uninitialized, then the behavior would be undefined. Looking at the variable declaration, I see that it is indeed initialized to NULL, so this code is safe. Okay.
Show 8 quoted lines
> 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);No need to make changes (such as inserting an unnecessary blank line) unrelated to the stated purposed of the patch.
Show 7 quoted lines
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > @@ -116,6 +116,42 @@ test_expect_success 'Add more commits' ' > +test_expect_success 'commit-graph write progress off by default for stderr' ' > + cd "$TRASH_DIRECTORY/full" && > + git commit-graph write 2>err && > + test_line_count = 0 err > +'
Changing the working directory ('cd') outside of a subshell is heavily discouraged in this test suite, however, since this particular script is riddled with the 'cd "$TRASH_DIRECTORY/full"' idiom, this can probably pass, however, it's not good to get into the habit of doing it this way.