[PATCH v7 0/7] progress: test fixes / cleanup

[PATCH v7 0/7] progress: test fixes / cleanup

[Ævar Arnfjörð Bjarmason]

A series that used to be about adding a "global progress" state, but
started with some cleanups & adding the ability to test that mode. See
[1] for the v6 and [2] for a discussion on the previous 8/8 patch.

This v7 ejects that 8/8 patch. I think it's safe to merge down, but
per [3] etc. this topic has been stalled for a while in "needs
review". I think it's had enough thorough review. The sticking point
has been me & SZEDER disagreeing on whether adding that BUG() in the
previous 8/8 can be deemed safe or not.

So let's bypass that for now and hopefully queue these up. Once this
lands we can re-visit that 8/8 and various other progress.c API & UX
improvements I have unsubmitted, which have been blocked by this
topic.

1. https://lore.kernel.org/git/cover-v6-0.8-00000000000-20211102T122507Z-avarab@gmail.com/
2. https://lore.kernel.org/git/211203.868rx2t0hv.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/211115.86wnl9qkfz.gmgdl@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (7):
  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(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)

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

Range-diff against v6:
1:  88d89652831 = 1:  5367293ee84 leak tests: fix a memory leaks in "test-progress" helper
2:  2fa92e7db83 = 2:  81788101763 progress.c test helper: add missing braces
3:  400b491449e = 3:  d685c248686 progress.c tests: make start/stop commands on stdin
4:  7557975a122 = 4:  40e446da277 progress.c tests: test some invalid usage
5:  61c8da31aeb = 5:  c2303bfd130 progress.c: add temporary variable from progress struct
6:  f6a76b80e91 = 6:  776362de897 pack-bitmap-write.c: don't return without stop_progress()
7:  1a2eadf28d0 ! 7:  0670d1aa5f2 various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
    @@ Commit message
         {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
         them.
     
    -    Let's change these for consistency, and because a subsequent commit's
    -    commit message outlines a recipe to change all of these for ad-hoc
    -    testing, not needing to match these with that ad-hoc regex will make
    -    things easier to explain.
    +    Let's change these for consistency, and because another commit that
    +    would like to be based on top of this one[1] has a recipe to change
    +    all of these for ad-hoc testing, not needing to match these with that
    +    ad-hoc regex will make things easier to explain. Only one of these is
    +    related to the "struct progress" code which it discusses, but let's
    +    change all of these while we're at it.
    +
    +    1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
8:  bff919994b5 < -:  ----------- progress.c: add & assert a "global_progress" variable
-- 
2.34.1.1119.g7a3fc8778ee

[PATCH v7 1/7] leak tests: fix a memory leaks in "test-progress" helper

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in the test-progress helper, and mark the
corresponding "t0500-progress-display.sh" test as being leak-free
under SANITIZE=leak. This fixes a leak added in 2bb74b53a4 (Test the
progress display, 2019-09-16).

My 48f68715b14 (tr2: stop leaking "thread_name" memory, 2021-08-27)
had fixed another memory leak in this test (as it did some trace2
testing).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-progress.c    | 1 +
 t/t0500-progress-display.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 5d05cbe7894..9265e6ab7cf 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -69,6 +69,7 @@ int cmd__progress(int argc, const char **argv)
 			die("invalid input: '%s'\n", line.buf);
 	}
 	stop_progress(&progress);
+	strbuf_release(&line);
 
 	return 0;
 }
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 22058b503ac..f37cf2eb9c9 100755
--- 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 () {
-- 
2.34.1.1119.g7a3fc8778ee

Re: [PATCH v7 1/7] leak tests: fix a memory leaks in "test-progress" helper

[Johannes Altmanninger]

looks good; maybe change the subject: "memory leaks" -> "memory leak"

[PATCH v7 2/7] progress.c test helper: add missing braces

[Ævar Arnfjörð Bjarmason]

If we have braces on one arm of an if/else all of them should have it,
per the CodingGuidelines's "When there are multiple arms to a
conditional[...]" advice. This formatting change makes a subsequent
commit smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-progress.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 9265e6ab7cf..50fd3be3dad 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -63,10 +63,11 @@ 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 {
 			die("invalid input: '%s'\n", line.buf);
+		}
 	}
 	stop_progress(&progress);
 	strbuf_release(&line);
-- 
2.34.1.1119.g7a3fc8778ee

[PATCH v7 3/7] progress.c tests: make start/stop commands on stdin

[Ævar Arnfjörð Bjarmason]

