Re: [PATCH 1/5] check-ignore: move setup into cmd_check_ignore()
On Thu, Apr 11, 2013 at 01:25:53AM -0400, Jeff King wrote: On Thu, Apr 11, 2013 at 02:59:31AM +0100, Adam Spiers wrote: -static int check_ignore(const char *prefix, const char **pathspec) +static int check_ignore(struct path_exclude_check check, + const char *prefix, const char **pathspec) Did you mean to pass the struct by value here? If it is truly a per-path value, shouldn't it be declared and initialized inside here? Otherwise you risk one invocation munging things that the struct points to, but the caller's copy does not know about the change. In particular, I see that the struct includes a strbuf. What happens when one invocation of check_ignore grows the strbuf, then returns? The copy of the struct in the caller will not know that the buffer it is pointing to is now bogus. -static int check_ignore_stdin_paths(const char *prefix) +static int check_ignore_stdin_paths(struct path_exclude_check check, const char *prefix) Ditto here. It's not a per-path value; it's supposed to be reused across checks for multiple paths, as explained in the comments above last_exclude_matching_path(): ... * A path to a directory known to be excluded is left in check-path to * optimize for repeated checks for files in the same excluded directory. */ struct exclude *last_exclude_matching_path(struct path_exclude_check *check, ... So I think you're probably right that there is potential for check-path to become effectively corrupted due to the caller not seeing the reallocation. I'll change this too. -- 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 1/5] check-ignore: move setup into cmd_check_ignore()
On Thu, Apr 11, 2013 at 12:05:11PM +0100, Adam Spiers wrote: On Thu, Apr 11, 2013 at 01:25:53AM -0400, Jeff King wrote: On Thu, Apr 11, 2013 at 02:59:31AM +0100, Adam Spiers wrote: -static int check_ignore(const char *prefix, const char **pathspec) +static int check_ignore(struct path_exclude_check check, + const char *prefix, const char **pathspec) Did you mean to pass the struct by value here? If it is truly a per-path [...] It's not a per-path value; it's supposed to be reused across checks for multiple paths, as explained in the comments above last_exclude_matching_path(): Makes sense (I didn't look into it very far, and was just guessing based on the pass-by-value). Passing a pointer is definitely the right fix, then. Thanks. -Peff -- 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 1/5] check-ignore: move setup into cmd_check_ignore()
Initialisation of the dir_struct and path_exclude_check structs was
previously done within check_ignore(). This was acceptable since
check_ignore() was only called once per check-ignore invocation;
however the next commit will convert it into an inner loop which is
called once per line of STDIN when --stdin is given. Therefore moving
the initialisation code out into cmd_check_ignore() ensures that
initialisation is still only performed once per check-ignore
invocation, and consequently that the output is identical whether
pathspecs are provided as CLI arguments or via STDIN.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
builtin/check-ignore.c | 39 ---
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 0240f99..0a4eef1 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -53,30 +53,20 @@ static void output_exclude(const char *path, struct exclude
*exclude)
}
}
-static int check_ignore(const char *prefix, const char **pathspec)
+static int check_ignore(struct path_exclude_check check,
+ const char *prefix, const char **pathspec)
{
- 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;
- /* read_cache() is only necessary so we can watch out for submodules. */
- if (read_cache() 0)
- die(_(index file corrupt));
-
- memset(dir, 0, sizeof(dir));
- dir.flags |= DIR_COLLECT_IGNORED;
- setup_standard_excludes(dir);
-
if (!pathspec || !*pathspec) {
if (!quiet)
fprintf(stderr, no pathspec given.\n);
return 0;
}
- 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
@@ -100,13 +90,11 @@ static int check_ignore(const char *prefix, const char
**pathspec)
}
}
free(seen);
- clear_directory(dir);
- path_exclude_check_clear(check);
return num_ignored;
}
-static int check_ignore_stdin_paths(const char *prefix)
+static int check_ignore_stdin_paths(struct path_exclude_check check, const
char *prefix)
{
struct strbuf buf, nbuf;
char **pathspec = NULL;
@@ -129,17 +117,18 @@ 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(check, prefix, (const char **)pathspec);
maybe_flush_or_die(stdout, attribute to stdout);
strbuf_release(buf);
strbuf_release(nbuf);
- free(pathspec);
return num_ignored;
}
int cmd_check_ignore(int argc, const char **argv, const char *prefix)
{
int num_ignored;
+ struct dir_struct dir;
+ struct path_exclude_check check;
git_config(git_default_config, NULL);
@@ -162,12 +151,24 @@ int cmd_check_ignore(int argc, const char **argv, const
char *prefix)
die(_(cannot have both --quiet and --verbose));
}
+ /* read_cache() is only necessary so we can watch out for submodules. */
+ if (read_cache() 0)
+ die(_(index file corrupt));
+
+ memset(dir, 0, sizeof(dir));
+ dir.flags |= DIR_COLLECT_IGNORED;
+ setup_standard_excludes(dir);
+
+ path_exclude_check_init(check, dir);
if (stdin_paths) {
- num_ignored = check_ignore_stdin_paths(prefix);
+ num_ignored = check_ignore_stdin_paths(check, prefix);
} else {
- num_ignored = check_ignore(prefix, argv);
+ num_ignored = check_ignore(check, prefix, argv);
maybe_flush_or_die(stdout, ignore to stdout);
}
+ clear_directory(dir);
+ path_exclude_check_clear(check);
+
return !num_ignored;
}
--
1.8.2.1.347.g37e0606
--
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 1/5] check-ignore: move setup into cmd_check_ignore()
On Thu, Apr 11, 2013 at 02:59:31AM +0100, Adam Spiers wrote: Initialisation of the dir_struct and path_exclude_check structs was previously done within check_ignore(). This was acceptable since check_ignore() was only called once per check-ignore invocation; however the next commit will convert it into an inner loop which is called once per line of STDIN when --stdin is given. Therefore moving the initialisation code out into cmd_check_ignore() ensures that initialisation is still only performed once per check-ignore invocation, and consequently that the output is identical whether pathspecs are provided as CLI arguments or via STDIN. Signed-off-by: Adam Spiers g...@adamspiers.org --- builtin/check-ignore.c | 39 --- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 0240f99..0a4eef1 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -53,30 +53,20 @@ static void output_exclude(const char *path, struct exclude *exclude) } } -static int check_ignore(const char *prefix, const char **pathspec) +static int check_ignore(struct path_exclude_check check, + const char *prefix, const char **pathspec) Did you mean to pass the struct by value here? If it is truly a per-path value, shouldn't it be declared and initialized inside here? Otherwise you risk one invocation munging things that the struct points to, but the caller's copy does not know about the change. In particular, I see that the struct includes a strbuf. What happens when one invocation of check_ignore grows the strbuf, then returns? The copy of the struct in the caller will not know that the buffer it is pointing to is now bogus. -static int check_ignore_stdin_paths(const char *prefix) +static int check_ignore_stdin_paths(struct path_exclude_check check, const char *prefix) Ditto here. -Peff -- 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
