From: Christian Couder Date: Tue, 18 Nov 2025 18:29:51 GMT Subject: Re: [PATCH v2 0/3] fast-import: add 'strip-if-invalid' mode to --signed-commits= Message-ID: In-Reply-To: On Mon, Nov 17, 2025 at 8:52 PM Elijah Newren wrote: > > On Sun, Nov 16, 2025 at 8:35 PM Christian Couder > wrote: > > There are no code changes in this v2, only commit message, > > documentation and test changes: > > > > * Rebased on current 'master'. This avoids the need to mark some > > strings for translation as a recent series doing that has been > > recently merged to 'master'. > > > > * In patch 3/3, improved the commit message to better justify the new > > feature using some sentences from Elijah. > > > > * In patch 3/3, removed tests with dual signatures. This avoids a > > conflict with a separate series from brian carlson that adds a > > "RUST" prereq that is then needed to run tests with dual signatures. > > I'm a bit surprised; from > https://lore.kernel.org/git/xmqqms4rry7f.fsf@gitster.g/, I thought you > were going to rearrange the tests to avoid the conflict, not delete > them. Are no tests of this new functionality needed? There are still 5 new tests left in patch 3/3 that are testing the new 'strip-if-invalid' functionality after I removed the 2 tests that are related to dual signatures. In "t/t9305-fast-import-signatures.sh", dual signatures are already tested to work with `git fast-import --signed-commits=` by the tests that brian's f6581e23 (repository: require Rust support for interoperability, 2025-10-27) modifies. f6581e23 not only adds the RUST prereq to these tests, but it also introduces the RUST prereq itself in "t/test-lib.sh" with: +test_lazy_prereq RUST ' + test "$(build_option rust)" = enabled +' So it's much simpler to just remove the 2 new dual signature tests that will need the RUST prereq when f6581e23 is merged. We can still add back these 2 new tests after f6581e23 is merged if we think it's worth it. To avoid the conflict I could introduce the RUST prereq itself in "t/test-lib.sh" with the same code that f6581e23 uses, but then how do I justify it? What happens if f6581e23 is not actually merged? It seems to me that if we really want the 2 new dual signature tests in this series, we would have to wait until f6581e23 is merged or discarded. > > * In patch 3/3, improved documentation of the new option to say that > > validation behaves as the validation performed by `git > > verify-commit`. > > Looking over the range diff, the other changes look good. Thanks for your review.