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

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e1f55c28ecb4767de17e815968d35f6..cab3eaa2acd141d306eeb820e6278561555a33cd 100644 (file)
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
 
        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) {
                for (i = 0; i < indexed_commits_nr; ++i)
                        push_bitmapped_commit(indexed_commits[i]);
                return;
        }
 
+       if (writer.show_progress)
+               writer.progress = start_progress("Selecting bitmap commits", 0);
+
        for (;;) {
                struct commit *chosen = NULL;
 

diff --git a/progress.c b/progress.c
index 680c6a8bf93b514a7b82510c475984ca32067eb7..0cdd875d37f166bedbbeb5f0e889046674ed58be 100644 (file)
--- a/progress.c
+++ b/progress.c
@@ -311,32 +311,39 @@ struct progress *start_delayed_sparse_progress(const char *title,
 
 static void finish_if_sparse(struct progress *progress)
 {
-       if (progress &&
-           progress->sparse &&
+       if (progress->sparse &&
            progress->last_value != 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)
-               BUG("don't provide NULL to stop_progress");
-
-       finish_if_sparse(*p_progress);
-
-       if (*p_progress) {
-               trace2_data_intmax("progress", the_repository, "total_objects",
-                                  (*p_progress)->total);
+       char *buf;
+       struct throughput *tp = progress->throughput;
+
+       if (tp) {
+               uint64_t now_ns = progress_getnanotime(progress);
+               unsigned int misecs, rate;
+               misecs = ((now_ns - progress->start_ns) * 4398) >> 32;
+               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);
+}
 
-               if ((*p_progress)->throughput)
-                       trace2_data_intmax("progress", the_repository,
-                                          "total_bytes",
-                                          (*p_progress)->throughput->curr_total);
+static void log_trace2(struct progress *progress)
+{
+       trace2_data_intmax("progress", the_repository, "total_objects",
+                          progress->total);
 
-               trace2_region_leave("progress", (*p_progress)->title, the_repository);
-       }
+       if (progress->throughput)
+               trace2_data_intmax("progress", the_repository, "total_bytes",
+                                  progress->throughput->curr_total);
 
-       stop_progress_msg(p_progress, _("done"));
+       trace2_region_leave("progress", progress->title, the_repository);
 }
 
 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)
                return;
        *p_progress = NULL;
-       if (progress->last_value != -1) {
-               /* Force the last update */
-               char *buf;
-               struct throughput *tp = progress->throughput;
-
-               if (tp) {
-                       uint64_t now_ns = progress_getnanotime(progress);
-                       unsigned int misecs, rate;
-                       misecs = ((now_ns - progress->start_ns) * 4398) >> 32;
-                       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);
-       }
+
+       finish_if_sparse(progress);
+       if (progress->last_value != -1)
+               force_last_update(progress, msg);
+       log_trace2(progress);
+
        clear_progress_signal();
        strbuf_release(&progress->counters_sb);
        if (progress->throughput)

diff --git a/progress.h b/progress.h
index f1913acf73f1790366ebd2f3f802f0472221bc7c..3a945637c81c22734b563325b66956ee4fb33b0b 100644 (file)
--- a/progress.h
+++ b/progress.h
@@ -1,5 +1,6 @@
 #ifndef PROGRESS_H
 #define PROGRESS_H
+#include "gettext.h"
 
 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_sparse_progress(const char *title,
                                               uint64_t total);
-void stop_progress(struct progress **progress);
-void stop_progress_msg(struct progress **progress, const char *msg);
-
+void stop_progress_msg(struct progress **p_progress, const char *msg);
+static inline void stop_progress(struct progress **p_progress)
+{
+       stop_progress_msg(p_progress, _("done"));
+}
 #endif

diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 5d05cbe7894097f6410328b966bda936e678237f..6cc9735b60127b2f5bab7d1728dc013a27b7adf4 100644 (file)
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -3,6 +3,9 @@
  *
  * 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
  *                        as parameter.
  *   "throughput <bytes> <millis> - Call display_throughput() with the given
@@ -10,6 +13,7 @@
  *                                  specify the time elapsed since the
  *                                  start_progress() call.
  *   "update" - Set the 'progress_update' flag.
+ *   "stop" - Call stop_progress().
  *
  * See 't0500-progress-display.sh' for examples.
  */
@@ -19,34 +23,50 @@
 #include "parse-options.h"
 #include "progress.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 int cmd__progress(int argc, const char **argv)
 {
-       int total = 0;
-       const char *title;
+       const char *const default_title = "Working hard";
+       struct string_list titles = STRING_LIST_INIT_DUP;
        struct strbuf line = STRBUF_INIT;
-       struct progress *progress;
+       struct progress *progress = NULL;
 
        const char *usage[] = {
-               "test-tool progress [--total=<n>] <progress-title>",
+               "test-tool progress <stdin",
                NULL
        };
        struct option options[] = {
-               OPT_INTEGER(0, "total", &total, "total number of items"),
                OPT_END(),
        };
 
        argc = parse_options(argc, argv, NULL, options, usage, 0);
-       if (argc != 1)
-               die("need a title for the progress output");
-       title = argv[0];
+       if (argc)
+               usage_with_options(usage, options);
 
        progress_testing = 1;
-       progress = start_progress(title, total);
        while (strbuf_getline(&line, stdin) != EOF) {
                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);
                        if (*end != '\0')
                                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);
                        progress_test_ns = test_ms * 1000 * 1000;
                        display_throughput(progress, byte_count);
-               } else if (!strcmp(line.buf, "update"))
+               } else if (!strcmp(line.buf, "update")) {
                        progress_test_force_update();
-               else
+               } else if (!strcmp(line.buf, "stop")) {
+                       stop_progress(&progress);
+               } else {
                        die("invalid input: '%s'\n", line.buf);
+               }
        }
