patchSimplify handling of setup_git_directory_gently() failure cases.

> setup_git_directory_gently() expects two types of failures to
> discover a git directory (e.g. .git/):
> 
>   - GIT_DIR_HIT_CEILING: could not find a git directory in any
> 	parent directories of the cwd.
>   - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in
> 	any parent directories up to the mount point of the cwd.
> 
> Both cases are handled in a similar way, but there are misleading
> and unimportant differences. In both cases, setup_git_directory_gently()
> should:
> 
>   - Die if we are not in a git repository. Otherwise:
>   - Set nongit_ok = 1, indicating that we are not in a git repository
> 	but this is ok.
>   - Call strbuf_release() on any non-static struct strbufs that we
> 	allocated.
> 
> Before this change are two misleading additional behaviors:
> 
>   - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> 	apparent reason. We never had the chance to change directories
> 	up to this point so chdir(current cwd) is pointless.
>   - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> 	of a static struct strbuf (cwd). This is unnecessary because the
> 	struct is static so its buffer is always reachable. This is also
> 	misleading because nowhere else in the function is this buffer
> 	released.
> 
> This change eliminates these two misleading additional behaviors and
> deletes setup_nogit() because the code is clearer without it. The
> result is that we can see clearly that GIT_DIR_HIT_CEILING and
> GIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the
> different help messages).
> 
> Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Erin Dahlgren <eedahlgren@gmail.com>

