Re: [PATCH 01/14] run_diff_files(): delay allocation of combine_diff_path

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

Jeff King <peff@peff.net> writes:

> While looping over the index entries, when we see a higher level stage
> the first thing we do is allocate a combine_diff_path struct for it. But
> this can leak; if check_removed() returns an error, we'll continue to
> the next iteration of the loop without cleaning up.
>
> We can fix this by just delaying the allocation by a few lines.
>
> I don't think this leak is triggered in the test suite, but it's pretty
> easy to see by inspection. My ulterior motive here is that the delayed
> allocation means we have all of the data needed to initialize "dpath" at
> the time of malloc, making it easier to factor out a constructor
> function.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  diff-lib.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

Makes sense.

>
> diff --git a/diff-lib.c b/diff-lib.c
> index c6d3bc4d37..85b8f1fa59 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -156,18 +156,6 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
>  			size_t path_len;
>  			struct stat st;
>  
> -			path_len = ce_namelen(ce);
> -
> -			dpath = xmalloc(combine_diff_path_size(5, path_len));
> -			dpath->path = (char *) &(dpath->parent[5]);
> -
> -			dpath->next = NULL;
> -			memcpy(dpath->path, ce->name, path_len);
> -			dpath->path[path_len] = '\0';
> -			oidclr(&dpath->oid, the_repository->hash_algo);
> -			memset(&(dpath->parent[0]), 0,
> -			       sizeof(struct combine_diff_parent)*5);
> -
>  			changed = check_removed(ce, &st);
>  			if (!changed)
>  				wt_mode = ce_mode_from_stat(ce, st.st_mode);
> @@ -178,7 +166,19 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
>  				}
>  				wt_mode = 0;
>  			}
> +
> +			path_len = ce_namelen(ce);
> +
> +			dpath = xmalloc(combine_diff_path_size(5, path_len));
> +			dpath->path = (char *) &(dpath->parent[5]);
> +
> +			dpath->next = NULL;
> +			memcpy(dpath->path, ce->name, path_len);
> +			dpath->path[path_len] = '\0';
> +			oidclr(&dpath->oid, the_repository->hash_algo);
>  			dpath->mode = wt_mode;
> +			memset(&(dpath->parent[0]), 0,
> +			       sizeof(struct combine_diff_parent)*5);
>  
>  			while (i < entries) {
>  				struct cache_entry *nce = istate->cache[i];