[PATCH v1 23/45] check-ignore: convert to use parse_pathspec
check-ignore (at least the test suite) seems to rely on the pattern
order. PATHSPEC_KEEP_ORDER is introduced to explictly express this.
The lack of PATHSPEC_MAXDEPTH_VALID is sufficient because it's the
only flag that reorders pathspecs, but it's less obvious that way.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/check-ignore.c | 34 +-
pathspec.c | 6 +-
pathspec.h | 1 +
t/t0008-ignores.sh | 8
4 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 0240f99..6e55f06 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -53,14 +53,14 @@ static void output_exclude(const char *path, struct exclude
*exclude)
}
}
-static int check_ignore(const char *prefix, const char **pathspec)
+static int check_ignore(int argc, const char **argv, const char *prefix)
{
struct dir_struct dir;
- const char *path, *full_path;
char *seen;
int num_ignored = 0, dtype = DT_UNKNOWN, i;
struct path_exclude_check check;
struct exclude *exclude;
+ struct pathspec pathspec;
/* read_cache() is only necessary so we can watch out for submodules. */
if (read_cache() < 0)
@@ -70,31 +70,39 @@ static int check_ignore(const char *prefix, const char
**pathspec)
dir.flags |= DIR_COLLECT_IGNORED;
setup_standard_excludes(&dir);
- if (!pathspec || !*pathspec) {
+ if (!argc) {
if (!quiet)
fprintf(stderr, "no pathspec given.\n");
return 0;
}
+ /*
+* check-ignore just needs paths. Magic beyond :/ is really
+* irrelevant.
+*/
+ parse_pathspec(&pathspec,
+ PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
+ PATHSPEC_SYMLINK_LEADING_PATH |
+ PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE |
+ PATHSPEC_KEEP_ORDER,
+ prefix, argv);
+
path_exclude_check_init(&check, &dir);
/*
* look for pathspecs matching entries in the index, since these
* should not be ignored, in order to be consistent with
* 'git status', 'git add' etc.
*/
- seen = find_pathspecs_matching_against_index(pathspec);
- for (i = 0; pathspec[i]; i++) {
- path = pathspec[i];
- full_path = prefix_path(prefix, prefix
- ? strlen(prefix) : 0, path);
- full_path = check_path_for_gitlink(full_path);
- die_if_path_beyond_symlink(full_path, prefix);
+ seen = find_pathspecs_matching_against_index(pathspec.raw);
+ for (i = 0; i < pathspec.nr; i++) {
+ const char *full_path = pathspec.raw[i];
if (!seen[i]) {
exclude = last_exclude_matching_path(&check, full_path,
-1, &dtype);
if (exclude) {
if (!quiet)
- output_exclude(path, exclude);
+
output_exclude(pathspec.items[i].original,
+ exclude);
num_ignored++;
}
}
@@ -129,7 +137,7 @@ static int check_ignore_stdin_paths(const char *prefix)
}
ALLOC_GROW(pathspec, nr + 1, alloc);
pathspec[nr] = NULL;
- num_ignored = check_ignore(prefix, (const char **)pathspec);
+ num_ignored = check_ignore(nr, (const char **)pathspec, prefix);
maybe_flush_or_die(stdout, "attribute to stdout");
strbuf_release(&buf);
strbuf_release(&nbuf);
@@ -165,7 +173,7 @@ int cmd_check_ignore(int argc, const char **argv, const
char *prefix)
if (stdin_paths) {
num_ignored = check_ignore_stdin_paths(prefix);
} else {
- num_ignored = check_ignore(prefix, argv);
+ num_ignored = check_ignore(argc, argv, prefix);
maybe_flush_or_die(stdout, "ignore to stdout");
}
diff --git a/pathspec.c b/pathspec.c
index 9a57c0c..f531038 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -368,9 +368,13 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->magic |= item[i].magic;
}
- if (pathspec->magic & PATHSPEC_MAXDEPTH)
+
+ if (pathspec->magic & PATHSPEC_MAXDEPTH) {
+ if (flags & PATHSPEC_KEEP_ORDER)
+ die("BUG: PATHSPEC_MAXDEPTH_VALID and
PATHSPEC_KEEP_ORDER are incompatible");
qsort(pathspec->items, pathspec->nr,
sizeof(struct pathspec_item), pathspec_item_cmp);
+ }
}
/*
diff --git a/pathspec.h b/pathspec.h
index ed5d3a6..44253c8 100644
--- a/pathspec.h
+++ b/
Re: [PATCH v1 23/45] check-ignore: convert to use parse_pathspec
On Fri, Mar 15, 2013 at 01:06:38PM +0700, Nguyễn Thái Ngọc Duy wrote:
> check-ignore (at least the test suite) seems to rely on the pattern
> order. PATHSPEC_KEEP_ORDER is introduced to explictly express this.
> The lack of PATHSPEC_MAXDEPTH_VALID is sufficient because it's the
> only flag that reorders pathspecs, but it's less obvious that way.
Sorry for the slow response - I only just noticed this today. (It
would be useful if any future patches to check-ignore Cc: me
explicitly, to catch my mail filters.)
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/check-ignore.c | 34 +-
> pathspec.c | 6 +-
> pathspec.h | 1 +
> t/t0008-ignores.sh | 8
> 4 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> index 0240f99..6e55f06 100644
> --- a/builtin/check-ignore.c
> +++ b/builtin/check-ignore.c
> @@ -53,14 +53,14 @@ static void output_exclude(const char *path, struct
> exclude *exclude)
> }
> }
>
> -static int check_ignore(const char *prefix, const char **pathspec)
> +static int check_ignore(int argc, const char **argv, const char *prefix)
> {
> struct dir_struct dir;
> - const char *path, *full_path;
> char *seen;
> int num_ignored = 0, dtype = DT_UNKNOWN, i;
> struct path_exclude_check check;
> struct exclude *exclude;
> + struct pathspec pathspec;
>
> /* read_cache() is only necessary so we can watch out for submodules. */
> if (read_cache() < 0)
> @@ -70,31 +70,39 @@ static int check_ignore(const char *prefix, const char
> **pathspec)
> dir.flags |= DIR_COLLECT_IGNORED;
> setup_standard_excludes(&dir);
>
> - if (!pathspec || !*pathspec) {
> + if (!argc) {
Is there a compelling reason for introducing argc as a new parameter
to check_ignore(), other than simplifying the above line? And why
rename the pathspec parameter to argv? Both these changes are
misleading AFAICS, since paths provided to check_ignore() can come
from sources other than CLI arguments (i.e. via --stdin).
The introduction of argc also makes it possible to invoke
check_ignore() with arguments which are not self-consistent.
I haven't been following your pathspec work, but FWIW the other
changes in this patch look reasonable at a glance.
Thanks,
Adam
--
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 v1 23/45] check-ignore: convert to use parse_pathspec
On Sat, Apr 13, 2013 at 1:03 AM, Adam Spiers wrote:
>> -static int check_ignore(const char *prefix, const char **pathspec)
>> +static int check_ignore(int argc, const char **argv, const char *prefix)
>> {
>> struct dir_struct dir;
>> - const char *path, *full_path;
>> char *seen;
>> int num_ignored = 0, dtype = DT_UNKNOWN, i;
>> struct path_exclude_check check;
>> struct exclude *exclude;
>> + struct pathspec pathspec;
>>
>> /* read_cache() is only necessary so we can watch out for submodules.
>> */
>> if (read_cache() < 0)
>> @@ -70,31 +70,39 @@ static int check_ignore(const char *prefix, const char
>> **pathspec)
>> dir.flags |= DIR_COLLECT_IGNORED;
>> setup_standard_excludes(&dir);
>>
>> - if (!pathspec || !*pathspec) {
>> + if (!argc) {
>
> Is there a compelling reason for introducing argc as a new parameter
> to check_ignore(), other than simplifying the above line? And why
> rename the pathspec parameter to argv? Both these changes are
> misleading AFAICS, since paths provided to check_ignore() can come
> from sources other than CLI arguments (i.e. via --stdin).
Because I introduced "struct pathspec pathspec;" I need to rename the
argument "pathspec" to something else. Maybe we could rename the
argument to "paths"?
> The introduction of argc also makes it possible to invoke
> check_ignore() with arguments which are not self-consistent.
This is the same problem with main() and other places that follow this
convention. But I don't mind dropping argc either.
--
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 v1 23/45] check-ignore: convert to use parse_pathspec
On Sat, Apr 13, 2013 at 09:09:33AM +1000, Duy Nguyen wrote:
> On Sat, Apr 13, 2013 at 1:03 AM, Adam Spiers wrote:
> >> -static int check_ignore(const char *prefix, const char **pathspec)
> >> +static int check_ignore(int argc, const char **argv, const char *prefix)
> >> {
> >> struct dir_struct dir;
> >> - const char *path, *full_path;
> >> char *seen;
> >> int num_ignored = 0, dtype = DT_UNKNOWN, i;
> >> struct path_exclude_check check;
> >> struct exclude *exclude;
> >> + struct pathspec pathspec;
> >>
> >> /* read_cache() is only necessary so we can watch out for
> >> submodules. */
> >> if (read_cache() < 0)
> >> @@ -70,31 +70,39 @@ static int check_ignore(const char *prefix, const char
> >> **pathspec)
> >> dir.flags |= DIR_COLLECT_IGNORED;
> >> setup_standard_excludes(&dir);
> >>
> >> - if (!pathspec || !*pathspec) {
> >> + if (!argc) {
> >
> > Is there a compelling reason for introducing argc as a new parameter
> > to check_ignore(), other than simplifying the above line? And why
> > rename the pathspec parameter to argv? Both these changes are
> > misleading AFAICS, since paths provided to check_ignore() can come
> > from sources other than CLI arguments (i.e. via --stdin).
>
> Because I introduced "struct pathspec pathspec;" I need to rename the
> argument "pathspec" to something else.
Ah, I see - that makes sense :-)
> Maybe we could rename the argument to "paths"?
Sounds fine to me.
> > The introduction of argc also makes it possible to invoke
> > check_ignore() with arguments which are not self-consistent.
>
> This is the same problem with main()
How could main() be invoked with argc inconsistent with argv?
> and other places that follow this convention. But I don't mind
> dropping argc either.
What is the reason for that convention? I'm willing to be persuaded
either way.
--
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 v1 23/45] check-ignore: convert to use parse_pathspec
On Mon, Apr 15, 2013 at 9:25 AM, Adam Spiers wrote: >> > The introduction of argc also makes it possible to invoke >> > check_ignore() with arguments which are not self-consistent. >> >> This is the same problem with main() > > How could main() be invoked with argc inconsistent with argv? The point is main's caller has to maintain the consistency. So do check_ignore's callers. -- 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 v1 23/45] check-ignore: convert to use parse_pathspec
On Mon, Apr 15, 2013 at 09:48:22AM +1000, Duy Nguyen wrote: > On Mon, Apr 15, 2013 at 9:25 AM, Adam Spiers wrote: > >> > The introduction of argc also makes it possible to invoke > >> > check_ignore() with arguments which are not self-consistent. > >> > >> This is the same problem with main() > > > > How could main() be invoked with argc inconsistent with argv? > > The point is main's caller has to maintain the consistency. So do > check_ignore's callers. But only the system runtime calls main(), right? So we can probably rely on it being called in a consistent manner ;-) -- 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
