Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Junio C Hamano]

Jonathan Nieder  writes:

> 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(>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?

Looks good and I agree with Peff's analysis.  Care to wrap it in a
patch with a log message?

Thanks.

>
> 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(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> -  get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(>env_array, "%s=%s",
> +  GIT_DIR_ENVIRONMENT, get_git_dir());
>  
>   code = start_command(helper);
>   if (code < 0 && errno == ENOENT)

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jeff King]

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

> > 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(>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.

> 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(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> -  get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(>env_array, "%s=%s",
> +  GIT_DIR_ENVIRONMENT, get_git_dir());

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

Thanks.

-Peff

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jonathan Nieder]

Hi,

Jeff King wrote:

> 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 
> ---
>  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;
> + }

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(>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(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
-get_git_dir());
+   if (have_git_dir())
+   argv_array_pushf(>env_array, "%s=%s",
+GIT_DIR_ENVIRONMENT, get_git_dir());
 
code = start_command(helper);
if (code < 0 && errno == ENOENT)
-- 

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jeff King]

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

> > 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

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Duy Nguyen]

On Wed, Oct 26, 2016 at 7:10 PM, Jeff King  wrote:
> On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:
>
>> > 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

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jeff King]

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

> > 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 
+#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);
 }
 

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Duy Nguyen]

On Tue, Oct 25, 2016 at 11:15:25AM -0400, Jeff King wrote:
> > 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 
+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

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jeff King]

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

> > 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 
+#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);
 }
 

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Duy Nguyen]

On Thu, Oct 20, 2016 at 1:24 PM, Jeff King  wrote:
> 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 
> ---
>  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");

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

[PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jeff King]

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 
---
 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(, git_dir))
-- 
2.10.1.619.g16351a7