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

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

Patrick Steinhardt <ps@pks.im> writes:

>>  	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)?