Re: [PATCH 16/30] merge-recursive: Introduce new functions to handle rename logic
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 14, 2017, 04:56 UTC
- Message-ID
- <xmqqd14l5ugx.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20171110190550.27059-17-newren@gmail.com>
Elijah Newren <newren@gmail.com> writes:
> +struct rename_info {
> + struct string_list *head_renames;
> + struct string_list *merge_renames;
> +};This type is added in order to allow the caller and the helper to communicate the findings in a single logical structure, instead of having to pass them as separate parameters, etc. If we anticipate that the information that needs to be passed will grow richer in later steps (or a follow-up series), such encapsulation makes a lot of sence.
Show 8 quoted lines
> +static struct rename_info *handle_renames(struct merge_options *o,
> + struct tree *common,
> + struct tree *head,
> + struct tree *merge,
> + struct string_list *entries,
> + int *clean)
> +{
> + struct rename_info *rei = xcalloc(1, sizeof(struct rename_info));I however notice that there is only one caller of this helper at this step, and also at the end of this series. I suspect that it would probably be a better design to make "clean" the return value of this helper, and instead have the caller pass an uninitialised rename_info structure on its stack by address to be filled by the helper.
Show 61 quoted lines
> + rei->head_renames = get_renames(o, head, common, head, merge, entries);
> + rei->merge_renames = get_renames(o, merge, common, head, merge, entries);
> + *clean = process_renames(o, rei->head_renames, rei->merge_renames);
> +
> + return rei;
> +}
> +
> +static void cleanup_renames(struct rename_info *re_info)
> +{
> + string_list_clear(re_info->head_renames, 0);
> + string_list_clear(re_info->merge_renames, 0);
> +
> + free(re_info->head_renames);
> + free(re_info->merge_renames);
> +
> + free(re_info);
> +}
> static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
> {
> return (is_null_oid(oid) || mode == 0) ? NULL: (struct object_id *)oid;
> @@ -1989,7 +2021,8 @@ int merge_trees(struct merge_options *o,
> }
>
> if (unmerged_cache()) {
> - struct string_list *entries, *re_head, *re_merge;
> + struct string_list *entries;
> + struct rename_info *re_info;
> int i;
> /*
> * Only need the hashmap while processing entries, so
> @@ -2003,9 +2036,7 @@ int merge_trees(struct merge_options *o,
> get_files_dirs(o, merge);
>
> entries = get_unmerged();
> - re_head = get_renames(o, head, common, head, merge, entries);
> - re_merge = get_renames(o, merge, common, head, merge, entries);
> - clean = process_renames(o, re_head, re_merge);
> + re_info = handle_renames(o, common, head, merge, entries, &clean);
> record_df_conflict_files(o, entries);
> if (clean < 0)
> goto cleanup;
> @@ -2030,16 +2061,13 @@ int merge_trees(struct merge_options *o,
> }
>
> cleanup:
> - string_list_clear(re_merge, 0);
> - string_list_clear(re_head, 0);
> + cleanup_renames(re_info);
> +
> string_list_clear(entries, 1);
> + free(entries);
>
> hashmap_free(&o->current_file_dir_set, 1);
>
> - free(re_merge);
> - free(re_head);
> - free(entries);
> -
> if (clean < 0)
> return clean;
> }