-       stop_progress(&progress);
+       strbuf_release(&line);
+       string_list_clear(&titles, 0);
 
        return 0;
 }

diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 22058b503ac78c41f542eb02a5a3dbd1cb200d3e..1eb3a8306ba15b3e782b940f605afa8ec551ec8c 100755 (executable)
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -2,6 +2,7 @@
 
 test_description='progress display'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 show_cr () {
@@ -17,6 +18,7 @@ test_expect_success 'simple progress display' '
        EOF
 
        cat >in <<-\EOF &&
+       start 0
        update
        progress 1
        update
@@ -25,8 +27,9 @@ test_expect_success 'simple progress display' '
        progress 4
        update
        progress 5
+       stop
        EOF
-       test-tool progress "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -41,11 +44,13 @@ test_expect_success 'progress display with total' '
        EOF
 
        cat >in <<-\EOF &&
+       start 3
        progress 1
        progress 2
        progress 3
+       stop
        EOF
-       test-tool progress --total=3 "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -62,14 +67,14 @@ Working hard.......2.........3.........4.........5.........6:
 EOF
 
        cat >in <<-\EOF &&
+       start 100000 Working hard.......2.........3.........4.........5.........6
        progress 100
        progress 1000
        progress 10000
        progress 100000
+       stop
        EOF
-       test-tool progress --total=100000 \
-               "Working hard.......2.........3.........4.........5.........6" \
-               <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -88,16 +93,16 @@ Working hard.......2.........3.........4.........5.........6:
 EOF
 
        cat >in <<-\EOF &&
+       start 100000 Working hard.......2.........3.........4.........5.........6
        update
        progress 1
        update
        progress 2
        progress 10000
        progress 100000
+       stop
        EOF
-       test-tool progress --total=100000 \
-               "Working hard.......2.........3.........4.........5.........6" \
-               <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -116,14 +121,14 @@ Working hard.......2.........3.........4.........5.........6:
 EOF
 
        cat >in <<-\EOF &&
+       start 100000 Working hard.......2.........3.........4.........5.........6
        progress 25000
        progress 50000
        progress 75000
        progress 100000
+       stop
        EOF
-       test-tool progress --total=100000 \
-               "Working hard.......2.........3.........4.........5.........6" \
-               <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -140,14 +145,14 @@ Working hard.......2.........3.........4.........5.........6.........7.........:
 EOF
 
        cat >in <<-\EOF &&
+       start 100000 Working hard.......2.........3.........4.........5.........6.........7.........
        progress 25000
        progress 50000
        progress 75000
        progress 100000
+       stop
        EOF
-       test-tool progress --total=100000 \
-               "Working hard.......2.........3.........4.........5.........6.........7........." \
-               <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -164,12 +169,14 @@ test_expect_success 'progress shortens - crazy caller' '
        EOF
 
        cat >in <<-\EOF &&
+       start 1000
        progress 100
        progress 200
        progress 1
        progress 1000
+       stop
        EOF
-       test-tool progress --total=1000 "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -185,6 +192,7 @@ test_expect_success 'progress display with throughput' '
        EOF
 
        cat >in <<-\EOF &&
+       start 0
        throughput 102400 1000
        update
        progress 10
@@ -197,8 +205,9 @@ test_expect_success 'progress display with throughput' '
        throughput 409600 4000
        update
        progress 40
+       stop
        EOF
-       test-tool progress "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -214,6 +223,7 @@ test_expect_success 'progress display with throughput and total' '
        EOF
 
        cat >in <<-\EOF &&
+       start 40
        throughput 102400 1000
        progress 10
        throughput 204800 2000
@@ -222,8 +232,9 @@ test_expect_success 'progress display with throughput and total' '
        progress 30
        throughput 409600 4000
        progress 40
+       stop
        EOF
-       test-tool progress --total=40 "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -239,6 +250,7 @@ test_expect_success 'cover up after throughput shortens' '
        EOF
 
        cat >in <<-\EOF &&
+       start 0
        throughput 409600 1000
        update
        progress 1
@@ -251,8 +263,9 @@ test_expect_success 'cover up after throughput shortens' '
        throughput 1638400 4000
        update
        progress 4
+       stop
        EOF
-       test-tool progress "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >out &&
        test_cmp expect out
@@ -267,6 +280,7 @@ test_expect_success 'cover up after throughput shortens a lot' '
        EOF
 
        cat >in <<-\EOF &&
+       start 0
        throughput 1 1000
        update
        progress 1
@@ -276,8 +290,9 @@ test_expect_success 'cover up after throughput shortens a lot' '
        throughput 3145728 3000
        update
        progress 3
+       stop
        EOF
-       test-tool progress "Working hard" <in 2>stderr &&
+       test-tool progress <in 2>stderr &&
 
        show_cr <stderr >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' '
        cat >in <<-\EOF &&
+       start 40
        throughput 102400 1000
        update
        progress 10
@@ -297,10 +313,11 @@ test_expect_success 'progress generates traces' '
        throughput 409600 4000
        update
        progress 40
+       stop
        EOF
 
-       GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress --total=40 \
-               "Working hard" <in 2>stderr &&
+       GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress \
+               <in 2>stderr &&
 
        # t0212/parse_events.perl intentionally omits regions and data.
        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
 '
 
+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

diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
index df524f7b6dde1fa63d268bb88730dc766a003f4d..e9045009a1105d09caaea43b830548f77707b096 100755 (executable)
--- a/t/t5316-pack-delta-depth.sh
+++ b/t/t5316-pack-delta-depth.sh
@@ -64,7 +64,11 @@ test_expect_success 'create series of packs' '
                        echo $cur &&
                        echo "$(git rev-parse :file) file"
                } | 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
        done
 '