Change the usage of the "test-tool progress" introduced in
2bb74b53a49 (Test the progress display, 2019-09-16) to take command
like "start" and "stop" on stdin, instead of running them implicitly.

This makes for tests that are easier to read, since the recipe will
mirror the API usage, and allows for easily testing invalid usage that
would yield (or should yield) a BUG(), e.g. providing two "start"
calls in a row. A subsequent commit will add such tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-progress.c    | 46 ++++++++++++++++++++++-------
 t/t0500-progress-display.sh | 58 +++++++++++++++++++++++--------------
 2 files changed, 72 insertions(+), 32 deletions(-)

diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 50fd3be3dad..1435c28e950 100644
--- 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,52 @@
 #include "parse-options.h"
 #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 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);
+			if (*end == '\0')
+				progress = start_progress(default_title, total);
+			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')
 				die("invalid input: '%s'\n", line.buf);
@@ -65,12 +87,14 @@ int cmd__progress(int argc, const char **argv)
 			display_throughput(progress, byte_count);
 		} else if (!strcmp(line.buf, "update")) {
 			progress_test_force_update();
+		} 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 f37cf2eb9c9..27ab4218b01 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -18,6 +18,7 @@ test_expect_success 'simple progress display' '
 	EOF
 
 	cat >in <<-\EOF &&
+	start 0
 	update
 	progress 1
 	update
@@ -26,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
@@ -42,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
@@ -63,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
@@ -89,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
@@ -117,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
@@ -141,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
@@ -165,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
@@ -186,6 +192,7 @@ test_expect_success 'progress display with throughput' '
 	EOF
 
 	cat >in <<-\EOF &&
+	start 0
 	throughput 102400 1000
 	update
 	progress 10
@@ -198,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
@@ -215,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
@@ -223,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
@@ -240,6 +250,7 @@ test_expect_success 'cover up after throughput shortens' '
 	EOF
 
 	cat >in <<-\EOF &&
+	start 0
 	throughput 409600 1000
 	update
 	progress 1
@@ -252,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
@@ -268,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
@@ -277,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
@@ -286,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
@@ -298,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 &&
-- 
2.34.1.1119.g7a3fc8778ee

Re: [PATCH v7 3/7] progress.c tests: make start/stop commands on stdin

[Johannes Altmanninger]

On Fri, Dec 17, 2021 at 05:24:58AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Change the usage of the "test-tool progress" introduced in
> 2bb74b53a49 (Test the progress display, 2019-09-16) to take command
> like "start" and "stop" on stdin, instead of running them implicitly.
> 
> This makes for tests that are easier to read, since the recipe will
> mirror the API usage, and allows for easily testing invalid usage that

(Of course invalid API usage wasn't really a problem before, but it's good
that you mention the upcoming tests, to calm any concerns)

> would yield (or should yield) a BUG(), e.g. providing two "start"
> calls in a row. A subsequent commit will add such tests.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/helper/test-progress.c    | 46 ++++++++++++++++++++++-------
>  t/t0500-progress-display.sh | 58 +++++++++++++++++++++++--------------
>  2 files changed, 72 insertions(+), 32 deletions(-)
> 
> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
> index 50fd3be3dad..1435c28e950 100644
> --- 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,52 @@
>  #include "parse-options.h"
>  #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;
> +}

It seems weird to reference someone else's local variables in "end + 1" here.
How about inlining this function instead?

			} else if (*end == ' ') {
				char *title_duped = string_list_insert(&titles, end + 1)->string;
				progress = start_progress(title_duped, total);
			} else {

and maybe add a comment there, but I'm not sure if we still need it.

>  
>  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

(unrelated: I'd always add a trailing comma if I can, even though in this case it won't ever matter)

>  	};
>  	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);
> +			if (*end == '\0')
> +				progress = start_progress(default_title, total);
> +			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')
>  				die("invalid input: '%s'\n", line.buf);
> @@ -65,12 +87,14 @@ int cmd__progress(int argc, const char **argv)
>  			display_throughput(progress, byte_count);
>  		} else if (!strcmp(line.buf, "update")) {
>  			progress_test_force_update();
> +		} 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 f37cf2eb9c9..27ab4218b01 100755
> --- a/t/t0500-progress-display.sh
> +++ b/t/t0500-progress-display.sh
> @@ -18,6 +18,7 @@ test_expect_success 'simple progress display' '
>  	EOF
>  
>  	cat >in <<-\EOF &&
> +	start 0
>  	update
>  	progress 1
>  	update
> @@ -26,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
> @@ -42,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
> @@ -63,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" \

