Re: [PATCH v3 2/2] diff: add -I<regex> that ignores matching changes

From: Michał Kępień <michal@isc.org>
Date: Oct 19, 2020, 09:48 UTC
Message-ID: <20201019094850.GB3717@larwa.hq.kempniu.pl>
In-Reply-To: <xmqqeelycajx.fsf@gitster.c.googlers.com>

First, thanks to Phillip for testing this.

Show 6 quoted lines

> Yes, this is a good location to clear the diff_queue, which is
> per-invocation.  But diff_options->ignore_regex[] can and should be
> shared across multiple invocations of the diff machinery, as we are
> parsing upfront in the command line option parser just once to be
> used throughout the life of the program.  It is wrong to free them
> early like this.

Ugh, sorry, I am only now starting to see the big picture.

Show 14 quoted lines

> I really hate to suggest this, one common API call made into the
> diff machinery from various consumers, when they are absolutely done
> with diff_options, is probably diff_result_code().  Its purpose is
> to collect bits computed to participate in the overall result into
> the final result code and anybody who ran one or more diff and wants
> to learn the outcome must call it, so it is unlikely that callers
> that matter are missing a call to finalize their uses of the diff
> machinery.  So if freeing .ignore_regex[] is really necessary, it
> could be used to tell us where to do so [*1*].
> 
> Unlike per-invocation resources that MUST be freed for repeated
> invocations of the diff machinery made in "git log" family,
> resources held for and repeatedly used during the entire session
> like this may not have to be freed, to be honest, though.

After reading your message, taking a closer look, and doing a few Valgrind runs, I tend to agree that perhaps there is no need to free the 'ignore_regex' array after all - it does *not* get allocated repeatedly during each invocation of the diff machinery and when Git exits, the OS will reclaim any memory allocated by the process anyway. On my laptop, a Valgrind run done for a -DSUPPRESS_ANNOTATED_LEAKS build claims that 'ignore_regex' is one of 1,300+ memory blocks still reachable at exit time, which amount to about 2.5 MB of memory in total. Given this, it feels slightly weird to single 'ignore_regex' out.

I will revert the diff_flush() changes in v4 unless somebody disagrees with the above reasoning.

-- 
Best regards,
Michał Kępień