check-attr doesn't respect recursive definitions

check-attr doesn't respect recursive definitions

[Jan Larres]

Hi,

I am trying to write a custom archiving script that checks the
export-ignore attribute to know which files from an ls-files output it
should skip. Through this I noticed that for files in directories for
which the export-ignore (or any other) attribute is set, check-attr
still reports 'unspecified'. More precisely:

$ git init test
Initialized empty Git repository in /home/jan/test/.git/
$ cd test
$ mkdir foo
$ touch foo/bar
$ echo "foo export-ignore" > .gitattributes
$ git check-attr export-ignore foo
foo: export-ignore: set
$ git check-attr export-ignore foo/bar
foo/bar: export-ignore: unspecified

I would expect the last command to also report 'set'. I've also tried
other patterns like 'foo/' and 'foo*', but it didn't make any
difference. Is this expected behaviour? It does make checking the
attributes of single files somewhat more difficult.

git-archive ignores the directory as expected, but unfortunately it
doesn't have an option to just list the files it would archive instead
of actually archiving them.

This is with git version 1.7.10.4.

-Jan

Re: check-attr doesn't respect recursive definitions

[Jeff King]

On Sat, Mar 30, 2013 at 09:45:51AM +0000, Jan Larres wrote:

> I am trying to write a custom archiving script that checks the
> export-ignore attribute to know which files from an ls-files output it
> should skip. Through this I noticed that for files in directories for
> which the export-ignore (or any other) attribute is set, check-attr
> still reports 'unspecified'. More precisely:
> 
> $ git init test
> Initialized empty Git repository in /home/jan/test/.git/
> $ cd test
> $ mkdir foo
> $ touch foo/bar
> $ echo "foo export-ignore" > .gitattributes
> $ git check-attr export-ignore foo
> foo: export-ignore: set
> $ git check-attr export-ignore foo/bar
> foo/bar: export-ignore: unspecified
> 
> I would expect the last command to also report 'set'. I've also tried
> other patterns like 'foo/' and 'foo*', but it didn't make any
> difference. Is this expected behaviour? It does make checking the
> attributes of single files somewhat more difficult.

Yes, it is the expected behavior, though I cannot offhand think of
anything that would break if we did apply it recursively.

> git-archive ignores the directory as expected, but unfortunately it
> doesn't have an option to just list the files it would archive instead
> of actually archiving them.

Yes, git-archive feeds the directories into the attribute machinery as
it traverses the tree, so it actually checks for attributes of "foo"
before recursing. You can do the same, but I agree it is quite a bit
more annoying than just piping "ls-files -z" into "check-attr --stdin
-z".

-Peff

Re: check-attr doesn't respect recursive definitions

[Jeff King]

On Tue, Apr 02, 2013 at 10:31:30AM -0400, Jeff King wrote:

> On Sat, Mar 30, 2013 at 09:45:51AM +0000, Jan Larres wrote:
> 
> > I am trying to write a custom archiving script that checks the
> > export-ignore attribute to know which files from an ls-files output it
> > should skip. Through this I noticed that for files in directories for
> > which the export-ignore (or any other) attribute is set, check-attr
> > still reports 'unspecified'. More precisely:
> > 
> > $ git init test
> > Initialized empty Git repository in /home/jan/test/.git/
> > $ cd test
> > $ mkdir foo
> > $ touch foo/bar
> > $ echo "foo export-ignore" > .gitattributes
> > $ git check-attr export-ignore foo
> > foo: export-ignore: set
> > $ git check-attr export-ignore foo/bar
> > foo/bar: export-ignore: unspecified
> > 
> > I would expect the last command to also report 'set'. I've also tried
> > other patterns like 'foo/' and 'foo*', but it didn't make any
> > difference. Is this expected behaviour? It does make checking the
> > attributes of single files somewhat more difficult.
> 
> Yes, it is the expected behavior, though I cannot offhand think of
> anything that would break if we did apply it recursively.

Naively, such a patch might look something like this:

diff --git a/attr.c b/attr.c
index de170ff..ac04188 100644
--- a/attr.c
+++ b/attr.c
@@ -791,8 +791,18 @@ static void collect_all_attrs(const char *path)
 		check_all_attr[i].value = ATTR__UNKNOWN;
 
 	rem = attr_nr;
