Re: [PATCH v3] diff: support anchoring line(s)

From: Stefan Beller <sbeller@google.com>
Date: Dec 4, 2017, 19:45 UTC
Message-ID: <CAGZ79kZ7EvpOUgdkMe=+nx1qV23jvDaY4-GVzjmtN_hnJ71bag@mail.gmail.com>
In-Reply-To: <20171130152605.1b775e9cc2ddd7f917424448@google.com>

On Thu, Nov 30, 2017 at 3:26 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 27 quoted lines

> On Thu, 30 Nov 2017 01:36:37 +0100 (CET)
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
>> Hi Jonathan,
>>
>> On Tue, 28 Nov 2017, Jonathan Tan wrote:
>>
>> > @@ -4607,7 +4627,14 @@ int diff_opt_parse(struct diff_options *options,
>> >             DIFF_XDL_CLR(options, NEED_MINIMAL);
>> >             options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
>> >             options->xdl_opts |= value;
>> > +           if (value == XDF_PATIENCE_DIFF)
>> > +                   clear_patience_anchors(options);
>> >             return argcount;
>> > +   } else if (skip_prefix(arg, "--anchored=", &arg)) {
>> > +           options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
>> > +           ALLOC_GROW(options->anchors, options->anchors_nr + 1,
>> > +                      options->anchors_alloc);
>> > +           options->anchors[options->anchors_nr++] = xstrdup(arg);
>>
>> I looked and failed to find the code that releases this array after the
>> diff is done... did I miss anything?
>
> You didn't miss anything. As far as I can tell, occurrences of struct
> diff_options live throughout the lifetime of an invocation of Git and
> are not freed. (Even if the struct itself is allocated on the stack, its
> pathspec has some elements allocated on the heap.)

I thought at least for the intra-line word diff, which allocates its own diff options struct, we'd clear them, but looking around we seem to leak the diff options consistently. (builtin/blame.c is nice enough to `clear_pathspec(&diff_opts.pathspec)`, but that's about it, no other command takes care of the cruft.

I wonder if diff_flush might be a good place to clean up the diff options and after the flush the diff options are declared invalid?

> test_expect_success '--anchored with non-unique line has no effect'

okay, if it turns out we need this case in the future we can still come up with ideas.

Thanks, Stefan