threads / patch / 51693

patchcommit-graph: add --[no-]progress to write and verify

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

## tl;dr

13 messages between Aug 20, 2019 and Sep 17, 2019. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Garima Singh via GitGitGadget· Aug 20, 2019, 18:37 UTC · lore
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 |  4 +++-
 builtin/commit-graph.c             | 29 +++++++++++++++++-------
 commit-graph.c                     |  7 ++++--
 t/t5318-commit-graph.sh            | 36 ++++++++++++++++++++++++++++++
 t/t5324-split-commit-graph.sh      |  2 +-
 5 files changed, 66 insertions(+), 12 deletions(-)
base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/315
-- 
gitgitgadget
Garima Singh via GitGitGadget· Aug 20, 2019, 18:37 UTC · re: Garima Singh via GitGitGadget · lore

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

From: Garima Singh <garima.singh@microsoft.com>

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>
---
 Documentation/git-commit-graph.txt |  4 +++-
 builtin/commit-graph.c             | 29 +++++++++++++++++-------
 commit-graph.c                     |  7 ++++--
 t/t5318-commit-graph.sh            | 36 ++++++++++++++++++++++++++++++
 t/t5324-split-commit-graph.sh      |  2 +-
 5 files changed, 66 insertions(+), 12 deletions(-)
Show changes to 5 files +66 −12

Documentation/git-commit-graph.txt, builtin/commit-graph.c, commit-graph.c, t/t5318-commit-graph.sh, t/t5324-split-commit-graph.sh

diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
index eb5e7865f0..1256a80a81 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 '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>]
 
 
@@ -29,6 +29,8 @@ 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::
+	Toggle whether to show progress or not.
 
 COMMANDS
 --------
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 38027b83d9..71796910fc 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -6,17 +6,18 @@
 #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>]"),
-	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>"),
 	NULL
 };
 
 static const char * const builtin_commit_graph_verify_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow]"),
+	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
 	NULL
 };
 
@@ -26,7 +27,7 @@ static const char * const builtin_commit_graph_read_usage[] = {
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] <split options>"),
+	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
 	NULL
 };
 
@@ -38,6 +39,7 @@ static struct opts_commit_graph {
 	int append;
 	int split;
 	int shallow;
+	int progress;
 } opts;
 
 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);
+	
 	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")),
+		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_END(),
 	};
 
+	opts.progress = defaultProgressState;
+	
 	argc = parse_options(argc, argv, NULL,
 			     builtin_commit_graph_verify_options,
 			     builtin_commit_graph_verify_usage, 0);
@@ -66,7 +72,9 @@ static int graph_verify(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 	if (opts.shallow)
 		flags |= COMMIT_GRAPH_VERIFY_SHALLOW;
-
+	if (opts.progress)
+		flags |= COMMIT_GRAPH_PROGRESS;
+	
 	graph_name = get_commit_graph_filename(opts.obj_dir);
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok && errno != ENOENT)
@@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv)
 	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);
+	
 	static struct option builtin_commit_graph_write_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
 			N_("dir"),
@@ -168,6 +177,7 @@ static int graph_write(int argc, const char **argv)
 			N_("start walk at commits listed by stdin")),
 		OPT_BOOL(0, "append", &opts.append,
 			N_("include all commits already in the commit-graph file")),
+		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_BOOL(0, "split", &opts.split,
 			N_("allow writing an incremental commit-graph file")),
 		OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
@@ -179,6 +189,7 @@ static int graph_write(int argc, const char **argv)
 		OPT_END(),
 	};
 
+	opts.progress = defaultProgressState;
 	split_opts.size_multiple = 2;
 	split_opts.max_commits = 0;
 	split_opts.expire_time = 0;
@@ -195,6 +206,8 @@ static int graph_write(int argc, const char **argv)
 		flags |= COMMIT_GRAPH_APPEND;
 	if (opts.split)
 		flags |= COMMIT_GRAPH_SPLIT;
+	if (opts.progress)
+		flags |= COMMIT_GRAPH_PROGRESS;
 
 	read_replace_refs = 0;
 
diff --git a/commit-graph.c b/commit-graph.c
index fe954ab5f8..b10d47f99a 100644
--- 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);
+
 	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
index 22cb9d6643..a98d9f88f4 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -116,6 +116,42 @@ test_expect_success 'Add more commits' '
 	git repack
 '
 
