Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic

Junio C Hamano [off-list ref] writes:

Duy Nguyen [off-list ref] writes:

quoted

quoted

I still can't reproduce it. But I think I found a bug that
miscalculates prefix length from absolute paths. Does this "fix" your
test?
 ...

Nope, that one could cause more crashes. Try this

-- 8< --

diff --git a/setup.c b/setup.c
index 3584f22..3d8eb97 100644
--- a/setup.c
+++ b/setup.c

@@ -14,6 +14,8 @@ char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
 		const char *temp = real_path(path);
 		sanitized = xmalloc(len + strlen(temp) + 1);
 		strcpy(sanitized, temp);
+		if (p_len)
+			*p_len = 0;

Yes, this one seems to. "$(pwd)/../src" was not handled correctly.

The callchain to this locaiton would look like

	parse_pathspec() with prefix="docs/", prefixlen set to 5
        -> prefix_pathspec(), &prefixlen passed down
          -> prefix_path_gently(), p_len points at the above prefixlen
	     your "this should fix" patch sets *p_len to 0,
             original leaves *p_len as 5.
             -> normalize_path_copy_len() with p_len
	        *p_len is used here.

Why could the test pass for you without it?  It doesn't look like a
bug that depended on uninitialized memory or something from the
above observation.

The change made to prefix_path_gently() in this series is beyond
"disgusting", especially with the above fix-up.

Sometimes it uses the original "len", sometimes it uses the fixed-up
*p_len (e.g. passes it down to normalize_path_copy_len()), and lets
normalize_path_copy_len() further update it, and thenit makes the
caller use the updated *p_len.

Does the caller know what the value in *p_len _mean_ after this
function returns?  Can it afford to lose the original length of the
prefix it saved in a variable, without getting confused?

I think any change that turns a value-passed argument in the
existing code into modifiable pointer-to-variable in this series
should add in-code comment to describe what the variable mean upon
entry and after return, just like normalize_path_copy_len() that was
built out of the original normalize_path_copy().  I didn't look if
there are many others, or if this is the only one that is tricky. it
is tricky that even the original author of the patch got it wrong
X-<.

On Thu, Mar 21, 2013 at 10:50:02AM -0700, Junio C Hamano wrote:

quoted

Why could the test pass for you without it?  It doesn't look like a
bug that depended on uninitialized memory or something from the
above observation.

It depends on uninitialized memory. For absolute paths, prefix is
useless and I should have set the useful prefix length to zero, but I
did not. Later in prefix_pathspec, I rely on this value to set
nowildcard_len without checking if it's sane. The actual pathspec
after prefix_pathspec is "src" (length of 3) but nowildcard_len is 5.

In common_prefix_len(), I use nowildcard_len without sanity checks. So
the code examines 's', 'r', 'c', '\0', '<random>'. In my case,
'<random>' has never been '/'. I guess yours is '/' (which leads to
wrong common prefix length).

I've added an assert() to make sure nowildcard_len and prefix have
sane values before exiting prefix_pathspec. This assert() chokes at
t7300.8 for me.

The change made to prefix_path_gently() in this series is beyond
"disgusting", especially with the above fix-up.

Sometimes it uses the original "len", sometimes it uses the fixed-up
*p_len (e.g. passes it down to normalize_path_copy_len()), and lets
normalize_path_copy_len() further update it, and thenit makes the
caller use the updated *p_len.

Does the caller know what the value in *p_len _mean_ after this
function returns?  Can it afford to lose the original length of the
prefix it saved in a variable, without getting confused?

I think any change that turns a value-passed argument in the
existing code into modifiable pointer-to-variable in this series
should add in-code comment to describe what the variable mean upon
entry and after return, just like normalize_path_copy_len() that was
built out of the original normalize_path_copy().  I didn't look if
there are many others, or if this is the only one that is tricky. it
is tricky that even the original author of the patch got it wrong
X-<.

The author of the patch totally forgot that prefix has nothing to do
with prefix. How about this? The prefix length is passed as value as
before. A separate pointer is for passing back the actual prefix
length. You can pull the actual patch from

