Re: [PATCH v3] diff: ensure consistent diff behavior with ignore options

From: Lidong Yan <yldhome2d2@gmail.com>
Date: Aug 7, 2025, 01:23 UTC
Message-ID: <5E4D0147-A4DC-4906-9627-A92CE34A4754@gmail.com>
In-Reply-To: <xmqqikj0uze2.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

> 
> 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.

Show 24 quoted lines

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