[PATCH 0/7] stop blind fallback to ".git"

This is a follow-on to the series in:

  http://public-inbox.org/git/20160913032242.coyuhyhn6uklewuk@sigill.intra.peff.net/

That series avoided looking at ".git/config" when we haven't discovered
whether we are in a git repo. This takes that further and avoids ever
looking at ".git" as a fallback.

Patches 1-6 just teach various low-level code to detect and handle the
case when we are not in a configure repository (along with associated
cleanups). The final patch actually disallows this case (and the early
fixes were found by running the test suite with just the final patch).

I think this is a step in the right direction, both in fixing bugs, but
also in making our setup and repository-access code easier to reason
about. However, it does carry a risk of regression, if there are more
"fixes" that I missed. If we wanted to be really conservative, we could
hold back the 7th patch as a separate topic and cook it in "next" for an
extra release cycle or something. I'm not all that worried, though.

I built this on top of jc/diff-unique-abbrev-comments, as it refactors
that same function (and it didn't seem like a bad topic to be held
hostage by).

  [1/7]: read info/{attributes,exclude} only when in repository
  [2/7]: test-*-cache-tree: setup git dir
  [3/7]: find_unique_abbrev: use 4-buffer ring
  [4/7]: diff_unique_abbrev: rename to diff_aligned_abbrev
  [5/7]: diff_aligned_abbrev: use "struct oid"
  [6/7]: diff: handle sha1 abbreviations outside of repository
  [7/7]: setup_git_env: avoid blind fall-back to ".git"

 attr.c                           |  6 +++++-
 builtin/merge.c                  | 11 +++++-----
 builtin/receive-pack.c           | 16 ++++++---------
 cache.h                          |  4 ++--
 combine-diff.c                   |  6 +++---
 diff.c                           | 43 +++++++++++++++++++++++++---------------
 diff.h                           |  6 +++++-
 dir.c                            | 12 +++++------
 environment.c                    |  5 ++++-
 sha1_name.c                      |  4 +++-
 t/helper/test-dump-cache-tree.c  |  1 +
 t/helper/test-scrap-cache-tree.c |  1 +
 12 files changed, 68 insertions(+), 47 deletions(-)

The low-level attribute and gitignore code will try to look
in $GIT_DIR/info for any repo-level configuration files,
even if we have not actually determined that we are in a
repository (e.g., running "git grep --no-index"). In such a
case they end up looking for ".git/info/attributes", etc.

This is generally harmless, as such a file is unlikely to
exist outside of a repository, but it's still conceptually
the wrong thing to do.

Let's detect this situation explicitly and skip reading the
file (i.e., the same behavior we'd get if we were in a
repository and the file did not exist).

Signed-off-by: Jeff King <redacted>
---
 attr.c |  6 +++++-
 dir.c  | 12 ++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/attr.c b/attr.c
index eec5d7d..1fcf042 100644
--- a/attr.c
+++ b/attr.c

@@ -531,7 +531,11 @@ static void bootstrap_attr_stack(void)
 		debug_push(elem);
 	}
 
-	elem = read_attr_from_file(git_path_info_attributes(), 1);
+	if (startup_info->have_repository)
+		elem = read_attr_from_file(git_path_info_attributes(), 1);
+	else
+		elem = NULL;
+
 	if (!elem)
 		elem = xcalloc(1, sizeof(*elem));
 	elem->origin = NULL;

diff --git a/dir.c b/dir.c
index 3bad1ad..6cde773 100644
--- a/dir.c
+++ b/dir.c

@@ -2195,8 +2195,6 @@ static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
 
 void setup_standard_excludes(struct dir_struct *dir)
 {
-	const char *path;
-
 	dir->exclude_per_dir = ".gitignore";
 
 	/* core.excludefile defaulting to $XDG_HOME/git/ignore */

@@ -2207,10 +2205,12 @@ void setup_standard_excludes(struct dir_struct *dir)
 					 dir->untracked ? &dir->ss_excludes_file : NULL);
 
 	/* per repository user preference */
-	path = git_path_info_exclude();
-	if (!access_or_warn(path, R_OK, 0))
-		add_excludes_from_file_1(dir, path,
-					 dir->untracked ? &dir->ss_info_exclude : NULL);
+	if (startup_info->have_repository) {
+		const char *path = git_path_info_exclude();
+		if (!access_or_warn(path, R_OK, 0))
+			add_excludes_from_file_1(dir, path,
+						 dir->untracked ? &dir->ss_info_exclude : NULL);
+	}
 }
 
 int remove_path(const char *name)

-- 
2.10.1.619.g16351a7

These test helper programs access the index, but do not ever
setup_git_directory(), meaning we just blindly looked in
".git/index". This happened to work for the purposes of our
tests (which do not run from subdirectories, nor in
non-repos), but it's a bad habit.

Signed-off-by: Jeff King <redacted>
---
 t/helper/test-dump-cache-tree.c  | 1 +
 t/helper/test-scrap-cache-tree.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c
index 44f3290..7af116d 100644
--- a/t/helper/test-dump-cache-tree.c
+++ b/t/helper/test-dump-cache-tree.c

@@ -58,6 +58,7 @@ int cmd_main(int ac, const char **av)
 {
 	struct index_state istate;
 	struct cache_tree *another = cache_tree();
+	setup_git_directory();
 	if (read_cache() < 0)
 		die("unable to read index file");
 	istate = the_index;

diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index 5b2fd09..27fe040 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c

@@ -7,6 +7,7 @@ static struct lock_file index_lock;
 
 int cmd_main(int ac, const char **av)
 {
+	setup_git_directory();
 	hold_locked_index(&index_lock, 1);
 	if (read_cache() < 0)
 		die("unable to read index file");

-- 
2.10.1.619.g16351a7

Some code paths want to format multiple abbreviated sha1s in
the same output line. Because we use a single static buffer
for our return value, they have to either break their output
into several calls or allocate their own arrays and use
find_unique_abbrev_r().

Intead, let's mimic sha1_to_hex() and use a ring of several
buffers, so that the return value stays valid through
multiple calls. This shortens some of the callers, and makes
it harder to for them to make a silly mistake.

Signed-off-by: Jeff King <redacted>
---
It feels a little funny in these callers to be moving from a reentrant
function to one that relies on a static buffer. But I feel like the
result is more obvious and less error-prone, and the "ring of buffers"
concept has proven useful and safe in sha1_to_hex().

My ulterior motive is that while refactoring one of the later patches, I
just assumed that we did have a ring of buffers, and introduced a subtle
bug.

 builtin/merge.c        | 11 +++++------
 builtin/receive-pack.c | 16 ++++++----------
 cache.h                |  4 ++--
 sha1_name.c            |  4 +++-
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a8b57c7..b65eeaa 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -1374,12 +1374,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		struct commit *commit;
 
 		if (verbosity >= 0) {
-			char from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];
-			find_unique_abbrev_r(from, head_commit->object.oid.hash,
-					      DEFAULT_ABBREV);
-			find_unique_abbrev_r(to, remoteheads->item->object.oid.hash,
-					      DEFAULT_ABBREV);
-			printf(_("Updating %s..%s\n"), from, to);
+			printf(_("Updating %s..%s\n"),
+			       find_unique_abbrev(head_commit->object.oid.hash,
+						  DEFAULT_ABBREV),
+			       find_unique_abbrev(remoteheads->item->object.oid.hash,
+						  DEFAULT_ABBREV));
 		}
 		strbuf_addstr(&msg, "Fast-forward");
 		if (have_message)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 04ed38e..680759d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -1163,10 +1163,6 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	struct string_list_item *item;
 	struct command *dst_cmd;
 	unsigned char sha1[GIT_SHA1_RAWSZ];
-	char cmd_oldh[GIT_SHA1_HEXSZ + 1],
-	     cmd_newh[GIT_SHA1_HEXSZ + 1],
-	     dst_oldh[GIT_SHA1_HEXSZ + 1],
-	     dst_newh[GIT_SHA1_HEXSZ + 1];
 	int flag;
 
 	strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);

