Andrew Ruder <andy@xxxxxxxxxxx> writes:
> 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.
> +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.
> + /* 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
