{"thread":{"id":"18798","subject":"[PATCH v2 0/3] support \"in-tree attributes\" for git-archive","startedAt":"2009-04-09T07:01:27Z","lastAt":"2009-04-16T00:26:52Z","messageCount":15,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy","René Scharfe"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"110890","messageId":"1239260490-6318-1-git-send-email-pclouds@gmail.com","threadId":"18798","inReplyTo":null,"subject":"[PATCH v2 0/3] support \"in-tree attributes\" for git-archive","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-04-09T07:01:27Z","receivedAt":"2009-04-09T07:01:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The series has been shortened to 3 patches by keeping the old codepath,\njust adding the index for attr.\n\n--fix-attributes is added for old behaviour.\n\nNguyễn Thái Ngọc Duy (3):\n archive: add shortcuts for --format and --prefix\n attr: add GIT_ATTR_INDEX \"direction\"\n archive: do not read .gitattributes in working directory\n\n Documentation/git-archive.txt | 9 +++++++--\n archive.c | 26 ++++++++++++++++++++++++--\n archive.h | 1 +\n attr.c | 4 +++-\n attr.h | 3 ++-\n builtin-tar-tree.c | 5 +++++\n t/t5000-tar-tree.sh | 28 ++++++++++++++++------------\n 7 files changed, 58 insertions(+), 18 deletions(-)\n"},{"id":"110892","messageId":"1239260490-6318-2-git-send-email-pclouds@gmail.com","threadId":"18798","inReplyTo":"1239260490-6318-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/3] archive: add shortcuts for --format and --prefix","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-04-09T07:01:28Z","receivedAt":"2009-04-09T07:01:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-archive.txt | 4 +++-\n archive.c | 4 ++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt\nindex c1adf59..2e31142 100644\n--- a/Documentation/git-archive.txt\n+++ b/Documentation/git-archive.txt\n@@ -9,7 +9,7 @@ git-archive - Create an archive of files from a named tree\n SYNOPSIS\n --------\n [verse]\n-'git archive' --format= [--list] [--prefix=/] []\n+'git archive' [-f |--format=] [--list] [-p /|--prefix=/] []\n \t [--output=]\n \t [--remote= [--exec=]] \n \t [path...]\n@@ -33,6 +33,7 @@ comment.\n OPTIONS\n -------\n \n+-f ::\n --format=::\n \tFormat of the resulting archive: 'tar' or 'zip'. The default\n \tis 'tar'.\n@@ -45,6 +46,7 @@ OPTIONS\n --verbose::\n \tReport progress to stderr.\n \n+-p /::\n --prefix=/::\n \tPrepend / to each filename in the archive.\n \ndiff --git a/archive.c b/archive.c\nindex 96b62d4..e87fed7 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -260,8 +260,8 @@ static int parse_archive_args(int argc, const char **argv,\n \tint list = 0;\n \tstruct option opts[] = {\n \t\tOPT_GROUP(\"\"),\n-\t\tOPT_STRING(0, \"format\", &format, \"fmt\", \"archive format\"),\n-\t\tOPT_STRING(0, \"prefix\", &base, \"prefix\",\n+\t\tOPT_STRING('f', \"format\", &format, \"fmt\", \"archive format\"),\n+\t\tOPT_STRING('p', \"prefix\", &base, \"prefix\",\n \t\t\t\"prepend prefix to each pathname in the archive\"),\n \t\tOPT_STRING(0, \"output\", &output, \"file\",\n \t\t\t\"write the archive to this file\"),\n-- \n1.6.2.2.602.g83ee9f\n"},{"id":"110891","messageId":"1239260490-6318-3-git-send-email-pclouds@gmail.com","threadId":"18798","inReplyTo":"1239260490-6318-2-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] attr: add GIT_ATTR_INDEX \"direction\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-04-09T07:01:29Z","receivedAt":"2009-04-09T07:01:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This instructs attr mechanism, not to look into working .gitattributes\nat all. Needed by tools that does not handle working directory, such\nas \"git archive\".\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 4 +++-\n attr.h | 3 ++-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 43259e5..37ca288 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -405,7 +405,7 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)\n \t\tif (!res)\n \t\t\tres = read_attr_from_file(path, macro_ok);\n \t}\n-\telse {\n+\telse if (direction == GIT_ATTR_CHECKIN) {\n \t\tres = read_attr_from_file(path, macro_ok);\n \t\tif (!res)\n \t\t\t/*\n@@ -415,6 +415,8 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)\n \t\t\t */\n \t\t\tres = read_attr_from_index(path, macro_ok);\n \t}\n+\telse\n+\t\tres = read_attr_from_index(path, macro_ok);\n \tif (!res)\n \t\tres = xcalloc(1, sizeof(*res));\n \treturn res;\ndiff --git a/attr.h b/attr.h\nindex 3a2f4ec..69b5767 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -33,7 +33,8 @@ int git_checkattr(const char *path, int, struct git_attr_check *);\n \n enum git_attr_direction {\n \tGIT_ATTR_CHECKIN,\n-\tGIT_ATTR_CHECKOUT\n+\tGIT_ATTR_CHECKOUT,\n+\tGIT_ATTR_INDEX,\n };\n void git_attr_set_direction(enum git_attr_direction, struct index_state *);\n \n-- \n1.6.2.2.602.g83ee9f\n"},{"id":"110893","messageId":"1239260490-6318-4-git-send-email-pclouds@gmail.com","threadId":"18798","inReplyTo":"1239260490-6318-3-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-04-09T07:01:30Z","receivedAt":"2009-04-09T07:01:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The old behaviour still remains with --fix-attributes.\nAlso fix tests in t5000-tar-tree.sh to use --fix-attributes.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n 2009/4/9 Junio C Hamano :\n > Hmmm, if you read_tree() into the_index upfront and do not change anything\n > else to the archive.c code, shouldn't it work without such a regression at\n > all? Am I missing something?\n >\n > It would allow you to export the index into an archive, but I doubt it is\n > worth the amount of code churn.\n > Hmmm, if you read_tree() into the_index upfront and do not change anything\n > else to the archive.c code, shouldn't it work without such a regression at\n > all? Am I missing something?\n >\n > It would allow you to export the index into an archive, but I doubt it is\n > worth the amount of code churn.\n \n I skipped the idea originally because of data duplication. But given\n the amount of code change in my approach, just loading index is better.\n \n 2009/4/9 René Scharfe :\n > I don't like the need to prepare an index of all paths up front, but\n > that's just a gut feeling. I haven't looked into implementing in-tree\n > attribute support in attr.c; is it really that hard? Other commands\n > would benefit from this, too, right (e.g. any command using attributes\n > in a bare repo)?\n \n You could try. At least with index, I only need a couple lines of\n modification in attr.c :) If it traverses directory upward for\n .gitattributes, then you may have problem. I'm not sure though.\n\n\n Documentation/git-archive.txt | 5 ++++-\n archive.c | 22 ++++++++++++++++++++++\n archive.h | 1 +\n builtin-tar-tree.c | 5 +++++\n t/t5000-tar-tree.sh | 28 ++++++++++++++++------------\n 5 files changed, 48 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt\nindex 2e31142..f468523 100644\n--- a/Documentation/git-archive.txt\n+++ b/Documentation/git-archive.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git archive' [-f |--format=] [--list] [-p /|--prefix=/] []\n-\t [--output=]\n+\t [--output=] [--fix-attributes]\n \t [--remote= [--exec=]] \n \t [path...]\n \n@@ -53,6 +53,9 @@ OPTIONS\n --output=::\n \tWrite the archive to instead of stdout.\n \n+--fix-attributes::\n+\tLook for attributes in .gitattributes in working directory too.\n+\n ::\n \tThis can be any options that the archiver backend understands.\n \tSee next section.\ndiff --git a/archive.c b/archive.c\nindex e87fed7..c1c5c3c 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -4,6 +4,7 @@\n #include \"attr.h\"\n #include \"archive.h\"\n #include \"parse-options.h\"\n+#include \"unpack-trees.h\"\n \n static char const * const archive_usage[] = {\n \t\"git archive [options] [path...]\",\n@@ -150,6 +151,8 @@ int write_archive_entries(struct archiver_args *args,\n \t\twrite_archive_entry_fn_t write_entry)\n {\n \tstruct archiver_context context;\n+\tstruct unpack_trees_options opts;\n+\tstruct tree_desc t;\n \tint err;\n \n \tif (args->baselen > 0 && args->base[args->baselen - 1] == '/') {\n@@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,\n \tcontext.args = args;\n \tcontext.write_entry = write_entry;\n \n+\t/*\n+\t * Setup index and instruct attr to read index only\n+\t */\n+\tif (!args->worktree_attributes) {\n+\t\tmemset(&opts, 0, sizeof(opts));\n+\t\topts.index_only = 1;\n+\t\topts.head_idx = -1;\n+\t\topts.src_index = &the_index;\n+\t\topts.dst_index = &the_index;\n+\t\topts.fn = oneway_merge;\n+\t\tinit_tree_desc(&t, args->tree->buffer, args->tree->size);\n+\t\tif (unpack_trees(1, &t, &opts))\n+\t\t\treturn -1;\n+\t\tgit_attr_set_direction(GIT_ATTR_INDEX, &the_index);\n+\t}\n+\n \terr = read_tree_recursive(args->tree, args->base, args->baselen, 0,\n \t\t\targs->pathspec, write_archive_entry, &context);\n \tif (err == READ_TREE_RECURSIVE)\n@@ -258,6 +277,7 @@ static int parse_archive_args(int argc, const char **argv,\n \tint verbose = 0;\n \tint i;\n \tint list = 0;\n+\tint worktree_attributes = 0;\n \tstruct option opts[] = {\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_STRING('f', \"format\", &format, \"fmt\", \"archive format\"),\n@@ -265,6 +285,7 @@ static int parse_archive_args(int argc, const char **argv,\n \t\t\t\"prepend prefix to each pathname in the archive\"),\n \t\tOPT_STRING(0, \"output\", &output, \"file\",\n \t\t\t\"write the archive to this file\"),\n+\t\tOPT_BOOLEAN(0, \"fix-attributes\", &worktree_attributes, \"read .gitattributes in working directory\"),\n \t\tOPT__VERBOSE(&verbose),\n \t\tOPT__COMPR('0', &compression_level, \"store only\", 0),\n \t\tOPT__COMPR('1', &compression_level, \"compress faster\", 1),\n@@ -324,6 +345,7 @@ static int parse_archive_args(int argc, const char **argv,\n \targs->verbose = verbose;\n \targs->base = base;\n \targs->baselen = strlen(base);\n+\targs->worktree_attributes = worktree_attributes;\n \n \treturn argc;\n }\ndiff --git a/archive.h b/archive.h\nindex 0b15b35..038ac35 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -10,6 +10,7 @@ struct archiver_args {\n \ttime_t time;\n \tconst char **pathspec;\n \tunsigned int verbose : 1;\n+\tunsigned int worktree_attributes : 1;\n \tint compression_level;\n };\n \ndiff --git a/builtin-tar-tree.c b/builtin-tar-tree.c\nindex 0713bca..69a93fc 100644\n--- a/builtin-tar-tree.c\n+++ b/builtin-tar-tree.c\n@@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)\n \t\targv++;\n \t\targc--;\n \t}\n+\tif (2 <= argc && !strcmp(argv[1], \"--fix-attributes\")) {\n+\t\tnargv[nargc++] = argv[1];\n+\t\targv++;\n+\t\targc--;\n+\t}\n \tswitch (argc) {\n \tdefault:\n \t\tusage(tar_tree_usage);\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 7641e0d..7ff600b 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -71,12 +71,16 @@ test_expect_success \\\n 'rm a/ignored'\n \n test_expect_success \\\n- 'git archive' \\\n- 'git archive HEAD >b.tar'\n+ 'git archive without --fix-attributes' \\\n+ 'git archive HEAD | tar t | grep -q ignored'\n+\n+test_expect_success \\\n+ 'git archive --fix-attributes' \\\n+ 'git archive --fix-attributes HEAD >b.tar'\n \n test_expect_success \\\n 'git tar-tree' \\\n- 'git tar-tree HEAD >b2.tar'\n+ 'git tar-tree --fix-attributes HEAD >b2.tar'\n \n test_expect_success \\\n 'git archive vs. git tar-tree' \\\n@@ -84,14 +88,14 @@ test_expect_success \\\n \n test_expect_success \\\n 'git archive in a bare repo' \\\n- '(cd bare.git && git archive HEAD) >b3.tar'\n+ '(cd bare.git && git archive --fix-attributes HEAD) >b3.tar'\n \n test_expect_success \\\n 'git archive vs. the same in a bare repo' \\\n 'test_cmp b.tar b3.tar'\n \n test_expect_success 'git archive with --output' \\\n- 'git archive --output=b4.tar HEAD &&\n+ 'git archive --fix-attributes --output=b4.tar HEAD &&\n test_cmp b.tar b4.tar'\n \n test_expect_success \\\n@@ -122,7 +126,7 @@ test_expect_success \\\n \n test_expect_success \\\n 'git tar-tree with prefix' \\\n- 'git tar-tree HEAD prefix >c.tar'\n+ 'git tar-tree --fix-attributes HEAD prefix >c.tar'\n \n test_expect_success \\\n 'extract tar archive with prefix' \\\n@@ -140,8 +144,8 @@ test_expect_success \\\n test_expect_success \\\n 'create archives with substfiles' \\\n 'echo \"substfile?\" export-subst >a/.gitattributes &&\n- git archive HEAD >f.tar &&\n- git archive --prefix=prefix/ HEAD >g.tar &&\n+ git archive --fix-attributes HEAD >f.tar &&\n+ git archive --fix-attributes --prefix=prefix/ HEAD >g.tar &&\n rm a/.gitattributes'\n \n test_expect_success \\\n@@ -170,18 +174,18 @@ test_expect_success \\\n \n test_expect_success \\\n 'git archive --format=zip' \\\n- 'git archive --format=zip HEAD >d.zip'\n+ 'git archive --fix-attributes --format=zip HEAD >d.zip'\n \n test_expect_success \\\n 'git archive --format=zip in a bare repo' \\\n- '(cd bare.git && git archive --format=zip HEAD) >d1.zip'\n+ '(cd bare.git && git archive --fix-attributes --format=zip HEAD) >d1.zip'\n \n test_expect_success \\\n 'git archive --format=zip vs. the same in a bare repo' \\\n 'test_cmp d.zip d1.zip'\n \n test_expect_success 'git archive --format=zip with --output' \\\n- 'git archive --format=zip --output=d2.zip HEAD &&\n+ 'git archive --fix-attributes --format=zip --output=d2.zip HEAD &&\n test_cmp d.zip d2.zip'\n \n $UNZIP -v >/dev/null 2>&1\n@@ -206,7 +210,7 @@ test_expect_success UNZIP \\\n \n test_expect_success \\\n 'git archive --format=zip with prefix' \\\n- 'git archive --format=zip --prefix=prefix/ HEAD >e.zip'\n+ 'git archive --fix-attributes --format=zip --prefix=prefix/ HEAD >e.zip'\n \n test_expect_success UNZIP \\\n 'extract ZIP archive with prefix' \\\n-- \n1.6.2.2.602.g83ee9f\n"},{"id":"110902","messageId":"7v8wma43ii.fsf@gitster.siamese.dyndns.org","threadId":"18798","inReplyTo":"1239260490-6318-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 1/3] archive: add shortcuts for --format and --prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-09T08:47:49Z","receivedAt":"2009-04-09T08:47:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Is this really necessary?\n\nI see little benefit from giving either of these a shorter alternative,\nand a huge downside for potential confusion, especially because nobody\nelse uses -p for --prefix, and other commands use -p for something\ncompletely different.\n\nThe same deal for -f, but it is probably worse because it is typically\nused for --force.\n"},{"id":"110905","messageId":"7vws9u2ov4.fsf@gitster.siamese.dyndns.org","threadId":"18798","inReplyTo":"1239260490-6318-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-09T08:49:35Z","receivedAt":"2009-04-09T08:49:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> @@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,\n> \tcontext.args = args;\n> \tcontext.write_entry = write_entry;\n> \n> +\t/*\n> +\t * Setup index and instruct attr to read index only\n> +\t */\n> +\tif (!args->worktree_attributes) {\n> +\t\tmemset(&opts, 0, sizeof(opts));\n> +\t\topts.index_only = 1;\n> +\t\topts.head_idx = -1;\n> +\t\topts.src_index = &the_index;\n> +\t\topts.dst_index = &the_index;\n> +\t\topts.fn = oneway_merge;\n> +\t\tinit_tree_desc(&t, args->tree->buffer, args->tree->size);\n> +\t\tif (unpack_trees(1, &t, &opts))\n> +\t\t\treturn -1;\n> +\t\tgit_attr_set_direction(GIT_ATTR_INDEX, &the_index);\n\nWhy use unpack_trees with oneway_merge? You won't be doing \"is this file\nup-to-date in the work tree?\", and you won't be writing the index out\neither, so there is nothing gained by keeping the cached stat information\nfresh, which is the major justification of using that mechanism. I think\nusing tree.c::read_tree() would be more appropriate.\n\n> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c\n> index 0713bca..69a93fc 100644\n> --- a/builtin-tar-tree.c\n> +++ b/builtin-tar-tree.c\n> @@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)\n> \t\targv++;\n> \t\targc--;\n> \t}\n> +\tif (2 <= argc && !strcmp(argv[1], \"--fix-attributes\")) {\n> +\t\tnargv[nargc++] = argv[1];\n> +\t\targv++;\n> +\t\targc--;\n> +\t}\n\nI am not sure if it is worth backporting this new option to tar-tree which\nis an essentially backward-compatibility interface, and worse yet, doing\nit poorly (i.e. --fix-attributes must come after --remote= for unexplained\nreason).\n\nIt would affect a bit more tests, but I think you would want to test both\nthe new \"normal\" mode of operation (generate archives with \"git archive\"\nand \"git tar-tree\" without options and compare, for example), instead of\nadding --fix-attributes everywhere.\n"},{"id":"110909","messageId":"fcaeb9bf0904090338y6295f459odd25628c1ceac93a@mail.gmail.com","threadId":"18798","inReplyTo":"7v8wma43ii.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3] archive: add shortcuts for --format and --prefix","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-04-09T10:38:55Z","receivedAt":"2009-04-09T10:38:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2009/4/9 Junio C Hamano :\n> Is this really necessary?\n\nI have used a lot it to test and typing --format=blah is just painful.\nAs I said earlier, this is just a convenient patch (at least for me).\nFeel free to drop it if it causes confusion.\n-- \nDuy\n"},{"id":"110911","messageId":"fcaeb9bf0904090353s4ec770bfk3cd3f6559c367a20@mail.gmail.com","threadId":"18798","inReplyTo":"7vws9u2ov4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-04-09T10:53:27Z","receivedAt":"2009-04-09T10:53:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2009/4/9 Junio C Hamano :\n> Nguyễn Thái Ngọc Duy   writes:\n>\n>> @@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,\n>>       context.args = args;\n>>       context.write_entry = write_entry;\n>>\n>> +     /*\n>> +      * Setup index and instruct attr to read index only\n>> +      */\n>> +     if (!args->worktree_attributes) {\n>> +             memset(&opts, 0, sizeof(opts));\n>> +             opts.index_only = 1;\n>> +             opts.head_idx = -1;\n>> +             opts.src_index = &the_index;\n>> +             opts.dst_index = &the_index;\n>> +             opts.fn = oneway_merge;\n>> +             init_tree_desc(&t, args->tree->buffer, args->tree->size);\n>> +             if (unpack_trees(1, &t, &opts))\n>> +                     return -1;\n>> +             git_attr_set_direction(GIT_ATTR_INDEX, &the_index);\n>\n> Why use unpack_trees with oneway_merge?  You won't be doing \"is this file\n> up-to-date in the work tree?\", and you won't be writing the index out\n> either, so there is nothing gained by keeping the cached stat information\n> fresh, which is the major justification of using that mechanism.  I think\n> using tree.c::read_tree() would be more appropriate.\n\nBecause I'm more familiar with unpack stuff than read-tree (or to be\nhonest, haven't touched tree stuff at all). Will look at read_tree().\n\n>> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c\n>> index 0713bca..69a93fc 100644\n>> --- a/builtin-tar-tree.c\n>> +++ b/builtin-tar-tree.c\n>> @@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)\n>>               argv++;\n>>               argc--;\n>>       }\n>> +     if (2 <= argc && !strcmp(argv[1], \"--fix-attributes\")) {\n>> +             nargv[nargc++] = argv[1];\n>> +             argv++;\n>> +             argc--;\n>> +     }\n>\n> I am not sure if it is worth backporting this new option to tar-tree which\n> is an essentially backward-compatibility interface, and worse yet, doing\n> it poorly (i.e. --fix-attributes must come after --remote= for unexplained\n> reason).\n\nIt's because git-tar-tree is used in tests and I don't want to migrate\nall to git-archive. I don't want to change too much in a deprecated\ncommand. Maybe just remove the option and make --fix-attributes\ndefault for git-tar-tree. In other words, keep git-tar-tree's current\nbehaviour.\n\n> It would affect a bit more tests, but I think you would want to test both\n> the new \"normal\" mode of operation (generate archives with \"git archive\"\n> and \"git tar-tree\" without options and compare, for example), instead of\n> adding --fix-attributes everywhere.\n\nThere is a new test to test the new \"normal\" mode. I'll think of more.\n-- \nDuy\n"},{"id":"111078","messageId":"7v8wm7f112.fsf@gitster.siamese.dyndns.org","threadId":"18798","inReplyTo":"fcaeb9bf0904090353s4ec770bfk3cd3f6559c367a20@mail.gmail.com","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-11T19:22:49Z","receivedAt":"2009-04-11T19:22:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> Maybe just remove the option and make --fix-attributes\n> default for git-tar-tree. In other words, keep git-tar-tree's current\n> behaviour.\n\nThat sounds like a sensible approach to me.\n\nThanks.\n"},{"id":"111173","messageId":"49E316CD.7030001@lsrfire.ath.cx","threadId":"18798","inReplyTo":"fcaeb9bf0904090353s4ec770bfk3cd3f6559c367a20@mail.gmail.com","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-04-13T10:41:17Z","receivedAt":"2009-04-13T10:41:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Nguyen Thai Ngoc Duy schrieb:\n> 2009/4/9 Junio C Hamano :\n>> It would affect a bit more tests, but I think you would want to test both\n>> the new \"normal\" mode of operation (generate archives with \"git archive\"\n>> and \"git tar-tree\" without options and compare, for example), instead of\n>> adding --fix-attributes everywhere.\n> \n> There is a new test to test the new \"normal\" mode. I'll think of more.\n\nThe following patch, which can be applied before yours, makes the tests\nstill pass after your changes by avoiding to use worktree attributes. I\nthink it makes sense to create a separate script for the new tests and\neventually move the existing archive attribute tests there.\n\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 7641e0d..abb41b0 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -50,7 +50,7 @@ test_expect_success \\\n test_expect_success \\\n 'add ignored file' \\\n 'echo ignore me >a/ignored &&\n- echo ignored export-ignore >.gitattributes'\n+ echo ignored export-ignore >.git/info/attributes'\n \n test_expect_success \\\n 'add files to repository' \\\n@@ -64,7 +64,7 @@ test_expect_success \\\n test_expect_success \\\n 'create bare clone' \\\n 'git clone --bare . bare.git &&\n- cp .gitattributes bare.git/info/attributes'\n+ cp .git/info/attributes bare.git/info/attributes'\n \n test_expect_success \\\n 'remove ignored file' \\\n@@ -139,10 +139,11 @@ test_expect_success \\\n \n test_expect_success \\\n 'create archives with substfiles' \\\n- 'echo \"substfile?\" export-subst >a/.gitattributes &&\n+ 'cp .git/info/attributes .git/info/attributes.before &&\n+ echo \"substfile?\" export-subst >>.git/info/attributes &&\n git archive HEAD >f.tar &&\n git archive --prefix=prefix/ HEAD >g.tar &&\n- rm a/.gitattributes'\n+ mv .git/info/attributes.before .git/info/attributes'\n \n test_expect_success \\\n 'extract substfiles' \\\n"},{"id":"111184","messageId":"49E32D9F.2050906@lsrfire.ath.cx","threadId":"18798","inReplyTo":"49E316CD.7030001@lsrfire.ath.cx","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-04-13T12:18:39Z","receivedAt":"2009-04-13T12:18:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"René Scharfe schrieb:\n> I\n> think it makes sense to create a separate script for the new tests and\n> eventually move the existing archive attribute tests there.\n\nSomething like this?\n\n\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nnew file mode 100755\nindex 0000000..b754f21\n--- /dev/null\n+++ b/t/t5001-archive-attr.sh\n@@ -0,0 +1,62 @@\n+#!/bin/sh\n+\n+test_description='git archive attribute tests'\n+\n+. ./test-lib.sh\n+\n+SUBSTFORMAT=%H%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+\techo ignored by tree >ignored-by-tree &&\n+\techo ignored-by-tree export-ignore >.gitattributes &&\n+\tgit add ignored-by-tree .gitattributes &&\n+\n+\techo ignored by worktree >ignored-by-worktree &&\n+\techo ignored-by-worktree export-ignore >.gitattributes &&\n+\tgit add ignored-by-worktree &&\n+\n+\tprintf \"A\\$Format:%s\\$O\" \"$SUBSTFORMAT\" >nosubstfile &&\n+\tprintf \"A\\$Format:%s\\$O\" \"$SUBSTFORMAT\" >substfile1 &&\n+\tprintf \"A not substituted O\" >substfile2 &&\n+\techo \"substfile?\" export-subst >>.git/info/attributes &&\n+\tgit add nosubstfile substfile1 substfile2 &&\n+\n+\tgit commit -m.\n+'\n+\n+test_expect_success 'git archive' '\n+\tgit archive HEAD >archive.tar &&\n+\t(mkdir archive && cd archive && \"$TAR\" xf -) worktree.tar &&\n+\t(mkdir worktree && cd worktree && \"$TAR\" xf -) substfile1.expected &&\n+\ttest_cmp nosubstfile archive/nosubstfile &&\n+\ttest_cmp substfile1.expected archive/substfile1 &&\n+\ttest_cmp substfile2 archive/substfile2\n+'\n+\n+test_expect_success 'git tar-tree vs. git archive with worktree attributes' '\n+\tgit tar-tree HEAD >tar-tree.tar &&\n+\ttest_cmp worktree.tar tar-tree.tar\n+'\n+\n+test_done\n"},{"id":"111186","messageId":"49E33935.5090203@lsrfire.ath.cx","threadId":"18798","inReplyTo":"49E32D9F.2050906@lsrfire.ath.cx","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-04-13T13:08:05Z","receivedAt":"2009-04-13T13:08:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"René Scharfe schrieb:\n>> I\n>> think it makes sense to create a separate script for the new tests and\n>> eventually move the existing archive attribute tests there.\n> \n> Something like this?\n\nI forgot to add tests against bare repositories. Otherwise I'd noticed\nearlier that read_attr() is only called for non-bare repositories\ncurrently, i.e. your patches won't allow reading of .gitattribute files\nfrom the tree in bare repos.\n\nRené\n"},{"id":"111260","messageId":"7vk55nakge.fsf@gitster.siamese.dyndns.org","threadId":"18798","inReplyTo":"49E33935.5090203@lsrfire.ath.cx","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-14T05:11:13Z","receivedAt":"2009-04-14T05:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> René Scharfe schrieb:\n>>> I\n>>> think it makes sense to create a separate script for the new tests and\n>>> eventually move the existing archive attribute tests there.\n>> \n>> Something like this?\n>\n> I forgot to add tests against bare repositories. Otherwise I'd noticed\n> earlier that read_attr() is only called for non-bare repositories\n> currently, i.e. your patches won't allow reading of .gitattribute files\n> from the tree in bare repos.\n\nCurious.\n\nShouldn't the call chain look like:\n\n write_archive()\n ->write_archive_entries()\n ->unpack_trees() to read the tree into the in-core index\n ->git_attr_set_direction()\n ->read_tree_recursive()\n ->write_archive_entry()\n ->git_checkattr()\n\nAh, bootstrap_attr_stack() and prepare_attr_stack() still assume that you\nwon't be doing any per-level attributes in a bare repository because the\nconcept of attributes is inherently tied to having a work tree from their\npoint of view.\n\nHow about this \"mostly re-indent with four line removal\" patch?\n\n attr.c | 48 ++++++++++++++++++++++--------------------------\n 1 files changed, 22 insertions(+), 26 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 37ca288..f5917de 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -468,13 +468,11 @@ static void bootstrap_attr_stack(void)\n \t\telem->prev = attr_stack;\n \t\tattr_stack = elem;\n \n-\t\tif (!is_bare_repository()) {\n-\t\t\telem = read_attr(GITATTRIBUTES_FILE, 1);\n-\t\t\telem->origin = strdup(\"\");\n-\t\t\telem->prev = attr_stack;\n-\t\t\tattr_stack = elem;\n-\t\t\tdebug_push(elem);\n-\t\t}\n+\t\telem = read_attr(GITATTRIBUTES_FILE, 1);\n+\t\telem->origin = strdup(\"\");\n+\t\telem->prev = attr_stack;\n+\t\tattr_stack = elem;\n+\t\tdebug_push(elem);\n \n \t\telem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);\n \t\tif (!elem)\n@@ -535,25 +533,23 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t/*\n \t * Read from parent directories and push them down\n \t */\n-\tif (!is_bare_repository()) {\n-\t\twhile (1) {\n-\t\t\tchar *cp;\n-\n-\t\t\tlen = strlen(attr_stack->origin);\n-\t\t\tif (dirlen <= len)\n-\t\t\t\tbreak;\n-\t\t\tstrbuf_reset(&pathbuf);\n-\t\t\tstrbuf_add(&pathbuf, path, dirlen);\n-\t\t\tstrbuf_addch(&pathbuf, '/');\n-\t\t\tcp = strchr(pathbuf.buf + len + 1, '/');\n-\t\t\tstrcpy(cp + 1, GITATTRIBUTES_FILE);\n-\t\t\telem = read_attr(pathbuf.buf, 0);\n-\t\t\t*cp = '\\0';\n-\t\t\telem->origin = strdup(pathbuf.buf);\n-\t\t\telem->prev = attr_stack;\n-\t\t\tattr_stack = elem;\n-\t\t\tdebug_push(elem);\n-\t\t}\n+\twhile (1) {\n+\t\tchar *cp;\n+\n+\t\tlen = strlen(attr_stack->origin);\n+\t\tif (dirlen <= len)\n+\t\t\tbreak;\n+\t\tstrbuf_reset(&pathbuf);\n+\t\tstrbuf_add(&pathbuf, path, dirlen);\n+\t\tstrbuf_addch(&pathbuf, '/');\n+\t\tcp = strchr(pathbuf.buf + len + 1, '/');\n+\t\tstrcpy(cp + 1, GITATTRIBUTES_FILE);\n+\t\telem = read_attr(pathbuf.buf, 0);\n+\t\t*cp = '\\0';\n+\t\telem->origin = strdup(pathbuf.buf);\n+\t\telem->prev = attr_stack;\n+\t\tattr_stack = elem;\n+\t\tdebug_push(elem);\n \t}\n \n \t/*\n"},{"id":"111323","messageId":"49E4FCF8.3050207@lsrfire.ath.cx","threadId":"18798","inReplyTo":"7vk55nakge.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-04-14T21:15:36Z","receivedAt":"2009-04-14T21:15:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> René Scharfe writes:\n> \n>> René Scharfe schrieb:\n>>>> I\n>>>> think it makes sense to create a separate script for the new tests and\n>>>> eventually move the existing archive attribute tests there.\n>>> Something like this?\n>> I forgot to add tests against bare repositories. Otherwise I'd noticed\n>> earlier that read_attr() is only called for non-bare repositories\n>> currently, i.e. your patches won't allow reading of .gitattribute files\n>> from the tree in bare repos.\n> \n> Curious.\n> \n> Shouldn't the call chain look like:\n> \n> write_archive()\n> ->write_archive_entries()\n> ->unpack_trees() to read the tree into the in-core index\n> ->git_attr_set_direction()\n> ->read_tree_recursive()\n> ->write_archive_entry()\n> ->git_checkattr()\n> \n> Ah, bootstrap_attr_stack() and prepare_attr_stack() still assume that you\n> won't be doing any per-level attributes in a bare repository because the\n> concept of attributes is inherently tied to having a work tree from their\n> point of view.\n> \n> How about this \"mostly re-indent with four line removal\" patch?\n\nPlus the following (on top of Duy's GIT_ATTR_INDEX patch)?\n\ndiff --git a/attr.c b/attr.c\nindex f5917de..cce561b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -672,6 +672,8 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)\n void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)\n {\n \tenum git_attr_direction old = direction;\n+\tif (is_bare_repository())\n+\t\tnew = GIT_ATTR_INDEX;\n \tdirection = new;\n \tif (new != old)\n \t\tdrop_attr_stack();\n"},{"id":"111405","messageId":"7vr5ztpho3.fsf@gitster.siamese.dyndns.org","threadId":"18798","inReplyTo":"49E4FCF8.3050207@lsrfire.ath.cx","subject":"Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-16T00:26:52Z","receivedAt":"2009-04-16T00:26:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Junio C Hamano schrieb:\n> ...\n>> Ah, bootstrap_attr_stack() and prepare_attr_stack() still assume that you\n>> won't be doing any per-level attributes in a bare repository because the\n>> concept of attributes is inherently tied to having a work tree from their\n>> point of view.\n>> \n>> How about this \"mostly re-indent with four line removal\" patch?\n>\n> Plus the following (on top of Duy's GIT_ATTR_INDEX patch)?\n\nActually, the \"mostly re-indent\" patch breaks things for normal users that\nexpect the attributes never kicks in when you are doing things in a bare\nrepository as-is.\n\n> diff --git a/attr.c b/attr.c\n> index f5917de..cce561b 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -672,6 +672,8 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)\n> void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)\n> {\n> \tenum git_attr_direction old = direction;\n> +\tif (is_bare_repository())\n> +\t\tnew = GIT_ATTR_INDEX;\n> \tdirection = new;\n> \tif (new != old)\n> \t\tdrop_attr_stack();\n\nThis looks _conceptually_ wrong.\n\nI think your patch came from the fact that check_updates() unconditionally\ncalls git_attr_set_direction() without checking o->update, and I think it\nis a bug in check_updates().\n\nA bare repository is by definition without a work tree, and we shouldn't\nbe reading the index in general. I wouldn't go as far to say that a bare\nrepository should not have the index file, because people often clone\nforgetting the --bare option and manually convert that to a bare\nrepository, and they forget to remove the index that is never used.\n\n\t... thinks a bit more ...\n\nI think codepaths that make calls to git_attr_set_direction() inside\nis_bare_repository() are special already. If we were to teach check-attr\nhow to check attributes for paths _inside a given tree-ish_, most likely\nthe implementation would be similar to what Duy did for archive; read the\ntree into in-core index, set direction to the INDEX, and start using the\nattribute mechanism.\n\nSo I think we'd rather not have the patch to force GIT_ATTR_INDEX in\nset_direction(); if anything, the patch should say something like \"if we\nare in a bare repository and the new direction is not INDEX, it is a\nprogramming error\".\n\nInstead, such specialized codepaths should call set_direction() itself,\nperhaps after reading the tree-ish into the in-core index. And we should\nfix the \"mostly re-indent\" patch not to remove the conditional, but make\nthe conditional to check \"If in a bare repository and the direction is not\nexplicitly set to INDEX, do not use the attributes\".\n"}]}