@@ -1197,14 +1193,14 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 
 	dst_cmd->skip_update = 1;
 
-	find_unique_abbrev_r(cmd_oldh, cmd->old_sha1, DEFAULT_ABBREV);
-	find_unique_abbrev_r(cmd_newh, cmd->new_sha1, DEFAULT_ABBREV);
-	find_unique_abbrev_r(dst_oldh, dst_cmd->old_sha1, DEFAULT_ABBREV);
-	find_unique_abbrev_r(dst_newh, dst_cmd->new_sha1, DEFAULT_ABBREV);
 	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
 		 " its target '%s' (%s..%s)",
-		 cmd->ref_name, cmd_oldh, cmd_newh,
-		 dst_cmd->ref_name, dst_oldh, dst_newh);
+		 cmd->ref_name,
+		 find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV),
+		 find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV),
+		 dst_cmd->ref_name,
+		 find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV),
+		 find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
 
 	cmd->error_string = dst_cmd->error_string =
 		"inconsistent aliased update";

diff --git a/cache.h b/cache.h
index 05ecb88..6e06311 100644
--- a/cache.h
+++ b/cache.h

@@ -901,8 +901,8 @@ extern char *sha1_pack_index_name(const unsigned char *sha1);
  * The result will be at least `len` characters long, and will be NUL
  * terminated.
  *
- * The non-`_r` version returns a static buffer which will be overwritten by
- * subsequent calls.
+ * The non-`_r` version returns a static buffer which remains valid until 4
+ * more calls to find_unique_abbrev are made.
  *
  * The `_r` variant writes to a buffer supplied by the caller, which must be at
  * least `GIT_SHA1_HEXSZ + 1` bytes. The return value is the number of bytes

diff --git a/sha1_name.c b/sha1_name.c
index 4092836..36ce9b9 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -472,7 +472,9 @@ int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
 
 const char *find_unique_abbrev(const unsigned char *sha1, int len)
 {
-	static char hex[GIT_SHA1_HEXSZ + 1];
+	static int bufno;
+	static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
+	char *hex = hexbuffer[3 & ++bufno];
 	find_unique_abbrev_r(hex, sha1, len);
 	return hex;
 }

-- 
2.10.1.619.g16351a7

The word "align" describes how the function actually differs
from find_unique_abbrev, and will make it less confusing
when we add more diff-specific abbrevation functions that do
not do this alignment.

Since this is a globally available function, let's also move
its descriptive comment to the header file, where we
typically document function interfaces.

Signed-off-by: Jeff King <redacted>
---
 combine-diff.c |  6 +++---
 diff.c         | 10 +++-------
 diff.h         |  6 +++++-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 8e2a577..b36c2d1 100644
--- a/combine-diff.c
+++ b/combine-diff.c

@@ -1203,9 +1203,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 
 		/* Show sha1's */
 		for (i = 0; i < num_parent; i++)