+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
+'
+
+test_expect_success 'commit-graph write force progress on for stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph write --progress 2>err &&
+	test_file_not_empty err
+'
+
+test_expect_success 'commit-graph write with the --no-progress option' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph write --no-progress 2>err &&
+	test_line_count = 0 err
+'
+
+test_expect_success 'commit-graph verify progress off by default for stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph verify 2>err &&
+	test_line_count = 0 err
+'
+
+test_expect_success 'commit-graph verify force progress on for stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph verify --progress 2>err &&
+	test_file_not_empty err
+'
+
+test_expect_success 'commit-graph verify with the --no-progress option' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph verify --no-progress 2>err &&
+	test_line_count = 0 err
+'
+
 # Current graph structure:
 #
 #   __M3___
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 99f4ef4c19..4fc3fda9d6 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '
 	git merge commits/3 commits/4 &&
 	git branch merge/octopus &&
 	git commit-graph write --reachable --split &&
-	git commit-graph verify 2>err &&
+	git commit-graph verify --progress 2>err &&
 	test_line_count = 3 err &&
 	test_i18ngrep ! warning err &&
 	test_line_count = 3 $graphdir/commit-graph-chain
-- 
gitgitgadget
Junio C Hamano· Aug 20, 2019, 21:11 UTC · re: Garima Singh via GitGitGadget · lore

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

"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> From: Garima Singh <garima.singh@microsoft.com>
>
> 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.
Nicely described.
Show 9 quoted lines
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 38027b83d9..71796910fc 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -6,17 +6,18 @@
>  #include "repository.h"
>  #include "commit-graph.h"
>  #include "object-store.h"
> +#include "unistd.h"
Please do not contaminate *.c files with #include of system headers.

Often, various platforms require system include files in specific order, and the project convention is to include them in git-compat-util.h in the right order (with #ifdef and friends as necessary). *.c files are required to include git-compat-util.h (or one of the well known headers that include git-compat-util.h as the first one) as the first file.

In fact, "builtin.h" includes "git-compat-util.h" as the first thing, and "git-compat-util.h" in turn includes unistd reasonably early. Do you really need to include it again here?

Show 6 quoted lines
> @@ -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);

As you can see from the naming of other variables, we do not do camelCase variable names.

In fact you do not need this variable, do you?
Show 11 quoted lines
>  	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")),
> +		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
>  		OPT_END(),
>  	};
>  
> +	opts.progress = defaultProgressState;
... as you can assign isatty(2) to opts.progress here directly.
Show 8 quoted lines
> @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv)
>  	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);
Likewise.
Show 13 quoted lines
> diff --git a/commit-graph.c b/commit-graph.c
> index fe954ab5f8..b10d47f99a 100644
> --- 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);
Makes sense.
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);
Drop this change---I do not see a reason for the extra blank line here.
Eric Sunshine· Aug 20, 2019, 21:13 UTC · re: Garima Singh via GitGitGadget · lore

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

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.

Junio C Hamano· Aug 21, 2019, 16:47 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
> This synopsis shows only 'verify' accepting --[no-]progress,
> however, ...
> This is misleading. The --progress option does not _toggle_ the
> setting. ...
Thanks for a careful review.
Derrick Stolee· Aug 20, 2019, 18:45 UTC · re: Garima Singh via GitGitGadget · lore

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

On 8/20/2019 2:37 PM, Garima Singh via GitGitGadget wrote:
Show 8 quoted lines
> 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. 

I just wanted to chime in and introduce Garima a bit myself. Garima and I were both on the Git Server team for Azure Repos and left that team around the same time. She went and did things in Azure Pipelines before leaving to pursue more education and returning to our current team working on the Git ecosystem. Garima will be focused mostly on core Git, so get used to seeing her on the list!

She's starting with a couple smaller series to get her feet wet, but then is working on some deep dives into performance features. Look forward to those!

> CC: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com
GitGitGadget is picky about the casing of "Cc:" so I have CC'd these people.

Thanks, -Stolee

Garima Singh via GitGitGadget· Aug 26, 2019, 16:29 UTC · re: Garima Singh via GitGitGadget · lore

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

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
-- 
gitgitgadget
Garima Singh via GitGitGadget· Aug 26, 2019, 16:29 UTC · re: Garima Singh via GitGitGadget · lore

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

From: Garima Singh <garima.singh@microsoft.com>

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>
---
 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(-)
Show changes to 5 files +61 −11

Documentation/git-commit-graph.txt, builtin/commit-graph.c, commit-graph.c, t/t5318-commit-graph.sh, t/t5324-split-commit-graph.sh

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]
 
 
 DESCRIPTION
