Re: [PATCH] Apply -p<value> on git-diffs that create/delete files

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 30, 2008, 09:03 UTC
Message-ID: <7v7i5i3ui1.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20081230011545.GA81224@bowser.Belkin>

Andrew Ruder <andy@aeruder.net> writes:

Show 21 quoted lines

> diff --git a/builtin-apply.c b/builtin-apply.c
> index 07244b0..584a910 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -656,137 +660,57 @@ static const char *stop_at_slash(const char *line, int llen)
>  }
>  
>  /*
> - * This is to extract the same name that appears on "diff --git"
> - * line.  We do not find and return anything if it is a rename
> - * patch, and it is OK because we will find the name elsewhere.
> + * This is to extract the same name that appears on "diff --git" line.
> + * The name that is returned also has the root applied to it and the
> + * p_value applied.  We do not find and return anything if it is a
> + * rename patch, and it is OK because we will find the name elsewhere.
>   * We need to reliably find name only when it is mode-change only,
> - * creation or deletion of an empty file.  In any of these cases,
> - * both sides are the same name under a/ and b/ respectively.
> + * creation or deletion of an empty file.  In any of these cases, both
> + * sides are the same name under a/ and b/ respectively.
>   */

This is a very good description of what the fix should do.

Show 26 quoted lines

> +static char *git_header_name(char *line)
> ...
> -	/*
> -	 * Accept a name only if it shows up twice, exactly the same
> -	 * form.
> -	 */
> -	for (len = 0 ; ; len++) {
> -		switch (name[len]) {
> -		default:
> -			continue;
> -		case '\n':
> -			return NULL;
> -		case '\t': case ' ':
> -			second = name+len;
> -			for (;;) {
> -				char c = *second++;
> -				if (c == '\n')
> -					return NULL;
> -				if (c == '/')
> -					break;
> -			}
> -			if (second[len] == '\n' && !memcmp(name, second, len)) {
> -				return xmemdupz(name, len);
> -			}
> -		}
> -	}

You lost the above logic, and instead call find_name() with TERM_SPACE | TERM_TAB to find the end of the first name and you expect it uniquely will find it. It unfortunately won't. Consider this patch:

        diff --git a/b is file b/b is file
        index e69de29..ce01362 100644
        --- a/b is file	
        +++ b/b is file	
        @@ -0,0 +1 @@
        +hello

Your version finds "b" as the first name, skips to "is file b/b is file" and assume that is the second name, and your new code later mistakenly declares it as a rename and returns NULL.

Show 13 quoted lines

> +	/* First we see if they match, if they do, we are done. */
> +	if (strcmp(first, second)) {
> +		const char *first_slash, *second_slash;
> +		/* If they don't, we check that we don't have a a/<match> b/<match>, if we
> + 		 * do we return one of those so the error messages go through correctly
> +		 * later on */
> +		first_slash = stop_at_slash(first, strlen(first));
> +		second_slash = stop_at_slash(second, strlen(second));
>  
> +		/* If this fails, it must be a rename, just return NULL */
> +		if (!first_slash || !second_slash || strcmp(first_slash, second_slash))
> +			goto error2;
>  	}

It should of course return "b is file"; the complex backgracking you removed is all about handling this case correctly.

>  builtin-apply.c       |  203 +++++++++++++++----------------------------------
>  t/t4120-apply-popt.sh |    5 +-
>  2 files changed, 64 insertions(+), 144 deletions(-)

I really wished that this reduction of lines resulted in a code with less bug, though.