-			printf(" %s", diff_unique_abbrev(p->parent[i].oid.hash,
-							 opt->abbrev));
-		printf(" %s ", diff_unique_abbrev(p->oid.hash, opt->abbrev));
+			printf(" %s", diff_aligned_abbrev(p->parent[i].oid.hash,
+							  opt->abbrev));
+		printf(" %s ", diff_aligned_abbrev(p->oid.hash, opt->abbrev));
 	}
 
 	if (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {

diff --git a/diff.c b/diff.c
index f5d6d7e..2d8b74b 100644
--- a/diff.c
+++ b/diff.c

@@ -4136,11 +4136,7 @@ void diff_free_filepair(struct diff_filepair *p)
 	free(p);
 }
 
-/*
- * This is different from find_unique_abbrev() in that
- * it stuffs the result with dots for alignment.
- */
-const char *diff_unique_abbrev(const unsigned char *sha1, int len)
+const char *diff_aligned_abbrev(const unsigned char *sha1, int len)
 {
 	int abblen;
 	const char *abbrev;

@@ -4188,9 +4184,9 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)
 	fprintf(opt->file, "%s", diff_line_prefix(opt));
 	if (!(opt->output_format & DIFF_FORMAT_NAME_STATUS)) {
 		fprintf(opt->file, ":%06o %06o %s ", p->one->mode, p->two->mode,
-			diff_unique_abbrev(p->one->oid.hash, opt->abbrev));
+			diff_aligned_abbrev(p->one->oid.hash, opt->abbrev));
 		fprintf(opt->file, "%s ",
-			diff_unique_abbrev(p->two->oid.hash, opt->abbrev));
+			diff_aligned_abbrev(p->two->oid.hash, opt->abbrev));
 	}
 	if (p->score) {
 		fprintf(opt->file, "%c%03d%c", p->status, similarity_index(p),

diff --git a/diff.h b/diff.h
index 25ae60d..f2b04b6 100644
--- a/diff.h
+++ b/diff.h

@@ -340,7 +340,11 @@ extern void diff_warn_rename_limit(const char *varname, int needed, int degraded
 #define DIFF_STATUS_FILTER_AON		'*'
 #define DIFF_STATUS_FILTER_BROKEN	'B'
 
-extern const char *diff_unique_abbrev(const unsigned char *, int);
+/*
+ * This is different from find_unique_abbrev() in that
+ * it stuffs the result with dots for alignment.
+ */
+extern const char *diff_aligned_abbrev(const unsigned char *sha1, int);
 
 /* do not report anything on removed paths */
 #define DIFF_SILENT_ON_REMOVED 01

-- 
2.10.1.619.g16351a7

Since we're modifying this function anyway, it's a good time
to update it to the more modern "struct oid". We can also
drop some of the magic numbers in favor of GIT_SHA1_HEXSZ,
along with some descriptive comments.

Signed-off-by: Jeff King <redacted>
---
 combine-diff.c |  4 ++--
 diff.c         | 20 +++++++++++---------
 diff.h         |  2 +-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index b36c2d1..59501db 100644
--- a/combine-diff.c
+++ b/combine-diff.c

@@ -1203,9 +1203,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 
 		/* Show sha1's */
 		for (i = 0; i < num_parent; i++)
-			printf(" %s", diff_aligned_abbrev(p->parent[i].oid.hash,
+			printf(" %s", diff_aligned_abbrev(&p->parent[i].oid,
 							  opt->abbrev));
-		printf(" %s ", diff_aligned_abbrev(p->oid.hash, opt->abbrev));
+		printf(" %s ", diff_aligned_abbrev(&p->oid, opt->abbrev));
 	}
 
 	if (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {

diff --git a/diff.c b/diff.c
index 2d8b74b..8f0f309 100644
--- a/diff.c
+++ b/diff.c

@@ -4136,14 +4136,15 @@ void diff_free_filepair(struct diff_filepair *p)
 	free(p);
 }
 
-const char *diff_aligned_abbrev(const unsigned char *sha1, int len)
+const char *diff_aligned_abbrev(const struct object_id *oid, int len)
 {
 	int abblen;
 	const char *abbrev;
-	if (len == 40)
-		return sha1_to_hex(sha1);
 
-	abbrev = find_unique_abbrev(sha1, len);
+	if (len == GIT_SHA1_HEXSZ)
+		return oid_to_hex(oid);
+
+	abbrev = find_unique_abbrev(oid->hash, len);
 	abblen = strlen(abbrev);
 
 	/*

@@ -4165,15 +4166,16 @@ const char *diff_aligned_abbrev(const unsigned char *sha1, int len)
 	 * the automatic sizing is supposed to give abblen that ensures
 	 * uniqueness across all objects (statistically speaking).
 	 */
-	if (abblen < 37) {
-		static char hex[41];
+	if (abblen < GIT_SHA1_HEXSZ - 3) {
+		static char hex[GIT_SHA1_HEXSZ + 1];
 		if (len < abblen && abblen <= len + 2)
 			xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");
 		else
 			xsnprintf(hex, sizeof(hex), "%s...", abbrev);
 		return hex;
 	}
-	return sha1_to_hex(sha1);
+
+	return oid_to_hex(oid);
 }
 
 static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)

@@ -4184,9 +4186,9 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)
 	fprintf(opt->file, "%s", diff_line_prefix(opt));
 	if (!(opt->output_format & DIFF_FORMAT_NAME_STATUS)) {
 		fprintf(opt->file, ":%06o %06o %s ", p->one->mode, p->two->mode,
-			diff_aligned_abbrev(p->one->oid.hash, opt->abbrev));
+			diff_aligned_abbrev(&p->one->oid, opt->abbrev));
 		fprintf(opt->file, "%s ",
-			diff_aligned_abbrev(p->two->oid.hash, opt->abbrev));
+			diff_aligned_abbrev(&p->two->oid, opt->abbrev));
 	}
 	if (p->score) {
 		fprintf(opt->file, "%c%03d%c", p->status, similarity_index(p),

diff --git a/diff.h b/diff.h
index f2b04b6..01afc70 100644
--- a/diff.h
+++ b/diff.h

@@ -344,7 +344,7 @@ extern void diff_warn_rename_limit(const char *varname, int needed, int degraded
  * This is different from find_unique_abbrev() in that
  * it stuffs the result with dots for alignment.
  */
-extern const char *diff_aligned_abbrev(const unsigned char *sha1, int);
+extern const char *diff_aligned_abbrev(const struct object_id *sha1, int);
 
 /* do not report anything on removed paths */
 #define DIFF_SILENT_ON_REMOVED 01

-- 
2.10.1.619.g16351a7

When generating diffs outside a repository (e.g., with "diff
--no-index"), we may write abbreviated sha1s as part of
"--raw" output or the "index" lines of "--patch" output.
Since we have no object database, we never find any
collisions, and these sha1s get whatever static abbreviation
length is configured (typically 7).

However, we do blindly look in ".git/objects" to see if any
objects exist, even though we know we are not in a
repository. This is usually harmless because such a
directory is unlikely to exist, but could be wrong in rare
circumstances.

Let's instead notice when we are not in a repository and
behave as if the object database is empty (i.e., just use
the default abbrev length). It would perhaps make sense to
be conservative and show full sha1s in that case, but
showing the default abbreviation is what we've always done
(and is certainly less ugly).

Note that this does mean that:

  cd /not/a/repo
  GIT_OBJECT_DIRECTORY=/some/real/objdir git diff --no-index ...

used to look for collisions in /some/real/objdir but now
does not. This could be considered either a bugfix (we do
not look at objects if we have no repository) or a
regression, but it seems unlikely that anybody would care
much either way.

Signed-off-by: Jeff King <redacted>
---
 diff.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 8f0f309..ef11001 100644
--- a/diff.c
+++ b/diff.c

@@ -3049,6 +3049,19 @@ static int similarity_index(struct diff_filepair *p)
 	return p->score * 100 / MAX_SCORE;
 }
 
+static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
+{
+	if (startup_info->have_repository)
+		return find_unique_abbrev(oid->hash, abbrev);
+	else {
+		char *hex = oid_to_hex(oid);
+		if (abbrev < 0 || abbrev > GIT_SHA1_HEXSZ)
+			die("BUG: oid abbreviation out of range: %d", abbrev);
+		hex[abbrev] = '\0';
+		return hex;
+	}
+}
+
 static void fill_metainfo(struct strbuf *msg,
 			  const char *name,
 			  const char *other,

@@ -3107,9 +3120,9 @@ static void fill_metainfo(struct strbuf *msg,
 			    (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
 				abbrev = 40;
 		}
-		strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
-			    find_unique_abbrev(one->oid.hash, abbrev));
-		strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
+		strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set,
+			    diff_abbrev_oid(&one->oid, abbrev),
+			    diff_abbrev_oid(&two->oid, abbrev));
 		if (one->mode == two->mode)
 			strbuf_addf(msg, " %06o", one->mode);
 		strbuf_addf(msg, "%s\n", reset);

@@ -4144,7 +4157,7 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len)
 	if (len == GIT_SHA1_HEXSZ)
 		return oid_to_hex(oid);
 
-	abbrev = find_unique_abbrev(oid->hash, len);
+	abbrev = diff_abbrev_oid(oid, len);
 	abblen = strlen(abbrev);
 
 	/*

-- 
2.10.1.619.g16351a7

When we default to ".git" without having done any kind of
repository setup, the results quite often do what the user
expects.  But this has also historically been the cause of
some poorly behaved corner cases. These cases can be hard to
find, because they happen at the conjunction of two
relatively rare circumstances:

  1. We are running some code which assumes there's a
     repository present, but there isn't necessarily one
     (e.g., low-level diff code triggered by "git diff
     --no-index" might try to look at some repository data).

  2. We have an unusual setup, like being in a subdirectory
     of the working tree, or we have a .git file (rather
     than a directory), or we are running a tool like "init"
     or "clone" which may operate on a repository in a
     different directory.

Our test scripts often cover (1), but miss doing (2) at the
same time, and so the fallback appears to work but has
lurking bugs. We can flush these bugs out by refusing to do
the fallback entirely., This makes potential problems a lot
more obvious by complaining even for "usual" setups.

This passes the test suite (after the adjustments in the
previous patches), but there's a risk of regression for any
cases where the fallback usually works fine but the code
isn't exercised by the test suite.  So by itself, this
commit is a potential step backward, but lets us take two
steps forward once we've identified and fixed any such
instances.

Signed-off-by: Jeff King <redacted>
---
 environment.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/environment.c b/environment.c
index cd5aa57..b1743e6 100644
--- a/environment.c
+++ b/environment.c

@@ -164,8 +164,11 @@ static void setup_git_env(void)
 	const char *replace_ref_base;
 
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
-	if (!git_dir)
+	if (!git_dir) {
+		if (!startup_info->have_repository)
+			die("BUG: setup_git_env called without repository");
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	}
 	gitfile = read_gitfile(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
 	if (get_common_dir(&sb, git_dir))

-- 
2.10.1.619.g16351a7

On Thu, Oct 20, 2016 at 02:21:25AM -0400, Jeff King wrote:

Note that this function has a semantic (but not textual) conflict with
lt/auto-abbrev in 'next', as it sets DEFAULT_ABBREV to -1.

The resolution is:

diff --git a/diff.c b/diff.c
index cab811e..4c09314 100644
--- a/diff.c
+++ b/diff.c

@@ -3102,7 +3102,9 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
 		return find_unique_abbrev(oid->hash, abbrev);
 	else {
 		char *hex = oid_to_hex(oid);
-		if (abbrev < 0 || abbrev > GIT_SHA1_HEXSZ)
+		if (abbrev < 0)
+			abbrev = FALLBACK_DEFAULT_ABBREV;
+		if (abbrev > GIT_SHA1_HEXSZ)
 			die("BUG: oid abbreviation out of range: %d", abbrev);
 		hex[abbrev] = '\0';
 		return hex;

This logic could be pushed down into the find_unique_abbrev() code
(where it _would_ just cause a textual conflict). I preferred to keep it
up here because other callers could conceivably want to handle the
non-repo case in some different way (e.g., by not abbreviating at all).

-Peff

On Thu, Oct 20, 2016 at 1:16 PM, Jeff King [off-list ref] wrote:

The low-level attribute and gitignore code will try to look
in $GIT_DIR/info for any repo-level configuration files,
even if we have not actually determined that we are in a
repository (e.g., running "git grep --no-index"). In such a
case they end up looking for ".git/info/attributes", etc.

This is generally harmless, as such a file is unlikely to
exist outside of a repository, but it's still conceptually
the wrong thing to do.

Let's detect this situation explicitly and skip reading the
file (i.e., the same behavior we'd get if we were in a
repository and the file did not exist).

On the other hand, if we invoke attr machinery too early by mistake,
before setup_git_directory* is called, then we skip
.git/info/attributes file as well even though I think we should shout
"call setup_git_directory first!" so the developer can fix it.

I wonder if we should have two flags in startup_info to say "yes
setup_git_dir... has been called, you can trust
startup_info->have_repository" and "yes, i do not call setup_git_dir
on purpose, quit complaining" then we could still catch unintended
.git/info/attributes ignore while letting git grep --no-index work
correctly.
-- 
Duy

On Thu, Oct 20, 2016 at 1:24 PM, Jeff King [off-list ref] wrote:

YES!!! Thank you for finally fixing this.

The "once we've identified" part could be tricky though. This message
alone will not give us any clue where it's called since it's buried
deep in git_path() usually, which is buried deep elsewhere. Without
falling back to core dumps (with debug info), glibc's backtrace
(platform specifc), the best we could do is turn git_path() into a
macro that takes __FILE__ and __LINE__ and somehow pass the info down
here, but "..." in macros is C99 specific, sigh..

Is it too bad to turn git_path() into a macro when we know the
compiler is C99 ? Older compilers will have no source location info in
git_path(), Hopefully they are rare, which means chances of this fault
popping up are also reduced.
-- 
Duy

On Tue, Oct 25, 2016 at 07:24:50PM +0700, Duy Nguyen wrote:

quoted

Let's detect this situation explicitly and skip reading the
file (i.e., the same behavior we'd get if we were in a
repository and the file did not exist).

On the other hand, if we invoke attr machinery too early by mistake,
before setup_git_directory* is called, then we skip
.git/info/attributes file as well even though I think we should shout
"call setup_git_directory first!" so the developer can fix it.

I wonder if we should have two flags in startup_info to say "yes
setup_git_dir... has been called, you can trust
startup_info->have_repository" and "yes, i do not call setup_git_dir
on purpose, quit complaining" then we could still catch unintended
.git/info/attributes ignore while letting git grep --no-index work
correctly.

Yeah, it would be nice for the low-level code to be able to detect such
errors. I don't mind if you want to extend startup_info in that way, but
it will probably introduce a period of instability and regressions
(sites that are perfectly fine, but forgot to set the "I know what I'm
doing" flag).

-Peff

On Tue, Oct 25, 2016 at 07:38:30PM +0700, Duy Nguyen wrote:

quoted

diff --git a/environment.c b/environment.c
index cd5aa57..b1743e6 100644
--- a/environment.c
+++ b/environment.c

@@ -164,8 +164,11 @@ static void setup_git_env(void)
        const char *replace_ref_base;

        git_dir = getenv(GIT_DIR_ENVIRONMENT);
-       if (!git_dir)
+       if (!git_dir) {
+               if (!startup_info->have_repository)
+                       die("BUG: setup_git_env called without repository");

YES!!! Thank you for finally fixing this.

Good, I'm glad somebody besides me is excited about this. I've been
wanting to write this patch for a long time, but it took years of
chipping away at all the edge cases.

The "once we've identified" part could be tricky though. This message
alone will not give us any clue where it's called since it's buried
deep in git_path() usually, which is buried deep elsewhere. Without
falling back to core dumps (with debug info), glibc's backtrace
(platform specifc), the best we could do is turn git_path() into a
macro that takes __FILE__ and __LINE__ and somehow pass the info down
here, but "..." in macros is C99 specific, sigh..

Is it too bad to turn git_path() into a macro when we know the
compiler is C99 ? Older compilers will have no source location info in
git_path(), Hopefully they are rare, which means chances of this fault
popping up are also reduced.

I think you could conditionally make git_path() and all of its
counterparts macros, similar to the way the trace code works. It seems
like a pretty maintenance-heavy solution, though. I'd prefer
conditionally compiling backtrace(); that also doesn't hit 100% of
cases, but at least it isn't too invasive.

But I think I still prefer just letting the corefile and the debugger do
their job. This error shouldn't happen much, and when it does, it should
be easily reproducible. Getting the bug reporter to give either a
reproduction recipe, or to run "gdb git" doesn't seem like that big a
hurdle.

For fun, here's a patch that uses backtrace(), but it does not actually
print the function names unless you compile with "-rdynamic" (and even
then it misses static functions). There are better libraries, but of
course that's one more thing for the user to deal with when building.

-Peff

---

diff --git a/usage.c b/usage.c
index 17f52c1b5c..4917c6bdfd 100644
--- a/usage.c
+++ b/usage.c

@@ -5,6 +5,9 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+#endif
 
 static FILE *error_handle;
 static int tweaked_error_buffering;

@@ -24,6 +27,32 @@ void vreportf(const char *prefix, const char *err, va_list params)
 	fputc('\n', fh);
 }
 
+#ifdef HAVE_BACKTRACE
+static void maybe_backtrace(void)
+{
+	void *bt[100];
+	char **symbols;
+	int nr;
+
+	if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+		return;
+
+	nr = backtrace(bt, ARRAY_SIZE(bt));
+	symbols = backtrace_symbols(bt, nr);
+	if (symbols) {
+		FILE *fh = error_handle ? error_handle : stderr;
+		int i;
+
+		fprintf(fh, "die() called from:\n");
+		for (i = 0; i < nr; i++)
+			fprintf(fh, "  %s\n", symbols[i]);
+		free(symbols);
+	}
+}
+#else
+#define maybe_backtrace()
+#endif
+
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
 	vreportf("usage: ", err, params);

@@ -33,6 +62,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 static NORETURN void die_builtin(const char *err, va_list params)
 {
 	vreportf("fatal: ", err, params);
+	maybe_backtrace();
 	exit(128);
 }
 

On Tue, Oct 25, 2016 at 11:15:25AM -0400, Jeff King wrote:

quoted

The "once we've identified" part could be tricky though. This message
alone will not give us any clue where it's called since it's buried
deep in git_path() usually, which is buried deep elsewhere. Without
falling back to core dumps (with debug info), glibc's backtrace
(platform specifc), the best we could do is turn git_path() into a
macro that takes __FILE__ and __LINE__ and somehow pass the info down
here, but "..." in macros is C99 specific, sigh..

Is it too bad to turn git_path() into a macro when we know the
compiler is C99 ? Older compilers will have no source location info in
git_path(), Hopefully they are rare, which means chances of this fault
popping up are also reduced.

I think you could conditionally make git_path() and all of its
counterparts macros, similar to the way the trace code works. It seems
like a pretty maintenance-heavy solution, though. I'd prefer
conditionally compiling backtrace(); that also doesn't hit 100% of
cases, but at least it isn't too invasive.

OK, a more polished patch is this. There are warnings about
-fomit-function-pointers in glibc man page, at least in my simple
tests it does not cause any issue.

But I think I still prefer just letting the corefile and the debugger do
their job. This error shouldn't happen much, and when it does, it should
be easily reproducible. Getting the bug reporter to give either a
reproduction recipe, or to run "gdb git" doesn't seem like that big a
hurdle.

There are other places where backtrace() support may come handy
too. If -rdynamic was not needed, I would push for this patch. Too bad
backtrace() is not a perfect magic wand.

-- 8< --

diff --git a/config.mak.uname b/config.mak.uname
index b232908..b38f62a 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	# for backtrace() support with glibc >= 2.1
+	BASIC_LDFLAGS += -rdynamic
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease

diff --git a/git-compat-util.h b/git-compat-util.h
index 43718da..3561ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -408,6 +408,7 @@ extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern NORETURN void BUG(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));

@@ -709,6 +710,7 @@ extern int git_vsnprintf(char *str, size_t maxsize,
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
+#define HAVE_BACKTRACE
 #endif
 #endif
 

@@ -722,6 +724,23 @@ static inline char *gitstrchrnul(const char *s, int c)
 }
 #endif
 
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+static inline void dump_backtrace(FILE *fp)
+{
+	void *buffer[32];
+	int nptrs;
+
+	nptrs = backtrace(buffer, sizeof(buffer) / sizeof(*buffer));
+	fflush(fp);
+	backtrace_symbols_fd(buffer, nptrs, fileno(fp));
+}
+#else
+static inline void dump_backtrace(FILE *fp)
+{
+}
+#endif
+
 #ifdef NO_INET_PTON
 int inet_pton(int af, const char *src, void *dst);
 #endif

diff --git a/usage.c b/usage.c
index 17f52c1..b00603c 100644
--- a/usage.c
+++ b/usage.c

@@ -204,3 +204,16 @@ void warning(const char *warn, ...)
 	warn_routine(warn, params);
 	va_end(params);
 }
+
+void NORETURN BUG(const char *fmt, ...)
+{
+	va_list params;
+
+	va_start(params, fmt);
+	vreportf("BUG: ", fmt, params);
+	va_end(params);
+
+	dump_backtrace(error_handle ? error_handle : stderr);
+
+	exit(128);
+}
-- 8< --
--

Duy

On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:

quoted

I think you could conditionally make git_path() and all of its
counterparts macros, similar to the way the trace code works. It seems
like a pretty maintenance-heavy solution, though. I'd prefer
conditionally compiling backtrace(); that also doesn't hit 100% of
cases, but at least it isn't too invasive.

OK, a more polished patch is this. There are warnings about
-fomit-function-pointers in glibc man page, at least in my simple
tests it does not cause any issue.

Yeah, I tried with -fno-omit-frame-pointer, but it didn't help. The
glibc backtrace(3) manpage specifically says:

  The symbol names may be unavailable without the use of special linker
  options. For systems using the GNU linker, it is necessary to use the
  -rdynamic linker option. Note that names of "static" functions are not
  exposed, and won't be available in the backtrace.

which matches the behavior I get.

Gcc ships with a libbacktrace which does seem to give reliable results
(patch below for reference). But that's still relying on gcc, and on
having debug symbols available. I'm not sure this is really any
convenience over dumping a corefile and using gdb to pull out the
symbols after the fact.

---

diff --git a/config.mak.uname b/config.mak.uname
index 76f885281c..62a14f10d3 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	BASIC_CFLAGS += -DHAVE_BACKTRACE
+	EXTLIBS += -lbacktrace
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease

diff --git a/usage.c b/usage.c
index 17f52c1b5c..4b9762ae62 100644
--- a/usage.c
+++ b/usage.c

@@ -5,6 +5,9 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include <backtrace.h>
+#endif
 
 static FILE *error_handle;
 static int tweaked_error_buffering;

@@ -24,6 +27,44 @@ void vreportf(const char *prefix, const char *err, va_list params)
 	fputc('\n', fh);
 }
 
+#ifdef HAVE_BACKTRACE
+static int full_callback(void *fh, uintptr_t pc,
+			 const char *filename, int lineno,
+			 const char *function)
+{
+	if (!function || !filename)
+		return 0;
+	fprintf(fh, "debug:  %s() at %s:%d\n", function, filename, lineno);
+	return 0;
+}
+
+static void error_callback(void *fh, const char *msg, int errnum)
+{
+	fprintf(fh, "backtrace error: %s", msg);
+	if (errnum > 0)
+		fprintf(fh, ": %s", strerror(errnum));
+	fputc('\n', fh);
+}
+
+static void maybe_backtrace(void)
+{
+	FILE *fh = error_handle ? error_handle : stderr;
+	struct backtrace_state *bt;
+
+	if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+		return;
+
+	/* XXX obviously unportable use of /proc */
+	bt = backtrace_create_state("/proc/self/exe", 0, error_callback, fh);
+	if (bt) {
+		fprintf(fh, "debug: die() called at:\n");
+		backtrace_full(bt, 3, full_callback, error_callback, fh);
+	}
+}
+#else
+#define maybe_backtrace()
+#endif
+
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
 	vreportf("usage: ", err, params);

@@ -33,6 +74,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 static NORETURN void die_builtin(const char *err, va_list params)
 {
 	vreportf("fatal: ", err, params);
+	maybe_backtrace();
 	exit(128);
 }
 

On Wed, Oct 26, 2016 at 7:10 PM, Jeff King [off-list ref] wrote:

On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:

quoted

quoted

I think you could conditionally make git_path() and all of its
counterparts macros, similar to the way the trace code works. It seems
like a pretty maintenance-heavy solution, though. I'd prefer
conditionally compiling backtrace(); that also doesn't hit 100% of
cases, but at least it isn't too invasive.

OK, a more polished patch is this. There are warnings about
-fomit-function-pointers in glibc man page, at least in my simple
tests it does not cause any issue.

Yeah, I tried with -fno-omit-frame-pointer, but it didn't help. The
glibc backtrace(3) manpage specifically says:

  The symbol names may be unavailable without the use of special linker
  options. For systems using the GNU linker, it is necessary to use the
  -rdynamic linker option. Note that names of "static" functions are not
  exposed, and won't be available in the backtrace.

which matches the behavior I get.

Gcc ships with a libbacktrace which does seem to give reliable results
(patch below for reference). But that's still relying on gcc, and on
having debug symbols available.

Yep. On an optimized build you can't get anywhere without debug info,
which has a giant database to describe "if your rip/pc register is
here, then you clue to find your caller is there" for basically every
instruction in your program. Dwarf3 at least is a crazy world.

I'm not sure this is really any convenience over dumping a corefile and using gdb to pull out the
symbols after the fact.

So are we back to forcing core files? I'm ok with that! The only
inconvenience I see is pointing out where the core file is, which
should be where `pwd` originally is. On linux we can even peek into
/proc/sys/kernel/core_pattern if we want to be precise. ulimit can get
in the way, but I don't if the default out there is enable or disable
core dumping. Once we got OS info and git version, our chances of
cracking the core files should be reasonably high.
-- 
Duy

On Wed, Oct 26, 2016 at 07:26:20PM +0700, Duy Nguyen wrote:

quoted

I'm not sure this is really any convenience over dumping a corefile
and using gdb to pull out the
symbols after the fact.

So are we back to forcing core files? I'm ok with that! The only
inconvenience I see is pointing out where the core file is, which
should be where `pwd` originally is. On linux we can even peek into
/proc/sys/kernel/core_pattern if we want to be precise. ulimit can get
in the way, but I don't if the default out there is enable or disable
core dumping. Once we got OS info and git version, our chances of
cracking the core files should be reasonably high.

TBH, most of the time I expect the solution to be walking the person
through:

  git clone git://kernel.org/pub/scm/git/git.git
  cd git
  make

  gdb --args ./git whatever
  break die
  run
  bt

which would cover most cases (reproducible breakage, and not in a
sub-program). It's relatively rare that even I resort to corefiles.

-Peff

Hi,

Jeff King wrote:

This trips reproducibly for

  git ls-remote https://kernel.googlesource.com/pub/scm/git/git

when run outside of a git repository.

Backtrace:

  #0  setup_git_env () at environment.c:172
  #1  get_git_dir () at environment.c:214
  #2  get_helper at transport-helper.c:127
  #3  get_refs_list (for_push=0) at transport-helper.c:1038
  #4  transport_get_remote_refs at transport.c:1076
  #5  cmd_ls_remote at builtin/ls-remote.c:97

transport-helper.c:127 is

	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
			 get_git_dir());

That code is pretty clearly wrong.  Should it be made conditional
on have_git_dir(), like this?

Thanks,
Jonathan

 transport-helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 91aed35..e4fd982 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
 
-	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
-			 get_git_dir());
+	if (have_git_dir())
+		argv_array_pushf(&helper->env_array, "%s=%s",
+				 GIT_DIR_ENVIRONMENT, get_git_dir());
 
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)

