{"thread":{"id":"51693","subject":"[PATCH 0/1] commit-graph: add --[no-]progress to write and verify","startedAt":"2019-08-20T18:37:59Z","lastAt":"2019-09-17T12:22:24Z","messageCount":13,"participants":["Garima Singh via GitGitGadget","Derrick Stolee","Junio C Hamano","Eric Sunshine","Garima Singh","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"380830","messageId":"pull.315.git.gitgitgadget@gmail.com","threadId":"51693","inReplyTo":null,"subject":"[PATCH 0/1] commit-graph: add --[no-]progress to write and verify","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-20T18:37:55Z","receivedAt":"2019-08-20T18:37:59Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey Git contributors! \n\nMy name is Garima Singh and I work at Microsoft. I recently started working\nclosely with the Microsoft team contributing to the git client ecosystem. I\nam very glad to have the opportunity to work with this community. I am new\nto the world of git client development but I did work on the Git service\noffering of Azure Developer Services for a few years. I am sure I will get\nto learn a lot from all of you. \n\nDr. Derrick Stolee helped me pick out my first task (Thanks Stolee!) He\nmentioned an issue in the commit-graph builtin where git did not support\nopting in and out of the progress output. This was bloating up the stderr\nlogs in VFS for Git. The progress feature was introduced in 7b0f229222\n(\"commit-graph write: add progress output\", 2018-09-17) but the ability to\nopt-out was overlooked. This patch adds the --no-progress option so that\ncallers can control the amount of logging they receive. \n\nLooking forward to your review. Cheers! Garima Singh\n\nCC: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com\n\nGarima Singh (1):\n commit-graph: add --[no-]progress to write and verify.\n\n Documentation/git-commit-graph.txt | 4 +++-\n builtin/commit-graph.c | 29 +++++++++++++++++-------\n commit-graph.c | 7 ++++--\n t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++\n t/t5324-split-commit-graph.sh | 2 +-\n 5 files changed, 66 insertions(+), 12 deletions(-)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/315\n-- \ngitgitgadget\n"},{"id":"380831","messageId":"da89f7dadb0be2d4ada22dd3e2d1f5524c73f70d.1566326275.git.gitgitgadget@gmail.com","threadId":"51693","inReplyTo":"pull.315.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-20T18:37:56Z","receivedAt":"2019-08-20T18:38:01Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nAdd --[no-]progress to git commit-graph write and verify.\nThe progress feature was introduced in 7b0f229\n(\"commit-graph write: add progress output\", 2018-09-17) but\nthe ability to opt-out was overlooked.\n\nSigned-off-by: Garima Singh \n---\n Documentation/git-commit-graph.txt | 4 +++-\n builtin/commit-graph.c | 29 +++++++++++++++++-------\n commit-graph.c | 7 ++++--\n t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++\n t/t5324-split-commit-graph.sh | 2 +-\n 5 files changed, 66 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex eb5e7865f0..1256a80a81 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git commit-graph read' [--object-dir ]\n-'git commit-graph verify' [--object-dir ] [--shallow]\n+'git commit-graph verify' [--object-dir ] [--shallow] [--[no-]progress]\n 'git commit-graph write' [--object-dir ]\n \n \n@@ -29,6 +29,8 @@ OPTIONS\n \tcommit-graph file is expected to be in the `/info` directory and\n \tthe packfiles are expected to be in `/pack`.\n \n+--[no-]progress::\n+\tToggle whether to show progress or not.\n \n COMMANDS\n --------\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 38027b83d9..71796910fc 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -6,17 +6,18 @@\n #include \"repository.h\"\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n+#include \"unistd.h\"\n \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph [--object-dir ]\"),\n \tN_(\"git commit-graph read [--object-dir ]\"),\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow]\"),\n-\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] \"),\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n+\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n \tNULL\n };\n \n static const char * const builtin_commit_graph_verify_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow]\"),\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n \tNULL\n };\n \n@@ -26,7 +27,7 @@ static const char * const builtin_commit_graph_read_usage[] = {\n };\n \n static const char * const builtin_commit_graph_write_usage[] = {\n-\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] \"),\n+\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n \tNULL\n };\n \n@@ -38,6 +39,7 @@ static struct opts_commit_graph {\n \tint append;\n \tint split;\n \tint shallow;\n+\tint progress;\n } opts;\n \n static int graph_verify(int argc, const char **argv)\n@@ -48,16 +50,20 @@ static int graph_verify(int argc, const char **argv)\n \tint fd;\n \tstruct stat st;\n \tint flags = 0;\n-\n+\tint defaultProgressState = isatty(2);\n+\t\n \tstatic struct option builtin_commit_graph_verify_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n \t\t\t N_(\"dir\"),\n \t\t\t N_(\"The object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n \n+\topts.progress = defaultProgressState;\n+\t\n \targc = parse_options(argc, argv, NULL,\n \t\t\t builtin_commit_graph_verify_options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n@@ -66,7 +72,9 @@ static int graph_verify(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \tif (opts.shallow)\n \t\tflags |= COMMIT_GRAPH_VERIFY_SHALLOW;\n-\n+\tif (opts.progress)\n+\t\tflags |= COMMIT_GRAPH_PROGRESS;\n+\t\n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok && errno != ENOENT)\n@@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv)\n \tstruct string_list *commit_hex = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n-\tunsigned int flags = COMMIT_GRAPH_PROGRESS;\n-\n+\tunsigned int flags = 0;\n+\tint defaultProgressState = isatty(2);\n+\t\n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n \t\t\tN_(\"dir\"),\n@@ -168,6 +177,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"start walk at commits listed by stdin\")),\n \t\tOPT_BOOL(0, \"append\", &opts.append,\n \t\t\tN_(\"include all commits already in the commit-graph file\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL(0, \"split\", &opts.split,\n \t\t\tN_(\"allow writing an incremental commit-graph file\")),\n \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n@@ -179,6 +189,7 @@ static int graph_write(int argc, const char **argv)\n \t\tOPT_END(),\n \t};\n \n+\topts.progress = defaultProgressState;\n \tsplit_opts.size_multiple = 2;\n \tsplit_opts.max_commits = 0;\n \tsplit_opts.expire_time = 0;\n@@ -195,6 +206,8 @@ static int graph_write(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_APPEND;\n \tif (opts.split)\n \t\tflags |= COMMIT_GRAPH_SPLIT;\n+\tif (opts.progress)\n+\t\tflags |= COMMIT_GRAPH_PROGRESS;\n \n \tread_replace_refs = 0;\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex fe954ab5f8..b10d47f99a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1986,14 +1986,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \tif (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)\n \t\treturn verify_commit_graph_error;\n \n-\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n-\t\t\t\t g->num_commits);\n+\tif (flags & COMMIT_GRAPH_PROGRESS)\n+\t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n+\t\t\t\t\tg->num_commits);\n+\n \tfor (i = 0; i < g->num_commits; i++) {\n \t\tstruct commit *graph_commit, *odb_commit;\n \t\tstruct commit_list *graph_parents, *odb_parents;\n \t\tuint32_t max_generation = 0;\n \n \t\tdisplay_progress(progress, i + 1);\n+\n \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 22cb9d6643..a98d9f88f4 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -116,6 +116,42 @@ test_expect_success 'Add more commits' '\n \tgit repack\n '\n \n+test_expect_success 'commit-graph write progress off by default for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph write 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n+test_expect_success 'commit-graph write force progress on for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph write --progress 2>err &&\n+\ttest_file_not_empty err\n+'\n+\n+test_expect_success 'commit-graph write with the --no-progress option' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph write --no-progress 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n+test_expect_success 'commit-graph verify progress off by default for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph verify 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n+test_expect_success 'commit-graph verify force progress on for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph verify --progress 2>err &&\n+\ttest_file_not_empty err\n+'\n+\n+test_expect_success 'commit-graph verify with the --no-progress option' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph verify --no-progress 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n # Current graph structure:\n #\n # __M3___\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 99f4ef4c19..4fc3fda9d6 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '\n \tgit merge commits/3 commits/4 &&\n \tgit branch merge/octopus &&\n \tgit commit-graph write --reachable --split &&\n-\tgit commit-graph verify 2>err &&\n+\tgit commit-graph verify --progress 2>err &&\n \ttest_line_count = 3 err &&\n \ttest_i18ngrep ! warning err &&\n \ttest_line_count = 3 $graphdir/commit-graph-chain\n-- \ngitgitgadget\n"},{"id":"380833","messageId":"502f808c-d8fe-81d3-d15b-6f92916035ba@gmail.com","threadId":"51693","inReplyTo":"pull.315.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] commit-graph: add --[no-]progress to write and verify","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-08-20T18:45:20Z","receivedAt":"2019-08-20T18:45:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2019 2:37 PM, Garima Singh via GitGitGadget wrote:\n> Hey Git contributors! \n> \n> My name is Garima Singh and I work at Microsoft. I recently started working\n> closely with the Microsoft team contributing to the git client ecosystem. I\n> am very glad to have the opportunity to work with this community. I am new\n> to the world of git client development but I did work on the Git service\n> offering of Azure Developer Services for a few years. I am sure I will get\n> to learn a lot from all of you. \n\nI just wanted to chime in and introduce Garima a bit myself. Garima and I\nwere both on the Git Server team for Azure Repos and left that team around\nthe same time. She went and did things in Azure Pipelines before leaving to\npursue more education and returning to our current team working on the Git\necosystem. Garima will be focused mostly on core Git, so get used to seeing\nher on the list!\n\nShe's starting with a couple smaller series to get her feet wet, but then\nis working on some deep dives into performance features. Look forward to\nthose!\n\n> CC: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com\n\nGitGitGadget is picky about the casing of \"Cc:\" so I have CC'd these people.\n\nThanks,\n-Stolee\n"},{"id":"380859","messageId":"xmqqftlvtucw.fsf@gitster-ct.c.googlers.com","threadId":"51693","inReplyTo":"da89f7dadb0be2d4ada22dd3e2d1f5524c73f70d.1566326275.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T21:11:43Z","receivedAt":"2019-08-20T21:11:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Garima Singh via GitGitGadget\" writes:\n\n> From: Garima Singh \n>\n> Add --[no-]progress to git commit-graph write and verify.\n> The progress feature was introduced in 7b0f229\n> (\"commit-graph write: add progress output\", 2018-09-17) but\n> the ability to opt-out was overlooked.\n\nNicely described.\n\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 38027b83d9..71796910fc 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -6,17 +6,18 @@\n> #include \"repository.h\"\n> #include \"commit-graph.h\"\n> #include \"object-store.h\"\n> +#include \"unistd.h\"\n\nPlease do not contaminate *.c files with #include of system headers.\n\nOften, various platforms require system include files in specific\norder, and the project convention is to include them in\ngit-compat-util.h in the right order (with #ifdef and friends as\nnecessary). *.c files are required to include git-compat-util.h (or\none of the well known headers that include git-compat-util.h as the\nfirst one) as the first file.\n\nIn fact, \"builtin.h\" includes \"git-compat-util.h\" as the first\nthing, and \"git-compat-util.h\" in turn includes unistd reasonably\nearly. Do you really need to include it again here?\n\n> @@ -48,16 +50,20 @@ static int graph_verify(int argc, const char **argv)\n> \tint fd;\n> \tstruct stat st;\n> \tint flags = 0;\n> -\n> +\tint defaultProgressState = isatty(2);\n\nAs you can see from the naming of other variables, we do not do\ncamelCase variable names.\n\nIn fact you do not need this variable, do you?\n\n> \tstatic struct option builtin_commit_graph_verify_options[] = {\n> \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> \t\t\t N_(\"dir\"),\n> \t\t\t N_(\"The object directory to store the graph\")),\n> \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n> \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n> +\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n> \t\tOPT_END(),\n> \t};\n> \n> +\topts.progress = defaultProgressState;\n\n... as you can assign isatty(2) to opts.progress here directly.\n\n> @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv)\n> \tstruct string_list *commit_hex = NULL;\n> \tstruct string_list lines;\n> \tint result = 0;\n> -\tunsigned int flags = COMMIT_GRAPH_PROGRESS;\n> -\n> +\tunsigned int flags = 0;\n> +\tint defaultProgressState = isatty(2);\n\nLikewise.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index fe954ab5f8..b10d47f99a 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1986,14 +1986,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n> \tif (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)\n> \t\treturn verify_commit_graph_error;\n> \n> -\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> -\t\t\t\t g->num_commits);\n> +\tif (flags & COMMIT_GRAPH_PROGRESS)\n> +\t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> +\t\t\t\t\tg->num_commits);\n\nMakes sense.\n\n> \tfor (i = 0; i < g->num_commits; i++) {\n> \t\tstruct commit *graph_commit, *odb_commit;\n> \t\tstruct commit_list *graph_parents, *odb_parents;\n> \t\tuint32_t max_generation = 0;\n> \n> \t\tdisplay_progress(progress, i + 1);\n> +\n> \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n\nDrop this change---I do not see a reason for the extra blank line here.\n"},{"id":"380861","messageId":"CAPig+cSR-ab_8AeZ9fJX2G8h6x_V_NUG01pXWQAgF+_pgmR2fQ@mail.gmail.com","threadId":"51693","inReplyTo":"da89f7dadb0be2d4ada22dd3e2d1f5524c73f70d.1566326275.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-20T21:13:06Z","receivedAt":"2019-08-20T21:13:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 20, 2019 at 2:38 PM Garima Singh via GitGitGadget\n wrote:\n> Add --[no-]progress to git commit-graph write and verify.\n> The progress feature was introduced in 7b0f229\n> (\"commit-graph write: add progress output\", 2018-09-17) but\n> the ability to opt-out was overlooked.\n>\n> Signed-off-by: Garima Singh \n> ---\n> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> @@ -10,7 +10,7 @@ SYNOPSIS\n> 'git commit-graph read' [--object-dir ]\n> -'git commit-graph verify' [--object-dir ] [--shallow]\n> +'git commit-graph verify' [--object-dir ] [--shallow] [--[no-]progress]\n> 'git commit-graph write' [--object-dir ]\n\nThis synopsis shows only 'verify' accepting --[no-]progress, however,\nthe \"usage\" message in commit-graph.c itself shows 'verify' and\n'write' accepting the option.\n\n> @@ -29,6 +29,8 @@ OPTIONS\n> +--[no-]progress::\n> + Toggle whether to show progress or not.\n\nThis is misleading. The --progress option does not _toggle_ the\nsetting. The positive form explicitly enables it, and the negated form\nexplicitly disables it. Try to take inspiration for the wording of\nthis description by consulting other existing documentation. For\ninstance, from Documentation/merge-options.txt:\n\n Turn progress on/off explicitly. If neither is specified,\n progress is shown if standard error is connected to a terminal.\n\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> @@ -6,17 +6,18 @@\n> static char const * const builtin_commit_graph_usage[] = {\n> - N_(\"git commit-graph verify [--object-dir ] [--shallow]\"),\n> - N_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] \"),\n> + N_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n> + N_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n\nThis disagrees with the synopsis in the documentation, as mentioned above.\n\n> static int graph_verify(int argc, const char **argv)\n> @@ -48,16 +50,20 @@ static int graph_verify(int argc, const char **argv)\n> int fd;\n> struct stat st;\n> int flags = 0;\n> -\n> + int defaultProgressState = isatty(2);\n> +\n\nYou have stray whitespace at the end of the \"blank\" line following the\nnew variable declaration, hence the odd-looking diff.\n\n> @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv)\n> - unsigned int flags = COMMIT_GRAPH_PROGRESS;\n> -\n> + unsigned int flags = 0;\n> + int defaultProgressState = isatty(2);\n> +\n\nDitto.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> @@ -1986,14 +1986,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n> if (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)\n> return verify_commit_graph_error;\n>\n> - progress = start_progress(_(\"Verifying commits in commit graph\"),\n> - g->num_commits);\n> + if (flags & COMMIT_GRAPH_PROGRESS)\n> + progress = start_progress(_(\"Verifying commits in commit graph\"),\n> + g->num_commits);\n\nThe progress reporting functions can safely handle a NULL pointer for\n'progress'. In the original code 'progress' was assigned explicitly,\nthus could not be NULL, However, in the revised code, it's not clear\nfrom this snippet what the value of 'progress' is if\nCOMMIT_GRAPH_PROGRESS is not in 'flags'. If 'progress' is\nuninitialized, then the behavior would be undefined. Looking at the\nvariable declaration, I see that it is indeed initialized to NULL, so\nthis code is safe. Okay.\n\n> for (i = 0; i < g->num_commits; i++) {\n> struct commit *graph_commit, *odb_commit;\n> struct commit_list *graph_parents, *odb_parents;\n> uint32_t max_generation = 0;\n>\n> display_progress(progress, i + 1);\n> +\n> hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n\nNo need to make changes (such as inserting an unnecessary blank line)\nunrelated to the stated purposed of the patch.\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> @@ -116,6 +116,42 @@ test_expect_success 'Add more commits' '\n> +test_expect_success 'commit-graph write progress off by default for stderr' '\n> + cd \"$TRASH_DIRECTORY/full\" &&\n> + git commit-graph write 2>err &&\n> + test_line_count = 0 err\n> +'\n\nChanging the working directory ('cd') outside of a subshell is heavily\ndiscouraged in this test suite, however, since this particular script\nis riddled with the 'cd \"$TRASH_DIRECTORY/full\"' idiom, this can\nprobably pass, however, it's not good to get into the habit of doing\nit this way.\n"},{"id":"380893","messageId":"xmqqy2zmsbx0.fsf@gitster-ct.c.googlers.com","threadId":"51693","inReplyTo":"CAPig+cSR-ab_8AeZ9fJX2G8h6x_V_NUG01pXWQAgF+_pgmR2fQ@mail.gmail.com","subject":"Re: [PATCH 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-21T16:47:39Z","receivedAt":"2019-08-21T16:47:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> This synopsis shows only 'verify' accepting --[no-]progress,\n> however, ...\n> This is misleading. The --progress option does not _toggle_ the\n> setting. ...\n\nThanks for a careful review.\n"},{"id":"381205","messageId":"pull.315.v2.git.gitgitgadget@gmail.com","threadId":"51693","inReplyTo":"pull.315.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] commit-graph: add --[no-]progress to write and verify","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-26T16:29:58Z","receivedAt":"2019-08-26T16:30:03Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey Git contributors! \n\nMy name is Garima Singh and I work at Microsoft. I recently started working\nclosely with the Microsoft team contributing to the git client ecosystem. I\nam very glad to have the opportunity to work with this community. I am new\nto the world of git client development but I did work on the Git service\noffering of Azure Developer Services for a few years. I am sure I will get\nto learn a lot from all of you. \n\nDr. Derrick Stolee helped me pick out my first task (Thanks Stolee!) He\nmentioned an issue in the commit-graph builtin where git did not support\nopting in and out of the progress output. This was bloating up the stderr\nlogs in VFS for Git. The progress feature was introduced in 7b0f229222\n(\"commit-graph write: add progress output\", 2018-09-17) but the ability to\nopt-out was overlooked. This patch adds the --no-progress option so that\ncallers can control the amount of logging they receive. \n\nLooking forward to your review. Cheers! Garima Singh\n\nCC: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com\n\nGarima Singh (1):\n commit-graph: add --[no-]progress to write and verify.\n\n Documentation/git-commit-graph.txt | 7 ++++--\n builtin/commit-graph.c | 21 ++++++++++++-----\n commit-graph.c | 6 +++--\n t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++\n t/t5324-split-commit-graph.sh | 2 +-\n 5 files changed, 61 insertions(+), 11 deletions(-)\n\n\nbase-commit: 745f6812895b31c02b29bdfe4ae8e5498f776c26\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/315\n\nRange-diff vs v1:\n\n 1: da89f7dadb ! 1: 47cc99bd15 commit-graph: add --[no-]progress to write and verify.\n @@ -17,16 +17,19 @@\n [verse]\n 'git commit-graph read' [--object-dir ]\n -'git commit-graph verify' [--object-dir ] [--shallow]\n +-'git commit-graph write' [--object-dir ]\n +'git commit-graph verify' [--object-dir ] [--shallow] [--[no-]progress]\n - 'git commit-graph write' [--object-dir ]\n ++'git commit-graph write' [--object-dir ] [--[no-]progress]\n \n \n + DESCRIPTION\n @@\n \tcommit-graph file is expected to be in the `/info` directory and\n \tthe packfiles are expected to be in `/pack`.\n \n +--[no-]progress::\n -+\tToggle whether to show progress or not.\n ++\tTurn progress on/off explicitly. If neither is specified, progress is \n ++\tshown if standard error is connected to a terminal.\n \n COMMANDS\n --------\n @@ -35,11 +38,6 @@\n --- a/builtin/commit-graph.c\n +++ b/builtin/commit-graph.c\n @@\n - #include \"repository.h\"\n - #include \"commit-graph.h\"\n - #include \"object-store.h\"\n -+#include \"unistd.h\"\n - \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph [--object-dir ]\"),\n \tN_(\"git commit-graph read [--object-dir ]\"),\n @@ -74,15 +72,6 @@\n \n static int graph_verify(int argc, const char **argv)\n @@\n - \tint fd;\n - \tstruct stat st;\n - \tint flags = 0;\n --\n -+\tint defaultProgressState = isatty(2);\n -+\t\n - \tstatic struct option builtin_commit_graph_verify_options[] = {\n - \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n - \t\t\t N_(\"dir\"),\n \t\t\t N_(\"The object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n @@ -90,8 +79,7 @@\n \t\tOPT_END(),\n \t};\n \n -+\topts.progress = defaultProgressState;\n -+\t\n ++\topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n \t\t\t builtin_commit_graph_verify_options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n @@ -101,7 +89,7 @@\n \t\tflags |= COMMIT_GRAPH_VERIFY_SHALLOW;\n -\n +\tif (opts.progress)\n -+\t\tflags |= COMMIT_GRAPH_PROGRESS;\n ++\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n +\t\n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n @@ -110,14 +98,11 @@\n \tstruct string_list *commit_hex = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n --\tunsigned int flags = COMMIT_GRAPH_PROGRESS;\n --\n -+\tunsigned int flags = 0;\n -+\tint defaultProgressState = isatty(2);\n -+\t\n +-\tenum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;\n ++\tenum commit_graph_write_flags flags = 0;\n + \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n - \t\t\tN_(\"dir\"),\n @@\n \t\t\tN_(\"start walk at commits listed by stdin\")),\n \t\tOPT_BOOL(0, \"append\", &opts.append,\n @@ -130,16 +115,16 @@\n \t\tOPT_END(),\n \t};\n \n -+\topts.progress = defaultProgressState;\n ++\topts.progress = isatty(2);\n \tsplit_opts.size_multiple = 2;\n \tsplit_opts.max_commits = 0;\n \tsplit_opts.expire_time = 0;\n @@\n - \t\tflags |= COMMIT_GRAPH_APPEND;\n + \t\tflags |= COMMIT_GRAPH_WRITE_APPEND;\n \tif (opts.split)\n - \t\tflags |= COMMIT_GRAPH_SPLIT;\n + \t\tflags |= COMMIT_GRAPH_WRITE_SPLIT;\n +\tif (opts.progress)\n -+\t\tflags |= COMMIT_GRAPH_PROGRESS;\n ++\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \tread_replace_refs = 0;\n \n @@ -153,20 +138,13 @@\n \n -\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n -\t\t\t\t g->num_commits);\n -+\tif (flags & COMMIT_GRAPH_PROGRESS)\n ++\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n +\t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n +\t\t\t\t\tg->num_commits);\n +\n \tfor (i = 0; i < g->num_commits; i++) {\n \t\tstruct commit *graph_commit, *odb_commit;\n \t\tstruct commit_list *graph_parents, *odb_parents;\n - \t\tuint32_t max_generation = 0;\n - \n - \t\tdisplay_progress(progress, i + 1);\n -+\n - \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n - \n - \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \n diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n --- a/t/t5318-commit-graph.sh\n @@ -175,7 +153,7 @@\n \tgit repack\n '\n \n -+test_expect_success 'commit-graph write progress off by default for stderr' '\n ++test_expect_success 'commit-graph write progress off for redirected stderr' '\n +\tcd \"$TRASH_DIRECTORY/full\" &&\n +\tgit commit-graph write 2>err &&\n +\ttest_line_count = 0 err\n @@ -193,7 +171,7 @@\n +\ttest_line_count = 0 err\n +'\n +\n -+test_expect_success 'commit-graph verify progress off by default for stderr' '\n ++test_expect_success 'commit-graph verify progress off for redirected stderr' '\n +\tcd \"$TRASH_DIRECTORY/full\" &&\n +\tgit commit-graph verify 2>err &&\n +\ttest_line_count = 0 err\n\n-- \ngitgitgadget\n"},{"id":"381206","messageId":"47cc99bd151db67fe2ee0f91bb98b3eb7e55786d.1566836997.git.gitgitgadget@gmail.com","threadId":"51693","inReplyTo":"pull.315.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-26T16:29:58Z","receivedAt":"2019-08-26T16:30:04Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nAdd --[no-]progress to git commit-graph write and verify.\nThe progress feature was introduced in 7b0f229\n(\"commit-graph write: add progress output\", 2018-09-17) but\nthe ability to opt-out was overlooked.\n\nSigned-off-by: Garima Singh \n---\n Documentation/git-commit-graph.txt | 7 ++++--\n builtin/commit-graph.c | 21 ++++++++++++-----\n commit-graph.c | 6 +++--\n t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++\n t/t5324-split-commit-graph.sh | 2 +-\n 5 files changed, 61 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex eb5e7865f0..ca0b1a683f 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -10,8 +10,8 @@ SYNOPSIS\n --------\n [verse]\n 'git commit-graph read' [--object-dir ]\n-'git commit-graph verify' [--object-dir ] [--shallow]\n-'git commit-graph write' [--object-dir ]\n+'git commit-graph verify' [--object-dir ] [--shallow] [--[no-]progress]\n+'git commit-graph write' [--object-dir ] [--[no-]progress]\n \n \n DESCRIPTION\n@@ -29,6 +29,9 @@ OPTIONS\n \tcommit-graph file is expected to be in the `/info` directory and\n \tthe packfiles are expected to be in `/pack`.\n \n+--[no-]progress::\n+\tTurn progress on/off explicitly. If neither is specified, progress is \n+\tshown if standard error is connected to a terminal.\n \n COMMANDS\n --------\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 57863619b7..faf349a6c1 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -10,13 +10,13 @@\n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph [--object-dir ]\"),\n \tN_(\"git commit-graph read [--object-dir ]\"),\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow]\"),\n-\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] \"),\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n+\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n \tNULL\n };\n \n static const char * const builtin_commit_graph_verify_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow]\"),\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n \tNULL\n };\n \n@@ -26,7 +26,7 @@ static const char * const builtin_commit_graph_read_usage[] = {\n };\n \n static const char * const builtin_commit_graph_write_usage[] = {\n-\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] \"),\n+\tN_(\"git commit-graph write [--object-dir ] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] \"),\n \tNULL\n };\n \n@@ -38,6 +38,7 @@ static struct opts_commit_graph {\n \tint append;\n \tint split;\n \tint shallow;\n+\tint progress;\n } opts;\n \n static int graph_verify(int argc, const char **argv)\n@@ -55,9 +56,11 @@ static int graph_verify(int argc, const char **argv)\n \t\t\t N_(\"The object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n \n+\topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n \t\t\t builtin_commit_graph_verify_options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n@@ -66,7 +69,9 @@ static int graph_verify(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \tif (opts.shallow)\n \t\tflags |= COMMIT_GRAPH_VERIFY_SHALLOW;\n-\n+\tif (opts.progress)\n+\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n+\t\n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok && errno != ENOENT)\n@@ -154,7 +159,7 @@ static int graph_write(int argc, const char **argv)\n \tstruct string_list *commit_hex = NULL;\n \tstruct string_list lines;\n \tint result = 0;\n-\tenum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;\n+\tenum commit_graph_write_flags flags = 0;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -168,6 +173,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"start walk at commits listed by stdin\")),\n \t\tOPT_BOOL(0, \"append\", &opts.append,\n \t\t\tN_(\"include all commits already in the commit-graph file\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL(0, \"split\", &opts.split,\n \t\t\tN_(\"allow writing an incremental commit-graph file\")),\n \t\tOPT_INTEGER(0, \"max-commits\", &split_opts.max_commits,\n@@ -179,6 +185,7 @@ static int graph_write(int argc, const char **argv)\n \t\tOPT_END(),\n \t};\n \n+\topts.progress = isatty(2);\n \tsplit_opts.size_multiple = 2;\n \tsplit_opts.max_commits = 0;\n \tsplit_opts.expire_time = 0;\n@@ -195,6 +202,8 @@ static int graph_write(int argc, const char **argv)\n \t\tflags |= COMMIT_GRAPH_WRITE_APPEND;\n \tif (opts.split)\n \t\tflags |= COMMIT_GRAPH_WRITE_SPLIT;\n+\tif (opts.progress)\n+\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \tread_replace_refs = 0;\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex f2888c203b..2802f2ade6 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1992,8 +1992,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \tif (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)\n \t\treturn verify_commit_graph_error;\n \n-\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n-\t\t\t\t g->num_commits);\n+\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n+\t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n+\t\t\t\t\tg->num_commits);\n+\n \tfor (i = 0; i < g->num_commits; i++) {\n \t\tstruct commit *graph_commit, *odb_commit;\n \t\tstruct commit_list *graph_parents, *odb_parents;\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ab3eccf0fa..df3fed3a08 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -124,6 +124,42 @@ test_expect_success 'Add more commits' '\n \tgit repack\n '\n \n+test_expect_success 'commit-graph write progress off for redirected stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph write 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n+test_expect_success 'commit-graph write force progress on for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph write --progress 2>err &&\n+\ttest_file_not_empty err\n+'\n+\n+test_expect_success 'commit-graph write with the --no-progress option' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph write --no-progress 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n+test_expect_success 'commit-graph verify progress off for redirected stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph verify 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n+test_expect_success 'commit-graph verify force progress on for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph verify --progress 2>err &&\n+\ttest_file_not_empty err\n+'\n+\n+test_expect_success 'commit-graph verify with the --no-progress option' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit commit-graph verify --no-progress 2>err &&\n+\ttest_line_count = 0 err\n+'\n+\n # Current graph structure:\n #\n # __M3___\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 99f4ef4c19..4fc3fda9d6 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '\n \tgit merge commits/3 commits/4 &&\n \tgit branch merge/octopus &&\n \tgit commit-graph write --reachable --split &&\n-\tgit commit-graph verify 2>err &&\n+\tgit commit-graph verify --progress 2>err &&\n \ttest_line_count = 3 err &&\n \ttest_i18ngrep ! warning err &&\n \ttest_line_count = 3 $graphdir/commit-graph-chain\n-- \ngitgitgadget\n"},{"id":"382130","messageId":"c32f1bb4-9d98-eb27-33c2-251417c2da55@gmail.com","threadId":"51693","inReplyTo":"pull.315.v2.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/1] commit-graph: add --[no-]progress to write and verify","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2019-09-10T14:00:29Z","receivedAt":"2019-09-10T14:00:31Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Ping :) Any more comments or concerns about this?\n\nOn 8/26/2019 12:29 PM, Garima Singh via GitGitGadget wrote:\n> Hey Git contributors!\n> \n> My name is Garima Singh and I work at Microsoft. I recently started working\n> closely with the Microsoft team contributing to the git client ecosystem. I\n> am very glad to have the opportunity to work with this community. I am new\n> to the world of git client development but I did work on the Git service\n> offering of Azure Developer Services for a few years. I am sure I will get\n> to learn a lot from all of you.\n> \n> Dr. Derrick Stolee helped me pick out my first task (Thanks Stolee!) He\n> mentioned an issue in the commit-graph builtin where git did not support\n> opting in and out of the progress output. This was bloating up the stderr\n> logs in VFS for Git. The progress feature was introduced in 7b0f229222\n> (\"commit-graph write: add progress output\", 2018-09-17) but the ability to\n> opt-out was overlooked. This patch adds the --no-progress option so that\n> callers can control the amount of logging they receive.\n> \n> Looking forward to your review. Cheers! Garima Singh\n> \n> CC: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com\n> \n> Garima Singh (1):\n> commit-graph: add --[no-]progress to write and verify.\n> \n> Documentation/git-commit-graph.txt | 7 ++++--\n> builtin/commit-graph.c | 21 ++++++++++++-----\n> commit-graph.c | 6 +++--\n> t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++\n> t/t5324-split-commit-graph.sh | 2 +-\n> 5 files changed, 61 insertions(+), 11 deletions(-)\n> \n> \n> base-commit: 745f6812895b31c02b29bdfe4ae8e5498f776c26\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/315\n> \n> Range-diff vs v1:\n> \n> 1: da89f7dadb ! 1: 47cc99bd15 commit-graph: add --[no-]progress to write and verify.\n> @@ -17,16 +17,19 @@\n> [verse]\n> 'git commit-graph read' [--object-dir ]\n> -'git commit-graph verify' [--object-dir ] [--shallow]\n> +-'git commit-graph write' [--object-dir ]\n> +'git commit-graph verify' [--object-dir ] [--shallow] [--[no-]progress]\n> - 'git commit-graph write' [--object-dir ]\n> ++'git commit-graph write' [--object-dir ] [--[no-]progress]\n> \n> \n> + DESCRIPTION\n> @@\n> \tcommit-graph file is expected to be in the `/info` directory and\n> \tthe packfiles are expected to be in `/pack`.\n> \n> +--[no-]progress::\n> -+\tToggle whether to show progress or not.\n> ++\tTurn progress on/off explicitly. If neither is specified, progress is\n> ++\tshown if standard error is connected to a terminal.\n> \n> COMMANDS\n> --------\n> @@ -35,11 +38,6 @@\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@\n> - #include \"repository.h\"\n> - #include \"commit-graph.h\"\n> - #include \"object-store.h\"\n> -+#include \"unistd.h\"\n> -\n> static char const * const builtin_commit_graph_usage[] = {\n> \tN_(\"git commit-graph [--object-dir ]\"),\n> \tN_(\"git commit-graph read [--object-dir ]\"),\n> @@ -74,15 +72,6 @@\n> \n> static int graph_verify(int argc, const char **argv)\n> @@\n> - \tint fd;\n> - \tstruct stat st;\n> - \tint flags = 0;\n> --\n> -+\tint defaultProgressState = isatty(2);\n> -+\t\n> - \tstatic struct option builtin_commit_graph_verify_options[] = {\n> - \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> - \t\t\t N_(\"dir\"),\n> \t\t\t N_(\"The object directory to store the graph\")),\n> \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n> \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n> @@ -90,8 +79,7 @@\n> \t\tOPT_END(),\n> \t};\n> \n> -+\topts.progress = defaultProgressState;\n> -+\t\n> ++\topts.progress = isatty(2);\n> \targc = parse_options(argc, argv, NULL,\n> \t\t\t builtin_commit_graph_verify_options,\n> \t\t\t builtin_commit_graph_verify_usage, 0);\n> @@ -101,7 +89,7 @@\n> \t\tflags |= COMMIT_GRAPH_VERIFY_SHALLOW;\n> -\n> +\tif (opts.progress)\n> -+\t\tflags |= COMMIT_GRAPH_PROGRESS;\n> ++\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n> +\t\n> \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n> \topen_ok = open_commit_graph(graph_name, &fd, &st);\n> @@ -110,14 +98,11 @@\n> \tstruct string_list *commit_hex = NULL;\n> \tstruct string_list lines;\n> \tint result = 0;\n> --\tunsigned int flags = COMMIT_GRAPH_PROGRESS;\n> --\n> -+\tunsigned int flags = 0;\n> -+\tint defaultProgressState = isatty(2);\n> -+\t\n> +-\tenum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;\n> ++\tenum commit_graph_write_flags flags = 0;\n> +\n> \tstatic struct option builtin_commit_graph_write_options[] = {\n> \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> - \t\t\tN_(\"dir\"),\n> @@\n> \t\t\tN_(\"start walk at commits listed by stdin\")),\n> \t\tOPT_BOOL(0, \"append\", &opts.append,\n> @@ -130,16 +115,16 @@\n> \t\tOPT_END(),\n> \t};\n> \n> -+\topts.progress = defaultProgressState;\n> ++\topts.progress = isatty(2);\n> \tsplit_opts.size_multiple = 2;\n> \tsplit_opts.max_commits = 0;\n> \tsplit_opts.expire_time = 0;\n> @@\n> - \t\tflags |= COMMIT_GRAPH_APPEND;\n> + \t\tflags |= COMMIT_GRAPH_WRITE_APPEND;\n> \tif (opts.split)\n> - \t\tflags |= COMMIT_GRAPH_SPLIT;\n> + \t\tflags |= COMMIT_GRAPH_WRITE_SPLIT;\n> +\tif (opts.progress)\n> -+\t\tflags |= COMMIT_GRAPH_PROGRESS;\n> ++\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n> \n> \tread_replace_refs = 0;\n> \n> @@ -153,20 +138,13 @@\n> \n> -\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> -\t\t\t\t g->num_commits);\n> -+\tif (flags & COMMIT_GRAPH_PROGRESS)\n> ++\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n> +\t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> +\t\t\t\t\tg->num_commits);\n> +\n> \tfor (i = 0; i < g->num_commits; i++) {\n> \t\tstruct commit *graph_commit, *odb_commit;\n> \t\tstruct commit_list *graph_parents, *odb_parents;\n> - \t\tuint32_t max_generation = 0;\n> -\n> - \t\tdisplay_progress(progress, i + 1);\n> -+\n> - \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n> -\n> - \t\tgraph_commit = lookup_commit(r, &cur_oid);\n> \n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> --- a/t/t5318-commit-graph.sh\n> @@ -175,7 +153,7 @@\n> \tgit repack\n> '\n> \n> -+test_expect_success 'commit-graph write progress off by default for stderr' '\n> ++test_expect_success 'commit-graph write progress off for redirected stderr' '\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n> +\tgit commit-graph write 2>err &&\n> +\ttest_line_count = 0 err\n> @@ -193,7 +171,7 @@\n> +\ttest_line_count = 0 err\n> +'\n> +\n> -+test_expect_success 'commit-graph verify progress off by default for stderr' '\n> ++test_expect_success 'commit-graph verify progress off for redirected stderr' '\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n> +\tgit commit-graph verify 2>err &&\n> +\ttest_line_count = 0 err\n> \n"},{"id":"382256","messageId":"xmqqef0lfdt4.fsf@gitster-ct.c.googlers.com","threadId":"51693","inReplyTo":"47cc99bd151db67fe2ee0f91bb98b3eb7e55786d.1566836997.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-12T20:40:55Z","receivedAt":"2019-09-12T20:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Garima Singh via GitGitGadget\" writes:\n\n> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> index eb5e7865f0..ca0b1a683f 100644\n> --- a/Documentation/git-commit-graph.txt\n> +++ b/Documentation/git-commit-graph.txt\n> @@ -10,8 +10,8 @@ SYNOPSIS\n> --------\n> [verse]\n> 'git commit-graph read' [--object-dir ]\n> -'git commit-graph verify' [--object-dir ] [--shallow]\n> -'git commit-graph write' [--object-dir ]\n> +'git commit-graph verify' [--object-dir ] [--shallow] [--[no-]progress]\n> +'git commit-graph write' [--object-dir ] [--[no-]progress]\n\nThis is not a problem with this patch, but it is disturbing to see\n and other concrete \"--option\" listed explicitly. It could\nbe that \"--object-dir \" is so important an option that deserves\nto be singled out while other random options can be left to individual\noption's description, but in that case, would \"--progress\" be equally\nimportant (if anything, as an option that is purely about appearance,\nI would expect it to be with a lot lower importance)?\n\nI guess with a preparatory clean-up patch to deal with the \npart, the result of applying this patch would not look so bad.\nPerhaps renaming to and moving it\nto the end of the line might be sufficient. I dunno. At least we'd\nneed to make sure that it is clear to readers what options are\nallowed where we wrote above.\n\n> @@ -29,6 +29,9 @@ OPTIONS\n> \tcommit-graph file is expected to be in the `/info` directory and\n> \tthe packfiles are expected to be in `/pack`.\n> \n> +--[no-]progress::\n> +\tTurn progress on/off explicitly. If neither is specified, progress is \n\nTrailing whitespace.\n\n> +\tshown if standard error is connected to a terminal.\n> ...\n> +\tif (opts.progress)\n> +\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n> +\t\n\nTrailing whitespace.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f2888c203b..2802f2ade6 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1992,8 +1992,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n> \tif (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)\n> \t\treturn verify_commit_graph_error;\n> \n> -\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> -\t\t\t\t g->num_commits);\n> +\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n> +\t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> +\t\t\t\t\tg->num_commits);\n> +\n\nThis is correct, but it feels funny that it is sufficient to\ncastrate start_progress() and we do not have to muck with existing\ncalls to show and stop progress output. We rely on progress being\nNULL for that to work, and existing code initializes the variable\nto NULL, so we are OK.\n"},{"id":"382469","messageId":"20190916223607.GE6190@szeder.dev","threadId":"51693","inReplyTo":"47cc99bd151db67fe2ee0f91bb98b3eb7e55786d.1566836997.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-16T22:36:07Z","receivedAt":"2019-09-16T22:36:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:\n> From: Garima Singh \n> \n> Add --[no-]progress to git commit-graph write and verify.\n> The progress feature was introduced in 7b0f229\n> (\"commit-graph write: add progress output\", 2018-09-17) but\n> the ability to opt-out was overlooked.\n\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index 99f4ef4c19..4fc3fda9d6 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '\n> \tgit merge commits/3 commits/4 &&\n> \tgit branch merge/octopus &&\n> \tgit commit-graph write --reachable --split &&\n> -\tgit commit-graph verify 2>err &&\n> +\tgit commit-graph verify --progress 2>err &&\n\nWhy is it necessary to use '--progress' here? It should not be\nnecessary, because the commit message doesn't mention that it changed\nthe default behavior of 'git commit-graph verify'...\n\n> \ttest_line_count = 3 err &&\n\nHaving said that, this test should not check the number of progress\nlines in the first place; see the recent discussion:\n\nhttps://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/\n\n> \ttest_i18ngrep ! warning err &&\n> \ttest_line_count = 3 $graphdir/commit-graph-chain\n> -- \n> gitgitgadget\n"},{"id":"382479","messageId":"7a9581ea-dc90-5ce1-fc3b-578c6dbf6efc@gmail.com","threadId":"51693","inReplyTo":"20190916223607.GE6190@szeder.dev","subject":"Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-17T10:47:38Z","receivedAt":"2019-09-17T10:47:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\nOn 9/16/2019 6:36 PM, SZEDER Gábor wrote:\n> On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:\n>> From: Garima Singh \n>>\n>> Add --[no-]progress to git commit-graph write and verify.\n>> The progress feature was introduced in 7b0f229\n>> (\"commit-graph write: add progress output\", 2018-09-17) but\n>> the ability to opt-out was overlooked.\n> \n>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n>> index 99f4ef4c19..4fc3fda9d6 100755\n>> --- a/t/t5324-split-commit-graph.sh\n>> +++ b/t/t5324-split-commit-graph.sh\n>> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '\n>> \tgit merge commits/3 commits/4 &&\n>> \tgit branch merge/octopus &&\n>> \tgit commit-graph write --reachable --split &&\n>> -\tgit commit-graph verify 2>err &&\n>> +\tgit commit-graph verify --progress 2>err &&\n> \n> Why is it necessary to use '--progress' here? It should not be\n> necessary, because the commit message doesn't mention that it changed\n> the default behavior of 'git commit-graph verify'...\n\nIt does change the default when stderr is not a terminal window. If we\nwere not redirecting to a file, this change would not be necessary.\n \n>> \ttest_line_count = 3 err &&\n> \n> Having said that, this test should not check the number of progress\n> lines in the first place; see the recent discussion:\n> \n> https://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/\n\nTrue, this is an old issue. I think it never got corrected because\nyour reply sounded like the issue doesn't exist in the normal test\nsuite, only in a private branch where you changed the behavior of\nGIT_TEST_GETTEXT_POISON.\n\nIf we still think that should be fixed, it should not be a part of\nthis series, but should be a separate one that focuses on just\nthose changes.\n\nThanks,\n-Stolee\n"},{"id":"382485","messageId":"20190917122215.GA29845@szeder.dev","threadId":"51693","inReplyTo":"7a9581ea-dc90-5ce1-fc3b-578c6dbf6efc@gmail.com","subject":"Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-17T12:22:15Z","receivedAt":"2019-09-17T12:22:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 17, 2019 at 06:47:38AM -0400, Derrick Stolee wrote:\n> \n> On 9/16/2019 6:36 PM, SZEDER Gábor wrote:\n> > On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:\n> >> From: Garima Singh \n> >>\n> >> Add --[no-]progress to git commit-graph write and verify.\n> >> The progress feature was introduced in 7b0f229\n> >> (\"commit-graph write: add progress output\", 2018-09-17) but\n> >> the ability to opt-out was overlooked.\n> > \n> >> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> >> index 99f4ef4c19..4fc3fda9d6 100755\n> >> --- a/t/t5324-split-commit-graph.sh\n> >> +++ b/t/t5324-split-commit-graph.sh\n> >> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '\n> >> \tgit merge commits/3 commits/4 &&\n> >> \tgit branch merge/octopus &&\n> >> \tgit commit-graph write --reachable --split &&\n> >> -\tgit commit-graph verify 2>err &&\n> >> +\tgit commit-graph verify --progress 2>err &&\n> > \n> > Why is it necessary to use '--progress' here? It should not be\n> > necessary, because the commit message doesn't mention that it changed\n> > the default behavior of 'git commit-graph verify'...\n> \n> It does change the default when stderr is not a terminal window. If we\n> were not redirecting to a file, this change would not be necessary.\n\nOK, yesterday I overlooked that the patch added this line:\n\n + opts.progress = isatty(2);\n\nSo, the first question is whether that behavior change is desired; I\ndon't really have an opinion. But if it is desired, then it should be\nchanged in a separate patch, explaining why it is desired, I would\nthink.\n\n> >> \ttest_line_count = 3 err &&\n> > \n> > Having said that, this test should not check the number of progress\n> > lines in the first place; see the recent discussion:\n> > \n> > https://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/\n> \n> True, this is an old issue. I think it never got corrected because\n> your reply sounded like the issue doesn't exist in the normal test\n> suite,\n\nWell, the way I see it the root issue is that the test checks things\nthat it shouldn't.\n\n> only in a private branch where you changed the behavior of\n> GIT_TEST_GETTEXT_POISON.\n> \n> If we still think that should be fixed, it should not be a part of\n> this series, but should be a separate one that focuses on just\n> those changes.\n\nYeah, it should rather go on top of 'ds/commit-graph-octopus-fix'.\n\n"}]}