Merge branch 'ab/only-single-progress-at-once'

Further tweaks on progress API.

* ab/only-single-progress-at-once:
  pack-bitmap-write.c: don't return without stop_progress()
  progress API: unify stop_progress{,_msg}(), fix trace2 bug
  progress.c: refactor stop_progress{,_msg}() to use helpers
  progress.c: use dereferenced "progress" variable, not "(*p_progress)"
  progress.h: format and be consistent with progress.c naming
  progress.c tests: test some invalid usage
  progress.c tests: make start/stop commands on stdin
  progress.c test helper: add missing braces
  leak tests: fix a memory leak in "test-progress" helper
This commit is contained in:
Junio C Hamano 2022-02-25 15:47:35 -08:00
commit a47fcfe871
6 changed files with 169 additions and 75 deletions

View file

@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
QSORT(indexed_commits, indexed_commits_nr, date_compare); QSORT(indexed_commits, indexed_commits_nr, date_compare);
if (writer.show_progress)
writer.progress = start_progress("Selecting bitmap commits", 0);
if (indexed_commits_nr < 100) { if (indexed_commits_nr < 100) {
for (i = 0; i < indexed_commits_nr; ++i) for (i = 0; i < indexed_commits_nr; ++i)
push_bitmapped_commit(indexed_commits[i]); push_bitmapped_commit(indexed_commits[i]);
return; return;
} }
if (writer.show_progress)
writer.progress = start_progress("Selecting bitmap commits", 0);
for (;;) { for (;;) {
struct commit *chosen = NULL; struct commit *chosen = NULL;

View file

@ -311,32 +311,39 @@ struct progress *start_delayed_sparse_progress(const char *title,
static void finish_if_sparse(struct progress *progress) static void finish_if_sparse(struct progress *progress)
{ {
if (progress && if (progress->sparse &&
progress->sparse &&
progress->last_value != progress->total) progress->last_value != progress->total)
display_progress(progress, progress->total); display_progress(progress, progress->total);
} }
void stop_progress(struct progress **p_progress) static void force_last_update(struct progress *progress, const char *msg)
{ {
if (!p_progress) char *buf;
BUG("don't provide NULL to stop_progress"); struct throughput *tp = progress->throughput;
finish_if_sparse(*p_progress); if (tp) {
uint64_t now_ns = progress_getnanotime(progress);
if (*p_progress) { unsigned int misecs, rate;
trace2_data_intmax("progress", the_repository, "total_objects", misecs = ((now_ns - progress->start_ns) * 4398) >> 32;
(*p_progress)->total); rate = tp->curr_total / (misecs ? misecs : 1);
throughput_string(&tp->display, tp->curr_total, rate);
if ((*p_progress)->throughput)
trace2_data_intmax("progress", the_repository,
"total_bytes",
(*p_progress)->throughput->curr_total);
trace2_region_leave("progress", (*p_progress)->title, the_repository);
} }
progress_update = 1;
buf = xstrfmt(", %s.\n", msg);
display(progress, progress->last_value, buf);
free(buf);
}
stop_progress_msg(p_progress, _("done")); static void log_trace2(struct progress *progress)
{
trace2_data_intmax("progress", the_repository, "total_objects",
progress->total);
if (progress->throughput)
trace2_data_intmax("progress", the_repository, "total_bytes",
progress->throughput->curr_total);
trace2_region_leave("progress", progress->title, the_repository);
} }
void stop_progress_msg(struct progress **p_progress, const char *msg) void stop_progress_msg(struct progress **p_progress, const char *msg)
@ -350,23 +357,12 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
if (!progress) if (!progress)
return; return;
*p_progress = NULL; *p_progress = NULL;
if (progress->last_value != -1) {
/* Force the last update */
char *buf;
struct throughput *tp = progress->throughput;
if (tp) { finish_if_sparse(progress);
uint64_t now_ns = progress_getnanotime(progress); if (progress->last_value != -1)
unsigned int misecs, rate; force_last_update(progress, msg);
misecs = ((now_ns - progress->start_ns) * 4398) >> 32; log_trace2(progress);
rate = tp->curr_total / (misecs ? misecs : 1);
throughput_string(&tp->display, tp->curr_total, rate);
}
progress_update = 1;
buf = xstrfmt(", %s.\n", msg);
display(progress, progress->last_value, buf);
free(buf);
}
clear_progress_signal(); clear_progress_signal();
strbuf_release(&progress->counters_sb); strbuf_release(&progress->counters_sb);
if (progress->throughput) if (progress->throughput)

View file

@ -1,5 +1,6 @@
#ifndef PROGRESS_H #ifndef PROGRESS_H
#define PROGRESS_H #define PROGRESS_H
#include "gettext.h"
struct progress; struct progress;
@ -18,7 +19,9 @@ struct progress *start_sparse_progress(const char *title, uint64_t total);
struct progress *start_delayed_progress(const char *title, uint64_t total); struct progress *start_delayed_progress(const char *title, uint64_t total);
struct progress *start_delayed_sparse_progress(const char *title, struct progress *start_delayed_sparse_progress(const char *title,
uint64_t total); uint64_t total);
void stop_progress(struct progress **progress); void stop_progress_msg(struct progress **p_progress, const char *msg);
void stop_progress_msg(struct progress **progress, const char *msg); static inline void stop_progress(struct progress **p_progress)
{
stop_progress_msg(p_progress, _("done"));
}
#endif #endif

View file

@ -3,6 +3,9 @@
* *
* Reads instructions from standard input, one instruction per line: * Reads instructions from standard input, one instruction per line:
* *
* "start <total>[ <title>]" - Call start_progress(title, total),
* Uses the default title of "Working hard"
* if the " <title>" is omitted.
* "progress <items>" - Call display_progress() with the given item count * "progress <items>" - Call display_progress() with the given item count
* as parameter. * as parameter.
* "throughput <bytes> <millis> - Call display_throughput() with the given * "throughput <bytes> <millis> - Call display_throughput() with the given
@ -10,6 +13,7 @@
* specify the time elapsed since the * specify the time elapsed since the
* start_progress() call. * start_progress() call.
* "update" - Set the 'progress_update' flag. * "update" - Set the 'progress_update' flag.
* "stop" - Call stop_progress().
* *
* See 't0500-progress-display.sh' for examples. * See 't0500-progress-display.sh' for examples.
*/ */
@ -19,34 +23,50 @@
#include "parse-options.h" #include "parse-options.h"
#include "progress.h" #include "progress.h"
#include "strbuf.h" #include "strbuf.h"
#include "string-list.h"
int cmd__progress(int argc, const char **argv) int cmd__progress(int argc, const char **argv)
{ {
int total = 0; const char *const default_title = "Working hard";
const char *title; struct string_list titles = STRING_LIST_INIT_DUP;
struct strbuf line = STRBUF_INIT; struct strbuf line = STRBUF_INIT;
struct progress *progress; struct progress *progress = NULL;
const char *usage[] = { const char *usage[] = {
"test-tool progress [--total=<n>] <progress-title>", "test-tool progress <stdin",
NULL NULL
}; };
struct option options[] = { struct option options[] = {
OPT_INTEGER(0, "total", &total, "total number of items"),
OPT_END(), OPT_END(),
}; };
argc = parse_options(argc, argv, NULL, options, usage, 0); argc = parse_options(argc, argv, NULL, options, usage, 0);
if (argc != 1) if (argc)
die("need a title for the progress output"); usage_with_options(usage, options);
title = argv[0];
progress_testing = 1; progress_testing = 1;
progress = start_progress(title, total);
while (strbuf_getline(&line, stdin) != EOF) { while (strbuf_getline(&line, stdin) != EOF) {
char *end; char *end;
if (skip_prefix(line.buf, "progress ", (const char **) &end)) { if (skip_prefix(line.buf, "start ", (const char **) &end)) {
uint64_t total = strtoull(end, &end, 10);
const char *title;
/*
* We can't use "end + 1" as an argument to
* start_progress(), it doesn't xstrdup() its
* "title" argument. We need to hold onto a
* valid "char *" for it until the end.
*/
if (!*end)
title = default_title;
else if (*end == ' ')
title = string_list_insert(&titles, end + 1)->string;
else
die("invalid input: '%s'\n", line.buf);
progress = start_progress(title, total);
} else if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
uint64_t item_count = strtoull(end, &end, 10); uint64_t item_count = strtoull(end, &end, 10);
if (*end != '\0') if (*end != '\0')
die("invalid input: '%s'\n", line.buf); die("invalid input: '%s'\n", line.buf);
@ -63,12 +83,16 @@ int cmd__progress(int argc, const char **argv)
die("invalid input: '%s'\n", line.buf); die("invalid input: '%s'\n", line.buf);
progress_test_ns = test_ms * 1000 * 1000; progress_test_ns = test_ms * 1000 * 1000;
display_throughput(progress, byte_count); display_throughput(progress, byte_count);
} else if (!strcmp(line.buf, "update")) } else if (!strcmp(line.buf, "update")) {
progress_test_force_update(); progress_test_force_update();
else } else if (!strcmp(line.buf, "stop")) {
stop_progress(&progress);
} else {
die("invalid input: '%s'\n", line.buf); die("invalid input: '%s'\n", line.buf);
}
} }
stop_progress(&progress); strbuf_release(&line);
string_list_clear(&titles, 0);
return 0; return 0;
} }

View file

@ -2,6 +2,7 @@
test_description='progress display' test_description='progress display'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh . ./test-lib.sh
show_cr () { show_cr () {
@ -17,6 +18,7 @@ test_expect_success 'simple progress display' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 0
update update
progress 1 progress 1
update update
@ -25,8 +27,9 @@ test_expect_success 'simple progress display' '
progress 4 progress 4
update update
progress 5 progress 5
stop
EOF EOF
test-tool progress "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -41,11 +44,13 @@ test_expect_success 'progress display with total' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 3
progress 1 progress 1
progress 2 progress 2
progress 3 progress 3
stop
EOF EOF
test-tool progress --total=3 "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -62,14 +67,14 @@ Working hard.......2.........3.........4.........5.........6:
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 100000 Working hard.......2.........3.........4.........5.........6
progress 100 progress 100
progress 1000 progress 1000
progress 10000 progress 10000
progress 100000 progress 100000
stop
EOF EOF
test-tool progress --total=100000 \ test-tool progress <in 2>stderr &&
"Working hard.......2.........3.........4.........5.........6" \
<in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -88,16 +93,16 @@ Working hard.......2.........3.........4.........5.........6:
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 100000 Working hard.......2.........3.........4.........5.........6
update update
progress 1 progress 1
update update
progress 2 progress 2
progress 10000 progress 10000
progress 100000 progress 100000
stop
EOF EOF
test-tool progress --total=100000 \ test-tool progress <in 2>stderr &&
"Working hard.......2.........3.........4.........5.........6" \
<in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -116,14 +121,14 @@ Working hard.......2.........3.........4.........5.........6:
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 100000 Working hard.......2.........3.........4.........5.........6
progress 25000 progress 25000
progress 50000 progress 50000
progress 75000 progress 75000
progress 100000 progress 100000
stop
EOF EOF
test-tool progress --total=100000 \ test-tool progress <in 2>stderr &&
"Working hard.......2.........3.........4.........5.........6" \
<in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -140,14 +145,14 @@ Working hard.......2.........3.........4.........5.........6.........7.........:
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 100000 Working hard.......2.........3.........4.........5.........6.........7.........
progress 25000 progress 25000
progress 50000 progress 50000
progress 75000 progress 75000
progress 100000 progress 100000
stop
EOF EOF
test-tool progress --total=100000 \ test-tool progress <in 2>stderr &&
"Working hard.......2.........3.........4.........5.........6.........7........." \
<in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -164,12 +169,14 @@ test_expect_success 'progress shortens - crazy caller' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 1000
progress 100 progress 100
progress 200 progress 200
progress 1 progress 1
progress 1000 progress 1000
stop
EOF EOF
test-tool progress --total=1000 "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -185,6 +192,7 @@ test_expect_success 'progress display with throughput' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 0
throughput 102400 1000 throughput 102400 1000
update update
progress 10 progress 10
@ -197,8 +205,9 @@ test_expect_success 'progress display with throughput' '
throughput 409600 4000 throughput 409600 4000
update update
progress 40 progress 40
stop
EOF EOF
test-tool progress "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -214,6 +223,7 @@ test_expect_success 'progress display with throughput and total' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 40
throughput 102400 1000 throughput 102400 1000
progress 10 progress 10
throughput 204800 2000 throughput 204800 2000
@ -222,8 +232,9 @@ test_expect_success 'progress display with throughput and total' '
progress 30 progress 30
throughput 409600 4000 throughput 409600 4000
progress 40 progress 40
stop
EOF EOF
test-tool progress --total=40 "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -239,6 +250,7 @@ test_expect_success 'cover up after throughput shortens' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 0
throughput 409600 1000 throughput 409600 1000
update update
progress 1 progress 1
@ -251,8 +263,9 @@ test_expect_success 'cover up after throughput shortens' '
throughput 1638400 4000 throughput 1638400 4000
update update
progress 4 progress 4
stop
EOF EOF
test-tool progress "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -267,6 +280,7 @@ test_expect_success 'cover up after throughput shortens a lot' '
EOF EOF
cat >in <<-\EOF && cat >in <<-\EOF &&
start 0
throughput 1 1000 throughput 1 1000
update update
progress 1 progress 1
@ -276,8 +290,9 @@ test_expect_success 'cover up after throughput shortens a lot' '
throughput 3145728 3000 throughput 3145728 3000
update update
progress 3 progress 3
stop
EOF EOF
test-tool progress "Working hard" <in 2>stderr && test-tool progress <in 2>stderr &&
show_cr <stderr >out && show_cr <stderr >out &&
test_cmp expect out test_cmp expect out
@ -285,6 +300,7 @@ test_expect_success 'cover up after throughput shortens a lot' '
test_expect_success 'progress generates traces' ' test_expect_success 'progress generates traces' '
cat >in <<-\EOF && cat >in <<-\EOF &&
start 40
throughput 102400 1000 throughput 102400 1000
update update
progress 10 progress 10
@ -297,10 +313,11 @@ test_expect_success 'progress generates traces' '
throughput 409600 4000 throughput 409600 4000
update update
progress 40 progress 40
stop
EOF EOF
GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress --total=40 \ GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress \
"Working hard" <in 2>stderr && <in 2>stderr &&
# t0212/parse_events.perl intentionally omits regions and data. # t0212/parse_events.perl intentionally omits regions and data.
test_region progress "Working hard" trace.event && test_region progress "Working hard" trace.event &&
@ -308,4 +325,54 @@ test_expect_success 'progress generates traces' '
grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
' '
test_expect_success 'progress generates traces: stop / start' '
cat >in <<-\EOF &&
start 0
stop
EOF
GIT_TRACE2_EVENT="$PWD/trace-startstop.event" test-tool progress \
<in 2>stderr &&
test_region progress "Working hard" trace-startstop.event
'
test_expect_success 'progress generates traces: start without stop' '
cat >in <<-\EOF &&
start 0
EOF
GIT_TRACE2_EVENT="$PWD/trace-start.event" \
LSAN_OPTIONS=detect_leaks=0 \
test-tool progress \
<in 2>stderr &&
grep region_enter.*progress trace-start.event &&
! grep region_leave.*progress trace-start.event
'
test_expect_success 'progress generates traces: stop without start' '
cat >in <<-\EOF &&
stop
EOF
GIT_TRACE2_EVENT="$PWD/trace-stop.event" test-tool progress \
<in 2>stderr &&
! grep region_enter.*progress trace-stop.event &&
! grep region_leave.*progress trace-stop.event
'
test_expect_success 'progress generates traces: start with active progress bar (no stops)' '
cat >in <<-\EOF &&
start 0 One
start 0 Two
EOF
GIT_TRACE2_EVENT="$PWD/trace-2start.event" \
LSAN_OPTIONS=detect_leaks=0 \
test-tool progress \
<in 2>stderr &&
grep region_enter.*progress.*One trace-2start.event &&
grep region_enter.*progress.*Two trace-2start.event &&
! grep region_leave trace-2start.event
'
test_done test_done

View file

@ -64,7 +64,11 @@ test_expect_success 'create series of packs' '
echo $cur && echo $cur &&
echo "$(git rev-parse :file) file" echo "$(git rev-parse :file) file"
} | git pack-objects --stdout >tmp && } | git pack-objects --stdout >tmp &&
git index-pack --stdin --fix-thin <tmp || return 1 GIT_TRACE2_EVENT=$PWD/trace \
git index-pack -v --stdin --fix-thin <tmp || return 1 &&
grep -c region_enter.*progress trace >enter &&
grep -c region_leave.*progress trace >leave &&
test_cmp enter leave &&
prev=$cur prev=$cur
done done
' '