-- 

On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:

quoted

 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
-	if (!git_dir)
+	if (!git_dir) {
+		if (!startup_info->have_repository)
+			die("BUG: setup_git_env called without repository");
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	}

This trips reproducibly for

  git ls-remote https://kernel.googlesource.com/pub/scm/git/git

when run outside of a git repository.

Backtrace:

  #0  setup_git_env () at environment.c:172
  #1  get_git_dir () at environment.c:214
  #2  get_helper at transport-helper.c:127
  #3  get_refs_list (for_push=0) at transport-helper.c:1038
  #4  transport_get_remote_refs at transport.c:1076
  #5  cmd_ls_remote at builtin/ls-remote.c:97

transport-helper.c:127 is

	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
			 get_git_dir());

That code is pretty clearly wrong.  Should it be made conditional
on have_git_dir(), like this?

Yeah, I think making it conditional makes the most sense. Just trying to
think of cases that might not be covered by your patch:

  - if we are not in a repository, we shouldn't ever need to override an
    existing $GIT_DIR from the environment. Because if $GIT_DIR is
    present, then we _would_ be in a repo if it's valid, and die if it
    isn't.

  - not setting $GIT_DIR may cause a helper to do the usual discovery
    walk to find the repository. But we know we're not in one, or we
    would have found it ourselves. So worst case it may expend a little
    effort to try to find a repository and fail (I think remote-curl
    would do this to try to find repo-level config).