-	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
+	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev) {
+		last_slash = path;
+		for (cp = path; *cp; cp++) {
+			if (*cp == '/') {
+				rem = fill(path, cp - path + 1,
+					   last_slash - path,
+					   stk, rem);
+				last_slash = cp;
+			}
+		}
 		rem = fill(path, pathlen, basename_offset, stk, rem);
+	}
 }
 
 int git_check_attr(const char *path, int num, struct git_attr_check *check)

which is based on current "next" (because it has some other related
fixes for handling directories). It seems to work for me, but I suspect
we could do it more optimally. If you have N files in "foo/", this will
check "foo/" against the attribute stack N times, and we should be able
to amortize that work.

Hmm. Actually, thinking on it more, I'm not sure this is even going to
give the right answer anyway, as it will check "foo" against the
"foo/.gitattributes", which is not right. I think we'd need to collect
attributes as we walk the stack in prepare_attr_stack.

So take this as a "this is not how to do it" patch. :)

-Peff

Re: check-attr doesn't respect recursive definitions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, it is the expected behavior, though I cannot offhand think of
> anything that would break if we did apply it recursively.

Conceptually that breaks our brain.  "All files in doc/ directories
are text" and "doc/ directory is text" are two different things, no?

Re: check-attr doesn't respect recursive definitions

[Jeff King]

On Tue, Apr 02, 2013 at 09:11:02AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yes, it is the expected behavior, though I cannot offhand think of
> > anything that would break if we did apply it recursively.
> 
> Conceptually that breaks our brain.  "All files in doc/ directories
> are text" and "doc/ directory is text" are two different things, no?

In some systems, yes, but git does not have any notion of "doc/" as an
item (after all, we track content in files, not directories), so I do
not see what it means to specify a directory except to say "everything
under it has this property".

