Re: [PATCH v2 0/3] fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 18, 2025, 19:04 UTC
- Message-ID
- <CABPp-BE=wcFicz0H7n4CQSFmLF0hv1-0tJQuWdsjKi0rWAQHFg@mail.gmail.com>
- In-Reply-To
- <CAP8UFD03YK47nONVRV_wqOEanC8Oth1iRzsFv=eFhbFs6Q5mPA@mail.gmail.com>
On Tue, Nov 18, 2025 at 10:30 AM Christian Couder <christian.couder@gmail.com> wrote:
Show 52 quoted lines
> > On Mon, Nov 17, 2025 at 8:52 PM Elijah Newren <newren@gmail.com> wrote: > > > > On Sun, Nov 16, 2025 at 8:35 PM Christian Couder > > <christian.couder@gmail.com> 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=<mode>` 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.
Oh, right, there were other tests. Sorry about that, I should have double checked the patches instead of only looking at the range-diff.
Show 7 quoted lines
> > > * 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.
Yeah, I think the series is good to advance.