Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 23, 2013 at 10:13 AM, Duy Nguyen pclo...@gmail.com wrote: which also includes all document bugs reported so far. s/all/fixes for all/ Slurped from your github repository and didn't find anything questionable relative to the original series that was posted here. Thanks. Will requeue. -- 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 v2 00/45] parse_pathspec and :(glob) magic
On Fri, Mar 22, 2013 at 11:13 PM, Duy Nguyen pclo...@gmail.com wrote: 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 s/still remains/still remaining/ ...or... s/still remains/which still remains/ ...or... s/still remains/which remains/ -- 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 v2 00/45] parse_pathspec and :(glob) magic
On Thu, Mar 21, 2013 at 10:50:02AM -0700, Junio C Hamano wrote:
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 +
Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic
On Sat, Mar 23, 2013 at 10:13 AM, Duy Nguyen pclo...@gmail.com wrote: which also includes all document bugs reported so far. s/all/fixes for all/ -- 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 v2 00/45] parse_pathspec and :(glob) magic
Duy Nguyen pclo...@gmail.com writes: 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. -- 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 v2 00/45] parse_pathspec and :(glob) magic
Junio C Hamano gits...@pobox.com writes: Duy Nguyen pclo...@gmail.com writes: 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-. -- 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 v2 00/45] parse_pathspec and :(glob) magic
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
Junio please pull the series from github [1]
[1] https://github.com/pclouds/git/commits/parse-pathspec
Please write it like this:
https://github.com/pclouds/git parse-pathspec
so that I can just say
git fetch that thing
git diff nd/magic-pathspecs FETCH_HEAD
I am still getting this out of 7300, though.
expecting success:
mkdir -p build docs
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so
would_clean=$(
cd docs
git clean -n $(pwd)/../src |
sed -n -e s|^Would remove ||p
)
test $would_clean = ../src/part3.c || {
echo OOps $would_clean
false
}
OOps
not ok 8 - git clean with absolute path
#
#
# mkdir -p build docs
# touch a.out src/part3.c docs/manual.txt obj.o build/lib.so
# would_clean=$(
# cd docs
# git clean -n $(pwd)/../src |
# sed -n -e s|^Would remove ||p
# )
# test $would_clean = ../src/part3.c || {
# echo OOps $would_clean
# false
# }
#
--
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 v2 00/45] parse_pathspec and :(glob) magic
On Wed, Mar 20, 2013 at 11:02:50AM -0700, Junio C Hamano wrote:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
Junio please pull the series from github [1]
[1] https://github.com/pclouds/git/commits/parse-pathspec
Please write it like this:
https://github.com/pclouds/git parse-pathspec
so that I can just say
git fetch that thing
git diff nd/magic-pathspecs FETCH_HEAD
OK
I am still getting this out of 7300, though.
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?
-- 8 --
diff --git a/setup.c b/setup.c
index 3584f22..6ae147a 100644
--- a/setup.c
+++ b/setup.c
@@ -14,6 +14,7 @@ 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);
+ *p_len = 0;
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
-- 8 --
--
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 v2 00/45] parse_pathspec and :(glob) magic
On Thu, Mar 21, 2013 at 12:33:26PM +0700, Duy Nguyen wrote:
I am still getting this out of 7300, though.
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?
-- 8 --
diff --git a/setup.c b/setup.c
index 3584f22..6ae147a 100644
--- a/setup.c
+++ b/setup.c
@@ -14,6 +14,7 @@ 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);
+ *p_len = 0;
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
-- 8 --
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;
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
-- 8 --
--
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