Both of those points assume that we've already called
setup_git_directory_gently(), but I think that's a reasonable
assumption. And certainly is true for ls-remote, and should be for any
git command that invokes the transport code.

So yeah, I think this is the extent of the change needed.

Thanks.

-Peff

To push from or fetch to the current repository, remote helpers need
to know what repository that is.  Accordingly, Git sets the GIT_DIR
environment variable to the path to the current repository when
invoking remote helpers.

There is a special case it does not handle: "git ls-remote" and "git
archive --remote" can be run to inspect a remote repository without
being run from any local repository.  GIT_DIR is not useful in this
scenario:

- if we are not in a repository, we don't need to set GIT_DIR to
  override an existing GIT_DIR value from the environment.  If GIT_DIR
  is present then we would be in a repository if it were valid and
  would have called die() if it weren't.

- not setting GIT_DIR may cause a helper to do the usual discovery
  walk to find the repository.  But we know we're not in one, or we
  would have found it ourselves.  So in the worst case it may expend
  a little extra effort to try to find a repository and fail (for
  example, remote-curl would do this to try to find repository-level
  configuration).

So leave GIT_DIR unset in this case.  This makes GIT_DIR easier to
understand for remote helper authors and makes transport code less of
a special case for repository discovery.

Noticed using b1ef400e (setup_git_env: avoid blind fall-back to
".git", 2016-10-20) from 'next':

 $ cd /tmp
 $ git ls-remote https://kernel.googlesource.com/pub/scm/git/git
 fatal: BUG: setup_git_env called without repository

