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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html