[PATCH 0/3] builtin/apply: simplify some gitdiff_* functions

[PATCH 0/3] builtin/apply: simplify some gitdiff_* functions

[Christian Couder]

While working on libifying "git apply" it appeared that some gitdiff_*
functions are unnecessarily complex.

Christian Couder (3):
  builtin/apply: get rid of useless 'name' variable
  builtin/apply: make gitdiff_verify_name() return void
  builtin/apply: simplify gitdiff_{old,new}name()

 builtin/apply.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

-- 
2.8.0.rc4.1.g302de0d.dirty

[PATCH 1/3] builtin/apply: get rid of useless 'name' variable

[Christian Couder]

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);
-- 
2.8.0.rc4.1.g302de0d.dirty

Re: [PATCH 1/3] builtin/apply: get rid of useless 'name' variable

[Junio C Hamano]

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

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

Looks correct; back when 1e3f6b6e (git-apply: more consistency
checks on gitdiff filenames, 2005-05-23) introduced this function,
the variable "name" was in the outer scope, and a subsequent update
narrowed its scope at ea56188a (Update git-apply to use C-style
quoting for funny pathnames., 2005-10-16) but it could have removed
the variable at the same time.

[PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void

[Christian Couder]

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;
-- 
2.8.0.rc4.1.g302de0d.dirty

Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void

[Junio C Hamano]

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

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

Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void

[Christian Couder]

On Tue, Mar 22, 2016 at 10:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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...

In my opinion it is an improvement because:

- the feature of allowing a caller to pass one variable as orig and
receive the result in another is useless here and just makes it more
difficult to understand what the code is doing because you first have
to realize that in both calls the argument that is passed is also
assigned to, and

- using up the return value to return a result prevents from using it
to signal an error instead of die()ing, so it makes libifying the
function more difficult.

But if you prefer I can just resend a series with only patchs 1/3 and
3/3. And we can discuss later in the context of libifying "git apply"
if this patch makes sense.

[PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name()

[Christian Couder]

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;
 }
 
-- 
2.8.0.rc4.1.g302de0d.dirty

Re: [PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name()

[Junio C Hamano]

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