We already treat "foo/" in gitignore this way (i.e., "everything under
foo is ignored"). And we treat export-ignore the same way in
git-archive.

I agree that saying "Documentation diff=text" is not something I would
expect people to do, but what _should_ it do? Recursively applying under
the directory seems a sane option. I would not want to paint us into a
corner, but I cannot think of any other reasonable thing to do with it
(which is why I phrased it as "I cannot think of anything that would
break", and not "this is a great idea"; counterpoints are welcome).

I am sympathetic to Jan's issue, though. If git were consistent about
applying attributes only to paths, then the repo owner would have
written "dir/* export-ignore", not "dir". But it is not, and git-archive
does ask for attributes on directories, but without providing a good
mechanism for doing those attribute lookups from external programs.

-Peff

Re: check-attr doesn't respect recursive definitions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 02, 2013 at 09:11:02AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Yes, it is the expected behavior, though I cannot offhand think of
>> > anything that would break if we did apply it recursively.
>> 
>> Conceptually that breaks our brain.  "All files in doc/ directories
>> are text" and "doc/ directory is text" are two different things, no?
>
> In some systems, yes, but git does not have any notion of "doc/" as an
> item (after all, we track content in files, not directories), so I do
> not see what it means to specify a directory except to say "everything
> under it has this property".

That was true back when gitattributes (and ignore) was defined to
apply only to the paths we track.  But export-ignore abuses the
attrtibute system, allows a directory to be specified in the match
pattern, and we declared that is a kosher use by the patch that
caused 1.8.1.X regression, no?  So "Git does not have any notion of
"doc/' as an item" is no longer true, I think.

Re: check-attr doesn't respect recursive definitions

[Jeff King]

On Tue, Apr 02, 2013 at 09:43:30AM -0700, Junio C Hamano wrote:

> > In some systems, yes, but git does not have any notion of "doc/" as an
> > item (after all, we track content in files, not directories), so I do
> > not see what it means to specify a directory except to say "everything
> > under it has this property".
> 
> That was true back when gitattributes (and ignore) was defined to
> apply only to the paths we track.  But export-ignore abuses the
> attrtibute system, allows a directory to be specified in the match
> pattern, and we declared that is a kosher use by the patch that
> caused 1.8.1.X regression, no?  So "Git does not have any notion of
> "doc/' as an item" is no longer true, I think.

Yes, but as I explained later, the meaning of "apply an attribute to
dir" in such cases is always equivalent to "apply attribute
recursively to dir/*". So I do not think we are violating that rule to
recursively apply all attributes.

But let's take a step back. I think Jan is trying to do a very
reasonable thing: come up with the same set of paths that git-archive
would. What's the best way to solve that? Recursive application of
attributes is one way, but is there another way we could help with
solving that?

Using:

  git ls-tree --name-only -zrt HEAD |
  git check-attr --stdin -z export-ignore

means we can find out that "foo/" is ignored. But he would have to
manually post-process the output to see that "foo/bar" is below "foo".
Not impossible, but I just wonder if git can be more helpful in figuring
this out.

-Peff

Re: check-attr doesn't respect recursive definitions

[Jeff King]

On Tue, Apr 02, 2013 at 12:51:28PM -0400, Jeff King wrote:

> But let's take a step back. I think Jan is trying to do a very
> reasonable thing: come up with the same set of paths that git-archive
> would. What's the best way to solve that? Recursive application of
> attributes is one way, but is there another way we could help with
> solving that?
> 
> Using:
> 
>   git ls-tree --name-only -zrt HEAD |
>   git check-attr --stdin -z export-ignore
> 
> means we can find out that "foo/" is ignored. But he would have to
> manually post-process the output to see that "foo/bar" is below "foo".
> Not impossible, but I just wonder if git can be more helpful in figuring
> this out.

One way to solve the problem is something like the patch below, which
allows "git archive --format=lstree" to give an lstree-like listing, but
with export-ignore attributes applied.

It feels weirdly specific, though, like there should be a more general
solution.

---
 Makefile         |  1 +
 archive-lstree.c | 43 +++++++++++++++++++++++++++++++++++
 archive.c        |  1 +
 archive.h        |  1 +
 quote.c          |  4 ++--
 quote.h          |  1 +
 6 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index e9749ca..5e63d72 100644
--- a/Makefile
+++ b/Makefile
@@ -772,6 +772,7 @@ LIB_OBJS += archive.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
 LIB_OBJS += archive.o
+LIB_OBJS += archive-lstree.o
 LIB_OBJS += archive-tar.o
 LIB_OBJS += archive-zip.o
 LIB_OBJS += argv-array.o
diff --git a/archive-lstree.c b/archive-lstree.c
new file mode 100644
index 0000000..5ca1bbc
--- /dev/null
+++ b/archive-lstree.c
@@ -0,0 +1,43 @@
+#include "cache.h"
+#include "archive.h"
+#include "quote.h"
+#include "commit.h"
+#include "blob.h"
+
+static int write_lstree_entry(struct archiver_args *args,
+			      const unsigned char *sha1,
+			      const char *path, size_t pathlen,
+			      unsigned int mode)
+{
+	const char *type;
+
+	if (S_ISDIR(mode))
+		return 0;
+	else if (S_ISGITLINK(mode))
+		type = commit_type;
+	else
+		type = blob_type;
+
+	printf("%06o %s %s\t", mode, type, sha1_to_hex(sha1));
+	quote_c_style_counted(path, pathlen, NULL, stdout, 0);
+	printf("\n");
+
+	return 0;
+}
+
+static int write_lstree_archive(const struct archiver *ar,
+				struct archiver_args *args)
+{
+	return write_archive_entries(args, write_lstree_entry);
+}
+
+static struct archiver lstree_archiver = {
+	"lstree",
+	write_lstree_archive,
+	ARCHIVER_REMOTE
+};
+
+void init_lstree_archiver(void)
+{
+	register_archiver(&lstree_archiver);
+}
diff --git a/archive.c b/archive.c
index 3f00da6..4966db7 100644
--- a/archive.c
+++ b/archive.c
@@ -420,6 +420,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	git_config(git_default_config, NULL);
 	init_tar_archiver();
 	init_zip_archiver();
+	init_lstree_archiver();
 
 	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
 	if (nongit) {
diff --git a/archive.h b/archive.h
index 895afcd..1428fc4 100644
--- a/archive.h
+++ b/archive.h
@@ -27,6 +27,7 @@ extern void init_zip_archiver(void);
 
 extern void init_tar_archiver(void);
 extern void init_zip_archiver(void);
+extern void init_lstree_archiver(void);
 
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
 					const unsigned char *sha1,
diff --git a/quote.c b/quote.c
index 911229f..da6b7e4 100644
--- a/quote.c
+++ b/quote.c
@@ -206,8 +206,8 @@ static size_t next_quote_pos(const char *s, ssize_t maxlen)
  *     of name, enclosed with double quotes if asked and needed only.
  *     Return value is the same as in (1).
  */
-static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
-                                    struct strbuf *sb, FILE *fp, int no_dq)
+size_t quote_c_style_counted(const char *name, ssize_t maxlen,
+			     struct strbuf *sb, FILE *fp, int no_dq)
 {
 #undef EMIT
 #define EMIT(c)                                 \
diff --git a/quote.h b/quote.h
index 133155a..f2b8acb 100644
--- a/quote.h
+++ b/quote.h
@@ -55,6 +55,7 @@ extern size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq
 
 extern int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
 extern size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq);
+extern size_t quote_c_style_counted(const char *name, ssize_t len, struct strbuf *, FILE *, int no_dq);
 extern void quote_two_c_style(struct strbuf *, const char *, const char *, int);
 
 extern void write_name_quoted(const char *name, FILE *, int terminator);

Re: check-attr doesn't respect recursive definitions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 02, 2013 at 09:43:30AM -0700, Junio C Hamano wrote:
>
>> > In some systems, yes, but git does not have any notion of "doc/" as an
>> > item (after all, we track content in files, not directories), so I do
>> > not see what it means to specify a directory except to say "everything
>> > under it has this property".
>> 
>> That was true back when gitattributes (and ignore) was defined to
>> apply only to the paths we track.  But export-ignore abuses the
>> attrtibute system, allows a directory to be specified in the match
>> pattern, and we declared that is a kosher use by the patch that
>> caused 1.8.1.X regression, no?  So "Git does not have any notion of
>> "doc/' as an item" is no longer true, I think.
>
> Yes, but as I explained later, the meaning of "apply an attribute to
> dir" in such cases is always equivalent to "apply attribute
> recursively to dir/*". So I do not think we are violating that rule to
> recursively apply all attributes.

I think this is where we disagree.  Attribute does not recursively
apply in general.  It was _never_ designed to (and that is the
authoritative answer, as I was who designed it in Apr 2007 ;-)).

It is not even true to say that "archive" applies export-ignore to
the directory recursively, with or without the recent change.  Would
it allow everything but dir/file to be excluded and still dir/file
to be included in the archive if you have a .gitattribute file like
this?

	dir/ export-ignore
        dir/file !export-ignore

I do not think so.

If we _were_ living in an alternate universe where we did not have
the .gitignore file and instead expressed what it does with an
"ignore" attribute, then having "dir" in the top-level .gitignore
file and "!file" in the .gitignore file in "dir/" directory may be
equivalent to having

	dir/ ignore
        dir/file -ignore

in your .gitattribute, and we would want to ignore dir/other while
including dir/file in the "may be interesting" paths.

The point is, yes, we could choose to define individual attribute
keys to apply recursively, but in general attributes were designed
not to recurse, and no existing attribute recurses.

> But let's take a step back. I think Jan is trying to do a very
> reasonable thing: come up with the same set of paths that git-archive
> would. What's the best way to solve that?

Because the attribute does not recursively apply in general, and it
is entirely up to the application and a particular attribute key to
decide how the key is applied in the context of the application,
"check-attr" by itself cannot know.  You need to know how "archive"
treats "export-ignore" attribute and then use "check-attr" with that
knowledge.

> Using:
>
>   git ls-tree --name-only -zrt HEAD |
>   git check-attr --stdin -z export-ignore
>
> means we can find out that "foo/" is ignored.

Yes, and after learning that, we need to reject everything foo/*
using the knowledge that "git archive" cuts all subpaths of a path
that has "export-ignore" attribute is attached to.

Re: check-attr doesn't respect recursive definitions

[Jeff King]

On Tue, Apr 02, 2013 at 10:16:55AM -0700, Junio C Hamano wrote:

> > Yes, but as I explained later, the meaning of "apply an attribute to
> > dir" in such cases is always equivalent to "apply attribute
> > recursively to dir/*". So I do not think we are violating that rule to
> > recursively apply all attributes.
> 
> I think this is where we disagree.  Attribute does not recursively
> apply in general.  It was _never_ designed to (and that is the
> authoritative answer, as I was who designed it in Apr 2007 ;-)).

My argument was not that they apply recursively in general, but that we
followed an "as if" rule where what happens in the gitignore and
export-ignore cases are indistinguishable from recursive application.
But...

> It is not even true to say that "archive" applies export-ignore to
> the directory recursively, with or without the recent change.  Would
> it allow everything but dir/file to be excluded and still dir/file
> to be included in the archive if you have a .gitattribute file like
> this?
> 
> 	dir/ export-ignore
>         dir/file !export-ignore
> 
> I do not think so.

This is a perfect counter-example. The current behavior _is_
distinguishable from recursive application, and my "as if" above is not
correct.

Thanks for the clarification.

> > But let's take a step back. I think Jan is trying to do a very
> > reasonable thing: come up with the same set of paths that git-archive
> > would. What's the best way to solve that?
> 
> Because the attribute does not recursively apply in general, and it
> is entirely up to the application and a particular attribute key to
> decide how the key is applied in the context of the application,
> "check-attr" by itself cannot know.  You need to know how "archive"
> treats "export-ignore" attribute and then use "check-attr" with that
> knowledge.

Yeah, I had the same thought. So you would have to either:

  1. Hook the feature into git-archive, which knows about how it
     recurses, and can report the correct set of paths.

or

  2. Tell check-attr (or some post-processor) to apply the attribute to
     elements below the path (or possibly to prune out such paths). This
     is not the same as recursive application, because you cannot negate
     it (i.e., you actually find out the final attrs for both "foo" and
     "foo/bar", and then say "the attr for 'foo' overrides the attr for
     'foo/bar'".

I posted a patch for (1), but it felt not-very-general. But (2) also
feels gross and not very general. Even though it could in theory be used
for things besides git-archive, it is really just applying git-archive's
pruning rule, which other programs likely don't care about.

-Peff

Re: check-attr doesn't respect recursive definitions

[Jan Larres]

Thanks for the clarifications. Just a quick comment about the summary:

Jeff King <peff@peff.net> wrote:
> Yeah, I had the same thought. So you would have to either:
>
>   1. Hook the feature into git-archive, which knows about how it
>      recurses, and can report the correct set of paths.
>
> or
>
>   2. Tell check-attr (or some post-processor) to apply the attribute to
>      elements below the path (or possibly to prune out such paths). This
>      is not the same as recursive application, because you cannot negate
>      it (i.e., you actually find out the final attrs for both "foo" and
>      "foo/bar", and then say "the attr for 'foo' overrides the attr for
>      'foo/bar'".
>
> I posted a patch for (1), but it felt not-very-general. But (2) also
> feels gross and not very general. Even though it could in theory be used
> for things besides git-archive, it is really just applying git-archive's
> pruning rule, which other programs likely don't care about.

I actually think the first approach is not such a bad idea, it would
make "archive" more of a general-purpose archiving tool/helper for the
repository rather than just something for the special cases of tars and
zips. But I guess that's somewhat subjective.

Cheers,
Jan

Re: check-attr doesn't respect recursive definitions

[Duy Nguyen]

On Sat, Mar 30, 2013 at 8:45 PM, Jan Larres <jan@majutsushi.net> wrote:
> I would expect the last command to also report 'set'. I've also tried
> other patterns like 'foo/' and 'foo*', but it didn't make any
> difference.

Try "foo/**". You need 1.8.2 though.
--
Duy

Re: check-attr doesn't respect recursive definitions

[Jan Larres]

Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Mar 30, 2013 at 8:45 PM, Jan Larres <jan@majutsushi.net> wrote:
>> I would expect the last command to also report 'set'. I've also tried
>> other patterns like 'foo/' and 'foo*', but it didn't make any
>> difference.
>
> Try "foo/**". You need 1.8.2 though.

That indeed works exactly the way I would like! Thanks a lot!

Jan