Re: [PATCH v5 10/13] xdiff: delete rchg aliasing

[Junio C Hamano <gitster@pobox.com>]

Ezekiel Newren <ezekielnewren@gmail.com> writes:

> On Wed, Sep 24, 2025 at 4:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote:
>> > From: Ezekiel Newren <ezekielnewren@gmail.com>
>>
>> The commit message should explain why this change is being made
>
> Reasons to delete local variable aliasing:
>   * Usage tracking: Tools are better able to follow the usage.
>   * Refactor churn: Later commits will refactor rchg.
>   * No additional meaning: The local variables express the same meaning
>     as the struct field itself.
>
> Would that suffice?

In general, I do not view the first one is a good excuse.

When using a separate local variable enhannces readability of the
code (which often is true, with a pointer that points deep into a
nested structure member) to humans, we shouldn't blindly bend the
code to cater to less intelligent tools; it needs balancing.

The third one alone is not a good excuse for the same reason.  It
(and the first one) depends on how much benefit we are gaining from
having a short-and-sweet local variables that may make the expressions
and statements they are involved in easier to read.

For this particular change, I would think it is on borderline, and
subjective.  I would be OK with the third point if you rephrase it
to additionally say that the conditional and the inner loop is easy
enough to follow without using the local aliases to make the code
shorter (which of course is the commit author's opinion, but they
deserve to have and express their opinion as part of the rationale
for a change).

Thanks.

-	char *rchg1 = xe->xdf1.rchg, *rchg2 = xe->xdf2.rchg;
 	long i1, i2, l1, l2;
 
 	/*
 	 * Trivial. Collects "groups" of changes and creates an edit script.
 	 */
 	for (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)
-		if (rchg1[i1 - 1] || rchg2[i2 - 1]) {
-			for (l1 = i1; rchg1[i1 - 1]; i1--);
-			for (l2 = i2; rchg2[i2 - 1]; i2--);
+		if (xe->xdf1.rchg[i1 - 1] || xe->xdf2.rchg[i2 - 1]) {
+			for (l1 = i1; xe->xdf1.rchg[i1 - 1]; i1--);
+			for (l2 = i2; xe->xdf2.rchg[i2 - 1]; i2--);