[v4,0/8] progress: assert "global_progress" + test fixes / cleanup

Message

Ævar Arnfjörð Bjarmason Oct. 25, 2021, 11:24 a.m. UTC

This series fixes various issues in and related to progress.c, and
adds a BUG() assertion for us not starting two progress bars at the
same time. Those changes are needed for subsequent changes that do
more interesting things with this new global progress bar.

I think this should address all the feedback on v3[1]. Changes:

 * Clarified the whole string_list_insert() in test-progress.c. I
   added a wrapper function with a comment to make that code flow
   clearer, and as a bonus could drop braces and a temporary variable
   from the main function as a result.

 * I elaborated on the "limitations in the progress output" part of
   the commit message, and tried to address the safety concerns SZEDER
   expressed in [2]. I think we might still disagree on whether 8/8 is
   worth it overall, but hopefully this addreses any outstanding
   questions about the approach & why it's being taken.

 * SZEDER pointed out that I should move the BUG() assert out of the
   signal handlers, there's now a couple of small helpers to do that,
   which also allowed for ejecting a couple of earlier refactoring
   commits (code moves as they were "static").

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason (8):
  leak tests: fix a memory leaks in "test-progress" helper
  progress.c test helper: add missing braces
  progress.c tests: make start/stop commands on stdin
  progress.c tests: test some invalid usage
  progress.c: add temporary variable from progress struct
  pack-bitmap-write.c: don't return without stop_progress()
  various *.c: use isatty(1|2), not isatty(STDIN_FILENO|STDERR_FILENO)
  progress.c: add & assert a "global_progress" variable

 builtin/bisect--helper.c    |   2 +-
 builtin/bundle.c            |   2 +-
 compat/mingw.c              |   2 +-
 pack-bitmap-write.c         |   6 +--
 progress.c                  |  23 +++++++-
 t/helper/test-progress.c    |  52 +++++++++++++-----
 t/t0500-progress-display.sh | 105 ++++++++++++++++++++++++++++--------
 7 files changed, 150 insertions(+), 42 deletions(-)

Range-diff against v3:
 1:  40f7c438a1e =  1:  a3bd032d1eb leak tests: fix a memory leaks in "test-progress" helper
 2:  ee177d253a8 =  2:  e441cfea7c5 progress.c test helper: add missing braces
 3:  045d58d8201 !  3:  1c5f9bdfe6d progress.c tests: make start/stop verbs on stdin
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    progress.c tests: make start/stop verbs on stdin
    +    progress.c tests: make start/stop commands on stdin
     
         Change the usage of the "test-tool progress" introduced in
         2bb74b53a49 (Test the progress display, 2019-09-16) to take command
    @@ t/helper/test-progress.c
      #include "progress.h"
      #include "strbuf.h"
     +#include "string-list.h"
    ++
    ++/*
    ++ * We can't use "end + 1" as an argument to start_progress() below, it
    ++ * doesn't xstrdup() its "title" argument. We need to hold onto a
    ++ * valid "char *" for it until the end.
    ++ */
    ++static char *dup_title(struct string_list *titles, const char *title)
    ++{
    ++	return string_list_insert(titles, title)->string;
    ++}
      
      int cmd__progress(int argc, const char **argv)
      {
     -	int total = 0;
     -	const char *title;
     +	const char *const default_title = "Working hard";
    -+	struct string_list list = STRING_LIST_INIT_DUP;
    -+	const struct string_list_item *item;
    ++	struct string_list titles = STRING_LIST_INIT_DUP;
      	struct strbuf line = STRBUF_INIT;
     -	struct progress *progress;
     +	struct progress *progress = NULL;
    @@ t/helper/test-progress.c
     -		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);
    -+			if (*end == '\0') {
    ++			if (*end == '\0')
     +				progress = start_progress(default_title, total);
    -+			} else if (*end == ' ') {
    -+				item = string_list_insert(&list, end + 1);
    -+				progress = start_progress(item->string, total);
    -+			} else {
    ++			else if (*end == ' ')
    ++				progress = start_progress(dup_title(&titles,
    ++								    end + 1),
    ++							  total);
    ++			else
     +				die("invalid input: '%s'\n", line.buf);
    -+			}
     +		} else if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
      			uint64_t item_count = strtoull(end, &end, 10);
      			if (*end != '\0')
    @@ t/helper/test-progress.c: int cmd__progress(int argc, const char **argv)
      	}
     -	stop_progress(&progress);
      	strbuf_release(&line);
    -+	string_list_clear(&list, 0);
    ++	string_list_clear(&titles, 0);
      
      	return 0;
      }
 4:  efc0ec360cc =  4:  474ce31f9d2 progress.c tests: test some invalid usage
 5:  9e36f03de46 <  -:  ----------- progress.c: move signal handler functions lower
 6:  c7c3843564e <  -:  ----------- progress.c: call progress_interval() from progress_test_force_update()
 7:  cd2d27b1626 =  5:  ff039742148 progress.c: add temporary variable from progress struct
 8:  e0a3510dd88 =  6:  3dfe31decff pack-bitmap-write.c: don't return without stop_progress()
 9:  2cf14881ecf =  7:  8a18eb40fae various *.c: use isatty(1|2), not isatty(STDIN_FILENO|STDERR_FILENO)
