[PATCH v2 0/3] support "in-tree attributes" for git-archive

[PATCH v2 0/3] support "in-tree attributes" for git-archive

[Nguyễn Thái Ngọc Duy]

The series has been shortened to 3 patches by keeping the old codepath,
just adding the index for attr.

--fix-attributes is added for old behaviour.

Nguyễn Thái Ngọc Duy (3):
  archive: add shortcuts for --format and --prefix
  attr: add GIT_ATTR_INDEX "direction"
  archive: do not read .gitattributes in working directory

 Documentation/git-archive.txt |    9 +++++++--
 archive.c                     |   26 ++++++++++++++++++++++++--
 archive.h                     |    1 +
 attr.c                        |    4 +++-
 attr.h                        |    3 ++-
 builtin-tar-tree.c            |    5 +++++
 t/t5000-tar-tree.sh           |   28 ++++++++++++++++------------
 7 files changed, 58 insertions(+), 18 deletions(-)

[PATCH v2 1/3] archive: add shortcuts for --format and --prefix

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-archive.txt |    4 +++-
 archive.c                     |    4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index c1adf59..2e31142 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -9,7 +9,7 @@ git-archive - Create an archive of files from a named tree
 SYNOPSIS
 --------
 [verse]
-'git archive' --format=<fmt> [--list] [--prefix=<prefix>/] [<extra>]
+'git archive' [-f <fmt>|--format=<fmt>] [--list] [-p <prefix>/|--prefix=<prefix>/] [<extra>]
 	      [--output=<file>]
 	      [--remote=<repo> [--exec=<git-upload-archive>]] <tree-ish>
 	      [path...]
@@ -33,6 +33,7 @@ comment.
 OPTIONS
 -------
 
+-f <fmt>::
 --format=<fmt>::
 	Format of the resulting archive: 'tar' or 'zip'.  The default
 	is 'tar'.
@@ -45,6 +46,7 @@ OPTIONS
 --verbose::
 	Report progress to stderr.
 
+-p <prefix>/::
 --prefix=<prefix>/::
 	Prepend <prefix>/ to each filename in the archive.
 
