Re: [PATCH 3/5] apply: whitespace errors in context lines if we have

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 27, 2024, 01:49 UTC
Message-ID: <xmqqv7zmd9a5.fsf@gitster.g>
In-Reply-To: <xmqqo75eeqx0.fsf@gitster.g>

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

Show 14 quoted lines

> Hmph.  0a80bc9f (apply: detect and mark whitespace errors in context
> lines when fixing, 2015-01-16) deliberately added this check because
> we will correct the whitespace breakages on these lines after
> parsing the hunk with this function while applying.
>
> It is iffy that this case arm for " " kicks in ONLY when applying in
> the forward direction (which is not what you are changing).  When
> applying a patch in reverse, " " is still an "unchanged" context
> line, so we should be treating it the same way regardless of the
> direction.
>
> But at least the call to check_whitespace() from this place when we
> are correcting whitespace rule violations is not iffy, as far as I
> can tell.

Having said all that, I do have to wonder how much value we are getting by supporting that odd "feature" that makes apply take input in a single session a patch that touches the same path TWICE.

If we can get rid of that feature (which I consider a misfeature), we can lose quote a lot of code (anything that touches fn_table can go) and recover the code quality that got visibly worse with the addition of that feature back.

And without the "input may touch the same path TWICE", we do not have to worry about this "context lines after applying a single patch with whitespace=fix will have to be matched loosely with respect to the whitespace when another patch modifies the same file around the same lines", making your changes in [3/5] trivially the right thing to do.

So, I am inclined to say that

 * we propose to get rid of that "a single input may touch the same
   path TWICE" feature at Git 3.0 boundary.

 * we at the same time apply [3/5] (and possibly others, but I do
   not think we want [1/5]).

But until we can shed our pretense that the "single input may touch the same path TWICE" is seriously supported, I do not think applying this series as-is makes sense, as it directly contradicts with that (mis)feature.