From d5e4aef3586c07c31e3e4d76ce7fdf0f9843314f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Sat, 6 Dec 2025 12:47:32 +0100 Subject: [PATCH 01/13] t/unit-tests: update clar to 39f11fe Update clar to commit 39f11fe (Merge pull request #131 from pks-gitlab/pks-integer-double-evaluation, 2025-12-05). This commit includes the following changes relevant to Git: - There are now typesafe integer comparison functions. Furthermore, the range of comparison functions has been included to also have relative comparisons, like "greater than". - There is a new `cl_failf()` macro that allows the caller to specify an error message with formatting directives. - The TAP format has been fixed to correctly terminate YAML blocks with "...\n" instead of "---\n". Note that we already had a `cl_failf()` function declared in our own sources. This function is equivalent to the upstreamed function, so we can simply drop it now. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/unit-tests/clar/.github/workflows/ci.yml | 2 +- t/unit-tests/clar/clar.c | 146 +++++++++++++++++- t/unit-tests/clar/clar.h | 82 +++++++++- t/unit-tests/clar/clar/print.h | 2 +- t/unit-tests/clar/test/expected/quiet | 40 ++++- .../clar/test/expected/summary_with_filename | 42 ++++- .../test/expected/summary_without_filename | 42 ++++- t/unit-tests/clar/test/expected/tap | 88 +++++++++-- .../clar/test/expected/without_arguments | 42 ++++- t/unit-tests/clar/test/selftest.c | 10 +- t/unit-tests/clar/test/suites/combined.c | 65 +++++++- t/unit-tests/unit-test.h | 6 - 12 files changed, 508 insertions(+), 59 deletions(-) diff --git a/t/unit-tests/clar/.github/workflows/ci.yml b/t/unit-tests/clar/.github/workflows/ci.yml index 4d4724222c3e89..14cb4ed1d4598a 100644 --- a/t/unit-tests/clar/.github/workflows/ci.yml +++ b/t/unit-tests/clar/.github/workflows/ci.yml @@ -53,7 +53,7 @@ jobs: if: matrix.platform.image == 'i386/debian:latest' run: apt -q update && apt -q -y install cmake gcc libc6-amd64 lib64stdc++6 make python3 - name: Check out - uses: actions/checkout@v4 + uses: actions/checkout@v6 - name: Build shell: bash run: | diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c index d6176e50b2214b..e959a5ae028b5a 100644 --- a/t/unit-tests/clar/clar.c +++ b/t/unit-tests/clar/clar.c @@ -24,6 +24,14 @@ #include #include +#ifndef va_copy +# ifdef __va_copy +# define va_copy(dst, src) __va_copy(dst, src) +# else +# define va_copy(dst, src) ((dst) = (src)) +# endif +#endif + #if defined(__UCLIBC__) && ! defined(__UCLIBC_HAS_WCHAR__) /* * uClibc can optionally be built without wchar support, in which case @@ -76,8 +84,10 @@ # define S_ISDIR(x) ((x & _S_IFDIR) != 0) # endif # define p_snprintf(buf,sz,fmt,...) _snprintf_s(buf,sz,_TRUNCATE,fmt,__VA_ARGS__) +# define p_vsnprintf _vsnprintf # else # define p_snprintf snprintf +# define p_vsnprintf vsnprintf # endif # define localtime_r(timer, buf) (localtime_s(buf, timer) == 0 ? buf : NULL) @@ -86,6 +96,7 @@ # include # define _MAIN_CC # define p_snprintf snprintf +# define p_vsnprintf vsnprintf typedef struct stat STAT_T; #endif @@ -699,13 +710,14 @@ void clar__skip(void) abort_test(); } -void clar__fail( +static void clar__failv( const char *file, const char *function, size_t line, + int should_abort, const char *error_msg, const char *description, - int should_abort) + va_list args) { struct clar_error *error; @@ -725,9 +737,19 @@ void clar__fail( error->line_number = _clar.invoke_line ? _clar.invoke_line : line; error->error_msg = error_msg; - if (description != NULL && - (error->description = strdup(description)) == NULL) - clar_abort("Failed to allocate description.\n"); + if (description != NULL) { + va_list args_copy; + int len; + + va_copy(args_copy, args); + if ((len = p_vsnprintf(NULL, 0, description, args_copy)) < 0) + clar_abort("Failed to compute description."); + va_end(args_copy); + + if ((error->description = calloc(1, len + 1)) == NULL) + clar_abort("Failed to allocate buffer."); + p_vsnprintf(error->description, len + 1, description, args); + } _clar.total_errors++; _clar.last_report->status = CL_TEST_FAILURE; @@ -736,6 +758,34 @@ void clar__fail( abort_test(); } +void clar__failf( + const char *file, + const char *function, + size_t line, + int should_abort, + const char *error_msg, + const char *description, + ...) +{ + va_list args; + va_start(args, description); + clar__failv(file, function, line, should_abort, error_msg, + description, args); + va_end(args); +} + +void clar__fail( + const char *file, + const char *function, + size_t line, + const char *error_msg, + const char *description, + int should_abort) +{ + clar__failf(file, function, line, should_abort, error_msg, + description ? "%s" : NULL, description); +} + void clar__assert( int condition, const char *file, @@ -889,6 +939,92 @@ void clar__assert_equal( clar__fail(file, function, line, err, buf, should_abort); } +void clar__assert_compare_i( + const char *file, + const char *func, + size_t line, + int should_abort, + enum clar_comparison cmp, + intmax_t value1, + intmax_t value2, + const char *error, + const char *description, + ...) +{ + int fulfilled; + switch (cmp) { + case CLAR_COMPARISON_EQ: + fulfilled = value1 == value2; + break; + case CLAR_COMPARISON_LT: + fulfilled = value1 < value2; + break; + case CLAR_COMPARISON_LE: + fulfilled = value1 <= value2; + break; + case CLAR_COMPARISON_GT: + fulfilled = value1 > value2; + break; + case CLAR_COMPARISON_GE: + fulfilled = value1 >= value2; + break; + default: + cl_assert(0); + return; + } + + if (!fulfilled) { + va_list args; + va_start(args, description); + clar__failv(file, func, line, should_abort, error, + description, args); + va_end(args); + } +} + +void clar__assert_compare_u( + const char *file, + const char *func, + size_t line, + int should_abort, + enum clar_comparison cmp, + uintmax_t value1, + uintmax_t value2, + const char *error, + const char *description, + ...) +{ + int fulfilled; + switch (cmp) { + case CLAR_COMPARISON_EQ: + fulfilled = value1 == value2; + break; + case CLAR_COMPARISON_LT: + fulfilled = value1 < value2; + break; + case CLAR_COMPARISON_LE: + fulfilled = value1 <= value2; + break; + case CLAR_COMPARISON_GT: + fulfilled = value1 > value2; + break; + case CLAR_COMPARISON_GE: + fulfilled = value1 >= value2; + break; + default: + cl_assert(0); + return; + } + + if (!fulfilled) { + va_list args; + va_start(args, description); + clar__failv(file, func, line, should_abort, error, + description, args); + va_end(args); + } +} + void cl_set_cleanup(void (*cleanup)(void *), void *opaque) { _clar.local_cleanup = cleanup; diff --git a/t/unit-tests/clar/clar.h b/t/unit-tests/clar/clar.h index ca72292ae918da..f7e43630226434 100644 --- a/t/unit-tests/clar/clar.h +++ b/t/unit-tests/clar/clar.h @@ -7,6 +7,7 @@ #ifndef __CLAR_TEST_H__ #define __CLAR_TEST_H__ +#include #include #include @@ -149,6 +150,7 @@ const char *cl_fixture_basename(const char *fixture_name); * Forced failure/warning */ #define cl_fail(desc) clar__fail(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, "Test failed.", desc, 1) +#define cl_failf(desc,...) clar__failf(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, 1, "Test failed.", desc, __VA_ARGS__) #define cl_warning(desc) clar__fail(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, "Warning during test execution:", desc, 0) #define cl_skip() clar__skip() @@ -168,9 +170,42 @@ const char *cl_fixture_basename(const char *fixture_name); #define cl_assert_equal_wcsn(wcs1,wcs2,len) clar__assert_equal(CLAR_CURRENT_FILE,CLAR_CURRENT_FUNC,CLAR_CURRENT_LINE,"String mismatch: " #wcs1 " != " #wcs2, 1, "%.*ls", (wcs1), (wcs2), (int)(len)) #define cl_assert_equal_wcsn_(wcs1,wcs2,len,note) clar__assert_equal(CLAR_CURRENT_FILE,CLAR_CURRENT_FUNC,CLAR_CURRENT_LINE,"String mismatch: " #wcs1 " != " #wcs2 " (" #note ")", 1, "%.*ls", (wcs1), (wcs2), (int)(len)) -#define cl_assert_equal_i(i1,i2) clar__assert_equal(CLAR_CURRENT_FILE,CLAR_CURRENT_FUNC,CLAR_CURRENT_LINE,#i1 " != " #i2, 1, "%d", (int)(i1), (int)(i2)) -#define cl_assert_equal_i_(i1,i2,note) clar__assert_equal(CLAR_CURRENT_FILE,CLAR_CURRENT_FUNC,CLAR_CURRENT_LINE,#i1 " != " #i2 " (" #note ")", 1, "%d", (i1), (i2)) -#define cl_assert_equal_i_fmt(i1,i2,fmt) clar__assert_equal(CLAR_CURRENT_FILE,CLAR_CURRENT_FUNC,CLAR_CURRENT_LINE,#i1 " != " #i2, 1, (fmt), (int)(i1), (int)(i2)) +#define cl_assert_compare_i_(i1, i2, cmp, error, ...) clar__assert_compare_i(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, 1, cmp, \ + (i1), (i2), "Expected comparison to hold: " error, __VA_ARGS__) +#define cl_assert_compare_i(i1, i2, cmp, error, fmt) do { \ + intmax_t v1 = (i1), v2 = (i2); \ + clar__assert_compare_i(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, 1, cmp, \ + v1, v2, "Expected comparison to hold: " error, fmt, v1, v2); \ +} while (0) +#define cl_assert_equal_i_(i1, i2, ...) cl_assert_compare_i_(i1, i2, CLAR_COMPARISON_EQ, #i1 " == " #i2, __VA_ARGS__) +#define cl_assert_equal_i(i1, i2) cl_assert_compare_i (i1, i2, CLAR_COMPARISON_EQ, #i1 " == " #i2, "%"PRIdMAX " != %"PRIdMAX) +#define cl_assert_equal_i_fmt(i1, i2, fmt) cl_assert_compare_i_(i1, i2, CLAR_COMPARISON_EQ, #i1 " == " #i2, fmt " != " fmt, (int)(i1), (int)(i2)) +#define cl_assert_lt_i_(i1, i2, ...) cl_assert_compare_i_(i1, i2, CLAR_COMPARISON_LT, #i1 " < " #i2, __VA_ARGS__) +#define cl_assert_lt_i(i1, i2) cl_assert_compare_i (i1, i2, CLAR_COMPARISON_LT, #i1 " < " #i2, "%"PRIdMAX " >= %"PRIdMAX) +#define cl_assert_le_i_(i1, i2, ...) cl_assert_compare_i_(i1, i2, CLAR_COMPARISON_LE, #i1 " <= " #i2, __VA_ARGS__) +#define cl_assert_le_i(i1, i2) cl_assert_compare_i (i1, i2, CLAR_COMPARISON_LE, #i1 " <= " #i2, "%"PRIdMAX " > %"PRIdMAX) +#define cl_assert_gt_i_(i1, i2, ...) cl_assert_compare_i_(i1, i2, CLAR_COMPARISON_GT, #i1 " > " #i2, __VA_ARGS__) +#define cl_assert_gt_i(i1, i2) cl_assert_compare_i (i1, i2, CLAR_COMPARISON_GT, #i1 " > " #i2, "%"PRIdMAX " <= %"PRIdMAX) +#define cl_assert_ge_i_(i1, i2, ...) cl_assert_compare_i_(i1, i2, CLAR_COMPARISON_GE, #i1 " >= " #i2, __VA_ARGS__) +#define cl_assert_ge_i(i1, i2) cl_assert_compare_i (i1, i2, CLAR_COMPARISON_GE, #i1 " >= " #i2, "%"PRIdMAX " < %"PRIdMAX) + +#define cl_assert_compare_u_(u1, u2, cmp, error, ...) clar__assert_compare_u(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, 1, cmp, \ + (u1), (u2), "Expected comparison to hold: " error, __VA_ARGS__) +#define cl_assert_compare_u(u1, u2, cmp, error, fmt) do { \ + uintmax_t v1 = (u1), v2 = (u2); \ + clar__assert_compare_u(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, CLAR_CURRENT_LINE, 1, cmp, \ + v1, v2, "Expected comparison to hold: " error, fmt, v1, v2); \ +} while (0) +#define cl_assert_equal_u_(u1, u2, ...) cl_assert_compare_u_(u1, u2, CLAR_COMPARISON_EQ, #u1 " == " #u2, __VA_ARGS__) +#define cl_assert_equal_u(u1, u2) cl_assert_compare_u (u1, u2, CLAR_COMPARISON_EQ, #u1 " == " #u2, "%"PRIuMAX " != %"PRIuMAX) +#define cl_assert_lt_u_(u1, u2, ...) cl_assert_compare_u_(u1, u2, CLAR_COMPARISON_LT, #u1 " < " #u2, __VA_ARGS__) +#define cl_assert_lt_u(u1, u2) cl_assert_compare_u (u1, u2, CLAR_COMPARISON_LT, #u1 " < " #u2, "%"PRIuMAX " >= %"PRIuMAX) +#define cl_assert_le_u_(u1, u2, ...) cl_assert_compare_u_(u1, u2, CLAR_COMPARISON_LE, #u1 " <= " #u2, __VA_ARGS__) +#define cl_assert_le_u(u1, u2) cl_assert_compare_u (u1, u2, CLAR_COMPARISON_LE, #u1 " <= " #u2, "%"PRIuMAX " > %"PRIuMAX) +#define cl_assert_gt_u_(u1, u2, ...) cl_assert_compare_u_(u1, u2, CLAR_COMPARISON_GT, #u1 " > " #u2, __VA_ARGS__) +#define cl_assert_gt_u(u1, u2) cl_assert_compare_u (u1, u2, CLAR_COMPARISON_GT, #u1 " > " #u2, "%"PRIuMAX " <= %"PRIuMAX) +#define cl_assert_ge_u_(u1, u2, ...) cl_assert_compare_u_(u1, u2, CLAR_COMPARISON_GE, #u1 " >= " #u2, __VA_ARGS__) +#define cl_assert_ge_u(u1, u2) cl_assert_compare_u (u1, u2, CLAR_COMPARISON_GE, #u1 " >= " #u2, "%"PRIuMAX " < %"PRIuMAX) #define cl_assert_equal_b(b1,b2) clar__assert_equal(CLAR_CURRENT_FILE,CLAR_CURRENT_FUNC,CLAR_CURRENT_LINE,#b1 " != " #b2, 1, "%d", (int)((b1) != 0),(int)((b2) != 0)) @@ -186,6 +221,15 @@ void clar__fail( const char *description, int should_abort); +void clar__failf( + const char *file, + const char *func, + size_t line, + int should_abort, + const char *error, + const char *description, + ...); + void clar__assert( int condition, const char *file, @@ -204,6 +248,38 @@ void clar__assert_equal( const char *fmt, ...); +enum clar_comparison { + CLAR_COMPARISON_EQ, + CLAR_COMPARISON_LT, + CLAR_COMPARISON_LE, + CLAR_COMPARISON_GT, + CLAR_COMPARISON_GE, +}; + +void clar__assert_compare_i( + const char *file, + const char *func, + size_t line, + int should_abort, + enum clar_comparison cmp, + intmax_t value1, + intmax_t value2, + const char *error, + const char *description, + ...); + +void clar__assert_compare_u( + const char *file, + const char *func, + size_t line, + int should_abort, + enum clar_comparison cmp, + uintmax_t value1, + uintmax_t value2, + const char *error, + const char *description, + ...); + void clar__set_invokepoint( const char *file, const char *func, diff --git a/t/unit-tests/clar/clar/print.h b/t/unit-tests/clar/clar/print.h index 89b66591d7556d..6a2321b399d192 100644 --- a/t/unit-tests/clar/clar/print.h +++ b/t/unit-tests/clar/clar/print.h @@ -164,7 +164,7 @@ static void clar_print_tap_ontest(const char *suite_name, const char *test_name, printf(" file: '"); print_escaped(error->file); printf("'\n"); printf(" line: %" PRIuMAX "\n", error->line_number); printf(" function: '%s'\n", error->function); - printf(" ---\n"); + printf(" ...\n"); } break; diff --git a/t/unit-tests/clar/test/expected/quiet b/t/unit-tests/clar/test/expected/quiet index 280c99d8ad5eba..a93273b5a23003 100644 --- a/t/unit-tests/clar/test/expected/quiet +++ b/t/unit-tests/clar/test/expected/quiet @@ -18,27 +18,57 @@ combined::strings_with_length [file:42] 5) Failure: combined::int [file:42] - 101 != value ("extra note on failing test") + Expected comparison to hold: 101 == value 101 != 100 6) Failure: +combined::int_note [file:42] + Expected comparison to hold: 101 == value + extra note on failing test + + 7) Failure: combined::int_fmt [file:42] - 022 != value + Expected comparison to hold: 022 == value 0022 != 0144 - 7) Failure: + 8) Failure: combined::bool [file:42] 0 != value 0 != 1 - 8) Failure: + 9) Failure: combined::multiline_description [file:42] Function call failed: -1 description line 1 description line 2 - 9) Failure: + 10) Failure: combined::null_string [file:42] String mismatch: "expected" != actual ("this one fails") 'expected' != NULL + 11) Failure: +combined::failf [file:42] + Test failed. + some reason: foo + + 12) Failure: +combined::compare_i [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 13) Failure: +combined::compare_i_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + + 14) Failure: +combined::compare_u [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 15) Failure: +combined::compare_u_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + diff --git a/t/unit-tests/clar/test/expected/summary_with_filename b/t/unit-tests/clar/test/expected/summary_with_filename index 460160791d14c0..a9471cc7d51762 100644 --- a/t/unit-tests/clar/test/expected/summary_with_filename +++ b/t/unit-tests/clar/test/expected/summary_with_filename @@ -1,6 +1,6 @@ Loaded 1 suites: Started (test status codes: OK='.' FAILURE='F' SKIPPED='S') -FFFFFFFFF +FFFFFFFFFFFFFFF 1) Failure: combined::1 [file:42] @@ -22,28 +22,58 @@ combined::strings_with_length [file:42] 5) Failure: combined::int [file:42] - 101 != value ("extra note on failing test") + Expected comparison to hold: 101 == value 101 != 100 6) Failure: +combined::int_note [file:42] + Expected comparison to hold: 101 == value + extra note on failing test + + 7) Failure: combined::int_fmt [file:42] - 022 != value + Expected comparison to hold: 022 == value 0022 != 0144 - 7) Failure: + 8) Failure: combined::bool [file:42] 0 != value 0 != 1 - 8) Failure: + 9) Failure: combined::multiline_description [file:42] Function call failed: -1 description line 1 description line 2 - 9) Failure: + 10) Failure: combined::null_string [file:42] String mismatch: "expected" != actual ("this one fails") 'expected' != NULL + 11) Failure: +combined::failf [file:42] + Test failed. + some reason: foo + + 12) Failure: +combined::compare_i [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 13) Failure: +combined::compare_i_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + + 14) Failure: +combined::compare_u [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 15) Failure: +combined::compare_u_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + written summary file to different.xml diff --git a/t/unit-tests/clar/test/expected/summary_without_filename b/t/unit-tests/clar/test/expected/summary_without_filename index 7874c1d98bc01b..83ba770d006e78 100644 --- a/t/unit-tests/clar/test/expected/summary_without_filename +++ b/t/unit-tests/clar/test/expected/summary_without_filename @@ -1,6 +1,6 @@ Loaded 1 suites: Started (test status codes: OK='.' FAILURE='F' SKIPPED='S') -FFFFFFFFF +FFFFFFFFFFFFFFF 1) Failure: combined::1 [file:42] @@ -22,28 +22,58 @@ combined::strings_with_length [file:42] 5) Failure: combined::int [file:42] - 101 != value ("extra note on failing test") + Expected comparison to hold: 101 == value 101 != 100 6) Failure: +combined::int_note [file:42] + Expected comparison to hold: 101 == value + extra note on failing test + + 7) Failure: combined::int_fmt [file:42] - 022 != value + Expected comparison to hold: 022 == value 0022 != 0144 - 7) Failure: + 8) Failure: combined::bool [file:42] 0 != value 0 != 1 - 8) Failure: + 9) Failure: combined::multiline_description [file:42] Function call failed: -1 description line 1 description line 2 - 9) Failure: + 10) Failure: combined::null_string [file:42] String mismatch: "expected" != actual ("this one fails") 'expected' != NULL + 11) Failure: +combined::failf [file:42] + Test failed. + some reason: foo + + 12) Failure: +combined::compare_i [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 13) Failure: +combined::compare_i_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + + 14) Failure: +combined::compare_u [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 15) Failure: +combined::compare_u_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + written summary file to summary.xml diff --git a/t/unit-tests/clar/test/expected/tap b/t/unit-tests/clar/test/expected/tap index bddbd5dfe98b61..e67118d3aedbf8 100644 --- a/t/unit-tests/clar/test/expected/tap +++ b/t/unit-tests/clar/test/expected/tap @@ -8,7 +8,7 @@ not ok 1 - combined::1 file: 'file' line: 42 function: 'func' - --- + ... not ok 2 - combined::2 --- reason: | @@ -17,7 +17,7 @@ not ok 2 - combined::2 file: 'file' line: 42 function: 'func' - --- + ... not ok 3 - combined::strings --- reason: | @@ -27,7 +27,7 @@ not ok 3 - combined::strings file: 'file' line: 42 function: 'func' - --- + ... not ok 4 - combined::strings_with_length --- reason: | @@ -37,28 +37,38 @@ not ok 4 - combined::strings_with_length file: 'file' line: 42 function: 'func' - --- + ... not ok 5 - combined::int --- reason: | - 101 != value ("extra note on failing test") + Expected comparison to hold: 101 == value 101 != 100 at: file: 'file' line: 42 function: 'func' + ... +not ok 6 - combined::int_note --- -not ok 6 - combined::int_fmt + reason: | + Expected comparison to hold: 101 == value + extra note on failing test + at: + file: 'file' + line: 42 + function: 'func' + ... +not ok 7 - combined::int_fmt --- reason: | - 022 != value + Expected comparison to hold: 022 == value 0022 != 0144 at: file: 'file' line: 42 function: 'func' - --- -not ok 7 - combined::bool + ... +not ok 8 - combined::bool --- reason: | 0 != value @@ -67,8 +77,8 @@ not ok 7 - combined::bool file: 'file' line: 42 function: 'func' - --- -not ok 8 - combined::multiline_description + ... +not ok 9 - combined::multiline_description --- reason: | Function call failed: -1 @@ -78,8 +88,8 @@ not ok 8 - combined::multiline_description file: 'file' line: 42 function: 'func' - --- -not ok 9 - combined::null_string + ... +not ok 10 - combined::null_string --- reason: | String mismatch: "expected" != actual ("this one fails") @@ -88,5 +98,55 @@ not ok 9 - combined::null_string file: 'file' line: 42 function: 'func' + ... +not ok 11 - combined::failf + --- + reason: | + Test failed. + some reason: foo + at: + file: 'file' + line: 42 + function: 'func' + ... +not ok 12 - combined::compare_i --- -1..9 + reason: | + Expected comparison to hold: two < 1 + 2 >= 1 + at: + file: 'file' + line: 42 + function: 'func' + ... +not ok 13 - combined::compare_i_with_format + --- + reason: | + Expected comparison to hold: two < 1 + foo: bar + at: + file: 'file' + line: 42 + function: 'func' + ... +not ok 14 - combined::compare_u + --- + reason: | + Expected comparison to hold: two < 1 + 2 >= 1 + at: + file: 'file' + line: 42 + function: 'func' + ... +not ok 15 - combined::compare_u_with_format + --- + reason: | + Expected comparison to hold: two < 1 + foo: bar + at: + file: 'file' + line: 42 + function: 'func' + ... +1..15 diff --git a/t/unit-tests/clar/test/expected/without_arguments b/t/unit-tests/clar/test/expected/without_arguments index 1111d418a060f7..9891f45a703984 100644 --- a/t/unit-tests/clar/test/expected/without_arguments +++ b/t/unit-tests/clar/test/expected/without_arguments @@ -1,6 +1,6 @@ Loaded 1 suites: Started (test status codes: OK='.' FAILURE='F' SKIPPED='S') -FFFFFFFFF +FFFFFFFFFFFFFFF 1) Failure: combined::1 [file:42] @@ -22,27 +22,57 @@ combined::strings_with_length [file:42] 5) Failure: combined::int [file:42] - 101 != value ("extra note on failing test") + Expected comparison to hold: 101 == value 101 != 100 6) Failure: +combined::int_note [file:42] + Expected comparison to hold: 101 == value + extra note on failing test + + 7) Failure: combined::int_fmt [file:42] - 022 != value + Expected comparison to hold: 022 == value 0022 != 0144 - 7) Failure: + 8) Failure: combined::bool [file:42] 0 != value 0 != 1 - 8) Failure: + 9) Failure: combined::multiline_description [file:42] Function call failed: -1 description line 1 description line 2 - 9) Failure: + 10) Failure: combined::null_string [file:42] String mismatch: "expected" != actual ("this one fails") 'expected' != NULL + 11) Failure: +combined::failf [file:42] + Test failed. + some reason: foo + + 12) Failure: +combined::compare_i [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 13) Failure: +combined::compare_i_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + + 14) Failure: +combined::compare_u [file:42] + Expected comparison to hold: two < 1 + 2 >= 1 + + 15) Failure: +combined::compare_u_with_format [file:42] + Expected comparison to hold: two < 1 + foo: bar + diff --git a/t/unit-tests/clar/test/selftest.c b/t/unit-tests/clar/test/selftest.c index eed83e4512006d..6eadc64c4813b8 100644 --- a/t/unit-tests/clar/test/selftest.c +++ b/t/unit-tests/clar/test/selftest.c @@ -298,7 +298,7 @@ void test_selftest__help(void) void test_selftest__without_arguments(void) { - cl_invoke(assert_output("combined", "without_arguments", 9, NULL)); + cl_invoke(assert_output("combined", "without_arguments", 15, NULL)); } void test_selftest__specific_test(void) @@ -313,12 +313,12 @@ void test_selftest__stop_on_failure(void) void test_selftest__quiet(void) { - cl_invoke(assert_output("combined", "quiet", 9, "-q", NULL)); + cl_invoke(assert_output("combined", "quiet", 15, "-q", NULL)); } void test_selftest__tap(void) { - cl_invoke(assert_output("combined", "tap", 9, "-t", NULL)); + cl_invoke(assert_output("combined", "tap", 15, "-t", NULL)); } void test_selftest__suite_names(void) @@ -329,7 +329,7 @@ void test_selftest__suite_names(void) void test_selftest__summary_without_filename(void) { struct stat st; - cl_invoke(assert_output("combined", "summary_without_filename", 9, "-r", NULL)); + cl_invoke(assert_output("combined", "summary_without_filename", 15, "-r", NULL)); /* The summary contains timestamps, so we cannot verify its contents. */ cl_must_pass(stat("summary.xml", &st)); } @@ -337,7 +337,7 @@ void test_selftest__summary_without_filename(void) void test_selftest__summary_with_filename(void) { struct stat st; - cl_invoke(assert_output("combined", "summary_with_filename", 9, "-rdifferent.xml", NULL)); + cl_invoke(assert_output("combined", "summary_with_filename", 15, "-rdifferent.xml", NULL)); /* The summary contains timestamps, so we cannot verify its contents. */ cl_must_pass(stat("different.xml", &st)); } diff --git a/t/unit-tests/clar/test/suites/combined.c b/t/unit-tests/clar/test/suites/combined.c index e8b41c98c37fa2..9e9dbc2fb1f180 100644 --- a/t/unit-tests/clar/test/suites/combined.c +++ b/t/unit-tests/clar/test/suites/combined.c @@ -55,7 +55,12 @@ void test_combined__strings_with_length(void) void test_combined__int(void) { int value = 100; - cl_assert_equal_i(100, value); + cl_assert_equal_i(101, value); +} + +void test_combined__int_note(void) +{ + int value = 100; cl_assert_equal_i_(101, value, "extra note on failing test"); } @@ -83,3 +88,61 @@ void test_combined__null_string(void) cl_assert_equal_s(actual, actual); cl_assert_equal_s_("expected", actual, "this one fails"); } + +void test_combined__failf(void) +{ + cl_failf("some reason: %s", "foo"); +} + +void test_combined__compare_i(void) +{ + int one = 1, two = 2; + + cl_assert_equal_i(one, 1); + cl_assert_equal_i(one, 1); + cl_assert_equal_i_(one, 1, "format"); + cl_assert_lt_i(one, 2); + cl_assert_lt_i_(one, 2, "format"); + cl_assert_le_i(one, 2); + cl_assert_le_i(two, 2); + cl_assert_le_i_(two, 2, "format"); + cl_assert_gt_i(two, 1); + cl_assert_gt_i_(two, 1, "format"); + cl_assert_ge_i(two, 2); + cl_assert_ge_i(3, two); + cl_assert_ge_i_(3, two, "format"); + + cl_assert_lt_i(two, 1); /* this one fails */ +} + +void test_combined__compare_i_with_format(void) +{ + int two = 2; + cl_assert_lt_i_(two, 1, "foo: %s", "bar"); +} + +void test_combined__compare_u(void) +{ + unsigned one = 1, two = 2; + + cl_assert_equal_u(one, 1); + cl_assert_equal_u_(one, 1, "format"); + cl_assert_lt_u(one, 2); + cl_assert_lt_u_(one, 2, "format"); + cl_assert_le_u(one, 2); + cl_assert_le_u(two, 2); + cl_assert_le_u_(two, 2, "format"); + cl_assert_gt_u(two, 1); + cl_assert_gt_u_(two, 1, "format"); + cl_assert_ge_u(two, 2); + cl_assert_ge_u(3, two); + cl_assert_ge_u_(3, two, "format"); + + cl_assert_lt_u(two, 1); /* this one fails */ +} + +void test_combined__compare_u_with_format(void) +{ + unsigned two = 2; + cl_assert_lt_u_(two, 1, "foo: %s", "bar"); +} diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h index 39a0b72a05dec3..5398b449171560 100644 --- a/t/unit-tests/unit-test.h +++ b/t/unit-tests/unit-test.h @@ -7,9 +7,3 @@ #else # include GIT_CLAR_DECLS_H #endif - -#define cl_failf(fmt, ...) do { \ - char desc[4096]; \ - snprintf(desc, sizeof(desc), fmt, __VA_ARGS__); \ - clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \ -} while (0) From 2e53d29f53e2a4c6bb5b9f8f3169c178e5ef62a0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Sat, 6 Dec 2025 12:47:33 +0100 Subject: [PATCH 02/13] t/unit-tests: demonstrate use of integer comparison assertions The clar project has introduced a couple of new assertions that perform relative integer comparisons, like "greater than" or "less or equal". Adapt the reftable-record unit tests to demonstrate their usage. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/unit-tests/u-reftable-record.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/t/unit-tests/u-reftable-record.c b/t/unit-tests/u-reftable-record.c index 6c8c0d5374a6a2..1bf2e170dc96a0 100644 --- a/t/unit-tests/u-reftable-record.c +++ b/t/unit-tests/u-reftable-record.c @@ -51,10 +51,10 @@ void test_reftable_record__varint_roundtrip(void) int n = put_var_int(&out, in); uint64_t got = 0; - cl_assert(n > 0); + cl_assert_gt_i(n, 0); out.len = n; n = get_var_int(&got, &out); - cl_assert(n > 0); + cl_assert_gt_i(n, 0); cl_assert_equal_i(got, in); } @@ -110,7 +110,7 @@ void test_reftable_record__ref_record_comparison(void) cl_assert(reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1) == 0); cl_assert_equal_i(reftable_record_cmp(&in[1], &in[2], &cmp), 0); - cl_assert(cmp > 0); + cl_assert_gt_i(cmp, 0); in[1].u.ref.value_type = in[0].u.ref.value_type; cl_assert(reftable_record_equal(&in[0], &in[1], @@ -184,7 +184,7 @@ void test_reftable_record__ref_record_roundtrip(void) reftable_record_key(&in, &key); n = reftable_record_encode(&in, dest, REFTABLE_HASH_SIZE_SHA1); - cl_assert(n > 0); + cl_assert_gt_i(n, 0); /* decode into a non-zero reftable_record to test for leaks. */ m = reftable_record_decode(&out, key, i, dest, REFTABLE_HASH_SIZE_SHA1, &scratch); @@ -228,11 +228,11 @@ void test_reftable_record__log_record_comparison(void) cl_assert_equal_i(reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1), 0); cl_assert_equal_i(reftable_record_cmp(&in[1], &in[2], &cmp), 0); - cl_assert(cmp > 0); + cl_assert_gt_i(cmp, 0); /* comparison should be reversed for equal keys, because * comparison is now performed on the basis of update indices */ cl_assert_equal_i(reftable_record_cmp(&in[0], &in[1], &cmp), 0); - cl_assert(cmp < 0); + cl_assert_lt_i(cmp, 0); in[1].u.log.update_index = in[0].u.log.update_index; cl_assert(reftable_record_equal(&in[0], &in[1], @@ -344,7 +344,7 @@ void test_reftable_record__log_record_roundtrip(void) reftable_record_key(&rec, &key); n = reftable_record_encode(&rec, dest, REFTABLE_HASH_SIZE_SHA1); - cl_assert(n >= 0); + cl_assert_ge_i(n, 0); valtype = reftable_record_val_type(&rec); m = reftable_record_decode(&out, key, valtype, dest, REFTABLE_HASH_SIZE_SHA1, &scratch); @@ -382,7 +382,7 @@ void test_reftable_record__key_roundtrip(void) extra = 6; n = reftable_encode_key(&restart, dest, last_key, key, extra); cl_assert(!restart); - cl_assert(n > 0); + cl_assert_gt_i(n, 0); cl_assert_equal_i(reftable_buf_addstr(&roundtrip, "refs/heads/master"), 0); @@ -432,7 +432,7 @@ void test_reftable_record__obj_record_comparison(void) cl_assert_equal_i(reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1), 0); cl_assert_equal_i(reftable_record_cmp(&in[1], &in[2], &cmp), 0); - cl_assert(cmp > 0); + cl_assert_gt_i(cmp, 0); in[1].u.obj.offset_len = in[0].u.obj.offset_len; cl_assert(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1) != 0); @@ -485,7 +485,7 @@ void test_reftable_record__obj_record_roundtrip(void) t_copy(&in); reftable_record_key(&in, &key); n = reftable_record_encode(&in, dest, REFTABLE_HASH_SIZE_SHA1); - cl_assert(n > 0); + cl_assert_gt_i(n, 0); extra = reftable_record_val_type(&in); m = reftable_record_decode(&out, key, extra, dest, REFTABLE_HASH_SIZE_SHA1, &scratch); @@ -535,7 +535,7 @@ void test_reftable_record__index_record_comparison(void) cl_assert_equal_i(reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1), 0); cl_assert_equal_i(reftable_record_cmp(&in[1], &in[2], &cmp), 0); - cl_assert(cmp > 0); + cl_assert_gt_i(cmp, 0); in[1].u.idx.offset = in[0].u.idx.offset; cl_assert(reftable_record_equal(&in[0], &in[1], From 84071a6deac84fdb99d2415900d7713829de393c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Sat, 6 Dec 2025 12:47:34 +0100 Subject: [PATCH 03/13] gitattributes: disable blank-at-eof errors for clar test expectations The clar unit testing framework carries a couple of files that contain expected output for its self-tests. Some of these files expectedly end with a blank line at the end of the file, which Git would consider to be a whitespace error by default. Teach our gitattributes to ignore those errors. Suggested-by: Junio C Hamano Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- .gitattributes | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitattributes b/.gitattributes index 32583149c2f927..e416c3720568eb 100644 --- a/.gitattributes +++ b/.gitattributes @@ -17,3 +17,4 @@ CODE_OF_CONDUCT.md -whitespace /Documentation/gitk.adoc conflict-marker-size=32 /Documentation/user-manual.adoc conflict-marker-size=32 /t/t????-*.sh conflict-marker-size=32 +/t/unit-tests/clar/test/expected/* whitespace=-blank-at-eof From 0b495cd390ec39045542f6fcae53ce64264301b7 Mon Sep 17 00:00:00 2001 From: Paul Tarjan Date: Sat, 3 Jan 2026 20:40:09 +0000 Subject: [PATCH 04/13] t7800: fix racy "difftool --dir-diff syncs worktree" test The "difftool --dir-diff syncs worktree without unstaged change" test fails intermittently on Windows CI, as seen at: https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 The root cause is that the original file content and the replacement content have identical sizes: - Original: "main\ntest\na\n" = 12 bytes - New: "new content\n" = 12 bytes When difftool's sync-back mechanism checks for changes, it compares stat data between the temporary index and the modified files. If the modification happens within the same timestamp granularity window and file size stays the same, the change goes undetected. On Windows, this is more likely to manifest because Git relies on inode changes as a fallback when other stat fields match, but Windows filesystems lack inodes. This is a real bug that could affect users scripting difftool similarly, as seen at: https://github.com/git-for-windows/git/issues/5132 Fix the test by changing the replacement content to "modified content" (17 bytes), ensuring the size difference is detected regardless of timestamp resolution or platform-specific stat behavior. Note: This fixes the test flakiness but not the underlying issue in difftool's change detection. Other tests with same-size file patterns (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they use normal index operations with proper racy-git detection. Signed-off-by: Paul Tarjan Reviewed-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- t/t7800-difftool.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index bf0f67378dbb23..8a91ff3603ff92 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch ' write_script modify-right-file <<\EOF -echo "new content" >"$2/file" +echo "modified content" >"$2/file" EOF run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' test_when_finished git reset --hard && echo "orig content" >file && git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && - echo "new content" >expect && + echo "modified content" >expect && test_cmp expect file ' run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' test_when_finished git reset --hard && git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && - echo "new content" >expect && + echo "modified content" >expect && test_cmp expect file ' From 76eab50f756fedfa28388213d7fea209f86dfae6 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:17 +0100 Subject: [PATCH 05/13] replay: remove dead code and rearrange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 22d99f01 (replay: add --advance or 'cherry-pick' mode, 2023-11-24) both added `--advance` and made one of `--onto` or `--advance` mandatory. But `determine_replay_mode` claims that there is a third alternative; neither of `--onto` or `--advance` were given: if (onto_name) { ... } else if (*advance_name) { ... } else { ... } But this is false—the fallthrough else-block is dead code. Commit 22d99f01 was iterated upon by several people.[1] The initial author wrote code for a sort of *guess mode*, allowing for shorter commands when that was possible. But the next person instead made one of the aforementioned options mandatory. In turn this code was dead on arrival in git.git. [1]: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/ Let’s remove this code. We can also join the if-block with the condition `!*advance_name` into the `*onto` block since we do not set `*advance_name` in this function. It only looked like we might set it since the dead code has this line: *advance_name = xstrdup_or_null(last_key); Let’s also rename the function since we do not determine the replay mode here. We just set up `*onto` and refs to update. Note that there might be more dead code caused by this *guess mode*. We only concern ourselves with this function for now. Helped-by: Elijah Newren Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 70 +++++++++++------------------------------------- 1 file changed, 16 insertions(+), 54 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 69c4c551297c03..524bf96ffd6c9d 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -162,12 +162,12 @@ static void get_ref_information(struct repository *repo, } } -static void determine_replay_mode(struct repository *repo, - struct rev_cmdline_info *cmd_info, - const char *onto_name, - char **advance_name, - struct commit **onto, - struct strset **update_refs) +static void set_up_replay_mode(struct repository *repo, + struct rev_cmdline_info *cmd_info, + const char *onto_name, + char **advance_name, + struct commit **onto, + struct strset **update_refs) { struct ref_info rinfo; @@ -182,10 +182,16 @@ static void determine_replay_mode(struct repository *repo, if (rinfo.positive_refexprs < strset_get_size(&rinfo.positive_refs)) die(_("all positive revisions given must be references")); - } else if (*advance_name) { + *update_refs = xcalloc(1, sizeof(**update_refs)); + **update_refs = rinfo.positive_refs; + memset(&rinfo.positive_refs, 0, sizeof(**update_refs)); + } else { struct object_id oid; char *fullname = NULL; + if (!*advance_name) + BUG("expected either onto_name or *advance_name in this function"); + *onto = peel_committish(repo, *advance_name); if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name), &oid, &fullname, 0) == 1) { @@ -196,51 +202,6 @@ static void determine_replay_mode(struct repository *repo, } if (rinfo.positive_refexprs > 1) die(_("cannot advance target with multiple sources because ordering would be ill-defined")); - } else { - int positive_refs_complete = ( - rinfo.positive_refexprs == - strset_get_size(&rinfo.positive_refs)); - int negative_refs_complete = ( - rinfo.negative_refexprs == - strset_get_size(&rinfo.negative_refs)); - /* - * We need either positive_refs_complete or - * negative_refs_complete, but not both. - */ - if (rinfo.negative_refexprs > 0 && - positive_refs_complete == negative_refs_complete) - die(_("cannot implicitly determine whether this is an --advance or --onto operation")); - if (negative_refs_complete) { - struct hashmap_iter iter; - struct strmap_entry *entry; - const char *last_key = NULL; - - if (rinfo.negative_refexprs == 0) - die(_("all positive revisions given must be references")); - else if (rinfo.negative_refexprs > 1) - die(_("cannot implicitly determine whether this is an --advance or --onto operation")); - else if (rinfo.positive_refexprs > 1) - die(_("cannot advance target with multiple source branches because ordering would be ill-defined")); - - /* Only one entry, but we have to loop to get it */ - strset_for_each_entry(&rinfo.negative_refs, - &iter, entry) { - last_key = entry->key; - } - - free(*advance_name); - *advance_name = xstrdup_or_null(last_key); - } else { /* positive_refs_complete */ - if (rinfo.negative_refexprs > 1) - die(_("cannot implicitly determine correct base for --onto")); - if (rinfo.negative_refexprs == 1) - *onto = rinfo.onto; - } - } - if (!*advance_name) { - *update_refs = xcalloc(1, sizeof(**update_refs)); - **update_refs = rinfo.positive_refs; - memset(&rinfo.positive_refs, 0, sizeof(**update_refs)); } strset_clear(&rinfo.negative_refs); strset_clear(&rinfo.positive_refs); @@ -451,8 +412,9 @@ int cmd_replay(int argc, revs.simplify_history = 0; } - determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name, - &onto, &update_refs); + set_up_replay_mode(repo, &revs.cmdline, + onto_name, &advance_name, + &onto, &update_refs); if (!onto) /* FIXME: Should handle replaying down to root commit */ die("Replaying down to root commit is not supported yet!"); From 17b7965a03bd38215cb78ae1c4b9646d0ee73a40 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:18 +0100 Subject: [PATCH 06/13] replay: find *onto only after testing for ref name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are about to make `peel_committish` die when it cannot find a commit-ish instead of returning `NULL`. But that would make e.g. `git replay --advance=refs/non-existent` die with a less descriptive error message; the highest-level error message is that the name does not exist as a ref, not that we cannot find a commit-ish based on the name. Let’s try to find the ref and only after that try to peel to as a commit-ish. Also add a regression test to protect this error order from future modifications. Suggested-by: Junio C Hamano Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 2 +- t/t3650-replay-basics.sh | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin/replay.c b/builtin/replay.c index 524bf96ffd6c9d..9265ebcd05d569 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -192,7 +192,6 @@ static void set_up_replay_mode(struct repository *repo, if (!*advance_name) BUG("expected either onto_name or *advance_name in this function"); - *onto = peel_committish(repo, *advance_name); if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name), &oid, &fullname, 0) == 1) { free(*advance_name); @@ -200,6 +199,7 @@ static void set_up_replay_mode(struct repository *repo, } else { die(_("argument to --advance must be a reference")); } + *onto = peel_committish(repo, *advance_name); if (rinfo.positive_refexprs > 1) die(_("cannot advance target with multiple sources because ordering would be ill-defined")); } diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index cf3aacf3551f8e..8ef0b1984d7324 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -51,6 +51,13 @@ test_expect_success 'setup bare' ' git clone --bare . bare ' +test_expect_success 'argument to --advance must be a reference' ' + echo "fatal: argument to --advance must be a reference" >expect && + oid=$(git rev-parse main) && + test_must_fail git replay --advance=$oid topic1..topic2 2>actual && + test_cmp expect actual +' + test_expect_success 'using replay to rebase two branches, one on top of other' ' git replay --ref-action=print --onto main topic1..topic2 >result && From 3074d08cfa1bfb75f96fa4a240c575fad4cb8060 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:19 +0100 Subject: [PATCH 07/13] replay: die descriptively when invalid commit-ish is given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Giving an invalid commit-ish to `--onto` makes git-replay(1) fail with: fatal: Replaying down to root commit is not supported yet! Going backwards from this point: 1. `onto` is `NULL` from `set_up_replay_mode`; 2. that function in turn calls `peel_committish`; and 3. here we return `NULL` if `repo_get_oid` fails. Let’s die immediately with a descriptive error message instead. Doing this also provides us with a descriptive error if we “forget” to provide an argument to `--onto` (but we really do unintentionally):[1] $ git replay --onto ^main topic1 fatal: '^main' is not a valid commit-ish Note that the `--advance` case won’t be triggered in practice because of the “argument to --advance must be a reference” check (see the previous test, and commit). † 1: The argument to `--onto` is mandatory and the option parser accepts both `--onto=` (stuck form) and `--onto name`. The latter form makes it easy to unintentionally pass something to the option when you really meant to pass a positional argument. Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 13 +++++++------ t/t3650-replay-basics.sh | 7 +++++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 9265ebcd05d569..1899ccc7cc3ff5 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -33,13 +33,15 @@ static const char *short_commit_name(struct repository *repo, DEFAULT_ABBREV); } -static struct commit *peel_committish(struct repository *repo, const char *name) +static struct commit *peel_committish(struct repository *repo, + const char *name, + const char *mode) { struct object *obj; struct object_id oid; if (repo_get_oid(repo, name, &oid)) - return NULL; + die(_("'%s' is not a valid commit-ish for %s"), name, mode); obj = parse_object(repo, &oid); return (struct commit *)repo_peel_to_type(repo, name, 0, obj, OBJ_COMMIT); @@ -178,7 +180,7 @@ static void set_up_replay_mode(struct repository *repo, die_for_incompatible_opt2(!!onto_name, "--onto", !!*advance_name, "--advance"); if (onto_name) { - *onto = peel_committish(repo, onto_name); + *onto = peel_committish(repo, onto_name, "--onto"); if (rinfo.positive_refexprs < strset_get_size(&rinfo.positive_refs)) die(_("all positive revisions given must be references")); @@ -199,7 +201,7 @@ static void set_up_replay_mode(struct repository *repo, } else { die(_("argument to --advance must be a reference")); } - *onto = peel_committish(repo, *advance_name); + *onto = peel_committish(repo, *advance_name, "--advance"); if (rinfo.positive_refexprs > 1) die(_("cannot advance target with multiple sources because ordering would be ill-defined")); } @@ -416,8 +418,7 @@ int cmd_replay(int argc, onto_name, &advance_name, &onto, &update_refs); - if (!onto) /* FIXME: Should handle replaying down to root commit */ - die("Replaying down to root commit is not supported yet!"); + /* FIXME: Should handle replaying down to root commit */ /* Build reflog message */ if (advance_name_opt) diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index 8ef0b1984d7324..8d82dad71486ee 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -58,6 +58,13 @@ test_expect_success 'argument to --advance must be a reference' ' test_cmp expect actual ' +test_expect_success '--onto with invalid commit-ish' ' + printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect && + printf "a valid commit-ish for --onto\n" >>expect && + test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual && + test_cmp expect actual +' + test_expect_success 'using replay to rebase two branches, one on top of other' ' git replay --ref-action=print --onto main topic1..topic2 >result && From f67f7ddbbd03634318d54b1d1ad7ed8df4a2b292 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:20 +0100 Subject: [PATCH 08/13] replay: improve code comment and die message Suggested-by: Elijah Newren Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 1899ccc7cc3ff5..402db44af2b38f 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -418,7 +418,7 @@ int cmd_replay(int argc, onto_name, &advance_name, &onto, &update_refs); - /* FIXME: Should handle replaying down to root commit */ + /* FIXME: Should allow replaying commits with the first as a root commit */ /* Build reflog message */ if (advance_name_opt) @@ -454,7 +454,7 @@ int cmd_replay(int argc, int hr; if (!commit->parents) - die(_("replaying down to root commit is not supported yet!")); + die(_("replaying down from root commit is not supported yet!")); if (commit->parents->next) die(_("replaying merge commits is not supported yet!")); From 6f693364cc183ea5a8296c9ce2ff515f47206f92 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:21 +0100 Subject: [PATCH 09/13] replay: die if we cannot parse object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parse_object` can return `NULL`. That will in turn make `repo_peel_to_type` return the same. Let’s die fast and descriptively with the `*_or_die` variant. Suggested-by: Junio C Hamano Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- builtin/replay.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/replay.c b/builtin/replay.c index 402db44af2b38f..1960bbbee8685d 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -42,7 +42,7 @@ static struct commit *peel_committish(struct repository *repo, if (repo_get_oid(repo, name, &oid)) die(_("'%s' is not a valid commit-ish for %s"), name, mode); - obj = parse_object(repo, &oid); + obj = parse_object_or_die(repo, &oid, name); return (struct commit *)repo_peel_to_type(repo, name, 0, obj, OBJ_COMMIT); } From 56b77a687eaf9c48482e9f59ab7077e442e85ff5 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Mon, 5 Jan 2026 20:53:22 +0100 Subject: [PATCH 10/13] t3650: add more regression tests for failure conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There isn’t much test coverage for basic failure conditions. Let’s add a few more since these are simple to write and remove if they become obsolete. Helped-by: Phillip Wood Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- t/t3650-replay-basics.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index 8d82dad71486ee..307101eeb911f7 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -43,6 +43,13 @@ test_expect_success 'setup' ' test_commit L && test_commit M && + git switch --detach topic4 && + test_commit N && + test_commit O && + git switch -c topic-with-merge topic4 && + test_merge P O --no-ff && + git switch main && + git switch -c conflict B && test_commit C.conflict C.t conflict ' @@ -65,6 +72,39 @@ test_expect_success '--onto with invalid commit-ish' ' test_cmp expect actual ' +test_expect_success 'option --onto or --advance is mandatory' ' + echo "error: option --onto or --advance is mandatory" >expect && + test_might_fail git replay -h >>expect && + test_must_fail git replay topic1..topic2 2>actual && + test_cmp expect actual +' + +test_expect_success 'no base or negative ref gives no-replaying down to root error' ' + echo "fatal: replaying down from root commit is not supported yet!" >expect && + test_must_fail git replay --onto=topic1 topic2 2>actual && + test_cmp expect actual +' + +test_expect_success 'options --advance and --contained cannot be used together' ' + printf "fatal: options ${SQ}--advance${SQ} " >expect && + printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect && + test_must_fail git replay --advance=main --contained \ + topic1..topic2 2>actual && + test_cmp expect actual +' + +test_expect_success 'cannot advance target ... ordering would be ill-defined' ' + echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect && + test_must_fail git replay --advance=main main topic1 topic2 2>actual && + test_cmp expect actual +' + +test_expect_success 'replaying merge commits is not supported yet' ' + echo "fatal: replaying merge commits is not supported yet!" >expect && + test_must_fail git replay --advance=main main..topic-with-merge 2>actual && + test_cmp expect actual +' + test_expect_success 'using replay to rebase two branches, one on top of other' ' git replay --ref-action=print --onto main topic1..topic2 >result && From b3449b151767e34a82bf996c4d63feed9ef854bd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 6 Jan 2026 13:58:49 +0100 Subject: [PATCH 11/13] builtin/gc: fix condition for whether to write commit graphs When performing auto-maintenance we check whether commit graphs need to be generated by counting the number of commits that are reachable by any reference, but not covered by a commit graph. This search is performed by iterating through all references and then doing a depth-first search until we have found enough commits that are not present in the commit graph. This logic has a memory leak though: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x55555562e433 in malloc (git+0xda433) #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8 #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9 #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35 #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4 #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12 #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9 #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9 #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11 #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8 #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9 #11 0x55555575166a in run_builtin ../git.c:506:11 #12 0x5555557502f0 in handle_builtin ../git.c:779:9 #13 0x555555751127 in run_argv ../git.c:862:4 #14 0x55555575007b in cmd_main ../git.c:984:19 #15 0x5555557523aa in main ../common-main.c:9:11 #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) #18 0x5555555f0934 in _start (git+0x9c934) The root cause of this memory leak is our use of `commit_list_append()`. This function expects as parameters the item to append and the _tail_ of the list to append. This tail will then be overwritten with the new tail of the list so that it can be used in subsequent calls. But we call it with `commit_list_append(parent->item, &stack)`, so we end up losing everything but the new item. This issue only surfaces when counting merge commits. Next to being a memory leak, it also shows that we're in fact miscounting as we only respect children of the last parent. All previous parents are discarded, so their children will be disregarded unless they are hit via another reference. While crafting a test case for the issue I was puzzled that I couldn't establish the proper border at which the auto-condition would be fulfilled. As it turns out, there's another bug: if an object is at the tip of any reference we don't mark it as seen. Consequently, if it is the tip of or reachable via another ref, we'd count that object multiple times. Fix both of these bugs so that we properly count objects without leaking any memory. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/gc.c | 8 +++++--- t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 92c6e7b954faff..17ff68cbd91037 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) return 0; commit = lookup_commit(the_repository, maybe_peeled); - if (!commit) + if (!commit || commit->object.flags & SEEN) return 0; + commit->object.flags |= SEEN; + if (repo_parse_commit(the_repository, commit) || commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH) return 0; @@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) if (data->num_not_in_graph >= data->limit) return 1; - commit_list_append(commit, &stack); + commit_list_insert(commit, &stack); while (!result && stack) { struct commit_list *parent; @@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) break; } - commit_list_append(parent->item, &stack); + commit_list_insert(parent->item, &stack); } } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 6b36f52df7c95d..7cc0ce57f8f320 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -206,6 +206,31 @@ test_expect_success 'commit-graph auto condition' ' test_subcommand $COMMIT_GRAPH_WRITE err && test_grep "is not a valid task" err From 3d099686560b848fefe71b7e8edf70d1674b9c73 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 6 Jan 2026 13:58:50 +0100 Subject: [PATCH 12/13] odb: properly close sources before freeing them It is possible to hit a memory leak when reading data from a submodule via git-grep(1): Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x55555562e726 in calloc (git+0xda726) #1 0x555555964734 in xcalloc ../wrapper.c:154:8 #2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2 #3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6 #4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17 #5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3 #6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2 #7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2 #8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4 #9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8 #10 0x5555558540bd in odb_read_object ../odb.c:920:6 #11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12 #12 0x55555580a13a in grep_source_load ../grep.c:1986:10 #13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7 #14 0x555555807574 in grep_source_1 ../grep.c:1625:8 #15 0x555555807322 in grep_source ../grep.c:1837:10 #16 0x5555556a5c58 in run ../builtin/grep.c:208:10 #17 0x55555562bb42 in void* ThreadStartFunc(void*) lsan_interceptors.cpp.o #18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) #19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) The root caues of this leak is the way we set up and release the submodule: 1. We use `repo_submodule_init()` to initialize a new repository. This repository is stored in `repos_to_free`. 2. We now read data from the submodule repository. 3. We then call `repo_clear()` on the submodule repositories. 4. `repo_clear()` calls `odb_free()`. 5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`. The issue here is the 5th step: we call `odb_free_sources()` _before_ we call `odb_close()`. But `odb_free_sources()` already frees all sources, so the logic that closes them in `odb_close()` now becomes a no-op. As a consequence, we never explicitly close sources at all. Fix the leak by closing the store before we free the sources. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/odb.c b/odb.c index dc8f292f3d9645..8e67afe185eae9 100644 --- a/odb.c +++ b/odb.c @@ -1132,13 +1132,13 @@ void odb_free(struct object_database *o) oidmap_clear(&o->replace_map, 1); pthread_mutex_destroy(&o->replace_mutex); + odb_close(o); odb_free_sources(o); for (size_t i = 0; i < o->cached_object_nr; i++) free((char *) o->cached_objects[i].value.buf); free(o->cached_objects); - odb_close(o); packfile_store_free(o->packfiles); string_list_clear(&o->submodule_source_paths, 0); From 7264e61d87e58b9d0f5e6424c47c11e9657dfb75 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 15 Jan 2026 05:59:37 -0800 Subject: [PATCH 13/13] Git 2.53-rc0 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.53.0.adoc | 7 +++++++ GIT-VERSION-GEN | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Documentation/RelNotes/2.53.0.adoc b/Documentation/RelNotes/2.53.0.adoc index 35a1ab91edc7fd..dcdebe8954f067 100644 --- a/Documentation/RelNotes/2.53.0.adoc +++ b/Documentation/RelNotes/2.53.0.adoc @@ -34,6 +34,10 @@ UI, Workflows & Features * More object database related information are shown in "git repo structure" output. + * Improve the error message when a bad argument is given to the + `--onto` option of "git replay". Test coverage of "git replay" has + been improved. + Performance, Internal Implementation, Development Support etc. -------------------------------------------------------------- @@ -92,6 +96,9 @@ Performance, Internal Implementation, Development Support etc. * Use hook API to replace ad-hoc invocation of hook scripts with the run_command() API. + * Import newer version of "clar", unit testing framework. + (merge 84071a6dea ps/clar-integers later to maint). + Fixes since v2.52 ----------------- diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 1f7af0328a0461..5adc4afd67488c 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,6 +1,6 @@ #!/bin/sh -DEF_VER=v2.52.GIT +DEF_VER=v2.53.0-rc0 LF=' '