Re: [PATCH 2/2] format-patch: don't include --stat with --range-diff output
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 24, 2018, 04:17 UTC
- Message-ID
- <xmqqk1l32jo2.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <xmqqa7lz43d8.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> if (rev->rdiff1) {
>> + struct diff_options opts;
>> + memcpy(&opts, &rev->diffopt, sizeof(opts));
>> + opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> +
>> fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> show_range_diff(rev->rdiff1, rev->rdiff2,
>> - rev->creation_factor, 1, &rev->diffopt);
>> + rev->creation_factor, 1, &opts);
>
> I am not quite convinced if this shallow copy is a safe thing to do.
> Quite honestly at this late in the release cycle, as a band-aid, I'd
> rather see a simpler revert than a change like this that we have to
> worry about what happens if/when show_range_diff() _thinks_ it is
> done with the opts and ends up discarding resources (e.g. "FILE *")
> that are shared with rev->diffopt that would still have to be used
> later.Well, let's take it anyway as-is, at least for today, as I notice show_range_diff() itself does another shallow copy, so we are not making anything dramatically worse.
Thanks.