Re: [PATCH v5 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall

From: Tao Klerks <tao@klerks.biz>
Date: Mar 30, 2022, 04:46 UTC
Message-ID: <CAPMMpoigpEi9mTET1r9sbSdw92kUXw9cRQPdUv-O179j3+_wAQ@mail.gmail.com>
In-Reply-To: <xmqq5ynwwvli.fsf@gitster.g>

On Tue, Mar 29, 2022 at 6:51 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +# Bypassing the untracked cache here is not desirable, but it expected
> > +# in the current implementation
>
> If that is the case, it is much more desirable to squash it into a
> single [2/2] patch so that the desirable working is documented (so
> future breakage can be caught), the reviewers can read what the
> intended behaviour is more easily (so we do not have to be confused
> by this one saying "expect success"), make it easier to cherry-pick
> the fix and test in the same patch elsewhere, and the existing
> breakage can easily be caught by applying only the test part of the
> patch.

Sorry, I'm not completely sure whether my comment was misleading, or whether I'm misunderstanding your feedback.

The test added here does not test "desirable" behavior from an end-user functional perspective, but it does test behavior that is working "as-designed" as of many years ago.

The intent in adding this test is to ensure that if/when someone changes this behavior down the line, they be forced to understand the implications (eg: the current untracked cache structure does not store the same data for a -uall run as for a -unormal run, and so using the data from one in another would lead to output correctness errors).

Importantly, the behavior that this test is exercising *is not changed* by the 2/2 "improvement" patch; this is why I separated it out - it's a change that I feel we should make either way, and could even be its own independent patch. It's in this series as it adds relevant context and I am later adding similar tests for the new/additional behavior.

Given the (I believe) misunderstanding, a better comment would be something like:

# Bypassing the untracked cache here is not desirable from an # end-user perspective, but is expected in the current implementation. # The untracked cache data stored for a -unormal run cannot be # correctly used in a -uall run - it would yield incorrect output.

Does that make more sense?