guard against new pathspec magic in pathspec matching code

GUARD_PATHSPEC() marks pathspec-sensitive code, basically all those
that touch anything in 'struct pathspec' except fields "nr" and
"original". GUARD_PATHSPEC() is not supposed to fail. It's mainly to
help the designers catch unsupported codepaths.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt
index 90d1aff6251dddbf5c946efafc38e2b83ea806af..540e45568990f89fceb50ad619ca8402e9925983 100644 (file)
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -28,3 +28,22 @@ parse_pathspec(). This function takes several arguments:
 - prefix and args come from cmd_* functions
 
 get_pathspec() is obsolete and should never be used in new code.
+
+parse_pathspec() helps catch unsupported features and reject them
+politely. At a lower level, different pathspec-related functions may
+not support the same set of features. Such pathspec-sensitive
+functions are guarded with GUARD_PATHSPEC(), which will die in an
+unfriendly way when an unsupported feature is requested.
+
+The command designers are supposed to make sure that GUARD_PATHSPEC()
+never dies. They have to make sure all unsupported features are caught
+by parse_pathspec(), not by GUARD_PATHSPEC. grepping GUARD_PATHSPEC()
+should give the designers all pathspec-sensitive codepaths and what
+features they support.
+
+A similar process is applied when a new pathspec magic is added. The
+designer lifts the GUARD_PATHSPEC restriction in the functions that
+support the new magic. At the same time (s)he has to make sure this
+new feature will be caught at parse_pathspec() in commands that cannot
+handle the new magic in some cases. grepping parse_pathspec() should
+help.

diff --git a/builtin/diff.c b/builtin/diff.c
index 9fc273d8cd78d53a55047e17d44eb89291741167..6bb41aff5d5ed8729373a654d509685c776dc069 100644 (file)
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -367,6 +367,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
                }
        }
        if (rev.prune_data.nr) {
+               /* builtin_diff_b_f() */
+               GUARD_PATHSPEC(&rev.prune_data, PATHSPEC_FROMTOP);
                if (!path)
                        path = rev.prune_data.items[0].match;
                paths += rev.prune_data.nr;

diff --git a/dir.c b/dir.c
index e28bc0d636d544a3305c9e6c71d666de568bc0fb..19978d3d59753a810503cf6dbac9eacb0f16c474 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -340,6 +340,8 @@ int match_pathspec_depth(const struct pathspec *ps,
 {
        int i, retval = 0;
 
+       GUARD_PATHSPEC(ps, PATHSPEC_FROMTOP | PATHSPEC_MAXDEPTH);
+
        if (!ps->nr) {
                if (!ps->recursive ||
                    !(ps->magic & PATHSPEC_MAXDEPTH) ||

diff --git a/pathspec.h b/pathspec.h
index 2e427d7f4c36bce6285d677cb29d911b110c2d14..6baf2058622d0a9d26e5a20547525a60ebd30ccb 100644 (file)
--- a/pathspec.h
+++ b/pathspec.h
@@ -27,6 +27,13 @@ struct pathspec {
        } *items;
 };
 
+#define GUARD_PATHSPEC(ps, mask) \
+       do { \
+               if ((ps)->magic & ~(mask))             \
+                       die("BUG:%s:%d: unsupported magic %x",  \
+                           __FILE__, __LINE__, (ps)->magic & ~(mask)); \
+       } while (0)
+
 /* parse_pathspec flags */
 #define PATHSPEC_PREFER_CWD (1<<0) /* No args means match cwd */
 #define PATHSPEC_PREFER_FULL (1<<1) /* No args means match everything */

diff --git a/tree-diff.c b/tree-diff.c
index 826512e84d0d99bd34d7d428e3d712d60d729eac..5a876148bb79c0cd80ae90068b4ee3c37b32fab1 100644 (file)
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -198,6 +198,25 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
        const char *paths[1];
        int i;
 
+       /*
+        * follow-rename code is very specific, we need exactly one
+        * path. Magic that matches more than one path is not
+        * supported.
+        */
+       GUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP);
+#if 0
+       /*
+        * We should reject wildcards as well. Unfortunately we
+        * haven't got a reliable way to detect that 'foo\*bar' in
+        * fact has no wildcards. nowildcard_len is merely a hint for
+        * optimization. Let it slip for now until wildmatch is taught
+        * about dry-run mode and returns wildcard info.
+        */
+       if (opt->pathspec.has_wildcard)
+               die("BUG:%s:%d: wildcards are not supported",
+                   __FILE__, __LINE__);
+#endif
+
        /* Remove the file creation entry from the diff queue, and remember it */
        choice = q->queue[0];
        q->nr = 0;

diff --git a/tree-walk.c b/tree-walk.c
index d399ca9dcc69a21eaae31f2926b8480d22e86672..37b157ed6e2c90d3ae24e39e944237a7c75d8bf2 100644 (file)
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -636,6 +636,8 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
        enum interesting never_interesting = ps->has_wildcard ?
                entry_not_interesting : all_entries_not_interesting;
 
+       GUARD_PATHSPEC(ps, PATHSPEC_FROMTOP | PATHSPEC_MAXDEPTH);
+
        if (!ps->nr) {
                if (!ps->recursive ||
                    !(ps->magic & PATHSPEC_MAXDEPTH) ||