{"thread":{"id":"53829","subject":"[PATCH 0/2] enable progress traces even when quiet","startedAt":"2020-07-10T01:42:54Z","lastAt":"2020-09-10T05:09:18Z","messageCount":27,"participants":["Emily Shaffer","Derrick Stolee","brian m. carlson","Taylor Blau","Junio C Hamano","Jonathan Tan","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"401281","messageId":"20200710014242.1088216-1-emilyshaffer@google.com","threadId":"53829","inReplyTo":null,"subject":"[PATCH 0/2] enable progress traces even when quiet","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T01:42:40Z","receivedAt":"2020-07-10T01:42:54Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"This is a simple but large-scale change, because it changes an API and\nfixes all callers. The bulk of the change is in the first patch,\nincluding justification in the commit message; the second patch is only\ntidying I noticed while fixing compiler errors from the former. If that\nchurn is unwanted I'm happy to drop it.\n\nI wanted to do this change because we received reports that 'git clone'\nwas only logging traces sometimes, not all the time. An alternative\napproach would have been to move the 'if (verbose)' wrappers to surround\ncalls to 'display_progress()' - but this would have worked in\nbuiltin/index-pack.c (where the reported bug was happening) only because\nof that code's use of static global flags. Placing the onus on the\ncaller to ensure that trace collection occurs, even in quiet mode, is\nflimsy and error-prone. Adding another flag to 'struct progress' ensures\nthat trace collection is consistent regardless of how the caller\norganizes their code.\n\nIn a couple of cases, this does result in some additional work\ncalculating titles and object counts where previously there wasn't;\nhowever, since those values will be used by the trace logger, I don't\nconsider it a waste of resources.\n\nFinally, 'git rev-list' has one weird case where it takes the progress\ntitle as an argument (i.e. 'git rev-list --progress \"Working hard\" ...')\nand also null-checks that string to decide whether to calle\n'start_progress()' - in this case, I added a placeholder text for the\ntraces to show. Bikeshedding welcome here.\n\n - Emily\n\nEmily Shaffer (2):\n  progress: create progress struct in 'verbose' mode\n  progress: remove redundant null-checking\n\n builtin/blame.c             |   4 +-\n builtin/commit-graph.c      |   8 +-\n builtin/fsck.c              |  25 +++----\n builtin/index-pack.c        |  16 ++--\n builtin/log.c               |   4 +-\n builtin/pack-objects.c      |  18 ++---\n builtin/prune.c             |   4 +-\n builtin/rev-list.c          |   9 ++-\n builtin/unpack-objects.c    |   3 +-\n commit-graph.c              | 143 +++++++++++++++++-------------------\n delta-islands.c             |   4 +-\n diffcore-rename.c           |   9 +--\n entry.c                     |   2 +-\n midx.c                      |  43 +++++------\n pack-bitmap-write.c         |   8 +-\n pack-bitmap.c               |   5 +-\n preload-index.c             |   7 +-\n progress.c                  |  27 ++++---\n progress.h                  |  10 ++-\n prune-packed.c              |   4 +-\n read-cache.c                |  15 ++--\n t/helper/test-progress.c    |   6 +-\n t/t0500-progress-display.sh |  27 +++++++\n unpack-trees.c              |  14 ++--\n walker.c                    |   4 +-\n 25 files changed, 217 insertions(+), 202 deletions(-)\n\n-- \n2.27.0.383.g050319c2ae-goog\n\n"},{"id":"401282","messageId":"20200710014242.1088216-2-emilyshaffer@google.com","threadId":"53829","inReplyTo":"20200710014242.1088216-1-emilyshaffer@google.com","subject":"[PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T01:42:41Z","receivedAt":"2020-07-10T01:42:56Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Before now, the progress API is used by conditionally calling\nstart_progress() or a similar call, and then unconditionally calling\ndisplay_progress() and stop_progress(), both of which are tolerant of\nNULL or uninitialized inputs. However, in\n98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\nthroughput), the progress library learned to log traces during expensive\noperations. In cases where progress should not be displayed to the user\n- such as when Git is called by a script - no traces will be logged,\nbecause the progress object is never created.\n\nInstead, to allow us to collect traces from scripted Git commands, teach\na progress->verbose flag, which is specified via a new argument to\nstart_progress() and friends. display_progress() also learns to filter\nfor that flag. With these changes, start_progress() can be called\nunconditionally but with a conditional as an argument to determine\nwhether to report progress to the user.\n\nSince this changes the API, also modify callers of start_progress() and\nfriends to drop their conditional and pass a new argument in instead.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n builtin/blame.c             |   4 +-\n builtin/commit-graph.c      |   5 +-\n builtin/fsck.c              |  25 ++++---\n builtin/index-pack.c        |  16 ++---\n builtin/log.c               |   4 +-\n builtin/pack-objects.c      |  18 +++--\n builtin/prune.c             |   4 +-\n builtin/rev-list.c          |   9 ++-\n builtin/unpack-objects.c    |   3 +-\n commit-graph.c              | 140 +++++++++++++++++-------------------\n delta-islands.c             |   4 +-\n diffcore-rename.c           |   9 ++-\n entry.c                     |   2 +-\n midx.c                      |  43 +++++------\n pack-bitmap-write.c         |   8 +--\n pack-bitmap.c               |   5 +-\n preload-index.c             |   7 +-\n progress.c                  |  27 ++++---\n progress.h                  |  10 +--\n prune-packed.c              |   4 +-\n read-cache.c                |   6 +-\n t/helper/test-progress.c    |   6 +-\n t/t0500-progress-display.sh |  27 +++++++\n unpack-trees.c              |  14 ++--\n walker.c                    |   4 +-\n 25 files changed, 212 insertions(+), 192 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 94ef57c1cc..1d72b27fda 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1129,8 +1129,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \n \tsb.found_guilty_entry = &found_guilty_entry;\n \tsb.found_guilty_entry_data = &pi;\n-\tif (show_progress)\n-\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n+\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines,\n+\t\t\t\t\t     show_progress);\n \n \tassign_blame(&sb, opt);\n \ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex f6797e2a9f..ae4dc2dfcd 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -240,9 +240,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\t\t\t   strbuf_detach(&buf, NULL));\n \t} else if (opts.stdin_commits) {\n \t\toidset_init(&commits, 0);\n-\t\tif (opts.progress)\n-\t\t\tprogress = start_delayed_progress(\n-\t\t\t\t_(\"Collecting commits from input\"), 0);\n+\t\tprogress = start_delayed_progress(\n+\t\t\t_(\"Collecting commits from input\"), 0, opts.progress);\n \n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\t\tif (read_one_commit(&commits, progress, buf.buf)) {\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 37aa07da78..3236ed91ac 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -205,8 +205,8 @@ static int traverse_reachable(void)\n \tstruct progress *progress = NULL;\n \tunsigned int nr = 0;\n \tint result = 0;\n-\tif (show_progress)\n-\t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0);\n+\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0,\n+\t\t\t\t\t  show_progress);\n \twhile (pending.nr) {\n \t\tresult |= traverse_one_object(object_array_pop(&pending));\n \t\tdisplay_progress(progress, ++nr);\n@@ -672,8 +672,8 @@ static void fsck_object_dir(const char *path)\n \tif (verbose)\n \t\tfprintf_ln(stderr, _(\"Checking object directory\"));\n \n-\tif (show_progress)\n-\t\tprogress = start_progress(_(\"Checking object directories\"), 256);\n+\tprogress = start_progress(_(\"Checking object directories\"), 256,\n+\t\t\t\t  show_progress);\n \n \tfor_each_loose_file_in_objdir(path, fsck_loose, fsck_cruft, fsck_subdir,\n \t\t\t\t      progress);\n@@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\tuint32_t total = 0, count = 0;\n \t\t\tstruct progress *progress = NULL;\n \n-\t\t\tif (show_progress) {\n-\t\t\t\tfor (p = get_all_packs(the_repository); p;\n-\t\t\t\t     p = p->next) {\n-\t\t\t\t\tif (open_pack_index(p))\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\ttotal += p->num_objects;\n-\t\t\t\t}\n-\n-\t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n+\t\t\tfor (p = get_all_packs(the_repository); p;\n+\t\t\t     p = p->next) {\n+\t\t\t\tif (open_pack_index(p))\n+\t\t\t\t\tcontinue;\n+\t\t\t\ttotal += p->num_objects;\n \t\t\t}\n+\n+\t\t\tprogress = start_progress(_(\"Checking objects\"), total,\n+\t\t\t\t\t\t  show_progress);\n \t\t\tfor (p = get_all_packs(the_repository); p;\n \t\t\t     p = p->next) {\n \t\t\t\t/* verify gives error messages itself */\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f865666db9..945fc13ddd 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -221,8 +221,7 @@ static unsigned check_objects(void)\n \n \tmax = get_max_object_index();\n \n-\tif (verbose)\n-\t\tprogress = start_delayed_progress(_(\"Checking objects\"), max);\n+\tprogress = start_delayed_progress(_(\"Checking objects\"), max, verbose);\n \n \tfor (i = 0; i < max; i++) {\n \t\tforeign_nr += check_object(get_indexed_object(i));\n@@ -1116,10 +1115,9 @@ static void parse_pack_objects(unsigned char *hash)\n \tstruct object_id ref_delta_oid;\n \tstruct stat st;\n \n-\tif (verbose)\n-\t\tprogress = start_progress(\n-\t\t\t\tfrom_stdin ? _(\"Receiving objects\") : _(\"Indexing objects\"),\n-\t\t\t\tnr_objects);\n+\tprogress = start_progress(\n+\t\t\tfrom_stdin ? _(\"Receiving objects\") : _(\"Indexing objects\"),\n+\t\t\tnr_objects, verbose);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct object_entry *obj = &objects[i];\n \t\tvoid *data = unpack_raw_entry(obj, &ofs_delta->offset,\n@@ -1194,9 +1192,9 @@ static void resolve_deltas(void)\n \tQSORT(ofs_deltas, nr_ofs_deltas, compare_ofs_delta_entry);\n \tQSORT(ref_deltas, nr_ref_deltas, compare_ref_delta_entry);\n \n-\tif (verbose || show_resolving_progress)\n-\t\tprogress = start_progress(_(\"Resolving deltas\"),\n-\t\t\t\t\t  nr_ref_deltas + nr_ofs_deltas);\n+\tprogress = start_progress(_(\"Resolving deltas\"),\n+\t\t\t\t  nr_ref_deltas + nr_ofs_deltas,\n+\t\t\t\t  (verbose || show_resolving_progress));\n \n \tnr_dispatched = 0;\n \tif (nr_threads > 1 || getenv(\"GIT_FORCE_THREADS\")) {\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d104d5c688..71874df75c 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2045,8 +2045,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t}\n \trev.add_signoff = do_signoff;\n \n-\tif (show_progress)\n-\t\tprogress = start_delayed_progress(_(\"Generating patches\"), total);\n+\tprogress = start_delayed_progress(_(\"Generating patches\"), total,\n+\t\t\t\t\t  show_progress);\n \twhile (0 <= --nr) {\n \t\tint shown;\n \t\tdisplay_progress(progress, total - nr);\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7016b28485..1a97a9f0be 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1012,8 +1012,8 @@ static void write_pack_file(void)\n \ttime_t last_mtime = 0;\n \tstruct object_entry **write_order;\n \n-\tif (progress > pack_to_stdout)\n-\t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n+\tprogress_state = start_progress(_(\"Writing objects\"), nr_result,\n+\t\t\t\t\t(progress > pack_to_stdout));\n \tALLOC_ARRAY(written_list, to_pack.nr_objects);\n \twrite_order = compute_write_order();\n \n@@ -2050,9 +2050,9 @@ static void get_object_details(void)\n \tuint32_t i;\n \tstruct object_entry **sorted_by_offset;\n \n-\tif (progress)\n-\t\tprogress_state = start_progress(_(\"Counting objects\"),\n-\t\t\t\t\t\tto_pack.nr_objects);\n+\tprogress_state = start_progress(_(\"Counting objects\"),\n+\t\t\t\t\tto_pack.nr_objects,\n+\t\t\t\t\tprogress);\n \n \tsorted_by_offset = xcalloc(to_pack.nr_objects, sizeof(struct object_entry *));\n \tfor (i = 0; i < to_pack.nr_objects; i++)\n@@ -2847,9 +2847,8 @@ static void prepare_pack(int window, int depth)\n \tif (nr_deltas && n > 1) {\n \t\tunsigned nr_done = 0;\n \n-\t\tif (progress)\n-\t\t\tprogress_state = start_progress(_(\"Compressing objects\"),\n-\t\t\t\t\t\t\tnr_deltas);\n+\t\tprogress_state = start_progress(_(\"Compressing objects\"),\n+\t\t\t\t\t\tnr_deltas, progress);\n \t\tQSORT(delta_list, n, type_size_sort);\n \t\tll_find_deltas(delta_list, n, window+1, depth, &nr_done);\n \t\tstop_progress(&progress_state);\n@@ -3699,8 +3698,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t    the_repository);\n \tprepare_packing_data(the_repository, &to_pack);\n \n-\tif (progress)\n-\t\tprogress_state = start_progress(_(\"Enumerating objects\"), 0);\n+\tprogress_state = start_progress(_(\"Enumerating objects\"), 0, progress);\n \tif (!use_internal_rev_list)\n \t\tread_object_list_from_stdin();\n \telse {\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 02c6ab7cba..004338f7e0 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -41,8 +41,8 @@ static void perform_reachability_traversal(struct rev_info *revs)\n \tif (initialized)\n \t\treturn;\n \n-\tif (show_progress)\n-\t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0);\n+\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0,\n+\t\t\t\t\t  show_progress);\n \tmark_reachable_objects(revs, 1, expire, progress);\n \tstop_progress(&progress);\n \tinitialized = 1;\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex f520111eda..f64cad8390 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (bisect_list)\n \t\trevs.limited = 1;\n \n-\tif (show_progress)\n-\t\tprogress = start_delayed_progress(show_progress, 0);\n+\t/*\n+\t * When progress is not printed to the user, we still want to be able to\n+\t * classify the progress during tracing. So, use a placeholder name.\n+\t */\n+\tprogress = start_delayed_progress(\n+\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n+\t\t\t0, show_progress != NULL);\n \n \tif (use_bitmap_index) {\n \t\tif (!try_bitmap_count(&revs, &filter_options))\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex dd4a75e030..719d446916 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -498,8 +498,7 @@ static void unpack_all(void)\n \t\t\tntohl(hdr->hdr_version));\n \tuse(sizeof(struct pack_header));\n \n-\tif (!quiet)\n-\t\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects);\n+\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects, !quiet);\n \tobj_list = xcalloc(nr_objects, sizeof(*obj_list));\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunpack_one(i);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 328ab06fd4..b9a784fece 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1152,10 +1152,10 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,\n \tstruct progress *progress = NULL;\n \tint i = 0;\n \n-\tif (ctx->report_progress)\n-\t\tprogress = start_delayed_progress(\n-\t\t\t_(\"Writing changed paths Bloom filters index\"),\n-\t\t\tctx->commits.nr);\n+\tprogress = start_delayed_progress(\n+\t\t_(\"Writing changed paths Bloom filters index\"),\n+\t\tctx->commits.nr,\n+\t\tctx->report_progress);\n \n \twhile (list < last) {\n \t\tstruct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);\n@@ -1177,10 +1177,10 @@ static void write_graph_chunk_bloom_data(struct hashfile *f,\n \tstruct progress *progress = NULL;\n \tint i = 0;\n \n-\tif (ctx->report_progress)\n-\t\tprogress = start_delayed_progress(\n-\t\t\t_(\"Writing changed paths Bloom filters data\"),\n-\t\t\tctx->commits.nr);\n+\tprogress = start_delayed_progress(\n+\t\t_(\"Writing changed paths Bloom filters data\"),\n+\t\tctx->commits.nr,\n+\t\tctx->report_progress);\n \n \thashwrite_be32(f, settings->hash_version);\n \thashwrite_be32(f, settings->num_hashes);\n@@ -1213,8 +1213,7 @@ static int add_packed_commits(const struct object_id *oid,\n \toff_t offset = nth_packed_object_offset(pack, pos);\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \n-\tif (ctx->progress)\n-\t\tdisplay_progress(ctx->progress, ++ctx->progress_done);\n+\tdisplay_progress(ctx->progress, ++ctx->progress_done);\n \n \toi.typep = &type;\n \tif (packed_object_info(ctx->r, pack, offset, &oi) < 0)\n@@ -1252,10 +1251,10 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \tenum commit_graph_split_flags flags = ctx->split_opts ?\n \t\tctx->split_opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;\n \n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Loading known commits in commit graph\"),\n-\t\t\t\t\tctx->oids.nr);\n+\tctx->progress = start_delayed_progress(\n+\t\t\t\t_(\"Loading known commits in commit graph\"),\n+\t\t\t\tctx->oids.nr,\n+\t\t\t\tctx->report_progress);\n \tfor (i = 0; i < ctx->oids.nr; i++) {\n \t\tdisplay_progress(ctx->progress, i + 1);\n \t\tcommit = lookup_commit(ctx->r, &ctx->oids.list[i]);\n@@ -1269,10 +1268,9 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \t * than the number of missing commits in the reachable\n \t * closure.\n \t */\n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Expanding reachable commits in commit graph\"),\n-\t\t\t\t\t0);\n+\tctx->progress = start_delayed_progress(\n+\t\t\t\t_(\"Expanding reachable commits in commit graph\"),\n+\t\t\t\t0, ctx->report_progress);\n \tfor (i = 0; i < ctx->oids.nr; i++) {\n \t\tdisplay_progress(ctx->progress, i + 1);\n \t\tcommit = lookup_commit(ctx->r, &ctx->oids.list[i]);\n@@ -1289,10 +1287,9 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \t}\n \tstop_progress(&ctx->progress);\n \n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Clearing commit marks in commit graph\"),\n-\t\t\t\t\tctx->oids.nr);\n+\tctx->progress = start_delayed_progress(\n+\t\t\t\t_(\"Clearing commit marks in commit graph\"),\n+\t\t\t\tctx->oids.nr, ctx->report_progress);\n \tfor (i = 0; i < ctx->oids.nr; i++) {\n \t\tdisplay_progress(ctx->progress, i + 1);\n \t\tcommit = lookup_commit(ctx->r, &ctx->oids.list[i]);\n@@ -1308,10 +1305,10 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \tint i;\n \tstruct commit_list *list = NULL;\n \n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n-\t\t\t\t\tctx->commits.nr);\n+\tctx->progress = start_delayed_progress(\n+\t\t\t\t_(\"Computing commit graph generation numbers\"),\n+\t\t\t\tctx->commits.nr,\n+\t\t\t\tctx->report_progress);\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tuint32_t generation = commit_graph_data_at(ctx->commits.list[i])->generation;\n \n@@ -1362,10 +1359,10 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \n \tinit_bloom_filters();\n \n-\tif (ctx->report_progress)\n-\t\tprogress = start_delayed_progress(\n-\t\t\t_(\"Computing commit changed paths Bloom filters\"),\n-\t\t\tctx->commits.nr);\n+\tprogress = start_delayed_progress(\n+\t\t_(\"Computing commit changed paths Bloom filters\"),\n+\t\tctx->commits.nr,\n+\t\tctx->report_progress);\n \n \tALLOC_ARRAY(sorted_commits, ctx->commits.nr);\n \tCOPY_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);\n@@ -1418,9 +1415,9 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.commits = &commits;\n-\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n-\t\tdata.progress = start_delayed_progress(\n-\t\t\t_(\"Collecting referenced commits\"), 0);\n+\tdata.progress = start_delayed_progress(\n+\t\t_(\"Collecting referenced commits\"), 0,\n+\t\t(flags & COMMIT_GRAPH_WRITE_PROGRESS));\n \n \tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n@@ -1442,15 +1439,14 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \n \tstrbuf_addf(&packname, \"%s/pack/\", ctx->odb->path);\n \tdirlen = packname.len;\n-\tif (ctx->report_progress) {\n-\t\tstrbuf_addf(&progress_title,\n-\t\t\t    Q_(\"Finding commits for commit graph in %d pack\",\n-\t\t\t       \"Finding commits for commit graph in %d packs\",\n-\t\t\t       pack_indexes->nr),\n-\t\t\t    pack_indexes->nr);\n-\t\tctx->progress = start_delayed_progress(progress_title.buf, 0);\n-\t\tctx->progress_done = 0;\n-\t}\n+\tstrbuf_addf(&progress_title,\n+\t\t    Q_(\"Finding commits for commit graph in %d pack\",\n+\t\t       \"Finding commits for commit graph in %d packs\",\n+\t\t       pack_indexes->nr),\n+\t\t    pack_indexes->nr);\n+\tctx->progress = start_delayed_progress(progress_title.buf, 0,\n+\t\t\t\t\t       ctx->report_progress);\n+\tctx->progress_done = 0;\n \tfor (i = 0; i < pack_indexes->nr; i++) {\n \t\tstruct packed_git *p;\n \t\tstrbuf_setlen(&packname, dirlen);\n@@ -1498,10 +1494,10 @@ static int fill_oids_from_commits(struct write_commit_graph_context *ctx,\n \n static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx)\n {\n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t_(\"Finding commits for commit graph among packed objects\"),\n-\t\t\tctx->approx_nr_objects);\n+\tctx->progress = start_delayed_progress(\n+\t\t_(\"Finding commits for commit graph among packed objects\"),\n+\t\tctx->approx_nr_objects,\n+\t\tctx->report_progress);\n \tfor_each_packed_object(add_packed_commits, ctx,\n \t\t\t       FOR_EACH_OBJECT_PACK_ORDER);\n \tif (ctx->progress_done < ctx->approx_nr_objects)\n@@ -1513,10 +1509,9 @@ static uint32_t count_distinct_commits(struct write_commit_graph_context *ctx)\n {\n \tuint32_t i, count_distinct = 1;\n \n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t_(\"Counting distinct commits in commit graph\"),\n-\t\t\tctx->oids.nr);\n+\tctx->progress = start_delayed_progress(\n+\t\t_(\"Counting distinct commits in commit graph\"),\n+\t\tctx->oids.nr, ctx->report_progress);\n \tdisplay_progress(ctx->progress, 0); /* TODO: Measure QSORT() progress */\n \tQSORT(ctx->oids.list, ctx->oids.nr, oid_compare);\n \n@@ -1545,10 +1540,10 @@ static void copy_oids_to_commits(struct write_commit_graph_context *ctx)\n \t\tctx->split_opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;\n \n \tctx->num_extra_edges = 0;\n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t_(\"Finding extra edges in commit graph\"),\n-\t\t\tctx->oids.nr);\n+\tctx->progress = start_delayed_progress(\n+\t\t_(\"Finding extra edges in commit graph\"),\n+\t\tctx->oids.nr,\n+\t\tctx->report_progress);\n \tfor (i = 0; i < ctx->oids.nr; i++) {\n \t\tunsigned int num_parents;\n \n@@ -1723,16 +1718,15 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\thashwrite(f, chunk_write, 12);\n \t}\n \n-\tif (ctx->report_progress) {\n-\t\tstrbuf_addf(&progress_title,\n-\t\t\t    Q_(\"Writing out commit graph in %d pass\",\n-\t\t\t       \"Writing out commit graph in %d passes\",\n-\t\t\t       num_chunks),\n-\t\t\t    num_chunks);\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\tprogress_title.buf,\n-\t\t\tnum_chunks * ctx->commits.nr);\n-\t}\n+\tstrbuf_addf(&progress_title,\n+\t\t    Q_(\"Writing out commit graph in %d pass\",\n+\t\t       \"Writing out commit graph in %d passes\",\n+\t\t       num_chunks),\n+\t\t    num_chunks);\n+\tctx->progress = start_delayed_progress(\n+\t\tprogress_title.buf,\n+\t\tnum_chunks * ctx->commits.nr,\n+\t\tctx->report_progress);\n \twrite_graph_chunk_fanout(f, ctx);\n \twrite_graph_chunk_oids(f, hashsz, ctx);\n \twrite_graph_chunk_data(f, hashsz, ctx);\n@@ -1930,10 +1924,10 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)\n {\n \tuint32_t i;\n \n-\tif (ctx->report_progress)\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Scanning merged commits\"),\n-\t\t\t\t\tctx->commits.nr);\n+\tctx->progress = start_delayed_progress(\n+\t\t\t\t_(\"Scanning merged commits\"),\n+\t\t\t\tctx->commits.nr,\n+\t\t\t\tctx->report_progress);\n \n \tQSORT(ctx->commits.list, ctx->commits.nr, commit_compare);\n \n@@ -1965,8 +1959,8 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)\n \twhile (g && current_graph_number >= ctx->num_commit_graphs_after) {\n \t\tcurrent_graph_number--;\n \n-\t\tif (ctx->report_progress)\n-\t\t\tctx->progress = start_delayed_progress(_(\"Merging commit-graph\"), 0);\n+\t\tctx->progress = start_delayed_progress(_(\"Merging commit-graph\"), 0,\n+\t\t\t\t\t\t       ctx->report_progress);\n \n \t\tmerge_commit_graph(ctx, g);\n \t\tstop_progress(&ctx->progress);\n@@ -2301,9 +2295,9 @@ 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-\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+\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n+\t\t\t\tg->num_commits,\n+\t\t\t\t(flags & COMMIT_GRAPH_WRITE_PROGRESS));\n \n \tfor (i = 0; i < g->num_commits; i++) {\n \t\tstruct commit *graph_commit, *odb_commit;\ndiff --git a/delta-islands.c b/delta-islands.c\nindex aa98b2e541..102d4a5573 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -268,8 +268,8 @@ void resolve_tree_islands(struct repository *r,\n \t}\n \tQSORT(todo, nr, tree_depth_compare);\n \n-\tif (progress)\n-\t\tprogress_state = start_progress(_(\"Propagating island marks\"), nr);\n+\tprogress_state = start_progress(_(\"Propagating island marks\"), nr,\n+\t\t\t\t\tprogress);\n \n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object_entry *ent = todo[i].entry;\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 99e63e90f8..4fd5a7b9da 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -587,11 +587,10 @@ void diffcore_rename(struct diff_options *options)\n \t\tbreak;\n \t}\n \n-\tif (options->show_rename_progress) {\n-\t\tprogress = start_delayed_progress(\n-\t\t\t\t_(\"Performing inexact rename detection\"),\n-\t\t\t\t(uint64_t)rename_dst_nr * (uint64_t)rename_src_nr);\n-\t}\n+\tprogress = start_delayed_progress(\n+\t\t\t_(\"Performing inexact rename detection\"),\n+\t\t\t(uint64_t)rename_dst_nr * (uint64_t)rename_src_nr,\n+\t\t\toptions->show_rename_progress);\n \n \tmx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));\n \tfor (dst_cnt = i = 0; i < rename_dst_nr; i++) {\ndiff --git a/entry.c b/entry.c\nindex 449bd32dee..e46e5b56ed 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -174,7 +174,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)\n \n \tdco->state = CE_RETRY;\n \tdelayed_object_count = dco->paths.nr;\n-\tprogress = start_delayed_progress(_(\"Filtering content\"), delayed_object_count);\n+\tprogress = start_delayed_progress(_(\"Filtering content\"), delayed_object_count, 1);\n \twhile (dco->filters.nr > 0) {\n \t\tfor_each_string_list_item(filter, &dco->filters) {\n \t\t\tstruct string_list available_paths = STRING_LIST_INIT_NODUP;\ndiff --git a/midx.c b/midx.c\nindex 6d1584ca51..d9ab163340 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -836,10 +836,8 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t}\n \n \tpacks.pack_paths_checked = 0;\n-\tif (flags & MIDX_PROGRESS)\n-\t\tpacks.progress = start_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n-\telse\n-\t\tpacks.progress = NULL;\n+\tpacks.progress = start_progress(_(\"Adding packfiles to multi-pack-index\"), 0,\n+\t\t\t\t\t\t(flags & MIDX_PROGRESS));\n \n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &packs);\n \tstop_progress(&packs.progress);\n@@ -973,9 +971,8 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\twritten += MIDX_CHUNKLOOKUP_WIDTH;\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_progress(_(\"Writing chunks to multi-pack-index\"),\n-\t\t\t\t\t  num_chunks);\n+\tprogress = start_progress(_(\"Writing chunks to multi-pack-index\"),\n+\t\t\t\t  num_chunks, (flags & MIDX_PROGRESS));\n \tfor (i = 0; i < num_chunks; i++) {\n \t\tif (written != chunk_offsets[i])\n \t\t\tBUG(\"incorrect chunk offset (%\"PRIu64\" != %\"PRIu64\") for chunk id %\"PRIx32,\n@@ -1108,9 +1105,8 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \tif (!m)\n \t\treturn 0;\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_progress(_(\"Looking for referenced packfiles\"),\n-\t\t\t\t\t  m->num_packs);\n+\tprogress = start_progress(_(\"Looking for referenced packfiles\"),\n+\t\t\t\t  m->num_packs, (flags & MIDX_PROGRESS));\n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tif (prepare_midx_pack(r, m, i))\n \t\t\tmidx_report(\"failed to load pack in position %d\", i);\n@@ -1137,9 +1133,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\treturn verify_midx_error;\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n-\t\t\t\t\t\t m->num_objects - 1);\n+\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n+\t\t\t\t\t m->num_objects - 1,\n+\t\t\t\t\t (flags & MIDX_PROGRESS));\n \tfor (i = 0; i < m->num_objects - 1; i++) {\n \t\tstruct object_id oid1, oid2;\n \n@@ -1166,15 +1162,16 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\tpairs[i].pack_int_id = nth_midxed_pack_int_id(m, i);\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_sparse_progress(_(\"Sorting objects by packfile\"),\n-\t\t\t\t\t\t m->num_objects);\n+\tprogress = start_sparse_progress(_(\"Sorting objects by packfile\"),\n+\t\t\t\t\t m->num_objects,\n+\t\t\t\t\t (flags & MIDX_PROGRESS));\n \tdisplay_progress(progress, 0); /* TODO: Measure QSORT() progress */\n \tQSORT(pairs, m->num_objects, compare_pair_pos_vs_id);\n \tstop_progress(&progress);\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_sparse_progress(_(\"Verifying object offsets\"), m->num_objects);\n+\tprogress = start_sparse_progress(_(\"Verifying object offsets\"),\n+\t\t\t\t\t m->num_objects,\n+\t\t\t\t\t (flags & MIDX_PROGRESS));\n \tfor (i = 0; i < m->num_objects; i++) {\n \t\tstruct object_id oid;\n \t\tstruct pack_entry e;\n@@ -1229,9 +1226,8 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n \n \tcount = xcalloc(m->num_packs, sizeof(uint32_t));\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_progress(_(\"Counting referenced objects\"),\n-\t\t\t\t\t  m->num_objects);\n+\tprogress = start_progress(_(\"Counting referenced objects\"),\n+\t\t\t\t  m->num_objects, (flags & MIDX_PROGRESS));\n \tfor (i = 0; i < m->num_objects; i++) {\n \t\tint pack_int_id = nth_midxed_pack_int_id(m, i);\n \t\tcount[pack_int_id]++;\n@@ -1239,9 +1235,8 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n \t}\n \tstop_progress(&progress);\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_progress(_(\"Finding and deleting unreferenced packfiles\"),\n-\t\t\t\t\t  m->num_packs);\n+\tprogress = start_progress(_(\"Finding and deleting unreferenced packfiles\"),\n+\t\t\t\t  m->num_packs, (flags & MIDX_PROGRESS));\n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tchar *pack_name;\n \t\tdisplay_progress(progress, i + 1);\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex a7a4964b50..9a8bfefeb5 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -259,8 +259,8 @@ void bitmap_writer_build(struct packing_data *to_pack)\n \twriter.bitmaps = kh_init_oid_map();\n \twriter.to_pack = to_pack;\n \n-\tif (writer.show_progress)\n-\t\twriter.progress = start_progress(\"Building bitmaps\", writer.selected_nr);\n+\twriter.progress = start_progress(\"Building bitmaps\", writer.selected_nr,\n+\t\t\t\t\t writer.show_progress);\n \n \trepo_init_revisions(to_pack->repo, &revs, NULL);\n \trevs.tag_objects = 1;\n@@ -397,8 +397,8 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n \n \tQSORT(indexed_commits, indexed_commits_nr, date_compare);\n \n-\tif (writer.show_progress)\n-\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n+\twriter.progress = start_progress(\"Selecting bitmap commits\", 0,\n+\t\t\t\t\t writer.show_progress);\n \n \tif (indexed_commits_nr < 100) {\n \t\tfor (i = 0; i < indexed_commits_nr; ++i)\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 4077e731e8..73232d460f 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1313,7 +1313,7 @@ void test_bitmap_walk(struct rev_info *revs)\n \n \ttdata.bitmap_git = bitmap_git;\n \ttdata.base = bitmap_new();\n-\ttdata.prg = start_progress(\"Verifying bitmap entries\", result_popcnt);\n+\ttdata.prg = start_progress(\"Verifying bitmap entries\", result_popcnt, 1);\n \ttdata.seen = 0;\n \n \ttraverse_commit_list(revs, &test_show_commit, &test_show_object, &tdata);\n@@ -1392,8 +1392,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,\n \trebuild = bitmap_new();\n \ti = 0;\n \n-\tif (show_progress)\n-\t\tprogress = start_progress(\"Reusing bitmaps\", 0);\n+\tprogress = start_progress(\"Reusing bitmaps\", 0, show_progress);\n \n \tkh_foreach_value(bitmap_git->bitmaps, stored, {\n \t\tif (stored->flags & BITMAP_FLAG_REUSE) {\ndiff --git a/preload-index.c b/preload-index.c\nindex ed6eaa4738..a924a84a14 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -115,10 +115,9 @@ void preload_index(struct index_state *index,\n \tmemset(&data, 0, sizeof(data));\n \n \tmemset(&pd, 0, sizeof(pd));\n-\tif (refresh_flags & REFRESH_PROGRESS && isatty(2)) {\n-\t\tpd.progress = start_delayed_progress(_(\"Refreshing index\"), index->cache_nr);\n-\t\tpthread_mutex_init(&pd.mutex, NULL);\n-\t}\n+\tpd.progress = start_delayed_progress(_(\"Refreshing index\"), index->cache_nr,\n+\t\t\t\t\t     (refresh_flags & REFRESH_PROGRESS && isatty(2)));\n+\tpthread_mutex_init(&pd.mutex, NULL);\n \n \tfor (i = 0; i < threads; i++) {\n \t\tstruct thread_data *p = data+i;\ndiff --git a/progress.c b/progress.c\nindex 3eda914518..f014908b05 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -43,6 +43,7 @@ struct progress {\n \tstruct strbuf counters_sb;\n \tint title_len;\n \tint split;\n+\tint verbose;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -245,12 +246,13 @@ void display_throughput(struct progress *progress, uint64_t total)\n \n void display_progress(struct progress *progress, uint64_t n)\n {\n-\tif (progress)\n+\tif (progress && progress->verbose)\n \t\tdisplay(progress, n, NULL);\n }\n \n static struct progress *start_progress_delay(const char *title, uint64_t total,\n-\t\t\t\t\t     unsigned delay, unsigned sparse)\n+\t\t\t\t\t     unsigned delay, unsigned sparse,\n+\t\t\t\t\t     int verbose)\n {\n \tstruct progress *progress = xmalloc(sizeof(*progress));\n \tprogress->title = title;\n@@ -264,6 +266,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tstrbuf_init(&progress->counters_sb, 0);\n \tprogress->title_len = utf8_strwidth(title);\n \tprogress->split = 0;\n+\tprogress->verbose = verbose;\n \tset_progress_signal();\n \ttrace2_region_enter(\"progress\", title, the_repository);\n \treturn progress;\n@@ -279,14 +282,16 @@ static int get_default_delay(void)\n \treturn delay_in_secs;\n }\n \n-struct progress *start_delayed_progress(const char *title, uint64_t total)\n+struct progress *start_delayed_progress(const char *title, uint64_t total,\n+\t\t\t\t\tint verbose)\n {\n-\treturn start_progress_delay(title, total, get_default_delay(), 0);\n+\treturn start_progress_delay(title, total, get_default_delay(), 0,\n+\t\t\t\t    verbose);\n }\n \n-struct progress *start_progress(const char *title, uint64_t total)\n+struct progress *start_progress(const char *title, uint64_t total, int verbose)\n {\n-\treturn start_progress_delay(title, total, 0, 0);\n+\treturn start_progress_delay(title, total, 0, 0, verbose);\n }\n \n /*\n@@ -298,15 +303,17 @@ struct progress *start_progress(const char *title, uint64_t total)\n  * When \"sparse\" is set, stop_progress() will automatically force the done\n  * message to show 100%.\n  */\n-struct progress *start_sparse_progress(const char *title, uint64_t total)\n+struct progress *start_sparse_progress(const char *title, uint64_t total,\n+\t\t\t\t       int verbose)\n {\n-\treturn start_progress_delay(title, total, 0, 1);\n+\treturn start_progress_delay(title, total, 0, 1, verbose);\n }\n \n struct progress *start_delayed_sparse_progress(const char *title,\n-\t\t\t\t\t       uint64_t total)\n+\t\t\t\t\t       uint64_t total, int verbose)\n {\n-\treturn start_progress_delay(title, total, get_default_delay(), 1);\n+\treturn start_progress_delay(title, total, get_default_delay(), 1,\n+\t\t\t\t    verbose);\n }\n \n static void finish_if_sparse(struct progress *progress)\ndiff --git a/progress.h b/progress.h\nindex f1913acf73..0c64557231 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -13,11 +13,13 @@ void progress_test_force_update(void);\n \n void display_throughput(struct progress *progress, uint64_t total);\n void display_progress(struct progress *progress, uint64_t n);\n-struct progress *start_progress(const char *title, uint64_t total);\n-struct progress *start_sparse_progress(const char *title, uint64_t total);\n-struct progress *start_delayed_progress(const char *title, uint64_t total);\n+struct progress *start_progress(const char *title, uint64_t total, int verbose);\n+struct progress *start_sparse_progress(const char *title, uint64_t total,\n+\t\t\t\t       int verbose);\n+struct progress *start_delayed_progress(const char *title, uint64_t total,\n+\t\t\t\t\tint verbose);\n struct progress *start_delayed_sparse_progress(const char *title,\n-\t\t\t\t\t       uint64_t total);\n+\t\t\t\t\t       uint64_t total, int verbose);\n void stop_progress(struct progress **progress);\n void stop_progress_msg(struct progress **progress, const char *msg);\n \ndiff --git a/prune-packed.c b/prune-packed.c\nindex 261520b472..dcc982e369 100644\n--- a/prune-packed.c\n+++ b/prune-packed.c\n@@ -31,8 +31,8 @@ static int prune_object(const struct object_id *oid, const char *path,\n \n void prune_packed_objects(int opts)\n {\n-\tif (opts & PRUNE_PACKED_VERBOSE)\n-\t\tprogress = start_delayed_progress(_(\"Removing duplicate objects\"), 256);\n+\tprogress = start_delayed_progress(_(\"Removing duplicate objects\"), 256,\n+\t\t\t\t\t  (opts & PRUNE_PACKED_VERBOSE));\n \n \tfor_each_loose_file_in_objdir(get_object_directory(),\n \t\t\t\t      prune_object, NULL, prune_subdir, &opts);\ndiff --git a/read-cache.c b/read-cache.c\nindex aa427c5c17..2ddc422dbd 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1532,9 +1532,9 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \tconst char *unmerged_fmt;\n \tstruct progress *progress = NULL;\n \n-\tif (flags & REFRESH_PROGRESS && isatty(2))\n-\t\tprogress = start_delayed_progress(_(\"Refresh index\"),\n-\t\t\t\t\t\t  istate->cache_nr);\n+\tprogress = start_delayed_progress(_(\"Refresh index\"),\n+\t\t\t\t\t  istate->cache_nr,\n+\t\t\t\t\t  (flags & REFRESH_PROGRESS && isatty(2)));\n \n \ttrace_performance_enter();\n \tmodified_fmt   = in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\";\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 5d05cbe789..19b874f9cd 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -23,16 +23,18 @@\n int cmd__progress(int argc, const char **argv)\n {\n \tint total = 0;\n+\tint quiet = 0;\n \tconst char *title;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct progress *progress;\n \n \tconst char *usage[] = {\n-\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n+\t\t\"test-tool progress [--total=<n>] [--quiet] <progress-title>\",\n \t\tNULL\n \t};\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n+\t\tOPT_BOOL(0, \"quiet\", &quiet, \"suppress stderr\"),\n \t\tOPT_END(),\n \t};\n \n@@ -42,7 +44,7 @@ int cmd__progress(int argc, const char **argv)\n \ttitle = argv[0];\n \n \tprogress_testing = 1;\n-\tprogress = start_progress(title, total);\n+\tprogress = start_progress(title, total, !quiet);\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n \t\tchar *end;\n \ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 1ed1df351c..9d6e6274ad 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -309,4 +309,31 @@ test_expect_success 'progress generates traces' '\n \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n '\n \n+test_expect_success 'progress generates traces even quietly' '\n+\tcat >in <<-\\EOF &&\n+\tthroughput 102400 1000\n+\tupdate\n+\tprogress 10\n+\tthroughput 204800 2000\n+\tupdate\n+\tprogress 20\n+\tthroughput 307200 3000\n+\tupdate\n+\tprogress 30\n+\tthroughput 409600 4000\n+\tupdate\n+\tprogress 40\n+\tEOF\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n+\t\t--quiet \"Working hard\" <in 2>stderr &&\n+\n+\t# t0212/parse_events.perl intentionally omits regions and data.\n+\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n+\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n+'\n+\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 4be5fc3075..fa84244466 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -338,16 +338,14 @@ static struct progress *get_progress(struct unpack_trees_options *o,\n {\n \tunsigned cnt = 0, total = 0;\n \n-\tif (!o->update || !o->verbose_update)\n-\t\treturn NULL;\n-\n \tfor (; cnt < index->cache_nr; cnt++) {\n \t\tconst struct cache_entry *ce = index->cache[cnt];\n \t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n \t\t\ttotal++;\n \t}\n \n-\treturn start_delayed_progress(_(\"Updating files\"), total);\n+\treturn start_delayed_progress(_(\"Updating files\"), total,\n+\t\t\t\t      (o->update && o->verbose_update));\n }\n \n static void setup_collided_checkout_detection(struct checkout *state,\n@@ -1493,10 +1491,10 @@ static int clear_ce_flags(struct index_state *istate,\n \tint rval;\n \n \tstrbuf_reset(&prefix);\n-\tif (show_progress)\n-\t\tistate->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Updating index flags\"),\n-\t\t\t\t\tistate->cache_nr);\n+\tistate->progress = start_delayed_progress(\n+\t\t\t\t_(\"Updating index flags\"),\n+\t\t\t\tistate->cache_nr,\n+\t\t\t\tshow_progress);\n \n \txsnprintf(label, sizeof(label), \"clear_ce_flags(0x%08lx,0x%08lx)\",\n \t\t  (unsigned long)select_mask, (unsigned long)clear_mask);\ndiff --git a/walker.c b/walker.c\nindex 4984bf8b3d..4758bdde70 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -166,8 +166,8 @@ static int loop(struct walker *walker)\n \tstruct progress *progress = NULL;\n \tuint64_t nr = 0;\n \n-\tif (walker->get_progress)\n-\t\tprogress = start_delayed_progress(_(\"Fetching objects\"), 0);\n+\tprogress = start_delayed_progress(_(\"Fetching objects\"), 0,\n+\t\t\t\t\t  walker->get_progress);\n \n \twhile (process_queue) {\n \t\tstruct object *obj = process_queue->item;\n-- \n2.27.0.383.g050319c2ae-goog\n\n"},{"id":"401283","messageId":"20200710014242.1088216-3-emilyshaffer@google.com","threadId":"53829","inReplyTo":"20200710014242.1088216-1-emilyshaffer@google.com","subject":"[PATCH 2/2] progress: remove redundant null-checking","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T01:42:42Z","receivedAt":"2020-07-10T01:42:58Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"display_progress() and stop_progress() are tolerant to NULL inputs.\nRemove some places where redundant checks are performed before these\nNULL-tolerant functions are called.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n builtin/commit-graph.c | 3 +--\n commit-graph.c         | 3 +--\n read-cache.c           | 9 +++------\n 3 files changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex ae4dc2dfcd..7964de3126 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -263,8 +263,7 @@ static int graph_write(int argc, const char **argv)\n cleanup:\n \tstring_list_clear(&pack_indexes, 0);\n \tstrbuf_release(&buf);\n-\tif (progress)\n-\t\tstop_progress(&progress);\n+\tstop_progress(&progress);\n \treturn result;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b9a784fece..30d9a75932 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1424,8 +1424,7 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t    flags, split_opts);\n \n \toidset_clear(&commits);\n-\tif (data.progress)\n-\t\tstop_progress(&data.progress);\n+\tstop_progress(&data.progress);\n \treturn result;\n }\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 2ddc422dbd..feb7abe37a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1581,8 +1581,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \t\tnew_entry = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);\n \t\tif (new_entry == ce)\n \t\t\tcontinue;\n-\t\tif (progress)\n-\t\t\tdisplay_progress(progress, i);\n+\t\tdisplay_progress(progress, i);\n \t\tif (!new_entry) {\n \t\t\tconst char *fmt;\n \n@@ -1614,10 +1613,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \n \t\treplace_index_entry(istate, i, new_entry);\n \t}\n-\tif (progress) {\n-\t\tdisplay_progress(progress, istate->cache_nr);\n-\t\tstop_progress(&progress);\n-\t}\n+\tdisplay_progress(progress, istate->cache_nr);\n+\tstop_progress(&progress);\n \ttrace_performance_leave(\"refresh index\");\n \treturn has_errors;\n }\n-- \n2.27.0.383.g050319c2ae-goog\n\n"},{"id":"401285","messageId":"ed6b2339-83cd-a7b9-c653-bc86d8686680@gmail.com","threadId":"53829","inReplyTo":"20200710014242.1088216-2-emilyshaffer@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-10T02:00:23Z","receivedAt":"2020-07-10T02:00:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/9/2020 9:42 PM, Emily Shaffer wrote:\n> Before now, the progress API is used by conditionally calling\n> start_progress() or a similar call, and then unconditionally calling\n> display_progress() and stop_progress(), both of which are tolerant of\n> NULL or uninitialized inputs. However, in\n> 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> throughput), the progress library learned to log traces during expensive\n> operations. In cases where progress should not be displayed to the user\n> - such as when Git is called by a script - no traces will be logged,\n> because the progress object is never created.\n> \n> Instead, to allow us to collect traces from scripted Git commands, teach\n> a progress->verbose flag, which is specified via a new argument to\n> start_progress() and friends. display_progress() also learns to filter\n> for that flag. With these changes, start_progress() can be called\n> unconditionally but with a conditional as an argument to determine\n> whether to report progress to the user.\n> \n> Since this changes the API, also modify callers of start_progress() and\n> friends to drop their conditional and pass a new argument in instead.\n\nThis is a worthwhile change. Thanks! I was hoping that we would\nget some of these regions for free, which extends what we can get\nout of trace2 events.\n\nCC'ing Taylor because he had some thoughts on adding a possible\ntrace2 category to make it easier to reason about the regions,\nwhen appropriate. Not sure if he's ready to apply that change\non top of this series.\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index f520111eda..f64cad8390 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n>  \tif (bisect_list)\n>  \t\trevs.limited = 1;\n>  \n> -\tif (show_progress)\n> -\t\tprogress = start_delayed_progress(show_progress, 0);\n> +\t/*\n> +\t * When progress is not printed to the user, we still want to be able to\n> +\t * classify the progress during tracing. So, use a placeholder name.\n> +\t */\n> +\tprogress = start_delayed_progress(\n> +\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n> +\t\t\t0, show_progress != NULL)\n\nThis is so strange, how we let the command-lines specify a progress\nindicator. I guess it is necessary when we use rev-list as a\nsubcommand instead of in-process. One such case is check_connected()\nin connected.c.\n\nIt's stranger still that \"show_progress\" is actually a string here,\nas opposed to being an int in most other places.\n\nYour transformation is correct, here, though. Thanks for calling it\nout in the commit message.\n\n>  \n>  \tif (use_bitmap_index) {\n>  \t\tif (!try_bitmap_count(&revs, &filter_options))\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index dd4a75e030..719d446916 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -498,8 +498,7 @@ static void unpack_all(void)\n>  \t\t\tntohl(hdr->hdr_version));\n>  \tuse(sizeof(struct pack_header));\n>  \n> -\tif (!quiet)\n> -\t\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects);\n> +\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects, !quiet);\n>  \tobj_list = xcalloc(nr_objects, sizeof(*obj_list));\n>  \tfor (i = 0; i < nr_objects; i++) {\n>  \t\tunpack_one(i);\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 328ab06fd4..b9a784fece 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1152,10 +1152,10 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,\n>  \tstruct progress *progress = NULL;\n>  \tint i = 0;\n>  \n> -\tif (ctx->report_progress)\n> -\t\tprogress = start_delayed_progress(\n> -\t\t\t_(\"Writing changed paths Bloom filters index\"),\n> -\t\t\tctx->commits.nr);\n> +\tprogress = start_delayed_progress(\n> +\t\t_(\"Writing changed paths Bloom filters index\"),\n> +\t\tctx->commits.nr,\n> +\t\tctx->report_progress);\n\nThere are a lot of blocks like this, where the progress string is long enough to\nrequire the first param to be after the method name. Since we are changing the\nAPI and every caller, would the resulting code be cleaner if the string value\nwas the last parameter? That would allow this code pattern in most cases:\n\n\tprogress = start_delayed_progress(count, show_progress,\n\t\t\t\t\t  _(\"My special string!\"));\n\nJust a thought. Not super-important.\n\nThe rest of the changes look to be correct.\n\n> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\n> index 5d05cbe789..19b874f9cd 100644\n> --- a/t/helper/test-progress.c\n> +++ b/t/helper/test-progress.c\n> @@ -23,16 +23,18 @@\n>  int cmd__progress(int argc, const char **argv)\n>  {\n>  \tint total = 0;\n> +\tint quiet = 0;\n>  \tconst char *title;\n>  \tstruct strbuf line = STRBUF_INIT;\n>  \tstruct progress *progress;\n>  \n>  \tconst char *usage[] = {\n> -\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n> +\t\t\"test-tool progress [--total=<n>] [--quiet] <progress-title>\",\n>  \t\tNULL\n>  \t};\n>  \tstruct option options[] = {\n>  \t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n> +\t\tOPT_BOOL(0, \"quiet\", &quiet, \"suppress stderr\"),\n>  \t\tOPT_END(),\n>  \t};\n>  \n> @@ -42,7 +44,7 @@ int cmd__progress(int argc, const char **argv)\n>  \ttitle = argv[0];\n>  \n>  \tprogress_testing = 1;\n> -\tprogress = start_progress(title, total);\n> +\tprogress = start_progress(title, total, !quiet);\n>  \twhile (strbuf_getline(&line, stdin) != EOF) {\n>  \t\tchar *end;\n>  \n> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> index 1ed1df351c..9d6e6274ad 100755\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -309,4 +309,31 @@ test_expect_success 'progress generates traces' '\n>  \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n>  '\n>  \n> +test_expect_success 'progress generates traces even quietly' '\n> +\tcat >in <<-\\EOF &&\n> +\tthroughput 102400 1000\n> +\tupdate\n> +\tprogress 10\n> +\tthroughput 204800 2000\n> +\tupdate\n> +\tprogress 20\n> +\tthroughput 307200 3000\n> +\tupdate\n> +\tprogress 30\n> +\tthroughput 409600 4000\n> +\tupdate\n> +\tprogress 40\n> +\tEOF\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n> +\t\t--quiet \"Working hard\" <in 2>stderr &&\n> +\n> +\t# t0212/parse_events.perl intentionally omits regions and data.\n> +\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n> +\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> +'\n\nThanks for adding a test, including the resulting trace events!\n\nThe patch of Taylor's that I mentioned earlier changes the \"category\"\nto something a bit more specific than \"progress\", when appropriate.\n\n> +\n> +\n>  test_done\n\nnit: extra empty line\n\nThanks!\n-Stolee\n"},{"id":"401286","messageId":"aebbc1ca-9936-ce1b-bacc-855d353f83f9@gmail.com","threadId":"53829","inReplyTo":"20200710014242.1088216-3-emilyshaffer@google.com","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-10T02:01:08Z","receivedAt":"2020-07-10T02:01:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/9/2020 9:42 PM, Emily Shaffer wrote:\n> display_progress() and stop_progress() are tolerant to NULL inputs.\n> Remove some places where redundant checks are performed before these\n> NULL-tolerant functions are called.\n> \n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  builtin/commit-graph.c | 3 +--\n>  commit-graph.c         | 3 +--\n>  read-cache.c           | 9 +++------\n>  3 files changed, 5 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index ae4dc2dfcd..7964de3126 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -263,8 +263,7 @@ static int graph_write(int argc, const char **argv)\n>  cleanup:\n>  \tstring_list_clear(&pack_indexes, 0);\n>  \tstrbuf_release(&buf);\n> -\tif (progress)\n> -\t\tstop_progress(&progress);\n> +\tstop_progress(&progress);\n>  \treturn result;\n>  }\n>  \n> diff --git a/commit-graph.c b/commit-graph.c\n> index b9a784fece..30d9a75932 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1424,8 +1424,7 @@ int write_commit_graph_reachable(struct object_directory *odb,\n>  \t\t\t\t    flags, split_opts);\n>  \n>  \toidset_clear(&commits);\n> -\tif (data.progress)\n> -\t\tstop_progress(&data.progress);\n> +\tstop_progress(&data.progress);\n>  \treturn result;\n>  }\n>  \n> diff --git a/read-cache.c b/read-cache.c\n> index 2ddc422dbd..feb7abe37a 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1581,8 +1581,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>  \t\tnew_entry = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);\n>  \t\tif (new_entry == ce)\n>  \t\t\tcontinue;\n> -\t\tif (progress)\n> -\t\t\tdisplay_progress(progress, i);\n> +\t\tdisplay_progress(progress, i);\n>  \t\tif (!new_entry) {\n>  \t\t\tconst char *fmt;\n>  \n> @@ -1614,10 +1613,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>  \n>  \t\treplace_index_entry(istate, i, new_entry);\n>  \t}\n> -\tif (progress) {\n> -\t\tdisplay_progress(progress, istate->cache_nr);\n> -\t\tstop_progress(&progress);\n> -\t}\n> +\tdisplay_progress(progress, istate->cache_nr);\n> +\tstop_progress(&progress);\n>  \ttrace_performance_leave(\"refresh index\");\n>  \treturn has_errors;\n\nLooks obviously correct to me.\n\nThanks,\n-Stolee\n"},{"id":"401288","messageId":"20200710021451.GI9782@camp.crustytoothpaste.net","threadId":"53829","inReplyTo":"20200710014242.1088216-2-emilyshaffer@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-07-10T02:14:51Z","receivedAt":"2020-07-10T02:15:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-07-10 at 01:42:41, Emily Shaffer wrote:\n> Before now, the progress API is used by conditionally calling\n> start_progress() or a similar call, and then unconditionally calling\n> display_progress() and stop_progress(), both of which are tolerant of\n> NULL or uninitialized inputs. However, in\n> 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> throughput), the progress library learned to log traces during expensive\n> operations. In cases where progress should not be displayed to the user\n> - such as when Git is called by a script - no traces will be logged,\n> because the progress object is never created.\n> \n> Instead, to allow us to collect traces from scripted Git commands, teach\n> a progress->verbose flag, which is specified via a new argument to\n> start_progress() and friends. display_progress() also learns to filter\n> for that flag. With these changes, start_progress() can be called\n> unconditionally but with a conditional as an argument to determine\n> whether to report progress to the user.\n\nSo to make sure I understand this right, we'll collect traces regardless\nif it's enabled, but we'll still honor the --quiet flag if the user\ndoesn't want to see them?  If so, I'm definitely in favor of this\nchange.  I was worried when I read the cover letter that we'd display\nthem to the user regardless, but from reading the patch and the commit\nmessage, it seems I misunderstood.\n\nI think the making the verbose flag a parameter simplifies the code\nnicely and puts the rendering decision in the right place.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"401289","messageId":"20200710021740.GA39052@syl.lan","threadId":"53829","inReplyTo":"ed6b2339-83cd-a7b9-c653-bc86d8686680@gmail.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-10T02:17:40Z","receivedAt":"2020-07-10T02:17:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi both,\n\nOn Thu, Jul 09, 2020 at 10:00:23PM -0400, Derrick Stolee wrote:\n> On 7/9/2020 9:42 PM, Emily Shaffer wrote:\n> > Before now, the progress API is used by conditionally calling\n> > start_progress() or a similar call, and then unconditionally calling\n> > display_progress() and stop_progress(), both of which are tolerant of\n> > NULL or uninitialized inputs. However, in\n> > 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> > throughput), the progress library learned to log traces during expensive\n> > operations. In cases where progress should not be displayed to the user\n> > - such as when Git is called by a script - no traces will be logged,\n> > because the progress object is never created.\n\nThis is such a fantastic idea. Just the other day, I was thinking of\ngetting your (for clarification, Emily, since I'm responding to\nStolee's mail) progress-emits-trace2-events work hooked into GitHub's\ntrace2 pipeline.\n\nThere were two unfortunate things that prevented this from working:\n\n  1. GitHub filters which trace2 categories are of interest to us (these\n     interesting ones get logged, and the uninteresting ones get\n     discarded) using an environment variable of comma-separated\n     categories. Since all of the trace2 metrics generated by the\n     progress API don't have categories, taking in one interesting\n     metric meant taking them all in, which is a non-starter for us.\n\n  2. On top of that, we don't even _generate_ these progress events most\n     of the time, since we're often running without a tty, and so we\n     never end up hitting those 'if (progress) start_progress()'\n     conditionals in the first place.\n\nIf we had something like this, it would reduce the problem to only (1),\nwhich would make a lot of my headaches go away. (It would also give me a\ngood excuse to convert many of our custom trace2 regions into patches on\nthe list, and get rid of a non-trivial amount of code that generates\nmerge conflicts often).\n\n> > Instead, to allow us to collect traces from scripted Git commands, teach\n> > a progress->verbose flag, which is specified via a new argument to\n> > start_progress() and friends. display_progress() also learns to filter\n> > for that flag. With these changes, start_progress() can be called\n> > unconditionally but with a conditional as an argument to determine\n> > whether to report progress to the user.\n> >\n> > Since this changes the API, also modify callers of start_progress() and\n> > friends to drop their conditional and pass a new argument in instead.\n\nI don't think that this is why I was CC'd, but could you perhaps talk a\nlittle bit about why this is all in the same patch? I don't think this\nchange needs to be broken out by the area affected per-se, but the\ncurrent form is a little unruly to review all at once.\n\n> This is a worthwhile change. Thanks! I was hoping that we would\n> get some of these regions for free, which extends what we can get\n> out of trace2 events.\n>\n> CC'ing Taylor because he had some thoughts on adding a possible\n> trace2 category to make it easier to reason about the regions,\n> when appropriate. Not sure if he's ready to apply that change\n> on top of this series.\n\nThis is what I was talking about above. It would be nice if we could\nsomehow teach the 'start_*_progress()' API about a trace2 category.\n\nUnfortunately, this would need to be a new parameter, since we need to\nknow the category when we enter the region. So, the API changes might be\nfar-reaching. It would be nice if there was a way to limit the blast\nradius (i.e., 'start_progress_trace2(..., category)'), but I haven't\nthought deeply about it.\n\nI don't want to delay this patch series with that. I'd be happy to build\nit in myself on top after this graduates.\n\n> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > index f520111eda..f64cad8390 100644\n> > --- a/builtin/rev-list.c\n> > +++ b/builtin/rev-list.c\n> > @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> >  \tif (bisect_list)\n> >  \t\trevs.limited = 1;\n> >\n> > -\tif (show_progress)\n> > -\t\tprogress = start_delayed_progress(show_progress, 0);\n> > +\t/*\n> > +\t * When progress is not printed to the user, we still want to be able to\n> > +\t * classify the progress during tracing. So, use a placeholder name.\n> > +\t */\n> > +\tprogress = start_delayed_progress(\n> > +\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n> > +\t\t\t0, show_progress != NULL)\n>\n> This is so strange, how we let the command-lines specify a progress\n> indicator. I guess it is necessary when we use rev-list as a\n> subcommand instead of in-process. One such case is check_connected()\n> in connected.c.\n>\n> It's stranger still that \"show_progress\" is actually a string here,\n> as opposed to being an int in most other places.\n>\n> Your transformation is correct, here, though. Thanks for calling it\n> out in the commit message.\n>\n> >\n> >  \tif (use_bitmap_index) {\n> >  \t\tif (!try_bitmap_count(&revs, &filter_options))\n> > diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> > index dd4a75e030..719d446916 100644\n> > --- a/builtin/unpack-objects.c\n> > +++ b/builtin/unpack-objects.c\n> > @@ -498,8 +498,7 @@ static void unpack_all(void)\n> >  \t\t\tntohl(hdr->hdr_version));\n> >  \tuse(sizeof(struct pack_header));\n> >\n> > -\tif (!quiet)\n> > -\t\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects);\n> > +\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects, !quiet);\n> >  \tobj_list = xcalloc(nr_objects, sizeof(*obj_list));\n> >  \tfor (i = 0; i < nr_objects; i++) {\n> >  \t\tunpack_one(i);\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 328ab06fd4..b9a784fece 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -1152,10 +1152,10 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,\n> >  \tstruct progress *progress = NULL;\n> >  \tint i = 0;\n> >\n> > -\tif (ctx->report_progress)\n> > -\t\tprogress = start_delayed_progress(\n> > -\t\t\t_(\"Writing changed paths Bloom filters index\"),\n> > -\t\t\tctx->commits.nr);\n> > +\tprogress = start_delayed_progress(\n> > +\t\t_(\"Writing changed paths Bloom filters index\"),\n> > +\t\tctx->commits.nr,\n> > +\t\tctx->report_progress);\n>\n> There are a lot of blocks like this, where the progress string is long enough to\n> require the first param to be after the method name. Since we are changing the\n> API and every caller, would the resulting code be cleaner if the string value\n> was the last parameter? That would allow this code pattern in most cases:\n>\n> \tprogress = start_delayed_progress(count, show_progress,\n> \t\t\t\t\t  _(\"My special string!\"));\n>\n> Just a thought. Not super-important.\n>\n> The rest of the changes look to be correct.\n>\n> > diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\n> > index 5d05cbe789..19b874f9cd 100644\n> > --- a/t/helper/test-progress.c\n> > +++ b/t/helper/test-progress.c\n> > @@ -23,16 +23,18 @@\n> >  int cmd__progress(int argc, const char **argv)\n> >  {\n> >  \tint total = 0;\n> > +\tint quiet = 0;\n> >  \tconst char *title;\n> >  \tstruct strbuf line = STRBUF_INIT;\n> >  \tstruct progress *progress;\n> >\n> >  \tconst char *usage[] = {\n> > -\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n> > +\t\t\"test-tool progress [--total=<n>] [--quiet] <progress-title>\",\n> >  \t\tNULL\n> >  \t};\n> >  \tstruct option options[] = {\n> >  \t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n> > +\t\tOPT_BOOL(0, \"quiet\", &quiet, \"suppress stderr\"),\n> >  \t\tOPT_END(),\n> >  \t};\n> >\n> > @@ -42,7 +44,7 @@ int cmd__progress(int argc, const char **argv)\n> >  \ttitle = argv[0];\n> >\n> >  \tprogress_testing = 1;\n> > -\tprogress = start_progress(title, total);\n> > +\tprogress = start_progress(title, total, !quiet);\n> >  \twhile (strbuf_getline(&line, stdin) != EOF) {\n> >  \t\tchar *end;\n> >\n> > diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> > index 1ed1df351c..9d6e6274ad 100755\n> > --- a/t/t0500-progress-display.sh\n> > +++ b/t/t0500-progress-display.sh\n> > @@ -309,4 +309,31 @@ test_expect_success 'progress generates traces' '\n> >  \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> >  '\n> >\n> > +test_expect_success 'progress generates traces even quietly' '\n> > +\tcat >in <<-\\EOF &&\n> > +\tthroughput 102400 1000\n> > +\tupdate\n> > +\tprogress 10\n> > +\tthroughput 204800 2000\n> > +\tupdate\n> > +\tprogress 20\n> > +\tthroughput 307200 3000\n> > +\tupdate\n> > +\tprogress 30\n> > +\tthroughput 409600 4000\n> > +\tupdate\n> > +\tprogress 40\n> > +\tEOF\n> > +\n> > +\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n> > +\t\t--quiet \"Working hard\" <in 2>stderr &&\n> > +\n> > +\t# t0212/parse_events.perl intentionally omits regions and data.\n> > +\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> > +\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> > +\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n> > +\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> > +'\n>\n> Thanks for adding a test, including the resulting trace events!\n>\n> The patch of Taylor's that I mentioned earlier changes the \"category\"\n> to something a bit more specific than \"progress\", when appropriate.\n>\n> > +\n> > +\n> >  test_done\n>\n> nit: extra empty line\n>\n> Thanks!\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"401290","messageId":"20200710022040.GB39052@syl.lan","threadId":"53829","inReplyTo":"aebbc1ca-9936-ce1b-bacc-855d353f83f9@gmail.com","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-10T02:20:40Z","receivedAt":"2020-07-10T02:20:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 09, 2020 at 10:01:08PM -0400, Derrick Stolee wrote:\n> On 7/9/2020 9:42 PM, Emily Shaffer wrote:\n> > display_progress() and stop_progress() are tolerant to NULL inputs.\n> > Remove some places where redundant checks are performed before these\n> > NULL-tolerant functions are called.\n> >\n> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> > ---\n> >  builtin/commit-graph.c | 3 +--\n> >  commit-graph.c         | 3 +--\n> >  read-cache.c           | 9 +++------\n> >  3 files changed, 5 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> > index ae4dc2dfcd..7964de3126 100644\n> > --- a/builtin/commit-graph.c\n> > +++ b/builtin/commit-graph.c\n> > @@ -263,8 +263,7 @@ static int graph_write(int argc, const char **argv)\n> >  cleanup:\n> >  \tstring_list_clear(&pack_indexes, 0);\n> >  \tstrbuf_release(&buf);\n> > -\tif (progress)\n> > -\t\tstop_progress(&progress);\n> > +\tstop_progress(&progress);\n> >  \treturn result;\n> >  }\n> >\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index b9a784fece..30d9a75932 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -1424,8 +1424,7 @@ int write_commit_graph_reachable(struct object_directory *odb,\n> >  \t\t\t\t    flags, split_opts);\n> >\n> >  \toidset_clear(&commits);\n> > -\tif (data.progress)\n> > -\t\tstop_progress(&data.progress);\n> > +\tstop_progress(&data.progress);\n> >  \treturn result;\n> >  }\n> >\n> > diff --git a/read-cache.c b/read-cache.c\n> > index 2ddc422dbd..feb7abe37a 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -1581,8 +1581,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n> >  \t\tnew_entry = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);\n> >  \t\tif (new_entry == ce)\n> >  \t\t\tcontinue;\n> > -\t\tif (progress)\n> > -\t\t\tdisplay_progress(progress, i);\n> > +\t\tdisplay_progress(progress, i);\n> >  \t\tif (!new_entry) {\n> >  \t\t\tconst char *fmt;\n> >\n> > @@ -1614,10 +1613,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n> >\n> >  \t\treplace_index_entry(istate, i, new_entry);\n> >  \t}\n> > -\tif (progress) {\n> > -\t\tdisplay_progress(progress, istate->cache_nr);\n> > -\t\tstop_progress(&progress);\n> > -\t}\n> > +\tdisplay_progress(progress, istate->cache_nr);\n> > +\tstop_progress(&progress);\n> >  \ttrace_performance_leave(\"refresh index\");\n> >  \treturn has_errors;\n>\n> Looks obviously correct to me.\n\nTo me too, although note that this will generate merge conflicts with\nSzeder's patch from earlier today[1].\n\nUnfortunately, the conflicts are a little deeper than \"we both removed\nan unnecessary 'if' statement\", since Szeder's patch moves the\n'stop_progress()' call earlier to avoid a bug that I introduced.\n\nThis is just something for Junio to look out for while queuing,\notherwise I think just removing the hunk in builtin/commit-graph.c would\nbe fine, too.\n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/xmqq5zawy4x5.fsf@gitster.c.googlers.com/T/#mbd2d5236a7fecf1287be75ffa396f3776fc9b891\n"},{"id":"401363","messageId":"xmqq1rljurvj.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200710022040.GB39052@syl.lan","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-10T18:50:08Z","receivedAt":"2020-07-10T18:50:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> To me too, although note that this will generate merge conflicts with\n> Szeder's patch from earlier today[1].\n>\n> Unfortunately, the conflicts are a little deeper than \"we both removed\n> an unnecessary 'if' statement\", since Szeder's patch moves the\n> 'stop_progress()' call earlier to avoid a bug that I introduced.\n>\n> This is just something for Junio to look out for while queuing,\n\nActually that is something contributors learn to play better with\neach other ;-)\n\nThanks for a heads-up.\n"},{"id":"401368","messageId":"20200710192140.GC3189386@google.com","threadId":"53829","inReplyTo":"20200710021740.GA39052@syl.lan","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T19:21:40Z","receivedAt":"2020-07-10T19:21:49Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jul 09, 2020 at 10:17:40PM -0400, Taylor Blau wrote:\n> \n> Hi both,\n> \n> On Thu, Jul 09, 2020 at 10:00:23PM -0400, Derrick Stolee wrote:\n> > On 7/9/2020 9:42 PM, Emily Shaffer wrote:\n> > > Before now, the progress API is used by conditionally calling\n> > > start_progress() or a similar call, and then unconditionally calling\n> > > display_progress() and stop_progress(), both of which are tolerant of\n> > > NULL or uninitialized inputs. However, in\n> > > 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> > > throughput), the progress library learned to log traces during expensive\n> > > operations. In cases where progress should not be displayed to the user\n> > > - such as when Git is called by a script - no traces will be logged,\n> > > because the progress object is never created.\n> \n> This is such a fantastic idea. Just the other day, I was thinking of\n> getting your (for clarification, Emily, since I'm responding to\n> Stolee's mail) progress-emits-trace2-events work hooked into GitHub's\n> trace2 pipeline.\n> \n> There were two unfortunate things that prevented this from working:\n> \n>   1. GitHub filters which trace2 categories are of interest to us (these\n>      interesting ones get logged, and the uninteresting ones get\n>      discarded) using an environment variable of comma-separated\n>      categories. Since all of the trace2 metrics generated by the\n>      progress API don't have categories, taking in one interesting\n>      metric meant taking them all in, which is a non-starter for us.\n> \n>   2. On top of that, we don't even _generate_ these progress events most\n>      of the time, since we're often running without a tty, and so we\n>      never end up hitting those 'if (progress) start_progress()'\n>      conditionals in the first place.\n> \n> If we had something like this, it would reduce the problem to only (1),\n> which would make a lot of my headaches go away. (It would also give me a\n> good excuse to convert many of our custom trace2 regions into patches on\n> the list, and get rid of a non-trivial amount of code that generates\n> merge conflicts often).\n> \n> > > Instead, to allow us to collect traces from scripted Git commands, teach\n> > > a progress->verbose flag, which is specified via a new argument to\n> > > start_progress() and friends. display_progress() also learns to filter\n> > > for that flag. With these changes, start_progress() can be called\n> > > unconditionally but with a conditional as an argument to determine\n> > > whether to report progress to the user.\n> > >\n> > > Since this changes the API, also modify callers of start_progress() and\n> > > friends to drop their conditional and pass a new argument in instead.\n> \n> I don't think that this is why I was CC'd, but could you perhaps talk a\n> little bit about why this is all in the same patch? I don't think this\n> change needs to be broken out by the area affected per-se, but the\n> current form is a little unruly to review all at once.\n\nI agree that it's a lot, but I didn't think very hard about how to split\nit up for review. The problem is that changing the signature of course\nbreaks the build, and I didn't want a commit which wouldn't build. I\ncould split it into one commit per start_progress() variation, I\nsuppose, but is it really easier to review? With only one or two\nexceptions these changes are all pretty rote.\n\n - Emily\n\n> \n> > This is a worthwhile change. Thanks! I was hoping that we would\n> > get some of these regions for free, which extends what we can get\n> > out of trace2 events.\n> >\n> > CC'ing Taylor because he had some thoughts on adding a possible\n> > trace2 category to make it easier to reason about the regions,\n> > when appropriate. Not sure if he's ready to apply that change\n> > on top of this series.\n> \n> This is what I was talking about above. It would be nice if we could\n> somehow teach the 'start_*_progress()' API about a trace2 category.\n\nYeah, we have been thinking about that too - right now I think we use\nthe title, which is pretty ugly for metrics as it's intended for human\neyes. One suggestion I heard was to teach an enum to the progress struct\nfor the region name (or category); but to get it into the region_enter\nmessage we'd have to take it during start_progress(), as you say.\n\n> \n> Unfortunately, this would need to be a new parameter, since we need to\n> know the category when we enter the region. So, the API changes might be\n> far-reaching. It would be nice if there was a way to limit the blast\n> radius (i.e., 'start_progress_trace2(..., category)'), but I haven't\n> thought deeply about it.\n\nYeah, this seems viable in the scenario you describe - those who aren't\nusing the *_trace2() constructors aren't any worse off than before.\n\n> \n> I don't want to delay this patch series with that. I'd be happy to build\n> it in myself on top after this graduates.\n> \n> > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > > index f520111eda..f64cad8390 100644\n> > > --- a/builtin/rev-list.c\n> > > +++ b/builtin/rev-list.c\n> > > @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> > >  \tif (bisect_list)\n> > >  \t\trevs.limited = 1;\n> > >\n> > > -\tif (show_progress)\n> > > -\t\tprogress = start_delayed_progress(show_progress, 0);\n> > > +\t/*\n> > > +\t * When progress is not printed to the user, we still want to be able to\n> > > +\t * classify the progress during tracing. So, use a placeholder name.\n> > > +\t */\n> > > +\tprogress = start_delayed_progress(\n> > > +\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n> > > +\t\t\t0, show_progress != NULL)\n> >\n> > This is so strange, how we let the command-lines specify a progress\n> > indicator. I guess it is necessary when we use rev-list as a\n> > subcommand instead of in-process. One such case is check_connected()\n> > in connected.c.\n> >\n> > It's stranger still that \"show_progress\" is actually a string here,\n> > as opposed to being an int in most other places.\n> >\n> > Your transformation is correct, here, though. Thanks for calling it\n> > out in the commit message.\n> >\n> > >\n> > >  \tif (use_bitmap_index) {\n> > >  \t\tif (!try_bitmap_count(&revs, &filter_options))\n> > > diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> > > index dd4a75e030..719d446916 100644\n> > > --- a/builtin/unpack-objects.c\n> > > +++ b/builtin/unpack-objects.c\n> > > @@ -498,8 +498,7 @@ static void unpack_all(void)\n> > >  \t\t\tntohl(hdr->hdr_version));\n> > >  \tuse(sizeof(struct pack_header));\n> > >\n> > > -\tif (!quiet)\n> > > -\t\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects);\n> > > +\tprogress = start_progress(_(\"Unpacking objects\"), nr_objects, !quiet);\n> > >  \tobj_list = xcalloc(nr_objects, sizeof(*obj_list));\n> > >  \tfor (i = 0; i < nr_objects; i++) {\n> > >  \t\tunpack_one(i);\n> > > diff --git a/commit-graph.c b/commit-graph.c\n> > > index 328ab06fd4..b9a784fece 100644\n> > > --- a/commit-graph.c\n> > > +++ b/commit-graph.c\n> > > @@ -1152,10 +1152,10 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,\n> > >  \tstruct progress *progress = NULL;\n> > >  \tint i = 0;\n> > >\n> > > -\tif (ctx->report_progress)\n> > > -\t\tprogress = start_delayed_progress(\n> > > -\t\t\t_(\"Writing changed paths Bloom filters index\"),\n> > > -\t\t\tctx->commits.nr);\n> > > +\tprogress = start_delayed_progress(\n> > > +\t\t_(\"Writing changed paths Bloom filters index\"),\n> > > +\t\tctx->commits.nr,\n> > > +\t\tctx->report_progress);\n> >\n> > There are a lot of blocks like this, where the progress string is long enough to\n> > require the first param to be after the method name. Since we are changing the\n> > API and every caller, would the resulting code be cleaner if the string value\n> > was the last parameter? That would allow this code pattern in most cases:\n> >\n> > \tprogress = start_delayed_progress(count, show_progress,\n> > \t\t\t\t\t  _(\"My special string!\"));\n> >\n> > Just a thought. Not super-important.\n> >\n> > The rest of the changes look to be correct.\n> >\n> > > diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\n> > > index 5d05cbe789..19b874f9cd 100644\n> > > --- a/t/helper/test-progress.c\n> > > +++ b/t/helper/test-progress.c\n> > > @@ -23,16 +23,18 @@\n> > >  int cmd__progress(int argc, const char **argv)\n> > >  {\n> > >  \tint total = 0;\n> > > +\tint quiet = 0;\n> > >  \tconst char *title;\n> > >  \tstruct strbuf line = STRBUF_INIT;\n> > >  \tstruct progress *progress;\n> > >\n> > >  \tconst char *usage[] = {\n> > > -\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n> > > +\t\t\"test-tool progress [--total=<n>] [--quiet] <progress-title>\",\n> > >  \t\tNULL\n> > >  \t};\n> > >  \tstruct option options[] = {\n> > >  \t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n> > > +\t\tOPT_BOOL(0, \"quiet\", &quiet, \"suppress stderr\"),\n> > >  \t\tOPT_END(),\n> > >  \t};\n> > >\n> > > @@ -42,7 +44,7 @@ int cmd__progress(int argc, const char **argv)\n> > >  \ttitle = argv[0];\n> > >\n> > >  \tprogress_testing = 1;\n> > > -\tprogress = start_progress(title, total);\n> > > +\tprogress = start_progress(title, total, !quiet);\n> > >  \twhile (strbuf_getline(&line, stdin) != EOF) {\n> > >  \t\tchar *end;\n> > >\n> > > diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> > > index 1ed1df351c..9d6e6274ad 100755\n> > > --- a/t/t0500-progress-display.sh\n> > > +++ b/t/t0500-progress-display.sh\n> > > @@ -309,4 +309,31 @@ test_expect_success 'progress generates traces' '\n> > >  \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> > >  '\n> > >\n> > > +test_expect_success 'progress generates traces even quietly' '\n> > > +\tcat >in <<-\\EOF &&\n> > > +\tthroughput 102400 1000\n> > > +\tupdate\n> > > +\tprogress 10\n> > > +\tthroughput 204800 2000\n> > > +\tupdate\n> > > +\tprogress 20\n> > > +\tthroughput 307200 3000\n> > > +\tupdate\n> > > +\tprogress 30\n> > > +\tthroughput 409600 4000\n> > > +\tupdate\n> > > +\tprogress 40\n> > > +\tEOF\n> > > +\n> > > +\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n> > > +\t\t--quiet \"Working hard\" <in 2>stderr &&\n> > > +\n> > > +\t# t0212/parse_events.perl intentionally omits regions and data.\n> > > +\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> > > +\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> > > +\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n> > > +\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> > > +'\n> >\n> > Thanks for adding a test, including the resulting trace events!\n> >\n> > The patch of Taylor's that I mentioned earlier changes the \"category\"\n> > to something a bit more specific than \"progress\", when appropriate.\n> >\n> > > +\n> > > +\n> > >  test_done\n> >\n> > nit: extra empty line\n> >\n> > Thanks!\n> > -Stolee\n> \n> Thanks,\n> Taylor\n"},{"id":"401366","messageId":"20200710192451.GD3189386@google.com","threadId":"53829","inReplyTo":"20200710021451.GI9782@camp.crustytoothpaste.net","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T19:24:51Z","receivedAt":"2020-07-10T19:24:58Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jul 10, 2020 at 02:14:51AM +0000, brian m. carlson wrote:\n> \n> On 2020-07-10 at 01:42:41, Emily Shaffer wrote:\n> > Before now, the progress API is used by conditionally calling\n> > start_progress() or a similar call, and then unconditionally calling\n> > display_progress() and stop_progress(), both of which are tolerant of\n> > NULL or uninitialized inputs. However, in\n> > 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> > throughput), the progress library learned to log traces during expensive\n> > operations. In cases where progress should not be displayed to the user\n> > - such as when Git is called by a script - no traces will be logged,\n> > because the progress object is never created.\n> > \n> > Instead, to allow us to collect traces from scripted Git commands, teach\n> > a progress->verbose flag, which is specified via a new argument to\n> > start_progress() and friends. display_progress() also learns to filter\n> > for that flag. With these changes, start_progress() can be called\n> > unconditionally but with a conditional as an argument to determine\n> > whether to report progress to the user.\n> \n> So to make sure I understand this right, we'll collect traces regardless\n> if it's enabled, but we'll still honor the --quiet flag if the user\n> doesn't want to see them?\n\nYes, precisely. display_progress() used to check whether to display or\nnot based on whether the 'struct progress' was valid; now it also checks\nwhether 'progress->verbose' is set. display() (where the real logic\nlives for printing to the user) still never gets called with this\nchange.\n\n>\n> If so, I'm definitely in favor of this\n> change.  I was worried when I read the cover letter that we'd display\n> them to the user regardless, but from reading the patch and the commit\n> message, it seems I misunderstood.\n> \n> I think the making the verbose flag a parameter simplifies the code\n> nicely and puts the rendering decision in the right place.\n\nThanks!\n\n - Emily\n"},{"id":"401369","messageId":"20200710192731.GE3189386@google.com","threadId":"53829","inReplyTo":"xmqq1rljurvj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T19:27:31Z","receivedAt":"2020-07-10T19:27:41Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jul 10, 2020 at 11:50:08AM -0700, Junio C Hamano wrote:\n> \n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> > To me too, although note that this will generate merge conflicts with\n> > Szeder's patch from earlier today[1].\n> >\n> > Unfortunately, the conflicts are a little deeper than \"we both removed\n> > an unnecessary 'if' statement\", since Szeder's patch moves the\n> > 'stop_progress()' call earlier to avoid a bug that I introduced.\n> >\n> > This is just something for Junio to look out for while queuing,\n> \n> Actually that is something contributors learn to play better with\n> each other ;-)\n\nYeah, I was worried about the nature of this series in general. Sorry to\nhave missed Szeder's change.\n\n> \n> Thanks for a heads-up.\n\nWill it make your life easier if I base this series on the other topic?\nOr would you rather I leave it to you?\n\n - Emily\n"},{"id":"401372","messageId":"xmqqpn93ta4h.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200710192731.GE3189386@google.com","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-10T19:58:54Z","receivedAt":"2020-07-10T19:58:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> Will it make your life easier if I base this series on the other topic?\n> Or would you rather I leave it to you?\n\nEither is fine.  \n\nJust don't expect any quick turnaround time for a new and involved\nchange like this while we are entering the pre-release freeze.  We\nare supposed to be stablizing, not churning ;-).\n"},{"id":"401379","messageId":"20200710202955.GF3189386@google.com","threadId":"53829","inReplyTo":"xmqqpn93ta4h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T20:29:55Z","receivedAt":"2020-07-10T20:30:08Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jul 10, 2020 at 12:58:54PM -0700, Junio C Hamano wrote:\n> \n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > Will it make your life easier if I base this series on the other topic?\n> > Or would you rather I leave it to you?\n> \n> Either is fine.  \n> \n> Just don't expect any quick turnaround time for a new and involved\n> change like this while we are entering the pre-release freeze.  We\n> are supposed to be stablizing, not churning ;-).\n\nUnderstood.\n\nIt's got a nit or two from Stolee's review so I need to reroll it\nanyway; I'll send it based on the other topic.\n\n - Emily\n"},{"id":"401383","messageId":"xmqqpn93rsb3.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200710021451.GI9782@camp.crustytoothpaste.net","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-10T21:09:04Z","receivedAt":"2020-07-10T21:09:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> So to make sure I understand this right, we'll collect traces regardless\n> if it's enabled, but we'll still honor the --quiet flag if the user\n> doesn't want to see them?  If so, I'm definitely in favor of this\n> change.\n\nHmph, if we know we won't be emitting, why spend cycles to collect\ntraces in the first place?  Does it make the code simpler to follow\nor something?  As long as it is done when the user requests a trace\nto be taken, I do not care about the wasted cycles too much, I think ;-)\n\n> I was worried when I read the cover letter that we'd display\n> them to the user regardless, but from reading the patch and the commit\n> message, it seems I misunderstood.\n>\n> I think the making the verbose flag a parameter simplifies the code\n> nicely and puts the rendering decision in the right place.\n\nOK.\n"},{"id":"401387","messageId":"20200710220003.GG3189386@google.com","threadId":"53829","inReplyTo":"xmqqpn93rsb3.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T22:00:03Z","receivedAt":"2020-07-10T22:00:11Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jul 10, 2020 at 02:09:04PM -0700, Junio C Hamano wrote:\n> \n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > So to make sure I understand this right, we'll collect traces regardless\n> > if it's enabled, but we'll still honor the --quiet flag if the user\n> > doesn't want to see them?  If so, I'm definitely in favor of this\n> > change.\n> \n> Hmph, if we know we won't be emitting, why spend cycles to collect\n> traces in the first place?  Does it make the code simpler to follow\n> or something?  As long as it is done when the user requests a trace\n> to be taken, I do not care about the wasted cycles too much, I think ;-)\n\nMaybe then I did misunderstand brian.\n\nWith this series, traces will always be emitted to location where\nGIT_TRACE2_EVENT indicates, or default location. Like before, if the\nuser asks for --quiet or equivalent, the user will not see \"Resolving\ndeltas... (4/30)\" in their stderr.\n\nBefore this series, when a user indicated --quiet or equivalent, the\ntraces were not emitted to GIT_TRACE2_EVENT's location, because the\nprogress struct was not created.\n\nI figured brian was saying \"we'll still honor the --quiet flag if the\nuser doesn't want to see [\"Resolving deltas...blah blah\" in their\nstderr]\", and that is true. The traces will be logged but the\nuser-facing output on stderr will be suppressed.\n\n> \n> > I was worried when I read the cover letter that we'd display\n> > them to the user regardless, but from reading the patch and the commit\n> > message, it seems I misunderstood.\n> >\n> > I think the making the verbose flag a parameter simplifies the code\n> > nicely and puts the rendering decision in the right place.\n> \n> OK.\n"},{"id":"401389","messageId":"xmqq365zro2u.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200710014242.1088216-2-emilyshaffer@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-10T22:40:25Z","receivedAt":"2020-07-10T22:40:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> Before now, the progress API is used by conditionally calling\n> start_progress() or a similar call, and then unconditionally calling\n> display_progress() and stop_progress(), both of which are tolerant of\n> NULL or uninitialized inputs. However, in\n> 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> throughput), the progress library learned to log traces during expensive\n> operations. In cases where progress should not be displayed to the user\n> - such as when Git is called by a script - no traces will be logged,\n> because the progress object is never created.\n>\n> Instead, to allow us to collect traces from scripted Git commands, teach\n> a progress->verbose flag, which is specified via a new argument to\n> start_progress() and friends. display_progress() also learns to filter\n> for that flag. With these changes, start_progress() can be called\n> unconditionally but with a conditional as an argument to determine\n> whether to report progress to the user.\n>\n> Since this changes the API, also modify callers of start_progress() and\n> friends to drop their conditional and pass a new argument in instead.\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  builtin/blame.c             |   4 +-\n>  builtin/commit-graph.c      |   5 +-\n>  builtin/fsck.c              |  25 ++++---\n>  builtin/index-pack.c        |  16 ++---\n>  builtin/log.c               |   4 +-\n>  builtin/pack-objects.c      |  18 +++--\n>  builtin/prune.c             |   4 +-\n>  builtin/rev-list.c          |   9 ++-\n>  builtin/unpack-objects.c    |   3 +-\n>  commit-graph.c              | 140 +++++++++++++++++-------------------\n>  delta-islands.c             |   4 +-\n>  diffcore-rename.c           |   9 ++-\n>  entry.c                     |   2 +-\n>  midx.c                      |  43 +++++------\n>  pack-bitmap-write.c         |   8 +--\n>  pack-bitmap.c               |   5 +-\n>  preload-index.c             |   7 +-\n>  progress.c                  |  27 ++++---\n>  progress.h                  |  10 +--\n>  prune-packed.c              |   4 +-\n>  read-cache.c                |   6 +-\n>  t/helper/test-progress.c    |   6 +-\n>  t/t0500-progress-display.sh |  27 +++++++\n>  unpack-trees.c              |  14 ++--\n>  walker.c                    |   4 +-\n>  25 files changed, 212 insertions(+), 192 deletions(-)\n\nSo, the gist of the change is these, where all functions in the\nprogress API that prepare a progress structure now take \"verbose\"\nparameter, and the function that emits the output looks at the\nflag.  Counters and timers are used no matter what as long as\nprogress is shown.\n\n> diff --git a/progress.c b/progress.c\n> index 3eda914518..f014908b05 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -43,6 +43,7 @@ struct progress {\n>  \tstruct strbuf counters_sb;\n>  \tint title_len;\n>  \tint split;\n> +\tint verbose;\n>  };\n>  \n>  static volatile sig_atomic_t progress_update;\n> @@ -245,12 +246,13 @@ void display_throughput(struct progress *progress, uint64_t total)\n>  \n>  void display_progress(struct progress *progress, uint64_t n)\n>  {\n> -\tif (progress)\n> +\tif (progress && progress->verbose)\n>  \t\tdisplay(progress, n, NULL);\n>  }\n>  \n>  static struct progress *start_progress_delay(const char *title, uint64_t total,\n> -\t\t\t\t\t     unsigned delay, unsigned sparse)\n> +\t\t\t\t\t     unsigned delay, unsigned sparse,\n> +\t\t\t\t\t     int verbose)\n>  {\n>  \tstruct progress *progress = xmalloc(sizeof(*progress));\n>  \tprogress->title = title;\n> @@ -264,6 +266,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n>  \tstrbuf_init(&progress->counters_sb, 0);\n>  \tprogress->title_len = utf8_strwidth(title);\n>  \tprogress->split = 0;\n> +\tprogress->verbose = verbose;\n>  \tset_progress_signal();\n>  \ttrace2_region_enter(\"progress\", title, the_repository);\n>  \treturn progress;\n> @@ -279,14 +282,16 @@ static int get_default_delay(void)\n>  \treturn delay_in_secs;\n>  }\n>  \n> -struct progress *start_delayed_progress(const char *title, uint64_t total)\n> +struct progress *start_delayed_progress(const char *title, uint64_t total,\n> +\t\t\t\t\tint verbose)\n>  {\n> -\treturn start_progress_delay(title, total, get_default_delay(), 0);\n> +\treturn start_progress_delay(title, total, get_default_delay(), 0,\n> +\t\t\t\t    verbose);\n>  }\n>  \n> -struct progress *start_progress(const char *title, uint64_t total)\n> +struct progress *start_progress(const char *title, uint64_t total, int verbose)\n>  {\n> -\treturn start_progress_delay(title, total, 0, 0);\n> +\treturn start_progress_delay(title, total, 0, 0, verbose);\n>  }\n>  \n>  /*\n> @@ -298,15 +303,17 @@ struct progress *start_progress(const char *title, uint64_t total)\n>   * When \"sparse\" is set, stop_progress() will automatically force the done\n>   * message to show 100%.\n>   */\n> -struct progress *start_sparse_progress(const char *title, uint64_t total)\n> +struct progress *start_sparse_progress(const char *title, uint64_t total,\n> +\t\t\t\t       int verbose)\n>  {\n> -\treturn start_progress_delay(title, total, 0, 1);\n> +\treturn start_progress_delay(title, total, 0, 1, verbose);\n>  }\n>  \n>  struct progress *start_delayed_sparse_progress(const char *title,\n> -\t\t\t\t\t       uint64_t total)\n> +\t\t\t\t\t       uint64_t total, int verbose)\n>  {\n> -\treturn start_progress_delay(title, total, get_default_delay(), 1);\n> +\treturn start_progress_delay(title, total, get_default_delay(), 1,\n> +\t\t\t\t    verbose);\n>  }\n>  \n>  static void finish_if_sparse(struct progress *progress)\n> diff --git a/progress.h b/progress.h\n> index f1913acf73..0c64557231 100644\n> --- a/progress.h\n> +++ b/progress.h\n> @@ -13,11 +13,13 @@ void progress_test_force_update(void);\n>  \n>  void display_throughput(struct progress *progress, uint64_t total);\n>  void display_progress(struct progress *progress, uint64_t n);\n> -struct progress *start_progress(const char *title, uint64_t total);\n> -struct progress *start_sparse_progress(const char *title, uint64_t total);\n> -struct progress *start_delayed_progress(const char *title, uint64_t total);\n> +struct progress *start_progress(const char *title, uint64_t total, int verbose);\n> +struct progress *start_sparse_progress(const char *title, uint64_t total,\n> +\t\t\t\t       int verbose);\n> +struct progress *start_delayed_progress(const char *title, uint64_t total,\n> +\t\t\t\t\tint verbose);\n>  struct progress *start_delayed_sparse_progress(const char *title,\n> -\t\t\t\t\t       uint64_t total);\n> +\t\t\t\t\t       uint64_t total, int verbose);\n>  void stop_progress(struct progress **progress);\n>  void stop_progress_msg(struct progress **progress, const char *msg);\n>  \n\n\nAnd then this is how a typical caller gets updated.\n\nFor this particular one, it is not _too_ bad, even though it does\nfeel a bit roundabout way to say \"we are starting delayed progress\nat this point\" by having this call unconditionally, but then the\nreader needs to realize \"ah, not really, because show_progress is\nnot enabled\" when the reading reaches the end of the call.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 94ef57c1cc..1d72b27fda 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1129,8 +1129,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>  \n>  \tsb.found_guilty_entry = &found_guilty_entry;\n>  \tsb.found_guilty_entry_data = &pi;\n> -\tif (show_progress)\n> -\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n> +\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines,\n> +\t\t\t\t\t     show_progress);\n>  \n>  \tassign_blame(&sb, opt);\n\nBut there are others that look a bit problematic.  In this example\ntaken from fsck, we open all the pack index, only because it is\nneeded to show the progress, and the existing conditionals are ways\nto avoid spending unneeded cycles.\n\n> @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n>  \t\t\tuint32_t total = 0, count = 0;\n>  \t\t\tstruct progress *progress = NULL;\n>  \n> -\t\t\tif (show_progress) {\n> -\t\t\t\tfor (p = get_all_packs(the_repository); p;\n> -\t\t\t\t     p = p->next) {\n> -\t\t\t\t\tif (open_pack_index(p))\n> -\t\t\t\t\t\tcontinue;\n> -\t\t\t\t\ttotal += p->num_objects;\n> -\t\t\t\t}\n> -\n> -\t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n> +\t\t\tfor (p = get_all_packs(the_repository); p;\n> +\t\t\t     p = p->next) {\n> +\t\t\t\tif (open_pack_index(p))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\ttotal += p->num_objects;\n>  \t\t\t}\n> +\n> +\t\t\tprogress = start_progress(_(\"Checking objects\"), total,\n> +\t\t\t\t\t\t  show_progress);\n>  \t\t\tfor (p = get_all_packs(the_repository); p;\n>  \t\t\t     p = p->next) {\n>  \t\t\t\t/* verify gives error messages itself */\n\nLikewise, we do not even have to be scanning the index entries\nupfront if we are not showing the progress report (and more\nimportantly, the user likely has chosen the speed/cycles over eye\ncandy progress meter) while checking out paths from the index.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 4be5fc3075..fa84244466 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -338,16 +338,14 @@ static struct progress *get_progress(struct unpack_trees_options *o,\n>  {\n>  \tunsigned cnt = 0, total = 0;\n>  \n> -\tif (!o->update || !o->verbose_update)\n> -\t\treturn NULL;\n> -\n>  \tfor (; cnt < index->cache_nr; cnt++) {\n>  \t\tconst struct cache_entry *ce = index->cache[cnt];\n>  \t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n>  \t\t\ttotal++;\n>  \t}\n>  \n> -\treturn start_delayed_progress(_(\"Updating files\"), total);\n> +\treturn start_delayed_progress(_(\"Updating files\"), total,\n> +\t\t\t\t      (o->update && o->verbose_update));\n>  }\n>  \n>  static void setup_collided_checkout_detection(struct checkout *state,\n\n\nAnd there are cases where the \"is progress enabled?\" parameter (by\nthe way, I am not sure 'verbose' is quite the right name for the\nparameter and the field name, as that word implies there can be\nmultiple verbosity levels---what I see you've done is that you added\nan \"is this enabled?\" flag) is not a simple variable, like these.\nSome are simple enough, but some are not.  Some of the worst appear\nin preload-index.c and read-cache.c---they may become easier to read\nif we introduced a variable to be passed as the \"enable?\"  parameter\nand compute it before makng a call, and that might be enough, though.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 7016b28485..1a97a9f0be 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1012,8 +1012,8 @@ static void write_pack_file(void)\n>  \ttime_t last_mtime = 0;\n>  \tstruct object_entry **write_order;\n>  \n> -\tif (progress > pack_to_stdout)\n> -\t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n> +\tprogress_state = start_progress(_(\"Writing objects\"), nr_result,\n> +\t\t\t\t\t(progress > pack_to_stdout));\n>  \tALLOC_ARRAY(written_list, to_pack.nr_objects);\n>  \twrite_order = compute_write_order();\n>  \n\n> @@ -1418,9 +1415,9 @@ int write_commit_graph_reachable(struct object_directory *odb,\n>  \n>  \tmemset(&data, 0, sizeof(data));\n>  \tdata.commits = &commits;\n> -\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n> -\t\tdata.progress = start_delayed_progress(\n> -\t\t\t_(\"Collecting referenced commits\"), 0);\n> +\tdata.progress = start_delayed_progress(\n> +\t\t_(\"Collecting referenced commits\"), 0,\n> +\t\t(flags & COMMIT_GRAPH_WRITE_PROGRESS));\n>  \n>  \tfor_each_ref(add_ref_to_set, &data);\n>  \tresult = write_commit_graph(odb, NULL, &commits,\n\n> @@ -2301,9 +2295,9 @@ 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> -\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> +\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> +\t\t\t\tg->num_commits,\n> +\t\t\t\t(flags & COMMIT_GRAPH_WRITE_PROGRESS));\n>  \n>  \tfor (i = 0; i < g->num_commits; i++) {\n>  \t\tstruct commit *graph_commit, *odb_commit;\n\n> diff --git a/midx.c b/midx.c\n> index 6d1584ca51..d9ab163340 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -836,10 +836,8 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n>  \t}\n>  \n>  \tpacks.pack_paths_checked = 0;\n> -\tif (flags & MIDX_PROGRESS)\n> -\t\tpacks.progress = start_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n> -\telse\n> -\t\tpacks.progress = NULL;\n> +\tpacks.progress = start_progress(_(\"Adding packfiles to multi-pack-index\"), 0,\n> +\t\t\t\t\t\t(flags & MIDX_PROGRESS));\n>  \n>  \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &packs);\n>  \tstop_progress(&packs.progress);\n\n> diff --git a/preload-index.c b/preload-index.c\n> index ed6eaa4738..a924a84a14 100644\n> --- a/preload-index.c\n> +++ b/preload-index.c\n> @@ -115,10 +115,9 @@ void preload_index(struct index_state *index,\n>  \tmemset(&data, 0, sizeof(data));\n>  \n>  \tmemset(&pd, 0, sizeof(pd));\n> -\tif (refresh_flags & REFRESH_PROGRESS && isatty(2)) {\n> -\t\tpd.progress = start_delayed_progress(_(\"Refreshing index\"), index->cache_nr);\n> -\t\tpthread_mutex_init(&pd.mutex, NULL);\n> -\t}\n> +\tpd.progress = start_delayed_progress(_(\"Refreshing index\"), index->cache_nr,\n> +\t\t\t\t\t     (refresh_flags & REFRESH_PROGRESS && isatty(2)));\n> +\tpthread_mutex_init(&pd.mutex, NULL);\n>  \n>  \tfor (i = 0; i < threads; i++) {\n>  \t\tstruct thread_data *p = data+i;\n\n> diff --git a/read-cache.c b/read-cache.c\n> index aa427c5c17..2ddc422dbd 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1532,9 +1532,9 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>  \tconst char *unmerged_fmt;\n>  \tstruct progress *progress = NULL;\n>  \n> -\tif (flags & REFRESH_PROGRESS && isatty(2))\n> -\t\tprogress = start_delayed_progress(_(\"Refresh index\"),\n> -\t\t\t\t\t\t  istate->cache_nr);\n> +\tprogress = start_delayed_progress(_(\"Refresh index\"),\n> +\t\t\t\t\t  istate->cache_nr,\n> +\t\t\t\t\t  (flags & REFRESH_PROGRESS && isatty(2)));\n>  \n>  \ttrace_performance_enter();\n>  \tmodified_fmt   = in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\";\n\n\nAnd here is the only one that may benefit from this change, which\ncares that the codepath prepared in rev-list, primarily to support\nthe progress meter, is exercised because those pieces of code\nhappens to be used by the tracing output as well.  Perhaps we\nshouldn't have tied the \"when showing progress, we can also trace\"\nearlier and we didn't have to touch this many codepath only to spend\ncycles when progress output is not desired?\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index f520111eda..f64cad8390 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n>  \tif (bisect_list)\n>  \t\trevs.limited = 1;\n>  \n> -\tif (show_progress)\n> -\t\tprogress = start_delayed_progress(show_progress, 0);\n> +\t/*\n> +\t * When progress is not printed to the user, we still want to be able to\n> +\t * classify the progress during tracing. So, use a placeholder name.\n> +\t */\n> +\tprogress = start_delayed_progress(\n> +\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n> +\t\t\t0, show_progress != NULL);\n\nI've cut out the hunks that exhibits similar looking patterns as the\nones I covered in the above discussion.  You are right to say that\nthese are simple and similar changes all over the place.  I am not\nyet convinced that the simplicity supports that this change is the\nright thing to do, though.  The only saving grace might be that\nthere aren't that many hunks that explicitly do unnecessary things\nlike the ones in unpack-trees.c and builtin/fsck.c even when we are\nnot showing progress.  \n\nBut the other codepaths may be doing conditional computation not\nbased on \"if (show_progress)\" but on \"if (progress)\", in which case\nwith this patch, we may be paying a lot more overhead even if we\nknow progress meter won't be shown and the worse part from\nreviewability point of view is that this patch does not explicitly\ndo anything to make it happen because start_delayed_progress() now\nunconditionally give a non-NULL progress structure to enable them.\n\nSo I dunno.  It certainly is not for the upcoming release, but we do\nwant to make sure that trace requested even when the command is quiet\ngets collected in the longer term (i.e. during the next cycle).\n\nThanks.\n\n"},{"id":"401391","messageId":"20200710230334.GH3189386@google.com","threadId":"53829","inReplyTo":"20200710202955.GF3189386@google.com","subject":"Re: [PATCH 2/2] progress: remove redundant null-checking","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-10T23:03:34Z","receivedAt":"2020-07-10T23:03:41Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jul 10, 2020 at 01:29:55PM -0700, Emily Shaffer wrote:\n> \n> On Fri, Jul 10, 2020 at 12:58:54PM -0700, Junio C Hamano wrote:\n> > \n> > Emily Shaffer <emilyshaffer@google.com> writes:\n> > \n> > > Will it make your life easier if I base this series on the other topic?\n> > > Or would you rather I leave it to you?\n> > \n> > Either is fine.  \n> > \n> > Just don't expect any quick turnaround time for a new and involved\n> > change like this while we are entering the pre-release freeze.  We\n> > are supposed to be stablizing, not churning ;-).\n> \n> Understood.\n> \n> It's got a nit or two from Stolee's review so I need to reroll it\n> anyway; I'll send it based on the other topic.\n\nHum. It seems that, as of `git fetch github.com/gitster/git` in the past\nhour, sg/commit-graph-progress-fix doesn't include\nes/trace-log-progress, so I don't think it is actually a good idea for\nme to submit this series based on sg/commit-graph-progress-fix instead.\nI'll send the next version based on master, after all.\n\n> \n>  - Emily\n"},{"id":"401522","messageId":"20200714001504.GI3189386@google.com","threadId":"53829","inReplyTo":"xmqq365zro2u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-07-14T00:15:04Z","receivedAt":"2020-07-14T00:15:13Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jul 10, 2020 at 03:40:25PM -0700, Junio C Hamano wrote:\n> \n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > Before now, the progress API is used by conditionally calling\n> > start_progress() or a similar call, and then unconditionally calling\n> > display_progress() and stop_progress(), both of which are tolerant of\n> > NULL or uninitialized inputs. However, in\n> > 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and\n> > throughput), the progress library learned to log traces during expensive\n> > operations. In cases where progress should not be displayed to the user\n> > - such as when Git is called by a script - no traces will be logged,\n> > because the progress object is never created.\n> >\n> > Instead, to allow us to collect traces from scripted Git commands, teach\n> > a progress->verbose flag, which is specified via a new argument to\n> > start_progress() and friends. display_progress() also learns to filter\n> > for that flag. With these changes, start_progress() can be called\n> > unconditionally but with a conditional as an argument to determine\n> > whether to report progress to the user.\n> >\n> > Since this changes the API, also modify callers of start_progress() and\n> > friends to drop their conditional and pass a new argument in instead.\n> >\n> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> > ---\n> >  builtin/blame.c             |   4 +-\n> >  builtin/commit-graph.c      |   5 +-\n> >  builtin/fsck.c              |  25 ++++---\n> >  builtin/index-pack.c        |  16 ++---\n> >  builtin/log.c               |   4 +-\n> >  builtin/pack-objects.c      |  18 +++--\n> >  builtin/prune.c             |   4 +-\n> >  builtin/rev-list.c          |   9 ++-\n> >  builtin/unpack-objects.c    |   3 +-\n> >  commit-graph.c              | 140 +++++++++++++++++-------------------\n> >  delta-islands.c             |   4 +-\n> >  diffcore-rename.c           |   9 ++-\n> >  entry.c                     |   2 +-\n> >  midx.c                      |  43 +++++------\n> >  pack-bitmap-write.c         |   8 +--\n> >  pack-bitmap.c               |   5 +-\n> >  preload-index.c             |   7 +-\n> >  progress.c                  |  27 ++++---\n> >  progress.h                  |  10 +--\n> >  prune-packed.c              |   4 +-\n> >  read-cache.c                |   6 +-\n> >  t/helper/test-progress.c    |   6 +-\n> >  t/t0500-progress-display.sh |  27 +++++++\n> >  unpack-trees.c              |  14 ++--\n> >  walker.c                    |   4 +-\n> >  25 files changed, 212 insertions(+), 192 deletions(-)\n> \n> So, the gist of the change is these, where all functions in the\n> progress API that prepare a progress structure now take \"verbose\"\n> parameter, and the function that emits the output looks at the\n> flag.  Counters and timers are used no matter what as long as\n> progress is shown.\n> \n> > diff --git a/progress.c b/progress.c\n> > index 3eda914518..f014908b05 100644\n> > --- a/progress.c\n> > +++ b/progress.c\n> > @@ -43,6 +43,7 @@ struct progress {\n> >  \tstruct strbuf counters_sb;\n> >  \tint title_len;\n> >  \tint split;\n> > +\tint verbose;\n> >  };\n> >  \n> >  static volatile sig_atomic_t progress_update;\n> > @@ -245,12 +246,13 @@ void display_throughput(struct progress *progress, uint64_t total)\n> >  \n> >  void display_progress(struct progress *progress, uint64_t n)\n> >  {\n> > -\tif (progress)\n> > +\tif (progress && progress->verbose)\n> >  \t\tdisplay(progress, n, NULL);\n> >  }\n> >  \n> >  static struct progress *start_progress_delay(const char *title, uint64_t total,\n> > -\t\t\t\t\t     unsigned delay, unsigned sparse)\n> > +\t\t\t\t\t     unsigned delay, unsigned sparse,\n> > +\t\t\t\t\t     int verbose)\n> >  {\n> >  \tstruct progress *progress = xmalloc(sizeof(*progress));\n> >  \tprogress->title = title;\n> > @@ -264,6 +266,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n> >  \tstrbuf_init(&progress->counters_sb, 0);\n> >  \tprogress->title_len = utf8_strwidth(title);\n> >  \tprogress->split = 0;\n> > +\tprogress->verbose = verbose;\n> >  \tset_progress_signal();\n> >  \ttrace2_region_enter(\"progress\", title, the_repository);\n> >  \treturn progress;\n> > @@ -279,14 +282,16 @@ static int get_default_delay(void)\n> >  \treturn delay_in_secs;\n> >  }\n> >  \n> > -struct progress *start_delayed_progress(const char *title, uint64_t total)\n> > +struct progress *start_delayed_progress(const char *title, uint64_t total,\n> > +\t\t\t\t\tint verbose)\n> >  {\n> > -\treturn start_progress_delay(title, total, get_default_delay(), 0);\n> > +\treturn start_progress_delay(title, total, get_default_delay(), 0,\n> > +\t\t\t\t    verbose);\n> >  }\n> >  \n> > -struct progress *start_progress(const char *title, uint64_t total)\n> > +struct progress *start_progress(const char *title, uint64_t total, int verbose)\n> >  {\n> > -\treturn start_progress_delay(title, total, 0, 0);\n> > +\treturn start_progress_delay(title, total, 0, 0, verbose);\n> >  }\n> >  \n> >  /*\n> > @@ -298,15 +303,17 @@ struct progress *start_progress(const char *title, uint64_t total)\n> >   * When \"sparse\" is set, stop_progress() will automatically force the done\n> >   * message to show 100%.\n> >   */\n> > -struct progress *start_sparse_progress(const char *title, uint64_t total)\n> > +struct progress *start_sparse_progress(const char *title, uint64_t total,\n> > +\t\t\t\t       int verbose)\n> >  {\n> > -\treturn start_progress_delay(title, total, 0, 1);\n> > +\treturn start_progress_delay(title, total, 0, 1, verbose);\n> >  }\n> >  \n> >  struct progress *start_delayed_sparse_progress(const char *title,\n> > -\t\t\t\t\t       uint64_t total)\n> > +\t\t\t\t\t       uint64_t total, int verbose)\n> >  {\n> > -\treturn start_progress_delay(title, total, get_default_delay(), 1);\n> > +\treturn start_progress_delay(title, total, get_default_delay(), 1,\n> > +\t\t\t\t    verbose);\n> >  }\n> >  \n> >  static void finish_if_sparse(struct progress *progress)\n> > diff --git a/progress.h b/progress.h\n> > index f1913acf73..0c64557231 100644\n> > --- a/progress.h\n> > +++ b/progress.h\n> > @@ -13,11 +13,13 @@ void progress_test_force_update(void);\n> >  \n> >  void display_throughput(struct progress *progress, uint64_t total);\n> >  void display_progress(struct progress *progress, uint64_t n);\n> > -struct progress *start_progress(const char *title, uint64_t total);\n> > -struct progress *start_sparse_progress(const char *title, uint64_t total);\n> > -struct progress *start_delayed_progress(const char *title, uint64_t total);\n> > +struct progress *start_progress(const char *title, uint64_t total, int verbose);\n> > +struct progress *start_sparse_progress(const char *title, uint64_t total,\n> > +\t\t\t\t       int verbose);\n> > +struct progress *start_delayed_progress(const char *title, uint64_t total,\n> > +\t\t\t\t\tint verbose);\n> >  struct progress *start_delayed_sparse_progress(const char *title,\n> > -\t\t\t\t\t       uint64_t total);\n> > +\t\t\t\t\t       uint64_t total, int verbose);\n> >  void stop_progress(struct progress **progress);\n> >  void stop_progress_msg(struct progress **progress, const char *msg);\n> >  \n> \n> \n> And then this is how a typical caller gets updated.\n> \n> For this particular one, it is not _too_ bad, even though it does\n> feel a bit roundabout way to say \"we are starting delayed progress\n> at this point\" by having this call unconditionally, but then the\n> reader needs to realize \"ah, not really, because show_progress is\n> not enabled\" when the reading reaches the end of the call.\n> \n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > index 94ef57c1cc..1d72b27fda 100644\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -1129,8 +1129,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> >  \n> >  \tsb.found_guilty_entry = &found_guilty_entry;\n> >  \tsb.found_guilty_entry_data = &pi;\n> > -\tif (show_progress)\n> > -\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n> > +\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines,\n> > +\t\t\t\t\t     show_progress);\n> >  \n> >  \tassign_blame(&sb, opt);\n> \n> But there are others that look a bit problematic.  In this example\n> taken from fsck, we open all the pack index, only because it is\n> needed to show the progress, and the existing conditionals are ways\n> to avoid spending unneeded cycles.\n\nThat info is still used, when performing trace log during\nstop_progress():\n\n  if (p_progress && *p_progress) {\n          trace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n                             (*p_progress)->total);\n\n          if ((*p_progress)->throughput)\n                  trace2_data_intmax(\"progress\", the_repository,\n                                     \"total_bytes\",\n                                     (*p_progress)->throughput->curr_total);\n\n          trace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n  }\n\nBecause the progress struct is no longer NULL, we can write down the\nnumber of objects calculated to the trace2 log file. So the expense of\ncalculating it is not wasted, at least in scenarios where someone cares\nabout the output of the trace log.\n\nBut yes, lots of users don't care about trace log - they are using their\nhome computer and don't care about metrics like how many objects were\ntransferred in every single push. Now it becomes a little strange - is\nit better to instead wrap each 'struct progress' constructor in \"if\n(should_i_log || should_i_trace)\"? Have we got a simple way to look up\n'should_i_trace'?\n\nSome later comments have the same point - \"why am I calculating the\ntotals if the user won't see and doesn't care?\" - so I won't respond to\neach other one; my reply is the same.\n\n> \n> > @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> >  \t\t\tuint32_t total = 0, count = 0;\n> >  \t\t\tstruct progress *progress = NULL;\n> >  \n> > -\t\t\tif (show_progress) {\n> > -\t\t\t\tfor (p = get_all_packs(the_repository); p;\n> > -\t\t\t\t     p = p->next) {\n> > -\t\t\t\t\tif (open_pack_index(p))\n> > -\t\t\t\t\t\tcontinue;\n> > -\t\t\t\t\ttotal += p->num_objects;\n> > -\t\t\t\t}\n> > -\n> > -\t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n> > +\t\t\tfor (p = get_all_packs(the_repository); p;\n> > +\t\t\t     p = p->next) {\n> > +\t\t\t\tif (open_pack_index(p))\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\ttotal += p->num_objects;\n> >  \t\t\t}\n> > +\n> > +\t\t\tprogress = start_progress(_(\"Checking objects\"), total,\n> > +\t\t\t\t\t\t  show_progress);\n> >  \t\t\tfor (p = get_all_packs(the_repository); p;\n> >  \t\t\t     p = p->next) {\n> >  \t\t\t\t/* verify gives error messages itself */\n> \n> Likewise, we do not even have to be scanning the index entries\n> upfront if we are not showing the progress report (and more\n> importantly, the user likely has chosen the speed/cycles over eye\n> candy progress meter) while checking out paths from the index.\n> \n> > diff --git a/unpack-trees.c b/unpack-trees.c\n> > index 4be5fc3075..fa84244466 100644\n> > --- a/unpack-trees.c\n> > +++ b/unpack-trees.c\n> > @@ -338,16 +338,14 @@ static struct progress *get_progress(struct unpack_trees_options *o,\n> >  {\n> >  \tunsigned cnt = 0, total = 0;\n> >  \n> > -\tif (!o->update || !o->verbose_update)\n> > -\t\treturn NULL;\n> > -\n> >  \tfor (; cnt < index->cache_nr; cnt++) {\n> >  \t\tconst struct cache_entry *ce = index->cache[cnt];\n> >  \t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n> >  \t\t\ttotal++;\n> >  \t}\n> >  \n> > -\treturn start_delayed_progress(_(\"Updating files\"), total);\n> > +\treturn start_delayed_progress(_(\"Updating files\"), total,\n> > +\t\t\t\t      (o->update && o->verbose_update));\n> >  }\n> >  \n> >  static void setup_collided_checkout_detection(struct checkout *state,\n> \n> \n> And there are cases where the \"is progress enabled?\" parameter (by\n> the way, I am not sure 'verbose' is quite the right name for the\n\nI struggled some with this; I also considered using 'quiet' but it\nrequired inverting the logic everywhere when all callers already were\nasking \"should I print?\" instead of \"should I not print?\". Maybe \"print\"\nis better?\n\n> parameter and the field name, as that word implies there can be\n> multiple verbosity levels---what I see you've done is that you added\n> an \"is this enabled?\" flag) is not a simple variable, like these.\n> Some are simple enough, but some are not.  Some of the worst appear\n> in preload-index.c and read-cache.c---they may become easier to read\n> if we introduced a variable to be passed as the \"enable?\"  parameter\n> and compute it before makng a call, and that might be enough, though.\n\nSure, makes sense.\n\n> \n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 7016b28485..1a97a9f0be 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -1012,8 +1012,8 @@ static void write_pack_file(void)\n> >  \ttime_t last_mtime = 0;\n> >  \tstruct object_entry **write_order;\n> >  \n> > -\tif (progress > pack_to_stdout)\n> > -\t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n> > +\tprogress_state = start_progress(_(\"Writing objects\"), nr_result,\n> > +\t\t\t\t\t(progress > pack_to_stdout));\n> >  \tALLOC_ARRAY(written_list, to_pack.nr_objects);\n> >  \twrite_order = compute_write_order();\n> >  \n> \n> > @@ -1418,9 +1415,9 @@ int write_commit_graph_reachable(struct object_directory *odb,\n> >  \n> >  \tmemset(&data, 0, sizeof(data));\n> >  \tdata.commits = &commits;\n> > -\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n> > -\t\tdata.progress = start_delayed_progress(\n> > -\t\t\t_(\"Collecting referenced commits\"), 0);\n> > +\tdata.progress = start_delayed_progress(\n> > +\t\t_(\"Collecting referenced commits\"), 0,\n> > +\t\t(flags & COMMIT_GRAPH_WRITE_PROGRESS));\n> >  \n> >  \tfor_each_ref(add_ref_to_set, &data);\n> >  \tresult = write_commit_graph(odb, NULL, &commits,\n> \n> > @@ -2301,9 +2295,9 @@ 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> > -\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> > +\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n> > +\t\t\t\tg->num_commits,\n> > +\t\t\t\t(flags & COMMIT_GRAPH_WRITE_PROGRESS));\n> >  \n> >  \tfor (i = 0; i < g->num_commits; i++) {\n> >  \t\tstruct commit *graph_commit, *odb_commit;\n> \n> > diff --git a/midx.c b/midx.c\n> > index 6d1584ca51..d9ab163340 100644\n> > --- a/midx.c\n> > +++ b/midx.c\n> > @@ -836,10 +836,8 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n> >  \t}\n> >  \n> >  \tpacks.pack_paths_checked = 0;\n> > -\tif (flags & MIDX_PROGRESS)\n> > -\t\tpacks.progress = start_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n> > -\telse\n> > -\t\tpacks.progress = NULL;\n> > +\tpacks.progress = start_progress(_(\"Adding packfiles to multi-pack-index\"), 0,\n> > +\t\t\t\t\t\t(flags & MIDX_PROGRESS));\n> >  \n> >  \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &packs);\n> >  \tstop_progress(&packs.progress);\n> \n> > diff --git a/preload-index.c b/preload-index.c\n> > index ed6eaa4738..a924a84a14 100644\n> > --- a/preload-index.c\n> > +++ b/preload-index.c\n> > @@ -115,10 +115,9 @@ void preload_index(struct index_state *index,\n> >  \tmemset(&data, 0, sizeof(data));\n> >  \n> >  \tmemset(&pd, 0, sizeof(pd));\n> > -\tif (refresh_flags & REFRESH_PROGRESS && isatty(2)) {\n> > -\t\tpd.progress = start_delayed_progress(_(\"Refreshing index\"), index->cache_nr);\n> > -\t\tpthread_mutex_init(&pd.mutex, NULL);\n> > -\t}\n> > +\tpd.progress = start_delayed_progress(_(\"Refreshing index\"), index->cache_nr,\n> > +\t\t\t\t\t     (refresh_flags & REFRESH_PROGRESS && isatty(2)));\n> > +\tpthread_mutex_init(&pd.mutex, NULL);\n> >  \n> >  \tfor (i = 0; i < threads; i++) {\n> >  \t\tstruct thread_data *p = data+i;\n> \n> > diff --git a/read-cache.c b/read-cache.c\n> > index aa427c5c17..2ddc422dbd 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -1532,9 +1532,9 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n> >  \tconst char *unmerged_fmt;\n> >  \tstruct progress *progress = NULL;\n> >  \n> > -\tif (flags & REFRESH_PROGRESS && isatty(2))\n> > -\t\tprogress = start_delayed_progress(_(\"Refresh index\"),\n> > -\t\t\t\t\t\t  istate->cache_nr);\n> > +\tprogress = start_delayed_progress(_(\"Refresh index\"),\n> > +\t\t\t\t\t  istate->cache_nr,\n> > +\t\t\t\t\t  (flags & REFRESH_PROGRESS && isatty(2)));\n> >  \n> >  \ttrace_performance_enter();\n> >  \tmodified_fmt   = in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\";\n> \n> \n> And here is the only one that may benefit from this change, which\n> cares that the codepath prepared in rev-list, primarily to support\n> the progress meter, is exercised because those pieces of code\n> happens to be used by the tracing output as well.  Perhaps we\n> shouldn't have tied the \"when showing progress, we can also trace\"\n> earlier and we didn't have to touch this many codepath only to spend\n> cycles when progress output is not desired?\n\nI guess I don't see how this is the only caller that benefits. The\ntracing output should appear in all the places where I touched the code;\nthat was the point of the earlier change. Since the object total is provided in\nstart_progress() and friends, and the runtime is logged by\nregion_enter() in start_progress() and region_leave() in\nstop_progress(), then every caller is now unconditionally leaving traces\nto give some impression of how much work and how long it took. So I am\nconfused - can you explain how the other callers don't benefit?\n\n> \n> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > index f520111eda..f64cad8390 100644\n> > --- a/builtin/rev-list.c\n> > +++ b/builtin/rev-list.c\n> > @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> >  \tif (bisect_list)\n> >  \t\trevs.limited = 1;\n> >  \n> > -\tif (show_progress)\n> > -\t\tprogress = start_delayed_progress(show_progress, 0);\n> > +\t/*\n> > +\t * When progress is not printed to the user, we still want to be able to\n> > +\t * classify the progress during tracing. So, use a placeholder name.\n> > +\t */\n> > +\tprogress = start_delayed_progress(\n> > +\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n> > +\t\t\t0, show_progress != NULL);\n> \n> I've cut out the hunks that exhibits similar looking patterns as the\n> ones I covered in the above discussion.  You are right to say that\n> these are simple and similar changes all over the place.  I am not\n> yet convinced that the simplicity supports that this change is the\n> right thing to do, though.  The only saving grace might be that\n> there aren't that many hunks that explicitly do unnecessary things\n> like the ones in unpack-trees.c and builtin/fsck.c even when we are\n> not showing progress.  \n> \n> But the other codepaths may be doing conditional computation not\n> based on \"if (show_progress)\" but on \"if (progress)\", in which case\n\nHm, good point. For what it's worth, I performed the changes manually:\n\n  git grep \"start_progress()\"\n  for each caller, open in vim\n  /progress\n  delete or move if necessary\n  (repeat until all matches for /progress look right)\n\nand I did not see cases where 'if (progress)' was being used. That is\nhard to express in a diff; I can try to audit it better. (This process\nis how I also noticed the things to change in 2/2 in this series, even\nthough they are unrelated to start_progress()).\n\n> with this patch, we may be paying a lot more overhead even if we\n> know progress meter won't be shown and the worse part from\n> reviewability point of view is that this patch does not explicitly\n> do anything to make it happen because start_delayed_progress() now\n> unconditionally give a non-NULL progress structure to enable them.\n> \n> So I dunno.  It certainly is not for the upcoming release, but we do\n> want to make sure that trace requested even when the command is quiet\n> gets collected in the longer term (i.e. during the next cycle).\n\nOk. It sounds like you are saying you want the spirit of the change,\neven if not in this exact form - that is good to know.\n\n - Emily\n"},{"id":"403900","messageId":"20200817221938.GA331156@google.com","threadId":"53829","inReplyTo":"20200714001504.GI3189386@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-08-17T22:19:38Z","receivedAt":"2020-08-17T22:20:39Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Jul 13, 2020 at 05:15:04PM -0700, Emily Shaffer wrote:\n> \n> On Fri, Jul 10, 2020 at 03:40:25PM -0700, Junio C Hamano wrote:\n> > > diff --git a/builtin/blame.c b/builtin/blame.c\n> > > index 94ef57c1cc..1d72b27fda 100644\n> > > --- a/builtin/blame.c\n> > > +++ b/builtin/blame.c\n> > > @@ -1129,8 +1129,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> > >  \n> > >  \tsb.found_guilty_entry = &found_guilty_entry;\n> > >  \tsb.found_guilty_entry_data = &pi;\n> > > -\tif (show_progress)\n> > > -\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n> > > +\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines,\n> > > +\t\t\t\t\t     show_progress);\n> > >  \n> > >  \tassign_blame(&sb, opt);\n> > \n> > But there are others that look a bit problematic.  In this example\n> > taken from fsck, we open all the pack index, only because it is\n> > needed to show the progress, and the existing conditionals are ways\n> > to avoid spending unneeded cycles.\n> \n> That info is still used, when performing trace log during\n> stop_progress():\n> \n>   if (p_progress && *p_progress) {\n>           trace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n>                              (*p_progress)->total);\n> \n>           if ((*p_progress)->throughput)\n>                   trace2_data_intmax(\"progress\", the_repository,\n>                                      \"total_bytes\",\n>                                      (*p_progress)->throughput->curr_total);\n> \n>           trace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n>   }\n> \n> Because the progress struct is no longer NULL, we can write down the\n> number of objects calculated to the trace2 log file. So the expense of\n> calculating it is not wasted, at least in scenarios where someone cares\n> about the output of the trace log.\n> \n> But yes, lots of users don't care about trace log - they are using their\n> home computer and don't care about metrics like how many objects were\n> transferred in every single push. Now it becomes a little strange - is\n> it better to instead wrap each 'struct progress' constructor in \"if\n> (should_i_log || should_i_trace)\"? Have we got a simple way to look up\n> 'should_i_trace'?\n> \n> Some later comments have the same point - \"why am I calculating the\n> totals if the user won't see and doesn't care?\" - so I won't respond to\n> each other one; my reply is the same.\n> \n> > \n> > > @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> > >  \t\t\tuint32_t total = 0, count = 0;\n> > >  \t\t\tstruct progress *progress = NULL;\n> > >  \n> > > -\t\t\tif (show_progress) {\n> > > -\t\t\t\tfor (p = get_all_packs(the_repository); p;\n> > > -\t\t\t\t     p = p->next) {\n> > > -\t\t\t\t\tif (open_pack_index(p))\n> > > -\t\t\t\t\t\tcontinue;\n> > > -\t\t\t\t\ttotal += p->num_objects;\n> > > -\t\t\t\t}\n> > > -\n> > > -\t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n> > > +\t\t\tfor (p = get_all_packs(the_repository); p;\n> > > +\t\t\t     p = p->next) {\n> > > +\t\t\t\tif (open_pack_index(p))\n> > > +\t\t\t\t\tcontinue;\n> > > +\t\t\t\ttotal += p->num_objects;\n> > >  \t\t\t}\n> > > +\n> > > +\t\t\tprogress = start_progress(_(\"Checking objects\"), total,\n> > > +\t\t\t\t\t\t  show_progress);\n> > >  \t\t\tfor (p = get_all_packs(the_repository); p;\n> > >  \t\t\t     p = p->next) {\n> > >  \t\t\t\t/* verify gives error messages itself */\n> > \n> > Likewise, we do not even have to be scanning the index entries\n> > upfront if we are not showing the progress report (and more\n> > importantly, the user likely has chosen the speed/cycles over eye\n> > candy progress meter) while checking out paths from the index.\n> > \n> > > diff --git a/unpack-trees.c b/unpack-trees.c\n> > > index 4be5fc3075..fa84244466 100644\n> > > --- a/unpack-trees.c\n> > > +++ b/unpack-trees.c\n> > > @@ -338,16 +338,14 @@ static struct progress *get_progress(struct unpack_trees_options *o,\n> > >  {\n> > >  \tunsigned cnt = 0, total = 0;\n> > >  \n> > > -\tif (!o->update || !o->verbose_update)\n> > > -\t\treturn NULL;\n> > > -\n> > >  \tfor (; cnt < index->cache_nr; cnt++) {\n> > >  \t\tconst struct cache_entry *ce = index->cache[cnt];\n> > >  \t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n> > >  \t\t\ttotal++;\n> > >  \t}\n> > >  \n> > > -\treturn start_delayed_progress(_(\"Updating files\"), total);\n> > > +\treturn start_delayed_progress(_(\"Updating files\"), total,\n> > > +\t\t\t\t      (o->update && o->verbose_update));\n> > >  }\n> > >  \n> > >  static void setup_collided_checkout_detection(struct checkout *state,\n> > \n> > \n> > And there are cases where the \"is progress enabled?\" parameter (by\n> > the way, I am not sure 'verbose' is quite the right name for the\n> \n> I struggled some with this; I also considered using 'quiet' but it\n> required inverting the logic everywhere when all callers already were\n> asking \"should I print?\" instead of \"should I not print?\". Maybe \"print\"\n> is better?\n> \n> > parameter and the field name, as that word implies there can be\n> > multiple verbosity levels---what I see you've done is that you added\n> > an \"is this enabled?\" flag) is not a simple variable, like these.\n> > Some are simple enough, but some are not.  Some of the worst appear\n> > in preload-index.c and read-cache.c---they may become easier to read\n> > if we introduced a variable to be passed as the \"enable?\"  parameter\n> > and compute it before makng a call, and that might be enough, though.\n> \n> Sure, makes sense.\n> \n> > > diff --git a/read-cache.c b/read-cache.c\n> > > index aa427c5c17..2ddc422dbd 100644\n> > > --- a/read-cache.c\n> > > +++ b/read-cache.c\n> > > @@ -1532,9 +1532,9 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n> > >  \tconst char *unmerged_fmt;\n> > >  \tstruct progress *progress = NULL;\n> > >  \n> > > -\tif (flags & REFRESH_PROGRESS && isatty(2))\n> > > -\t\tprogress = start_delayed_progress(_(\"Refresh index\"),\n> > > -\t\t\t\t\t\t  istate->cache_nr);\n> > > +\tprogress = start_delayed_progress(_(\"Refresh index\"),\n> > > +\t\t\t\t\t  istate->cache_nr,\n> > > +\t\t\t\t\t  (flags & REFRESH_PROGRESS && isatty(2)));\n> > >  \n> > >  \ttrace_performance_enter();\n> > >  \tmodified_fmt   = in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\";\n> > \n> > \n> > And here is the only one that may benefit from this change, which\n> > cares that the codepath prepared in rev-list, primarily to support\n> > the progress meter, is exercised because those pieces of code\n> > happens to be used by the tracing output as well.  Perhaps we\n> > shouldn't have tied the \"when showing progress, we can also trace\"\n> > earlier and we didn't have to touch this many codepath only to spend\n> > cycles when progress output is not desired?\n> \n> I guess I don't see how this is the only caller that benefits. The\n> tracing output should appear in all the places where I touched the code;\n> that was the point of the earlier change. Since the object total is provided in\n> start_progress() and friends, and the runtime is logged by\n> region_enter() in start_progress() and region_leave() in\n> stop_progress(), then every caller is now unconditionally leaving traces\n> to give some impression of how much work and how long it took. So I am\n> confused - can you explain how the other callers don't benefit?\n> \n> > \n> > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > > index f520111eda..f64cad8390 100644\n> > > --- a/builtin/rev-list.c\n> > > +++ b/builtin/rev-list.c\n> > > @@ -620,8 +620,13 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> > >  \tif (bisect_list)\n> > >  \t\trevs.limited = 1;\n> > >  \n> > > -\tif (show_progress)\n> > > -\t\tprogress = start_delayed_progress(show_progress, 0);\n> > > +\t/*\n> > > +\t * When progress is not printed to the user, we still want to be able to\n> > > +\t * classify the progress during tracing. So, use a placeholder name.\n> > > +\t */\n> > > +\tprogress = start_delayed_progress(\n> > > +\t\t\tshow_progress ? show_progress : _(\"Quiet rev-list operation\"),\n> > > +\t\t\t0, show_progress != NULL);\n> > \n> > I've cut out the hunks that exhibits similar looking patterns as the\n> > ones I covered in the above discussion.  You are right to say that\n> > these are simple and similar changes all over the place.  I am not\n> > yet convinced that the simplicity supports that this change is the\n> > right thing to do, though.  The only saving grace might be that\n> > there aren't that many hunks that explicitly do unnecessary things\n> > like the ones in unpack-trees.c and builtin/fsck.c even when we are\n> > not showing progress.  \n> > \n> > But the other codepaths may be doing conditional computation not\n> > based on \"if (show_progress)\" but on \"if (progress)\", in which case\n> \n> Hm, good point. For what it's worth, I performed the changes manually:\n> \n>   git grep \"start_progress()\"\n>   for each caller, open in vim\n>   /progress\n>   delete or move if necessary\n>   (repeat until all matches for /progress look right)\n> \n> and I did not see cases where 'if (progress)' was being used. That is\n> hard to express in a diff; I can try to audit it better. (This process\n> is how I also noticed the things to change in 2/2 in this series, even\n> though they are unrelated to start_progress()).\n> \n> > with this patch, we may be paying a lot more overhead even if we\n> > know progress meter won't be shown and the worse part from\n> > reviewability point of view is that this patch does not explicitly\n> > do anything to make it happen because start_delayed_progress() now\n> > unconditionally give a non-NULL progress structure to enable them.\n> > \n> > So I dunno.  It certainly is not for the upcoming release, but we do\n> > want to make sure that trace requested even when the command is quiet\n> > gets collected in the longer term (i.e. during the next cycle).\n> \n> Ok. It sounds like you are saying you want the spirit of the change,\n> even if not in this exact form - that is good to know.\n\nA month later, are we still interested in this change? I'd like to\npursue it - we are trying to use this progress tracing internally to\ngenerate some metrics and not having this change (or some version of it)\nmeans those metrics aren't accurate.\n\nThe main concern I saw here was \"we are doing a lot of work that isn't\nused if the user doesn't want to log traces\" - should I approach a\nreroll of this topic by trying to be smarter about whether to set\n'quiet' or 'print' or 'verbose' or whatever it is renamed to, based on\nwhether there is a trace destination? Then for systems which are logging\ntraces the extra work is worth it, but for everyone else it can function\nas before.\n\nI don't love it from a design perspective - it feels a little like\nprogress module is looking a little too closely at trace module\ninternals - but from a performance perspective I see the appeal.\n\nOn the other hand, progress operations are showing progress because they\nare slow - is a little more work on something that's already\nhuman-noticeable slow such a big deal?\n\n - Emily\n"},{"id":"403904","messageId":"xmqqlficzydj.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200817221938.GA331156@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-17T22:44:08Z","receivedAt":"2020-08-17T22:44:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> The main concern I saw here was \"we are doing a lot of work that isn't\n> used if the user doesn't want to log traces\" - should I approach a\n> reroll of this topic by trying to be smarter about whether to set\n> 'quiet' or 'print' or 'verbose' or whatever it is renamed to, based on\n> whether there is a trace destination? Then for systems which are logging\n> traces the extra work is worth it, but for everyone else it can function\n> as before.\n>\n> I don't love it from a design perspective - it feels a little like\n> progress module is looking a little too closely at trace module\n> internals.\n\nIsn't that primarily due to the decision to tie progress and trace\ntoo closely?  If so, perhaps that needs to be revisited?\n\nI dunno.\n"},{"id":"403906","messageId":"xmqqh7t0zy43.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"xmqqlficzydj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-17T22:49:48Z","receivedAt":"2020-08-17T22:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Emily Shaffer <emilyshaffer@google.com> writes:\n>\n>> The main concern I saw here was \"we are doing a lot of work that isn't\n>> used if the user doesn't want to log traces\" - should I approach a\n>> reroll of this topic by trying to be smarter about whether to set\n>> 'quiet' or 'print' or 'verbose' or whatever it is renamed to, based on\n>> whether there is a trace destination? Then for systems which are logging\n>> traces the extra work is worth it, but for everyone else it can function\n>> as before.\n>>\n>> I don't love it from a design perspective - it feels a little like\n>> progress module is looking a little too closely at trace module\n>> internals.\n>\n> Isn't that primarily due to the decision to tie progress and trace\n> too closely?  If so, perhaps that needs to be revisited?\n\nOr the \"too close coupling\" needs to be accepted as the cost of\ndoing so (as \"progress is often a good cue for an event worth\ntracing\" was a convenient way to cheat by programmers not to spend\ntoo many braincycles to decide adding trace points---they\nautomatically got them when they decided to show progress output).\n\n>\n> I dunno.\n"},{"id":"405294","messageId":"20200909223631.864145-1-jonathantanmy@google.com","threadId":"53829","inReplyTo":"xmqq365zro2u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-09T22:36:31Z","receivedAt":"2020-09-09T22:37:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> But there are others that look a bit problematic.  In this example\n> taken from fsck, we open all the pack index, only because it is\n> needed to show the progress, and the existing conditionals are ways\n> to avoid spending unneeded cycles.\n> \n> > @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> >  \t\t\tuint32_t total = 0, count = 0;\n> >  \t\t\tstruct progress *progress = NULL;\n> >  \n> > -\t\t\tif (show_progress) {\n> > -\t\t\t\tfor (p = get_all_packs(the_repository); p;\n> > -\t\t\t\t     p = p->next) {\n> > -\t\t\t\t\tif (open_pack_index(p))\n> > -\t\t\t\t\t\tcontinue;\n> > -\t\t\t\t\ttotal += p->num_objects;\n> > -\t\t\t\t}\n> > -\n> > -\t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n> > +\t\t\tfor (p = get_all_packs(the_repository); p;\n> > +\t\t\t     p = p->next) {\n> > +\t\t\t\tif (open_pack_index(p))\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\ttotal += p->num_objects;\n> >  \t\t\t}\n> > +\n> > +\t\t\tprogress = start_progress(_(\"Checking objects\"), total,\n> > +\t\t\t\t\t\t  show_progress);\n> >  \t\t\tfor (p = get_all_packs(the_repository); p;\n> >  \t\t\t     p = p->next) {\n> >  \t\t\t\t/* verify gives error messages itself */\n> \n> Likewise, we do not even have to be scanning the index entries\n> upfront if we are not showing the progress report (and more\n> importantly, the user likely has chosen the speed/cycles over eye\n> candy progress meter) while checking out paths from the index.\n\nThis was the most problematic one I saw, and I don't think it's that\nproblematic - the loop at the bottom of the quotation above calls\nverify_pack(), which also calls open_pack_index(), so (unless some of\nthe \"struct packed_git\" are freed in the meantime - I haven't looked at\nthis closely) the opening of the pack indexes are not being wasted.\n\nI also saw some strbuf manipulation to generate the title, but I also\ndon't think that takes significant cycles compared to the task that\nrequires the progress display.\n\nBut if this is a problem, one thing we could do is pass the total as a\ncallback instead of as an int, and provide a generic callback that just\nreturns the dereferenced cb_data. Most invocations would just use that\ngeneric callback. (Alternatively, as discussed in-office, we could allow\nstart_progress() to return NULL when no progress display is needed,\nchange start_progress() to not take a total, add a progress_set_total(),\nand check in display_progress() that the total has been set before\nproceeding.)\n\n> But the other codepaths may be doing conditional computation not\n> based on \"if (show_progress)\" but on \"if (progress)\", in which case\n> with this patch, we may be paying a lot more overhead even if we\n> know progress meter won't be shown and the worse part from\n> reviewability point of view is that this patch does not explicitly\n> do anything to make it happen because start_delayed_progress() now\n> unconditionally give a non-NULL progress structure to enable them.\n\nOne way to enumerate this might be to get the LHS of all the assignments\nfrom start_progress() and friends (e.g. \"pi.progress\" in\nbuiltin/blame.c, \"progress\" in builtin/commit-graph.c) and then grepping\nthe respective files to see if \"if (.*[LHS]\" is done.\n"},{"id":"405295","messageId":"20200909224253.866718-1-jonathantanmy@google.com","threadId":"53829","inReplyTo":"xmqqh7t0zy43.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-09T22:42:53Z","receivedAt":"2020-09-09T22:43:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Emily Shaffer <emilyshaffer@google.com> writes:\n> >\n> >> The main concern I saw here was \"we are doing a lot of work that isn't\n> >> used if the user doesn't want to log traces\" - should I approach a\n> >> reroll of this topic by trying to be smarter about whether to set\n> >> 'quiet' or 'print' or 'verbose' or whatever it is renamed to, based on\n> >> whether there is a trace destination? Then for systems which are logging\n> >> traces the extra work is worth it, but for everyone else it can function\n> >> as before.\n> >>\n> >> I don't love it from a design perspective - it feels a little like\n> >> progress module is looking a little too closely at trace module\n> >> internals.\n> >\n> > Isn't that primarily due to the decision to tie progress and trace\n> > too closely?  If so, perhaps that needs to be revisited?\n> \n> Or the \"too close coupling\" needs to be accepted as the cost of\n> doing so (as \"progress is often a good cue for an event worth\n> tracing\" was a convenient way to cheat by programmers not to spend\n> too many braincycles to decide adding trace points---they\n> automatically got them when they decided to show progress output).\n\nI wouldn't describe it as \"cheat\", but I agree with the general\nsentiment - in general, I would think that if something is lengthy\nenough to need to indicate progress to the user, we should trace its\nperformance.\n"},{"id":"405296","messageId":"xmqqzh5ysg71.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200909223631.864145-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-09T23:06:42Z","receivedAt":"2020-09-10T02:00:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> I also saw some strbuf manipulation to generate the title, but I also\n> don't think that takes significant cycles compared to the task that\n> requires the progress display.\n\nI do not think we care so much about cycles.  What I find a lot more\ndisturbing was that this loses conceptual clarity (e.g. \"why are we\ngenerating the title unconditionally when we know we are not going\nto use it?\"), and that is why in the message you are responding to I\nwondered if this was a price we should accept to pay, as decided to\ntie the progress and tracing closely earlier.\n\n"},{"id":"405304","messageId":"20200910003157.GB667601@google.com","threadId":"53829","inReplyTo":"20200710014242.1088216-2-emilyshaffer@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-09-10T00:31:57Z","receivedAt":"2020-09-10T02:47:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nEmily Shaffer wrote:\n\n> Before now, the progress API is used by conditionally calling\n> start_progress() or a similar call, and then unconditionally calling\n> display_progress() and stop_progress(), both of which are tolerant of\n> NULL or uninitialized inputs.\n\nBeing super nitpicky for a moment:\n\n- Git's commit messages describe the behavior without the patch in\n  the present tense, as though the author were writing a bug report.\n  so \"Before now\" can be \"Today\".\n\n- with an uninitialized input, display_progress would produce undefined\n  behavior and likely crash ;-)\n\n[...]\n> Since this changes the API, also modify callers of start_progress() and\n> friends to drop their conditional and pass a new argument in instead.\n\nIf I understand correctly, the calling sequence before this patch is\nsomething like\n\n\tstruct progress *progress =\n\t\twant_progress ? start_progress(title, n) : NULL;\n\n\tfor (int i = 0; i < n; i++) {\n\t\t... do some work ...\n\t\tdisplay_progress(progress, i);\n\t}\n\n\tstop_progress(progress);\n\nand this patch changes it to\n\n\tstruct progress *progress = start_progress(title, n, want_progress);\n\n\tfor (int i = 0; i < n; i++) {\n\t\t... do some work ...\n\t\tdisplay_progress(progress, i);\n\t}\n\n\tstop_progress(progress);\n\nA few consequences:\n\n- it's a little briefer, which is nice\n\n- progress is always non-NULL, so we can't express\n\n\tif (progress) {\n\t\tfor ( ... ) {\n\t\t\t... do one chunk of work ...\n\t\t\tdisplay_progress(...);\n\t\t}\n\t} else {\n\t\t... do work slightly more efficiently, all in one chunk ...\n\t}\n\n- even if we don't want progress, we always spend the overhead of\n  allocating a progress struct (not a big deal)\n\n- if 'n' is a more complex expression (e.g. a function call), it gets\n  computed even if we don't want progress.  For example, in \"git fsck\",\n  as Junio noticed, this means accumulating the object counts from all\n  idx files just to throw them away.\n\n- the motivation: it means the progress API can be aware of whether\n  the caller wants to write progress to the terminal and has control\n  over what to do with that information.\n\n  In particular this makes the function name display_progress even\n  more of a misnomer --- before this patch, display_progress on a\n  non-NULL progress struct would display some progress information and\n  possibly also write something to traces, but after this patch it\n  sometimes only writes something to traces.\n\nOverall I think it's an improvement in the API.  It opens the door to\nfuture changes like making the progress API handle isatty checks in\nsome cases, perhaps.\n\n[...]\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n[...]\n> @@ -836,16 +836,15 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n>  \t\t\tuint32_t total = 0, count = 0;\n>  \t\t\tstruct progress *progress = NULL;\n>  \n> -\t\t\tif (show_progress) {\n> -\t\t\t\tfor (p = get_all_packs(the_repository); p;\n> -\t\t\t\t     p = p->next) {\n> -\t\t\t\t\tif (open_pack_index(p))\n> -\t\t\t\t\t\tcontinue;\n> -\t\t\t\t\ttotal += p->num_objects;\n> -\t\t\t\t}\n> -\n> -\t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n> +\t\t\tfor (p = get_all_packs(the_repository); p;\n> +\t\t\t     p = p->next) {\n> +\t\t\t\tif (open_pack_index(p))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\ttotal += p->num_objects;\n>  \t\t\t}\n> +\n> +\t\t\tprogress = start_progress(_(\"Checking objects\"), total,\n> +\t\t\t\t\t\t  show_progress);\n\nAs Jonathan mentions[1], this is nothing next to the work that fsck is\nabout to do on those objects, so although it's aesthetically\nunappealing to have to compute this total just in order to throw it\naway, it's not likely to make a big difference.\n\nThat said, what would an API look like that avoids that?\n\nOne possibility would be to make separate initialization and\nstart-of-progress calls:\n\n\tstruct progress *progress = progress_new(show_progress, the_repository);\n\n\tif (progress_is_enabled(progress)) {\n\t\tfor (...) {\n\t\t\t...\n\t\t\ttotal += ...\n\t\t}\n\n\t\tstart_progress(progress, _(\"Checking objects\"), total);\n\t}\n\nUsing a callback to supply the title and total like Jonathan suggests\nis another possibility.  It seems a bit more fussy, though.\n\n[...]\n> --- a/progress.h\n> +++ b/progress.h\n> @@ -13,11 +13,13 @@ void progress_test_force_update(void);\n>  \n>  void display_throughput(struct progress *progress, uint64_t total);\n>  void display_progress(struct progress *progress, uint64_t n);\n> -struct progress *start_progress(const char *title, uint64_t total);\n> -struct progress *start_sparse_progress(const char *title, uint64_t total);\n> -struct progress *start_delayed_progress(const char *title, uint64_t total);\n> +struct progress *start_progress(const char *title, uint64_t total, int verbose);\n> +struct progress *start_sparse_progress(const char *title, uint64_t total,\n> +\t\t\t\t       int verbose);\n> +struct progress *start_delayed_progress(const char *title, uint64_t total,\n> +\t\t\t\t\tint verbose);\n>  struct progress *start_delayed_sparse_progress(const char *title,\n> -\t\t\t\t\t       uint64_t total);\n> +\t\t\t\t\t       uint64_t total, int verbose);\n>  void stop_progress(struct progress **progress);\n>  void stop_progress_msg(struct progress **progress, const char *msg);\n\nThis header contains no comments and there's no API docs for it in\nDocumentation/technical/ waiting to be copied into comments, so we end\nup having to reverse engineer it.  Not about this patch, but it would\nbe nice to add an overview of the progress API to this file (e.g., to\nshow a typical calling sequence).\n\nSince display_progress and display_throughput are no longer about\ndisplay, would it make sense to rename them (in a separate patch)?\nFor example,\n\n\tupdate_progress(progress, ++i);\n\tupdate_throughput(progress, bytes);\n\n\"update\" is a bit vague but it may convey more clearly that this\naffects traces too and not just what is written to the terminal.\n\n> --- a/prune-packed.c\n> +++ b/prune-packed.c\n> @@ -31,8 +31,8 @@ static int prune_object(const struct object_id *oid, const char *path,\n>  \n>  void prune_packed_objects(int opts)\n>  {\n> -\tif (opts & PRUNE_PACKED_VERBOSE)\n> -\t\tprogress = start_delayed_progress(_(\"Removing duplicate objects\"), 256);\n> +\tprogress = start_delayed_progress(_(\"Removing duplicate objects\"), 256,\n> +\t\t\t\t\t  (opts & PRUNE_PACKED_VERBOSE));\n\nstyle nit: no need for parens around the function parameter (likewise\nfor most of these)\n\n[...]\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -309,4 +309,31 @@ test_expect_success 'progress generates traces' '\n>  \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n>  '\n>  \n> +test_expect_success 'progress generates traces even quietly' '\n\nNice.\n\nWith whatever subset of the changes discussed makes sense,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n[1] https://lore.kernel.org/git/20200909224253.866718-1-jonathantanmy@google.com\n"},{"id":"405313","messageId":"xmqqeenarzf2.fsf@gitster.c.googlers.com","threadId":"53829","inReplyTo":"20200910003157.GB667601@google.com","subject":"Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-10T05:09:05Z","receivedAt":"2020-09-10T05:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> A few consequences:\n>\n> - it's a little briefer, which is nice\n\nAlso this makes start always pair with stop, which is cleaner,\nespecially if there is no extra work.\n\nThe current \"if you do not want the overhead when not collecting\nstats for and showing progress, just avoid calling start---stop\nwithout calling start will be a no-op and safe anyway\" arrangement\nwe have feels a bit kludgy.\n\n> - progress is always non-NULL, so we can't express\n>\n> \tif (progress) {\n> \t\tfor ( ... ) {\n> \t\t\t... do one chunk of work ...\n> \t\t\tdisplay_progress(...);\n> \t\t}\n> \t} else {\n> \t\t... do work slightly more efficiently, all in one chunk ...\n> \t}\n\nYes, this is the other side of the coin.  When there is significant\ndifference in the work between with and without progress codepath,\nit is convenient to be able to switch on the \"progress\" pointer\nitself.  The progress_is_enabled() helper you bring up later may be\na way to solve it.\n\n> - even if we don't want progress, we always spend the overhead of\n>   allocating a progress struct (not a big deal)\n\nTrue.\n\n> - if 'n' is a more complex expression (e.g. a function call), it gets\n>   computed even if we don't want progress.  For example, in \"git fsck\",\n>   as Junio noticed, this means accumulating the object counts from all\n>   idx files just to throw them away.\n\nYes, I think the conceptual muddiness caused by this is what\ndisturbed me the most.  The cost of counting would likely to be\nnegligible; developers' time to understand why things are counted\nin the first place however is the true waste.\n\n> - the motivation: it means the progress API can be aware of whether\n>   the caller wants to write progress to the terminal and has control\n>   over what to do with that information.\n>\n>   In particular this makes the function name display_progress even\n>   more of a misnomer --- before this patch, display_progress on a\n>   non-NULL progress struct would display some progress information and\n>   possibly also write something to traces, but after this patch it\n>   sometimes only writes something to traces.\n\nYeah, this might show us a way to an acceptable solution to the\nproblem of conceptual uncleanliness, as naming may have a lot to\ncontribute to it.\n\n> That said, what would an API look like that avoids that?\n>\n> One possibility would be to make separate initialization and\n> start-of-progress calls:\n>\n> \tstruct progress *progress = progress_new(show_progress, the_repository);\n>\n> \tif (progress_is_enabled(progress)) {\n> \t\tfor (...) {\n> \t\t\t...\n> \t\t\ttotal += ...\n> \t\t}\n>\n> \t\tstart_progress(progress, _(\"Checking objects\"), total);\n> \t}\n\nOK.\n"}]}