Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
Martin Erik Werner martinerikwer...@gmail.com writes:
In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
I agree 100% with this reasoning (modulo s/symliks/symlinks/).
As to the implementation, it looks a bit overly complicated,
though. I haven't tried writing the same myself, but
* I suspect that strbuf would help simplifying the allocation and
deallocation;
* Also I suspect that use of string-list to split and then join is
making the code unnecessarily complex. In Python/Perl, that would
be a normal approach, but in C, it would be a lot simpler if you
prepare a writable temporary in wtpart[], walk from left to right
finding '/' and replacing it temporarily with NUL to terminate in
order to check with real_path(), restore the NUL back to '/' to
check deeper, and rinse and repeat.
Having said that, I am not absolutely sure if the repeated
calls to real_path() are doing the right thing, though ;-)
diff --git a/setup.c b/setup.c
index 5432a31..0789a96 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ int i, match;
+ size_t wtpartlen;
+ char *npath, *wtpart;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree)
+ return NULL;
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+
+ string_list_split(list, npath, '/', -1);
+ wtpart = xmalloc(strlen(npath) + 1);
+ i = 0;
+ match = 0;
+ strcpy(wtpart, list.items[i++].string);
+ strcat(wtpart, /);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ } else {
+ while (i list.nr) {
+ strcat(wtpart, list.items[i++].string);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ break;
+ }
+ strcat(wtpart, /);
+ }
+ }
+ string_list_clear(list, 0);
+ if (!match) {
+ free(npath);
+ free(wtpart);
+ return NULL;
+ }
+
+ wtpartlen = strlen(wtpart);
+ sanitized = xmalloc(strlen(npath) - wtpartlen);
+ strcpy(sanitized, npath + wtpartlen + 1);
+ free(npath);
+ free(wtpart);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on
symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '
[PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
The prefix_path_gently() function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify prefix_path_gently() to first normalize the path in order to
make sure path levels are separated by '/', then use this separator to
check the real path of each level of the path until it has found the
length that corresponds to the work tree.
For absolute paths, the function did not, nor does now do, any actual
prefixing, hence we simply remove the path corresponding to the work
tree and return the remaining in-tree part of the path.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 64 +++
t/t0060-path-utils.sh | 2 +-
2 files changed, 45 insertions(+), 21 deletions(-)
diff --git a/setup.c b/setup.c
index 5432a31..0789a96 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ int i, match;
+ size_t wtpartlen;
+ char *npath, *wtpart;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree)
+ return NULL;
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+
+ string_list_split(list, npath, '/', -1);
+ wtpart = xmalloc(strlen(npath) + 1);
+ i = 0;
+ match = 0;
+ strcpy(wtpart, list.items[i++].string);
+ strcat(wtpart, /);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ } else {
+ while (i list.nr) {
+ strcat(wtpart, list.items[i++].string);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ break;
+ }
+ strcat(wtpart, /);
+ }
+ }
+ string_list_clear(list, 0);
+ if (!match) {
+ free(npath);
+ free(wtpart);
+ return NULL;
+ }
+
+ wtpartlen = strlen(wtpart);
+ sanitized = xmalloc(strlen(npath) - wtpartlen);
+ strcpy(sanitized, npath + wtpartlen + 1);
+ free(npath);
+ free(wtpart);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with work tree
Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
On Mon, Jan 27, 2014 at 7:07 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
diff --git a/setup.c b/setup.c
index 5432a31..0789a96 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ int i, match;
+ size_t wtpartlen;
+ char *npath, *wtpart;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree)
+ return NULL;
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+
+ string_list_split(list, npath, '/', -1);
+ wtpart = xmalloc(strlen(npath) + 1);
+ i = 0;
+ match = 0;
+ strcpy(wtpart, list.items[i++].string);
+ strcat(wtpart, /);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ } else {
Could we remove this part and let the while loop handle the first path
component too? The only difference I see is if this code matches, we
have a trailing slash, while the while loop does not have a trailing
slash in wtpart.
+ while (i list.nr) {
+ strcat(wtpart, list.items[i++].string);
+ if (strcmp(real_path(wtpart), work_tree) ==
0) {
+ match = 1;
+ break;
+ }
+ strcat(wtpart, /);
+ }
+ }
+ string_list_clear(list, 0);
+ if (!match) {
+ free(npath);
+ free(wtpart);
+ return NULL;
+ }
+
+ wtpartlen = strlen(wtpart);
+ sanitized = xmalloc(strlen(npath) - wtpartlen);
+ strcpy(sanitized, npath + wtpartlen + 1);
This + 1 is to ignore '/', isn't it? If so we should not do if match
is set 1 outside while loop.
+ free(npath);
+ free(wtpart);
All this new code looks long enough to be a separate function with a
meaningful name. And we could travese through each path component in
npath without wtpart (replacing '/' with '\0' to terminate the string
temporarily for real_path()). But it's up to you. Whichever way is
easier to read to you.
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0'
sanitized[len] != '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