@@ -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 
+	shown if standard error is connected to a terminal.
 
 COMMANDS
 --------
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 57863619b7..faf349a6c1 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -10,13 +10,13 @@
 static char const * const builtin_commit_graph_usage[] = {
 	N_("git commit-graph [--object-dir <objdir>]"),
 	N_("git commit-graph read [--object-dir <objdir>]"),
-	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>"),
 	NULL
 };
 
 static const char * const builtin_commit_graph_verify_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow]"),
+	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
 	NULL
 };
 
@@ -26,7 +26,7 @@ static const char * const builtin_commit_graph_read_usage[] = {
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] <split options>"),
+	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
 	NULL
 };
 
@@ -38,6 +38,7 @@ static struct opts_commit_graph {
 	int append;
 	int split;
 	int shallow;
+	int progress;
 } opts;
 
 static int graph_verify(int argc, const char **argv)
@@ -55,9 +56,11 @@ static int graph_verify(int argc, const char **argv)
 			   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")),
+		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_END(),
 	};
 
+	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     builtin_commit_graph_verify_options,
 			     builtin_commit_graph_verify_usage, 0);
@@ -66,7 +69,9 @@ static int graph_verify(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 	if (opts.shallow)
 		flags |= COMMIT_GRAPH_VERIFY_SHALLOW;
-
+	if (opts.progress)
+		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
+	
 	graph_name = get_commit_graph_filename(opts.obj_dir);
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok && errno != ENOENT)
@@ -154,7 +159,7 @@ static int graph_write(int argc, const char **argv)
 	struct string_list *commit_hex = NULL;
 	struct string_list lines;
 	int result = 0;
-	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,
@@ -168,6 +173,7 @@ static int graph_write(int argc, const char **argv)
 			N_("start walk at commits listed by stdin")),
 		OPT_BOOL(0, "append", &opts.append,
 			N_("include all commits already in the commit-graph file")),
+		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_BOOL(0, "split", &opts.split,
 			N_("allow writing an incremental commit-graph file")),
 		OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
@@ -179,6 +185,7 @@ static int graph_write(int argc, const char **argv)
 		OPT_END(),
 	};
 
+	opts.progress = isatty(2);
 	split_opts.size_multiple = 2;
 	split_opts.max_commits = 0;
 	split_opts.expire_time = 0;
@@ -195,6 +202,8 @@ static int graph_write(int argc, const char **argv)
 		flags |= COMMIT_GRAPH_WRITE_APPEND;
 	if (opts.split)
 		flags |= COMMIT_GRAPH_WRITE_SPLIT;
