Various test, leak and other fixes for the progress.c code and its tests. This v8 addresses feedback on v7[1] by Johannes Altmanninger. For that round I accidentally broke the In-Reply-To chain, so I'm replying to the v6 here to attach it to the original thread again. 1. https://lore.kernel.org/git/cover-v7-0.7-00000000000-20211217T041945Z-avarab@xxxxxxxxx/ Ævar Arnfjörð Bjarmason (7): leak tests: fix a memory leak 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() *.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 | 14 +++--- t/helper/test-progress.c | 52 +++++++++++++++----- t/t0500-progress-display.sh | 94 ++++++++++++++++++++++++++++--------- 7 files changed, 126 insertions(+), 46 deletions(-) Range-diff against v7: 1: 5367293ee84 ! 1: aa08dab654d leak tests: fix a memory leaks in "test-progress" helper @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - leak tests: fix a memory leaks in "test-progress" helper + leak tests: fix a memory leak in "test-progress" helper Fix a memory leak in the test-progress helper, and mark the corresponding "t0500-progress-display.sh" test as being leak-free 2: 81788101763 = 2: 3ecdab074b6 progress.c test helper: add missing braces 3: d685c248686 ! 3: 271f6d7ec3b progress.c tests: make start/stop commands on stdin @@ 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) { @@ 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') -+ progress = start_progress(default_title, total); ++ const char *title; ++ const char *str; ++ ++ /* ++ * 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 == ' ') -+ progress = start_progress(dup_title(&titles, -+ end + 1), -+ total); ++ title = string_list_insert(&titles, end + 1)->string; + else + die("invalid input: '%s'\n", line.buf); ++ ++ str = title ? title : default_title; ++ progress = start_progress(str, total); + } else if (skip_prefix(line.buf, "progress ", (const char **) &end)) { uint64_t item_count = strtoull(end, &end, 10); if (*end != '\0') 4: 40e446da277 = 4: 7c1b8b287c5 progress.c tests: test some invalid usage 5: c2303bfd130 ! 5: 72a31bd7191 progress.c: add temporary variable from progress struct @@ Metadata ## Commit message ## progress.c: add temporary variable from progress struct - 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. + 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. + + Now instead of using "*p_progress" several times in stop_progress() we + check it once for NULL and then use a dereferenced "progress" variable + thereafter. This continues the same pattern used in the above + stop_progress() function, see ac900fddb7f (progress: don't dereference + before checking for NULL, 2020-08-10). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## progress.c ## -@@ progress.c: void stop_progress(struct progress **p_progress) - finish_if_sparse(*p_progress); +@@ progress.c: static void finish_if_sparse(struct progress *progress) + + void stop_progress(struct progress **p_progress) + { ++ struct progress *progress; + if (!p_progress) + BUG("don't provide NULL to stop_progress"); ++ progress = *p_progress; + +- finish_if_sparse(*p_progress); ++ finish_if_sparse(progress); - if (*p_progress) { -+ struct progress *progress = *p_progress; +- if (*p_progress) { ++ if (progress) { trace2_data_intmax("progress", the_repository, "total_objects", - (*p_progress)->total); +- (*p_progress)->total); ++ progress->total); - if ((*p_progress)->throughput) +- if ((*p_progress)->throughput) ++ if (progress->throughput) trace2_data_intmax("progress", the_repository, "total_bytes", - (*p_progress)->throughput->curr_total); 6: 776362de897 ! 6: 0bd08e1b018 pack-bitmap-write.c: don't return without stop_progress() @@ Commit message 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. + defer calling start_progress() until we need it. For now this only + matters in practice because we'd previously omit the "region_leave" + for the progress trace2 event. Suggested-by: SZEDER Gábor <szeder.dev@xxxxxxxxx> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 7: 0670d1aa5f2 ! 7: 060483fb5ce various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) + *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase, - and around 10 "isatty(0)", but these used the + and around 10 "isatty(0)", but three callers 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. + 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]. 1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@xxxxxxxxx/ -- 2.34.1.1257.g2af47340c7b
