patch, 3 partsbuiltin/apply: simplify some gitdiff_* functions

> While at it put an 'else' on the same line as the previous '}'.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/apply.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 42c610e..465f954 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -931,22 +931,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
>  		return find_name(line, NULL, p_value, TERM_TAB);
>  
>  	if (orig_name) {
> -		int len;
> -		const char *name;
> +		int len = strlen(orig_name);
>  		char *another;
> -		name = orig_name;
> -		len = strlen(name);
>  		if (isnull)
> -			die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), name, linenr);
> +			die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"),
> +			    orig_name, linenr);
>  		another = find_name(line, NULL, p_value, TERM_TAB);
> -		if (!another || memcmp(another, name, len + 1))
> +		if (!another || memcmp(another, orig_name, len + 1))
>  			die((side == DIFF_NEW_NAME) ?
>  			    _("git apply: bad git-diff - inconsistent new filename on line %d") :
>  			    _("git apply: bad git-diff - inconsistent old filename on line %d"), linenr);
>  		free(another);
>  		return orig_name;
> -	}
> -	else {
> +	} else {
>  		/* expect "/dev/null" */
>  		if (memcmp("/dev/null", line, 9) || line[9] != '\n')
>  			die(_("git apply: bad git-diff - expected /dev/null on line %d"), linenr);

> As the value returned by gitdiff_verify_name() is put into the
> same variable that is passed as a parameter to this function,
> it is simpler to pass the address of the variable and have
> gitdiff_verify_name() change the variable itself.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

>  builtin/apply.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 465f954..4cafdaf 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -925,37 +925,37 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
>  #define DIFF_OLD_NAME 0
>  #define DIFF_NEW_NAME 1
>  
> -static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, int side)
> +static void gitdiff_verify_name(const char *line, int isnull, char **name, int side)
>  {
> -	if (!orig_name && !isnull)
> -		return find_name(line, NULL, p_value, TERM_TAB);
> +	if (!*name && !isnull) {
> +		*name = find_name(line, NULL, p_value, TERM_TAB);
> +		return;
> +	}
>  
> -	if (orig_name) {
> -		int len = strlen(orig_name);
> +	if (*name) {
> +		int len = strlen(*name);
>  		char *another;
>  		if (isnull)
>  			die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"),
> -			    orig_name, linenr);
> +			    *name, linenr);
>  		another = find_name(line, NULL, p_value, TERM_TAB);
> -		if (!another || memcmp(another, orig_name, len + 1))
> +		if (!another || memcmp(another, *name, len + 1))
>  			die((side == DIFF_NEW_NAME) ?
>  			    _("git apply: bad git-diff - inconsistent new filename on line %d") :
>  			    _("git apply: bad git-diff - inconsistent old filename on line %d"), linenr);
>  		free(another);
> -		return orig_name;
>  	} else {
>  		/* expect "/dev/null" */
>  		if (memcmp("/dev/null", line, 9) || line[9] != '\n')
>  			die(_("git apply: bad git-diff - expected /dev/null on line %d"), linenr);
> -		return NULL;
>  	}
>  }
>  
>  static int gitdiff_oldname(const char *line, struct patch *patch)
>  {
>  	char *orig = patch->old_name;
> -	patch->old_name = gitdiff_verify_name(line, patch->is_new, patch->old_name,
> -					      DIFF_OLD_NAME);
> +	gitdiff_verify_name(line, patch->is_new, &patch->old_name,
> +			    DIFF_OLD_NAME);
>  	if (orig != patch->old_name)
>  		free(orig);
>  	return 0;
> @@ -964,8 +964,8 @@ static int gitdiff_oldname(const char *line, struct patch *patch)
>  static int gitdiff_newname(const char *line, struct patch *patch)
>  {
>  	char *orig = patch->new_name;
> -	patch->new_name = gitdiff_verify_name(line, patch->is_delete, patch->new_name,
> -					      DIFF_NEW_NAME);
> +	gitdiff_verify_name(line, patch->is_delete, &patch->new_name,
> +			    DIFF_NEW_NAME);
>  	if (orig != patch->new_name)
>  		free(orig);
>  	return 0;

> Christian Couder <christian.couder@gmail.com> writes:
>
>> As the value returned by gitdiff_verify_name() is put into the
>> same variable that is passed as a parameter to this function,
>> it is simpler to pass the address of the variable and have
>> gitdiff_verify_name() change the variable itself.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>
>
> This change makes the function less useful by restricting the
> callers--only the ones that wants in-place update can call it after
> this change, while the old function signature allowed a caller to
> pass one variable as orig, receive the result in another variable
> (and probably compare them).
>
> It does not matter very much for this file scope static helper
> either way, and I would probably say the same thing if the patch
> were in reverse (i.e. if the patch were loosening the restriction),
> but I cannot offhand see why this is an improvement.  Puzzled...

> 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>
> ---

>  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;
>  }