diff --git a/archive.c b/archive.c
index 96b62d4..e87fed7 100644
--- a/archive.c
+++ b/archive.c
@@ -260,8 +260,8 @@ static int parse_archive_args(int argc, const char **argv,
 	int list = 0;
 	struct option opts[] = {
 		OPT_GROUP(""),
-		OPT_STRING(0, "format", &format, "fmt", "archive format"),
-		OPT_STRING(0, "prefix", &base, "prefix",
+		OPT_STRING('f', "format", &format, "fmt", "archive format"),
+		OPT_STRING('p', "prefix", &base, "prefix",
 			"prepend prefix to each pathname in the archive"),
 		OPT_STRING(0, "output", &output, "file",
 			"write the archive to this file"),
-- 
1.6.2.2.602.g83ee9f

[PATCH v2 2/3] attr: add GIT_ATTR_INDEX "direction"

[Nguyễn Thái Ngọc Duy]

This instructs attr mechanism, not to look into working .gitattributes
at all. Needed by tools that does not handle working directory, such
as "git archive".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c |    4 +++-
 attr.h |    3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 43259e5..37ca288 100644
--- a/attr.c
+++ b/attr.c
@@ -405,7 +405,7 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 		if (!res)
 			res = read_attr_from_file(path, macro_ok);
 	}
-	else {
+	else if (direction == GIT_ATTR_CHECKIN) {
 		res = read_attr_from_file(path, macro_ok);
 		if (!res)
 			/*
@@ -415,6 +415,8 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 			 */
 			res = read_attr_from_index(path, macro_ok);
 	}
+	else
+		res = read_attr_from_index(path, macro_ok);
 	if (!res)
 		res = xcalloc(1, sizeof(*res));
 	return res;
diff --git a/attr.h b/attr.h
index 3a2f4ec..69b5767 100644
--- a/attr.h
+++ b/attr.h
@@ -33,7 +33,8 @@ int git_checkattr(const char *path, int, struct git_attr_check *);
 
 enum git_attr_direction {
 	GIT_ATTR_CHECKIN,
-	GIT_ATTR_CHECKOUT
+	GIT_ATTR_CHECKOUT,
+	GIT_ATTR_INDEX,
 };
 void git_attr_set_direction(enum git_attr_direction, struct index_state *);
 
-- 
1.6.2.2.602.g83ee9f

[PATCH v2 3/3] archive: do not read .gitattributes in working directory

[Nguyễn Thái Ngọc Duy]

The old behaviour still remains with --fix-attributes.
Also fix tests in t5000-tar-tree.sh to use --fix-attributes.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
  2009/4/9 Junio C Hamano <gitster@pobox.com>:
  > Hmmm, if you read_tree() into the_index upfront and do not change anything
  > else to the archive.c code, shouldn't it work without such a regression at
  > all?  Am I missing something?
  >
  > It would allow you to export the index into an archive, but I doubt it is
  > worth the amount of code churn.
  > Hmmm, if you read_tree() into the_index upfront and do not change anything
  > else to the archive.c code, shouldn't it work without such a regression at
  > all?  Am I missing something?
  >
  > It would allow you to export the index into an archive, but I doubt it is
  > worth the amount of code churn.
  
  I skipped the idea originally because of data duplication. But given
  the amount of code change in my approach, just loading index is better.
  
  2009/4/9 René Scharfe <rene.scharfe@lsrfire.ath.cx>:
  > I don't like the need to prepare an index of all paths up front, but
  > that's just a gut feeling.  I haven't looked into implementing in-tree
  > attribute support in attr.c; is it really that hard?  Other commands
  > would benefit from this, too, right (e.g. any command using attributes
  > in a bare repo)?
  
  You could try. At least with index, I only need a couple lines of
  modification in attr.c :) If it traverses directory upward for
  .gitattributes, then you may have problem. I'm not sure though.


 Documentation/git-archive.txt |    5 ++++-
 archive.c                     |   22 ++++++++++++++++++++++
 archive.h                     |    1 +
 builtin-tar-tree.c            |    5 +++++
 t/t5000-tar-tree.sh           |   28 ++++++++++++++++------------
 5 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 2e31142..f468523 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git archive' [-f <fmt>|--format=<fmt>] [--list] [-p <prefix>/|--prefix=<prefix>/] [<extra>]
-	      [--output=<file>]
+	      [--output=<file>] [--fix-attributes]
 	      [--remote=<repo> [--exec=<git-upload-archive>]] <tree-ish>
 	      [path...]
 
@@ -53,6 +53,9 @@ OPTIONS
 --output=<file>::
 	Write the archive to <file> instead of stdout.
 
+--fix-attributes::
+	Look for attributes in .gitattributes in working directory too.
+
 <extra>::
 	This can be any options that the archiver backend understands.
 	See next section.
diff --git a/archive.c b/archive.c
index e87fed7..c1c5c3c 100644
--- a/archive.c
+++ b/archive.c
@@ -4,6 +4,7 @@
 #include "attr.h"
 #include "archive.h"
 #include "parse-options.h"
+#include "unpack-trees.h"
 
 static char const * const archive_usage[] = {
 	"git archive [options] <tree-ish> [path...]",
@@ -150,6 +151,8 @@ int write_archive_entries(struct archiver_args *args,
 		write_archive_entry_fn_t write_entry)
 {
 	struct archiver_context context;
+	struct unpack_trees_options opts;
+	struct tree_desc t;
 	int err;
 
 	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
@@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,
 	context.args = args;
 	context.write_entry = write_entry;
 
+	/*
+	 * Setup index and instruct attr to read index only
+	 */
+	if (!args->worktree_attributes) {
+		memset(&opts, 0, sizeof(opts));
+		opts.index_only = 1;
+		opts.head_idx = -1;
+		opts.src_index = &the_index;
+		opts.dst_index = &the_index;
+		opts.fn = oneway_merge;
+		init_tree_desc(&t, args->tree->buffer, args->tree->size);
+		if (unpack_trees(1, &t, &opts))
+			return -1;
+		git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
+	}
+
 	err =  read_tree_recursive(args->tree, args->base, args->baselen, 0,
 			args->pathspec, write_archive_entry, &context);
 	if (err == READ_TREE_RECURSIVE)
@@ -258,6 +277,7 @@ static int parse_archive_args(int argc, const char **argv,
 	int verbose = 0;
 	int i;
 	int list = 0;
+	int worktree_attributes = 0;
 	struct option opts[] = {
 		OPT_GROUP(""),
 		OPT_STRING('f', "format", &format, "fmt", "archive format"),
@@ -265,6 +285,7 @@ static int parse_archive_args(int argc, const char **argv,
 			"prepend prefix to each pathname in the archive"),
 		OPT_STRING(0, "output", &output, "file",
 			"write the archive to this file"),
+		OPT_BOOLEAN(0, "fix-attributes", &worktree_attributes, "read .gitattributes in working directory"),
 		OPT__VERBOSE(&verbose),
 		OPT__COMPR('0', &compression_level, "store only", 0),
 		OPT__COMPR('1', &compression_level, "compress faster", 1),
@@ -324,6 +345,7 @@ static int parse_archive_args(int argc, const char **argv,
 	args->verbose = verbose;
 	args->base = base;
 	args->baselen = strlen(base);
+	args->worktree_attributes = worktree_attributes;
 
 	return argc;
 }
diff --git a/archive.h b/archive.h
index 0b15b35..038ac35 100644
--- a/archive.h
+++ b/archive.h
@@ -10,6 +10,7 @@ struct archiver_args {
 	time_t time;
 	const char **pathspec;
 	unsigned int verbose : 1;
+	unsigned int worktree_attributes : 1;
 	int compression_level;
 };
 
diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
index 0713bca..69a93fc 100644
--- a/builtin-tar-tree.c
+++ b/builtin-tar-tree.c
@@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 		argv++;
 		argc--;
 	}
+	if (2 <= argc && !strcmp(argv[1], "--fix-attributes")) {
+		nargv[nargc++] = argv[1];
+		argv++;
+		argc--;
+	}
 	switch (argc) {
 	default:
 		usage(tar_tree_usage);
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 7641e0d..7ff600b 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -71,12 +71,16 @@ test_expect_success \
     'rm a/ignored'
 
 test_expect_success \
-    'git archive' \
-    'git archive HEAD >b.tar'
+    'git archive without --fix-attributes' \
+    'git archive HEAD | tar t | grep -q ignored'
+
+test_expect_success \
+    'git archive --fix-attributes' \
+    'git archive --fix-attributes HEAD >b.tar'
 
 test_expect_success \
     'git tar-tree' \
-    'git tar-tree HEAD >b2.tar'
+    'git tar-tree --fix-attributes HEAD >b2.tar'
 
 test_expect_success \
     'git archive vs. git tar-tree' \
@@ -84,14 +88,14 @@ test_expect_success \
 
 test_expect_success \
     'git archive in a bare repo' \
-    '(cd bare.git && git archive HEAD) >b3.tar'
+    '(cd bare.git && git archive --fix-attributes HEAD) >b3.tar'
 
 test_expect_success \
     'git archive vs. the same in a bare repo' \
     'test_cmp b.tar b3.tar'
 
 test_expect_success 'git archive with --output' \
-    'git archive --output=b4.tar HEAD &&
+    'git archive --fix-attributes --output=b4.tar HEAD &&
     test_cmp b.tar b4.tar'
 
 test_expect_success \
@@ -122,7 +126,7 @@ test_expect_success \
 
 test_expect_success \
     'git tar-tree with prefix' \
-    'git tar-tree HEAD prefix >c.tar'
+    'git tar-tree --fix-attributes HEAD prefix >c.tar'
 
 test_expect_success \
     'extract tar archive with prefix' \
@@ -140,8 +144,8 @@ test_expect_success \
 test_expect_success \
     'create archives with substfiles' \
     'echo "substfile?" export-subst >a/.gitattributes &&
-     git archive HEAD >f.tar &&
-     git archive --prefix=prefix/ HEAD >g.tar &&
+     git archive --fix-attributes HEAD >f.tar &&
+     git archive --fix-attributes --prefix=prefix/ HEAD >g.tar &&
      rm a/.gitattributes'
 
 test_expect_success \
@@ -170,18 +174,18 @@ test_expect_success \
 
 test_expect_success \
     'git archive --format=zip' \
-    'git archive --format=zip HEAD >d.zip'
+    'git archive --fix-attributes --format=zip HEAD >d.zip'
 
 test_expect_success \
     'git archive --format=zip in a bare repo' \
-    '(cd bare.git && git archive --format=zip HEAD) >d1.zip'
+    '(cd bare.git && git archive --fix-attributes --format=zip HEAD) >d1.zip'
 
 test_expect_success \
     'git archive --format=zip vs. the same in a bare repo' \
     'test_cmp d.zip d1.zip'
 
 test_expect_success 'git archive --format=zip with --output' \
-    'git archive --format=zip --output=d2.zip HEAD &&
+    'git archive --fix-attributes --format=zip --output=d2.zip HEAD &&
     test_cmp d.zip d2.zip'
 
 $UNZIP -v >/dev/null 2>&1
@@ -206,7 +210,7 @@ test_expect_success UNZIP \
 
 test_expect_success \
     'git archive --format=zip with prefix' \
-    'git archive --format=zip --prefix=prefix/ HEAD >e.zip'
+    'git archive --fix-attributes --format=zip --prefix=prefix/ HEAD >e.zip'
 
 test_expect_success UNZIP \
     'extract ZIP archive with prefix' \
-- 
1.6.2.2.602.g83ee9f

Re: [PATCH v2 1/3] archive: add shortcuts for --format and --prefix

[Junio C Hamano]

Is this really necessary?

I see little benefit from giving either of these a shorter alternative,
and a huge downside for potential confusion, especially because nobody
else uses -p for --prefix, and other commands use -p for something
completely different.

The same deal for -f, but it is probably worse because it is typically
used for --force.

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> @@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,
>  	context.args = args;
>  	context.write_entry = write_entry;
>  
> +	/*
> +	 * Setup index and instruct attr to read index only
> +	 */
> +	if (!args->worktree_attributes) {
> +		memset(&opts, 0, sizeof(opts));
> +		opts.index_only = 1;
> +		opts.head_idx = -1;
> +		opts.src_index = &the_index;
> +		opts.dst_index = &the_index;
> +		opts.fn = oneway_merge;
> +		init_tree_desc(&t, args->tree->buffer, args->tree->size);
> +		if (unpack_trees(1, &t, &opts))
> +			return -1;
> +		git_attr_set_direction(GIT_ATTR_INDEX, &the_index);

Why use unpack_trees with oneway_merge?  You won't be doing "is this file
up-to-date in the work tree?", and you won't be writing the index out
either, so there is nothing gained by keeping the cached stat information
fresh, which is the major justification of using that mechanism.  I think
using tree.c::read_tree() would be more appropriate.

> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
> index 0713bca..69a93fc 100644
> --- a/builtin-tar-tree.c
> +++ b/builtin-tar-tree.c
> @@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
>  		argv++;
>  		argc--;
>  	}
> +	if (2 <= argc && !strcmp(argv[1], "--fix-attributes")) {
> +		nargv[nargc++] = argv[1];
> +		argv++;
> +		argc--;
> +	}

I am not sure if it is worth backporting this new option to tar-tree which
is an essentially backward-compatibility interface, and worse yet, doing
it poorly (i.e. --fix-attributes must come after --remote= for unexplained
reason).

It would affect a bit more tests, but I think you would want to test both
the new "normal" mode of operation (generate archives with "git archive"
and "git tar-tree" without options and compare, for example), instead of
adding --fix-attributes everywhere.

Re: [PATCH v2 1/3] archive: add shortcuts for --format and --prefix

[Nguyen Thai Ngoc Duy]

2009/4/9 Junio C Hamano <gitster@pobox.com>:
> Is this really necessary?

I have used a lot it to test and typing --format=blah is just painful.
As I said earlier, this is just a convenient patch (at least for me).
Feel free to drop it if it causes confusion.
-- 
Duy

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[Nguyen Thai Ngoc Duy]

2009/4/9 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> @@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,
>>       context.args = args;
>>       context.write_entry = write_entry;
>>
>> +     /*
>> +      * Setup index and instruct attr to read index only
>> +      */
>> +     if (!args->worktree_attributes) {
>> +             memset(&opts, 0, sizeof(opts));
>> +             opts.index_only = 1;
>> +             opts.head_idx = -1;
>> +             opts.src_index = &the_index;
>> +             opts.dst_index = &the_index;
>> +             opts.fn = oneway_merge;
>> +             init_tree_desc(&t, args->tree->buffer, args->tree->size);
>> +             if (unpack_trees(1, &t, &opts))
>> +                     return -1;
>> +             git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
>
> Why use unpack_trees with oneway_merge?  You won't be doing "is this file
> up-to-date in the work tree?", and you won't be writing the index out
> either, so there is nothing gained by keeping the cached stat information
> fresh, which is the major justification of using that mechanism.  I think
> using tree.c::read_tree() would be more appropriate.

Because I'm more familiar with unpack stuff than read-tree (or to be
honest, haven't touched tree stuff at all). Will look at read_tree().

>> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
>> index 0713bca..69a93fc 100644
>> --- a/builtin-tar-tree.c
>> +++ b/builtin-tar-tree.c
>> @@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
>>               argv++;
>>               argc--;
>>       }
>> +     if (2 <= argc && !strcmp(argv[1], "--fix-attributes")) {
>> +             nargv[nargc++] = argv[1];
>> +             argv++;
>> +             argc--;
>> +     }
>
> I am not sure if it is worth backporting this new option to tar-tree which
> is an essentially backward-compatibility interface, and worse yet, doing
> it poorly (i.e. --fix-attributes must come after --remote= for unexplained
> reason).

It's because git-tar-tree is used in tests and I don't want to migrate
all to git-archive. I don't want to change too much in a deprecated
command. Maybe just remove the option and make --fix-attributes
default for git-tar-tree. In other words, keep git-tar-tree's current
behaviour.

> It would affect a bit more tests, but I think you would want to test both
> the new "normal" mode of operation (generate archives with "git archive"
> and "git tar-tree" without options and compare, for example), instead of
> adding --fix-attributes everywhere.

There is a new test to test the new "normal" mode. I'll think of more.
-- 
Duy

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Maybe just remove the option and make --fix-attributes
> default for git-tar-tree. In other words, keep git-tar-tree's current
> behaviour.

That sounds like a sensible approach to me.

Thanks.

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[René Scharfe]

Nguyen Thai Ngoc Duy schrieb:
> 2009/4/9 Junio C Hamano <gitster@pobox.com>:
>> It would affect a bit more tests, but I think you would want to test both
>> the new "normal" mode of operation (generate archives with "git archive"
>> and "git tar-tree" without options and compare, for example), instead of
>> adding --fix-attributes everywhere.
> 
> There is a new test to test the new "normal" mode. I'll think of more.

The following patch, which can be applied before yours, makes the tests
still pass after your changes by avoiding to use worktree attributes.  I
think it makes sense to create a separate script for the new tests and
eventually move the existing archive attribute tests there.


diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 7641e0d..abb41b0 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -50,7 +50,7 @@ test_expect_success \
 test_expect_success \
     'add ignored file' \
     'echo ignore me >a/ignored &&
-     echo ignored export-ignore >.gitattributes'
+     echo ignored export-ignore >.git/info/attributes'
 
 test_expect_success \
     'add files to repository' \
@@ -64,7 +64,7 @@ test_expect_success \
 test_expect_success \
     'create bare clone' \
     'git clone --bare . bare.git &&
-     cp .gitattributes bare.git/info/attributes'
+     cp .git/info/attributes bare.git/info/attributes'
 
 test_expect_success \
     'remove ignored file' \
@@ -139,10 +139,11 @@ test_expect_success \
 
 test_expect_success \
     'create archives with substfiles' \
-    'echo "substfile?" export-subst >a/.gitattributes &&
+    'cp .git/info/attributes .git/info/attributes.before &&
+     echo "substfile?" export-subst >>.git/info/attributes &&
      git archive HEAD >f.tar &&
      git archive --prefix=prefix/ HEAD >g.tar &&
-     rm a/.gitattributes'
+     mv .git/info/attributes.before .git/info/attributes'
 
 test_expect_success \
     'extract substfiles' \

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[René Scharfe]

René Scharfe schrieb:
> I
> think it makes sense to create a separate script for the new tests and
> eventually move the existing archive attribute tests there.

Something like this?


diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
new file mode 100755
index 0000000..b754f21
--- /dev/null
+++ b/t/t5001-archive-attr.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+test_description='git archive attribute tests'
+
+. ./test-lib.sh
+
+SUBSTFORMAT=%H%n
+
+test_expect_success 'setup' '
+	echo ignored >ignored &&
+	echo ignored export-ignore >>.git/info/attributes &&
+	git add ignored &&
+
+	echo ignored by tree >ignored-by-tree &&
+	echo ignored-by-tree export-ignore >.gitattributes &&
+	git add ignored-by-tree .gitattributes &&
+
+	echo ignored by worktree >ignored-by-worktree &&
+	echo ignored-by-worktree export-ignore >.gitattributes &&
+	git add ignored-by-worktree &&
+
+	printf "A\$Format:%s\$O" "$SUBSTFORMAT" >nosubstfile &&
+	printf "A\$Format:%s\$O" "$SUBSTFORMAT" >substfile1 &&
+	printf "A not substituted O" >substfile2 &&
+	echo "substfile?" export-subst >>.git/info/attributes &&
+	git add nosubstfile substfile1 substfile2 &&
+
+	git commit -m.
+'
+
+test_expect_success 'git archive' '
+	git archive HEAD >archive.tar &&
+	(mkdir archive && cd archive && "$TAR" xf -) <archive.tar
+'
+
+test_expect_success 'git archive with worktree attributes' '
+	git archive --fix-attributes HEAD >worktree.tar &&
+	(mkdir worktree && cd worktree && "$TAR" xf -) <worktree.tar
+'
+
+test_expect_success 'export-ignore' '
+	test ! -e archive/ignored &&
+	test ! -e archive/ignored-by-tree &&
+	test   -e archive/ignored-by-worktree &&
+	test ! -e worktree/ignored &&
+	test   -e worktree/ignored-by-tree &&
+	test ! -e worktree/ignored-by-worktree
+'
+
+test_expect_success 'export-subst' '
+	git log "--pretty=format:A${SUBSTFORMAT}O" HEAD >substfile1.expected &&
+	test_cmp nosubstfile archive/nosubstfile &&
+	test_cmp substfile1.expected archive/substfile1 &&
+	test_cmp substfile2 archive/substfile2
+'
+
+test_expect_success 'git tar-tree vs. git archive with worktree attributes' '
+	git tar-tree HEAD >tar-tree.tar &&
+	test_cmp worktree.tar tar-tree.tar
+'
+
+test_done

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[René Scharfe]

René Scharfe schrieb:
>> I
>> think it makes sense to create a separate script for the new tests and
>> eventually move the existing archive attribute tests there.
> 
> Something like this?

I forgot to add tests against bare repositories.  Otherwise I'd noticed
earlier that read_attr() is only called for non-bare repositories
currently, i.e. your patches won't allow reading of .gitattribute files
from the tree in bare repos.

René

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> René Scharfe schrieb:
>>> I
>>> think it makes sense to create a separate script for the new tests and
>>> eventually move the existing archive attribute tests there.
>> 
>> Something like this?
>
> I forgot to add tests against bare repositories.  Otherwise I'd noticed
> earlier that read_attr() is only called for non-bare repositories
> currently, i.e. your patches won't allow reading of .gitattribute files
> from the tree in bare repos.

Curious.

Shouldn't the call chain look like:

    write_archive()
    ->write_archive_entries()
      ->unpack_trees() to read the tree into the in-core index
      ->git_attr_set_direction()
      ->read_tree_recursive()
        ->write_archive_entry()
          ->git_checkattr()

Ah, bootstrap_attr_stack() and prepare_attr_stack() still assume that you
won't be doing any per-level attributes in a bare repository because the
concept of attributes is inherently tied to having a work tree from their
point of view.

How about this "mostly re-indent with four line removal" patch?

 attr.c |   48 ++++++++++++++++++++++--------------------------
 1 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/attr.c b/attr.c
index 37ca288..f5917de 100644
--- a/attr.c
+++ b/attr.c
@@ -468,13 +468,11 @@ static void bootstrap_attr_stack(void)
 		elem->prev = attr_stack;
 		attr_stack = elem;
 
-		if (!is_bare_repository()) {
-			elem = read_attr(GITATTRIBUTES_FILE, 1);
-			elem->origin = strdup("");
-			elem->prev = attr_stack;
-			attr_stack = elem;
-			debug_push(elem);
-		}
+		elem = read_attr(GITATTRIBUTES_FILE, 1);
+		elem->origin = strdup("");
+		elem->prev = attr_stack;
+		attr_stack = elem;
+		debug_push(elem);
 
 		elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);
 		if (!elem)
@@ -535,25 +533,23 @@ static void prepare_attr_stack(const char *path, int dirlen)
 	/*
 	 * Read from parent directories and push them down
 	 */
-	if (!is_bare_repository()) {
-		while (1) {
-			char *cp;
-
-			len = strlen(attr_stack->origin);
-			if (dirlen <= len)
-				break;
-			strbuf_reset(&pathbuf);
-			strbuf_add(&pathbuf, path, dirlen);
-			strbuf_addch(&pathbuf, '/');
-			cp = strchr(pathbuf.buf + len + 1, '/');
-			strcpy(cp + 1, GITATTRIBUTES_FILE);
-			elem = read_attr(pathbuf.buf, 0);
-			*cp = '\0';
-			elem->origin = strdup(pathbuf.buf);
-			elem->prev = attr_stack;
-			attr_stack = elem;
-			debug_push(elem);
-		}
+	while (1) {
+		char *cp;
+
+		len = strlen(attr_stack->origin);
+		if (dirlen <= len)
+			break;
+		strbuf_reset(&pathbuf);
+		strbuf_add(&pathbuf, path, dirlen);
+		strbuf_addch(&pathbuf, '/');
+		cp = strchr(pathbuf.buf + len + 1, '/');
+		strcpy(cp + 1, GITATTRIBUTES_FILE);
+		elem = read_attr(pathbuf.buf, 0);
+		*cp = '\0';
+		elem->origin = strdup(pathbuf.buf);
+		elem->prev = attr_stack;
+		attr_stack = elem;
+		debug_push(elem);
 	}
 
 	/*

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[René Scharfe]

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> René Scharfe schrieb:
>>>> I
>>>> think it makes sense to create a separate script for the new tests and
>>>> eventually move the existing archive attribute tests there.
>>> Something like this?
>> I forgot to add tests against bare repositories.  Otherwise I'd noticed
>> earlier that read_attr() is only called for non-bare repositories
>> currently, i.e. your patches won't allow reading of .gitattribute files
>> from the tree in bare repos.
> 
> Curious.
> 
> Shouldn't the call chain look like:
> 
>     write_archive()
>     ->write_archive_entries()
>       ->unpack_trees() to read the tree into the in-core index
>       ->git_attr_set_direction()
>       ->read_tree_recursive()
>         ->write_archive_entry()
>           ->git_checkattr()
> 
> Ah, bootstrap_attr_stack() and prepare_attr_stack() still assume that you
> won't be doing any per-level attributes in a bare repository because the
> concept of attributes is inherently tied to having a work tree from their
> point of view.
> 
> How about this "mostly re-indent with four line removal" patch?

Plus the following (on top of Duy's GIT_ATTR_INDEX patch)?

diff --git a/attr.c b/attr.c
index f5917de..cce561b 100644
--- a/attr.c
+++ b/attr.c
@@ -672,6 +672,8 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
 void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
 {
 	enum git_attr_direction old = direction;
+	if (is_bare_repository())
+		new = GIT_ATTR_INDEX;
 	direction = new;
 	if (new != old)
 		drop_attr_stack();

Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Junio C Hamano schrieb:
> ...
>> Ah, bootstrap_attr_stack() and prepare_attr_stack() still assume that you
>> won't be doing any per-level attributes in a bare repository because the
>> concept of attributes is inherently tied to having a work tree from their
>> point of view.
>> 
>> How about this "mostly re-indent with four line removal" patch?
>
> Plus the following (on top of Duy's GIT_ATTR_INDEX patch)?

Actually, the "mostly re-indent" patch breaks things for normal users that
expect the attributes never kicks in when you are doing things in a bare
repository as-is.

> diff --git a/attr.c b/attr.c
> index f5917de..cce561b 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -672,6 +672,8 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
>  void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
>  {
>  	enum git_attr_direction old = direction;
> +	if (is_bare_repository())
> +		new = GIT_ATTR_INDEX;
>  	direction = new;
>  	if (new != old)
>  		drop_attr_stack();

This looks _conceptually_ wrong.

I think your patch came from the fact that check_updates() unconditionally
calls git_attr_set_direction() without checking o->update, and I think it
is a bug in check_updates().

A bare repository is by definition without a work tree, and we shouldn't
be reading the index in general.  I wouldn't go as far to say that a bare
repository should not have the index file, because people often clone
forgetting the --bare option and manually convert that to a bare
repository, and they forget to remove the index that is never used.

	... thinks a bit more ...

I think codepaths that make calls to git_attr_set_direction() inside
is_bare_repository() are special already.  If we were to teach check-attr
how to check attributes for paths _inside a given tree-ish_, most likely
the implementation would be similar to what Duy did for archive; read the
tree into in-core index, set direction to the INDEX, and start using the
attribute mechanism.

So I think we'd rather not have the patch to force GIT_ATTR_INDEX in
set_direction(); if anything, the patch should say something like "if we
are in a bare repository and the new direction is not INDEX, it is a
programming error".

Instead, such specialized codepaths should call set_direction() itself,
perhaps after reading the tree-ish into the in-core index.  And we should
fix the "mostly re-indent" patch not to remove the conditional, but make
the conditional to check "If in a bare repository and the direction is not
explicitly set to INDEX, do not use the attributes".