Re: [PATCH 1/3] tree-walk: learn get_tree_enty_follow_symlinks

[David Turner <dturner@twopensource.com>]

On Fri, 2015-05-08 at 12:26 -0700, Junio C Hamano wrote:
> > +static int find_tree_entry_nonrecursive(struct tree_desc *t, char *name, unsigned char *result, unsigned *mode) {
> > +	int namelen = strlen(name);
> > +
> > +	while (t->size) {
> 
> Why do you need an almost duplicate of existing find_tree_entry()
> here?  The argument "name" above is not const, so isn't that just
> the matter of the caller to temporarily replace '/' in name[] before
> calling find_tree_entry() if all you wanted to avoid was to prevent
> it from falling into get_tree_entry() codepath?  Or are there more
> to this function?

Oh, right, actually, we already replaced the '/'!  So I can just use the
existing one.  That was silly.  

> > +#define GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS 40
> 
> Is 40 just a randomly chosen number?
> 
> I do not think 40 is particularly unreasonable, but so is 5 (which I
> think is also reasonable and is already used as MAXDEPTH in refs.c
> to follow symrefs), and am curious where that number came from.

http://lxr.linux.no/linux+v3.6.4/fs/namei.c#L783

I'll add a comment to that effect.

> > +/**
> > + * Find a tree entry by following symlinks in tree_sha (which is
> > + * assumed to be the root of the repository).  In the event that a
> > + * symlink points outside the repository (e.g. a link to /foo or a
> > + * root-level link to ../foo), the portion of the link which is
> > + * outside the repository will be copied into result_path (which is
> > + * assumed to hold at least PATH_MAX bytes), and *mode will be set to
> > + * 0.
> 
> As the API to this new function is not constrained by existing
> callers, you might want to consider using strbuf for result_path,
> which would make it easier for both the callers and this function.

The API is sort-of constrained, because the caller has a fixed-size
buffer to fill in (see the 2nd patch in this series).  

> It is customary to name variables to control an ALLOC_GROW()-managed
> array 'foo' as foo_nr and foo_alloc.  Deviating from the convention
> makes the patch harder to read by people who are familiar with it
> without any benefit, and those who are familiar with the existing
> code are the people you want your patch reviewed by.

Will fix.

> I am context-switching now; will review the remainder some other
> time.

Thanks for the review.