From: Elijah Newren <newren@gmail.com>
Date: Tue, 18 Nov 2025 19:04:26 GMT
Subject: Re: [PATCH v2 0/3] fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>
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:
>
> 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.

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