From: Junio C Hamano <gitster@pobox.com>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 3/5] apply: whitespace errors in context lines if we have
Date: Mon, 26 Aug 2024 18:49:54 -0700 [thread overview]
Message-ID: <xmqqv7zmd9a5.fsf@gitster.g> (raw)
In-Reply-To: <xmqqo75eeqx0.fsf@gitster.g> (Junio C. Hamano's message of "Mon, 26 Aug 2024 17:43:39 -0700")
Junio C Hamano <gitster@pobox.com> writes:
> 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.
next prev parent reply other threads:[~2024-08-27 1:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-25 10:09 [PATCH 0/5] `--whitespace=fix` with `--no-ignore-whitespace` Rubén Justo
2024-08-25 10:17 ` [PATCH 1/5] apply: introduce `ignore_ws_default` Rubén Justo
2024-08-27 1:11 ` Junio C Hamano
2024-08-25 10:18 ` [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error` Rubén Justo
2024-08-27 0:35 ` Junio C Hamano
2024-08-29 5:07 ` Rubén Justo
2024-08-29 23:13 ` Junio C Hamano
2024-09-03 22:06 ` Rubén Justo
2024-09-04 4:41 ` Junio C Hamano
2024-09-04 18:20 ` Rubén Justo
2024-08-25 10:18 ` [PATCH 3/5] apply: whitespace errors in context lines if we have Rubén Justo
2024-08-27 0:43 ` Junio C Hamano
2024-08-27 1:49 ` Junio C Hamano [this message]
2024-08-27 16:12 ` Junio C Hamano
2024-08-27 0:51 ` Junio C Hamano
2024-08-25 10:19 ` [PATCH 4/5] apply: error message in `record_ws_error()` Rubén Justo
2024-08-27 0:44 ` Junio C Hamano
2024-08-25 10:19 ` [PATCH 5/5] t4124: move test preparation into the test context Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqv7zmd9a5.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).