While at it, add some tests for other variables in the environment of
commands run by git-remote-ext.

Helped-by: Jeff King [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:

So yeah, I think this is the extent of the change needed.

Thanks.  Here's the patch again, now with commit messages and a test.
Thanks for the analysis and sorry for the slow turnaround.

Sincerely,
Jonathan

 t/t5550-http-fetch-dumb.sh  | 15 ++++++++
 t/t5551-http-fetch-smart.sh | 15 ++++++++
 t/t5570-git-daemon.sh       | 15 ++++++++
 t/t5802-connect-helper.sh   | 84 ++++++++++++++++++++++++++++++++++++++++++++-
 transport-helper.c          |  5 +--
 5 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index aeb3a63f7c..a86fca3e6c 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh

@@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '
 	test_cmp file clone/file
 '
 
+test_expect_success 'list refs from outside any repository' '
+	cat >expect <<-EOF &&
+	$(git rev-parse master)	HEAD
+	$(git rev-parse master)	refs/heads/master
+	EOF
+	mkdir lsremote-root &&
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd lsremote-root &&
+		git ls-remote "$HTTPD_URL/dumb/repo.git" >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6e5b9e42fb..7ba894f0f4 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh

@@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '
 	expect_askpass both user@host auth/smart/repo.git
 '
 
+test_expect_success 'list refs from outside any repository' '
+	cat >expect <<-EOF &&
+	$(git rev-parse master)	HEAD
+	$(git rev-parse master)	refs/heads/master
+	EOF
+	mkdir lsremote-root &&
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd lsremote-root &&
+		git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'disable dumb http on server' '
 	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
 		config http.getanyfile false

diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..4573d98e6c 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh

@@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '
 	test_cmp file clone/file
 '
 
+test_expect_success 'list refs from outside any repository' '
+	cat >expect <<-EOF &&
+	$(git rev-parse master)	HEAD
+	$(git rev-parse master)	refs/heads/master
+	EOF
+	mkdir lsremote-root &&
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd lsremote-root &&
+		git ls-remote "$GIT_DAEMON_URL/repo.git" >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'fetch changes via git protocol' '
 	echo content >>file &&
 	git commit -a -m two &&

diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index c6c2661878..7232032cd2 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh

@@ -3,6 +3,10 @@
 test_description='ext::cmd remote "connect" helper'
 . ./test-lib.sh
 
+escape_spaces () {
+	echo "$*" | sed -e "s/ /% /g"
+}
+
 test_expect_success setup '
 	git config --global protocol.ext.allow user &&
 	test_tick &&

@@ -19,7 +23,7 @@ test_expect_success setup '
 '
 
 test_expect_success clone '
-	cmd=$(echo "echo >&2 ext::sh invoked && %S .." | sed -e "s/ /% /g") &&
+	cmd=$(escape_spaces "echo >&2 ext::sh invoked && %S ..") &&
 	git clone "ext::sh -c %S% ." dst &&
 	git for-each-ref refs/heads/ refs/tags/ >expect &&
 	(

@@ -70,6 +74,84 @@ test_expect_success 'update backfilled tag without primary transfer' '
 	test_cmp expect actual
 '
 
+test_expect_success 'GIT_EXT_SERVICE for clone, ls-remote, push, archive' '
+	rm -rf dst &&
+	>actual &&
+	cat >expect <<-\EOF &&
+	git-upload-pack
+	git-upload-pack
+	git-receive-pack
+	git-upload-archive
+	EOF
+	git archive HEAD >expect.tar &&
+	cmd=$(escape_spaces "echo >>actual \$GIT_EXT_SERVICE && %S .") &&
+
+	git clone "ext::sh -c $cmd" dst &&
+	git ls-remote "ext::sh -c $cmd" &&
+	test_when_finished "git update-ref -d refs/heads/newbranch" &&
+	git push "ext::sh -c $cmd" master:newbranch &&
+	git archive --remote="ext::sh -c $cmd" HEAD >actual.tar &&
+
+	test_cmp expect actual &&
+	test_cmp expect.tar actual.tar
+'
+
+test_expect_success 'GIT_EXT_SERVICE_NOPREFIX for clone, ls-remote, push, archive' '
+	rm -rf dst &&
+	>actual &&
+	cat >expect <<-\EOF &&
+	upload-pack
+	upload-pack
+	receive-pack
+	upload-archive
+	EOF
+	git archive HEAD >expect.tar &&
+	cmd=$(escape_spaces "echo >>actual \$GIT_EXT_SERVICE_NOPREFIX && %S .") &&
+
+	git clone "ext::sh -c $cmd" dst &&
+	git ls-remote "ext::sh -c $cmd" &&
+	test_when_finished "git update-ref -d refs/heads/newbranch" &&
+	git push "ext::sh -c $cmd" master:newbranch &&
+	git archive --remote="ext::sh -c $cmd" HEAD >actual.tar &&
+
+	test_cmp expect actual &&
+	test_cmp expect.tar actual.tar
+'
+
+test_expect_success 'GIT_DIR is set to the enclosing repo for ls-remote' '
+	git rev-parse --git-dir >expect &&
+	cmd=$(escape_spaces "echo \$GIT_DIR >actual && %S .") &&
+	git ls-remote "ext::sh -c $cmd" &&
+	test_cmp expect actual
+'
+
+test_expect_success 'GIT_DIR is set to the enclosing repo for archive' '
+	git rev-parse --git-dir >expect &&
+	git archive HEAD >expect.tar &&
+	cmd=$(escape_spaces "echo \$GIT_DIR >actual && %S .") &&
+	git archive --remote="ext::sh -c $cmd" HEAD >actual.tar &&
+	test_cmp expect actual &&
+	test_cmp expect.tar actual.tar
+'
+
+test_expect_success 'GIT_DIR is not set if there is no enclosing repo' '
+	rm -rf subdir &&
+	>actual &&
+	printf "%s\n" unset unset >expect &&
+	git archive HEAD >expect.tar &&
+
+	mkdir subdir &&
+	cmd=$(escape_spaces "echo \${GIT_DIR-unset} >>../actual && %S ..") &&
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd subdir &&
+		git ls-remote "ext::sh -c $cmd" &&
+		git archive --remote="ext::sh -c $cmd" HEAD >../actual.tar
+	) &&
+	test_cmp expect actual &&
+	test_cmp expect.tar actual.tar
+'
 
 test_expect_success 'set up fake git-daemon' '
 	mkdir remote &&

diff --git a/transport-helper.c b/transport-helper.c
index 91aed35ebb..e4fd982383 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
 
-	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
-			 get_git_dir());
+	if (have_git_dir())
+		argv_array_pushf(&helper->env_array, "%s=%s",
+				 GIT_DIR_ENVIRONMENT, get_git_dir());
 
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)

