[PATCH] Simplify handling of setup_git_directory_gently() failure cases.

[PATCH] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

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>
---
 setup.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

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

Re: [PATCH] Simplify handling of setup_git_directory_gently() failure cases.

[Johannes Schindelin]

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

Re: [PATCH] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

On Fri, Dec 14, 2018 at 2:32 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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.

Hi Johannes,

No problem. Thanks for your time discussing and reviewing!

- Erin

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

[PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

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).
---
Changes in v2:
  - Aligned indentation of second line of die message for case
  GIT_DIR_HIT_MOUNT_POINT.

 setup.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

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

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Johannes Schindelin]

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

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

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;

?

-Peff

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

Sorry Johannes for the repeat message, I failed to send to all.

On Tue, Dec 18, 2018 at 4:35 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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.

Hi Johannes,

No problem, I'll make those changes.

I'd be interested to hear the arguments against a "fully automatic way
to format Git's source code according to Git's preferred coding
style". If there aren't any then I'd be willing to take a crack at it.
Should we start a separate "discussion" thread?

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

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

Hi Peff,

On Tue, Dec 18, 2018 at 9:54 AM Jeff King <peff@peff.net> wrote:
>
> 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. :)

Thanks :)

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

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?

Thanks,
Erin

>
> -Peff

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote:

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

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

The condition would also be a lot less confusing if nongit_ok were
flipped, but I think that would be a big refactor due to the way we pass
it around.

-Peff

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

WTH is (A || A)?

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

Sounds good.

Thanks both.

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

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

-Peff

[PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

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

During review, this change was amended to additionally include:

  - Neither GIT_DIR_HIT_CEILING nor GIT_DIR_HIT_MOUNT_POINT may
	return early from setup_git_directory_gently() before the
	GIT_PREFIX environment variable is reset. Change both cases to
	break instead of return. See GIT_PREFIX below for more details.

  - GIT_DIR_NONE: setup_git_directory_gently_1() never returns this
	value, but if it ever did, setup_git_directory_gently() would
	incorrectly record that it had found a repository. Explicitly
	BUG on this case because it is underspecified.

  - GIT_PREFIX: this environment variable must always match the
	value of startup_info->prefix and the prefix returned from
	setup_git_directory_gently(). Make how we handle this slightly
	more repetitive but also more clear.

  - setup_git_env() and repo_set_hash_algo(): Add comments showing
	that only GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, and GIT_DIR_BARE
	will cause setup_git_directory_gently() to call these setup
	functions. This was obvious (but partly incorrect) before this
	change when GIT_DIR_HIT_MOUNT_POINT returned early from
	setup_git_directory_gently().
---
Changes in v3:

  - Re-aligned arguments to die() calls to match formatting convention.

  - Neither GIT_DIR_HIT_CEILING nor GIT_DIR_HIT_MOUNT_POINT may
  return early from setup_git_directory_gently() before the
  GIT_PREFIX environment variable is reset. Change both cases to
  break instead of return. See GIT_PREFIX below for more details.

  - GIT_DIR_NONE: setup_git_directory_gently_1() never returns this
  value, but if it ever did, setup_git_directory_gently() would
  incorrectly record that it had found a repository. Explicitly
  BUG on this case because it is underspecified.

  - GIT_PREFIX: this environment variable must always match the
  value of startup_info->prefix and the prefix returned from
  setup_git_directory_gently(). Make how we handle this slightly
  more repetitive but also more clear.

  - setup_git_env() and repo_set_hash_algo(): Add comments showing
  that only GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, and GIT_DIR_BARE
  will cause setup_git_directory_gently() to call these setup
  functions. This was obvious (but partly incorrect) before this
  change when GIT_DIR_HIT_MOUNT_POINT returned early from
  setup_git_directory_gently().

 setup.c | 75 ++++++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/setup.c b/setup.c
index 1be5037..eb8332b 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;
@@ -1054,7 +1044,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 {
 	static struct strbuf cwd = STRBUF_INIT;
 	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
-	const char *prefix;
+	const char *prefix = NULL;
 	struct repository_format repo_fmt;
 
 	/*
@@ -1079,9 +1069,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	strbuf_addbuf(&dir, &cwd);
 
 	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
-	case GIT_DIR_NONE:
-		prefix = NULL;
-		break;
 	case GIT_DIR_EXPLICIT:
 		prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
 		break;
@@ -1097,29 +1084,52 @@ 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);
+		if (!nongit_ok)
+			die(_("not a git repository (or any of the parent directories): %s"),
+			    DEFAULT_GIT_DIR_ENVIRONMENT);
+		*nongit_ok = 1;
 		break;
 	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;
+		break;
+	case GIT_DIR_NONE:
+		/*
+		 * As a safeguard against setup_git_directory_gently_1 returning
+		 * this value, fallthrough to BUG. Otherwise it is possible to
+		 * set startup_info->have_repository to 1 when we did nothing to
+		 * find a repository.
+		 */
 	default:
 		BUG("unhandled setup_git_directory_1() result");
 	}
 
-	if (prefix)
-		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
-	else
+	/*
+	 * 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);
+	}
 
 	/*
 	 * Not all paths through the setup code will call 'set_git_dir()' (which
@@ -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)) {
 		if (!the_repository->gitdir) {
 			const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
 			if (!gitdir)
-- 
2.7.4

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

On Thu, Dec 27, 2018 at 8:24 AM Jeff King <peff@peff.net> wrote:
>
> 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).

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.

Thanks.

- Erin

>
> -Peff

Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

On Thu, Dec 27, 2018 at 03:46:10PM -0800, Erin Dahlgren wrote:

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

Ah yes, that's much better.

-Peff

Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

On Thu, Dec 27, 2018 at 03:36:29PM -0800, Erin Dahlgren wrote:

> 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.
> [..]

Overall this looks good to me, and I'd be fine to see it go in as-is.

A few minor nits/comments, though:

> During review, this change was amended to additionally include:
> [...]

When I find myself writing big bullet lists of changes in a commit
message, that is often a good time to split the commit into several
sub-parts.

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

> +	/*
> +	 * 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.

Right, makes sense.

> +	 *
> +	 * 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.
> +	 */

I think this "must match" makes sense to comment. I didn't find
config/alias.txt particularly enlightening in that regard, though. :)

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

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.

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.

So I think this is the best we can do for now.

-Peff

Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

[Junio C Hamano]

>Subject: Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

Perhaps

	Subject: setup: simplify setup_git_directory_gently() failure cases

to clarify which part of the entire world this patch is touching.

Jeff King <peff@peff.net> writes:

> 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

Yeah, that organization does make sense.  And I agree with your rule
of thumb to use the length and complexity of the log message to
judge if a single step is getting too big.

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

FWIW "git grep" for // seems to show that we reserve use of //
inside commented out code samples.

I also agree the comments behind // in this patch are probably
unneeded.

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

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

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.

Thanks.

Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

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? If not, then I can try
to prepare a few patches on top of yours.

-Peff

Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

[Erin Dahlgren]

On Fri, Jan 4, 2019 at 12:26 AM Jeff King <peff@peff.net> wrote:
>
> 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?

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.

Thanks all for the comments so far.

- Erin

> If not, then I can try to prepare a few patches on top of yours.
>
> -Peff

Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.

[Jeff King]

On Sat, Jan 05, 2019 at 08:57:32AM -0800, Erin Dahlgren wrote:

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

Nope, I don't think it's urgent at all.  I just didn't want to dump work
on you if you were losing interest in the topic. :)

-Peff