{"thread":{"id":"39279","subject":"[PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","startedAt":"2015-05-08T22:38:44Z","lastAt":"2015-05-09T20:34:30Z","messageCount":8,"participants":["dturner@twopensource.com","Junio C Hamano","David Turner"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"260877","messageId":"1431124726-22562-1-git-send-email-dturner@twopensource.com","threadId":"39279","inReplyTo":null,"subject":"[PATCH v2 1/3] tree-walk: learn get_tree_entry_follow_symlinks","fromName":"","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T22:38:44Z","receivedAt":"2015-05-08T22:38:44Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner <dturner@twitter.com>\n\nAdd a new function, get_tree_entry_follow_symlinks, to tree-walk.[ch].\nThe function is not yet used.  It will be used to implement git\ncat-file --batch --follow-symlinks.\n\nThe function locates an object by path, following symlinks in the\nrepository.  If the symlinks lead outside the repository, the function\nreports this to the caller.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n tree-walk.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n tree-walk.h |   2 +\n 2 files changed, 195 insertions(+)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 5dd9a71..2df31a2 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -415,6 +415,12 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \treturn error;\n }\n \n+struct dir_state {\n+\tvoid *tree;\n+\tunsigned long size;\n+\tunsigned char sha1[20];\n+};\n+\n static int find_tree_entry(struct tree_desc *t, const char *name, unsigned char *result, unsigned *mode)\n {\n \tint namelen = strlen(name);\n@@ -478,6 +484,193 @@ int get_tree_entry(const unsigned char *tree_sha1, const char *name, unsigned ch\n \treturn retval;\n }\n \n+/* This is Linux's built-in max for the number of symlinks to follow.\n+ * That limit, of course, does not affect git, but it's a reasonable\n+ * choice.\n+ */\n+#define GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS 40\n+\n+/**\n+ * Find a tree entry by following symlinks in tree_sha (which is\n+ * assumed to be the root of the repository).  In the event that a\n+ * symlink points outside the repository (e.g. a link to /foo or a\n+ * root-level link to ../foo), the portion of the link which is\n+ * outside the repository will be copied into result_path, and *mode\n+ * will be set to 0.  Otherwise, result will be filled in with the\n+ * sha1 of the found object, and *mode will hold the mode of the\n+ * object.\n+ */\n+int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, unsigned char *result, unsigned char *result_path, unsigned *mode)\n+{\n+\tint retval = -1;\n+\tvoid *tree;\n+\tstruct dir_state *parents = NULL;\n+\tsize_t parents_alloc = 0;\n+\tssize_t parents_nr = 0;\n+\tunsigned long size;\n+\tunsigned char root[20];\n+\tunsigned char current_tree_sha1[20];\n+\tstruct strbuf namebuf = STRBUF_INIT;\n+\tenum object_type type;\n+\tint already_have_tree = 0;\n+\tstruct tree_desc t = {0};\n+\tint follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n+\tint i;\n+\n+\tstrbuf_addstr(&namebuf, name);\n+\thashcpy(current_tree_sha1, tree_sha1);\n+\n+\twhile (1) {\n+\t\tchar *first_slash;\n+\t\tchar *remainder = NULL;\n+\t\tint find_result;\n+\n+\t\tif (!t.buffer) {\n+\t\t\ttree = read_object_with_reference(current_tree_sha1,\n+\t\t\t\t\t\t\t  tree_type, &size,\n+\t\t\t\t\t\t\t  root);\n+\t\t\tif (!tree)\n+\t\t\t\tgoto done;\n+\n+\t\t\tALLOC_GROW(parents, parents_nr + 1, parents_alloc);\n+\t\t\tparents[parents_nr].tree = tree;\n+\t\t\tparents[parents_nr].size = size;\n+\t\t\thashcpy(parents[parents_nr].sha1, root);\n+\n+\t\t\tparents_nr++;\n+\n+\t\t\tif (namebuf.buf[0] == '\\0') {\n+\t\t\t\thashcpy(result, root);\n+\t\t\t\tretval = 0;\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\n+\t\t\tif (!size)\n+\t\t\t\tgoto done;\n+\n+\t\t\t/* descend */\n+\t\t\tinit_tree_desc(&t, tree, size);\n+\t\t}\n+\n+\t\t/* Handle symlinks to e.g. a//b by removing leading slashes */\n+\t\twhile (namebuf.buf[0] == '/') {\n+\t\t\tstrbuf_remove(&namebuf, 0, 1);\n+\t\t}\n+\n+\t\t/* Split namebuf into a first component and a\n+\t\t * remainder */\n+\t\tif ((first_slash = strchr(namebuf.buf, '/'))) {\n+\t\t\t*first_slash = 0;\n+\t\t\tremainder = first_slash + 1;\n+\t\t}\n+\n+\t\tif (!strcmp(namebuf.buf, \"..\")) {\n+\t\t\tstruct dir_state *parent;\n+\t\t\t/* We could end up with .. in the namebuf if\n+\t\t\t * it appears in a symlink. */\n+\n+\t\t\tif (parents_nr == 1) {\n+\t\t\t\tif (remainder)\n+\t\t\t\t\t*first_slash = '/';\n+\t\t\t\tif (strlcpy(result_path, namebuf.buf,\n+\t\t\t\t\t    PATH_MAX) < PATH_MAX) {\n+\t\t\t\t\t*mode = 0;\n+\t\t\t\t\tretval = 0;\n+\t\t\t\t}\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t\tparent = &parents[parents_nr - 1];\n+\t\t\tfree(parent->tree);\n+\t\t\tparents_nr--;\n+\t\t\tparent = &parents[parents_nr - 1];\n+\t\t\tinit_tree_desc(&t, parent->tree, parent->size);\n+\t\t\tstrbuf_remove(&namebuf, 0, remainder ? 3 : 2);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* We could end up here via a symlink to dir/.. */\n+\t\tif (namebuf.buf[0] == '\\0') {\n+\t\t\thashcpy(result, parents[parents_nr - 1].sha1);\n+\t\t\tretval = 0;\n+\t\t\tgoto done;\n+\t\t}\n+\n+\t\t/* Look up the first (or only) path component\n+\t\t * in the tree. */\n+\t\tfind_result = find_tree_entry(&t, namebuf.buf,\n+\t\t\t\t\t      current_tree_sha1, mode);\n+\t\tif (find_result) {\n+\t\t\tretval = find_result;\n+\t\t\tgoto done;\n+\t\t}\n+\n+\t\tif (S_ISDIR(*mode)) {\n+\t\t\tif (!remainder) {\n+\t\t\t\thashcpy(result, current_tree_sha1);\n+\t\t\t\tretval = 0;\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t\t/* Descend the tree */\n+\t\t\tt.buffer = NULL;\n+\t\t\tstrbuf_remove(&namebuf, 0,\n+\t\t\t\t      1 + first_slash - namebuf.buf);\n+\t\t} else if (S_ISREG(*mode)) {\n+\t\t\tif (!remainder) {\n+\t\t\t\thashcpy(result, current_tree_sha1);\n+\t\t\t\tretval = 0;\n+\t\t\t}\n+\t\t\tgoto done;\n+\t\t} else if (S_ISLNK(*mode)) {\n+\t\t\t/* Follow a symlink */\n+\t\t\tsize_t link_len, len;\n+\t\t\tchar *contents, *contents_start;\n+\t\t\tstruct dir_state *parent;\n+\n+\t\t\tif (follows_remaining-- == 0)\n+\t\t\t\t/* Too many symlinks followed */\n+\t\t\t\tgoto done;\n+\n+\t\t\tcontents = read_sha1_file(current_tree_sha1, &type,\n+\t\t\t\t\t\t  &link_len);\n+\n+\t\t\tif (!contents)\n+\t\t\t\tgoto done;\n+\n+\t\t\tif (contents[0] == '/') {\n+\t\t\t\tif (strlcpy(result_path,\n+\t\t\t\t\t    contents, PATH_MAX) < PATH_MAX) {\n+\t\t\t\t\t*mode = 0;\n+\t\t\t\t\tretval = 0;\n+\t\t\t\t}\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\n+\t\t\tif (remainder)\n+\t\t\t\tlen = first_slash - namebuf.buf;\n+\t\t\telse\n+\t\t\t\tlen = namebuf.len;\n+\n+\t\t\tcontents_start = contents;\n+\n+\t\t\tparent = &parents[parents_nr - 1];\n+\t\t\tinit_tree_desc(&t, parent->tree, parent->size);\n+\t\t\tstrbuf_splice(&namebuf, 0, len,\n+\t\t\t\t      contents_start, link_len);\n+\t\t\tif (remainder)\n+\t\t\t\tnamebuf.buf[link_len] = '/';\n+\t\t\tfree(contents);\n+\t\t}\n+\t}\n+done:\n+\tfor (i = 0; i < parents_nr; ++i) {\n+\t\tfree(parents[i].tree);\n+\t}\n+\tfree(parents);\n+\n+\tstrbuf_release(&namebuf);\n+\treturn retval;\n+}\n+\n static int match_entry(const struct pathspec_item *item,\n \t\t       const struct name_entry *entry, int pathlen,\n \t\t       const char *match, int matchlen,\ndiff --git a/tree-walk.h b/tree-walk.h\nindex ae7fb3a..d9ad768 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -40,6 +40,8 @@ struct traverse_info;\n typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *);\n int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info);\n \n+int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, unsigned char *result, unsigned char *result_path, unsigned *mode);\n+\n struct traverse_info {\n \tstruct traverse_info *prev;\n \tstruct name_entry name;\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"260876","messageId":"1431124726-22562-2-git-send-email-dturner@twopensource.com","threadId":"39279","inReplyTo":"1431124726-22562-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T22:38:45Z","receivedAt":"2015-05-08T22:38:45Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner <dturner@twitter.com>\n\nWire up get_sha1_with_context to call get_tree_entry_follow_symlinks\nwhen GET_SHA1_FOLLOW_SYMLINKS is passed in flags. G_S_FOLLOW_SYMLINKS\nis incompatible with G_S_ONLY_TO_DIE because the diagnosis that\nONLY_TO_DIE triggers does not consider symlinks.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n cache.h     | 15 ++++++++-------\n sha1_name.c | 24 ++++++++++++++++--------\n 2 files changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..16743de 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -924,13 +924,14 @@ struct object_context {\n \tunsigned mode;\n };\n \n-#define GET_SHA1_QUIETLY        01\n-#define GET_SHA1_COMMIT         02\n-#define GET_SHA1_COMMITTISH     04\n-#define GET_SHA1_TREE          010\n-#define GET_SHA1_TREEISH       020\n-#define GET_SHA1_BLOB\t       040\n-#define GET_SHA1_ONLY_TO_DIE 04000\n+#define GET_SHA1_QUIETLY           01\n+#define GET_SHA1_COMMIT            02\n+#define GET_SHA1_COMMITTISH        04\n+#define GET_SHA1_TREE             010\n+#define GET_SHA1_TREEISH          020\n+#define GET_SHA1_BLOB             040\n+#define GET_SHA1_FOLLOW_SYMLINKS 0100\n+#define GET_SHA1_ONLY_TO_DIE    04000\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n extern int get_sha1_commit(const char *str, unsigned char *sha1);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6d10f05..cbe679e 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1434,15 +1434,21 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\tnew_filename = resolve_relative_path(filename);\n \t\t\tif (new_filename)\n \t\t\t\tfilename = new_filename;\n-\t\t\tret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);\n-\t\t\tif (ret && only_to_die) {\n-\t\t\t\tdiagnose_invalid_sha1_path(prefix, filename,\n-\t\t\t\t\t\t\t   tree_sha1,\n-\t\t\t\t\t\t\t   name, len);\n+\t\t\tif (flags & GET_SHA1_FOLLOW_SYMLINKS) {\n+\t\t\t\tret = get_tree_entry_follow_symlinks(tree_sha1,\n+\t\t\t\t\tfilename, sha1, oc->path, &oc->mode);\n+\t\t\t} else {\n+\t\t\t\tret = get_tree_entry(tree_sha1, filename,\n+\t\t\t\t\t\t     sha1, &oc->mode);\n+\t\t\t\tif (ret && only_to_die) {\n+\t\t\t\t\tdiagnose_invalid_sha1_path(prefix,\n+\t\t\t\t\t\t\t\t   filename,\n+\t\t\t\t\t\t\t\t   tree_sha1,\n+\t\t\t\t\t\t\t\t   name, len);\n+\t\t\t\t}\n+\t\t\t\thashcpy(oc->tree, tree_sha1);\n+\t\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n \t\t\t}\n-\t\t\thashcpy(oc->tree, tree_sha1);\n-\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n-\n \t\t\tfree(new_filename);\n \t\t\treturn ret;\n \t\t} else {\n@@ -1469,5 +1475,7 @@ void maybe_die_on_misspelt_object_name(const char *name, const char *prefix)\n \n int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc)\n {\n+\tif (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & GET_SHA1_ONLY_TO_DIE)\n+\t\tdie(\"BUG: incompatible flags for get_sha1_with_context\");\n \treturn get_sha1_with_context_1(str, flags, NULL, sha1, orc);\n }\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"260878","messageId":"1431124726-22562-3-git-send-email-dturner@twopensource.com","threadId":"39279","inReplyTo":"1431124726-22562-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v2 3/3] cat-file: add --follow-symlinks to --batch","fromName":"","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T22:38:46Z","receivedAt":"2015-05-08T22:38:46Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner <dturner@twitter.com>\n\nThis wires the in-repo-symlink following code through to the cat-file\nbuiltin.  In the event of an out-of-repo link, cat-file will print\nthe link in a new format.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n Documentation/git-cat-file.txt |  28 ++++++-\n builtin/cat-file.c             |  23 +++++-\n t/t1006-cat-file.sh            | 184 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 230 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex f6a16f4..18b67a3 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git cat-file' (-t | -s | -e | -p | <type> | --textconv ) <object>\n-'git cat-file' (--batch | --batch-check) < <list-of-objects>\n+'git cat-file' (--batch | --batch-check) [--follow-symlinks] < <list-of-objects>\n \n DESCRIPTION\n -----------\n@@ -69,6 +69,19 @@ OPTIONS\n \tnot be combined with any other options or arguments.  See the\n \tsection `BATCH OUTPUT` below for details.\n \n+--follow-symlinks::\n+\tFollow symlinks inside the repository.  Instead of providing\n+\toutput about the link itself, provide output about the linked-to\n+\tobject.  This option requires --batch or --batch-check.  In the\n+\tevent of a symlink loop (or more than 40 symlinks in a symlink\n+\tresolution chain), the file will be treated as missing.  If a\n+\tsymlink points outside the repository (e.g. a link to /foo or a\n+\troot-level link to ../foo), the portion of the link which is\n+\toutside the repository will be printed.  Follow-symlinks will\n+\tbe silently turned off if <object> specifies an object in the\n+\tindex rather than one in the object database.\n+\n+\n OUTPUT\n ------\n If '-t' is specified, one of the <type>.\n@@ -148,6 +161,19 @@ the repository, then `cat-file` will ignore any custom format and print:\n <object> SP missing LF\n ------------\n \n+If --follow-symlinks is used, and a symlink in the repository points\n+outside the repository, then `cat-file` will ignore any custom format\n+and print:\n+\n+------------\n+symlink SP <size> LF <symlink> LF\n+------------\n+\n+The symlink will either be absolute (beginning with a /), or relative\n+to the repository root.  For instance, if dir/link points to ../../foo,\n+then <symlink> will be ../foo.  <size> is the size of the symlink in\n+bytes.\n+\n \n CAVEATS\n -------\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex df99df4..8e96dac 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -224,6 +224,7 @@ static void print_object_or_die(int fd, struct expand_data *data)\n \n struct batch_options {\n \tint enabled;\n+\tint follow_symlinks;\n \tint print_contents;\n \tconst char *format;\n };\n@@ -232,16 +233,24 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\t\t    struct expand_data *data)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct object_context ctx;\n+\tint flags = opt->follow_symlinks ? GET_SHA1_FOLLOW_SYMLINKS : 0;\n \n \tif (!obj_name)\n \t   return 1;\n \n-\tif (get_sha1(obj_name, data->sha1)) {\n+\tif (get_sha1_with_context(obj_name, flags, data->sha1, &ctx)) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n \t\treturn 0;\n \t}\n \n+\tif (ctx.mode == 0) {\n+\t\tprintf(\"symlink %\"PRIuMAX\"\\n%s\\n\", (uintmax_t)strlen(ctx.path),\n+\t\t       ctx.path);\n+\t\treturn 0;\n+\t}\n+\n \tif (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n@@ -342,9 +351,8 @@ static int batch_option_callback(const struct option *opt,\n {\n \tstruct batch_options *bo = opt->value;\n \n-\tif (unset) {\n-\t\tmemset(bo, 0, sizeof(*bo));\n-\t\treturn 0;\n+\tif (bo->enabled) {\n+\t\treturn 1;\n \t}\n \n \tbo->enabled = 1;\n@@ -369,6 +377,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n \t\tOPT_SET_INT(0, \"textconv\", &opt,\n \t\t\t    N_(\"for blob objects, run textconv on object's content\"), 'c'),\n+\t\tOPT_SET_INT(0, \"follow-symlinks\", &batch.follow_symlinks,\n+\t\t\tN_(\"follow in-repo symlinks; report out-of-repo symlinks (requires --batch or --batch-check)\"),\n+\t\t\t    1),\n \t\t{ OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n \t\t\tN_(\"show info and content of objects fed from the standard input\"),\n \t\t\tPARSE_OPT_OPTARG, batch_option_callback },\n@@ -402,6 +413,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(cat_file_usage, options);\n \t}\n \n+\tif (batch.follow_symlinks && !batch.enabled) {\n+\t\tusage_with_options(cat_file_usage, options);\n+\t}\n+\n \tif (batch.enabled)\n \t\treturn batch_objects(&batch);\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ab36b1e..a9ef3a6 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -189,6 +189,13 @@ do\n     '\n done\n \n+for opt in t s e p\n+do\n+    test_expect_success \"Passing -$opt with --follow-symlinks fails\" '\n+\t    test_must_fail git cat-file --follow-symlinks -$opt $hello_sha1\n+\t'\n+done\n+\n test_expect_success \"--batch-check for a non-existent named object\" '\n     test \"foobar42 missing\n foobar84 missing\" = \\\n@@ -296,4 +303,181 @@ test_expect_success '%(deltabase) reports packed delta bases' '\n \t}\n '\n \n+# Tests for git cat-file --follow-symlinks\n+test_expect_success 'prep for symlink tests' '\n+\techo_without_newline \"$hello_content\" > morx &&\n+\tln -s morx same-dir-link &&\n+\tln -s ../fleem out-of-repo-link &&\n+\tln -s .. out-of-repo-link-dir &&\n+\tln -s same-dir-link link-to-link &&\n+\tln -s nope broken-same-dir-link &&\n+\tmkdir dir &&\n+\tln -s ../morx dir/parent-dir-link &&\n+\tln -s .. dir/link-dir &&\n+\tln -s ../../escape dir/out-of-repo-link &&\n+\tln -s ../.. dir/out-of-repo-link-dir &&\n+\tln -s nope dir/broken-link-in-dir &&\n+\tmkdir dir/subdir &&\n+\tln -s ../../morx dir/subdir/grandparent-dir-link &&\n+\tln -s ../../../great-escape dir/subdir/out-of-repo-link &&\n+\tln -s ../../.. dir/subdir/out-of-repo-link-dir &&\n+\tln -s ../../../ dir/subdir/out-of-repo-link-dir-trailing &&\n+\tln -s ../parent-dir-link dir/subdir/parent-dir-link-to-link &&\n+\techo_without_newline \"$hello_content\" >dir/subdir/ind2 &&\n+\techo_without_newline \"$hello_content\" >dir/ind1 &&\n+\tln -s dir dirlink &&\n+\tln -s dir/subdir subdirlink &&\n+\tln -s subdir/ind2 dir/link-to-child &&\n+\tln -s dir/link-to-child link-to-down-link &&\n+\tln -s dir/.. up-down &&\n+\tln -s dir/../ up-down-trailing &&\n+\tln -s dir/../morx up-down-file &&\n+\tln -s dir/../../morx up-up-down-file &&\n+\tln -s subdirlink/../../morx up-two-down-file &&\n+\tln -s loop1 loop2 &&\n+\tln -s loop2 loop1 &&\n+\tgit add . &&\n+\tgit commit -am \"test\"\n+'\n+\n+echo $hello_sha1 blob $hello_size > found\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for non-links' '\n+\techo HEAD:morx | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual &&\n+\techo HEAD:nope missing > expect &&\n+\techo HEAD:nope | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for in-repo, same-dir links' '\n+\techo HEAD:same-dir-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for broken in-repo, same-dir links' '\n+\techo HEAD:broken-same-dir-link missing > expect &&\n+\techo HEAD:broken-same-dir-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for same-dir links-to-links' '\n+\techo HEAD:link-to-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for parent-dir links' '\n+\techo HEAD:dir/parent-dir-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual &&\n+\techo HEAD:dir/parent-dir-link/nope missing > expect &&\n+\techo HEAD:dir/parent-dir-link/nope | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for .. links' '\n+\techo HEAD:dir/link-dir/nope missing > expect &&\n+\techo HEAD:dir/link-dir/nope | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:dir/link-dir/morx | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual &&\n+\techo HEAD:dir/broken-link-in-dir missing > expect &&\n+\techo HEAD:dir/broken-link-in-dir | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for ../.. links' '\n+\techo HEAD:dir/subdir/grandparent-dir-link/nope missing > expect &&\n+\techo HEAD:dir/subdir/grandparent-dir-link/nope | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:dir/subdir/grandparent-dir-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual &&\n+\techo HEAD:dir/subdir/parent-dir-link-to-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for dir/ links' '\n+\techo HEAD:dirlink/morx missing > expect &&\n+\techo HEAD:dirlink/morx | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo $hello_sha1 blob $hello_size > expect &&\n+\techo HEAD:dirlink/ind1 | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for dir/subdir links' '\n+\techo HEAD:subdirlink/morx missing > expect &&\n+\techo HEAD:subdirlink/morx | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:subdirlink/ind2 | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for dir -> subdir links' '\n+\techo HEAD:dir/link-to-child/morx missing > expect &&\n+\techo HEAD:dir/link-to-child/morx | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:dir/link-to-child | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual &&\n+\techo HEAD:link-to-down-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for out-of-repo symlinks' '\n+\techo symlink 8 > expect &&\n+\techo ../fleem >> expect &&\n+\techo HEAD:out-of-repo-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo symlink 2 > expect &&\n+\techo .. >> expect &&\n+\techo HEAD:out-of-repo-link-dir | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for out-of-repo symlinks in dirs' '\n+\techo symlink 9 > expect &&\n+\techo ../escape >> expect &&\n+\techo HEAD:dir/out-of-repo-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo symlink 2 > expect &&\n+\techo .. >> expect &&\n+\techo HEAD:dir/out-of-repo-link-dir | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for out-of-repo symlinks in subdirs' '\n+\techo symlink 15 > expect &&\n+\techo ../great-escape >> expect &&\n+\techo HEAD:dir/subdir/out-of-repo-link | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo symlink 2 > expect &&\n+\techo .. >> expect &&\n+\techo HEAD:dir/subdir/out-of-repo-link-dir | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo symlink 3 > expect &&\n+\techo ../ >> expect &&\n+\techo HEAD:dir/subdir/out-of-repo-link-dir-trailing | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlinks works for symlinks with internal ..' '\n+\techo HEAD: | git cat-file --batch-check > expect &&\n+\techo HEAD:up-down | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:up-down-file | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual &&\n+\techo symlink 7 > expect &&\n+\techo ../morx >> expect &&\n+\techo HEAD:up-up-down-file | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\techo HEAD:up-two-down-file | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp found actual\n+'\n+\n+test_expect_success 'git cat-file --batch-check --follow-symlink breaks loops' '\n+\techo HEAD:loop1 missing > expect &&\n+\techo HEAD:loop1 | git cat-file --batch-check --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n test_done\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"260891","messageId":"xmqq7fsiifcz.fsf@gitster.dls.corp.google.com","threadId":"39279","inReplyTo":"1431124726-22562-2-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-09T03:45:00Z","receivedAt":"2015-05-09T03:45:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"dturner@twopensource.com writes:\n\n> From: David Turner <dturner@twitter.com>\n>\n> Wire up get_sha1_with_context to call get_tree_entry_follow_symlinks\n> when GET_SHA1_FOLLOW_SYMLINKS is passed in flags. G_S_FOLLOW_SYMLINKS\n> is incompatible with G_S_ONLY_TO_DIE because the diagnosis that\n> ONLY_TO_DIE triggers does not consider symlinks.\n\nIs \"does not consider\" something fundamental, or it just happens to\nbe that way right now?\n"},{"id":"260893","messageId":"xmqqzj5egxb2.fsf@gitster.dls.corp.google.com","threadId":"39279","inReplyTo":"1431124726-22562-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v2 1/3] tree-walk: learn get_tree_entry_follow_symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-09T05:00:17Z","receivedAt":"2015-05-09T05:00:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"dturner@twopensource.com writes:\n\n> From: David Turner <dturner@twitter.com>\n>\n> Add a new function, get_tree_entry_follow_symlinks, to tree-walk.[ch].\n> The function is not yet used.  It will be used to implement git\n> cat-file --batch --follow-symlinks.\n>\n> The function locates an object by path, following symlinks in the\n> repository.  If the symlinks lead outside the repository, the function\n> reports this to the caller.\n>\n> Signed-off-by: David Turner <dturner@twitter.com>\n> ---\n>  tree-walk.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  tree-walk.h |   2 +\n>  2 files changed, 195 insertions(+)\n\nOverall this looks fairly straight-forward and much cleaner than I\nfeared it would end up to be.\n\n\tSide Note: Earlier I stupidly thought that it may have to\n\tinvolve ugly code to avoid penalizing common cases by first\n\tattempting to resolve the whole thing (e.g. a/b/c) and fall\n\tback to \"is there any symbolic link?\" codepath only after\n\tseeing it fail (i.e. if we can find \"a/b/c\" in the tree, by\n\tdefinition \"a/\" and \"a/b/\" cannot be a symbolic link).  But\n\tof course that is a silly thought. As this implementation\n\tmakes it clear, for us to resolve \"a/b/c\" in the common case\n\twithout symbolic link, we need to incrementally find \"a\" in\n\tthe root, then find \"b\" in that \"a\", and then find \"c\" in\n\t\"a/b\" _anyway_.\n\n\n\nI'll leave others (or maybe tomorrow's myself ;-) to nitpick on\nstyles, code layout and possibly logic errors (if any), but from\na cursory read, I like the general structure of this patch a lot.\n\nGood job.\n\nThanks.\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 5dd9a71..2df31a2 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -415,6 +415,12 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n>  \treturn error;\n>  }\n>  \n> +struct dir_state {\n> +\tvoid *tree;\n> +\tunsigned long size;\n> +\tunsigned char sha1[20];\n> +};\n> +\n>  static int find_tree_entry(struct tree_desc *t, const char *name, unsigned char *result, unsigned *mode)\n>  {\n>  \tint namelen = strlen(name);\n> @@ -478,6 +484,193 @@ int get_tree_entry(const unsigned char *tree_sha1, const char *name, unsigned ch\n>  \treturn retval;\n>  }\n>  \n> +/* This is Linux's built-in max for the number of symlinks to follow.\n> + * That limit, of course, does not affect git, but it's a reasonable\n> + * choice.\n> + */\n> +#define GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS 40\n> +\n> +/**\n> + * Find a tree entry by following symlinks in tree_sha (which is\n> + * assumed to be the root of the repository).  In the event that a\n> + * symlink points outside the repository (e.g. a link to /foo or a\n> + * root-level link to ../foo), the portion of the link which is\n> + * outside the repository will be copied into result_path, and *mode\n> + * will be set to 0.  Otherwise, result will be filled in with the\n> + * sha1 of the found object, and *mode will hold the mode of the\n> + * object.\n> + */\n> +int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, unsigned char *result, unsigned char *result_path, unsigned *mode)\n> +{\n> +\tint retval = -1;\n> +\tvoid *tree;\n> +\tstruct dir_state *parents = NULL;\n> +\tsize_t parents_alloc = 0;\n> +\tssize_t parents_nr = 0;\n> +\tunsigned long size;\n> +\tunsigned char root[20];\n> +\tunsigned char current_tree_sha1[20];\n> +\tstruct strbuf namebuf = STRBUF_INIT;\n> +\tenum object_type type;\n> +\tint already_have_tree = 0;\n> +\tstruct tree_desc t = {0};\n> +\tint follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n> +\tint i;\n> +\n> +\tstrbuf_addstr(&namebuf, name);\n> +\thashcpy(current_tree_sha1, tree_sha1);\n> +\n> +\twhile (1) {\n> +\t\tchar *first_slash;\n> +\t\tchar *remainder = NULL;\n> +\t\tint find_result;\n> +\n> +\t\tif (!t.buffer) {\n> +\t\t\ttree = read_object_with_reference(current_tree_sha1,\n> +\t\t\t\t\t\t\t  tree_type, &size,\n> +\t\t\t\t\t\t\t  root);\n> +\t\t\tif (!tree)\n> +\t\t\t\tgoto done;\n> +\n> +\t\t\tALLOC_GROW(parents, parents_nr + 1, parents_alloc);\n> +\t\t\tparents[parents_nr].tree = tree;\n> +\t\t\tparents[parents_nr].size = size;\n> +\t\t\thashcpy(parents[parents_nr].sha1, root);\n> +\n> +\t\t\tparents_nr++;\n> +\n> +\t\t\tif (namebuf.buf[0] == '\\0') {\n> +\t\t\t\thashcpy(result, root);\n> +\t\t\t\tretval = 0;\n> +\t\t\t\tgoto done;\n> +\t\t\t}\n> +\n> +\t\t\tif (!size)\n> +\t\t\t\tgoto done;\n> +\n> +\t\t\t/* descend */\n> +\t\t\tinit_tree_desc(&t, tree, size);\n> +\t\t}\n> +\n> +\t\t/* Handle symlinks to e.g. a//b by removing leading slashes */\n> +\t\twhile (namebuf.buf[0] == '/') {\n> +\t\t\tstrbuf_remove(&namebuf, 0, 1);\n> +\t\t}\n> +\n> +\t\t/* Split namebuf into a first component and a\n> +\t\t * remainder */\n> +\t\tif ((first_slash = strchr(namebuf.buf, '/'))) {\n> +\t\t\t*first_slash = 0;\n> +\t\t\tremainder = first_slash + 1;\n> +\t\t}\n> +\n> +\t\tif (!strcmp(namebuf.buf, \"..\")) {\n> +\t\t\tstruct dir_state *parent;\n> +\t\t\t/* We could end up with .. in the namebuf if\n> +\t\t\t * it appears in a symlink. */\n> +\n> +\t\t\tif (parents_nr == 1) {\n> +\t\t\t\tif (remainder)\n> +\t\t\t\t\t*first_slash = '/';\n> +\t\t\t\tif (strlcpy(result_path, namebuf.buf,\n> +\t\t\t\t\t    PATH_MAX) < PATH_MAX) {\n> +\t\t\t\t\t*mode = 0;\n> +\t\t\t\t\tretval = 0;\n> +\t\t\t\t}\n> +\t\t\t\tgoto done;\n> +\t\t\t}\n> +\t\t\tparent = &parents[parents_nr - 1];\n> +\t\t\tfree(parent->tree);\n> +\t\t\tparents_nr--;\n> +\t\t\tparent = &parents[parents_nr - 1];\n> +\t\t\tinit_tree_desc(&t, parent->tree, parent->size);\n> +\t\t\tstrbuf_remove(&namebuf, 0, remainder ? 3 : 2);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* We could end up here via a symlink to dir/.. */\n> +\t\tif (namebuf.buf[0] == '\\0') {\n> +\t\t\thashcpy(result, parents[parents_nr - 1].sha1);\n> +\t\t\tretval = 0;\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\t/* Look up the first (or only) path component\n> +\t\t * in the tree. */\n> +\t\tfind_result = find_tree_entry(&t, namebuf.buf,\n> +\t\t\t\t\t      current_tree_sha1, mode);\n> +\t\tif (find_result) {\n> +\t\t\tretval = find_result;\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\tif (S_ISDIR(*mode)) {\n> +\t\t\tif (!remainder) {\n> +\t\t\t\thashcpy(result, current_tree_sha1);\n> +\t\t\t\tretval = 0;\n> +\t\t\t\tgoto done;\n> +\t\t\t}\n> +\t\t\t/* Descend the tree */\n> +\t\t\tt.buffer = NULL;\n> +\t\t\tstrbuf_remove(&namebuf, 0,\n> +\t\t\t\t      1 + first_slash - namebuf.buf);\n> +\t\t} else if (S_ISREG(*mode)) {\n> +\t\t\tif (!remainder) {\n> +\t\t\t\thashcpy(result, current_tree_sha1);\n> +\t\t\t\tretval = 0;\n> +\t\t\t}\n> +\t\t\tgoto done;\n> +\t\t} else if (S_ISLNK(*mode)) {\n> +\t\t\t/* Follow a symlink */\n> +\t\t\tsize_t link_len, len;\n> +\t\t\tchar *contents, *contents_start;\n> +\t\t\tstruct dir_state *parent;\n> +\n> +\t\t\tif (follows_remaining-- == 0)\n> +\t\t\t\t/* Too many symlinks followed */\n> +\t\t\t\tgoto done;\n> +\n> +\t\t\tcontents = read_sha1_file(current_tree_sha1, &type,\n> +\t\t\t\t\t\t  &link_len);\n> +\n> +\t\t\tif (!contents)\n> +\t\t\t\tgoto done;\n> +\n> +\t\t\tif (contents[0] == '/') {\n> +\t\t\t\tif (strlcpy(result_path,\n> +\t\t\t\t\t    contents, PATH_MAX) < PATH_MAX) {\n> +\t\t\t\t\t*mode = 0;\n> +\t\t\t\t\tretval = 0;\n> +\t\t\t\t}\n> +\t\t\t\tgoto done;\n> +\t\t\t}\n> +\n> +\t\t\tif (remainder)\n> +\t\t\t\tlen = first_slash - namebuf.buf;\n> +\t\t\telse\n> +\t\t\t\tlen = namebuf.len;\n> +\n> +\t\t\tcontents_start = contents;\n> +\n> +\t\t\tparent = &parents[parents_nr - 1];\n> +\t\t\tinit_tree_desc(&t, parent->tree, parent->size);\n> +\t\t\tstrbuf_splice(&namebuf, 0, len,\n> +\t\t\t\t      contents_start, link_len);\n> +\t\t\tif (remainder)\n> +\t\t\t\tnamebuf.buf[link_len] = '/';\n> +\t\t\tfree(contents);\n> +\t\t}\n> +\t}\n> +done:\n> +\tfor (i = 0; i < parents_nr; ++i) {\n> +\t\tfree(parents[i].tree);\n> +\t}\n> +\tfree(parents);\n> +\n> +\tstrbuf_release(&namebuf);\n> +\treturn retval;\n> +}\n> +\n>  static int match_entry(const struct pathspec_item *item,\n>  \t\t       const struct name_entry *entry, int pathlen,\n>  \t\t       const char *match, int matchlen,\n> diff --git a/tree-walk.h b/tree-walk.h\n> index ae7fb3a..d9ad768 100644\n> --- a/tree-walk.h\n> +++ b/tree-walk.h\n> @@ -40,6 +40,8 @@ struct traverse_info;\n>  typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *);\n>  int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info);\n>  \n> +int get_tree_entry_follow_symlinks(unsigned char *tree_sha1, const char *name, unsigned char *result, unsigned char *result_path, unsigned *mode);\n> +\n>  struct traverse_info {\n>  \tstruct traverse_info *prev;\n>  \tstruct name_entry name;\n"},{"id":"260903","messageId":"xmqqlhgxhcqb.fsf@gitster.dls.corp.google.com","threadId":"39279","inReplyTo":"xmqq7fsiifcz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-09T17:39:24Z","receivedAt":"2015-05-09T17:39:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> dturner@twopensource.com writes:\n>\n>> From: David Turner <dturner@twitter.com>\n>>\n>> Wire up get_sha1_with_context to call get_tree_entry_follow_symlinks\n>> when GET_SHA1_FOLLOW_SYMLINKS is passed in flags. G_S_FOLLOW_SYMLINKS\n>> is incompatible with G_S_ONLY_TO_DIE because the diagnosis that\n>> ONLY_TO_DIE triggers does not consider symlinks.\n>\n> Is \"does not consider\" something fundamental, or it just happens to\n> be that way right now?\n\nRegardless of the answer to this question, I find the last part of\nthis hunk puzzling.\n\n\n> +\t\t\tif (flags & GET_SHA1_FOLLOW_SYMLINKS) {\n> +\t\t\t\tret = get_tree_entry_follow_symlinks(tree_sha1,\n> +\t\t\t\t\tfilename, sha1, oc->path, &oc->mode);\n> +\t\t\t} else {\n> +\t\t\t\tret = get_tree_entry(tree_sha1, filename,\n> +\t\t\t\t\t\t     sha1, &oc->mode);\n> +\t\t\t\tif (ret && only_to_die) {\n> +\t\t\t\t\tdiagnose_invalid_sha1_path(prefix,\n> +\t\t\t\t\t\t\t\t   filename,\n> +\t\t\t\t\t\t\t\t   tree_sha1,\n> +\t\t\t\t\t\t\t\t   name, len);\n> +\t\t\t\t}\n> +\t\t\t\thashcpy(oc->tree, tree_sha1);\n> +\t\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n>  \t\t\t}\n> -\t\t\thashcpy(oc->tree, tree_sha1);\n> -\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n> -\n\nBoth variants of get_tree_entry() receive tree_sha1 and &oc->mode as\nplaces to store the discovered results and that is why we do hashcpy\nand strlcpy in the original codepath.\n\n - With your patch, the new codepath discards tree_sha1[] because it\n   lost the copy back to oc->tree[]; is this change intended?  As we\n   are not passing oc itself to the function, there is no way for it\n   to return the object name directly to oc->tree[], no?\n\n - In the new codepath, oc->path[] is also not copied but I can\n   sort-of guess why (you want to return something other than\n   \"filename\" from get-tree-entry-follow-symlinks in it, or\n   something).  But then the caller is losing the result of parsing\n   the extended SHA-1.\n\nYou explain why \"if (ret && only_to_die)\" part is skipped, but these\ntwo differences are equally, if not more, important differences\nbetween the two codepaths.  I do not think I saw it explained.\n\nIn any case, I would think that get_sha1_with_context() should have\nan external interface that is as close as the original, with\nenhancement (i.e. not with modification of what existing fields\nmean) [*1*].\n\nThat is, if oc->path[] is meant to store filename parsed from the\nend-user input, it should keep doing so with or without\nfollow-symlinks.  And if follow-symlinks feature needs to return\nextra information to the caller, it should add a new field to return\nthat information.\n\nAnd my gut feeling is that such a correction to the way how the\nupdated get_sha1_with_context() behaves would mean you can (and need\nto) keep hashcpy() and strlcpy() common to both codepaths in this\npatch.\n\nThanks.\n\n\n[Footnote]\n\n*1* The reason is simple.  On a user input without any symbolic\nlink, a caller (not just the caller you are adding in patch 3/3)\nshould be able to expect to get the identical outcome from\nget_sha1_with_context(), with or without GET_SHA1_FOLLOW_SYMLINKS.\n"},{"id":"260905","messageId":"1431201766.24139.1.camel@ubuntu","threadId":"39279","inReplyTo":"xmqq7fsiifcz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-09T20:02:46Z","receivedAt":"2015-05-09T20:02:46Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-05-08 at 20:45 -0700, Junio C Hamano wrote:\n> dturner@twopensource.com writes:\n> \n> > From: David Turner <dturner@twitter.com>\n> >\n> > Wire up get_sha1_with_context to call get_tree_entry_follow_symlinks\n> > when GET_SHA1_FOLLOW_SYMLINKS is passed in flags. G_S_FOLLOW_SYMLINKS\n> > is incompatible with G_S_ONLY_TO_DIE because the diagnosis that\n> > ONLY_TO_DIE triggers does not consider symlinks.\n> \n> Is \"does not consider\" something fundamental, or it just happens to\n> be that way right now?\n\nIt just happens to be that way right now.  It would require work\napproximately equivalent to this patch series to fix.\n"},{"id":"260906","messageId":"1431203670.24139.5.camel@ubuntu","threadId":"39279","inReplyTo":"xmqqlhgxhcqb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-09T20:34:30Z","receivedAt":"2015-05-09T20:34:30Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Sat, 2015-05-09 at 10:39 -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > dturner@twopensource.com writes:\n> >\n> >> From: David Turner <dturner@twitter.com>\n> >>\n> >> Wire up get_sha1_with_context to call get_tree_entry_follow_symlinks\n> >> when GET_SHA1_FOLLOW_SYMLINKS is passed in flags. G_S_FOLLOW_SYMLINKS\n> >> is incompatible with G_S_ONLY_TO_DIE because the diagnosis that\n> >> ONLY_TO_DIE triggers does not consider symlinks.\n> >\n> > Is \"does not consider\" something fundamental, or it just happens to\n> > be that way right now?\n> \n> Regardless of the answer to this question, I find the last part of\n> this hunk puzzling.\n\nYou're right -- this code is wrong, for the reasons you explain.  I'm\ngoing to follow your earlier suggestion and use a strbuf for the new\nfield.\n"}]}