From d3eebaad5ef02284c86d6c4b80199eac58c6729b Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 22 Oct 2019 21:22:49 +0000 Subject: merge-recursive: clean up get_renamed_dir_portion() Dscho noted a few things making this function hard to follow. Restructure it a bit and add comments to make it easier to follow. The restructurings include: * There was a special case if-check at the end of the function checking whether someone just renamed a file within its original directory, meaning that there could be no directory rename involved. That check was slightly convoluted; it could be done in a more straightforward fashion earlier in the function, and can be done more cheaply too (no call to strncmp). * The conditions for advancing end_of_old and end_of_new before calling strchr were both confusing and unnecessary. If either points at a '/', then they need to be advanced in order to find the next '/'. If either doesn't point at a '/', then advancing them one char before calling strchr() doesn't hurt. So, just rip out the if conditions and advance both before calling strchr(). Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 60 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 24 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 22a12cfeba..f80e48f623 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1943,8 +1943,8 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path, char **old_dir, char **new_dir) { char *end_of_old, *end_of_new; - int old_len, new_len; + /* Default return values: NULL, meaning no rename */ *old_dir = NULL; *new_dir = NULL; @@ -1955,43 +1955,55 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path, * "a/b/c/d" was renamed to "a/b/some/thing/else" * so, for this example, this function returns "a/b/c/d" in * *old_dir and "a/b/some/thing/else" in *new_dir. - * - * Also, if the basename of the file changed, we don't care. We - * want to know which portion of the directory, if any, changed. + */ + + /* + * If the basename of the file changed, we don't care. We want + * to know which portion of the directory, if any, changed. */ end_of_old = strrchr(old_path, '/'); end_of_new = strrchr(new_path, '/'); - if (end_of_old == NULL || end_of_new == NULL) - return; + return; /* We haven't modified *old_dir or *new_dir yet. */ + + /* Find the first non-matching character traversing backwards */ while (*--end_of_new == *--end_of_old && end_of_old != old_path && end_of_new != new_path) ; /* Do nothing; all in the while loop */ + /* - * We've found the first non-matching character in the directory - * paths. That means the current directory we were comparing - * represents the rename. Move end_of_old and end_of_new back - * to the full directory name. + * If both got back to the beginning of their strings, then the + * directory didn't change at all, only the basename did. */ - if (*end_of_old == '/') - end_of_old++; - if (*end_of_old != '/') - end_of_new++; - end_of_old = strchr(end_of_old, '/'); - end_of_new = strchr(end_of_new, '/'); + if (end_of_old == old_path && end_of_new == new_path && + *end_of_old == *end_of_new) + return; /* We haven't modified *old_dir or *new_dir yet. */ /* - * It may have been the case that old_path and new_path were the same - * directory all along. Don't claim a rename if they're the same. + * We've found the first non-matching character in the directory + * paths. That means the current characters we were looking at + * were part of the first non-matching subdir name going back from + * the end of the strings. Get the whole name by advancing both + * end_of_old and end_of_new to the NEXT '/' character. That will + * represent the entire directory rename. + * + * The reason for the increment is cases like + * a/b/star/foo/whatever.c -> a/b/tar/foo/random.c + * After dropping the basename and going back to the first + * non-matching character, we're now comparing: + * a/b/s and a/b/ + * and we want to be comparing: + * a/b/star/ and a/b/tar/ + * but without the pre-increment, the one on the right would stay + * a/b/. */ - old_len = end_of_old - old_path; - new_len = end_of_new - new_path; + end_of_old = strchr(++end_of_old, '/'); + end_of_new = strchr(++end_of_new, '/'); - if (old_len != new_len || strncmp(old_path, new_path, old_len)) { - *old_dir = xstrndup(old_path, old_len); - *new_dir = xstrndup(new_path, new_len); - } + /* Copy the old and new directories into *old_dir and *new_dir. */ + *old_dir = xstrndup(old_path, end_of_old - old_path); + *new_dir = xstrndup(new_path, end_of_new - new_path); } static void remove_hashmap_entries(struct hashmap *dir_renames, -- cgit v1.2.3 From 49b8133a9ece199a17db8bb2545202c6eac67485 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 22 Oct 2019 21:22:50 +0000 Subject: merge-recursive: fix merging a subdirectory into the root directory We allow renaming all entries in e.g. a directory named z/ into a directory named y/ to be detected as a z/ -> y/ rename, so that if the other side of history adds any files to the directory z/ in the mean time, we can provide the hint that they should be moved to y/. There is no reason to not allow 'y/' to be the root directory, but the code did not handle that case correctly. Add a testcase and the necessary special checks to support this case. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index f80e48f623..ec60715368 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1931,6 +1931,16 @@ static char *apply_dir_rename(struct dir_rename_entry *entry, return NULL; oldlen = strlen(entry->dir); + if (entry->new_dir.len == 0) + /* + * If someone renamed/merged a subdirectory into the root + * directory (e.g. 'some/subdir' -> ''), then we want to + * avoid returning + * '' + '/filename' + * as the rename; we need to make old_path + oldlen advance + * past the '/' character. + */ + oldlen++; newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1; strbuf_grow(&new_path, newlen); strbuf_addbuf(&new_path, &entry->new_dir); @@ -1963,8 +1973,26 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path, */ end_of_old = strrchr(old_path, '/'); end_of_new = strrchr(new_path, '/'); - if (end_of_old == NULL || end_of_new == NULL) - return; /* We haven't modified *old_dir or *new_dir yet. */ + + /* + * If end_of_old is NULL, old_path wasn't in a directory, so there + * could not be a directory rename (our rule elsewhere that a + * directory which still exists is not considered to have been + * renamed means the root directory can never be renamed -- because + * the root directory always exists). + */ + if (end_of_old == NULL) + return; /* Note: *old_dir and *new_dir are still NULL */ + + /* + * If new_path contains no directory (end_of_new is NULL), then we + * have a rename of old_path's directory to the root directory. + */ + if (end_of_new == NULL) { + *old_dir = xstrndup(old_path, end_of_old - old_path); + *new_dir = xstrdup(""); + return; + } /* Find the first non-matching character traversing backwards */ while (*--end_of_new == *--end_of_old && @@ -1978,7 +2006,25 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path, */ if (end_of_old == old_path && end_of_new == new_path && *end_of_old == *end_of_new) - return; /* We haven't modified *old_dir or *new_dir yet. */ + return; /* Note: *old_dir and *new_dir are still NULL */ + + /* + * If end_of_new got back to the beginning of its string, and + * end_of_old got back to the beginning of some subdirectory, then + * we have a rename/merge of a subdirectory into the root, which + * needs slightly special handling. + * + * Note: There is no need to consider the opposite case, with a + * rename/merge of the root directory into some subdirectory + * because as noted above the root directory always exists so it + * cannot be considered to be renamed. + */ + if (end_of_new == new_path && + end_of_old != old_path && end_of_old[-1] == '/') { + *old_dir = xstrndup(old_path, --end_of_old - old_path); + *new_dir = xstrdup(""); + return; + } /* * We've found the first non-matching character in the directory -- cgit v1.2.3