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

> > +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?

> > +#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.

> > +/**
> > + * 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.

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