{"thread":{"id":"44334","subject":"[PATCH 0/7] stop blind fallback to \".git\"","startedAt":"2016-10-20T06:15:44Z","lastAt":"2017-02-14T20:36:43Z","messageCount":29,"participants":["Jeff King","Duy Nguyen","Jonathan Nieder","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"304447","messageId":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","threadId":"44334","inReplyTo":null,"subject":"[PATCH 0/7] stop blind fallback to \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:15:36Z","receivedAt":"2016-10-20T06:15:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a follow-on to the series in:\n\n http://public-inbox.org/git/20160913032242.coyuhyhn6uklewuk@sigill.intra.peff.net/\n\nThat series avoided looking at \".git/config\" when we haven't discovered\nwhether we are in a git repo. This takes that further and avoids ever\nlooking at \".git\" as a fallback.\n\nPatches 1-6 just teach various low-level code to detect and handle the\ncase when we are not in a configure repository (along with associated\ncleanups). The final patch actually disallows this case (and the early\nfixes were found by running the test suite with just the final patch).\n\nI think this is a step in the right direction, both in fixing bugs, but\nalso in making our setup and repository-access code easier to reason\nabout. However, it does carry a risk of regression, if there are more\n\"fixes\" that I missed. If we wanted to be really conservative, we could\nhold back the 7th patch as a separate topic and cook it in \"next\" for an\nextra release cycle or something. I'm not all that worried, though.\n\nI built this on top of jc/diff-unique-abbrev-comments, as it refactors\nthat same function (and it didn't seem like a bad topic to be held\nhostage by).\n\n [1/7]: read info/{attributes,exclude} only when in repository\n [2/7]: test-*-cache-tree: setup git dir\n [3/7]: find_unique_abbrev: use 4-buffer ring\n [4/7]: diff_unique_abbrev: rename to diff_aligned_abbrev\n [5/7]: diff_aligned_abbrev: use \"struct oid\"\n [6/7]: diff: handle sha1 abbreviations outside of repository\n [7/7]: setup_git_env: avoid blind fall-back to \".git\"\n\n attr.c | 6 +++++-\n builtin/merge.c | 11 +++++-----\n builtin/receive-pack.c | 16 ++++++---------\n cache.h | 4 ++--\n combine-diff.c | 6 +++---\n diff.c | 43 +++++++++++++++++++++++++---------------\n diff.h | 6 +++++-\n dir.c | 12 +++++------\n environment.c | 5 ++++-\n sha1_name.c | 4 +++-\n t/helper/test-dump-cache-tree.c | 1 +\n t/helper/test-scrap-cache-tree.c | 1 +\n 12 files changed, 68 insertions(+), 47 deletions(-)\n"},{"id":"304448","messageId":"20161020061641.3enppkoxfzr76has@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 1/7] read info/{attributes,exclude} only when in repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:16:41Z","receivedAt":"2016-10-20T06:16:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The low-level attribute and gitignore code will try to look\nin $GIT_DIR/info for any repo-level configuration files,\neven if we have not actually determined that we are in a\nrepository (e.g., running \"git grep --no-index\"). In such a\ncase they end up looking for \".git/info/attributes\", etc.\n\nThis is generally harmless, as such a file is unlikely to\nexist outside of a repository, but it's still conceptually\nthe wrong thing to do.\n\nLet's detect this situation explicitly and skip reading the\nfile (i.e., the same behavior we'd get if we were in a\nrepository and the file did not exist).\n\nSigned-off-by: Jeff King \n---\n attr.c | 6 +++++-\n dir.c | 12 ++++++------\n 2 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex eec5d7d..1fcf042 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -531,7 +531,11 @@ static void bootstrap_attr_stack(void)\n \t\tdebug_push(elem);\n \t}\n \n-\telem = read_attr_from_file(git_path_info_attributes(), 1);\n+\tif (startup_info->have_repository)\n+\t\telem = read_attr_from_file(git_path_info_attributes(), 1);\n+\telse\n+\t\telem = NULL;\n+\n \tif (!elem)\n \t\telem = xcalloc(1, sizeof(*elem));\n \telem->origin = NULL;\ndiff --git a/dir.c b/dir.c\nindex 3bad1ad..6cde773 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2195,8 +2195,6 @@ static GIT_PATH_FUNC(git_path_info_exclude, \"info/exclude\")\n \n void setup_standard_excludes(struct dir_struct *dir)\n {\n-\tconst char *path;\n-\n \tdir->exclude_per_dir = \".gitignore\";\n \n \t/* core.excludefile defaulting to $XDG_HOME/git/ignore */\n@@ -2207,10 +2205,12 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\t\t\t\t dir->untracked ? &dir->ss_excludes_file : NULL);\n \n \t/* per repository user preference */\n-\tpath = git_path_info_exclude();\n-\tif (!access_or_warn(path, R_OK, 0))\n-\t\tadd_excludes_from_file_1(dir, path,\n-\t\t\t\t\t dir->untracked ? &dir->ss_info_exclude : NULL);\n+\tif (startup_info->have_repository) {\n+\t\tconst char *path = git_path_info_exclude();\n+\t\tif (!access_or_warn(path, R_OK, 0))\n+\t\t\tadd_excludes_from_file_1(dir, path,\n+\t\t\t\t\t\t dir->untracked ? &dir->ss_info_exclude : NULL);\n+\t}\n }\n \n int remove_path(const char *name)\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304449","messageId":"20161020061659.6yzrphnhd4jkbyhg@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 2/7] test-*-cache-tree: setup git dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:16:59Z","receivedAt":"2016-10-20T06:17:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These test helper programs access the index, but do not ever\nsetup_git_directory(), meaning we just blindly looked in\n\".git/index\". This happened to work for the purposes of our\ntests (which do not run from subdirectories, nor in\nnon-repos), but it's a bad habit.\n\nSigned-off-by: Jeff King \n---\n t/helper/test-dump-cache-tree.c | 1 +\n t/helper/test-scrap-cache-tree.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c\nindex 44f3290..7af116d 100644\n--- a/t/helper/test-dump-cache-tree.c\n+++ b/t/helper/test-dump-cache-tree.c\n@@ -58,6 +58,7 @@ int cmd_main(int ac, const char **av)\n {\n \tstruct index_state istate;\n \tstruct cache_tree *another = cache_tree();\n+\tsetup_git_directory();\n \tif (read_cache() < 0)\n \t\tdie(\"unable to read index file\");\n \tistate = the_index;\ndiff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c\nindex 5b2fd09..27fe040 100644\n--- a/t/helper/test-scrap-cache-tree.c\n+++ b/t/helper/test-scrap-cache-tree.c\n@@ -7,6 +7,7 @@ static struct lock_file index_lock;\n \n int cmd_main(int ac, const char **av)\n {\n+\tsetup_git_directory();\n \thold_locked_index(&index_lock, 1);\n \tif (read_cache() < 0)\n \t\tdie(\"unable to read index file\");\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304450","messageId":"20161020061919.qu443ud7lbkopvth@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 3/7] find_unique_abbrev: use 4-buffer ring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:19:19Z","receivedAt":"2016-10-20T06:19:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some code paths want to format multiple abbreviated sha1s in\nthe same output line. Because we use a single static buffer\nfor our return value, they have to either break their output\ninto several calls or allocate their own arrays and use\nfind_unique_abbrev_r().\n\nIntead, let's mimic sha1_to_hex() and use a ring of several\nbuffers, so that the return value stays valid through\nmultiple calls. This shortens some of the callers, and makes\nit harder to for them to make a silly mistake.\n\nSigned-off-by: Jeff King \n---\nIt feels a little funny in these callers to be moving from a reentrant\nfunction to one that relies on a static buffer. But I feel like the\nresult is more obvious and less error-prone, and the \"ring of buffers\"\nconcept has proven useful and safe in sha1_to_hex().\n\nMy ulterior motive is that while refactoring one of the later patches, I\njust assumed that we did have a ring of buffers, and introduced a subtle\nbug.\n\n builtin/merge.c | 11 +++++------\n builtin/receive-pack.c | 16 ++++++----------\n cache.h | 4 ++--\n sha1_name.c | 4 +++-\n 4 files changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8b57c7..b65eeaa 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1374,12 +1374,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tstruct commit *commit;\n \n \t\tif (verbosity >= 0) {\n-\t\t\tchar from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];\n-\t\t\tfind_unique_abbrev_r(from, head_commit->object.oid.hash,\n-\t\t\t\t\t DEFAULT_ABBREV);\n-\t\t\tfind_unique_abbrev_r(to, remoteheads->item->object.oid.hash,\n-\t\t\t\t\t DEFAULT_ABBREV);\n-\t\t\tprintf(_(\"Updating %s..%s\\n\"), from, to);\n+\t\t\tprintf(_(\"Updating %s..%s\\n\"),\n+\t\t\t find_unique_abbrev(head_commit->object.oid.hash,\n+\t\t\t\t\t\t DEFAULT_ABBREV),\n+\t\t\t find_unique_abbrev(remoteheads->item->object.oid.hash,\n+\t\t\t\t\t\t DEFAULT_ABBREV));\n \t\t}\n \t\tstrbuf_addstr(&msg, \"Fast-forward\");\n \t\tif (have_message)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 04ed38e..680759d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1163,10 +1163,6 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tstruct string_list_item *item;\n \tstruct command *dst_cmd;\n \tunsigned char sha1[GIT_SHA1_RAWSZ];\n-\tchar cmd_oldh[GIT_SHA1_HEXSZ + 1],\n-\t cmd_newh[GIT_SHA1_HEXSZ + 1],\n-\t dst_oldh[GIT_SHA1_HEXSZ + 1],\n-\t dst_newh[GIT_SHA1_HEXSZ + 1];\n \tint flag;\n \n \tstrbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n@@ -1197,14 +1193,14 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \n \tdst_cmd->skip_update = 1;\n \n-\tfind_unique_abbrev_r(cmd_oldh, cmd->old_sha1, DEFAULT_ABBREV);\n-\tfind_unique_abbrev_r(cmd_newh, cmd->new_sha1, DEFAULT_ABBREV);\n-\tfind_unique_abbrev_r(dst_oldh, dst_cmd->old_sha1, DEFAULT_ABBREV);\n-\tfind_unique_abbrev_r(dst_newh, dst_cmd->new_sha1, DEFAULT_ABBREV);\n \trp_error(\"refusing inconsistent update between symref '%s' (%s..%s) and\"\n \t\t \" its target '%s' (%s..%s)\",\n-\t\t cmd->ref_name, cmd_oldh, cmd_newh,\n-\t\t dst_cmd->ref_name, dst_oldh, dst_newh);\n+\t\t cmd->ref_name,\n+\t\t find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV),\n+\t\t find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV),\n+\t\t dst_cmd->ref_name,\n+\t\t find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV),\n+\t\t find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));\n \n \tcmd->error_string = dst_cmd->error_string =\n \t\t\"inconsistent aliased update\";\ndiff --git a/cache.h b/cache.h\nindex 05ecb88..6e06311 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -901,8 +901,8 @@ extern char *sha1_pack_index_name(const unsigned char *sha1);\n * The result will be at least `len` characters long, and will be NUL\n * terminated.\n *\n- * The non-`_r` version returns a static buffer which will be overwritten by\n- * subsequent calls.\n+ * The non-`_r` version returns a static buffer which remains valid until 4\n+ * more calls to find_unique_abbrev are made.\n *\n * The `_r` variant writes to a buffer supplied by the caller, which must be at\n * least `GIT_SHA1_HEXSZ + 1` bytes. The return value is the number of bytes\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 4092836..36ce9b9 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -472,7 +472,9 @@ int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)\n \n const char *find_unique_abbrev(const unsigned char *sha1, int len)\n {\n-\tstatic char hex[GIT_SHA1_HEXSZ + 1];\n+\tstatic int bufno;\n+\tstatic char hexbuffer[4][GIT_SHA1_HEXSZ + 1];\n+\tchar *hex = hexbuffer[3 & ++bufno];\n \tfind_unique_abbrev_r(hex, sha1, len);\n \treturn hex;\n }\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304451","messageId":"20161020061943.tcndl4cngwycw7zi@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 4/7] diff_unique_abbrev: rename to diff_aligned_abbrev","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:19:43Z","receivedAt":"2016-10-20T06:19:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The word \"align\" describes how the function actually differs\nfrom find_unique_abbrev, and will make it less confusing\nwhen we add more diff-specific abbrevation functions that do\nnot do this alignment.\n\nSince this is a globally available function, let's also move\nits descriptive comment to the header file, where we\ntypically document function interfaces.\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 6 +++---\n diff.c | 10 +++-------\n diff.h | 6 +++++-\n 3 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 8e2a577..b36c2d1 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1203,9 +1203,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \n \t\t/* Show sha1's */\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tprintf(\" %s\", diff_unique_abbrev(p->parent[i].oid.hash,\n-\t\t\t\t\t\t\t opt->abbrev));\n-\t\tprintf(\" %s \", diff_unique_abbrev(p->oid.hash, opt->abbrev));\n+\t\t\tprintf(\" %s\", diff_aligned_abbrev(p->parent[i].oid.hash,\n+\t\t\t\t\t\t\t opt->abbrev));\n+\t\tprintf(\" %s \", diff_aligned_abbrev(p->oid.hash, opt->abbrev));\n \t}\n \n \tif (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {\ndiff --git a/diff.c b/diff.c\nindex f5d6d7e..2d8b74b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4136,11 +4136,7 @@ void diff_free_filepair(struct diff_filepair *p)\n \tfree(p);\n }\n \n-/*\n- * This is different from find_unique_abbrev() in that\n- * it stuffs the result with dots for alignment.\n- */\n-const char *diff_unique_abbrev(const unsigned char *sha1, int len)\n+const char *diff_aligned_abbrev(const unsigned char *sha1, int len)\n {\n \tint abblen;\n \tconst char *abbrev;\n@@ -4188,9 +4184,9 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)\n \tfprintf(opt->file, \"%s\", diff_line_prefix(opt));\n \tif (!(opt->output_format & DIFF_FORMAT_NAME_STATUS)) {\n \t\tfprintf(opt->file, \":%06o %06o %s \", p->one->mode, p->two->mode,\n-\t\t\tdiff_unique_abbrev(p->one->oid.hash, opt->abbrev));\n+\t\t\tdiff_aligned_abbrev(p->one->oid.hash, opt->abbrev));\n \t\tfprintf(opt->file, \"%s \",\n-\t\t\tdiff_unique_abbrev(p->two->oid.hash, opt->abbrev));\n+\t\t\tdiff_aligned_abbrev(p->two->oid.hash, opt->abbrev));\n \t}\n \tif (p->score) {\n \t\tfprintf(opt->file, \"%c%03d%c\", p->status, similarity_index(p),\ndiff --git a/diff.h b/diff.h\nindex 25ae60d..f2b04b6 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -340,7 +340,11 @@ extern void diff_warn_rename_limit(const char *varname, int needed, int degraded\n #define DIFF_STATUS_FILTER_AON\t\t'*'\n #define DIFF_STATUS_FILTER_BROKEN\t'B'\n \n-extern const char *diff_unique_abbrev(const unsigned char *, int);\n+/*\n+ * This is different from find_unique_abbrev() in that\n+ * it stuffs the result with dots for alignment.\n+ */\n+extern const char *diff_aligned_abbrev(const unsigned char *sha1, int);\n \n /* do not report anything on removed paths */\n #define DIFF_SILENT_ON_REMOVED 01\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304452","messageId":"20161020062007.2iiveklem2fikbvu@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 5/7] diff_aligned_abbrev: use \"struct oid\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:20:07Z","receivedAt":"2016-10-20T06:20:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since we're modifying this function anyway, it's a good time\nto update it to the more modern \"struct oid\". We can also\ndrop some of the magic numbers in favor of GIT_SHA1_HEXSZ,\nalong with some descriptive comments.\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 4 ++--\n diff.c | 20 +++++++++++---------\n diff.h | 2 +-\n 3 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex b36c2d1..59501db 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1203,9 +1203,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \n \t\t/* Show sha1's */\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tprintf(\" %s\", diff_aligned_abbrev(p->parent[i].oid.hash,\n+\t\t\tprintf(\" %s\", diff_aligned_abbrev(&p->parent[i].oid,\n \t\t\t\t\t\t\t opt->abbrev));\n-\t\tprintf(\" %s \", diff_aligned_abbrev(p->oid.hash, opt->abbrev));\n+\t\tprintf(\" %s \", diff_aligned_abbrev(&p->oid, opt->abbrev));\n \t}\n \n \tif (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {\ndiff --git a/diff.c b/diff.c\nindex 2d8b74b..8f0f309 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4136,14 +4136,15 @@ void diff_free_filepair(struct diff_filepair *p)\n \tfree(p);\n }\n \n-const char *diff_aligned_abbrev(const unsigned char *sha1, int len)\n+const char *diff_aligned_abbrev(const struct object_id *oid, int len)\n {\n \tint abblen;\n \tconst char *abbrev;\n-\tif (len == 40)\n-\t\treturn sha1_to_hex(sha1);\n \n-\tabbrev = find_unique_abbrev(sha1, len);\n+\tif (len == GIT_SHA1_HEXSZ)\n+\t\treturn oid_to_hex(oid);\n+\n+\tabbrev = find_unique_abbrev(oid->hash, len);\n \tabblen = strlen(abbrev);\n \n \t/*\n@@ -4165,15 +4166,16 @@ const char *diff_aligned_abbrev(const unsigned char *sha1, int len)\n \t * the automatic sizing is supposed to give abblen that ensures\n \t * uniqueness across all objects (statistically speaking).\n \t */\n-\tif (abblen < 37) {\n-\t\tstatic char hex[41];\n+\tif (abblen < GIT_SHA1_HEXSZ - 3) {\n+\t\tstatic char hex[GIT_SHA1_HEXSZ + 1];\n \t\tif (len < abblen && abblen <= len + 2)\n \t\t\txsnprintf(hex, sizeof(hex), \"%s%.*s\", abbrev, len+3-abblen, \"..\");\n \t\telse\n \t\t\txsnprintf(hex, sizeof(hex), \"%s...\", abbrev);\n \t\treturn hex;\n \t}\n-\treturn sha1_to_hex(sha1);\n+\n+\treturn oid_to_hex(oid);\n }\n \n static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)\n@@ -4184,9 +4186,9 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)\n \tfprintf(opt->file, \"%s\", diff_line_prefix(opt));\n \tif (!(opt->output_format & DIFF_FORMAT_NAME_STATUS)) {\n \t\tfprintf(opt->file, \":%06o %06o %s \", p->one->mode, p->two->mode,\n-\t\t\tdiff_aligned_abbrev(p->one->oid.hash, opt->abbrev));\n+\t\t\tdiff_aligned_abbrev(&p->one->oid, opt->abbrev));\n \t\tfprintf(opt->file, \"%s \",\n-\t\t\tdiff_aligned_abbrev(p->two->oid.hash, opt->abbrev));\n+\t\t\tdiff_aligned_abbrev(&p->two->oid, opt->abbrev));\n \t}\n \tif (p->score) {\n \t\tfprintf(opt->file, \"%c%03d%c\", p->status, similarity_index(p),\ndiff --git a/diff.h b/diff.h\nindex f2b04b6..01afc70 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -344,7 +344,7 @@ extern void diff_warn_rename_limit(const char *varname, int needed, int degraded\n * This is different from find_unique_abbrev() in that\n * it stuffs the result with dots for alignment.\n */\n-extern const char *diff_aligned_abbrev(const unsigned char *sha1, int);\n+extern const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n \n /* do not report anything on removed paths */\n #define DIFF_SILENT_ON_REMOVED 01\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304453","messageId":"20161020062125.3iqej3bpdoitr3fz@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 6/7] diff: handle sha1 abbreviations outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:21:25Z","receivedAt":"2016-10-20T06:21:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When generating diffs outside a repository (e.g., with \"diff\n--no-index\"), we may write abbreviated sha1s as part of\n\"--raw\" output or the \"index\" lines of \"--patch\" output.\nSince we have no object database, we never find any\ncollisions, and these sha1s get whatever static abbreviation\nlength is configured (typically 7).\n\nHowever, we do blindly look in \".git/objects\" to see if any\nobjects exist, even though we know we are not in a\nrepository. This is usually harmless because such a\ndirectory is unlikely to exist, but could be wrong in rare\ncircumstances.\n\nLet's instead notice when we are not in a repository and\nbehave as if the object database is empty (i.e., just use\nthe default abbrev length). It would perhaps make sense to\nbe conservative and show full sha1s in that case, but\nshowing the default abbreviation is what we've always done\n(and is certainly less ugly).\n\nNote that this does mean that:\n\n cd /not/a/repo\n GIT_OBJECT_DIRECTORY=/some/real/objdir git diff --no-index ...\n\nused to look for collisions in /some/real/objdir but now\ndoes not. This could be considered either a bugfix (we do\nnot look at objects if we have no repository) or a\nregression, but it seems unlikely that anybody would care\nmuch either way.\n\nSigned-off-by: Jeff King \n---\n diff.c | 21 +++++++++++++++++----\n 1 file changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8f0f309..ef11001 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3049,6 +3049,19 @@ static int similarity_index(struct diff_filepair *p)\n \treturn p->score * 100 / MAX_SCORE;\n }\n \n+static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n+{\n+\tif (startup_info->have_repository)\n+\t\treturn find_unique_abbrev(oid->hash, abbrev);\n+\telse {\n+\t\tchar *hex = oid_to_hex(oid);\n+\t\tif (abbrev < 0 || abbrev > GIT_SHA1_HEXSZ)\n+\t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n+\t\thex[abbrev] = '\\0';\n+\t\treturn hex;\n+\t}\n+}\n+\n static void fill_metainfo(struct strbuf *msg,\n \t\t\t const char *name,\n \t\t\t const char *other,\n@@ -3107,9 +3120,9 @@ static void fill_metainfo(struct strbuf *msg,\n \t\t\t (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))\n \t\t\t\tabbrev = 40;\n \t\t}\n-\t\tstrbuf_addf(msg, \"%s%sindex %s..\", line_prefix, set,\n-\t\t\t find_unique_abbrev(one->oid.hash, abbrev));\n-\t\tstrbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n+\t\tstrbuf_addf(msg, \"%s%sindex %s..%s\", line_prefix, set,\n+\t\t\t diff_abbrev_oid(&one->oid, abbrev),\n+\t\t\t diff_abbrev_oid(&two->oid, abbrev));\n \t\tif (one->mode == two->mode)\n \t\t\tstrbuf_addf(msg, \" %06o\", one->mode);\n \t\tstrbuf_addf(msg, \"%s\\n\", reset);\n@@ -4144,7 +4157,7 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len)\n \tif (len == GIT_SHA1_HEXSZ)\n \t\treturn oid_to_hex(oid);\n \n-\tabbrev = find_unique_abbrev(oid->hash, len);\n+\tabbrev = diff_abbrev_oid(oid, len);\n \tabblen = strlen(abbrev);\n \n \t/*\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304455","messageId":"20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020061536.6fqh23xb2nhxodpa@sigill.intra.peff.net","subject":"[PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:24:31Z","receivedAt":"2016-10-20T06:24:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we default to \".git\" without having done any kind of\nrepository setup, the results quite often do what the user\nexpects. But this has also historically been the cause of\nsome poorly behaved corner cases. These cases can be hard to\nfind, because they happen at the conjunction of two\nrelatively rare circumstances:\n\n 1. We are running some code which assumes there's a\n repository present, but there isn't necessarily one\n (e.g., low-level diff code triggered by \"git diff\n --no-index\" might try to look at some repository data).\n\n 2. We have an unusual setup, like being in a subdirectory\n of the working tree, or we have a .git file (rather\n than a directory), or we are running a tool like \"init\"\n or \"clone\" which may operate on a repository in a\n different directory.\n\nOur test scripts often cover (1), but miss doing (2) at the\nsame time, and so the fallback appears to work but has\nlurking bugs. We can flush these bugs out by refusing to do\nthe fallback entirely., This makes potential problems a lot\nmore obvious by complaining even for \"usual\" setups.\n\nThis passes the test suite (after the adjustments in the\nprevious patches), but there's a risk of regression for any\ncases where the fallback usually works fine but the code\nisn't exercised by the test suite. So by itself, this\ncommit is a potential step backward, but lets us take two\nsteps forward once we've identified and fixed any such\ninstances.\n\nSigned-off-by: Jeff King \n---\n environment.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/environment.c b/environment.c\nindex cd5aa57..b1743e6 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -164,8 +164,11 @@ static void setup_git_env(void)\n \tconst char *replace_ref_base;\n \n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n-\tif (!git_dir)\n+\tif (!git_dir) {\n+\t\tif (!startup_info->have_repository)\n+\t\t\tdie(\"BUG: setup_git_env called without repository\");\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\t}\n \tgitfile = read_gitfile(git_dir);\n \tgit_dir = xstrdup(gitfile ? gitfile : git_dir);\n \tif (get_common_dir(&sb, git_dir))\n-- \n2.10.1.619.g16351a7\n"},{"id":"304456","messageId":"20161020063120.auxwjybljo4hz3p5@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161020062125.3iqej3bpdoitr3fz@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] diff: handle sha1 abbreviations outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T06:31:20Z","receivedAt":"2016-10-20T06:31:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2016 at 02:21:25AM -0400, Jeff King wrote:\n\n> diff --git a/diff.c b/diff.c\n> index 8f0f309..ef11001 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3049,6 +3049,19 @@ static int similarity_index(struct diff_filepair *p)\n> \treturn p->score * 100 / MAX_SCORE;\n> }\n> \n> +static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n> +{\n> +\tif (startup_info->have_repository)\n> +\t\treturn find_unique_abbrev(oid->hash, abbrev);\n> +\telse {\n> +\t\tchar *hex = oid_to_hex(oid);\n> +\t\tif (abbrev < 0 || abbrev > GIT_SHA1_HEXSZ)\n> +\t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n> +\t\thex[abbrev] = '\\0';\n> +\t\treturn hex;\n> +\t}\n> +}\n\nNote that this function has a semantic (but not textual) conflict with\nlt/auto-abbrev in 'next', as it sets DEFAULT_ABBREV to -1.\n\nThe resolution is:\n\ndiff --git a/diff.c b/diff.c\nindex cab811e..4c09314 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3102,7 +3102,9 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n \t\treturn find_unique_abbrev(oid->hash, abbrev);\n \telse {\n \t\tchar *hex = oid_to_hex(oid);\n-\t\tif (abbrev < 0 || abbrev > GIT_SHA1_HEXSZ)\n+\t\tif (abbrev < 0)\n+\t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n+\t\tif (abbrev > GIT_SHA1_HEXSZ)\n \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n \t\thex[abbrev] = '\\0';\n \t\treturn hex;\n\nThis logic could be pushed down into the find_unique_abbrev() code\n(where it _would_ just cause a textual conflict). I preferred to keep it\nup here because other callers could conceivably want to handle the\nnon-repo case in some different way (e.g., by not abbreviating at all).\n\n-Peff\n"},{"id":"304838","messageId":"CACsJy8Bk32TcivD-5UO28UhdbpvCcxTE71cxFO2p_A4TZ1+GVw@mail.gmail.com","threadId":"44334","inReplyTo":"20161020061641.3enppkoxfzr76has@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T12:24:50Z","receivedAt":"2016-10-25T12:26:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 20, 2016 at 1:16 PM, Jeff King wrote:\n> The low-level attribute and gitignore code will try to look\n> in $GIT_DIR/info for any repo-level configuration files,\n> even if we have not actually determined that we are in a\n> repository (e.g., running \"git grep --no-index\"). In such a\n> case they end up looking for \".git/info/attributes\", etc.\n>\n> This is generally harmless, as such a file is unlikely to\n> exist outside of a repository, but it's still conceptually\n> the wrong thing to do.\n>\n> Let's detect this situation explicitly and skip reading the\n> file (i.e., the same behavior we'd get if we were in a\n> repository and the file did not exist).\n\nOn the other hand, if we invoke attr machinery too early by mistake,\nbefore setup_git_directory* is called, then we skip\n.git/info/attributes file as well even though I think we should shout\n\"call setup_git_directory first!\" so the developer can fix it.\n\nI wonder if we should have two flags in startup_info to say \"yes\nsetup_git_dir... has been called, you can trust\nstartup_info->have_repository\" and \"yes, i do not call setup_git_dir\non purpose, quit complaining\" then we could still catch unintended\n.git/info/attributes ignore while letting git grep --no-index work\ncorrectly.\n-- \nDuy\n"},{"id":"304839","messageId":"CACsJy8DZWN0RRaDy9w2BVG5pn4FWCY=1YQDsP0V5obrr1wSzZQ@mail.gmail.com","threadId":"44334","inReplyTo":"20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T12:38:30Z","receivedAt":"2016-10-25T12:39:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 20, 2016 at 1:24 PM, Jeff King wrote:\n> This passes the test suite (after the adjustments in the\n> previous patches), but there's a risk of regression for any\n> cases where the fallback usually works fine but the code\n> isn't exercised by the test suite. So by itself, this\n> commit is a potential step backward, but lets us take two\n> steps forward once we've identified and fixed any such\n> instances.\n>\n> Signed-off-by: Jeff King \n> ---\n> environment.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/environment.c b/environment.c\n> index cd5aa57..b1743e6 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -164,8 +164,11 @@ static void setup_git_env(void)\n> const char *replace_ref_base;\n>\n> git_dir = getenv(GIT_DIR_ENVIRONMENT);\n> - if (!git_dir)\n> + if (!git_dir) {\n> + if (!startup_info->have_repository)\n> + die(\"BUG: setup_git_env called without repository\");\n\nYES!!! Thank you for finally fixing this.\n\nThe \"once we've identified\" part could be tricky though. This message\nalone will not give us any clue where it's called since it's buried\ndeep in git_path() usually, which is buried deep elsewhere. Without\nfalling back to core dumps (with debug info), glibc's backtrace\n(platform specifc), the best we could do is turn git_path() into a\nmacro that takes __FILE__ and __LINE__ and somehow pass the info down\nhere, but \"...\" in macros is C99 specific, sigh..\n\nIs it too bad to turn git_path() into a macro when we know the\ncompiler is C99 ? Older compilers will have no source location info in\ngit_path(), Hopefully they are rare, which means chances of this fault\npopping up are also reduced.\n-- \nDuy\n"},{"id":"304843","messageId":"20161025145654.6cd3lb2gsgjtc5ou@sigill.intra.peff.net","threadId":"44334","inReplyTo":"CACsJy8Bk32TcivD-5UO28UhdbpvCcxTE71cxFO2p_A4TZ1+GVw@mail.gmail.com","subject":"Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-25T14:56:54Z","receivedAt":"2016-10-25T14:57:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2016 at 07:24:50PM +0700, Duy Nguyen wrote:\n\n> > Let's detect this situation explicitly and skip reading the\n> > file (i.e., the same behavior we'd get if we were in a\n> > repository and the file did not exist).\n> \n> On the other hand, if we invoke attr machinery too early by mistake,\n> before setup_git_directory* is called, then we skip\n> .git/info/attributes file as well even though I think we should shout\n> \"call setup_git_directory first!\" so the developer can fix it.\n\n> I wonder if we should have two flags in startup_info to say \"yes\n> setup_git_dir... has been called, you can trust\n> startup_info->have_repository\" and \"yes, i do not call setup_git_dir\n> on purpose, quit complaining\" then we could still catch unintended\n> .git/info/attributes ignore while letting git grep --no-index work\n> correctly.\n\nYeah, it would be nice for the low-level code to be able to detect such\nerrors. I don't mind if you want to extend startup_info in that way, but\nit will probably introduce a period of instability and regressions\n(sites that are perfectly fine, but forgot to set the \"I know what I'm\ndoing\" flag).\n\n-Peff\n"},{"id":"304845","messageId":"20161025151524.y7wwtetohhqgcvob@sigill.intra.peff.net","threadId":"44334","inReplyTo":"CACsJy8DZWN0RRaDy9w2BVG5pn4FWCY=1YQDsP0V5obrr1wSzZQ@mail.gmail.com","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-25T15:15:25Z","receivedAt":"2016-10-25T15:15:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2016 at 07:38:30PM +0700, Duy Nguyen wrote:\n\n> > diff --git a/environment.c b/environment.c\n> > index cd5aa57..b1743e6 100644\n> > --- a/environment.c\n> > +++ b/environment.c\n> > @@ -164,8 +164,11 @@ static void setup_git_env(void)\n> > const char *replace_ref_base;\n> >\n> > git_dir = getenv(GIT_DIR_ENVIRONMENT);\n> > - if (!git_dir)\n> > + if (!git_dir) {\n> > + if (!startup_info->have_repository)\n> > + die(\"BUG: setup_git_env called without repository\");\n> \n> YES!!! Thank you for finally fixing this.\n\nGood, I'm glad somebody besides me is excited about this. I've been\nwanting to write this patch for a long time, but it took years of\nchipping away at all the edge cases.\n\n> The \"once we've identified\" part could be tricky though. This message\n> alone will not give us any clue where it's called since it's buried\n> deep in git_path() usually, which is buried deep elsewhere. Without\n> falling back to core dumps (with debug info), glibc's backtrace\n> (platform specifc), the best we could do is turn git_path() into a\n> macro that takes __FILE__ and __LINE__ and somehow pass the info down\n> here, but \"...\" in macros is C99 specific, sigh..\n> \n> Is it too bad to turn git_path() into a macro when we know the\n> compiler is C99 ? Older compilers will have no source location info in\n> git_path(), Hopefully they are rare, which means chances of this fault\n> popping up are also reduced.\n\nI think you could conditionally make git_path() and all of its\ncounterparts macros, similar to the way the trace code works. It seems\nlike a pretty maintenance-heavy solution, though. I'd prefer\nconditionally compiling backtrace(); that also doesn't hit 100% of\ncases, but at least it isn't too invasive.\n\nBut I think I still prefer just letting the corefile and the debugger do\ntheir job. This error shouldn't happen much, and when it does, it should\nbe easily reproducible. Getting the bug reporter to give either a\nreproduction recipe, or to run \"gdb git\" doesn't seem like that big a\nhurdle.\n\nFor fun, here's a patch that uses backtrace(), but it does not actually\nprint the function names unless you compile with \"-rdynamic\" (and even\nthen it misses static functions). There are better libraries, but of\ncourse that's one more thing for the user to deal with when building.\n\n-Peff\n\n---\ndiff --git a/usage.c b/usage.c\nindex 17f52c1b5c..4917c6bdfd 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -5,6 +5,9 @@\n */\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n+#ifdef HAVE_BACKTRACE\n+#include \n+#endif\n \n static FILE *error_handle;\n static int tweaked_error_buffering;\n@@ -24,6 +27,32 @@ void vreportf(const char *prefix, const char *err, va_list params)\n \tfputc('\\n', fh);\n }\n \n+#ifdef HAVE_BACKTRACE\n+static void maybe_backtrace(void)\n+{\n+\tvoid *bt[100];\n+\tchar **symbols;\n+\tint nr;\n+\n+\tif (!git_env_bool(\"GIT_BACKTRACE_ON_DIE\", 0))\n+\t\treturn;\n+\n+\tnr = backtrace(bt, ARRAY_SIZE(bt));\n+\tsymbols = backtrace_symbols(bt, nr);\n+\tif (symbols) {\n+\t\tFILE *fh = error_handle ? error_handle : stderr;\n+\t\tint i;\n+\n+\t\tfprintf(fh, \"die() called from:\\n\");\n+\t\tfor (i = 0; i < nr; i++)\n+\t\t\tfprintf(fh, \" %s\\n\", symbols[i]);\n+\t\tfree(symbols);\n+\t}\n+}\n+#else\n+#define maybe_backtrace()\n+#endif\n+\n static NORETURN void usage_builtin(const char *err, va_list params)\n {\n \tvreportf(\"usage: \", err, params);\n@@ -33,6 +62,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n+\tmaybe_backtrace();\n \texit(128);\n }\n \n"},{"id":"304886","messageId":"20161026102921.GA31311@ash","threadId":"44334","inReplyTo":"20161025151524.y7wwtetohhqgcvob@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-26T10:29:21Z","receivedAt":"2016-10-26T10:29:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 25, 2016 at 11:15:25AM -0400, Jeff King wrote:\n> > The \"once we've identified\" part could be tricky though. This message\n> > alone will not give us any clue where it's called since it's buried\n> > deep in git_path() usually, which is buried deep elsewhere. Without\n> > falling back to core dumps (with debug info), glibc's backtrace\n> > (platform specifc), the best we could do is turn git_path() into a\n> > macro that takes __FILE__ and __LINE__ and somehow pass the info down\n> > here, but \"...\" in macros is C99 specific, sigh..\n> > \n> > Is it too bad to turn git_path() into a macro when we know the\n> > compiler is C99 ? Older compilers will have no source location info in\n> > git_path(), Hopefully they are rare, which means chances of this fault\n> > popping up are also reduced.\n> \n> I think you could conditionally make git_path() and all of its\n> counterparts macros, similar to the way the trace code works. It seems\n> like a pretty maintenance-heavy solution, though. I'd prefer\n> conditionally compiling backtrace(); that also doesn't hit 100% of\n> cases, but at least it isn't too invasive.\n\nOK, a more polished patch is this. There are warnings about\n-fomit-function-pointers in glibc man page, at least in my simple\ntests it does not cause any issue.\n\n> But I think I still prefer just letting the corefile and the debugger do\n> their job. This error shouldn't happen much, and when it does, it should\n> be easily reproducible. Getting the bug reporter to give either a\n> reproduction recipe, or to run \"gdb git\" doesn't seem like that big a\n> hurdle.\n\nThere are other places where backtrace() support may come handy\ntoo. If -rdynamic was not needed, I would push for this patch. Too bad\nbacktrace() is not a perfect magic wand.\n\n-- 8< --\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b232908..b38f62a 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)\n \tNEEDS_LIBRT = YesPlease\n \tHAVE_GETDELIM = YesPlease\n \tSANE_TEXT_GREP=-a\n+\t# for backtrace() support with glibc >= 2.1\n+\tBASIC_LDFLAGS += -rdynamic\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 43718da..3561ab9 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -408,6 +408,7 @@ extern NORETURN void usage(const char *err);\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+extern NORETURN void BUG(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n@@ -709,6 +710,7 @@ extern int git_vsnprintf(char *str, size_t maxsize,\n #ifdef __GLIBC_PREREQ\n #if __GLIBC_PREREQ(2, 1)\n #define HAVE_STRCHRNUL\n+#define HAVE_BACKTRACE\n #endif\n #endif\n \n@@ -722,6 +724,23 @@ static inline char *gitstrchrnul(const char *s, int c)\n }\n #endif\n \n+#ifdef HAVE_BACKTRACE\n+#include \n+static inline void dump_backtrace(FILE *fp)\n+{\n+\tvoid *buffer[32];\n+\tint nptrs;\n+\n+\tnptrs = backtrace(buffer, sizeof(buffer) / sizeof(*buffer));\n+\tfflush(fp);\n+\tbacktrace_symbols_fd(buffer, nptrs, fileno(fp));\n+}\n+#else\n+static inline void dump_backtrace(FILE *fp)\n+{\n+}\n+#endif\n+\n #ifdef NO_INET_PTON\n int inet_pton(int af, const char *src, void *dst);\n #endif\ndiff --git a/usage.c b/usage.c\nindex 17f52c1..b00603c 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -204,3 +204,16 @@ void warning(const char *warn, ...)\n \twarn_routine(warn, params);\n \tva_end(params);\n }\n+\n+void NORETURN BUG(const char *fmt, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tvreportf(\"BUG: \", fmt, params);\n+\tva_end(params);\n+\n+\tdump_backtrace(error_handle ? error_handle : stderr);\n+\n+\texit(128);\n+}\n-- 8< --\n--\nDuy\n"},{"id":"304887","messageId":"20161026121058.a2pmf57oc7p2mlsp@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161026102921.GA31311@ash","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-26T12:10:58Z","receivedAt":"2016-10-26T12:11:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:\n\n> > I think you could conditionally make git_path() and all of its\n> > counterparts macros, similar to the way the trace code works. It seems\n> > like a pretty maintenance-heavy solution, though. I'd prefer\n> > conditionally compiling backtrace(); that also doesn't hit 100% of\n> > cases, but at least it isn't too invasive.\n> \n> OK, a more polished patch is this. There are warnings about\n> -fomit-function-pointers in glibc man page, at least in my simple\n> tests it does not cause any issue.\n\nYeah, I tried with -fno-omit-frame-pointer, but it didn't help. The\nglibc backtrace(3) manpage specifically says:\n\n The symbol names may be unavailable without the use of special linker\n options. For systems using the GNU linker, it is necessary to use the\n -rdynamic linker option. Note that names of \"static\" functions are not\n exposed, and won't be available in the backtrace.\n\nwhich matches the behavior I get.\n\nGcc ships with a libbacktrace which does seem to give reliable results\n(patch below for reference). But that's still relying on gcc, and on\nhaving debug symbols available. I'm not sure this is really any\nconvenience over dumping a corefile and using gdb to pull out the\nsymbols after the fact.\n\n---\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 76f885281c..62a14f10d3 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)\n \tNEEDS_LIBRT = YesPlease\n \tHAVE_GETDELIM = YesPlease\n \tSANE_TEXT_GREP=-a\n+\tBASIC_CFLAGS += -DHAVE_BACKTRACE\n+\tEXTLIBS += -lbacktrace\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\ndiff --git a/usage.c b/usage.c\nindex 17f52c1b5c..4b9762ae62 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -5,6 +5,9 @@\n */\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n+#ifdef HAVE_BACKTRACE\n+#include \n+#endif\n \n static FILE *error_handle;\n static int tweaked_error_buffering;\n@@ -24,6 +27,44 @@ void vreportf(const char *prefix, const char *err, va_list params)\n \tfputc('\\n', fh);\n }\n \n+#ifdef HAVE_BACKTRACE\n+static int full_callback(void *fh, uintptr_t pc,\n+\t\t\t const char *filename, int lineno,\n+\t\t\t const char *function)\n+{\n+\tif (!function || !filename)\n+\t\treturn 0;\n+\tfprintf(fh, \"debug: %s() at %s:%d\\n\", function, filename, lineno);\n+\treturn 0;\n+}\n+\n+static void error_callback(void *fh, const char *msg, int errnum)\n+{\n+\tfprintf(fh, \"backtrace error: %s\", msg);\n+\tif (errnum > 0)\n+\t\tfprintf(fh, \": %s\", strerror(errnum));\n+\tfputc('\\n', fh);\n+}\n+\n+static void maybe_backtrace(void)\n+{\n+\tFILE *fh = error_handle ? error_handle : stderr;\n+\tstruct backtrace_state *bt;\n+\n+\tif (!git_env_bool(\"GIT_BACKTRACE_ON_DIE\", 0))\n+\t\treturn;\n+\n+\t/* XXX obviously unportable use of /proc */\n+\tbt = backtrace_create_state(\"/proc/self/exe\", 0, error_callback, fh);\n+\tif (bt) {\n+\t\tfprintf(fh, \"debug: die() called at:\\n\");\n+\t\tbacktrace_full(bt, 3, full_callback, error_callback, fh);\n+\t}\n+}\n+#else\n+#define maybe_backtrace()\n+#endif\n+\n static NORETURN void usage_builtin(const char *err, va_list params)\n {\n \tvreportf(\"usage: \", err, params);\n@@ -33,6 +74,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n+\tmaybe_backtrace();\n \texit(128);\n }\n \n"},{"id":"304889","messageId":"CACsJy8AHMJ7OJFKyjYAsPuT0nG=e0RGn59PuX06PJ5pYjinsqA@mail.gmail.com","threadId":"44334","inReplyTo":"20161026121058.a2pmf57oc7p2mlsp@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-26T12:26:20Z","receivedAt":"2016-10-26T12:27:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 26, 2016 at 7:10 PM, Jeff King wrote:\n> On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:\n>\n>> > I think you could conditionally make git_path() and all of its\n>> > counterparts macros, similar to the way the trace code works. It seems\n>> > like a pretty maintenance-heavy solution, though. I'd prefer\n>> > conditionally compiling backtrace(); that also doesn't hit 100% of\n>> > cases, but at least it isn't too invasive.\n>>\n>> OK, a more polished patch is this. There are warnings about\n>> -fomit-function-pointers in glibc man page, at least in my simple\n>> tests it does not cause any issue.\n>\n> Yeah, I tried with -fno-omit-frame-pointer, but it didn't help. The\n> glibc backtrace(3) manpage specifically says:\n>\n> The symbol names may be unavailable without the use of special linker\n> options. For systems using the GNU linker, it is necessary to use the\n> -rdynamic linker option. Note that names of \"static\" functions are not\n> exposed, and won't be available in the backtrace.\n>\n> which matches the behavior I get.\n>\n> Gcc ships with a libbacktrace which does seem to give reliable results\n> (patch below for reference). But that's still relying on gcc, and on\n> having debug symbols available.\n\nYep. On an optimized build you can't get anywhere without debug info,\nwhich has a giant database to describe \"if your rip/pc register is\nhere, then you clue to find your caller is there\" for basically every\ninstruction in your program. Dwarf3 at least is a crazy world.\n\n> I'm not sure this is really any convenience over dumping a corefile and using gdb to pull out the\n> symbols after the fact.\n\nSo are we back to forcing core files? I'm ok with that! The only\ninconvenience I see is pointing out where the core file is, which\nshould be where `pwd` originally is. On linux we can even peek into\n/proc/sys/kernel/core_pattern if we want to be precise. ulimit can get\nin the way, but I don't if the default out there is enable or disable\ncore dumping. Once we got OS info and git version, our chances of\ncracking the core files should be reasonably high.\n-- \nDuy\n"},{"id":"304890","messageId":"20161026123156.g76z66rxxlkwosht@sigill.intra.peff.net","threadId":"44334","inReplyTo":"CACsJy8AHMJ7OJFKyjYAsPuT0nG=e0RGn59PuX06PJ5pYjinsqA@mail.gmail.com","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-26T12:31:56Z","receivedAt":"2016-10-26T12:39:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2016 at 07:26:20PM +0700, Duy Nguyen wrote:\n\n> > I'm not sure this is really any convenience over dumping a corefile\n> > and using gdb to pull out the\n> > symbols after the fact.\n> \n> So are we back to forcing core files? I'm ok with that! The only\n> inconvenience I see is pointing out where the core file is, which\n> should be where `pwd` originally is. On linux we can even peek into\n> /proc/sys/kernel/core_pattern if we want to be precise. ulimit can get\n> in the way, but I don't if the default out there is enable or disable\n> core dumping. Once we got OS info and git version, our chances of\n> cracking the core files should be reasonably high.\n\nTBH, most of the time I expect the solution to be walking the person\nthrough:\n\n git clone git://kernel.org/pub/scm/git/git.git\n cd git\n make\n\n gdb --args ./git whatever\n break die\n run\n bt\n\nwhich would cover most cases (reproducible breakage, and not in a\nsub-program). It's relatively rare that even I resort to corefiles.\n\n-Peff\n"},{"id":"306274","messageId":"20161122004421.GA12263@google.com","threadId":"44334","inReplyTo":"20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-11-22T00:44:21Z","receivedAt":"2016-11-22T00:44:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> This passes the test suite (after the adjustments in the\n> previous patches), but there's a risk of regression for any\n> cases where the fallback usually works fine but the code\n> isn't exercised by the test suite. So by itself, this\n> commit is a potential step backward, but lets us take two\n> steps forward once we've identified and fixed any such\n> instances.\n>\n> Signed-off-by: Jeff King \n> ---\n> environment.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/environment.c b/environment.c\n> index cd5aa57..b1743e6 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -164,8 +164,11 @@ static void setup_git_env(void)\n> \tconst char *replace_ref_base;\n> \n> \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n> -\tif (!git_dir)\n> +\tif (!git_dir) {\n> +\t\tif (!startup_info->have_repository)\n> +\t\t\tdie(\"BUG: setup_git_env called without repository\");\n> \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n> +\t}\n\nThis trips reproducibly for\n\n git ls-remote https://kernel.googlesource.com/pub/scm/git/git\n\nwhen run outside of a git repository.\n\nBacktrace:\n\n #0 setup_git_env () at environment.c:172\n #1 get_git_dir () at environment.c:214\n #2 get_helper at transport-helper.c:127\n #3 get_refs_list (for_push=0) at transport-helper.c:1038\n #4 transport_get_remote_refs at transport.c:1076\n #5 cmd_ls_remote at builtin/ls-remote.c:97\n\ntransport-helper.c:127 is\n\n\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n\t\t\t get_git_dir());\n\nThat code is pretty clearly wrong. Should it be made conditional\non have_git_dir(), like this?\n\nThanks,\nJonathan\n\n transport-helper.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 91aed35..e4fd982 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n-\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n-\t\t\t get_git_dir());\n+\tif (have_git_dir())\n+\t\targv_array_pushf(&helper->env_array, \"%s=%s\",\n+\t\t\t\t GIT_DIR_ENVIRONMENT, get_git_dir());\n \n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n-- \n"},{"id":"306282","messageId":"20161122024102.otlnl6jcrb3pejux@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161122004421.GA12263@google.com","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-22T02:41:02Z","receivedAt":"2016-11-22T02:41:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:\n\n> > \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n> > -\tif (!git_dir)\n> > +\tif (!git_dir) {\n> > +\t\tif (!startup_info->have_repository)\n> > +\t\t\tdie(\"BUG: setup_git_env called without repository\");\n> > \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n> > +\t}\n> \n> This trips reproducibly for\n> \n> git ls-remote https://kernel.googlesource.com/pub/scm/git/git\n> \n> when run outside of a git repository.\n> \n> Backtrace:\n> \n> #0 setup_git_env () at environment.c:172\n> #1 get_git_dir () at environment.c:214\n> #2 get_helper at transport-helper.c:127\n> #3 get_refs_list (for_push=0) at transport-helper.c:1038\n> #4 transport_get_remote_refs at transport.c:1076\n> #5 cmd_ls_remote at builtin/ls-remote.c:97\n> \n> transport-helper.c:127 is\n> \n> \targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n> \t\t\t get_git_dir());\n> \n> That code is pretty clearly wrong. Should it be made conditional\n> on have_git_dir(), like this?\n\nYeah, I think making it conditional makes the most sense. Just trying to\nthink of cases that might not be covered by your patch:\n\n - if we are not in a repository, we shouldn't ever need to override an\n existing $GIT_DIR from the environment. Because if $GIT_DIR is\n present, then we _would_ be in a repo if it's valid, and die if it\n isn't.\n\n - not setting $GIT_DIR may cause a helper to do the usual discovery\n walk to find the repository. But we know we're not in one, or we\n would have found it ourselves. So worst case it may expend a little\n effort to try to find a repository and fail (I think remote-curl\n would do this to try to find repo-level config).\n\nBoth of those points assume that we've already called\nsetup_git_directory_gently(), but I think that's a reasonable\nassumption. And certainly is true for ls-remote, and should be for any\ngit command that invokes the transport code.\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 91aed35..e4fd982 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)\n> \thelper->git_cmd = 0;\n> \thelper->silent_exec_failure = 1;\n> \n> -\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n> -\t\t\t get_git_dir());\n> +\tif (have_git_dir())\n> +\t\targv_array_pushf(&helper->env_array, \"%s=%s\",\n> +\t\t\t\t GIT_DIR_ENVIRONMENT, get_git_dir());\n\nSo yeah, I think this is the extent of the change needed.\n\nThanks.\n\n-Peff\n"},{"id":"306285","messageId":"xmqq4m30gp2i.fsf@gitster.mtv.corp.google.com","threadId":"44334","inReplyTo":"20161122004421.GA12263@google.com","subject":"Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to \".git\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T03:40:05Z","receivedAt":"2016-11-22T03:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> This trips reproducibly for\n>\n> git ls-remote https://kernel.googlesource.com/pub/scm/git/git\n>\n> when run outside of a git repository.\n>\n> Backtrace:\n>\n> #0 setup_git_env () at environment.c:172\n> #1 get_git_dir () at environment.c:214\n> #2 get_helper at transport-helper.c:127\n> #3 get_refs_list (for_push=0) at transport-helper.c:1038\n> #4 transport_get_remote_refs at transport.c:1076\n> #5 cmd_ls_remote at builtin/ls-remote.c:97\n>\n> transport-helper.c:127 is\n>\n> \targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n> \t\t\t get_git_dir());\n>\n> That code is pretty clearly wrong. Should it be made conditional\n> on have_git_dir(), like this?\n\nLooks good and I agree with Peff's analysis. Care to wrap it in a\npatch with a log message?\n\nThanks.\n\n>\n> Thanks,\n> Jonathan\n>\n> transport-helper.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 91aed35..e4fd982 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)\n> \thelper->git_cmd = 0;\n> \thelper->silent_exec_failure = 1;\n> \n> -\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n> -\t\t\t get_git_dir());\n> +\tif (have_git_dir())\n> +\t\targv_array_pushf(&helper->env_array, \"%s=%s\",\n> +\t\t\t\t GIT_DIR_ENVIRONMENT, get_git_dir());\n> \n> \tcode = start_command(helper);\n> \tif (code < 0 && errno == ENOENT)\n"},{"id":"308507","messageId":"20161230001114.GB7883@aiede.mtv.corp.google.com","threadId":"44334","inReplyTo":"20161122024102.otlnl6jcrb3pejux@sigill.intra.peff.net","subject":"[PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-12-30T00:11:14Z","receivedAt":"2016-12-30T00:11:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"To push from or fetch to the current repository, remote helpers need\nto know what repository that is. Accordingly, Git sets the GIT_DIR\nenvironment variable to the path to the current repository when\ninvoking remote helpers.\n\nThere is a special case it does not handle: \"git ls-remote\" and \"git\narchive --remote\" can be run to inspect a remote repository without\nbeing run from any local repository. GIT_DIR is not useful in this\nscenario:\n\n- if we are not in a repository, we don't need to set GIT_DIR to\n override an existing GIT_DIR value from the environment. If GIT_DIR\n is present then we would be in a repository if it were valid and\n would have called die() if it weren't.\n\n- not setting GIT_DIR may cause a helper to do the usual discovery\n walk to find the repository. But we know we're not in one, or we\n would have found it ourselves. So in the worst case it may expend\n a little extra effort to try to find a repository and fail (for\n example, remote-curl would do this to try to find repository-level\n configuration).\n\nSo leave GIT_DIR unset in this case. This makes GIT_DIR easier to\nunderstand for remote helper authors and makes transport code less of\na special case for repository discovery.\n\nNoticed using b1ef400e (setup_git_env: avoid blind fall-back to\n\".git\", 2016-10-20) from 'next':\n\n $ cd /tmp\n $ git ls-remote https://kernel.googlesource.com/pub/scm/git/git\n fatal: BUG: setup_git_env called without repository\n\nWhile at it, add some tests for other variables in the environment of\ncommands run by git-remote-ext.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Nieder \n---\nJeff King wrote:\n\n> So yeah, I think this is the extent of the change needed.\n\nThanks. Here's the patch again, now with commit messages and a test.\nThanks for the analysis and sorry for the slow turnaround.\n\nSincerely,\nJonathan\n\n t/t5550-http-fetch-dumb.sh | 15 ++++++++\n t/t5551-http-fetch-smart.sh | 15 ++++++++\n t/t5570-git-daemon.sh | 15 ++++++++\n t/t5802-connect-helper.sh | 84 ++++++++++++++++++++++++++++++++++++++++++++-\n transport-helper.c | 5 +--\n 5 files changed, 131 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex aeb3a63f7c..a86fca3e6c 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '\n \ttest_cmp file clone/file\n '\n \n+test_expect_success 'list refs from outside any repository' '\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse master)\tHEAD\n+\t$(git rev-parse master)\trefs/heads/master\n+\tEOF\n+\tmkdir lsremote-root &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd lsremote-root &&\n+\t\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create password-protected repository' '\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6e5b9e42fb..7ba894f0f4 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '\n \texpect_askpass both user@host auth/smart/repo.git\n '\n \n+test_expect_success 'list refs from outside any repository' '\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse master)\tHEAD\n+\t$(git rev-parse master)\trefs/heads/master\n+\tEOF\n+\tmkdir lsremote-root &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd lsremote-root &&\n+\t\tgit ls-remote \"$HTTPD_URL/smart/repo.git\" >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'disable dumb http on server' '\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n \t\tconfig http.getanyfile false\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 225a022e8a..4573d98e6c 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '\n \ttest_cmp file clone/file\n '\n \n+test_expect_success 'list refs from outside any repository' '\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse master)\tHEAD\n+\t$(git rev-parse master)\trefs/heads/master\n+\tEOF\n+\tmkdir lsremote-root &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd lsremote-root &&\n+\t\tgit ls-remote \"$GIT_DAEMON_URL/repo.git\" >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'fetch changes via git protocol' '\n \techo content >>file &&\n \tgit commit -a -m two &&\ndiff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh\nindex c6c2661878..7232032cd2 100755\n--- a/t/t5802-connect-helper.sh\n+++ b/t/t5802-connect-helper.sh\n@@ -3,6 +3,10 @@\n test_description='ext::cmd remote \"connect\" helper'\n . ./test-lib.sh\n \n+escape_spaces () {\n+\techo \"$*\" | sed -e \"s/ /% /g\"\n+}\n+\n test_expect_success setup '\n \tgit config --global protocol.ext.allow user &&\n \ttest_tick &&\n@@ -19,7 +23,7 @@ test_expect_success setup '\n '\n \n test_expect_success clone '\n-\tcmd=$(echo \"echo >&2 ext::sh invoked && %S ..\" | sed -e \"s/ /% /g\") &&\n+\tcmd=$(escape_spaces \"echo >&2 ext::sh invoked && %S ..\") &&\n \tgit clone \"ext::sh -c %S% .\" dst &&\n \tgit for-each-ref refs/heads/ refs/tags/ >expect &&\n \t(\n@@ -70,6 +74,84 @@ test_expect_success 'update backfilled tag without primary transfer' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'GIT_EXT_SERVICE for clone, ls-remote, push, archive' '\n+\trm -rf dst &&\n+\t>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tgit-upload-pack\n+\tgit-upload-pack\n+\tgit-receive-pack\n+\tgit-upload-archive\n+\tEOF\n+\tgit archive HEAD >expect.tar &&\n+\tcmd=$(escape_spaces \"echo >>actual \\$GIT_EXT_SERVICE && %S .\") &&\n+\n+\tgit clone \"ext::sh -c $cmd\" dst &&\n+\tgit ls-remote \"ext::sh -c $cmd\" &&\n+\ttest_when_finished \"git update-ref -d refs/heads/newbranch\" &&\n+\tgit push \"ext::sh -c $cmd\" master:newbranch &&\n+\tgit archive --remote=\"ext::sh -c $cmd\" HEAD >actual.tar &&\n+\n+\ttest_cmp expect actual &&\n+\ttest_cmp expect.tar actual.tar\n+'\n+\n+test_expect_success 'GIT_EXT_SERVICE_NOPREFIX for clone, ls-remote, push, archive' '\n+\trm -rf dst &&\n+\t>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tupload-pack\n+\tupload-pack\n+\treceive-pack\n+\tupload-archive\n+\tEOF\n+\tgit archive HEAD >expect.tar &&\n+\tcmd=$(escape_spaces \"echo >>actual \\$GIT_EXT_SERVICE_NOPREFIX && %S .\") &&\n+\n+\tgit clone \"ext::sh -c $cmd\" dst &&\n+\tgit ls-remote \"ext::sh -c $cmd\" &&\n+\ttest_when_finished \"git update-ref -d refs/heads/newbranch\" &&\n+\tgit push \"ext::sh -c $cmd\" master:newbranch &&\n+\tgit archive --remote=\"ext::sh -c $cmd\" HEAD >actual.tar &&\n+\n+\ttest_cmp expect actual &&\n+\ttest_cmp expect.tar actual.tar\n+'\n+\n+test_expect_success 'GIT_DIR is set to the enclosing repo for ls-remote' '\n+\tgit rev-parse --git-dir >expect &&\n+\tcmd=$(escape_spaces \"echo \\$GIT_DIR >actual && %S .\") &&\n+\tgit ls-remote \"ext::sh -c $cmd\" &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'GIT_DIR is set to the enclosing repo for archive' '\n+\tgit rev-parse --git-dir >expect &&\n+\tgit archive HEAD >expect.tar &&\n+\tcmd=$(escape_spaces \"echo \\$GIT_DIR >actual && %S .\") &&\n+\tgit archive --remote=\"ext::sh -c $cmd\" HEAD >actual.tar &&\n+\ttest_cmp expect actual &&\n+\ttest_cmp expect.tar actual.tar\n+'\n+\n+test_expect_success 'GIT_DIR is not set if there is no enclosing repo' '\n+\trm -rf subdir &&\n+\t>actual &&\n+\tprintf \"%s\\n\" unset unset >expect &&\n+\tgit archive HEAD >expect.tar &&\n+\n+\tmkdir subdir &&\n+\tcmd=$(escape_spaces \"echo \\${GIT_DIR-unset} >>../actual && %S ..\") &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd subdir &&\n+\t\tgit ls-remote \"ext::sh -c $cmd\" &&\n+\t\tgit archive --remote=\"ext::sh -c $cmd\" HEAD >../actual.tar\n+\t) &&\n+\ttest_cmp expect actual &&\n+\ttest_cmp expect.tar actual.tar\n+'\n \n test_expect_success 'set up fake git-daemon' '\n \tmkdir remote &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 91aed35ebb..e4fd982383 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n-\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n-\t\t\t get_git_dir());\n+\tif (have_git_dir())\n+\t\targv_array_pushf(&helper->env_array, \"%s=%s\",\n+\t\t\t\t GIT_DIR_ENVIRONMENT, get_git_dir());\n \n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n-- \n2.11.0.355.g1ede815\n\n"},{"id":"308508","messageId":"CAGZ79kZiR7grGeMrMFRuQQ0kW6eQ4m=ZizGHDbT-s14iAj8hWQ@mail.gmail.com","threadId":"44334","inReplyTo":"20161230001114.GB7883@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-30T00:37:30Z","receivedAt":"2016-12-30T00:39:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> + mkdir lsremote-root &&\n> + (\n> + GIT_CEILING_DIRECTORIES=$(pwd) &&\n> + export GIT_CEILING_DIRECTORIES &&\n> + cd lsremote-root &&\n> + git ls-remote \"$HTTPD_URL/smart/repo.git\" >../actual\n> + ) &&\n\nWe could avoid the subshell via\n\nGIT_CEILING_DIRECTORIES=$(pwd) \\\n git -C lsremote-root lsremote ... >actual\n\nNot sure if it is worth to trade off a block of code (and an extra shell\nat run time) for an overly long line.\n\nThe rest looks good to me.\n\nThanks,\nStefan\n"},{"id":"308510","messageId":"20161230004845.rknafqsyosmyr6z2@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161230001114.GB7883@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-30T00:48:45Z","receivedAt":"2016-12-30T00:48:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:\n\n> Thanks. Here's the patch again, now with commit messages and a test.\n> Thanks for the analysis and sorry for the slow turnaround.\n\nThanks for following up. While working on a similar one recently, I had\nthe nagging feeling that there was a case we had found that was still to\nbe dealt with, and I think this was it. :)\n\nThe patch to the C code looks good. I have a few comments on the tests:\n\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index aeb3a63f7c..a86fca3e6c 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '\n> \ttest_cmp file clone/file\n> '\n> \n> +test_expect_success 'list refs from outside any repository' '\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse master)\tHEAD\n> +\t$(git rev-parse master)\trefs/heads/master\n> +\tEOF\n> +\tmkdir lsremote-root &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd lsremote-root &&\n> +\t\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >../actual\n> +\t) &&\n> +\ttest_cmp expect actual\n> +'\n\nSince my recent de95302a4c (t5000: extract nongit function to\ntest-lib-functions.sh, 2016-12-15), this can be shortened to:\n\n cat >expect <<-EOF &&\n ...\n EOF\n nongit git ls-remote \"$HTTPD_URL/dumb/repo.git\" >actual &&\n test_cmp expect actual\n\nI think that commit is in 'master' now.\n\nWithout my patch to die() in setup_git_env(), I think this would pass\nwith or without your patch, right? I think the current behavior _is_\nbuggy, but a setup to show off the improvement would be so arcane that I\ndon't think it is worth it. E.g., something like:\n\n echo '[http]extraHeader = \"Foo: Bar\" >nongit/.git/config\n\nwould probably trigger the use of that config when it shouldn't. But\nthat's really stretching.\n\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 6e5b9e42fb..7ba894f0f4 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '\n> \texpect_askpass both user@host auth/smart/repo.git\n> '\n> \n> +test_expect_success 'list refs from outside any repository' '\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse master)\tHEAD\n> +\t$(git rev-parse master)\trefs/heads/master\n> +\tEOF\n> +\tmkdir lsremote-root &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd lsremote-root &&\n> +\t\tgit ls-remote \"$HTTPD_URL/smart/repo.git\" >../actual\n> +\t) &&\n> +\ttest_cmp expect actual\n> +'\n\nIs this really testing anything that the dumb one isn't? The interesting\nbit is in invoking git-remote-http, not what it does under the hood. I\ndon't mind too much being thorough, but if we are going to go that route\nwe should probably come up with a standard battery of tests for dumb\nand smart HTTP, and then invoke them for each case without having to\nwrite each one twice.\n\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> index 225a022e8a..4573d98e6c 100755\n> --- a/t/t5570-git-daemon.sh\n> +++ b/t/t5570-git-daemon.sh\n> @@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '\n> \ttest_cmp file clone/file\n> '\n> \n> +test_expect_success 'list refs from outside any repository' '\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse master)\tHEAD\n> +\t$(git rev-parse master)\trefs/heads/master\n> +\tEOF\n> +\tmkdir lsremote-root &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd lsremote-root &&\n> +\t\tgit ls-remote \"$GIT_DAEMON_URL/repo.git\" >../actual\n> +\t) &&\n> +\ttest_cmp expect actual\n> +'\n\nThis one isn't actually broken now, right? The test is just there to\ncatch future regressions?\n\nIf we are being thorough, then would we also care about file-local\nrepos, git-over-ssh, etc?\n\n> diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh\n> index c6c2661878..7232032cd2 100755\n> --- a/t/t5802-connect-helper.sh\n> +++ b/t/t5802-connect-helper.sh\n\nThis one is quite a big addition. I know this falls under the \"while\nwe're at it\" line at the end of your commit message, but maybe it's\nworth pulling the GIT_EXT_SERVICE bits out into their own patch (and\nexplaining briefly what's going on; I had to go look up what\nGIT_EXT_SERVICE_NOPREFIX even was).\n\n-Peff\n"},{"id":"308511","messageId":"20161230004944.pkblwpxwkacpk7dd@sigill.intra.peff.net","threadId":"44334","inReplyTo":"CAGZ79kZiR7grGeMrMFRuQQ0kW6eQ4m=ZizGHDbT-s14iAj8hWQ@mail.gmail.com","subject":"Re: [PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-30T00:49:44Z","receivedAt":"2016-12-30T00:49:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 29, 2016 at 04:37:30PM -0800, Stefan Beller wrote:\n\n> > + mkdir lsremote-root &&\n> > + (\n> > + GIT_CEILING_DIRECTORIES=$(pwd) &&\n> > + export GIT_CEILING_DIRECTORIES &&\n> > + cd lsremote-root &&\n> > + git ls-remote \"$HTTPD_URL/smart/repo.git\" >../actual\n> > + ) &&\n> \n> We could avoid the subshell via\n> \n> GIT_CEILING_DIRECTORIES=$(pwd) \\\n> git -C lsremote-root lsremote ... >actual\n> \n> Not sure if it is worth to trade off a block of code (and an extra shell\n> at run time) for an overly long line.\n> \n> The rest looks good to me.\n\nI mentioned elsewhere that we now have a \"nongit\" function to do this as\na one-liner. It might be worth applying your optimization to that\nfunction, so it would take effect in may places.\n\n-Peff\n"},{"id":"311521","messageId":"20170214061607.qyucfue335aqgji2@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20161230004845.rknafqsyosmyr6z2@sigill.intra.peff.net","subject":"Re: [PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:16:07Z","receivedAt":"2017-02-14T06:16:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:\n\n> On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:\n> \n> > Thanks. Here's the patch again, now with commit messages and a test.\n> > Thanks for the analysis and sorry for the slow turnaround.\n> \n> Thanks for following up. While working on a similar one recently, I had\n> the nagging feeling that there was a case we had found that was still to\n> be dealt with, and I think this was it. :)\n> \n> The patch to the C code looks good. I have a few comments on the tests:\n\nI happened to run into this problem myself today, so I thought I'd prod\nyou. I think your patch looks good. Hopefully I didn't scare you off\nwith my comments. :)\n\n-Peff\n"},{"id":"311564","messageId":"xmqqtw7w8uay.fsf@gitster.mtv.corp.google.com","threadId":"44334","inReplyTo":"20170214061607.qyucfue335aqgji2@sigill.intra.peff.net","subject":"Re: [PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T19:08:05Z","receivedAt":"2017-02-14T19:08:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:\n>\n>> On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:\n>> \n>> > Thanks. Here's the patch again, now with commit messages and a test.\n>> > Thanks for the analysis and sorry for the slow turnaround.\n>> \n>> Thanks for following up. While working on a similar one recently, I had\n>> the nagging feeling that there was a case we had found that was still to\n>> be dealt with, and I think this was it. :)\n>> \n>> The patch to the C code looks good. I have a few comments on the tests:\n>\n> I happened to run into this problem myself today, so I thought I'd prod\n> you. I think your patch looks good. Hopefully I didn't scare you off\n> with my comments. :)\n\nThanks for prodding. I'm tempted to just rip out everything other\nthan the 5-liner fix to transport.c and apply it, expecting that a\nfollow-up patch with updated tests to come sometime not in too\ndistant future.\n\n"},{"id":"311571","messageId":"20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net","threadId":"44334","inReplyTo":"xmqqtw7w8uay.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T20:31:17Z","receivedAt":"2017-02-14T20:31:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 11:08:05AM -0800, Junio C Hamano wrote:\n\n> Thanks for prodding. I'm tempted to just rip out everything other\n> than the 5-liner fix to transport.c and apply it, expecting that a\n> follow-up patch with updated tests to come sometime not in too\n> distant future.\n\nI think we can at least include one basic test. Also, as it turns out\nthe problem I was seeing _wasn't_ the same one Jonathan fixed. There's\nanother bug. :)\n\nSo here's a patch series that fixes my bug, and then a cut-down version\nof Jonathan's patch. I'd be happy to see more tests come on top. I don't\nthink there's a huge rush on getting any of this into master. There are\nbugs in the existing code, but they're very hard to trigger in practice\n(e.g., a non-repo which happens to have a bunch of repo-like files). It\nonly becomes an issue in 'next' when we die(\"BUG\") to flush these cases\nout.\n\n [1/2]: remote: avoid reading $GIT_DIR config in non-repo\n [2/2]: remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR\n\n remote.c | 2 +-\n t/t5512-ls-remote.sh | 9 +++++++++\n t/t5550-http-fetch-dumb.sh | 9 +++++++++\n transport-helper.c | 5 +++--\n 4 files changed, 22 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"311572","messageId":"20170214203327.icdvrhwpmarvwe5y@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net","subject":"[PATCH 1/2] remote: avoid reading $GIT_DIR config in non-repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T20:33:28Z","receivedAt":"2017-02-14T20:33:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"git ls-remote\" command can be run outside of a\nrepository, but needs to look up configured remotes. The\nconfig code is smart enough to handle this case itself, but\nwe also check the historical \"branches\" and \"remotes\" paths\nin $GIT_DIR. The git_path() function causes us to blindly\nlook at \".git/remotes\", even if we know we aren't in a git\nrepository.\n\nFor now, this is just an unlikely bug (you probably don't\nhave such a file if you're not in a repository), but it will\nbecome more obvious once we merge b1ef400ee (setup_git_env:\navoid blind fall-back to \".git\", 2016-10-20):\n\n [now]\n $ git ls-remote\n fatal: No remote configured to list refs from.\n\n [with b1ef400ee]\n $ git ls-remote\n fatal: BUG: setup_git_env called without repository\n\nWe can fix this by skipping these sources entirely when\nwe're outside of a repository.\n\nThe test is a little more complex than the demonstration\nabove. Rather than detect the correct behavior by parsing\nthe error message, we can actually set up a case where the\nremote name we give is a valid repository, but b1ef400ee\nwould cause us to die in the configuration step.\n\nThis test doesn't fail now, but it future-proofs us for the\nb1ef400ee change.\n\nSigned-off-by: Jeff King \n---\n remote.c | 2 +-\n t/t5512-ls-remote.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex bf1bf2309..9f83fe2c4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -693,7 +693,7 @@ static struct remote *remote_get_1(const char *name,\n \t\tname = get_default(current_branch, &name_given);\n \n \tret = make_remote(name, 0);\n-\tif (valid_remote_nick(name)) {\n+\tif (valid_remote_nick(name) && have_git_dir()) {\n \t\tif (!valid_remote(ret))\n \t\t\tread_remotes_file(ret);\n \t\tif (!valid_remote(ret))\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 55fc83fc0..94fc9be9c 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -248,4 +248,13 @@ test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-complian\n \ttest_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\n '\n \n+test_expect_success 'ls-remote works outside repository' '\n+\t# It is important for this repo to be inside the nongit\n+\t# area, as we want a repo name that does not include\n+\t# slashes (because those inhibit some of our configuration\n+\t# lookups).\n+\tnongit git init --bare dst.git &&\n+\tnongit git ls-remote dst.git\n+'\n+\n test_done\n-- \n2.12.0.rc1.479.g59880b11e\n\n"},{"id":"311574","messageId":"20170214203619.62plnss65mdwf3na@sigill.intra.peff.net","threadId":"44334","inReplyTo":"20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net","subject":"[PATCH 2/2] remote helpers: avoid blind fall-back to \".git\" when setting GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T20:36:19Z","receivedAt":"2017-02-14T20:36:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jonathan Nieder \n\nTo push from or fetch to the current repository, remote helpers need\nto know what repository that is. Accordingly, Git sets the GIT_DIR\nenvironment variable to the path to the current repository when\ninvoking remote helpers.\n\nThere is a special case it does not handle: \"git ls-remote\" and \"git\narchive --remote\" can be run to inspect a remote repository without\nbeing run from any local repository. GIT_DIR is not useful in this\nscenario:\n\n- if we are not in a repository, we don't need to set GIT_DIR to\n override an existing GIT_DIR value from the environment. If GIT_DIR\n is present then we would be in a repository if it were valid and\n would have called die() if it weren't.\n\n- not setting GIT_DIR may cause a helper to do the usual discovery\n walk to find the repository. But we know we're not in one, or we\n would have found it ourselves. So in the worst case it may expend\n a little extra effort to try to find a repository and fail (for\n example, remote-curl would do this to try to find repository-level\n configuration).\n\nSo leave GIT_DIR unset in this case. This makes GIT_DIR easier to\nunderstand for remote helper authors and makes transport code less of\na special case for repository discovery.\n\nNoticed using b1ef400e (setup_git_env: avoid blind fall-back to\n\".git\", 2016-10-20) from 'next':\n\n $ cd /tmp\n $ git ls-remote https://kernel.googlesource.com/pub/scm/git/git\n fatal: BUG: setup_git_env called without repository\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Nieder \nSigned-off-by: Jeff King \n---\nI dropped this down to a single test instance, and used the nongit\nhelper to shorten it.\n\nPossible patches on top:\n\n - if we want to test this across more protocols, we can. I'm not sure\n I see all that much value in it, given that we know the source of\n the bug.\n\n We probably _should_ have some kind of standard test-battery that\n hits all protocols, or at the very least hits both dumb/smart http.\n But waiting for that may be making the perfect the enemy of the\n good. So I'm OK with doing it piece-wise for now if people really\n feel we need to cover more protocols.\n\n - Jonathan's original had some nice remote-ext tests, but they were\n sufficiently complex that they should be spun into their own patch\n with more explanation.\n\n t/t5550-http-fetch-dumb.sh | 9 +++++++++\n transport-helper.c | 5 +++--\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex aeb3a63f7..b69ece1d6 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -34,6 +34,15 @@ test_expect_success 'clone http repository' '\n \ttest_cmp file clone/file\n '\n \n+test_expect_success 'list refs from outside any repository' '\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse master)\tHEAD\n+\t$(git rev-parse master)\trefs/heads/master\n+\tEOF\n+\tnongit git ls-remote \"$HTTPD_URL/dumb/repo.git\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create password-protected repository' '\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 91aed35eb..e4fd98238 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n-\targv_array_pushf(&helper->env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT,\n-\t\t\t get_git_dir());\n+\tif (have_git_dir())\n+\t\targv_array_pushf(&helper->env_array, \"%s=%s\",\n+\t\t\t\t GIT_DIR_ENVIRONMENT, get_git_dir());\n \n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n-- \n2.12.0.rc1.479.g59880b11e\n"}]}