+	if (opts.progress)
+		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
 
 	read_replace_refs = 0;
 
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);
+
 	for (i = 0; i < g->num_commits; i++) {
 		struct commit *graph_commit, *odb_commit;
 		struct commit_list *graph_parents, *odb_parents;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ab3eccf0fa..df3fed3a08 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -124,6 +124,42 @@ test_expect_success 'Add more commits' '
 	git repack
 '
 
+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
+'
+
+test_expect_success 'commit-graph write force progress on for stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph write --progress 2>err &&
+	test_file_not_empty err
+'
+
+test_expect_success 'commit-graph write with the --no-progress option' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph write --no-progress 2>err &&
+	test_line_count = 0 err
+'
+
+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
+'
+
+test_expect_success 'commit-graph verify force progress on for stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph verify --progress 2>err &&
+	test_file_not_empty err
+'
+
+test_expect_success 'commit-graph verify with the --no-progress option' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git commit-graph verify --no-progress 2>err &&
+	test_line_count = 0 err
+'
+
 # Current graph structure:
 #
 #   __M3___
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 99f4ef4c19..4fc3fda9d6 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '
 	git merge commits/3 commits/4 &&
 	git branch merge/octopus &&
 	git commit-graph write --reachable --split &&
-	git commit-graph verify 2>err &&
+	git commit-graph verify --progress 2>err &&
 	test_line_count = 3 err &&
 	test_i18ngrep ! warning err &&
 	test_line_count = 3 $graphdir/commit-graph-chain
-- 
gitgitgadget
Junio C Hamano· Sep 12, 2019, 20:40 UTC · re: Garima Singh via GitGitGadget · lore

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

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

SZEDER Gábor· Sep 16, 2019, 22:36 UTC · re: Garima Singh via GitGitGadget · lore

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

On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:
Show 6 quoted lines
> From: Garima Singh <garima.singh@microsoft.com>
> 
> 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.
Show 10 quoted lines
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 99f4ef4c19..4fc3fda9d6 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '
>  	git merge commits/3 commits/4 &&
>  	git branch merge/octopus &&
>  	git commit-graph write --reachable --split &&
> -	git commit-graph verify 2>err &&
> +	git commit-graph verify --progress 2>err &&

Why is it necessary to use '--progress' here? It should not be necessary, because the commit message doesn't mention that it changed the default behavior of 'git commit-graph verify'...

>  	test_line_count = 3 err &&

Having said that, this test should not check the number of progress lines in the first place; see the recent discussion:

https://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/
>  	test_i18ngrep ! warning err &&
>  	test_line_count = 3 $graphdir/commit-graph-chain
> -- 
> gitgitgadget
Derrick Stolee· Sep 17, 2019, 10:47 UTC · re: SZEDER Gábor · lore

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

On 9/16/2019 6:36 PM, SZEDER Gábor wrote:
Show 22 quoted lines
> On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:
>> From: Garima Singh <garima.singh@microsoft.com>
>>
>> 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.
> 
>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
>> index 99f4ef4c19..4fc3fda9d6 100755
>> --- a/t/t5324-split-commit-graph.sh
>> +++ b/t/t5324-split-commit-graph.sh
>> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '
>>  	git merge commits/3 commits/4 &&
>>  	git branch merge/octopus &&
>>  	git commit-graph write --reachable --split &&
>> -	git commit-graph verify 2>err &&
>> +	git commit-graph verify --progress 2>err &&
> 
> Why is it necessary to use '--progress' here?  It should not be
> necessary, because the commit message doesn't mention that it changed
> the default behavior of 'git commit-graph verify'...
It does change the default when stderr is not a terminal window. If we
were not redirecting to a file, this change would not be necessary.
 
Show 6 quoted lines
>>  	test_line_count = 3 err &&
> 
> Having said that, this test should not check the number of progress
> lines in the first place; see the recent discussion:
> 
> https://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/

True, this is an old issue. I think it never got corrected because your reply sounded like the issue doesn't exist in the normal test suite, only in a private branch where you changed the behavior of GIT_TEST_GETTEXT_POISON.

If we still think that should be fixed, it should not be a part of this series, but should be a separate one that focuses on just those changes.

Thanks, -Stolee

SZEDER Gábor· Sep 17, 2019, 12:22 UTC · re: Derrick Stolee · lore

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

On Tue, Sep 17, 2019 at 06:47:38AM -0400, Derrick Stolee wrote:
Show 27 quoted lines
> 
> On 9/16/2019 6:36 PM, SZEDER Gábor wrote:
> > On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:
> >> From: Garima Singh <garima.singh@microsoft.com>
> >>
> >> 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.
> > 
> >> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> >> index 99f4ef4c19..4fc3fda9d6 100755
> >> --- a/t/t5324-split-commit-graph.sh
> >> +++ b/t/t5324-split-commit-graph.sh
> >> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '
> >>  	git merge commits/3 commits/4 &&
> >>  	git branch merge/octopus &&
> >>  	git commit-graph write --reachable --split &&
> >> -	git commit-graph verify 2>err &&
> >> +	git commit-graph verify --progress 2>err &&
> > 
> > Why is it necessary to use '--progress' here?  It should not be
> > necessary, because the commit message doesn't mention that it changed
> > the default behavior of 'git commit-graph verify'...
> 
> It does change the default when stderr is not a terminal window. If we
> were not redirecting to a file, this change would not be necessary.
OK, yesterday I overlooked that the patch added this line:
  +       opts.progress = isatty(2);

So, the first question is whether that behavior change is desired; I don't really have an opinion. But if it is desired, then it should be changed in a separate patch, explaining why it is desired, I would think.

Show 10 quoted lines
> >>  	test_line_count = 3 err &&
> > 
> > Having said that, this test should not check the number of progress
> > lines in the first place; see the recent discussion:
> > 
> > https://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/
> 
> True, this is an old issue. I think it never got corrected because
> your reply sounded like the issue doesn't exist in the normal test
> suite,

Well, the way I see it the root issue is that the test checks things that it shouldn't.

Show 6 quoted lines
> only in a private branch where you changed the behavior of
> GIT_TEST_GETTEXT_POISON.
> 
> If we still think that should be fixed, it should not be a part of
> this series, but should be a separate one that focuses on just
> those changes.
Yeah, it should rather go on top of 'ds/commit-graph-octopus-fix'.
Garima Singh· Sep 10, 2019, 14:00 UTC · re: Garima Singh via GitGitGadget · lore

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

Ping :) Any more comments or concerns about this?
On 8/26/2019 12:29 PM, Garima Singh via GitGitGadget wrote:
Show 189 quoted lines
> 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
> 

← back to recent threads