{"thread":{"id":"39275","subject":"[PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","startedAt":"2015-05-08T18:13:37Z","lastAt":"2015-05-08T21:31:39Z","messageCount":15,"participants":["dturner@twopensource.com","Junio C Hamano","Eric Sunshine","David Turner","Jeff King","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"260833","messageId":"1431108819-6831-1-git-send-email-dturner@twopensource.com","threadId":"39275","inReplyTo":null,"subject":"[PATCH 1/3] tree-walk: learn get_tree_enty_follow_symlinks","fromName":"","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T18:13:37Z","receivedAt":"2015-05-08T18:13:37Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner \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 \n---\n tree-walk.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n tree-walk.h | 2 +\n 2 files changed, 224 insertions(+)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 5dd9a71..6fb4b7d 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -415,6 +415,228 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \treturn error;\n }\n \n+static int find_tree_entry_nonrecursive(struct tree_desc *t, char *name, unsigned char *result, unsigned *mode) {\n+\tint namelen = strlen(name);\n+\n+\twhile (t->size) {\n+\t\tconst char *entry;\n+\t\tconst unsigned char *sha1;\n+\t\tint entrylen, cmp;\n+\n+\t\tsha1 = tree_entry_extract(t, &entry, mode);\n+\t\tentrylen = tree_entry_len(&t->entry);\n+\t\tupdate_tree_entry(t);\n+\t\tif (entrylen > namelen)\n+\t\t\tcontinue;\n+\t\tcmp = memcmp(name, entry, entrylen);\n+\t\tif (cmp > 0)\n+\t\t\tcontinue;\n+\t\tif (cmp < 0)\n+\t\t\tbreak;\n+\t\tif (entrylen == namelen) {\n+\t\t\thashcpy(result, sha1);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (name[entrylen] != '/')\n+\t\t\tcontinue;\n+\t\tif (!S_ISDIR(*mode))\n+\t\t\tbreak;\n+\t\thashcpy(result, sha1);\n+\t\treturn 0;\n+\t}\n+\treturn -1;\n+}\n+\n+struct dir_state {\n+\tvoid *tree;\n+\tunsigned long size;\n+\tunsigned char sha1[20];\n+};\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 (which is\n+ * assumed to hold at least PATH_MAX bytes), and *mode will be set to\n+ * 0. Otherwise, result will be filled in with the sha1 of the found\n+ * object, and *mode will hold the mode of the object.\n+ */\n+int get_tree_enty_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_cap = 0;\n+\tssize_t parents_len = 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_len + 1, parents_cap);\n+\t\t\tparents[parents_len].tree = tree;\n+\t\t\tparents[parents_len].size = size;\n+\t\t\thashcpy(parents[parents_len].sha1, root);\n+\n+\t\t\tparents_len++;\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_len == 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_len - 1];\n+\t\t\tfree(parent->tree);\n+\t\t\tparents_len--;\n+\t\t\tparent = &parents[parents_len - 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_len - 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_nonrecursive(&t, namebuf.buf,\n+\t\t\t\t\t\t\t current_tree_sha1,\n+\t\t\t\t\t\t\t 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_len - 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_len; ++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 find_tree_entry(struct tree_desc *t, const char *name, unsigned char *result, unsigned *mode)\n {\n \tint namelen = strlen(name);\ndiff --git a/tree-walk.h b/tree-walk.h\nindex ae7fb3a..002e5a9 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_enty_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":"260832","messageId":"1431108819-6831-2-git-send-email-dturner@twopensource.com","threadId":"39275","inReplyTo":"1431108819-6831-1-git-send-email-dturner@twopensource.com","subject":"[PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T18:13:38Z","receivedAt":"2015-05-08T18:13:38Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner \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 \n---\n cache.h | 15 ++++++++-------\n sha1_name.c | 8 ++++++++\n 2 files changed, 16 insertions(+), 7 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..23863f7 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1434,6 +1434,12 @@ 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\tif (flags & GET_SHA1_FOLLOW_SYMLINKS) {\n+\t\t\t\tret = get_tree_enty_follow_symlinks(tree_sha1,\n+\t\t\t\t\tfilename, sha1, oc->path, &oc->mode);\n+\t\t\t\tfree(new_filename);\n+\t\t\t\treturn ret;\n+\t\t\t}\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@@ -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(_(\"internal error: bad 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":"260834","messageId":"1431108819-6831-3-git-send-email-dturner@twopensource.com","threadId":"39275","inReplyTo":"1431108819-6831-1-git-send-email-dturner@twopensource.com","subject":"[PATCH 3/3] cat-file: add --follow-symlinks to --batch","fromName":"","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T18:13:39Z","receivedAt":"2015-05-08T18:13:39Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner \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 \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 | | --textconv ) \n-'git cat-file' (--batch | --batch-check) < \n+'git cat-file' (--batch | --batch-check) [--follow-symlinks] < \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 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 .\n@@ -148,6 +161,19 @@ the repository, then `cat-file` will ignore any custom format and print:\n 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 LF 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 will be ../foo. 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..277af32 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":"260843","messageId":"xmqqd22alvk6.fsf@gitster.dls.corp.google.com","threadId":"39275","inReplyTo":"1431108819-6831-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH 1/3] tree-walk: learn get_tree_enty_follow_symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-08T19:26:49Z","receivedAt":"2015-05-08T19:26:49Z","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 \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 \n> ---\n> tree-walk.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> tree-walk.h | 2 +\n> 2 files changed, 224 insertions(+)\n>\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 5dd9a71..6fb4b7d 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -415,6 +415,228 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> \treturn error;\n> }\n> \n> +static int find_tree_entry_nonrecursive(struct tree_desc *t, char *name, unsigned char *result, unsigned *mode) {\n> +\tint namelen = strlen(name);\n> +\n> +\twhile (t->size) {\n\nWhy do you need an almost duplicate of existing find_tree_entry()\nhere? The argument \"name\" above is not const, so isn't that just\nthe matter of the caller to temporarily replace '/' in name[] before\ncalling find_tree_entry() if all you wanted to avoid was to prevent\nit from falling into get_tree_entry() codepath? Or are there more\nto this function?\n\n> +#define GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS 40\n\nIs 40 just a randomly chosen number?\n\nI do not think 40 is particularly unreasonable, but so is 5 (which I\nthink is also reasonable and is already used as MAXDEPTH in refs.c\nto follow symrefs), and am curious where that number came from.\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 (which is\n> + * assumed to hold at least PATH_MAX bytes), and *mode will be set to\n> + * 0.\n\nAs the API to this new function is not constrained by existing\ncallers, you might want to consider using strbuf for result_path,\nwhich would make it easier for both the callers and this function.\n\n> +int get_tree_enty_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_cap = 0;\n> +\tssize_t parents_len = 0;\n\nIt is customary to name variables to control an ALLOC_GROW()-managed\narray 'foo' as foo_nr and foo_alloc. Deviating from the convention\nmakes the patch harder to read by people who are familiar with it\nwithout any benefit, and those who are familiar with the existing\ncode are the people you want your patch reviewed by.\n\nI am context-switching now; will review the remainder some other\ntime.\n\nThanks.\n"},{"id":"260846","messageId":"CAPig+cQTN5OCxfH_OLjaqza4Os8=kton8baiQMgiNZjR10C=ag@mail.gmail.com","threadId":"39275","inReplyTo":"1431108819-6831-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH 1/3] tree-walk: learn get_tree_enty_follow_symlinks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-08T19:43:11Z","receivedAt":"2015-05-08T19:43:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 8, 2015 at 2:13 PM, wrote:\n> tree-walk: learn get_tree_enty_follow_symlinks\n\ns/enty/entry/ here and in several places in the patch itself.\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 \n> ---\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 5dd9a71..6fb4b7d 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -415,6 +415,228 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> static int find_tree_entry(struct tree_desc *t, const char *name, unsigned char *result, unsigned *mode)\n> {\n> diff --git a/tree-walk.h b/tree-walk.h\n> index ae7fb3a..002e5a9 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_enty_follow_symlinks(unsigned char *tree_sha1, const char *name, unsigned char *result, unsigned char *result_path, unsigned *mode);\n"},{"id":"260847","messageId":"CAPig+cRzXQp_afDrOdKrUQ-=DD1P_UcDzs8aSZnTfesd7R20wQ@mail.gmail.com","threadId":"39275","inReplyTo":"1431108819-6831-2-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-08T19:45:24Z","receivedAt":"2015-05-08T19:45:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 8, 2015 at 2:13 PM, wrote:\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> Signed-off-by: David Turner \n> ---\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 6d10f05..23863f7 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1434,6 +1434,12 @@ static int get_sha1_with_context_1(const char *name,\n> new_filename = resolve_relative_path(filename);\n> if (new_filename)\n> filename = new_filename;\n> + if (flags & GET_SHA1_FOLLOW_SYMLINKS) {\n> + ret = get_tree_enty_follow_symlinks(tree_sha1,\n> + filename, sha1, oc->path, &oc->mode);\n> + free(new_filename);\n> + return ret;\n> + }\n> ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);\n> if (ret && only_to_die) {\n> diagnose_invalid_sha1_path(prefix, filename,\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> + if (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & GET_SHA1_ONLY_TO_DIE)\n> + die(_(\"internal error: bad flags for get_sha1_with_context\"));\n\nThere may not be much value in marking an \"internal error\" string for\ntranslation.\n\n> return get_sha1_with_context_1(str, flags, NULL, sha1, orc);\n> }\n"},{"id":"260848","messageId":"CAPig+cQ2p439dtMwbUEpWdxFB4gf3iuaataab=ghJ6Jm1Snv6w@mail.gmail.com","threadId":"39275","inReplyTo":"1431108819-6831-3-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH 3/3] cat-file: add --follow-symlinks to --batch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-08T19:51:51Z","receivedAt":"2015-05-08T19:51:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 8, 2015 at 2:13 PM, wrote:\n> This wires the in-repo-symlink following code through to the cat-file\n> builtin. In the event of an out-of-repo link, cat-file will print\n> the link in a new format.\n>\n> Signed-off-by: David Turner \n> ---\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index df99df4..277af32 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -369,6 +377,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n> OPT_SET_INT('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n> OPT_SET_INT(0, \"textconv\", &opt,\n> N_(\"for blob objects, run textconv on object's content\"), 'c'),\n> + OPT_SET_INT(0, \"follow-symlinks\", &batch.follow_symlinks,\n> + N_(\"Follow in-repo symlinks; report out-of-repo symlinks (requires --batch or --batch-check)\"),\n> + 1),\n\nCapitalization is not used for descriptions of the other options.\n\n> { OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n> N_(\"show info and content of objects fed from the standard input\"),\n> PARSE_OPT_OPTARG, batch_option_callback },\n"},{"id":"260850","messageId":"1431115351.9179.7.camel@ubuntu","threadId":"39275","inReplyTo":"xmqqd22alvk6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] tree-walk: learn get_tree_enty_follow_symlinks","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T20:02:31Z","receivedAt":"2015-05-08T20:02:31Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-05-08 at 12:26 -0700, Junio C Hamano wrote:\n> > +static int find_tree_entry_nonrecursive(struct tree_desc *t, char *name, unsigned char *result, unsigned *mode) {\n> > +\tint namelen = strlen(name);\n> > +\n> > +\twhile (t->size) {\n> \n> Why do you need an almost duplicate of existing find_tree_entry()\n> here? The argument \"name\" above is not const, so isn't that just\n> the matter of the caller to temporarily replace '/' in name[] before\n> calling find_tree_entry() if all you wanted to avoid was to prevent\n> it from falling into get_tree_entry() codepath? Or are there more\n> to this function?\n\nOh, right, actually, we already replaced the '/'! So I can just use the\nexisting one. That was silly. \n\n> > +#define GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS 40\n> \n> Is 40 just a randomly chosen number?\n> \n> I do not think 40 is particularly unreasonable, but so is 5 (which I\n> think is also reasonable and is already used as MAXDEPTH in refs.c\n> to follow symrefs), and am curious where that number came from.\n\nhttp://lxr.linux.no/linux+v3.6.4/fs/namei.c#L783\n\nI'll add a comment to that effect.\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 (which is\n> > + * assumed to hold at least PATH_MAX bytes), and *mode will be set to\n> > + * 0.\n> \n> As the API to this new function is not constrained by existing\n> callers, you might want to consider using strbuf for result_path,\n> which would make it easier for both the callers and this function.\n\nThe API is sort-of constrained, because the caller has a fixed-size\nbuffer to fill in (see the 2nd patch in this series). \n\n> It is customary to name variables to control an ALLOC_GROW()-managed\n> array 'foo' as foo_nr and foo_alloc. Deviating from the convention\n> makes the patch harder to read by people who are familiar with it\n> without any benefit, and those who are familiar with the existing\n> code are the people you want your patch reviewed by.\n\nWill fix.\n\n> I am context-switching now; will review the remainder some other\n> time.\n\nThanks for the review.\n"},{"id":"260853","messageId":"CAPc5daXmJSfdB_eyQ5ppVp2NhYGZvPChGAyn4p2hEde=4x9EEQ@mail.gmail.com","threadId":"39275","inReplyTo":"CAPig+cRzXQp_afDrOdKrUQ-=DD1P_UcDzs8aSZnTfesd7R20wQ@mail.gmail.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-08T20:17:58Z","receivedAt":"2015-05-08T20:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, May 8, 2015 at 12:45 PM, Eric Sunshine wrote:\n>> + if (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & GET_SHA1_ONLY_TO_DIE)\n>> + die(_(\"internal error: bad flags for get_sha1_with_context\"));\n>\n> There may not be much value in marking an \"internal error\" string for\n> translation.\n\nThe whole point of this kind of messages is so that the end users,\nwhen they see it trigger, can notify us and then to allow us to\nidentify which die() triggered. It is not just \"may not be much\nvalue in\", but is actively unproductive to make it translatable,\neven if we ignore the cost to i18n/l10n teams to translate such\nmessages.\n\nBy the way, we would want to standardise the string before the\ncolon, so that we can tell users \"If you see an error message that\nbegins with 'internal error:', please report that to us---it is a\nprogramming error\". I think the majority of existing code uses the\nstring \"BUG:\" for that, and I do not mind using 'internal error:'\nfor that purpose instead, but the important thing is to use just one\nsingle string thoughout the codebase, so that the \"please report\nwhen you see this\" message we give the users can be simple.\n\nThanks.\n"},{"id":"260855","messageId":"xmqq6182ke9i.fsf@gitster.dls.corp.google.com","threadId":"39275","inReplyTo":"CAPc5daXmJSfdB_eyQ5ppVp2NhYGZvPChGAyn4p2hEde=4x9EEQ@mail.gmail.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-08T20:25:45Z","receivedAt":"2015-05-08T20:25:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Perhaps something like the attached patch is a good idea (I used\n\"BUG:\" for now), I wonder?\n\n Documentation/CodingGuidelines | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex c6e536f..020370e 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -232,7 +232,12 @@ For C programs:\n \t to be translated, that follows immediately after it */\n \t_(\"Here is a translatable string explained by the above.\");\n \n+ - Speaking of translations, do not mark the message to die() that\n+ should never trigger unless there is an programming error. E.g.\n+\n+\tdie(\"BUG: frotz() called with NULL pointer to xyzzy parameter\");\n+\n- - Double negation is often harder to understand than no negation\n+ - Double negation is not easier to understand than no negation\n at all.\n \n - There are two schools of thought when it comes to comparison,\n"},{"id":"260856","messageId":"1431116829.9179.12.camel@ubuntu","threadId":"39275","inReplyTo":"CAPc5daXmJSfdB_eyQ5ppVp2NhYGZvPChGAyn4p2hEde=4x9EEQ@mail.gmail.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-08T20:27:09Z","receivedAt":"2015-05-08T20:27:09Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-05-08 at 13:17 -0700, Junio C Hamano wrote:\n> On Fri, May 8, 2015 at 12:45 PM, Eric Sunshine wrote:\n> >> + if (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & GET_SHA1_ONLY_TO_DIE)\n> >> + die(_(\"internal error: bad flags for get_sha1_with_context\"));\n> >\n> > There may not be much value in marking an \"internal error\" string for\n> > translation.\n> \n> The whole point of this kind of messages is so that the end users,\n> when they see it trigger, can notify us and then to allow us to\n> identify which die() triggered. It is not just \"may not be much\n> value in\", but is actively unproductive to make it translatable,\n> even if we ignore the cost to i18n/l10n teams to translate such\n> messages.\n>\n> By the way, we would want to standardise the string before the\n> colon, so that we can tell users \"If you see an error message that\n> begins with 'internal error:', please report that to us---it is a\n> programming error\". I think the majority of existing code uses the\n> string \"BUG:\" for that, and I do not mind using 'internal error:'\n> for that purpose instead, but the important thing is to use just one\n> single string thoughout the codebase, so that the \"please report\n> when you see this\" message we give the users can be simple.\n\nIt appears that \"BUG:\" is preferred to \"internal error:\" by a margin of\n2:1. So I'll change it. Fixing the remaining ~40 instances of\n\"internal error\" would be a great project for someone new to git\ndevelopment.\n\n>From an i18n perspective, I think the correct thing to do would be to\ni18n the \"BUG\" portion and nothing else. That way, users can understand\nwhat has happened, but if they report it upstream, git developers know\nwhere in the code to look. But this would probably be complicated to\nimplement, so maybe not worthwhile.\n"},{"id":"260859","messageId":"BB246DF6A6DA4735B0B627F5555CB803@PhilipOakley","threadId":"39275","inReplyTo":"CAPc5daXmJSfdB_eyQ5ppVp2NhYGZvPChGAyn4p2hEde=4x9EEQ@mail.gmail.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-05-08T20:38:06Z","receivedAt":"2015-05-08T20:38:06Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> On Fri, May 8, 2015 at 12:45 PM, Eric Sunshine \n> wrote:\n>>> + if (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & \n>>> GET_SHA1_ONLY_TO_DIE)\n>>> + die(_(\"internal error: bad flags for \n>>> get_sha1_with_context\"));\n>>\n>> There may not be much value in marking an \"internal error\" string for\n>> translation.\n>\n> The whole point of this kind of messages is so that the end users,\n> when they see it trigger, can notify us and then to allow us to\n> identify which die() triggered. It is not just \"may not be much\n> value in\", but is actively unproductive to make it translatable,\n> even if we ignore the cost to i18n/l10n teams to translate such\n> messages.\n>\n> By the way, we would want to standardise the string before the\n> colon, so that we can tell users \"If you see an error message that\n> begins with 'internal error:', please report that to us---it is a\n> programming error\". I think the majority of existing code uses the\n> string \"BUG:\" for that, and I do not mind using 'internal error:'\n> for that purpose instead, but the important thing is to use just one\n> single string thoughout the codebase, so that the \"please report\n> when you see this\" message we give the users can be simple.\n>\nI would see \"BUG:\" as being internationally recognised, while \"internal \nerror:\" would need to be translated, and perhaps be written \"internal \nerror, report this to git@vger.kernel.org:\" to ensure that the reader \nknows immediately what to do.\n--\nPhilip \n"},{"id":"260858","messageId":"20150508203952.GA13457@peff.net","threadId":"39275","inReplyTo":"xmqq6182ke9i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-08T20:39:52Z","receivedAt":"2015-05-08T20:39:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 08, 2015 at 01:25:45PM -0700, Junio C Hamano wrote:\n\n> Perhaps something like the attached patch is a good idea (I used\n> \"BUG:\" for now), I wonder?\n> \n> Documentation/CodingGuidelines | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n\nYeah, this looks good (and we definitely prefer BUG:, though I am\nresponsible for over 1/3 of the existing incidences).\n\nI have been tempted to provide a macro or function for it, and have it\nactually call abort() to trigger a coredump. I.e., basically assert(),\nexcept unconditional but you get to write a more useful message.\n\nSo all of the sites would just become:\n\n BUG(\"frotz() called with NULL pointer to xyzzy parameter\");\n\nor similar.\n\n-Peff\n"},{"id":"260870","messageId":"xmqqsib6ix28.fsf@gitster.dls.corp.google.com","threadId":"39275","inReplyTo":"20150508203952.GA13457@peff.net","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-08T21:22:39Z","receivedAt":"2015-05-08T21:22:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 08, 2015 at 01:25:45PM -0700, Junio C Hamano wrote:\n>\n>> Perhaps something like the attached patch is a good idea (I used\n>> \"BUG:\" for now), I wonder?\n>> \n>> Documentation/CodingGuidelines | 7 ++++++-\n>> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> Yeah, this looks good (and we definitely prefer BUG:, though I am\n> responsible for over 1/3 of the existing incidences).\n>\n> I have been tempted to provide a macro or function for it, and have it\n> actually call abort() to trigger a coredump. I.e., basically assert(),\n> except unconditional but you get to write a more useful message.\n>\n> So all of the sites would just become:\n>\n> BUG(\"frotz() called with NULL pointer to xyzzy parameter\");\n>\n> or similar.\n\nYeah, that sounds more sensible (especially the abort() part).\n"},{"id":"260871","messageId":"xmqqoaluiwn8.fsf@gitster.dls.corp.google.com","threadId":"39275","inReplyTo":"1431108819-6831-2-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH 2/3] sha1_name: get_sha1_with_context learns to follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-08T21:31:39Z","receivedAt":"2015-05-08T21:31:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"dturner@twopensource.com writes:\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 6d10f05..23863f7 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1434,6 +1434,12 @@ 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\tif (flags & GET_SHA1_FOLLOW_SYMLINKS) {\n> +\t\t\t\tret = get_tree_enty_follow_symlinks(tree_sha1,\n> +\t\t\t\t\tfilename, sha1, oc->path, &oc->mode);\n> +\t\t\t\tfree(new_filename);\n> +\t\t\t\treturn ret;\n> +\t\t\t}\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\nIn general, I'd prefer to avoid this kind of \"I only need this part\nof the existing codeflow and I'll ignore rest by returning early\".\nYou cannot anticipate any future needs of callers (e.g. only-to-die\nflag might want to be passed when doing follow-symlinks in the\nfuture) and clean-ups that may be added to the later part of the\nfunction that you are skipping with this early return.\n\nIn other words,\n\n\tif (new_filename)\n \tfilename = new_filename;\n-\tret = get_tree_entry(...);\n+\tif (flags & GET_SHA1_FOLLOW_SYMLINKS) {\n+\t\tret = ... whatever new thing you want to do ...\n+\t} else {\n+\t\tret = get_tree_entry(... just reindented ... );\n+\t}\n\tif (ret && ...)\n\nis what we would prefer to see here.\n\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(_(\"internal error: bad flags for get_sha1_with_context\"));\n> \treturn get_sha1_with_context_1(str, flags, NULL, sha1, orc);\n> }\n"}]}