Re: [PATCH v5 3/4] branch: add --delete-integrated to delete rebase and squash merged branches

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static void find_landed_commits(struct landing_search *searches,
> +				size_t searches_nr,
> +				struct strmap *landed_commits)
> +{
> +	struct changed_paths cache;
> +	struct diff_options opt;

A tiny nit, but because so many things are called x_option in our
codebase, I am used to seeing the variable also carry "x" to match,
or the use of mere "opt" down below would make us go back to see
what kind of option it refers to while reading the code.  I'd liked
to see this called "diffopt" instead.

> +	repo_diff_setup(the_repository, &opt);
> +	opt.flags.recursive = 1;
> +	opt.output_format = DIFF_FORMAT_NO_OUTPUT;
> +	opt.no_free = 1;
> +	diff_setup_done(&opt);
> +	init_changed_paths(&cache);

The .no_free member is set to avoid the diff_free() call made in
changed_path_of(), which in turn is called by find_landed_commit().
Everytime I see manipulation of .no_free member, it makes me look at
the code twice but this seems to be a good use of the feature.  And ...

> +	for (size_t i = 0; i < searches_nr; i++) {
> +		struct commit *landed;
> +
> +		if (!searches[i].changes_nr)
> +			continue;
> +		landed = find_landed_commit(&searches[i], &cache, &opt);
> +		if (landed)
> +			strmap_put(landed_commits, searches[i].branch_name,
> +				   landed);
> +	}
> +
> +	deep_clear_changed_paths(&cache, free_changed_paths);
> +	opt.no_free = 0;
> +	diff_free(&opt);

... clearing the member before calling diff_free() to release the
resources is also in line with the established practice.

I didn't have time to look at other parts of this patch (or other
patches in the series), but the use of .no_free in this function
looked OK.

Thanks.