Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
Ah indeed, this should catch that:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
- else
+ else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
/* work tree is the root, or the whole path */
memmove(path, path + wtlen, len - wtlen + 1);
+ else
+ return -1;
return 0;
}
path0 = path;
Is it worth adding a test for this as well?:
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index f6f378b..05d3366 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -201,6 +201,10 @@ test_expect_success 'prefix_path works with only absolute
path to work tree' '
test_cmp expected actual
'
+test_expect_success 'prefix_path rejects absolute path to dir with same
beginning as work tree' '
+ test_must_fail test-path-utils prefix_path prefix $(pwd)a
+'
+
relative_path /foo/a/b/c/ /foo/a/b/ c/
relative_path /foo/a/b/c/ /foo/a/bc/
relative_path /foo/a//b//c////foo/a/b//c/ POSIX
+ path0 = path;
+ path += offset_1st_component(path);
+
+ /* check each level */
+ while (*path != '\0') {
+ path++;
To me it looks like we could write
for (; *path; path++) {
or even
for (path += offset_1st_component(path); *path; path++) {
but it's personal taste..
Yeah, I think aesthetically I don't like cramming too much into the for loop:
for (path += offset_1st_component(path0) + 1; *path; path++) {
neither leaving the init expression unused. So as long as it's just personal
taste I think I'll stick with the current while loop format. But I'll exchange
(*path == '\0') for (*path) though.
--
Martin Erik Werner martinerikwer...@gmail.com
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
Martin Erik Werner martinerikwer...@gmail.com writes:
On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
Ah indeed, this should catch that:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
- else
+ else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
Is wtlen guaranteed to be nonzero?
--
David Kastrup
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On 2014-02-02 12.21, David Kastrup wrote:
Martin Erik Werner martinerikwer...@gmail.com writes:
On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
Ah indeed, this should catch that:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
-else
+else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
Is wtlen guaranteed to be nonzero?
Another comment:
The raw comparison with '/' is probably working well on all
POSIX/Linux/Unix systems.
To be more portable, the macro
is_dir_sep()
can be used:
if (is_dir_sep(path[wtlen]))
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 02, 2014 at 12:37:16PM +0100, Torsten Bögershausen wrote:
On 2014-02-02 12.21, David Kastrup wrote:
Martin Erik Werner martinerikwer...@gmail.com writes:
On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
Ah indeed, this should catch that:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
- else
+ else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
Is wtlen guaranteed to be nonzero?
Hmm, am I incorrect in thinking
if (!work_tree)
takes care of that?
Another comment:
The raw comparison with '/' is probably working well on all
POSIX/Linux/Unix systems.
To be more portable, the macro
is_dir_sep()
can be used:
if (is_dir_sep(path[wtlen]))
Since the path is already normalized by 'normalize_path_copy_len' which
seems to guarantee '/'-separation, I have assumed that this was
unnecessary?
--
Martin Erik Werner martinerikwer...@gmail.com
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 2, 2014 at 6:13 PM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
- else
+ else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
/* work tree is the root, or the whole path */
memmove(path, path + wtlen, len - wtlen + 1);
+ else
+ return -1;
No, you can't return here. /abc/defghi might actually be a symlink
to /abc/def. If it does not match, let the following loop handle it.
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
Another comment: The raw comparison with '/' is probably working well on all POSIX/Linux/Unix systems. To be more portable, the macro is_dir_sep() can be used: if (is_dir_sep(path[wtlen])) Since the path is already normalized by 'normalize_path_copy_len' which seems to guarantee '/'-separation, I have assumed that this was unnecessary? So true, sorry for the noise. -- 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
[PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the in-repo part).
The path being exactly equal to the work tree is handled separately,
since then there is no directory separator between the work tree and
in-repo part.
This function is currently only intended for use in
'prefix_path_gently'.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 57 +
1 file changed, 57 insertions(+)
diff --git a/setup.c b/setup.c
index 5432a31..e938785 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,63 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)
+{
+ size_t len;
+ size_t wtlen;
+ char *path0;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ wtlen = strlen(work_tree);
+ len = strlen(path);
+
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
+ path0 = path;
+ path += offset_1st_component(path);
+
+ /* check each level */
+ while (*path != '\0') {
+ path++;
+ if (*path == '/') {
+ *path = '\0';
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ memmove(path0, path + 1, len - (path - path0));
+ return 0;
+ }
+ *path = '/';
+ }
+ }
+
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
+
+ return -1;
+}
+
+/*
* Normalize path, prepending the prefix for relative paths. If
* remaining_prefix is not NULL, return the actual prefix still
* remains in the path. For example, prefix = sub1/sub2/ and path is
--
1.8.5.2
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
+ path0 = path;
+ path += offset_1st_component(path);
+
+ /* check each level */
+ while (*path != '\0') {
+ path++;
To me it looks like we could write
for (; *path; path++) {
or even
for (path += offset_1st_component(path); *path; path++) {
but it's personal taste..
+ if (*path == '/') {
+ *path = '\0';
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ memmove(path0, path + 1, len - (path -
path0));
+ return 0;
+ }
+ *path = '/';
+ }
+ }
+
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
I think this is already handled by the check if work tree is already
the prefix block.
+
+ return -1;
+}
+
+/*
* Normalize path, prepending the prefix for relative paths. If
* remaining_prefix is not NULL, return the actual prefix still
* remains in the path. For example, prefix = sub1/sub2/ and path is
--
1.8.5.2
--
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
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 2, 2014 at 9:19 AM, Duy Nguyen pclo...@gmail.com wrote:
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
I think this is already handled by the check if work tree is already
the prefix block.
No, the check if.. block does not do real_path() so we still need it
here. Sorry for the noise.
--
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
