dturner@xxxxxxxxxxxxxxxx writes:
> From: David Turner <dturner@xxxxxxxxxxx>
>
> Add a new function, get_tree_entry_follow_symlinks, to tree-walk.[ch].
> The function is not yet used. It will be used to implement git
> cat-file --batch --follow-symlinks.
>
> The function locates an object by path, following symlinks in the
> repository. If the symlinks lead outside the repository, the function
> reports this to the caller.
>
> Signed-off-by: David Turner <dturner@xxxxxxxxxxx>
> ---
> tree-walk.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tree-walk.h | 2 +
> 2 files changed, 224 insertions(+)
>
> diff --git a/tree-walk.c b/tree-walk.c
> index 5dd9a71..6fb4b7d 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -415,6 +415,228 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
> return error;
> }
>
> +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.
> +int get_tree_enty_follow_symlinks(unsigned char *tree_sha1, const char *name, unsigned char *result, unsigned char *result_path, unsigned *mode)
> +{
> + int retval = -1;
> + void *tree;
> + struct dir_state *parents = NULL;
> + size_t parents_cap = 0;
> + ssize_t parents_len = 0;
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.
I am context-switching now; will review the remainder some other
time.
Thanks.
--
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