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

[Garima Singh <garimasigit@gmail.com>]

Ping :) Any more comments or concerns about this?

On 8/26/2019 12:29 PM, Garima Singh via GitGitGadget wrote:
> Hey Git contributors!
> 
> My name is Garima Singh and I work at Microsoft. I recently started working
> closely with the Microsoft team contributing to the git client ecosystem. I
> am very glad to have the opportunity to work with this community. I am new
> to the world of git client development but I did work on the Git service
> offering of Azure Developer Services for a few years. I am sure I will get
> to learn a lot from all of you.
> 
> Dr. Derrick Stolee helped me pick out my first task (Thanks Stolee!) He
> mentioned an issue in the commit-graph builtin where git did not support
> opting in and out of the progress output. This was bloating up the stderr
> logs in VFS for Git. The progress feature was introduced in 7b0f229222
> ("commit-graph write: add progress output", 2018-09-17) but the ability to
> opt-out was overlooked. This patch adds the --no-progress option so that
> callers can control the amount of logging they receive.
> 
> Looking forward to your review. Cheers! Garima Singh
> 
> CC: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com
> 
> Garima Singh (1):
>    commit-graph: add --[no-]progress to write and verify.
> 
>   Documentation/git-commit-graph.txt |  7 ++++--
>   builtin/commit-graph.c             | 21 ++++++++++++-----
>   commit-graph.c                     |  6 +++--
>   t/t5318-commit-graph.sh            | 36 ++++++++++++++++++++++++++++++
>   t/t5324-split-commit-graph.sh      |  2 +-
>   5 files changed, 61 insertions(+), 11 deletions(-)
> 
> 
> base-commit: 745f6812895b31c02b29bdfe4ae8e5498f776c26
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/315
> 
> Range-diff vs v1:
> 
>   1:  da89f7dadb ! 1:  47cc99bd15 commit-graph: add --[no-]progress to write and verify.
>       @@ -17,16 +17,19 @@
>         [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>]
>       ++'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress]
>         
>         
>       + DESCRIPTION
>        @@
>         	commit-graph file is expected to be in the `<dir>/info` directory and
>         	the packfiles are expected to be in `<dir>/pack`.
>         
>        +--[no-]progress::
>       -+	Toggle whether to show progress or not.
>       ++	Turn progress on/off explicitly. If neither is specified, progress is
>       ++	shown if standard error is connected to a terminal.
>         
>         COMMANDS
>         --------
>       @@ -35,11 +38,6 @@
>         --- a/builtin/commit-graph.c
>         +++ b/builtin/commit-graph.c
>        @@
>       - #include "repository.h"
>       - #include "commit-graph.h"
>       - #include "object-store.h"
>       -+#include "unistd.h"
>       -
>         static char const * const builtin_commit_graph_usage[] = {
>         	N_("git commit-graph [--object-dir <objdir>]"),
>         	N_("git commit-graph read [--object-dir <objdir>]"),
>       @@ -74,15 +72,6 @@
>         
>         static int graph_verify(int argc, const char **argv)
>        @@
>       - 	int fd;
>       - 	struct stat st;
>       - 	int flags = 0;
>       --
>       -+	int defaultProgressState = isatty(2);
>       -+	
>       - 	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")),
>       @@ -90,8 +79,7 @@
>         		OPT_END(),
>         	};
>         
>       -+	opts.progress = defaultProgressState;
>       -+	
>       ++	opts.progress = isatty(2);
>         	argc = parse_options(argc, argv, NULL,
>         			     builtin_commit_graph_verify_options,
>         			     builtin_commit_graph_verify_usage, 0);
>       @@ -101,7 +89,7 @@
>         		flags |= COMMIT_GRAPH_VERIFY_SHALLOW;
>        -
>        +	if (opts.progress)
>       -+		flags |= COMMIT_GRAPH_PROGRESS;
>       ++		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
>        +	
>         	graph_name = get_commit_graph_filename(opts.obj_dir);
>         	open_ok = open_commit_graph(graph_name, &fd, &st);
>       @@ -110,14 +98,11 @@
>         	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);
>       -+	
>       +-	enum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;
>       ++	enum commit_graph_write_flags flags = 0;
>       +
>         	static struct option builtin_commit_graph_write_options[] = {
>         		OPT_STRING(0, "object-dir", &opts.obj_dir,
>       - 			N_("dir"),
>        @@
>         			N_("start walk at commits listed by stdin")),
>         		OPT_BOOL(0, "append", &opts.append,
>       @@ -130,16 +115,16 @@
>         		OPT_END(),
>         	};
>         
>       -+	opts.progress = defaultProgressState;
>       ++	opts.progress = isatty(2);
>         	split_opts.size_multiple = 2;
>         	split_opts.max_commits = 0;
>         	split_opts.expire_time = 0;
>        @@
>       - 		flags |= COMMIT_GRAPH_APPEND;
>       + 		flags |= COMMIT_GRAPH_WRITE_APPEND;
>         	if (opts.split)
>       - 		flags |= COMMIT_GRAPH_SPLIT;
>       + 		flags |= COMMIT_GRAPH_WRITE_SPLIT;
>        +	if (opts.progress)
>       -+		flags |= COMMIT_GRAPH_PROGRESS;
>       ++		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
>         
>         	read_replace_refs = 0;
>         
>       @@ -153,20 +138,13 @@
>         
>        -	progress = start_progress(_("Verifying commits in commit graph"),
>        -				  g->num_commits);
>       -+	if (flags & COMMIT_GRAPH_PROGRESS)
>       ++	if (flags & COMMIT_GRAPH_WRITE_PROGRESS)
>        +		progress = start_progress(_("Verifying commits in commit graph"),
>        +					g->num_commits);
>        +
>         	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);
>       -
>       - 		graph_commit = lookup_commit(r, &cur_oid);
>        
>         diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>         --- a/t/t5318-commit-graph.sh
>       @@ -175,7 +153,7 @@
>         	git repack
>         '
>         
>       -+test_expect_success 'commit-graph write progress off by default for stderr' '
>       ++test_expect_success 'commit-graph write progress off for redirected stderr' '
>        +	cd "$TRASH_DIRECTORY/full" &&
>        +	git commit-graph write 2>err &&
>        +	test_line_count = 0 err
>       @@ -193,7 +171,7 @@
>        +	test_line_count = 0 err
>        +'
>        +
>       -+test_expect_success 'commit-graph verify progress off by default for stderr' '
>       ++test_expect_success 'commit-graph verify progress off for redirected stderr' '
>        +	cd "$TRASH_DIRECTORY/full" &&
>        +	git commit-graph verify 2>err &&
>        +	test_line_count = 0 err
>