From: Duy Nguyen <pclouds@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic
Date: Sat, 23 Mar 2013 10:13:34 +0700 [thread overview]
Message-ID: <20130323031334.GA10484@lanh> (raw)
In-Reply-To: <7vvc8kabhx.fsf@alter.siamese.dyndns.org>
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 + strlen(path) + 1);
if (len)
memcpy(sanitized, prefix, len);
strcpy(sanitized + len, path);
+ if (remaining_prefix)
+ *remaining_prefix = len;
}
- if (normalize_path_copy_len(sanitized, sanitized, p_len))
+ if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
goto error_out;
if (is_absolute_path(orig)) {
size_t root_len, len, total;
@@ -47,7 +60,7 @@ char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
char *prefix_path(const char *prefix, int len, const char *path)
{
- char *r = prefix_path_gently(prefix, &len, path);
+ char *r = prefix_path_gently(prefix, len, NULL, path);
if (!r)
die("'%s' is outside repository", path);
return r;
@@ -56,7 +69,7 @@ char *prefix_path(const char *prefix, int len, const char *path)
int path_inside_repo(const char *prefix, const char *path)
{
int len = prefix ? strlen(prefix) : 0;
- char *r = prefix_path_gently(prefix, &len, path);
+ char *r = prefix_path_gently(prefix, len, NULL, path);
if (r) {
free(r);
return 1;
-- 8< --
next prev parent reply other threads:[~2013-03-23 3:15 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-15 6:06 [PATCH v1 00/45] nd/parse-pathspec and :(glob) pathspec magic Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 01/45] setup.c: check that the pathspec magic ends with ")" Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 02/45] clean: remove unused variable "seen" Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 03/45] Move struct pathspec and related functions to pathspec.[ch] Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 04/45] pathspec: i18n-ize error strings in pathspec parsing code Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 05/45] pathspec: add copy_pathspec Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 06/45] Add parse_pathspec() that converts cmdline args to struct pathspec Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 07/45] parse_pathspec: save original pathspec for reporting Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 08/45] parse_pathspec: add PATHSPEC_PREFER_{CWD,FULL} Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 09/45] Convert some get_pathspec() calls to parse_pathspec() Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 10/45] parse_pathspec: a special flag for max_depth feature Nguyễn Thái Ngọc Duy
2013-03-15 21:28 ` Eric Sunshine
2013-03-15 6:06 ` [PATCH v1 11/45] parse_pathspec: support stripping submodule trailing slashes Nguyễn Thái Ngọc Duy
2013-03-17 21:55 ` Junio C Hamano
2013-03-18 0:53 ` Duy Nguyen
2013-03-15 6:06 ` [PATCH v1 12/45] parse_pathspec: support stripping/checking submodule paths Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 13/45] parse_pathspec: support prefixing original patterns Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 14/45] Guard against new pathspec magic in pathspec matching code Nguyễn Thái Ngọc Duy
2013-03-17 22:00 ` Junio C Hamano
2013-03-15 6:06 ` [PATCH v1 15/45] clean: convert to use parse_pathspec Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 16/45] commit: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 17/45] status: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 18/45] rerere: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 19/45] checkout: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 20/45] rm: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 21/45] ls-files: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 22/45] archive: " Nguyễn Thái Ngọc Duy
2013-03-15 17:56 ` Junio C Hamano
2013-03-16 1:08 ` Duy Nguyen
2013-03-17 5:00 ` Junio C Hamano
2013-03-17 5:31 ` Duy Nguyen
2013-03-17 6:22 ` Junio C Hamano
2013-03-15 6:06 ` [PATCH v1 23/45] check-ignore: " Nguyễn Thái Ngọc Duy
2013-04-12 15:03 ` Adam Spiers
2013-04-12 23:09 ` Duy Nguyen
2013-04-14 23:25 ` Adam Spiers
2013-04-14 23:48 ` Duy Nguyen
2013-04-15 16:17 ` Adam Spiers
2013-03-15 6:06 ` [PATCH v1 24/45] add: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 25/45] reset: " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 26/45] Convert read_cache_preload() to take struct pathspec Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 27/45] Convert run_add_interactive to use " Nguyễn Thái Ngọc Duy
2013-03-18 18:26 ` John Keeping
2013-03-19 1:58 ` Duy Nguyen
2013-03-19 10:58 ` John Keeping
2013-03-15 6:06 ` [PATCH v1 28/45] Convert unmerge_cache to take " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 29/45] checkout: convert read_tree_some " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 30/45] Convert report_path_error " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 31/45] Convert refresh_index " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 32/45] Convert {read,fill}_directory " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 33/45] Convert add_files_to_cache " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 34/45] Convert common_prefix() to use " Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 35/45] Remove diff_tree_{setup,release}_paths Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 36/45] Remove init_pathspec() in favor of parse_pathspec() Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 37/45] Remove match_pathspec() in favor of match_pathspec_depth() Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 38/45] tree-diff: remove the use of pathspec's raw[] in follow-rename codepath Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 39/45] parse_pathspec: make sure the prefix part is wildcard-free Nguyễn Thái Ngọc Duy
2013-03-19 18:34 ` Junio C Hamano
2013-03-20 1:32 ` Duy Nguyen
2013-03-20 10:54 ` Duy Nguyen
2013-03-15 6:06 ` [PATCH v1 40/45] parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN Nguyễn Thái Ngọc Duy
2013-03-15 22:00 ` Eric Sunshine
2013-03-15 6:06 ` [PATCH v1 41/45] Kill limit_pathspec_to_literal() as it's only used by parse_pathspec() Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 42/45] pathspec: support :(literal) syntax for noglob pathspec Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 43/45] pathspec: make --literal-pathspecs disable pathspec magic Nguyễn Thái Ngọc Duy
2013-03-15 6:06 ` [PATCH v1 44/45] pathspec: support :(glob) syntax Nguyễn Thái Ngọc Duy
2013-03-15 22:11 ` Eric Sunshine
2013-03-15 6:07 ` [PATCH v1 45/45] Rename field "raw" to "_raw" in struct pathspec Nguyễn Thái Ngọc Duy
2013-03-15 17:48 ` [PATCH v1 00/45] nd/parse-pathspec and :(glob) pathspec magic Junio C Hamano
2013-03-20 12:16 ` [PATCH v2 00/45] parse_pathspec and :(glob) magic Nguyễn Thái Ngọc Duy
2013-03-20 12:16 ` [PATCH v2 06/45] Add parse_pathspec() that converts cmdline args to struct pathspec Nguyễn Thái Ngọc Duy
2013-03-20 19:40 ` Eric Sunshine
2013-03-22 21:55 ` Junio C Hamano
2013-03-20 12:16 ` [PATCH v2 11/45] parse_pathspec: support stripping submodule trailing slashes Nguyễn Thái Ngọc Duy
2013-03-20 12:16 ` [PATCH v2 12/45] parse_pathspec: support stripping/checking submodule paths Nguyễn Thái Ngọc Duy
2013-03-20 12:16 ` [PATCH v2 14/45] Guard against new pathspec magic in pathspec matching code Nguyễn Thái Ngọc Duy
2013-03-20 19:45 ` Eric Sunshine
2013-03-20 12:16 ` [PATCH v2 27/45] Convert run_add_interactive to use struct pathspec Nguyễn Thái Ngọc Duy
2013-03-20 12:16 ` [PATCH v2 39/45] parse_pathspec: make sure the prefix part is wildcard-free Nguyễn Thái Ngọc Duy
2013-03-20 18:02 ` [PATCH v2 00/45] parse_pathspec and :(glob) magic Junio C Hamano
2013-03-21 5:33 ` Duy Nguyen
2013-03-21 5:43 ` Duy Nguyen
2013-03-21 17:28 ` Junio C Hamano
2013-03-21 17:50 ` Junio C Hamano
2013-03-23 3:13 ` Duy Nguyen [this message]
2013-03-23 3:15 ` Duy Nguyen
2013-03-27 15:39 ` Junio C Hamano
2013-03-24 0:57 ` Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130323031334.GA10484@lanh \
--to=pclouds@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).