-- 
2.11.0.355.g1ede815

+       mkdir lsremote-root &&
+       (
+               GIT_CEILING_DIRECTORIES=$(pwd) &&
+               export GIT_CEILING_DIRECTORIES &&
+               cd lsremote-root &&
+               git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
+       ) &&

We could avoid the subshell via

GIT_CEILING_DIRECTORIES=$(pwd) \
    git -C lsremote-root lsremote ... >actual

Not sure if it is worth to trade off a block of code (and an extra shell
at run time) for an overly long line.

The rest looks good to me.

Thanks,
Stefan

On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:

Thanks.  Here's the patch again, now with commit messages and a test.
Thanks for the analysis and sorry for the slow turnaround.

Thanks for following up. While working on a similar one recently, I had
the nagging feeling that there was a case we had found that was still to
be dealt with, and I think this was it. :)

The patch to the C code looks good. I have a few comments on the tests:

Since my recent de95302a4c (t5000: extract nongit function to
test-lib-functions.sh, 2016-12-15), this can be shortened to:

  cat >expect <<-EOF &&
  ...
  EOF
  nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
  test_cmp expect actual

I think that commit is in 'master' now.

Without my patch to die() in setup_git_env(), I think this would pass
with or without your patch, right?  I think the current behavior _is_
buggy, but a setup to show off the improvement would be so arcane that I
don't think it is worth it. E.g., something like:

  echo '[http]extraHeader = "Foo: Bar" >nongit/.git/config

