From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 03 Dec 2020 17:00:26 GMT
Subject: Re: [PATCH 2/7] merge-ort: add a clear_internal_opts helper
Message-ID: <42a99014-573f-cc5d-ffa0-b22aec288425@gmail.com>
In-Reply-To: <5d73827b8d6c254f5bc4a99afa421ae34f07182e.1607011187.git.gitgitgadget@gmail.com>

On 12/3/2020 10:59 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Move most of merge_finalize() into a new helper function,
> clear_internal_opts().  This is a step to facilitate recursive merges,
> as well as some future optimizations.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/merge-ort.c b/merge-ort.c
> index b556897bc0..0654c76c8c 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -194,6 +194,29 @@ static void free_strmap_strings(struct strmap *map)
>  	}
>  }
>  
> +static void clear_internal_opts(struct merge_options_internal *opti,
> +				int reinitialize)
> +{
> +	assert(!reinitialize);

I was first confused by this new assert, but you are essentially
saying "this parameter doesn't do anything (yet)" which makes sense.

> +
> +	/*
> +	 * We marked opti->paths with strdup_strings = 0, so that we
> +	 * wouldn't have to make another copy of the fullpath created by
> +	 * make_traverse_path from setup_path_info().  But, now that we've
> +	 * used it and have no other references to these strings, it is time
> +	 * to deallocate them.
> +	 */
> +	free_strmap_strings(&opti->paths);
> +	strmap_clear(&opti->paths, 1);
> +
> +	/*
> +	 * All keys and values in opti->conflicted are a subset of those in
> +	 * opti->paths.  We don't want to deallocate anything twice, so we
> +	 * don't free the keys and we pass 0 for free_values.
> +	 */
> +	strmap_clear(&opti->conflicted, 0);
...
> -	/*
> -	 * We marked opti->paths with strdup_strings = 0, so that we
> -	 * wouldn't have to make another copy of the fullpath created by
> -	 * make_traverse_path from setup_path_info().  But, now that we've
> -	 * used it and have no other references to these strings, it is time
> -	 * to deallocate them.
> -	 */
> -	free_strmap_strings(&opti->paths);
> -	strmap_clear(&opti->paths, 1);
> -
> -	/*
> -	 * All keys and values in opti->conflicted are a subset of those in
> -	 * opti->paths.  We don't want to deallocate anything twice, so we
> -	 * don't free the keys and we pass 0 for free_values.
> -	 */
> -	strmap_clear(&opti->conflicted, 0);

the rest of this is a clear code move.

Thanks,
-Stolee