I don't know enough about progress tests to judge if this is better.
The start invocation does look nicer, but it might feel weird to always
include "stop". We could do that automatically but then we're no longer
mirroring the API...

> -		<in 2>stderr &&
> +	test-tool progress <in 2>stderr &&

Re: [PATCH v7 3/7] progress.c tests: make start/stop commands on stdin

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 27 2021, Johannes Altmanninger wrote:

[Will reply to the rest later, thanks for the review...]

> On Fri, Dec 17, 2021 at 05:24:58AM +0100, Ævar Arnfjörð Bjarmason wrote:
> [...]
>>  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
>
> (unrelated: I'd always add a trailing comma if I can, even though in this case it won't ever matter)

FWIW this bit is intentional coding style in git.git, see Junio's
https://lore.kernel.org/git/xmqqk0g5656r.fsf@gitster.g/:
    
    It is a good idea to leave a comma even after the last element,
    _unless_ there is a strong reason why the element that currently is
    at the last MUST stay to be last when new elements are added[...]

Well, in that case he's talking about enums, but the same applies even
more to these sorts of lists here the NULL must remain the last element.
    

[PATCH v7 4/7] progress.c tests: test some invalid usage

[Ævar Arnfjörð Bjarmason]

Test what happens when we "stop" without a "start", omit the "stop"
after a "start", or try to start two concurrent progress bars. This
extends the trace2 tests added in 98a13647408 (trace2: log progress
time and throughput, 2020-05-12).

These tests are not merely testing the helper, but invalid API usage
that can happen if the progress.c API is misused.

The "without stop" test will leak under SANITIZE=leak, since this
buggy use of the API will leak memory. But let's not skip it entirely,
or use the "!SANITIZE_LEAK" prerequisite check as we'd do with tests
that we're skipping due to leaks we haven't fixed yet. Instead
annotate the specific command that should skip leak checking with
custom $LSAN_OPTIONS[1].

1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0500-progress-display.sh | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 27ab4218b01..59e9f226ea4 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -325,4 +325,39 @@ 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_done
-- 
2.34.1.1119.g7a3fc8778ee

Re: [PATCH v7 4/7] progress.c tests: test some invalid usage

[Johannes Altmanninger]

On Fri, Dec 17, 2021 at 05:24:59AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Test what happens when we "stop" without a "start", omit the "stop"
> after a "start", or try to start two concurrent progress bars.

I think the concurrent case is missing (I guess that's not blocking)

> This extends the trace2 tests added in 98a13647408 (trace2: log progress
> time and throughput, 2020-05-12).
> 
> These tests are not merely testing the helper, but invalid API usage
> that can happen if the progress.c API is misused.
> 
> The "without stop" test will leak under SANITIZE=leak, since this
> buggy use of the API will leak memory. But let's not skip it entirely,
> or use the "!SANITIZE_LEAK" prerequisite check as we'd do with tests
> that we're skipping due to leaks we haven't fixed yet. Instead
> annotate the specific command that should skip leak checking with
> custom $LSAN_OPTIONS[1].
> 
> 1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t0500-progress-display.sh | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
> index 27ab4218b01..59e9f226ea4 100755
> --- a/t/t0500-progress-display.sh
> +++ b/t/t0500-progress-display.sh
> @@ -325,4 +325,39 @@ 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 &&

nit: I personally prefer always indenting after continuation lines, like

	GIT_TRACE2_EVENT="$(pwd)/trace-start.event" \
		LSAN_OPTIONS=detect_leaks=0 \
		test-tool progress \
		<in 2>stderr &&

but we don't have a uniform style, so it doesn't really matter.

> +	grep region_enter.*progress trace-start.event &&
> +	! grep region_leave.*progress trace-start.event

should "!" be "test_must_fail"? Same below.

> +'
> +
> +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_done
> -- 
> 2.34.1.1119.g7a3fc8778ee
> 

Re: [PATCH v7 4/7] progress.c tests: test some invalid usage

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

>> +	grep region_enter.*progress trace-start.event &&
>> +	! grep region_leave.*progress trace-start.event
>
> should "!" be "test_must_fail"? Same below.

The answer is no.  The main difference between test_must_fail and
"!" is that the former is designed to catch ungraceful death (e.g.
by segfault) and to be used to expect controlled failure from OUR
programs (i.e. "git" and "test-tool" invocations).  We are not in
the business of catching segfaulting bug in system's "grep", so it
is perfectly fine to use "!" here.

[PATCH v7 5/7] progress.c: add temporary variable from progress struct

[Ævar Arnfjörð Bjarmason]

Add a temporary "progress" variable for the dereferenced p_progress
pointer to a "struct progress *". Before 98a13647408 (trace2: log
progress time and throughput, 2020-05-12) we didn't dereference
"p_progress" in this function, now that we do it's easier to read the
code if we work with a "progress" struct pointer like everywhere else,
instead of a pointer to a pointer.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 progress.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/progress.c b/progress.c
index 680c6a8bf93..76a95cb7322 100644
--- a/progress.c
+++ b/progress.c
@@ -325,15 +325,16 @@ void stop_progress(struct progress **p_progress)
 	finish_if_sparse(*p_progress);
 
 	if (*p_progress) {
+		struct progress *progress = *p_progress;
 		trace2_data_intmax("progress", the_repository, "total_objects",
 				   (*p_progress)->total);
 
 		if ((*p_progress)->throughput)
 			trace2_data_intmax("progress", the_repository,
 					   "total_bytes",
-					   (*p_progress)->throughput->curr_total);
+					   progress->throughput->curr_total);
 
-		trace2_region_leave("progress", (*p_progress)->title, the_repository);
+		trace2_region_leave("progress", progress->title, the_repository);
 	}
 
 	stop_progress_msg(p_progress, _("done"));
