Simplify 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 <[email protected]>

Signed-off-by Erin Dahlgren <[email protected]>

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

setup.c +6 -15

index 1be5037..b441e39 100644
@@ -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);

setup.c +3 -0

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

Signature · 1 line

2.7.4

Hi Erin,

Quoted 29 lines · 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.

Quoted 8 lines

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 <[email protected]>

Signed-off-by Erin Dahlgren <[email protected]>

Thank you for working on this!

Quoted 3 lines

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

Quoted 42 lines

setup.c +6 -20

index 1be5037..b441e39 100644
@@ -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)."),

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1100

Quoted 5 lines

setup.c +4 -1

-                 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

Quoted 10 lines

setup.c +3 -1

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

2.7.4

setup.c +0 -10

index 1be5037..e1a9e17 100644
@@ -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;

setup.c +3 -3

+		*nongit_ok = 1;
+		strbuf_release(&dir);
+		return NULL;
 	case GIT_DIR_HIT_MOUNT_POINT:
-		if (nongit_ok) {
-			*nongit_ok = 1;
-			strbuf_release(&cwd);

setup.c +3 -0

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

Signature · 1 line

2.7.4

Hi Erin,

Quoted 22 lines · Erin Dahlgren

setup.c +0 -10

index 1be5037..e1a9e17 100644
@@ -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;

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1092

Quoted 8 lines

setup.c +3 -2

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

setup.c +3 -0

+               if (!nongit_ok)
+                       die(_("not a git repository (or any of the parent directories): %s"),
+                           DEFAULT_GIT_DIR_ENVIRONMENT);

Quoted 8 lines

setup.c +3 -3

+             *nongit_ok = 1;
+             strbuf_release(&dir);
+             return NULL;
      case GIT_DIR_HIT_MOUNT_POINT:
-             if (nongit_ok) {
-                     *nongit_ok = 1;
-                     strbuf_release(&cwd);

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1100

Quoted 10 lines

setup.c +4 -6

-                     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

Quoted 10 lines

setup.c +3 -1

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

2.7.4

setup_git_directory_gently() expects two types of failures to discover a git directory (e.g. .git/):
[...]

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.

Quoted 3 lines · Erin Dahlgren

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

Quoted 6 lines

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.

Quoted 5 lines

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

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1095

Quoted 9 lines

case GIT_DIR_HIT_CEILING:

setup.c +2 -2

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

setup.c +3 -0

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

Quoted 17 lines

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;

Quoted 1 line

?

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

Quoted 2 lines

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/[email protected]/

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

Quoted 7 lines

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

Quoted 8 lines

Quoted 7 lines

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?

Quoted 5 lines

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

Quoted 14 lines · Junio C Hamano

Quoted 13 lines

Quoted 7 lines

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

Quoted 1 line

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

Quoted 5 lines

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

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

setup.c +3 -2

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

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

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.

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1148

Quoted 10 lines

setup.c +4 -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)) {

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.

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -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)) {

Quoted 8 lines

Review const char *setup_git_directory_gently(int *nongit_ok) · line 1148

Quoted 4 lines

(quoted diff) +4 -0

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

Quoted 4 lines

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

Quoted 1 line

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

Quoted 22 lines · Junio C Hamano

Quoted 5 lines

Quoted 5 lines

setup.c +0 -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)) {

Quoted 8 lines

Review const char *setup_git_directory_gently(int *nongit_ok) · line 1148

Quoted 4 lines

(quoted diff) +4 -0

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

Quoted 5 lines

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:

setup.c +1 -1

index 1be5037f12..e2c03e9bbc 100644
@@ -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)

Quoted 7 lines

Quoted 5 lines

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.

Quoted 4 lines

It is a separate clean-up and does not affect the validity of this simplification patchd, so I agreee with ...

Quoted 1 line

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:

setup.c +9 -18

index eb8332bc02..edf65c44bf 100644
@@ -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

Quoted 2 lines

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.