Skip to content

Dir rename fixes #390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 36 additions & 24 deletions merge-recursive.c
Original file line number Diff line number Diff line change
Expand Up @@ -1943,8 +1943,8 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
char **old_dir, char **new_dir)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Elijah,

On Fri, 11 Oct 2019, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> 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 <newren@gmail.com>

This commit message, as well as the patch, make a lot of sense to me.
Thank you for doing this!

Ciao,
Dscho

> ---
>  merge-recursive.c | 60 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
>
> 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 *ol=
d_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 =3D NULL;
>  	*new_dir =3D 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 =3D strrchr(old_path, '/');
>  	end_of_new =3D strrchr(new_path, '/');
> -
>  	if (end_of_old =3D=3D NULL || end_of_new =3D=3D 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 =3D=3D *--end_of_old &&
>  	       end_of_old !=3D old_path &&
>  	       end_of_new !=3D 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 =3D=3D '/')
> -		end_of_old++;
> -	if (*end_of_old !=3D '/')
> -		end_of_new++;
> -	end_of_old =3D strchr(end_of_old, '/');
> -	end_of_new =3D strchr(end_of_new, '/');
> +	if (end_of_old =3D=3D old_path && end_of_new =3D=3D new_path &&
> +	    *end_of_old =3D=3D *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 =3D end_of_old - old_path;
> -	new_len =3D end_of_new - new_path;
> +	end_of_old =3D strchr(++end_of_old, '/');
> +	end_of_new =3D strchr(++end_of_new, '/');
>
> -	if (old_len !=3D new_len || strncmp(old_path, new_path, old_len)) {
> -		*old_dir =3D xstrndup(old_path, old_len);
> -		*new_dir =3D xstrndup(new_path, new_len);
> -	}
> +	/* Copy the old and new directories into *old_dir and *new_dir. */
> +	*old_dir =3D xstrndup(old_path, end_of_old - old_path);
> +	*new_dir =3D xstrndup(new_path, end_of_new - new_path);
>  }
>
>  static void remove_hashmap_entries(struct hashmap *dir_renames,
> --
> gitgitgadget
>
>

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

Expand All @@ -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,
Expand Down