Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> The code looks much easier to reason about than the previous rounds.
>
> A few comments about the design.
>
> - Are there other possible values that might fit in this "optimize"
> member, and what kind of behaviour would they trigger, that we
> can envision? I do not think of any and that is why the "enum
> diff_optimize" member in the diff_options structure smells more
> like a "bool dry_run".
I was thinking about DIFF_OPT_BUFFER , but "bool dry_run" would be good
enough for now.
>
> By the way, giving a member "diff_" prefix when the enclosing
> struct is clearly about "diff" by having a name "diff_options" is
> often a waste of readers' time.
Good advice, field name should avoid overlapping with struct name.
>
> - It is unclear why the dry-run need to imply 0-line context.
I was thinking about matching ignored regex in `quick_consume()`, in which
case we only need to match regex for changes rather than context, so I set
ctxlen to zero. But anyway more `if` reduce both readability and maintainability,
so I won’t set ctxlen in the next version.
>
> - diff_flush_patch_quietly() would be a better name for
> diff_flush_patch_quiet().
Understand, I suppose the idea behind this is to stick to proper grammar as
much as possible when choosing names.
>
> On to the details.
>
>> diff --git a/diff.c b/diff.c
>> index dca87e164f..5254ef9373 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2444,6 +2444,15 @@ static int fn_out_consume(void *priv, char *line, unsigned long len)
>> return 0;
>> }
>>
>> +static int quick_consume(void *priv, char *line, unsigned long len)
>> +{
>> + struct emit_callback *ecbdata = priv;
>> + struct diff_options *o = ecbdata->opt;
>> +
>> + o->found_changes = 1;
>> + return 1;
>> +}
>
> OK, as a non-zero return value from consume callbacks is supposed
> to signal an error and causes xdiff_outf() an early return, this
> serves as a short-cut. One downside is that we cannot truly notice
> and signal an error to our callers, as we will see in a later hunk.
I want to think about how to solve this problem later, but I believe I shouldn’t
address it in this patch.
Thanks,
Lidong
