Re: [PATCH v3 18/22] builtin/format-patch: fix various trivial memory leaks

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

Patrick Steinhardt <ps@pks.im> writes:

> There are various memory leaks hit by git-format-patch(1). Basically all
> of them are trivial, except that un-setting `diffopt.no_free` requires
> us to unset the `diffopt.file` because we manually close it already.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/log.c           | 12 +++++++++---
>  t/t4014-format-patch.sh |  1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index a73a767606..ff997a0d0e 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1833,6 +1833,7 @@ static struct commit *get_base_commit(const struct format_config *cfg,
>  			}
>  
>  			rev[i] = merge_base->item;
> +			free_commit_list(merge_base);
>  		}
>  
>  		if (rev_nr % 2)

This is correct, but isn't merge_base leaking when there are
multiple and we are not dying on failure?  Perhaps something along
this line?

			struct commit_list *merge_base = NULL;
			if (repo_get_merge_bases(the_repository,
						 rev[2 * i],
						 rev[2 * i + 1], &merge_base) < 0 ||
			    !merge_base || merge_base->next) {
				if (die_on_failure) {
					die(_("failed to find exact merge base"));
				} else {
                 +               	free_commit_list(merge_base);
					free(rev);
					return NULL;
				}
			}

> @@ -2548,12 +2550,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			else
>  				print_signature(signature, rev.diffopt.file);
>  		}
> -		if (output_directory)
> +		if (output_directory) {
>  			fclose(rev.diffopt.file);
> +			rev.diffopt.file = NULL;

Is this a leakfix, or just a general code hygiene improvement?

> +		}
>  	}
>  	stop_progress(&progress);
>  	free(list);
> -	free(branch_name);
>  	if (ignore_if_in_upstream)
>  		free_patch_ids(&ids);

Good eyes. branch_name can be set and then "goto done" can jump this
one over, so it makes sense to move it below and make it part of the
centralized clean-up.  list is not leaking in the current code, and
there is no "goto done" or "return" after it gets allocated before
this point, so it can stay here.  On the other hand, it appears to
me that everything below stop_progress() we see above can be moved
below the "done:" label, except for that ids may still be left
uninitialized without getting populated by get_patch_ids() when
ignore-if-in-upstream is asked but head and upstream are the same
when we jump to the "done:" label, so it needs a bit more work _if_
we wanted to go that route.  I think the postimage of this patch,
i.e.  freeing the "list" and "ids" here before the "done:" label, is
a good place to stop.

Thanks.