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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Aug 20, 2019 at 2:38 PM Garima Singh via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.

> 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.

>  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.

> @@ -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.

> 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.

>         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.

> 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.