{"thread":{"id":"32276","subject":"[PATCH] Add directory pattern matching to attributes","startedAt":"2012-12-05T22:10:29Z","lastAt":"2012-12-06T09:02:12Z","messageCount":3,"participants":["Jean-Noël AVILA","Junio C Hamano","Jean-Noël Avila"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"204539","messageId":"201212052310.30690.jn.avila@free.fr","threadId":"32276","inReplyTo":null,"subject":"[PATCH] Add directory pattern matching to attributes","fromName":"Jean-Noël AVILA","fromEmail":"jn.avila@free.fr","sentAt":"2012-12-05T22:10:29Z","receivedAt":"2012-12-05T22:10:29Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"The manpage of gitattributes says: \"The rules how the pattern\nmatches paths are the same as in .gitignore files\" and the gitignore\npattern rules has a pattern ending with / for directory matching.\n\nThis rule is specifically relevant for the 'export-ignore' rule used\nfor git archive.\n\nSigned-off-by: Jean-Noel Avila \n---\n archive.c | 2 +-\n attr.c | 47 ++++++++++++++++++++++++-------------\n attr.h | 4 ++--\n builtin/check-attr.c | 7 ++++--\n builtin/pack-objects.c | 2 +-\n convert.c | 2 +-\n ll-merge.c | 4 ++--\n t/t5002-archive-attr-pattern.sh | 49 +++++++++++++++++++++++++++++++++++++++\n userdiff.c | 2 +-\n ws.c | 2 +-\n 10 files changed, 94 insertions(+), 27 deletions(-)\n create mode 100644 t/t5002-archive-attr-pattern.sh\n\ndiff --git a/archive.c b/archive.c\nindex 4666404..b8ff14f 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \tpath_without_prefix = path.buf + args->baselen;\n \n \tsetup_archive_check(check);\n-\tif (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {\n+\tif (!git_check_attr(path_without_prefix, mode, ARRAY_SIZE(check), check)) {\n \t\tif (ATTR_TRUE(check[0].value))\n \t\t\treturn 0;\n \t\targs->convert = ATTR_TRUE(check[1].value);\ndiff --git a/attr.c b/attr.c\nindex 887a9ae..da386f2 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -548,7 +548,7 @@ static void bootstrap_attr_stack(void)\n \tattr_stack = elem;\n }\n \n-static void prepare_attr_stack(const char *path)\n+static void prepare_attr_stack(const char *path, unsigned mode)\n {\n \tstruct attr_stack *elem, *info;\n \tint dirlen, len;\n@@ -645,28 +645,43 @@ static void prepare_attr_stack(const char *path)\n }\n \n static int path_matches(const char *pathname, int pathlen,\n-\t\t\tconst char *pattern,\n+\t\t\tconst unsigned mode, char *pattern,\n \t\t\tconst char *base, int baselen)\n {\n-\tif (!strchr(pattern, '/')) {\n+\tsize_t len;\n+\tchar buf[PATH_MAX];\n+\tchar * lpattern = buf;\n+\tlen = strlen(pattern);\n+\tif (PATH_MAX <= len)\n+\t\treturn 0;\n+\tstrncpy(buf,pattern,len);\n+\tbuf[len] ='\\0';\n+\tif (len && lpattern[len - 1] == '/') {\n+\t\tif (S_ISDIR(mode))\n+\t\t\tlpattern[len - 1] = '\\0';\n+\t\telse\n+\t\t\treturn 0;\n+\t}\n+\n+\tif (!strchr(lpattern, '/')) {\n \t\t/* match basename */\n \t\tconst char *basename = strrchr(pathname, '/');\n \t\tbasename = basename ? basename + 1 : pathname;\n-\t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n+\t\treturn (fnmatch_icase(lpattern, basename, 0) == 0);\n \t}\n \t/*\n \t * match with FNM_PATHNAME; the pattern has base implicitly\n \t * in front of it.\n \t */\n-\tif (*pattern == '/')\n-\t\tpattern++;\n+\tif (*lpattern == '/')\n+\t\tlpattern++;\n \tif (pathlen < baselen ||\n \t (baselen && pathname[baselen] != '/') ||\n \t strncmp(pathname, base, baselen))\n \t\treturn 0;\n \tif (baselen != 0)\n \t\tbaselen++;\n-\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n+\treturn fnmatch_icase(lpattern, pathname + baselen, FNM_PATHNAME) == 0;\n }\n \n static int macroexpand_one(int attr_nr, int rem);\n@@ -693,7 +708,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)\n \treturn rem;\n }\n \n-static int fill(const char *path, int pathlen, struct attr_stack *stk, int rem)\n+static int fill(const char *path, int pathlen, unsigned mode, struct attr_stack *stk, int rem)\n {\n \tint i;\n \tconst char *base = stk->origin ? stk->origin : \"\";\n@@ -702,7 +717,7 @@ static int fill(const char *path, int pathlen, struct attr_stack *stk, int rem)\n \t\tstruct match_attr *a = stk->attrs[i];\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n-\t\tif (path_matches(path, pathlen,\n+\t\tif (path_matches(path, pathlen, mode,\n \t\t\t\t a->u.pattern, base, strlen(base)))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n@@ -737,26 +752,26 @@ static int macroexpand_one(int attr_nr, int rem)\n * Collect all attributes for path into the array pointed to by\n * check_all_attr.\n */\n-static void collect_all_attrs(const char *path)\n+static void collect_all_attrs(const char *path, unsigned mode)\n {\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem;\n \n-\tprepare_attr_stack(path);\n+\tprepare_attr_stack(path, mode);\n \tfor (i = 0; i < attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n \n \tpathlen = strlen(path);\n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n-\t\trem = fill(path, pathlen, stk, rem);\n+\t\trem = fill(path, pathlen, mode, stk, rem);\n }\n \n-int git_check_attr(const char *path, int num, struct git_attr_check *check)\n+int git_check_attr(const char *path, unsigned mode, int num, struct git_attr_check *check)\n {\n \tint i;\n \n-\tcollect_all_attrs(path);\n+\tcollect_all_attrs(path, mode);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n@@ -768,11 +783,11 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)\n \treturn 0;\n }\n \n-int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n+int git_all_attrs(const char *path, unsigned mode, int *num, struct git_attr_check **check)\n {\n \tint i, count, j;\n \n-\tcollect_all_attrs(path);\n+\tcollect_all_attrs(path, mode);\n \n \t/* Count the number of attributes that are set. */\n \tcount = 0;\ndiff --git a/attr.h b/attr.h\nindex 8b08d33..125d9c2 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -36,7 +36,7 @@ struct git_attr_check {\n */\n char *git_attr_name(struct git_attr *);\n \n-int git_check_attr(const char *path, int, struct git_attr_check *);\n+int git_check_attr(const char *path, unsigned mode, int, struct git_attr_check *);\n \n /*\n * Retrieve all attributes that apply to the specified path. *num\n@@ -45,7 +45,7 @@ int git_check_attr(const char *path, int, struct git_attr_check *);\n * objects describing the attributes and their values. *check must be\n * free()ed by the caller.\n */\n-int git_all_attrs(const char *path, int *num, struct git_attr_check **check);\n+int git_all_attrs(const char *path, unsigned mode, int *num, struct git_attr_check **check);\n \n enum git_attr_direction {\n \tGIT_ATTR_CHECKIN,\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 075d01d..2928480 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -46,14 +46,17 @@ static void output_attr(int cnt, struct git_attr_check *check,\n static void check_attr(const char *prefix, int cnt,\n \tstruct git_attr_check *check, const char *file)\n {\n+\tstruct stat st;\n \tchar *full_path =\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n+\tstat(full_path,&st);\n \tif (check != NULL) {\n-\t\tif (git_check_attr(full_path, cnt, check))\n+\t\tstat(full_path,&st);\n+\t\tif (git_check_attr(full_path, st.st_mode, cnt, check))\n \t\t\tdie(\"git_check_attr died\");\n \t\toutput_attr(cnt, check, file);\n \t} else {\n-\t\tif (git_all_attrs(full_path, &cnt, &check))\n+\t\tif (git_all_attrs(full_path, st.st_mode, &cnt, &check))\n \t\t\tdie(\"git_all_attrs died\");\n \t\toutput_attr(cnt, check, file);\n \t\tfree(check);\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 5e14064..acf75d4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -894,7 +894,7 @@ static int no_try_delta(const char *path)\n \tstruct git_attr_check check[1];\n \n \tsetup_delta_attr_check(check);\n-\tif (git_check_attr(path, ARRAY_SIZE(check), check))\n+\tif (git_check_attr(path, 0, ARRAY_SIZE(check), check))\n \t\treturn 0;\n \tif (ATTR_FALSE(check->value))\n \t\treturn 1;\ndiff --git a/convert.c b/convert.c\nindex 6602155..e01c290 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -759,7 +759,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tgit_config(read_convert_config, NULL);\n \t}\n \n-\tif (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {\n+\tif (!git_check_attr(path, 0, NUM_CONV_ATTRS, ccheck)) {\n \t\tca->crlf_action = git_path_check_crlf(path, ccheck + 4);\n \t\tif (ca->crlf_action == CRLF_GUESS)\n \t\t\tca->crlf_action = git_path_check_crlf(path, ccheck + 0);\ndiff --git a/ll-merge.c b/ll-merge.c\nindex acea33b..a8d3b2a 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -346,7 +346,7 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]\n \t\tcheck[0].attr = git_attr(\"merge\");\n \t\tcheck[1].attr = git_attr(\"conflict-marker-size\");\n \t}\n-\treturn git_check_attr(path, 2, check);\n+\treturn git_check_attr(path, 0, 2, check);\n }\n \n static void normalize_file(mmfile_t *mm, const char *path)\n@@ -403,7 +403,7 @@ int ll_merge_marker_size(const char *path)\n \n \tif (!check.attr)\n \t\tcheck.attr = git_attr(\"conflict-marker-size\");\n-\tif (!git_check_attr(path, 1, &check) && check.value) {\n+\tif (!git_check_attr(path, 0, 1, &check) && check.value) {\n \t\tmarker_size = atoi(check.value);\n \t\tif (marker_size <= 0)\n \t\t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nnew file mode 100644\nindex 0000000..3274057\n--- /dev/null\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='git archive attribute pattern tests'\n+\n+. ./test-lib.sh\n+\n+test_expect_exists() {\n+\ttest_expect_success \" $1 exists\" \"test -e $1\"\n+}\n+\n+test_expect_missing() {\n+\ttest_expect_success \" $1 does not exist\" \"test ! -e $1\"\n+}\n+\n+test_expect_success 'setup' '\n+\techo ignored >ignored &&\n+\techo ignored export-ignore >>.git/info/attributes &&\n+\tgit add ignored &&\n+\n+\tmkdir not-ignored-dir &&\n+\techo ignored-in-tree >not-ignored-dir/ignored &&\n+\techo not-ignored-in-tree >not-ignored-dir/not-ignored &&\n+\tgit add not-ignored-dir &&\n+\n+\tmkdir ignored-dir &&\n+\techo ignored by ignored dir >ignored-dir/ignored-by-ignored-dir &&\n+\techo ignored-dir/ export-ignore >>.git/info/attributes &&\n+\tgit add ignored-dir &&\n+\n+\tgit commit -m. &&\n+\n+\tgit clone --bare . bare &&\n+\tcp .git/info/attributes bare/info/attributes\n+'\n+\n+test_expect_success 'git archive' '\n+\tgit archive HEAD >archive.tar &&\n+\t(mkdir archive && cd archive && \"$TAR\" xf -) writes:\n\n> -static void prepare_attr_stack(const char *path)\n> +static void prepare_attr_stack(const char *path, unsigned mode)\n> {\n> \tstruct attr_stack *elem, *info;\n> \tint dirlen, len;\n> @@ -645,28 +645,43 @@ static void prepare_attr_stack(const char *path)\n> }\n\nWhy?\n\nThe new \"mode\" parameter does not seem to be used in this function\nat all.\n\n> static int path_matches(const char *pathname, int pathlen,\n> -\t\t\tconst char *pattern,\n> +\t\t\tconst unsigned mode, char *pattern,\n> \t\t\tconst char *base, int baselen)\n> {\n> -\tif (!strchr(pattern, '/')) {\n> +\tsize_t len;\n> +\tchar buf[PATH_MAX];\n> +\tchar * lpattern = buf;\n> +\tlen = strlen(pattern);\n> +\tif (PATH_MAX <= len)\n> +\t\treturn 0;\n> +\tstrncpy(buf,pattern,len);\n> +\tbuf[len] ='\\0';\n> +\tif (len && lpattern[len - 1] == '/') {\n> +\t\tif (S_ISDIR(mode))\n> +\t\t\tlpattern[len - 1] = '\\0';\n> +\t\telse\n> +\t\t\treturn 0;\n> +\t}\n> +\tif (!strchr(lpattern, '/')) {\n> \t\t/* match basename */\n> \t\tconst char *basename = strrchr(pathname, '/');\n> \t\tbasename = basename ? basename + 1 : pathname;\n> -\t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n> +\t\treturn (fnmatch_icase(lpattern, basename, 0) == 0);\n> \t}\n> \t/*\n> \t * match with FNM_PATHNAME; the pattern has base implicitly\n> \t * in front of it.\n> \t */\n> -\tif (*pattern == '/')\n> -\t\tpattern++;\n> +\tif (*lpattern == '/')\n> +\t\tlpattern++;\n> \tif (pathlen < baselen ||\n> \t (baselen && pathname[baselen] != '/') ||\n> \t strncmp(pathname, base, baselen))\n> \t\treturn 0;\n> \tif (baselen != 0)\n> \t\tbaselen++;\n> -\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n> +\treturn fnmatch_icase(lpattern, pathname + baselen, FNM_PATHNAME) == 0;\n> }\n\nIt appears to me that you are forcing the caller to tell this\nfunction if the path is a directory, but in the attribute system,\nthe caller does not necessarily know if the path it is passing is\nmeant to be a directory or a regular file. \"check-attr\" is meant to\nbe usable against a path that does not even exist on the working\ntree, so using stat() or lstat() in it is not a solution. In other\nwords, it is unfair (read: unworkable) to force it to append a\ntrailing slash after the path it calls this function with.\n\nIf you are interested in export-subst, all is not lost, though:\n\n\t$ git init\n $ mkdir a\n $ >a/b\n $ echo a export-ignore >.gitattributes\n $ git add a/b .gitattributes\n\t$ git commit -m initial\n $ git archive HEAD | tar tf -\n .gitattributes\n $ exit\n\nYou could change the \"echo\" to\n\n\t$ echo \"a/*\" export-ignore >.gitattributes\n\nas well, but it seems to create an useless empty directory \"a/\" in\nthe output, which I think is an unrelated bug in \"git archive\".\n\nThis patch seems to be based on a stale codebase. I do not think\nI'd be opposed to change the sementics to allow the callers that\nknow that a path is a directory to optionally pass mode parameter by\nending the pathname with slash (in other words, have \"git\ncheck-attr\" ask about a directory 'a' by saying \"git check-attr\nexport-subst a/\", and lose the \"mode\" argument in this patch), or\nkeep the \"mode\" parameter and instead allow \"git check-attr\" to ask\nabout a directory that does not exist in the working tree by a more\nexplicit \"git check-attr --directory export-ignore a\" or something.\nSuch an enhancement should be done on top of the current codebase.\n\nThanks.\n"},{"id":"204555","messageId":"50C05F14.8080809@gmail.com","threadId":"32276","inReplyTo":"7vlidcjcm9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add directory pattern matching to attributes","fromName":"Jean-Noël Avila","fromEmail":"avila.jn@gmail.com","sentAt":"2012-12-06T09:02:12Z","receivedAt":"2012-12-06T09:02:12Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"On 06/12/2012 00:35, Junio C Hamano wrote:\n> \"Jean-Noël AVILA\" writes:\n>\n>> -static void prepare_attr_stack(const char *path)\n>> +static void prepare_attr_stack(const char *path, unsigned mode)\n>> {\n>> \tstruct attr_stack *elem, *info;\n>> \tint dirlen, len;\n>> @@ -645,28 +645,43 @@ static void prepare_attr_stack(const char *path)\n>> }\n> Why?\n>\n> The new \"mode\" parameter does not seem to be used in this function\n> at all.\n>\n>> static int path_matches(const char *pathname, int pathlen,\n>> -\t\t\tconst char *pattern,\n>> +\t\t\tconst unsigned mode, char *pattern,\n>> \t\t\tconst char *base, int baselen)\n>> {\n>> -\tif (!strchr(pattern, '/')) {\n>> +\tsize_t len;\n>> +\tchar buf[PATH_MAX];\n>> +\tchar * lpattern = buf;\n>> +\tlen = strlen(pattern);\n>> +\tif (PATH_MAX <= len)\n>> +\t\treturn 0;\n>> +\tstrncpy(buf,pattern,len);\n>> +\tbuf[len] ='\\0';\n>> +\tif (len && lpattern[len - 1] == '/') {\n>> +\t\tif (S_ISDIR(mode))\n>> +\t\t\tlpattern[len - 1] = '\\0';\n>> +\t\telse\n>> +\t\t\treturn 0;\n>> +\t}\n>> +\tif (!strchr(lpattern, '/')) {\n>> \t\t/* match basename */\n>> \t\tconst char *basename = strrchr(pathname, '/');\n>> \t\tbasename = basename ? basename + 1 : pathname;\n>> -\t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n>> +\t\treturn (fnmatch_icase(lpattern, basename, 0) == 0);\n>> \t}\n>> \t/*\n>> \t * match with FNM_PATHNAME; the pattern has base implicitly\n>> \t * in front of it.\n>> \t */\n>> -\tif (*pattern == '/')\n>> -\t\tpattern++;\n>> +\tif (*lpattern == '/')\n>> +\t\tlpattern++;\n>> \tif (pathlen < baselen ||\n>> \t (baselen && pathname[baselen] != '/') ||\n>> \t strncmp(pathname, base, baselen))\n>> \t\treturn 0;\n>> \tif (baselen != 0)\n>> \t\tbaselen++;\n>> -\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n>> +\treturn fnmatch_icase(lpattern, pathname + baselen, FNM_PATHNAME) == 0;\n>> }\n> It appears to me that you are forcing the caller to tell this\n> function if the path is a directory, but in the attribute system,\n> the caller does not necessarily know if the path it is passing is\n> meant to be a directory or a regular file. \"check-attr\" is meant to\n> be usable against a path that does not even exist on the working\n> tree, so using stat() or lstat() in it is not a solution. In other\n> words, it is unfair (read: unworkable) to force it to append a\n> trailing slash after the path it calls this function with.\n\nThank you for your comments. Changing the whole attr.h interface header\nis definitely not a good option, but at some point, we may need more \ninformation\non the path to be able to match a path pattern against it.\n\n>\n> If you are interested in export-subst, all is not lost, though:\n>\n> \t$ git init\n> $ mkdir a\n> $ >a/b\n> $ echo a export-ignore >.gitattributes\n> $ git add a/b .gitattributes\n> \t$ git commit -m initial\n> $ git archive HEAD | tar tf -\n> .gitattributes\n> $ exit\n>\n> You could change the \"echo\" to\n>\n> \t$ echo \"a/*\" export-ignore >.gitattributes\n>\n> as well, but it seems to create an useless empty directory \"a/\" in\n> the output, which I think is an unrelated bug in \"git archive\".\n\nThis is quite different from the pattern matching documented for gitignore.\n\nMoreover,\n\n $ mkdir -p not-ignored-dir/ignored-dir\n $ echo test >not-ignored-dir/ignored-dir/ignored\n $ echo 'ignored-dir/*' >.gitattributes\n $ git add not-ignored-dir .gitattributes\n $ git commit -m '.'\n $ git archive HEAD | tar tf -\n.gitattributes\nnot-ignored-dir/\nnot-ignored-dir/ignored-dir/\nnot-ignored-dir/ignored-dir/ignored\n\n\n>\n> This patch seems to be based on a stale codebase.\nSorry. I thought the patch submissions had to be based on the 'maint' \nbranch.\n\n> I do not think\n> I'd be opposed to change the sementics to allow the callers that\n> know that a path is a directory to optionally pass mode parameter by\n> ending the pathname with slash (in other words, have \"git\n> check-attr\" ask about a directory 'a' by saying \"git check-attr\n> export-subst a/\", and lose the \"mode\" argument in this patch), or\n> keep the \"mode\" parameter and instead allow \"git check-attr\" to ask\n> about a directory that does not exist in the working tree by a more\n> explicit \"git check-attr --directory export-ignore a\" or something.\n> Such an enhancement should be done on top of the current codebase.\n\nOK. I like the idea of proposing a path ending with '/' when it is meant \nto be\ndirectory. This would not change the interface attr.h . I will rework \nwith this idea.\n\nThank you.\n"}]}