On Tue, Mar 29, 2022 at 6:51 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Tao Klerks via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
