Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Martin Erik Werner <martinerikwerner@gmail.com>]

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
> <martinerikwerner@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/b	c/
 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 <martinerikwerner@gmail.com>