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

On Fri, 2015-05-08 at 12:26 -0700, Junio C Hamano wrote:

quoted

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

quoted

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

quoted

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