Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
Duy Nguyen pclo...@gmail.com writes: Btw I'm thinking of extending pathspec magic syntax a bit to allow path completion. Right now the user has to write git log -- :-Documentation which does not play well with path completion. I'm thinking of accepting git log -- :- Documentation Please don't. That does not help our users, but actively harm them. They have to stop and wonder why a single pathspec is spelled as two tokens on the command line of some other people. Doing that stupidity only to help those who polish the tool (namely, bash completion) to be lazy is doubly wrong (in the meantime, the users can type your second variant and then edit the result). For the same reason why I do not think rewriting echo hello, world! to echo hello, world- only to work around a pitfall of a particular tool (namely bash) makes any sense, I do not think it makes sense to make _our_ tool inconsistent by using !excluded in the files (and --exclude) and -not this pattern only here. + if (nr_exclude == n) + die(_(There is nothing to exclude from by :(exclude) patterns.\n + Perhaps you forgot to add either ':/' or '.' ?)); ;-). Hey it was originally not there,... I am not objecting. I noticed it and was commending on it as a nice touch ;-) + /* + * # | positive | negative | result + * -+--+--+--- + * 1..4 | -1 |* | -1 + * 5..8 |0 |* | 0 + * 9 |1 | -1 | 1 + * 10 |1 |0 | 1 + * 11 |1 |1 | 0 + * 12 |1 |2 | 0 + * 13 |2 | -1 | 2 + * 14 |2 |0 | 2 + * 15 |2 |1 | 0 + * 16 |2 |2 | -1 + */ Not sure what this case-table means... Sorry, because tree_entry_interesting_1() returns more than match or not, we need to combine the result from positive pathspec with the negative one to correctly handle all_not_interesting and all_interesting. This table sums it up. I'll add more explanation in the next patch. I managed to have guessed what the three columns on the right meant; I was wondering about the meaning of the # column and where it is defined/explained. -- 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] Support pathspec magic :(exclude) and its short form :-
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
This is yet another stab at the negative pathspec thing. It's not
ready yet (there are a few XXXs) but I could use some feedback
regarding the interface, or the behavior. It looks better this time
now that pathspec magic is supported (or maybe I'm just biased).
For :(glob) or :(icase) you're more likely to enable it for all
pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed
more often (it does not make sense to add --exclude-pathspecs to
exclude everything), which is why I add the short form for it.
We don't have many options that say negative in short form.
Either '!', '-' or '~'. '!' is already used for bash history expansion.
~ looks more like $HOME expansion. Which left me '-'.
I agree with your decision to reject ~, but !not-this-pattern is
very much consistent with the patterns used in .gitignore (and the
--exclude pattern option), so avoiding ! and introducing an
inconsistent - only to appease bash leaves somewhat a funny taste
in my mouth.
Documentation/glossary-content.txt | 5
builtin/add.c | 5 +++-
dir.c | 50 +++-
pathspec.c | 9 ++-
pathspec.h | 4 ++-
tree-walk.c| 52
+++---
6 files changed, 112 insertions(+), 13 deletions(-)
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index e470661..f7d7d8c 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -377,6 +377,11 @@ full pathname may have special meaning:
- Other consecutive asterisks are considered invalid.
+
Glob magic is incompatible with literal magic.
+
+exclude `-`;;
+ After a path matches any non-exclude pathspec, it will be run
+ through all exclude pathspec. If it matches, the path is
+ ignored.
--
+
Currently only the slash `/` is recognized as the magic signature,
No longer, no? magic signature is a non-alphanumeric that follows
the ':' introducer, as opposed to magic words that are in :(...).
diff --git a/builtin/add.c b/builtin/add.c
index 226f758..0df73ae 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
PATHSPEC_FROMTOP |
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
-PATHSPEC_ICASE);
+PATHSPEC_ICASE |
+PATHSPEC_EXCLUDE);
for (i = 0; i pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
+ if (pathspec.items[i].magic PATHSPEC_EXCLUDE)
+ continue;
if (!seen[i]
((pathspec.items[i].magic
(PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
So git add ':(exclude)junk/' '*.c' to add all .c files except for
the ones in the 'junk/' directory may find that ':(exclude)junk/'
matched nothing (because there is no .c file in there), and that is
not an error. It makes sense to me.
diff --git a/dir.c b/dir.c
index 23b6de4..e2df82f 100644
--- a/dir.c
+++ b/dir.c
@@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec
*pathspec)
PATHSPEC_MAXDEPTH |
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
-PATHSPEC_ICASE);
+PATHSPEC_ICASE |
+PATHSPEC_EXCLUDE);
for (n = 0; n pathspec-nr; n++) {
size_t i = 0, len = 0, item_len;
+ if (pathspec-items[n].magic PATHSPEC_EXCLUDE)
+ continue;
if (pathspec-items[n].magic PATHSPEC_ICASE)
item_len = pathspec-items[n].prefix;
else
Likewise. Exclusion does not participate in the early culling with
the common prefix.
@@ -1375,11 +1407,17 @@ int read_directory(struct dir_struct *dir, const char
*path, int len, const stru
PATHSPEC_MAXDEPTH |
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
-PATHSPEC_ICASE);
+PATHSPEC_ICASE |
+PATHSPEC_EXCLUDE);
if (has_symlink_leading_path(path, len))
return dir-nr;
+ /*
+ * XXX: exclude patterns are treated like positive ones in
+ * create_simplify! This is not wrong, but may make path
+ * filtering less efficient.
+ */
True, but git add ':(exclude)a/b/c' a/b would not suffer. And
those who do git add ':(exclude)a/b'
Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
On Thu, Nov 21, 2013 at 6:48 AM, Junio C Hamano gits...@pobox.com wrote:
We don't have many options that say negative in short form.
Either '!', '-' or '~'. '!' is already used for bash history expansion.
~ looks more like $HOME expansion. Which left me '-'.
I agree with your decision to reject ~, but !not-this-pattern is
very much consistent with the patterns used in .gitignore (and the
--exclude pattern option), so avoiding ! and introducing an
inconsistent - only to appease bash leaves somewhat a funny taste
in my mouth.
The thing about '!' is it's history expansion in bash and I suspect
not many people are aware of it. So git log -- :!something may
recall the last command that has something in it, which is confusing
for those new people and may potentially be dangerous (multiple
command in one line, separated by semicolon). Compared to :git log --
(exclude)somethign the worst that could happen is a syntax error
message from bash.
Other than that I'm fine with '!' being the shortcut.
Btw I'm thinking of extending pathspec magic syntax a bit to allow
path completion. Right now the user has to write
git log -- :-Documentation
which does not play well with path completion. I'm thinking of accepting
git log -- :- Documentation
In other words, if there's no path (or pattern) component after the
magic, then the next argument must contain the path. This enables path
completion and I haven't seen any drawbacks yet..
@@ -427,6 +430,10 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec-magic |= item[i].magic;
}
+ if (nr_exclude == n)
+ die(_(There is nothing to exclude from by :(exclude)
patterns.\n
+ Perhaps you forgot to add either ':/' or '.' ?));
;-).
Hey it was originally not there, then I made a mistake of typing git
log -- :-po and wondered why it shows nothing. Intuitively, if git
log shows every path, then git log -- :-po should show every path
except 'po' and the user should not be required to type git log -- :/
:-po. parse_pathspec() can do that, but it's more work and I'm lazy
so I push that back to the user until they scream :)
+enum interesting tree_entry_interesting(const struct name_entry *entry,
+ struct strbuf *base, int base_offset,
+ const struct pathspec *ps)
+{
+ enum interesting positive, negative;
+ positive = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
+
+ /*
+ * # | positive | negative | result
+ * -+--+--+---
+ * 1..4 | -1 |* | -1
+ * 5..8 |0 |* | 0
+ * 9 |1 | -1 | 1
+ * 10 |1 |0 | 1
+ * 11 |1 |1 | 0
+ * 12 |1 |2 | 0
+ * 13 |2 | -1 | 2
+ * 14 |2 |0 | 2
+ * 15 |2 |1 | 0
+ * 16 |2 |2 | -1
+ */
Not sure what this case-table means...
Sorry, because tree_entry_interesting_1() returns more than match or
not, we need to combine the result from positive pathspec with the
negative one to correctly handle all_not_interesting and
all_interesting. This table sums it up. I'll add more explanation in
the next patch.
--
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
[PATCH] Support pathspec magic :(exclude) and its short form :-
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
This is yet another stab at the negative pathspec thing. It's not
ready yet (there are a few XXXs) but I could use some feedback
regarding the interface, or the behavior. It looks better this time
now that pathspec magic is supported (or maybe I'm just biased).
For :(glob) or :(icase) you're more likely to enable it for all
pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed
more often (it does not make sense to add --exclude-pathspecs to
exclude everything), which is why I add the short form for it.
We don't have many options that say negative in short form.
Either '!', '-' or '~'. '!' is already used for bash history expansion.
~ looks more like $HOME expansion. Which left me '-'.
Documentation/glossary-content.txt | 5
builtin/add.c | 5 +++-
dir.c | 50 +++-
pathspec.c | 9 ++-
pathspec.h | 4 ++-
tree-walk.c| 52 +++---
6 files changed, 112 insertions(+), 13 deletions(-)
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index e470661..f7d7d8c 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -377,6 +377,11 @@ full pathname may have special meaning:
- Other consecutive asterisks are considered invalid.
+
Glob magic is incompatible with literal magic.
+
+exclude `-`;;
+ After a path matches any non-exclude pathspec, it will be run
+ through all exclude pathspec. If it matches, the path is
+ ignored.
--
+
Currently only the slash `/` is recognized as the magic signature,
diff --git a/builtin/add.c b/builtin/add.c
index 226f758..0df73ae 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
PATHSPEC_FROMTOP |
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
- PATHSPEC_ICASE);
+ PATHSPEC_ICASE |
+ PATHSPEC_EXCLUDE);
for (i = 0; i pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
+ if (pathspec.items[i].magic PATHSPEC_EXCLUDE)
+ continue;
if (!seen[i]
((pathspec.items[i].magic
(PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
diff --git a/dir.c b/dir.c
index 23b6de4..e2df82f 100644
--- a/dir.c
+++ b/dir.c
@@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec
*pathspec)
PATHSPEC_MAXDEPTH |
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
- PATHSPEC_ICASE);
+ PATHSPEC_ICASE |
+ PATHSPEC_EXCLUDE);
for (n = 0; n pathspec-nr; n++) {
size_t i = 0, len = 0, item_len;
+ if (pathspec-items[n].magic PATHSPEC_EXCLUDE)
+ continue;
if (pathspec-items[n].magic PATHSPEC_ICASE)
item_len = pathspec-items[n].prefix;
else
@@ -279,9 +282,10 @@ static int match_pathspec_item(const struct pathspec_item
*item, int prefix,
* pathspec did not match any names, which could indicate that the
* user mistyped the nth pathspec.
*/
-int match_pathspec_depth(const struct pathspec *ps,
-const char *name, int namelen,
-int prefix, char *seen)
+static int match_pathspec_depth_1(const struct pathspec *ps,
+ const char *name, int namelen,
+ int prefix, char *seen,
+ int exclude)
{
int i, retval = 0;
@@ -290,7 +294,8 @@ int match_pathspec_depth(const struct pathspec *ps,
PATHSPEC_MAXDEPTH |
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
- PATHSPEC_ICASE);
+ PATHSPEC_ICASE |
+ PATHSPEC_EXCLUDE);
if (!ps-nr) {
if (!ps-recursive ||
@@ -309,6 +314,11 @@ int match_pathspec_depth(const struct pathspec *ps,
for (i = ps-nr - 1; i = 0; i--) {
int how;
+
+ if ((!excludeps-items[i].magic PATHSPEC_EXCLUDE) ||
+ ( exclude !(ps-items[i].magic PATHSPEC_EXCLUDE)))
+ continue;
+
if (seen seen[i] == MATCHED_EXACTLY)
continue;
how = match_pathspec_item(ps-items+i, prefix, name, namelen);
@@ -327,6 +337,16 @@ int match_pathspec_depth(const struct pathspec *ps,
