Re: [PATCH 2/2] config: respect includes in protected config

From: Glen Choo <chooglen@google.com>
Date: Oct 12, 2022, 22:09 UTC
Message-ID: <kl6l4jw81yor.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqv8ookbua.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> Not commenting on the code yet as I am in the middle of today's
> integration run, but as I notice a bad pattern being followed, let
> me comment before it spreads too widely.
>
> The "add failing test first and then fix the code with flipping the
> test to success" is very much unwelcome.  For whoever gets curious
> enough (me included when accepting posted patch), it is easy to
> revert only the part of the commit outside t/ tentatively to see how
> the original code breaks.  Keeping the fix and protection of the fix
> together will avoid mistakes.  The post context of the hunk that
> changes test_expect_failure to test_expect_success does not cover
> the test script, thereby hiding the body of the test that changes
> behaviour while reviewing the patch text, which is another downside.

Thanks for voicing this, and sorry. I tried this pattern specifically because I thought it make it easier to review for folks who don't touch t/, but I hadn't considered that the reverse might be preferred.