From: Daniel Barkalow <barkalow@iabervon.org>
Date: Mon, 12 Sep 2005 20:22:44 GMT
Subject: Re: [PATCH 00/22] cache cursors: an introduction
Message-ID: <Pine.LNX.4.63.0509121614140.23242@iabervon.org>
In-Reply-To: <7vaciiawrm.fsf@assigned-by-dhcp.cox.net>

On Mon, 12 Sep 2005, Junio C Hamano wrote:

> I've only skimmed the surface of your patchset and cannot
> comment on the correctness of all the conversion of active_cache
> users; today is my day-job day not a GIT day.
> 
> I have to say you did quite a lot of work, and I am pleasantly
> surprised to see the massive clean-up this change brings us.  It
> seems like this makes the active_cache users a lot easier to
> read.
> 
> I have a couple of comments on the API, though.
> 
> * Doesn't function to be applied usually want to have its own
>   data when passed to walk, maybe something like this?
> 
>   This was a question I had when I read [PATCH 01/22] before
>   reading the rest of the patches, but the actual conversion
>   does not seem to find much need for it.  A new global variable
>   pathspec is introduced to pass information the API is unable
>   to pass to diff_one() in diff-index.c, which may be a sign
>   that an extra "user data" parameter might help.  Your call.

I agree that it only works for the current conversion, due to there only 
being a limited amount you might try to do in a single git executable 
currently. Long-term, that should be fixed.

> * It may make sense to give another param to describe which
>   cache the caller is talking about so that we can later have
>   more than one cache at the same time:
> 
>   We could argue that this should be left for later rounds.  On
>   the other hand, we will be changing all the cc_* function call
>   sites during that round, which is by definition the places you
>   are touching in this round anyway. 

Wouldn't it be better to only take it in cc_init(), and have the cursor 
remember what it's iterating through?

I'm actually particularly interested in having a pair of caches for 
read-tree, because it would actually like to keep the old index separate 
from the index it's building.

	-Daniel
*This .sig left intentionally blank*