https://github.com/pclouds/git parse-pathspec

which also includes all document bugs reported so far.

-- 8< --

diff --git a/pathspec.c b/pathspec.c
index 0771e48..126771c 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -205,7 +205,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 		match = xstrdup(copyfrom);
 		prefixlen = 0;
 	} else {
-		match = prefix_path_gently(prefix, &prefixlen, copyfrom);
+		match = prefix_path_gently(prefix, prefixlen, &prefixlen, copyfrom);
 		if (!match)
 			die("%s: '%s' is outside repository", elt, copyfrom);
 	}

@@ -284,6 +284,10 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 		    no_wildcard(item->match + item->nowildcard_len + 1))
 			item->flags |= PATHSPEC_ONESTAR;
 	}
+
+	/* sanity checks, pathspec matchers assume these are sane */
+	assert(item->nowildcard_len <= item->len &&
+	       item->prefix         <= item->len);
 	return magic;
 }
 

@@ -315,7 +319,7 @@ static void NORETURN unsupported_magic(const char *pattern,
 		n++;
 	}
 	/*
-	 * We may want to substitue "this command" with a command
+	 * We may want to substitute "this command" with a command
 	 * name. E.g. when add--interactive dies when running
 	 * "checkout -p"
 	 */

diff --git a/setup.c b/setup.c
index e59146b..6cf2bc6 100644
--- a/setup.c
+++ b/setup.c

@@ -5,24 +5,37 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
-char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
+/*
+ * 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
+ *
+ *  foo          -> sub1/sub2/foo  (full prefix)
+ *  ../foo       -> sub1/foo       (remaining prefix is sub1/)
+ *  ../../bar    -> bar            (no remaining prefix)
+ *  ../../sub1/sub2/foo -> sub1/sub2/foo (but no remaining prefix)
+ *  `pwd`/../bar -> sub1/bar       (no remaining prefix)
+ */
+char *prefix_path_gently(const char *prefix, int len,
+			 int *remaining_prefix, const char *path)
 {
 	const char *orig = path;
 	char *sanitized;
-	int len = *p_len;
 	if (is_absolute_path(orig)) {
 		const char *temp = real_path(path);
 		sanitized = xmalloc(len + strlen(temp) + 1);
 		strcpy(sanitized, temp);
-		if (p_len)
-			*p_len = 0;
+		if (remaining_prefix)
+			*remaining_prefix = 0;
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
 			memcpy(sanitized, prefix, len);
 		strcpy(sanitized + len, path);
+		if (remaining_prefix)
+			*remaining_prefix = len;
 	}
-	if (normalize_path_copy_len(sanitized, sanitized, p_len))
+	if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
 		goto error_out;
 	if (is_absolute_path(orig)) {
 		size_t root_len, len, total;

@@ -47,7 +60,7 @@ char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
 
 char *prefix_path(const char *prefix, int len, const char *path)
 {
-	char *r = prefix_path_gently(prefix, &len, path);
+	char *r = prefix_path_gently(prefix, len, NULL, path);
 	if (!r)
 		die("'%s' is outside repository", path);
 	return r;

@@ -56,7 +69,7 @@ char *prefix_path(const char *prefix, int len, const char *path)
 int path_inside_repo(const char *prefix, const char *path)
 {
 	int len = prefix ? strlen(prefix) : 0;
-	char *r = prefix_path_gently(prefix, &len, path);
+	char *r = prefix_path_gently(prefix, len, NULL, path);
 	if (r) {
 		free(r);
 		return 1;
-- 8< --

On Sat, Mar 23, 2013 at 10:13 AM, Duy Nguyen [off-list ref] wrote:

which also includes all document bugs reported so far.

s/all/fixes for all/
-- 
Duy

On Fri, Mar 22, 2013 at 11:13 PM, Duy Nguyen [off-list ref] wrote:

s/still remains/still remaining/
...or...
s/still remains/which still remains/
...or...
s/still remains/which remains/