Re: [PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 22, 2016, 21:35 UTC
- 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:
Show 7 quoted lines
> 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.
Show 28 quoted lines
> 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;
> }