-- 
2.34.1.1119.g7a3fc8778ee

Re: [PATCH v7 5/7] progress.c: add temporary variable from progress struct

[Johannes Altmanninger]

On Fri, Dec 17, 2021 at 05:25:00AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Add a temporary "progress" variable for the dereferenced p_progress
> pointer to a "struct progress *". Before 98a13647408 (trace2: log
> progress time and throughput, 2020-05-12) we didn't dereference
> "p_progress" in this function, now that we do it's easier to read the
> code if we work with a "progress" struct pointer like everywhere else,
> instead of a pointer to a pointer.

This message contains lots of nice details, but some of that is only really
useful after reading the diff. You mention the parameter's name but not the
function name (other combinations would make more sense).
I'd maybe write it like this, trying to follow our usual cadence of
diagnosis -> action:

	Since 98a13647408 (trace2: log progress time and throughput, 2020-05-12)
	stop_progress() dereferences a "struct progress **" parameter in several
	places. Extract a dereferenced variable (like in stop_progress_msg()) to
	reduce clutter and make it clearer who needs to write to this parameter.

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  progress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/progress.c b/progress.c
> index 680c6a8bf93..76a95cb7322 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -325,15 +325,16 @@ void stop_progress(struct progress **p_progress)
>  	finish_if_sparse(*p_progress);
>  
>  	if (*p_progress) {
> +		struct progress *progress = *p_progress;

Yeah, it's a good idea to reduce uses of the struct progress ** parameter.
Why not do this even earlier in this function:

	struct progress *progress;

	if (!p_progress)
		BUG("don't provide NULL to stop_progress");
	progress = *p_progress;

	finish_if_sparse(progress);

	if (progress) {
		...
	}

	stop_progress_msg(p_progress, _("done"));

this way we only need to dereference once (right next to the null check)
and it's clearer that stop_progress_msg() is the only one who wants to modify
the parameter.

>  		trace2_data_intmax("progress", the_repository, "total_objects",
>  				   (*p_progress)->total);

s/(\*p_progress)/progress/, same in the next line

>  
>  		if ((*p_progress)->throughput)
>  			trace2_data_intmax("progress", the_repository,
>  					   "total_bytes",
> -					   (*p_progress)->throughput->curr_total);
> +					   progress->throughput->curr_total);
>  
> -		trace2_region_leave("progress", (*p_progress)->title, the_repository);
> +		trace2_region_leave("progress", progress->title, the_repository);
>  	}
>  
>  	stop_progress_msg(p_progress, _("done"));
> -- 
> 2.34.1.1119.g7a3fc8778ee
> 

[PATCH v7 6/7] pack-bitmap-write.c: don't return without stop_progress()

[Ævar Arnfjörð Bjarmason]

Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.

We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it.

This will matter in a subsequent commit where we BUG(...) out if this
happens, and matters now e.g. because we don't have a corresponding
"region_end" for the progress trace2 event.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pack-bitmap-write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e1..cab3eaa2acd 100644
--- 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;
 
-- 
2.34.1.1119.g7a3fc8778ee

Re: [PATCH v7 6/7] pack-bitmap-write.c: don't return without stop_progress()

[Johannes Altmanninger]

On Fri, Dec 17, 2021 at 05:25:01AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
> bitmap writing, 2013-12-21), we did not call stop_progress() if we
> reached the early exit in this function.
> 
> We could call stop_progress() before we return, but better yet is to
> defer calling start_progress() until we need it.

