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

On Thu, Nov 30, 2017 at 3:26 PM, Jonathan Tan [off-list ref] wrote:

On Thu, 30 Nov 2017 01:36:37 +0100 (CET)
Johannes Schindelin [off-list ref] wrote:

quoted

Hi Jonathan,

On Tue, 28 Nov 2017, Jonathan Tan wrote:

quoted

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