From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 22 Mar 2016 21:35:31 GMT
Subject: Re: [PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name()
Message-ID: <xmqqy49afb6k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458680322-17681-4-git-send-email-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

> After the previous simplifications, it is easy to see that
> there is no need to free the original string passed to
> gitdiff_verify_name(), because this string can be changed
> only when it is NULL.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

I do not think you need either 1/3 or 2/3 to see that (I actually
think it is easier to see this without 2/3).  The caller passes
patch->old_name and

 - if that is NULL, then we will either get NULL or a new string
   from find_name(); either way, we do not have to worry about
   calling free() on the original NULL;

 - if that is not NULL, then the only possible value returned from
   the function is itself (otherwise it will die()), so we won't be
   calling free() in this code.

so I agree with the conclusion, i.e. the conditional free() can go
from these places.

>  builtin/apply.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 4cafdaf..9cfa9f4 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -953,21 +953,15 @@ static void gitdiff_verify_name(const char *line, int isnull, char **name, int s
>  
>  static int gitdiff_oldname(const char *line, struct patch *patch)
>  {
> -	char *orig = patch->old_name;
>  	gitdiff_verify_name(line, patch->is_new, &patch->old_name,
>  			    DIFF_OLD_NAME);
> -	if (orig != patch->old_name)
> -		free(orig);
>  	return 0;
>  }
>  
>  static int gitdiff_newname(const char *line, struct patch *patch)
>  {
> -	char *orig = patch->new_name;
>  	gitdiff_verify_name(line, patch->is_delete, &patch->new_name,
>  			    DIFF_NEW_NAME);
> -	if (orig != patch->new_name)
> -		free(orig);
>  	return 0;
>  }