threads / patch / 26143

patchsetup: translate symlinks in filename when using absolute paths

Subject: [PATCH] setup: translate symlinks in filename when using absolute paths

## tl;dr

4 messages between Dec 27, 2010 and Dec 29, 2010. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Carlo Marcelo Arenas Belon· Dec 27, 2010, 10:54 UTC · lore

otherwise, comparison to validate against work tree will fail when the path includes a symlink and the name passed is not canonical.

Signed-off-by: Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe>
---
 setup.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)
Show changes to setup.c +7 −4
diff --git a/setup.c b/setup.c
index 91887a4..e7c0d4d 100644
--- a/setup.c
+++ b/setup.c
@@ -7,10 +7,13 @@ static int inside_work_tree = -1;
 char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
-	char *sanitized = xmalloc(len + strlen(path) + 1);
-	if (is_absolute_path(orig))
-		strcpy(sanitized, path);
-	else {
+	char *sanitized;
+	if (is_absolute_path(orig)) {
+		const char *temp = make_absolute_path(path);
+		sanitized = xmalloc(len + strlen(temp) + 1);
+		strcpy(sanitized, temp);
+	} else {
+		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
 			memcpy(sanitized, prefix, len);
 		strcpy(sanitized + len, path);

-- 
1.7.3.4.626.g73e7b.dirty
Junio C Hamano· Dec 28, 2010, 19:47 UTC · re: Carlo Marcelo Arenas Belon · lore

Re: [PATCH] setup: translate symlinks in filename when using absolute paths

Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe> writes:
> otherwise, comparison to validate against work tree will fail when
> the path includes a symlink and the name passed is not canonical.
>
> Signed-off-by: Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe>

I take that "path" and "name passed" refer to the same thing (i.e. "path" parameter) in the above.

I think you are trying to handle the case where:
 - you give "/home/carenas/one" from the command line;
 - $PWD is "/home/carenas"; and
 - "/home/carenas" is a symlink to "/net/host/home/carenas"

and the scan-from-the-beginning-of-string check done between "/home/carenas/one" and the return value of get_git_work_tree() which presumably is "/net/host/home/carenas" disagrees. I wonder if a more correct solution might be to help get_git_work_tree() to match the notion of where the repository and its worktree are to the idea of where the user thinks they are, i.e. not "/net/host/home/carenas" but "/home/carenas", a bit better?

That would involve tweaking make_absolute_path() I guess?

Note that your patch is the right thing to do either case, i.e. with or without such a change to make_absolute_path(), as the function is used to set up the return value from get_git_work_tree(). Anything we compare with it should have passed make_absolute_path() at least once.

Nguyễn?
Show 28 quoted lines
> ---
>  setup.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 91887a4..e7c0d4d 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -7,10 +7,13 @@ static int inside_work_tree = -1;
>  char *prefix_path(const char *prefix, int len, const char *path)
>  {
>  	const char *orig = path;
> -	char *sanitized = xmalloc(len + strlen(path) + 1);
> -	if (is_absolute_path(orig))
> -		strcpy(sanitized, path);
> -	else {
> +	char *sanitized;
> +	if (is_absolute_path(orig)) {
> +		const char *temp = make_absolute_path(path);
> +		sanitized = xmalloc(len + strlen(temp) + 1);
> +		strcpy(sanitized, temp);
> +	} else {
> +		sanitized = xmalloc(len + strlen(path) + 1);
>  		if (len)
>  			memcpy(sanitized, prefix, len);
>  		strcpy(sanitized + len, path);
> -- 
> 1.7.3.4.626.g73e7b.dirty
Carlo Marcelo Arenas Belon· Dec 29, 2010, 09:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup: translate symlinks in filename when using absolute paths

On Tue, Dec 28, 2010 at 11:47:07AM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe> writes:
> 
> > otherwise, comparison to validate against work tree will fail when
> > the path includes a symlink and the name passed is not canonical.
> >
> > Signed-off-by: Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe>
> 
> I take that "path" and "name passed" refer to the same thing (i.e. "path"
> parameter) in the above.

yes, and sorry for the cryptic description; you detailed below though this is triggered by the fact that when using an absolute path filename as a parameter detection for worktree is failing because it was normalized through make_absolute_path.

Show 5 quoted lines
> I think you are trying to handle the case where:
> 
>  - you give "/home/carenas/one" from the command line;
>  - $PWD is "/home/carenas"; and
>  - "/home/carenas" is a symlink to "/net/host/home/carenas"

this will be a valid scenario, but the issue (with a different use case) was reported in (which I missed to refer to when running git send-email):

  http://thread.gmane.org/gmane.comp.version-control.git/164212
Show 7 quoted lines
> and the scan-from-the-beginning-of-string check done between
> "/home/carenas/one" and the return value of get_git_work_tree() which
> presumably is "/net/host/home/carenas" disagrees.  I wonder if a more
> correct solution might be to help get_git_work_tree() to match the notion
> of where the repository and its worktree are to the idea of where the user
> thinks they are, i.e. not "/net/host/home/carenas" but "/home/carenas", a
> bit better?

are you suggesting symlinks would be left untouched at least during resolution for work_dir?, why is even necesary to resolve the links for other users of that function?

Carlo
Nguyen Thai Ngoc Duy· Dec 29, 2010, 13:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup: translate symlinks in filename when using absolute paths

2010/12/29 Junio C Hamano <gitster@pobox.com>:
Show 23 quoted lines
> Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe> writes:
>
>> otherwise, comparison to validate against work tree will fail when
>> the path includes a symlink and the name passed is not canonical.
>>
>> Signed-off-by: Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe>
>
> I take that "path" and "name passed" refer to the same thing (i.e. "path"
> parameter) in the above.
>
> I think you are trying to handle the case where:
>
>  - you give "/home/carenas/one" from the command line;
>  - $PWD is "/home/carenas"; and
>  - "/home/carenas" is a symlink to "/net/host/home/carenas"
>
> and the scan-from-the-beginning-of-string check done between
> "/home/carenas/one" and the return value of get_git_work_tree() which
> presumably is "/net/host/home/carenas" disagrees.  I wonder if a more
> correct solution might be to help get_git_work_tree() to match the notion
> of where the repository and its worktree are to the idea of where the user
> thinks they are, i.e. not "/net/host/home/carenas" but "/home/carenas", a
> bit better?
I tend to agree. Will cause less surprises (such as this one).
> That would involve tweaking make_absolute_path() I guess?

Hm.. can we avoid converting work_tree to absolute path unless people explicitly set it (via --work-tree and GIT_WORK_TREE)? Basically worktree will be relative to cwd. Usually it's just ".". When people run commands outside worktree, it's the relative "cwd/to/worktree". I'm wondering if we can just avoid the use of make_absolute_path() completely in get_git_work_tree()..

> Note that your patch is the right thing to do either case, i.e. with or
> without such a change to make_absolute_path(), as the function is used to
> set up the return value from get_git_work_tree().  Anything we compare
> with it should have passed make_absolute_path() at least once.

Yes, I think that should that be done inside normalize_path_copy(), not prefix_path().

>>  setup.c |   11 +++++++----
>>  1 files changed, 7 insertions(+), 4 deletions(-)

Also Carlo, tests should be good for illustration and regression purposes. I know you described in detail in another mail. But mails tend to get lost.

-- 
Duy

← back to recent threads