Looks great.

> 
> This will matter in a subsequent commit where we BUG(...) out if this happens,

"this" = "call start without stop"? Let's maybe be explicit here

(I guess the BUG() commit was postponed, but it doesn't hurt to mention it.)

> and matters now e.g. because we don't have a corresponding
> "region_end" for the progress trace2 event.

s/region_end/region_leave/

> 
> Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  pack-bitmap-write.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 9c55c1531e1..cab3eaa2acd 100644
> --- 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;
>  
> -- 
> 2.34.1.1119.g7a3fc8778ee
> 

[PATCH v7 7/7] various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)

[Ævar Arnfjörð Bjarmason]

We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
and around 10 "isatty(0)", but these used the
{STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
them.

Let's change these for consistency, and because another commit that
would like to be based on top of this one[1] has a recipe to change
all of these for ad-hoc testing, not needing to match these with that
ad-hoc regex will make things easier to explain. Only one of these is
related to the "struct progress" code which it discusses, but let's
change all of these while we're at it.

1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect--helper.c | 2 +-
 builtin/bundle.c         | 2 +-
 compat/mingw.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28a2e6a5750..21360a4e70b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)
 	fprintf_ln(stderr, _("You need to start by \"git bisect "
 			  "start\"\n"));
 
-	if (!isatty(STDIN_FILENO))
+	if (!isatty(0))
 		return -1;
 
 	/*
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 5a85d7cd0fe..df69c651753 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,
 
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	int all_progress_implied = 0;
-	int progress = isatty(STDERR_FILENO);
+	int progress = isatty(2);
 	struct strvec pack_opts;
 	int version = -1;
 	int ret;
diff --git a/compat/mingw.c b/compat/mingw.c
index e14f2d5f77c..7c55d0f0414 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2376,7 +2376,7 @@ int mingw_raise(int sig)
 	switch (sig) {
 	case SIGALRM:
 		if (timer_fn == SIG_DFL) {
-			if (isatty(STDERR_FILENO))
+			if (isatty(2))
 				fputs("Alarm clock\n", stderr);
 			exit(128 + SIGALRM);
 		} else if (timer_fn != SIG_IGN)
-- 
2.34.1.1119.g7a3fc8778ee

Re: [PATCH v7 7/7] various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)

[Johannes Altmanninger]

nit: I'd maybe drop the "various *.c: " subject prefix. It's already
implied that the change is across the entire tree.

On Fri, Dec 17, 2021 at 05:25:02AM +0100, Ævar Arnfjörð Bjarmason wrote:
> We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
> and around 10 "isatty(0)", but these used the

nit: ambiguous pronouns like "these" here can trip up the reader for a
second. Maybe "three places" is clearer.

> {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
> them.
> 
> Let's change these for consistency, and because another commit that
> would like to be based on top of this one[1] has a recipe to change
> all of these for ad-hoc testing, not needing to match these with that
> ad-hoc regex will make things easier to explain.

this sentence is quite long, and it also doesn't help that "these" refers
to something other than it did in the previous sentence.
Here's my attempt:

	Let's change these for consistency.  This makes it easier to change all
	calls to isatty() at a whim, which is useful to test some scenarios[1].

> Only one of these is
> related to the "struct progress" code which it discusses, but let's
> change all of these while we're at it.
> 
> 1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/bisect--helper.c | 2 +-
>  builtin/bundle.c         | 2 +-
>  compat/mingw.c           | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 28a2e6a5750..21360a4e70b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	fprintf_ln(stderr, _("You need to start by \"git bisect "
>  			  "start\"\n"));
>  
> -	if (!isatty(STDIN_FILENO))
> +	if (!isatty(0))
>  		return -1;
>  
>  	/*
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 5a85d7cd0fe..df69c651753 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,
>  
>  static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  	int all_progress_implied = 0;
> -	int progress = isatty(STDERR_FILENO);
> +	int progress = isatty(2);
>  	struct strvec pack_opts;
>  	int version = -1;
>  	int ret;
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e14f2d5f77c..7c55d0f0414 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2376,7 +2376,7 @@ int mingw_raise(int sig)
>  	switch (sig) {
>  	case SIGALRM:
>  		if (timer_fn == SIG_DFL) {
> -			if (isatty(STDERR_FILENO))
> +			if (isatty(2))
>  				fputs("Alarm clock\n", stderr);
>  			exit(128 + SIGALRM);
>  		} else if (timer_fn != SIG_IGN)
> -- 
> 2.34.1.1119.g7a3fc8778ee
>