[PATCH] Add read_cache to builtin-check-attr

[PATCH] Add read_cache to builtin-check-attr

[Brian Downing]

We can now read .gitattributes files out of the index, but the index
must be loaded for this to work.

Signed-off-by: Brian Downing <bdowning@lavos.net>
---
 builtin-check-attr.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/builtin-check-attr.c b/builtin-check-attr.c
index 9d77f76..d949733 100644
--- a/builtin-check-attr.c
+++ b/builtin-check-attr.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "attr.h"
 #include "quote.h"
 
@@ -10,6 +11,10 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	struct git_attr_check *check;
 	int cnt, i, doubledash;
 
+	if (read_cache() < 0) {
+		die("invalid cache");
+	}
+
 	doubledash = -1;
 	for (i = 1; doubledash < 0 && i < argc; i++) {
 		if (!strcmp(argv[i], "--"))
-- 
1.5.3.GIT

Re: [PATCH] Add read_cache to builtin-check-attr

[Brian Downing]

On Tue, Aug 14, 2007 at 08:18:38AM -0500, Brian Downing wrote:
> We can now read .gitattributes files out of the index, but the index
> must be loaded for this to work.

This was supposed to be In-Reply-To Junio's patch, "attr.c: read
.gitattributes from index as well."  It's not much use without it.

-bcd

Re: [PATCH] Add read_cache to builtin-check-attr

[Johannes Schindelin]

Hi,

On Tue, 14 Aug 2007, Brian Downing wrote:

> On Tue, Aug 14, 2007 at 08:18:38AM -0500, Brian Downing wrote:
> > We can now read .gitattributes files out of the index, but the index
> > must be loaded for this to work.
> 
> This was supposed to be In-Reply-To Junio's patch, "attr.c: read
> .gitattributes from index as well."  It's not much use without it.

Shouldn't read_cache() be _only_ called if

- it has not been read yet, and
- .gitattributes was not found in the work tree?

IOW check-attr is the wrong place for your patch IMHO.

Ciao,
Dscho

Re: [PATCH] Add read_cache to builtin-check-attr

[Brian Downing]

On Tue, Aug 14, 2007 at 03:08:52PM +0100, Johannes Schindelin wrote:
> Shouldn't read_cache() be _only_ called if
> 
> - it has not been read yet, and
> - .gitattributes was not found in the work tree?
> 
> IOW check-attr is the wrong place for your patch IMHO.

I admit I just cargo-culted what builtin-checkout-index did upon starting.
Off the cuff, though, I don't see how the cache could ever already be
loaded upon the start of cmd_check_attr, and the way the attr.c code is
written, the cache be loaded when we check attributes or it will default
to the old behavior (only checking the working directory.)

What would you suggest here?

-bcd

Re: [PATCH] Add read_cache to builtin-check-attr

[Johannes Schindelin]

Hi,

On Tue, 14 Aug 2007, Brian Downing wrote:

> On Tue, Aug 14, 2007 at 03:08:52PM +0100, Johannes Schindelin wrote:
> > Shouldn't read_cache() be _only_ called if
> > 
> > - it has not been read yet, and
> > - .gitattributes was not found in the work tree?
> > 
> > IOW check-attr is the wrong place for your patch IMHO.
> 
> I admit I just cargo-culted what builtin-checkout-index did upon starting.
> Off the cuff, though, I don't see how the cache could ever already be
> loaded upon the start of cmd_check_attr,

Right.  I was talking more about read_cache() being called later anyway, 
so you do not have to read the cache if a .gitattributes is there and you 
do not need the index to begin with.

> and the way the attr.c code is
> written, the cache be loaded when we check attributes or it will default
> to the old behavior (only checking the working directory.)

Why not just make sure that the index is read in read_index_data()?  
Something like

	/* read index if that was not already done yet */
	if (!istate->mmap)
		read_index(&istate);

(Yes, I know that read_index() calls read_index_from(), which in turn 
checks that, but read_attr() is called possibly pretty often, right?  So 
we might just as well spare a few cycles here.)

Ciao,
Dscho

Re: [PATCH] Add read_cache to builtin-check-attr

[Junio C Hamano]

Brian Downing <bdowning@lavos.net> writes:

> We can now read .gitattributes files out of the index, but the index
> must be loaded for this to work.

That interface is at too low a level, I am afraid.  Many
commands do want to control when they read the index and it
affects the result, especially when the work tree traversal
implemented in dir.c is involved.

I am not rejecting/objecting, but just raising concerns.  I do
not have time to review this today, but just wanted to see if
you fully assessed the implications (and if so that would save
work on my end).

Re: [PATCH] Add read_cache to builtin-check-attr

[Brian Downing]

On Tue, Aug 14, 2007 at 11:38:24AM -0700, Junio C Hamano wrote:
> That interface is at too low a level, I am afraid.  Many
> commands do want to control when they read the index and it
> affects the result, especially when the work tree traversal
> implemented in dir.c is involved.
> 
> I am not rejecting/objecting, but just raising concerns.  I do
> not have time to review this today, but just wanted to see if
> you fully assessed the implications (and if so that would save
> work on my end).

I really don't understand the implications.  That was just something
that got it working on my end, and I figured I should send it along
in case it was just that simple.

-bcd

Re: [PATCH] Add read_cache to builtin-check-attr

[Junio C Hamano]

Ah, silly me.  The patch was to builtin-check-attr.  I somehow
thought it was to attr.c::git_checkattr().

Thanks, will apply.