would probably trigger the use of that config when it shouldn't. But
that's really stretching.

Is this really testing anything that the dumb one isn't? The interesting
bit is in invoking git-remote-http, not what it does under the hood. I
don't mind too much being thorough, but if we are going to go that route
we should probably come up with a standard battery of tests for dumb
and smart HTTP, and then invoke them for each case without having to
write each one twice.

This one isn't actually broken now, right? The test is just there to
catch future regressions?

If we are being thorough, then would we also care about file-local
repos, git-over-ssh, etc?

This one is quite a big addition. I know this falls under the "while
we're at it" line at the end of your commit message, but maybe it's
worth pulling the GIT_EXT_SERVICE bits out into their own patch (and
explaining briefly what's going on; I had to go look up what
GIT_EXT_SERVICE_NOPREFIX even was).

-Peff

On Thu, Dec 29, 2016 at 04:37:30PM -0800, Stefan Beller wrote:

quoted

+       mkdir lsremote-root &&
+       (
+               GIT_CEILING_DIRECTORIES=$(pwd) &&
+               export GIT_CEILING_DIRECTORIES &&
+               cd lsremote-root &&
+               git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
+       ) &&

We could avoid the subshell via

GIT_CEILING_DIRECTORIES=$(pwd) \
    git -C lsremote-root lsremote ... >actual

Not sure if it is worth to trade off a block of code (and an extra shell
at run time) for an overly long line.

The rest looks good to me.

I mentioned elsewhere that we now have a "nongit" function to do this as
a one-liner. It might be worth applying your optimization to that
function, so it would take effect in may places.

-Peff

On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:

On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:

quoted

Thanks.  Here's the patch again, now with commit messages and a test.
Thanks for the analysis and sorry for the slow turnaround.

Thanks for following up. While working on a similar one recently, I had
the nagging feeling that there was a case we had found that was still to
be dealt with, and I think this was it. :)

The patch to the C code looks good. I have a few comments on the tests:

I happened to run into this problem myself today, so I thought I'd prod
you. I think your patch looks good. Hopefully I didn't scare you off
with my comments. :)

-Peff