> diff --git a/setup.c b/setup.c
> index 1be5037..b441e39 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
>  	return NULL;
>  }
>  
> -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> -{
> -	if (!nongit_ok)
> -		die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> -	if (chdir(cwd))
> -		die_errno(_("cannot come back to cwd"));
> -	*nongit_ok = 1;
> -	return NULL;
> -}
> -
>  static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
>  {
>  	struct stat buf;
> @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
>  		break;
>  	case GIT_DIR_HIT_CEILING:
> -		prefix = setup_nongit(cwd.buf, nongit_ok);
> -		break;
> +		if (!nongit_ok)
> +			die(_("not a git repository (or any of the parent directories): %s"),
> +					DEFAULT_GIT_DIR_ENVIRONMENT);
> +		*nongit_ok = 1;
> +		strbuf_release(&dir);
> +		return NULL;
>  	case GIT_DIR_HIT_MOUNT_POINT:
> -		if (nongit_ok) {
> -			*nongit_ok = 1;
> -			strbuf_release(&cwd);
> -			strbuf_release(&dir);
> -			return NULL;
> -		}
> -		die(_("not a git repository (or any parent up to mount point %s)\n"
> -		      "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> -		    dir.buf);
> +		if (!nongit_ok)
> +			die(_("not a git repository (or any parent up to mount point %s)\n"
> +						"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> +					dir.buf);

> +		*nongit_ok = 1;
> +		strbuf_release(&dir);
> +		return NULL;
>  	default:
>  		BUG("unhandled setup_git_directory_1() result");
>  	}
> -- 
> 2.7.4
> 
> 

>
> Hi Erin,
>
> On Thu, 13 Dec 2018, Erin Dahlgren wrote:
>
> > setup_git_directory_gently() expects two types of failures to
> > discover a git directory (e.g. .git/):
> >
> >   - GIT_DIR_HIT_CEILING: could not find a git directory in any
> >       parent directories of the cwd.
> >   - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in
> >       any parent directories up to the mount point of the cwd.
> >
> > Both cases are handled in a similar way, but there are misleading
> > and unimportant differences. In both cases, setup_git_directory_gently()
> > should:
> >
> >   - Die if we are not in a git repository. Otherwise:
> >   - Set nongit_ok = 1, indicating that we are not in a git repository
> >       but this is ok.
> >   - Call strbuf_release() on any non-static struct strbufs that we
> >       allocated.
> >
> > Before this change are two misleading additional behaviors:
> >
> >   - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> >       apparent reason. We never had the chance to change directories
> >       up to this point so chdir(current cwd) is pointless.
> >   - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> >       of a static struct strbuf (cwd). This is unnecessary because the
> >       struct is static so its buffer is always reachable. This is also
> >       misleading because nowhere else in the function is this buffer
> >       released.
> >
> > This change eliminates these two misleading additional behaviors and
> > deletes setup_nogit() because the code is clearer without it. The
> > result is that we can see clearly that GIT_DIR_HIT_CEILING and
> > GIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the
> > different help messages).
> >
> > Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Signed-off-by: Erin Dahlgren <eedahlgren@gmail.com>
>
> Thank you for working on this!
>
> > ---
> >  setup.c | 34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
>
> Nice! It's always good to see a code reduction with such a cleanup.
>
> Just one thing:
>
> > diff --git a/setup.c b/setup.c
> > index 1be5037..b441e39 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
> >       return NULL;
> >  }
> >
> > -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> > -{
> > -     if (!nongit_ok)
> > -             die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> > -     if (chdir(cwd))
> > -             die_errno(_("cannot come back to cwd"));
> > -     *nongit_ok = 1;
> > -     return NULL;
> > -}
> > -
> >  static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
> >  {
> >       struct stat buf;
> > @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >               prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
> >               break;
> >       case GIT_DIR_HIT_CEILING:
> > -             prefix = setup_nongit(cwd.buf, nongit_ok);
> > -             break;
> > +             if (!nongit_ok)
> > +                     die(_("not a git repository (or any of the parent directories): %s"),
> > +                                     DEFAULT_GIT_DIR_ENVIRONMENT);
> > +             *nongit_ok = 1;
> > +             strbuf_release(&dir);
> > +             return NULL;
> >       case GIT_DIR_HIT_MOUNT_POINT:
> > -             if (nongit_ok) {
> > -                     *nongit_ok = 1;
> > -                     strbuf_release(&cwd);
> > -                     strbuf_release(&dir);
> > -                     return NULL;
> > -             }
> > -             die(_("not a git repository (or any parent up to mount point %s)\n"
> > -                   "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> > -                 dir.buf);
> > +             if (!nongit_ok)
> > +                     die(_("not a git repository (or any parent up to mount point %s)\n"
> > +                                             "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> > +                                     dir.buf);
>
> This indentation would probably be better aligned with the first
> double-quote on the `die` line.

>
> Otherwise: ACK!
>
> Thank you,
> Johannes
>
> > +             *nongit_ok = 1;
> > +             strbuf_release(&dir);
> > +             return NULL;
> >       default:
> >               BUG("unhandled setup_git_directory_1() result");
> >       }
> > --
> > 2.7.4
> >
> >

> diff --git a/setup.c b/setup.c
> index 1be5037..e1a9e17 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
>  	return NULL;
>  }
>  
> -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> -{
> -	if (!nongit_ok)
> -		die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> -	if (chdir(cwd))
> -		die_errno(_("cannot come back to cwd"));
> -	*nongit_ok = 1;
> -	return NULL;
> -}
> -
>  static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
>  {
>  	struct stat buf;
> @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
>  		break;
>  	case GIT_DIR_HIT_CEILING:
> -		prefix = setup_nongit(cwd.buf, nongit_ok);
> -		break;
> +		if (!nongit_ok)
> +			die(_("not a git repository (or any of the parent directories): %s"),
> +					DEFAULT_GIT_DIR_ENVIRONMENT);

> +		*nongit_ok = 1;
> +		strbuf_release(&dir);
> +		return NULL;
>  	case GIT_DIR_HIT_MOUNT_POINT:
> -		if (nongit_ok) {
> -			*nongit_ok = 1;
> -			strbuf_release(&cwd);
> -			strbuf_release(&dir);
> -			return NULL;
> -		}
> -		die(_("not a git repository (or any parent up to mount point %s)\n"
> -		      "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> -		    dir.buf);
> +		if (!nongit_ok)
> +			die(_("not a git repository (or any parent up to mount point %s)\n"
> +			      "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> +					dir.buf);

> +		*nongit_ok = 1;
> +		strbuf_release(&dir);
> +		return NULL;
>  	default:
>  		BUG("unhandled setup_git_directory_1() result");
>  	}
> -- 
> 2.7.4
> 
> 

>
> Hi Erin,
>
> On Sat, 15 Dec 2018, Erin Dahlgren wrote:
>
> > diff --git a/setup.c b/setup.c
> > index 1be5037..e1a9e17 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
> >       return NULL;
> >  }
> >
> > -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> > -{
> > -     if (!nongit_ok)
> > -             die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> > -     if (chdir(cwd))
> > -             die_errno(_("cannot come back to cwd"));
> > -     *nongit_ok = 1;
> > -     return NULL;
> > -}
> > -
> >  static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
> >  {
> >       struct stat buf;
> > @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >               prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
> >               break;
> >       case GIT_DIR_HIT_CEILING:
> > -             prefix = setup_nongit(cwd.buf, nongit_ok);
> > -             break;
> > +             if (!nongit_ok)
> > +                     die(_("not a git repository (or any of the parent directories): %s"),
> > +                                     DEFAULT_GIT_DIR_ENVIRONMENT);
>
> I am terribly sorry to bother you about formatting issues (in my mind, it
> is quite an annoying thing that we still have no fully automatic way to
> format Git's source code according to Git's preferred coding style, but
> there you go...): this `DEFAULT_GIT_DIR_ENVIRONMENT` should be aligned
> with the first parameter of `die()`, i.e.
>
> +               if (!nongit_ok)
> +                       die(_("not a git repository (or any of the parent directories): %s"),
> +                           DEFAULT_GIT_DIR_ENVIRONMENT);
>
> > +             *nongit_ok = 1;
> > +             strbuf_release(&dir);
> > +             return NULL;
> >       case GIT_DIR_HIT_MOUNT_POINT:
> > -             if (nongit_ok) {
> > -                     *nongit_ok = 1;
> > -                     strbuf_release(&cwd);
> > -                     strbuf_release(&dir);
> > -                     return NULL;
> > -             }
> > -             die(_("not a git repository (or any parent up to mount point %s)\n"
> > -                   "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> > -                 dir.buf);
> > +             if (!nongit_ok)
> > +                     die(_("not a git repository (or any parent up to mount point %s)\n"
> > +                           "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> > +                                     dir.buf);
>
> Likewise, `dir.buf` should be aligned with the `_` two lines above.

>
> Otherwise I think this patch is good to go!
>
> Thank you,
> Johannes
>
> > +             *nongit_ok = 1;
> > +             strbuf_release(&dir);
> > +             return NULL;
> >       default:
> >               BUG("unhandled setup_git_directory_1() result");
> >       }
> > --
> > 2.7.4
> >
> >

> Before this change are two misleading additional behaviors:
> 
>   - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> 	apparent reason. We never had the chance to change directories
> 	up to this point so chdir(current cwd) is pointless.

>   - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> 	of a static struct strbuf (cwd). This is unnecessary because the
> 	struct is static so its buffer is always reachable. This is also
> 	misleading because nowhere else in the function is this buffer
> 	released.

>  	case GIT_DIR_HIT_CEILING:
> -		prefix = setup_nongit(cwd.buf, nongit_ok);
> -		break;
> +		if (!nongit_ok)
> +			die(_("not a git repository (or any of the parent directories): %s"),
>a +					DEFAULT_GIT_DIR_ENVIRONMENT);
> +		*nongit_ok = 1;
> +		strbuf_release(&dir);
> +		return NULL;

>
> On Sat, Dec 15, 2018 at 05:05:08PM -0800, Erin Dahlgren wrote:
>
> > setup_git_directory_gently() expects two types of failures to
> > discover a git directory (e.g. .git/):
> > [...]
>
> When I read your subject line, I'll admit to having a funny feeling in
> the pit of my stomach. This setup code has historically been full of
> subtle corner cases, and I expected a very tricky review.
>
> However, your explanation was so well-written that it was a pleasure to
> read. :)

>
> > Before this change are two misleading additional behaviors:
> >
> >   - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> >       apparent reason. We never had the chance to change directories
> >       up to this point so chdir(current cwd) is pointless.
>
> That makes sense. I think this is a holdover from when we used to walk
> backwards via chdir(), prior to ce9b8aab5d (setup_git_directory_1():
> avoid changing global state, 2017-03-13). Back then, we needed to
> restore the state at this point, but now we don't.
>
> >   - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> >       of a static struct strbuf (cwd). This is unnecessary because the
> >       struct is static so its buffer is always reachable. This is also
> >       misleading because nowhere else in the function is this buffer
> >       released.
>
> Makes sense.
>
> I do have one question, though:
>
> >       case GIT_DIR_HIT_CEILING:
> > -             prefix = setup_nongit(cwd.buf, nongit_ok);
> > -             break;
> > +             if (!nongit_ok)
> > +                     die(_("not a git repository (or any of the parent directories): %s"),
> >a +                                    DEFAULT_GIT_DIR_ENVIRONMENT);
> > +             *nongit_ok = 1;
> > +             strbuf_release(&dir);
> > +             return NULL;
>
> This case used to break out of the switch, and now returns early.
>
> So we do not execute the later code which clears GIT_PREFIX_ENVIRONMENT,
> and zeroes the fields in startup_info. Those probably don't matter in
> most cases, but I wonder if there are some obscure ones where it might.
>
> Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like
> GIT_DIR_HIT_CEILING currently is, rather than the other way around?
> I.e., something like:
>
>   case GIT_DIR_HIT_CEILING:
>         if (!nongit_ok)
>                 die(...);
>         *nongit_ok = 1;
>         prefix = NULL;
>         break;
>   case GIT_DIR_HIT_MOUNT_POINT:
>         if (!nongit_ok)
>                 die(...);
>         *nongit_ok = 1;
>         prefix = NULL;
>         break;
>
> ?

> > Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like
> > GIT_DIR_HIT_CEILING currently is, rather than the other way around?
> > I.e., something like:
> >
> >   case GIT_DIR_HIT_CEILING:
> >         if (!nongit_ok)
> >                 die(...);
> >         *nongit_ok = 1;
> >         prefix = NULL;
> >         break;
> >   case GIT_DIR_HIT_MOUNT_POINT:
> >         if (!nongit_ok)
> >                 die(...);
> >         *nongit_ok = 1;
> >         prefix = NULL;
> >         break;
> >
> > ?
> 
> After some digging, there seems to be a documented guarantee that
> "`GIT_PREFIX` is set as returned by running 'git rev-parse
> --show-prefix'". See Documentation/config/alias.txt. If the
> environment variable GIT_PREFIX is already set to something and we
> don't clear it when prefix is NULL, then the two can be out of sync.
> So that's a problem with my patch and the current handling of
> GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it
> is, but we should respect what's documented.

> Side note: One of the secondary goals of my patch was to make it
> really obvious that neither the GIT_DIR_HIT_CEILING nor the
> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
> your suggestion (and it's a fair one) I don't think that's feasible
> without more significant refactoring. Should I just settle with a
> comment that explains this?

> On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote:
> ...
>> GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it
>> is, but we should respect what's documented.
>
> Yeah, agreed.
>
> Another benefit of avoiding the early return is that we hit the cleanup
> code at the end of the function. That saves us having to release "dir"
> here. I assume we don't have to care about "gitdir" in these cases, but
> hitting the cleanup code means we don't even have to think about it.
>
> Over in:
>
>   https://public-inbox.org/git/20181219154853.GC14802@sigill.intra.peff.net/
>
> I suggested adding more cleanup to that block, too (though I _think_ in
> these cases it would also be a noop, it's again nice to not have to
> worry about it).
>
>> Side note: One of the secondary goals of my patch was to make it
>> really obvious that neither the GIT_DIR_HIT_CEILING nor the
>> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
>> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
>> your suggestion (and it's a fair one) I don't think that's feasible
>> without more significant refactoring. Should I just settle with a
>> comment that explains this?
>
> Yeah, I think a comment would probably be sufficient. Though we could
> also perhaps make the two cases (i.e., we found a repo or not) more
> clear. Something like:
>
>   if (!*nongit_ok || !*nongit_ok) {

> 	startup_info->have_repository = 1;
> 	startup_info->prefix = prefix;
> 	if (prefix) ...etc...
>   } else {
> 	start_info->have_repository = 0;
> 	startup_info->prefix = NULL;
> 	setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
>   }
>
> That may introduce some slight repetition, but I think it's probably
> clearer to deal with the cases separately (and it saves earlier code
> from make-work like setting prefix=NULL when there's no repo).

> >> Side note: One of the secondary goals of my patch was to make it
> >> really obvious that neither the GIT_DIR_HIT_CEILING nor the
> >> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
> >> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
> >> your suggestion (and it's a fair one) I don't think that's feasible
> >> without more significant refactoring. Should I just settle with a
> >> comment that explains this?
> >
> > Yeah, I think a comment would probably be sufficient. Though we could
> > also perhaps make the two cases (i.e., we found a repo or not) more
> > clear. Something like:
> >
> >   if (!*nongit_ok || !*nongit_ok) {
> 
> WTH is (A || A)?

>
> On Wed, Dec 26, 2018 at 02:22:39PM -0800, Junio C Hamano wrote:
>
> > >> Side note: One of the secondary goals of my patch was to make it
> > >> really obvious that neither the GIT_DIR_HIT_CEILING nor the
> > >> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
> > >> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
> > >> your suggestion (and it's a fair one) I don't think that's feasible
> > >> without more significant refactoring. Should I just settle with a
> > >> comment that explains this?
> > >
> > > Yeah, I think a comment would probably be sufficient. Though we could
> > > also perhaps make the two cases (i.e., we found a repo or not) more
> > > clear. Something like:
> > >
> > >   if (!*nongit_ok || !*nongit_ok) {
> >
> > WTH is (A || A)?
>
> Heh, I should learn to cut and paste better. This should be:
>
>   if (!nongit_ok || !*nongit_ok)
>
> (which comes from the current code).

> > Heh, I should learn to cut and paste better. This should be:
> >
> >   if (!nongit_ok || !*nongit_ok)
> >
> > (which comes from the current code).
> 
> Yep, but I think we can benefit from De Morgan's law here, where:
> 
>   (!nongit_ok || !*nongit_ok) == (!(nongit_ok && *nongit_ok))
> 
> PATCH v3 (just sent) uses that transformation like this:
> 
> if (nongit_ok && *nongit_ok) {
>   ... startup_info->has_repository = 0;
> } else {
>   // !nongit_ok || !*nongit_ok
>   .. startup_info->has_repository = 1;
> }
> 
> Because IMHO (nongit_ok && *nongit_ok) is easier to read and reason
> about. Added brief comments as well.

> Before this change are two misleading additional behaviors:
> 
>   - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> 	apparent reason. We never had the chance to change directories
> 	up to this point so chdir(current cwd) is pointless.
>   - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> 	of a static struct strbuf (cwd). This is unnecessary because the
> 	struct is static so its buffer is always reachable. This is also
> 	misleading because nowhere else in the function is this buffer
> 	released.
> [..]

> +	/*
> +	 * At this point, nongit_ok is stable. If it is non-NULL and points
> +	 * to a non-zero value, then this means that we haven't found a
> +	 * repository and that the caller expects startup_info to reflect
> +	 * this.

> +	 *
> +	 * Regardless of the state of nongit_ok, startup_info->prefix and
> +	 * the GIT_PREFIX environment variable must always match. For details
> +	 * see Documentation/config/alias.txt.
> +	 */

> +	if (nongit_ok && *nongit_ok) {
> +		startup_info->have_repository = 0;
> +		startup_info->prefix = NULL;
>  		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
> -
> -	startup_info->have_repository = !nongit_ok || !*nongit_ok;
> -	startup_info->prefix = prefix;
> +	} else {
> +		// !nongit_ok || !*nongit_ok
> +		startup_info->have_repository = 1;
> +		startup_info->prefix = prefix;
> +		if (prefix)
> +			setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> +		else
> +			setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
> +	}

> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  	 * the user has set GIT_DIR.  It may be beneficial to disallow bogus
>  	 * GIT_DIR values at some point in the future.
>  	 */
> -	if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
> +	if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
> +	    startup_info->have_repository ||
> +	    // GIT_DIR_EXPLICIT
> +	    getenv(GIT_DIR_ENVIRONMENT)) {

> This patch isn't _too_ big, so I don't think it's worth the effort at
> this point for this particular case, but just something to think about
> for the future. A series around this topic would probably be something
> like:
>
>   1: drop the useless chdir and fold setup_nongit() into the main
>      function
>
>   2: stop doing the early return from HIT_MOUNT_POINT, and treat it just
>      like HIT_CEILING (and drop the useless strbuf release)
>
>   3: treat GIT_DIR_NONE as a BUG
>
>   4: rearrange the nongit logic at the end of the function for clarity

> We usually avoid "//" comments, even for single lines (though that is
> perhaps superstition at this point, as we've started to embrace several
> other C99-isms). IMHO this particular comment is not even really
> necessary, as the whole conditional is suitably short and obvious after
> your refactor.

>
>> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
>>  	 * the user has set GIT_DIR.  It may be beneficial to disallow bogus
>>  	 * GIT_DIR values at some point in the future.
>>  	 */
>> -	if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
>> +	if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
>> +	    startup_info->have_repository ||
>> +	    // GIT_DIR_EXPLICIT
>> +	    getenv(GIT_DIR_ENVIRONMENT)) {
>
> Same "//" style issue as above. I'm not sure how much value there is in
> repeating the GIT_DIR_* cases here, as they're likely to grow out of
> sync with the switch() statement above.

> >> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >>  	 * the user has set GIT_DIR.  It may be beneficial to disallow bogus
> >>  	 * GIT_DIR values at some point in the future.
> >>  	 */
> >> -	if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
> >> +	if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
> >> +	    startup_info->have_repository ||
> >> +	    // GIT_DIR_EXPLICIT
> >> +	    getenv(GIT_DIR_ENVIRONMENT)) {
> >
> > Same "//" style issue as above. I'm not sure how much value there is in
> > repeating the GIT_DIR_* cases here, as they're likely to grow out of
> > sync with the switch() statement above.
> 
> It is unclear to me if the original code is doing the right thing
> under one condition, and this patch does not seem to change the
> behaviour.
>
> What happens if GIT_DIR environment is set to an invalid path and
> nongit_ok is non-NULL?  setup_explicit_git_dir() would have assigned
> 1 to *nongit_ok, so have_repository is false at this point.
> 
> We enter the if() statement in such a case, and end up calling
> setup_git_env(gitdir) using the bogus path that is not pointing at a
> repository.  We leave have_repository to be false but the paths
> recorded in the_repository by setup_git_env() would all point at
> bogus places.

>
> On Thu, Jan 03, 2019 at 10:09:18AM -0800, Junio C Hamano wrote:
>
> > >> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
> > >>     * the user has set GIT_DIR.  It may be beneficial to disallow bogus
> > >>     * GIT_DIR values at some point in the future.
> > >>     */
> > >> -  if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
> > >> +  if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
> > >> +      startup_info->have_repository ||
> > >> +      // GIT_DIR_EXPLICIT
> > >> +      getenv(GIT_DIR_ENVIRONMENT)) {
> > >
> > > Same "//" style issue as above. I'm not sure how much value there is in
> > > repeating the GIT_DIR_* cases here, as they're likely to grow out of
> > > sync with the switch() statement above.
> >
> > It is unclear to me if the original code is doing the right thing
> > under one condition, and this patch does not seem to change the
> > behaviour.
> >
> > What happens if GIT_DIR environment is set to an invalid path and
> > nongit_ok is non-NULL?  setup_explicit_git_dir() would have assigned
> > 1 to *nongit_ok, so have_repository is false at this point.
> >
> > We enter the if() statement in such a case, and end up calling
> > setup_git_env(gitdir) using the bogus path that is not pointing at a
> > repository.  We leave have_repository to be false but the paths
> > recorded in the_repository by setup_git_env() would all point at
> > bogus places.
>
> Yeah, that matches my analysis of what the code is doing.
>
> Looks like this code (and comment) come from 73f192c991 (setup: don't
> perform lazy initialization of repository state, 2017-06-20). I'm
> guessing this was mostly a hack to quiet some test that complained about
> the other changes in that commit. And indeed, dropping the getenv() half
> of the conditional and running the tests in 73f192c991 gets me a failure
> in t1050, which does:
>
>   GIT_DIR=non-existent git index-pack --strict --verify foo/.git/objects/pack/*.pack
>
> But that's a bug in index-pack, which should be able to verify a pack
> outside of a repository. And I think (according to bisection) we fixed
> that in 14a9bd2898 (prepare_commit_graft: treat non-repository as a
> noop, 2018-05-31), and the tests currently pass even with this patch
> applied:
>
> diff --git a/setup.c b/setup.c
> index 1be5037f12..e2c03e9bbc 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1132,7 +1132,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>          * the user has set GIT_DIR.  It may be beneficial to disallow bogus
>          * GIT_DIR values at some point in the future.
>          */
> -       if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
> +       if (startup_info->have_repository) {
>                 if (!the_repository->gitdir) {
>                         const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
>                         if (!gitdir)
>
> > > At first I thought this could all be folded into the "else" clause of
> > > the conditional above (which would make the logic much easier to
> > > follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is
> > > what that getenv() is trying to handle here.
> >
> > Yes, but should GIT_DIR=/bogus even be touching the_repository?
>
> Probably not. And from my poking around above, I think we're probably
> safe to remove this hackery now.
>
> > It is a separate clean-up and does not affect the validity of this
> > simplification patchd, so I agreee with ...
> >
> > > So I think this is the best we can do for now.
>
> Yep, it's definitely orthogonal. But if we do this cleanup as part of
> it, we should be able to simplify further on top of Erin's patch, like
> this:
>
> diff --git a/setup.c b/setup.c
> index eb8332bc02..edf65c44bf 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1125,35 +1125,26 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                 // !nongit_ok || !*nongit_ok
>                 startup_info->have_repository = 1;
>                 startup_info->prefix = prefix;
> +
>                 if (prefix)
>                         setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
>                 else
>                         setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
> -       }
>
> -       /*
> -        * Not all paths through the setup code will call 'set_git_dir()' (which
> -        * directly sets up the environment) so in order to guarantee that the
> -        * environment is in a consistent state after setup, explicitly setup
> -        * the environment if we have a repository.
> -        *
> -        * NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some
> -        * code paths so we also need to explicitly setup the environment if
> -        * the user has set GIT_DIR.  It may be beneficial to disallow bogus
> -        * GIT_DIR values at some point in the future.
> -        */
> -       if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
> -           startup_info->have_repository ||
> -           // GIT_DIR_EXPLICIT
> -           getenv(GIT_DIR_ENVIRONMENT)) {
> +               /*
> +                * Not all paths through the setup code will call 'set_git_dir()' (which
> +                * directly sets up the environment) so in order to guarantee that the
> +                * environment is in a consistent state after setup, explicitly setup
> +                * the environment if we have a repository.
> +                */
>                 if (!the_repository->gitdir) {
>                         const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
>                         if (!gitdir)
>                                 gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
>                         setup_git_env(gitdir);
>                 }
> -               if (startup_info->have_repository)
> -                       repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> +
> +               repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
>         }
>
>         strbuf_release(&dir);
>
> I actually wonder if the "not all paths..." bit is even still true these
> days (and if it is, I think we should consider fixing those code paths).
> Certainly that setup_git_env() needed to trigger for the GIT_DIR=bogus,
> but with that gone, any $GIT_DIR should have triggered GIT_DIR_EXPLICIT,
> and any "default to .git" should be handled as part of discovery, I'd
> think.
>
> So what next? Erin, are you interested in using the details of this
> conversation to take the cleanups a bit further?

> > So what next? Erin, are you interested in using the details of this
> > conversation to take the cleanups a bit further?
> 
> Sure, no problem. If this is urgent, then I would probably be more
> inclined to keep this small and do more cleanup in followup patches.
> But if it's not urgent (that is my understanding), I'd be happy to
> take the cleanups further. I'm traveling today through next week but
> I'll try to post another version addressing the comments in the next
> couple of days.