Re: [PATCH 3/3] diff: remove parseopts member of struct diff_options

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2022, 21:56 UTC
Message-ID: <xmqqiliuss8k.fsf@gitster.g>
In-Reply-To: <20b5d6d1-ec09-e09f-4f92-5925fb9c96f8@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 22 quoted lines

> Am 01.12.22 um 02:25 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> repo_diff_setup() builds the struct option array with git diff's command
>>> line options and stores a pointer to it in the parseopts member of
>>> struct diff_options.  The array is freed by diff_setup_done(), but not
>>> by release_revisions().  repo_init_revisions() calls repo_diff_setup(),
>>> thus calling repo_init_revisions() then release_revisions() leaks it.
>>>
>>> We could free the array in release_revisions() as well to plug that
>>> leak, but there is a better way: Only build it when needed.  Move the
>>> get_diff_parseopts() calls to the two places that use the array, free it
>>> after use and get rid of the parseopts member.
>>>
>>> Signed-off-by: René Scharfe <l.s.r@web.de>
>>> ---
>>>  diff.c | 17 ++++++++---------
>>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> I think this hunk is missing?
>
> Yes. O_o

I did not see any other issue in the series, so if no further tweaks are needed, I could just squash it in.

Thanks for working on this clean-up. The way parse-options parser was bolted on (eh, rather, the way the original parser of diff command line options way predated the parse-options machinery) has always disturbed me as being klunky.

Show 17 quoted lines

>
>>
>>  diff.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git c/diff.h w/diff.h
>> index 5229f20486..6840499844 100644
>> --- c/diff.h
>> +++ w/diff.h
>> @@ -394,7 +394,6 @@ struct diff_options {
>>  	unsigned color_moved_ws_handling;
>>
>>  	struct repository *repo;
>> -	struct option *parseopts;
>>  	struct strmap *additional_path_headers;
>>
>>  	int no_free;