Re: [PATCH 01/11] merge-ort: add basic data structures for handling renames
From: Derrick Stolee <hidden>
Date: 2020-12-11 02:05:16
On 12/9/2020 2:41 PM, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted> 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.
quoted hunk ↗ jump to hunk
+ */ + 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