Re: [PATCH v2 05/12] diff: refactor output of incomplete line

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2025, 17:58 UTC
Message-ID: <xmqqtsz122i8.fsf@gitster.g>
In-Reply-To: <aRG5KeoZg1Q1y8DE@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 13 quoted lines

>>  	case DIFF_SYMBOL_CONTEXT_INCOMPLETE:
>> +		set = diff_get_color_opt(o, DIFF_CONTEXT);
>> +		reset = diff_get_color_opt(o, DIFF_RESET);
>> +		emit_line(o, set, reset, line, len);
>> +		break;
>>  	case DIFF_SYMBOL_CONTEXT_MARKER:
>>  		context = diff_get_color_opt(o, DIFF_CONTEXT);
>>  		reset = diff_get_color_opt(o, DIFF_RESET);
>
> I found it a bit confusing that we use `set`/`reset` here instead of
> `context`/`reset` as before. It doesn't make any difference as these are
> local variables anyway, but it might make sense to explain why you chose
> to use different variables.

Yup, when DIFF_SYMBOL_* stuff was introduced to this codebase, it made the code much harder to reason about X-<, and set/reset/context are used in this switch() statement in apparently "random" ways; no case arm in this switch statement use all three, so there is no need to use these three in the first place.

> Honestly, this whole hunk is somewhat confusing in the first place. It
> doesn't seem to connect with the description in any way, as it's a no-op
> change and we don't even use the newly introduced function.

The hunk in fn_out_consume() is about reacting to "\ No newline", and it calls a new helper function created just for it in the middle hunk. The way that new helper function affects the output will be updated in later steps, but for now, this hunk simply makes sure that the two DIFF_SYMBOL_*s can be treated differently.

Perhaps it would become easier to read if this hunk is removed from this step and a later step that gives a different behaviour for CONTEXT_INCOMPLETE introduce an entirely different code afresh (instead of showing how it updates what we copy here from the context code the original is abusing)?