On 12/9/2020 2:41 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> This will grow later, but we only need a few fields for basic rename
> handling.
Perhaps these things will be extremely clear as the patch
series continues, but...
> +struct rename_info {
> + /*
> + * pairs: pairing of filenames from diffcore_rename()
> + *
> + * Index 1 and 2 correspond to sides 1 & 2 as used in
> + * conflict_info.stages. Index 0 unused.
Hm. This seems wasteful. I'm sure that you have a reason to use
index 0 in the future instead of just avoiding instances of [i-1]
indexes.
> + */
> + struct diff_queue_struct pairs[3];
> +
> + /*
> + * needed_limit: value needed for inexact rename detection to run
> + *
> + * If the current rename limit wasn't high enough for inexact
> + * rename detection to run, this records the limit needed. Otherwise,
> + * this value remains 0.
> + */
> + int needed_limit;
> +};
> +
> struct merge_options_internal {
> /*
> * paths: primary data structure in all of merge ort.
> @@ -96,6 +115,11 @@ struct merge_options_internal {
> */
> struct strmap output;
>
> + /*
> + * renames: various data relating to rename detection
> + */
> + struct rename_info *renames;
> +
And here, you create this as a pointer, but...
> /* Initialization of opt->priv, our internal merge data */
> opt->priv = xcalloc(1, sizeof(*opt->priv));
> + opt->priv->renames = xcalloc(1, sizeof(*opt->priv->renames));
...unconditionally allocate it here. Perhaps there are other cases
where 'struct merge_options_internal' is allocated without the renames
member?
Searching merge-ort.c at this point does not appear to have any
other allocations of opt->priv or struct merge_options_internal.
Perhaps it would be best to include struct rename_info not as a
pointer?
If you do have a reason to keep it as a pointer, then perhaps it
should be freed in clear_internal_opts()?
Thanks,
-Stolee