10:  01d5bbfce76 !  8:  06124e8ac5e progress.c: add & assert a "global_progress" variable
    @@ Commit message
         limitations in the progress output: The current output must be
         "driven" by calls to the likes of display_progress(). Once we have a
         global current progress object we'll be able to update that object via
    -    SIGALRM. See [3] for early code to do that.
    +    SIGALRM, this will cover cases where we're busy, but either haven't
    +    invoked our first display_progress() yet, or the time between
    +    display_progress() is too long. See [3] for early code to do that.
     
         It's conceivable that this change will hit the BUG() condition in some
         scenario that we don't currently have tests for, this would be very
    @@ Commit message
             above, and the tests that are testing that the progress output
             itself is present (but for testing I'd made display() a NOOP).
     
    -    Between those three points I think it's safe to go ahead with this
    +    This doesn't address any currently out-of-tree user of progress.c,
    +    i.e. WIP patches, or progress output that's a part of forks of
    +    git.git. Those hopefully have test coverage that would expose the
    +    BUG().
    +
    +    If they don't they'll either run into it in code that displays more
    +    than one progress bar for the lifetime of the progress, or which calls
    +    stop_progress() with a non-NULL "progress" without a corresponding
    +    start_progress(). Both of those cases are less likely than the general
    +    cases of progress.c API misuse.
    +
    +    Between those three points above and the discussion of how this could
    +    impact out-of-tree users I think it's safe to go ahead with this
         change.
     
         1. 6f9d5f2fda1 (commit-graph: fix progress of reachable commits, 2020-07-09)
    @@ progress.c: struct progress {
      
      /*
       * These are only intended for testing the progress output, i.e. exclusively
    -@@ progress.c: void progress_test_force_update(void)
    - 	progress_interval(SIGALRM);
    +@@ progress.c: void display_progress(struct progress *progress, uint64_t n)
    + 		display(progress, n, NULL);
      }
      
    --static void set_progress_signal(void)
    -+static void set_progress_signal(struct progress *progress)
    - {
    - 	struct sigaction sa;
    - 	struct itimerval v;
    - 
    ++static void set_global_progress(struct progress *progress)
    ++{
     +	if (global_progress)
     +		BUG("'%s' progress still active when trying to start '%s'",
     +		    global_progress->title, progress->title);
     +	global_progress = progress;
    ++}
     +
    - 	if (progress_testing)
    - 		return;
    - 
    -@@ progress.c: static void set_progress_signal(void)
    - 	setitimer(ITIMER_REAL, &v, NULL);
    - }
    - 
    --static void clear_progress_signal(void)
    -+static void clear_progress_signal(struct progress *progress)
    + static struct progress *start_progress_delay(const char *title, uint64_t total,
    + 					     unsigned delay, unsigned sparse)
      {
    - 	struct itimerval v = {{0,},};
    - 
    -+	if (!global_progress)
    -+		BUG("should have active global_progress when cleaning up");
    -+	global_progress = NULL;
    -+
    - 	if (progress_testing)
    - 		return;
    - 
     @@ progress.c: static struct progress *start_progress_delay(const char *title, uint64_t total,
      	strbuf_init(&progress->counters_sb, 0);
      	progress->title_len = utf8_strwidth(title);
      	progress->split = 0;
    --	set_progress_signal();
    -+	set_progress_signal(progress);
    ++	set_global_progress(progress);
    + 	set_progress_signal();
      	trace2_region_enter("progress", title, the_repository);
      	return progress;
    +@@ progress.c: void stop_progress(struct progress **p_progress)
    + 	stop_progress_msg(p_progress, _("done"));
      }
    + 
    ++static void unset_global_progress(void)
    ++{
    ++	if (!global_progress)
    ++		BUG("should have active global_progress when cleaning up");
    ++	global_progress = NULL;
    ++}
    ++
    + void stop_progress_msg(struct progress **p_progress, const char *msg)
    + {
    + 	struct progress *progress;
     @@ progress.c: void stop_progress_msg(struct progress **p_progress, const char *msg)
    - 		display(progress, progress->last_value, buf);
      		free(buf);
      	}
    --	clear_progress_signal();
    -+	clear_progress_signal(progress);
    + 	clear_progress_signal();
    ++	unset_global_progress();
      	strbuf_release(&progress->counters_sb);
      	if (progress->throughput)
      		strbuf_release(&progress->throughput->display);