Re: [PATCH v3 1/3] tree-walk: learn get_tree_entry_follow_symlinks
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:04:42
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH v3 1/3] tree-walk: learn get_tree_entry_follow_symlinks · <hidden>
dturner@twopensource.com writes:
From: David Turner <redacted> 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 <redacted> ---
Thanks.
quoted hunk
cache.h | 5 ++ tree-walk.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ tree-walk.h | 2 + 3 files changed, 202 insertions(+)diff --git a/cache.h b/cache.h index 3d3244b..679faa9 100644 --- a/cache.h +++ b/cache.h@@ -922,6 +922,11 @@ struct object_context { unsigned char tree[20]; char path[PATH_MAX]; unsigned mode; + /* + * symlink_path is only used by get_tree_entry_follow_symlinks, + * and only for symlinks that point outside the repository. + */ + struct strbuf symlink_path; };
The new low-level helper introduced here does not fill this field,
it seems, and it would make it clear to move the above hunk to
[2/3] where it becomes necessary.
I haven't checked carefully enough to say there isn't any missing
corner cases in the follow-symlinks codepath with confidence, but
the logic there seems very sound. Other than the above suggestion
and removal of "int already_have_tree" variable that is not used, I
have only minor style nits (attached to be squashable) [*1*] on this
patch.
Thanks.
[Footnote]
*1* Style nits:
a. Multi-line comment format.
b. ALLOC_GROW() followed by filling parents_nr slot is concluded
by incrementing parents_nr itself; that single logical unit is
easier to see without a blank line in the middle.
c. In C, we prefer post-increment i++ over pre-increment when its
value is discarded.
d. We tend to omit {} around a single-statement block.
tree-walk.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index f93492d..b7ee665 100644
--- a/tree-walk.c
+++ b/tree-walk.c@@ -484,7 +484,8 @@ int get_tree_entry(const unsigned char *tree_sha1, const char *name, unsigned ch return retval; } -/* This is Linux's built-in max for the number of symlinks to follow. +/* + * This is Linux's built-in max for the number of symlinks to follow. * That limit, of course, does not affect git, but it's a reasonable * choice. */
@@ -514,7 +515,6 @@ int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, u unsigned char current_tree_sha1[20]; struct strbuf namebuf = STRBUF_INIT; enum object_type type; - int already_have_tree = 0; struct tree_desc t = {0}; int follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS; int i;
@@ -541,7 +541,6 @@ int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, u parents[parents_nr].tree = tree; parents[parents_nr].size = size; hashcpy(parents[parents_nr].sha1, root); - parents_nr++; if (namebuf.buf[0] == '\0') {
@@ -562,8 +561,7 @@ int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, u strbuf_remove(&namebuf, 0, 1); } - /* Split namebuf into a first component and a - * remainder */ + /* Split namebuf into a first component and a remainder */ if ((first_slash = strchr(namebuf.buf, '/'))) { *first_slash = 0; remainder = first_slash + 1;
@@ -571,8 +569,10 @@ int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, u if (!strcmp(namebuf.buf, "..")) { struct dir_state *parent; - /* We could end up with .. in the namebuf if - * it appears in a symlink. */ + /* + * We could end up with .. in the namebuf if it + * appears in a symlink. + */ if (parents_nr == 1) { if (remainder)
@@ -599,8 +599,7 @@ int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, u goto done; } - /* Look up the first (or only) path component - * in the tree. */ + /* Look up the first (or only) path component in the tree. */ find_result = find_tree_entry(&t, namebuf.buf, current_tree_sha1, mode); if (find_result) {
@@ -664,9 +663,8 @@ int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, u } } done: - for (i = 0; i < parents_nr; ++i) { + for (i = 0; i < parents_nr; i++) free(parents[i].tree); - } free(parents); strbuf_release(&namebuf);