Jeff King [off-list ref] writes:
quoted hunk
On Thu, Dec 20, 2012 at 08:28:57AM +0700, Nguyen Thai Ngoc Duy wrote:
quoted
quoted
So I think this is a nice, simple approach for sites that want it, and
noglob magic can come later (and will not be any harder to implement as
a result of this patch).
Any chance to make use of nd/pathspec-wildcard? It changes the same
code path in match_one. If you base on top of nd/pathspec-wildcard,
all you have to do is assign nowildcard_len to len (i.e. no wildcard
part).
I'd rather keep it separate for now. One, just because they really are
independent topics, and two, because I am actually back-porting it for
GitHub (we are fairly conservative about upgrading our backend git
versions, as most of the interesting stuff happens on the client side; I
cherry-pick critical patches with no regard to the release cycle).
And the resolution is pretty trivial, too. It looks like this:
diff --cc dir.c
index 5c0e5f6,03ff36b..81cb439
@@@ -1456,14 -1433,10 +1460,18 @@@ int init_pathspec(struct pathspec *path
item->match = path;
item->len = strlen(path);
- item->nowildcard_len = simple_length(path);
- item->use_wildcard = !limit_pathspec_to_literal() &&
- !no_wildcard(path);
- if (item->use_wildcard)
- pathspec->has_wildcard = 1;
+ item->flags = 0;
- if (item->nowildcard_len < item->len) {
- pathspec->has_wildcard = 1;
- if (path[item->nowildcard_len] == '*' &&
- no_wildcard(path + item->nowildcard_len + 1))
- item->flags |= PATHSPEC_ONESTAR;
++ if (limit_pathspec_to_literal())
++ item->nowildcard_len = item->len;
++ else {
++ item->nowildcard_len = simple_length(path);
++ if (item->nowildcard_len < item->len) {
++ pathspec->has_wildcard = 1;
++ if (path[item->nowildcard_len] == '*' &&
++ no_wildcard(path + item->nowildcard_len + 1))
++ item->flags |= PATHSPEC_ONESTAR;
++ }
+ }
}
qsort(pathspec->items, pathspec->nr,
Hmph. I thought that returning the length without any "stop at glob
special" trick from simple_length() would be a simpler resolution.
That is what is queued at the tip of 'pu', anyway.
On Wed, Dec 19, 2012 at 07:51:16PM -0800, Junio C Hamano wrote:
quoted
++ if (limit_pathspec_to_literal())
++ item->nowildcard_len = item->len;
++ else {
++ item->nowildcard_len = simple_length(path);
++ if (item->nowildcard_len < item->len) {
++ pathspec->has_wildcard = 1;
++ if (path[item->nowildcard_len] == '*' &&
++ no_wildcard(path + item->nowildcard_len + 1))
++ item->flags |= PATHSPEC_ONESTAR;
++ }
+ }
Hmph. I thought that returning the length without any "stop at glob
special" trick from simple_length() would be a simpler resolution.
That is what is queued at the tip of 'pu', anyway.
I don't think we can make a change in simple_length. It gets used not
only for pathspecs, but also for parsing exclude patterns, which I do
not think should be affected by this option.
-Peff
On Wed, Dec 19, 2012 at 10:55:43PM -0500, Jeff King wrote:
On Wed, Dec 19, 2012 at 07:51:16PM -0800, Junio C Hamano wrote:
quoted
quoted
++ if (limit_pathspec_to_literal())
++ item->nowildcard_len = item->len;
++ else {
++ item->nowildcard_len = simple_length(path);
++ if (item->nowildcard_len < item->len) {
++ pathspec->has_wildcard = 1;
++ if (path[item->nowildcard_len] == '*' &&
++ no_wildcard(path + item->nowildcard_len + 1))
++ item->flags |= PATHSPEC_ONESTAR;
++ }
+ }
Hmph. I thought that returning the length without any "stop at glob
special" trick from simple_length() would be a simpler resolution.
That is what is queued at the tip of 'pu', anyway.
I don't think we can make a change in simple_length. It gets used not
only for pathspecs, but also for parsing exclude patterns, which I do
not think should be affected by this option.
Our test suite wouldn't catch such a misfeature, of course, because the
feature is not turned on by default. But I found it instructive to run
all of the tests with GIT_LITERAL_PATHSPECS on. There are failures, of
course, but by inspecting each failure you can see that it is an
intended effect of the patch (i.e., each tries to use a wildcard
pathspec, which no longer works).
When you suggested changing common_prefix, I ran such a test both with
and without the change[1] and confirmed that it did not change the set
of failure sites. I did not try it, but I suspect running such a test
with the tip of pu would reveal new failures in the .gitignore tests.
-Peff
[1] This is in addition to reading and reasoning about the code, of
course. I would not consider this a very robust form of testing,
though a test failure which cannot be easily explained would be a
good starting point for investigation.
On Wed, Dec 19, 2012 at 11:06:02PM -0500, Jeff King wrote:
quoted
I don't think we can make a change in simple_length. It gets used not
only for pathspecs, but also for parsing exclude patterns, which I do
not think should be affected by this option.
Our test suite wouldn't catch such a misfeature, of course, because the
feature is not turned on by default. But I found it instructive to run
all of the tests with GIT_LITERAL_PATHSPECS on. There are failures, of
course, but by inspecting each failure you can see that it is an
intended effect of the patch (i.e., each tries to use a wildcard
pathspec, which no longer works).
When you suggested changing common_prefix, I ran such a test both with
and without the change[1] and confirmed that it did not change the set
of failure sites. I did not try it, but I suspect running such a test
with the tip of pu would reveal new failures in the .gitignore tests.
I just tried it, and indeed, running the test suite with this patch:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 256f1c6..1c43593 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -102,6 +102,9 @@ export EDITOR
export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
export EDITOR
+GIT_LITERAL_PATHSPECS=1
+export GIT_LITERAL_PATHSPECS
+
# Add libc MALLOC and MALLOC_PERTURB test
# only if we are not executing the test with valgrind
if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
produces many more failures on "pu" than it does on jk/pathspec-literal.
-Peff