threads / patch / 9522

patchAdd read_cache to builtin-check-attr

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

## tl;dr

8 messages between Aug 14, 2007 and Aug 15, 2007. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Brian Downing· Aug 14, 2007, 13:18 UTC · lore

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(-)
Show changes to builtin-check-attr.c +5 −0
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
Brian Downing· Aug 14, 2007, 13:22 UTC · re: Brian Downing · lore

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

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
Johannes Schindelin· Aug 14, 2007, 14:08 UTC · re: Brian Downing · lore

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

Hi,
On Tue, 14 Aug 2007, Brian Downing wrote:
Show 6 quoted lines
> 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

Brian Downing· Aug 14, 2007, 14:24 UTC · re: Johannes Schindelin · lore

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

On Tue, Aug 14, 2007 at 03:08:52PM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> 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
Johannes Schindelin· Aug 14, 2007, 14:46 UTC · re: Brian Downing · lore

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

Hi,
On Tue, 14 Aug 2007, Brian Downing wrote:
Show 11 quoted lines
> 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

Junio C Hamano· Aug 14, 2007, 18:38 UTC · re: Brian Downing · lore

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

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).

Brian Downing· Aug 14, 2007, 18:45 UTC · re: Junio C Hamano · lore

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

On Tue, Aug 14, 2007 at 11:38:24AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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

← back to recent threads