Re: [PATCH 11/16] Read cache-tree in index-v5

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Aug 3, 2012, 12:41 UTC
Message-ID: <20120803124113.GB3226@tgummerer.unibz.it>
In-Reply-To: <87haskbcmi.fsf@thomas.inf.ethz.ch>

On 08/03, Thomas Rast wrote:

Show 9 quoted lines

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Since the cache-tree data is saved as part of the directory data,
> > we have already read it, when we want to read the cache-tree. The
> > cache-tree then only has to be converted from the directory data.
> 
> I think the first sentence is wrong.  You have already read it at the
> very beginning of reading the index format, when you parsed the
> directory records, haven't you?

Yes, that's what I wanted to say, I'll rephrase this.

Show 10 quoted lines

> > The cache-tree isn't lexically sorted, but after the pathlen at
> > each level, therefore the directories have to be reordered with
> > respect to the ondisk layout.
> 
> I'm not a native speaker either, but I think this does't parse well.
> Maybe
> 
>   The cache-tree data is arranged in a tree, with the children sorted by
>   pathlen at each node.  So we have to rebuild this format from the
>   on-disk directory list.

Thanks, that sounds better.

Show 24 quoted lines

> > +	for (i = 0; i < subtree_nr; i++) {
> > +		struct cache_tree *sub;
> > +		struct cache_tree_sub *subtree;
> > +		char *buf, *name;
> > +
> > +		name = "";
> > +		buf = strtok(down[i].de->pathname, "/");
> 
> man 3 strtok says
> 
>    Be cautious when using these functions.  If you do use them, note
>    that:
> 
>    * These functions modify their first argument.
> 
>    * These functions cannot be used on constant strings.
> 
>    * The identity of the delimiting character is lost.
> 
>    * The strtok() function uses a static buffer while parsing, so it's
>      not thread safe.  Use strtok_r() if this matters to you.
> 
> I don't think the last point will be a problem, but what about modifying
> the argument?

Hrm the function is only called at the end of read_index_v5, after which we don't need the directory entries anymore. If we want to use the directory entries for a new in-memory format later however, we should probably change it. Maybe a comment for the cache_tree_convert_v5 function would be enough?