{"thread":{"id":"50030","subject":"[PATCH] Simplify handling of setup_git_directory_gently() failure cases.","startedAt":"2018-12-13T17:31:11Z","lastAt":"2019-01-06T06:27:44Z","messageCount":19,"participants":["Erin Dahlgren","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365277","messageId":"1544722211-13370-1-git-send-email-eedahlgren@gmail.com","threadId":"50030","inReplyTo":null,"subject":"[PATCH] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-13T17:30:11Z","receivedAt":"2018-12-13T17:31:11Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"setup_git_directory_gently() expects two types of failures to\ndiscover a git directory (e.g. .git/):\n\n - GIT_DIR_HIT_CEILING: could not find a git directory in any\n\tparent directories of the cwd.\n - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in\n\tany parent directories up to the mount point of the cwd.\n\nBoth cases are handled in a similar way, but there are misleading\nand unimportant differences. In both cases, setup_git_directory_gently()\nshould:\n\n - Die if we are not in a git repository. Otherwise:\n - Set nongit_ok = 1, indicating that we are not in a git repository\n\tbut this is ok.\n - Call strbuf_release() on any non-static struct strbufs that we\n\tallocated.\n\nBefore this change are two misleading additional behaviors:\n\n - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n\tapparent reason. We never had the chance to change directories\n\tup to this point so chdir(current cwd) is pointless.\n - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n\tof a static struct strbuf (cwd). This is unnecessary because the\n\tstruct is static so its buffer is always reachable. This is also\n\tmisleading because nowhere else in the function is this buffer\n\treleased.\n\nThis change eliminates these two misleading additional behaviors and\ndeletes setup_nogit() because the code is clearer without it. The\nresult is that we can see clearly that GIT_DIR_HIT_CEILING and\nGIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the\ndifferent help messages).\n\nThanks-to: Johannes Schindelin \nSigned-off-by: Erin Dahlgren \n---\n setup.c | 34 +++++++++++++---------------------\n 1 file changed, 13 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037..b441e39 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n \treturn NULL;\n }\n \n-static const char *setup_nongit(const char *cwd, int *nongit_ok)\n-{\n-\tif (!nongit_ok)\n-\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n-\tif (chdir(cwd))\n-\t\tdie_errno(_(\"cannot come back to cwd\"));\n-\t*nongit_ok = 1;\n-\treturn NULL;\n-}\n-\n static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n {\n \tstruct stat buf;\n@@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tprefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n \t\tbreak;\n \tcase GIT_DIR_HIT_CEILING:\n-\t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n-\t\tbreak;\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n+\t\t\t\t\tDEFAULT_GIT_DIR_ENVIRONMENT);\n+\t\t*nongit_ok = 1;\n+\t\tstrbuf_release(&dir);\n+\t\treturn NULL;\n \tcase GIT_DIR_HIT_MOUNT_POINT:\n-\t\tif (nongit_ok) {\n-\t\t\t*nongit_ok = 1;\n-\t\t\tstrbuf_release(&cwd);\n-\t\t\tstrbuf_release(&dir);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n-\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n-\t\t dir.buf);\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n+\t\t\t\t\t\t\"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n+\t\t\t\t\tdir.buf);\n+\t\t*nongit_ok = 1;\n+\t\tstrbuf_release(&dir);\n+\t\treturn NULL;\n \tdefault:\n \t\tBUG(\"unhandled setup_git_directory_1() result\");\n \t}\n-- \n2.7.4\n\n"},{"id":"365340","messageId":"nycvar.QRO.7.76.6.1812141130180.43@tvgsbejvaqbjf.bet","threadId":"50030","inReplyTo":"1544722211-13370-1-git-send-email-eedahlgren@gmail.com","subject":"Re: [PATCH] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-14T10:32:12Z","receivedAt":"2018-12-14T10:32:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Erin,\n\nOn Thu, 13 Dec 2018, Erin Dahlgren wrote:\n\n> setup_git_directory_gently() expects two types of failures to\n> discover a git directory (e.g. .git/):\n> \n> - GIT_DIR_HIT_CEILING: could not find a git directory in any\n> \tparent directories of the cwd.\n> - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in\n> \tany parent directories up to the mount point of the cwd.\n> \n> Both cases are handled in a similar way, but there are misleading\n> and unimportant differences. In both cases, setup_git_directory_gently()\n> should:\n> \n> - Die if we are not in a git repository. Otherwise:\n> - Set nongit_ok = 1, indicating that we are not in a git repository\n> \tbut this is ok.\n> - Call strbuf_release() on any non-static struct strbufs that we\n> \tallocated.\n> \n> Before this change are two misleading additional behaviors:\n> \n> - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n> \tapparent reason. We never had the chance to change directories\n> \tup to this point so chdir(current cwd) is pointless.\n> - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n> \tof a static struct strbuf (cwd). This is unnecessary because the\n> \tstruct is static so its buffer is always reachable. This is also\n> \tmisleading because nowhere else in the function is this buffer\n> \treleased.\n> \n> This change eliminates these two misleading additional behaviors and\n> deletes setup_nogit() because the code is clearer without it. The\n> result is that we can see clearly that GIT_DIR_HIT_CEILING and\n> GIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the\n> different help messages).\n> \n> Thanks-to: Johannes Schindelin \n> Signed-off-by: Erin Dahlgren \n\nThank you for working on this!\n\n> ---\n> setup.c | 34 +++++++++++++---------------------\n> 1 file changed, 13 insertions(+), 21 deletions(-)\n\nNice! It's always good to see a code reduction with such a cleanup.\n\nJust one thing:\n\n> diff --git a/setup.c b/setup.c\n> index 1be5037..b441e39 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n> \treturn NULL;\n> }\n> \n> -static const char *setup_nongit(const char *cwd, int *nongit_ok)\n> -{\n> -\tif (!nongit_ok)\n> -\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n> -\tif (chdir(cwd))\n> -\t\tdie_errno(_(\"cannot come back to cwd\"));\n> -\t*nongit_ok = 1;\n> -\treturn NULL;\n> -}\n> -\n> static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n> {\n> \tstruct stat buf;\n> @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t\tprefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n> \t\tbreak;\n> \tcase GIT_DIR_HIT_CEILING:\n> -\t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n> -\t\tbreak;\n> +\t\tif (!nongit_ok)\n> +\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n> +\t\t\t\t\tDEFAULT_GIT_DIR_ENVIRONMENT);\n> +\t\t*nongit_ok = 1;\n> +\t\tstrbuf_release(&dir);\n> +\t\treturn NULL;\n> \tcase GIT_DIR_HIT_MOUNT_POINT:\n> -\t\tif (nongit_ok) {\n> -\t\t\t*nongit_ok = 1;\n> -\t\t\tstrbuf_release(&cwd);\n> -\t\t\tstrbuf_release(&dir);\n> -\t\t\treturn NULL;\n> -\t\t}\n> -\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> -\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> -\t\t dir.buf);\n> +\t\tif (!nongit_ok)\n> +\t\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> +\t\t\t\t\t\t\"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> +\t\t\t\t\tdir.buf);\n\nThis indentation would probably be better aligned with the first\ndouble-quote on the `die` line.\n\nOtherwise: ACK!\n\nThank you,\nJohannes\n\n> +\t\t*nongit_ok = 1;\n> +\t\tstrbuf_release(&dir);\n> +\t\treturn NULL;\n> \tdefault:\n> \t\tBUG(\"unhandled setup_git_directory_1() result\");\n> \t}\n> -- \n> 2.7.4\n> \n> \n"},{"id":"365440","messageId":"CAP_Smy1q4kdZTX-38XvG47PA-HOWJZk=kmT4yS+swprC-5q4rw@mail.gmail.com","threadId":"50030","inReplyTo":"nycvar.QRO.7.76.6.1812141130180.43@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-16T01:05:01Z","receivedAt":"2018-12-16T01:05:19Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"On Fri, Dec 14, 2018 at 2:32 AM Johannes Schindelin\n wrote:\n>\n> Hi Erin,\n>\n> On Thu, 13 Dec 2018, Erin Dahlgren wrote:\n>\n> > setup_git_directory_gently() expects two types of failures to\n> > discover a git directory (e.g. .git/):\n> >\n> > - GIT_DIR_HIT_CEILING: could not find a git directory in any\n> > parent directories of the cwd.\n> > - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in\n> > any parent directories up to the mount point of the cwd.\n> >\n> > Both cases are handled in a similar way, but there are misleading\n> > and unimportant differences. In both cases, setup_git_directory_gently()\n> > should:\n> >\n> > - Die if we are not in a git repository. Otherwise:\n> > - Set nongit_ok = 1, indicating that we are not in a git repository\n> > but this is ok.\n> > - Call strbuf_release() on any non-static struct strbufs that we\n> > allocated.\n> >\n> > Before this change are two misleading additional behaviors:\n> >\n> > - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n> > apparent reason. We never had the chance to change directories\n> > up to this point so chdir(current cwd) is pointless.\n> > - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n> > of a static struct strbuf (cwd). This is unnecessary because the\n> > struct is static so its buffer is always reachable. This is also\n> > misleading because nowhere else in the function is this buffer\n> > released.\n> >\n> > This change eliminates these two misleading additional behaviors and\n> > deletes setup_nogit() because the code is clearer without it. The\n> > result is that we can see clearly that GIT_DIR_HIT_CEILING and\n> > GIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the\n> > different help messages).\n> >\n> > Thanks-to: Johannes Schindelin \n> > Signed-off-by: Erin Dahlgren \n>\n> Thank you for working on this!\n>\n> > ---\n> > setup.c | 34 +++++++++++++---------------------\n> > 1 file changed, 13 insertions(+), 21 deletions(-)\n>\n> Nice! It's always good to see a code reduction with such a cleanup.\n>\n> Just one thing:\n>\n> > diff --git a/setup.c b/setup.c\n> > index 1be5037..b441e39 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n> > return NULL;\n> > }\n> >\n> > -static const char *setup_nongit(const char *cwd, int *nongit_ok)\n> > -{\n> > - if (!nongit_ok)\n> > - die(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n> > - if (chdir(cwd))\n> > - die_errno(_(\"cannot come back to cwd\"));\n> > - *nongit_ok = 1;\n> > - return NULL;\n> > -}\n> > -\n> > static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n> > {\n> > struct stat buf;\n> > @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> > prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n> > break;\n> > case GIT_DIR_HIT_CEILING:\n> > - prefix = setup_nongit(cwd.buf, nongit_ok);\n> > - break;\n> > + if (!nongit_ok)\n> > + die(_(\"not a git repository (or any of the parent directories): %s\"),\n> > + DEFAULT_GIT_DIR_ENVIRONMENT);\n> > + *nongit_ok = 1;\n> > + strbuf_release(&dir);\n> > + return NULL;\n> > case GIT_DIR_HIT_MOUNT_POINT:\n> > - if (nongit_ok) {\n> > - *nongit_ok = 1;\n> > - strbuf_release(&cwd);\n> > - strbuf_release(&dir);\n> > - return NULL;\n> > - }\n> > - die(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> > - \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> > - dir.buf);\n> > + if (!nongit_ok)\n> > + die(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> > + \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> > + dir.buf);\n>\n> This indentation would probably be better aligned with the first\n> double-quote on the `die` line.\n\nHi Johannes,\n\nNo problem. Thanks for your time discussing and reviewing!\n\n- Erin\n\n>\n> Otherwise: ACK!\n>\n> Thank you,\n> Johannes\n>\n> > + *nongit_ok = 1;\n> > + strbuf_release(&dir);\n> > + return NULL;\n> > default:\n> > BUG(\"unhandled setup_git_directory_1() result\");\n> > }\n> > --\n> > 2.7.4\n> >\n> >\n"},{"id":"365441","messageId":"1544922308-740-1-git-send-email-eedahlgren@gmail.com","threadId":"50030","inReplyTo":"1544722211-13370-1-git-send-email-eedahlgren@gmail.com","subject":"[PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-16T01:05:08Z","receivedAt":"2018-12-16T01:05:24Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"setup_git_directory_gently() expects two types of failures to\ndiscover a git directory (e.g. .git/):\n\n - GIT_DIR_HIT_CEILING: could not find a git directory in any\n\tparent directories of the cwd.\n - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in\n\tany parent directories up to the mount point of the cwd.\n\nBoth cases are handled in a similar way, but there are misleading\nand unimportant differences. In both cases, setup_git_directory_gently()\nshould:\n\n - Die if we are not in a git repository. Otherwise:\n - Set nongit_ok = 1, indicating that we are not in a git repository\n\tbut this is ok.\n - Call strbuf_release() on any non-static struct strbufs that we\n\tallocated.\n\nBefore this change are two misleading additional behaviors:\n\n - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n\tapparent reason. We never had the chance to change directories\n\tup to this point so chdir(current cwd) is pointless.\n - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n\tof a static struct strbuf (cwd). This is unnecessary because the\n\tstruct is static so its buffer is always reachable. This is also\n\tmisleading because nowhere else in the function is this buffer\n\treleased.\n\nThis change eliminates these two misleading additional behaviors and\ndeletes setup_nogit() because the code is clearer without it. The\nresult is that we can see clearly that GIT_DIR_HIT_CEILING and\nGIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the\ndifferent help messages).\n---\nChanges in v2:\n - Aligned indentation of second line of die message for case\n GIT_DIR_HIT_MOUNT_POINT.\n\n setup.c | 34 +++++++++++++---------------------\n 1 file changed, 13 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037..e1a9e17 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n \treturn NULL;\n }\n \n-static const char *setup_nongit(const char *cwd, int *nongit_ok)\n-{\n-\tif (!nongit_ok)\n-\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n-\tif (chdir(cwd))\n-\t\tdie_errno(_(\"cannot come back to cwd\"));\n-\t*nongit_ok = 1;\n-\treturn NULL;\n-}\n-\n static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n {\n \tstruct stat buf;\n@@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tprefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n \t\tbreak;\n \tcase GIT_DIR_HIT_CEILING:\n-\t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n-\t\tbreak;\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n+\t\t\t\t\tDEFAULT_GIT_DIR_ENVIRONMENT);\n+\t\t*nongit_ok = 1;\n+\t\tstrbuf_release(&dir);\n+\t\treturn NULL;\n \tcase GIT_DIR_HIT_MOUNT_POINT:\n-\t\tif (nongit_ok) {\n-\t\t\t*nongit_ok = 1;\n-\t\t\tstrbuf_release(&cwd);\n-\t\t\tstrbuf_release(&dir);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n-\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n-\t\t dir.buf);\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n+\t\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n+\t\t\t\t\tdir.buf);\n+\t\t*nongit_ok = 1;\n+\t\tstrbuf_release(&dir);\n+\t\treturn NULL;\n \tdefault:\n \t\tBUG(\"unhandled setup_git_directory_1() result\");\n \t}\n-- \n2.7.4\n\n"},{"id":"365526","messageId":"nycvar.QRO.7.76.6.1812181332370.43@tvgsbejvaqbjf.bet","threadId":"50030","inReplyTo":"1544922308-740-1-git-send-email-eedahlgren@gmail.com","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-18T12:35:13Z","receivedAt":"2018-12-18T12:35:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Erin,\n\nOn Sat, 15 Dec 2018, Erin Dahlgren wrote:\n\n> diff --git a/setup.c b/setup.c\n> index 1be5037..e1a9e17 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n> \treturn NULL;\n> }\n> \n> -static const char *setup_nongit(const char *cwd, int *nongit_ok)\n> -{\n> -\tif (!nongit_ok)\n> -\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n> -\tif (chdir(cwd))\n> -\t\tdie_errno(_(\"cannot come back to cwd\"));\n> -\t*nongit_ok = 1;\n> -\treturn NULL;\n> -}\n> -\n> static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n> {\n> \tstruct stat buf;\n> @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t\tprefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n> \t\tbreak;\n> \tcase GIT_DIR_HIT_CEILING:\n> -\t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n> -\t\tbreak;\n> +\t\tif (!nongit_ok)\n> +\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n> +\t\t\t\t\tDEFAULT_GIT_DIR_ENVIRONMENT);\n\nI am terribly sorry to bother you about formatting issues (in my mind, it\nis quite an annoying thing that we still have no fully automatic way to\nformat Git's source code according to Git's preferred coding style, but\nthere you go...): this `DEFAULT_GIT_DIR_ENVIRONMENT` should be aligned\nwith the first parameter of `die()`, i.e.\n\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n+\t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n\n> +\t\t*nongit_ok = 1;\n> +\t\tstrbuf_release(&dir);\n> +\t\treturn NULL;\n> \tcase GIT_DIR_HIT_MOUNT_POINT:\n> -\t\tif (nongit_ok) {\n> -\t\t\t*nongit_ok = 1;\n> -\t\t\tstrbuf_release(&cwd);\n> -\t\t\tstrbuf_release(&dir);\n> -\t\t\treturn NULL;\n> -\t\t}\n> -\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> -\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> -\t\t dir.buf);\n> +\t\tif (!nongit_ok)\n> +\t\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> +\t\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> +\t\t\t\t\tdir.buf);\n\nLikewise, `dir.buf` should be aligned with the `_` two lines above.\n\nOtherwise I think this patch is good to go!\n\nThank you,\nJohannes\n\n> +\t\t*nongit_ok = 1;\n> +\t\tstrbuf_release(&dir);\n> +\t\treturn NULL;\n> \tdefault:\n> \t\tBUG(\"unhandled setup_git_directory_1() result\");\n> \t}\n> -- \n> 2.7.4\n> \n> \n"},{"id":"365546","messageId":"20181218175418.GB31070@sigill.intra.peff.net","threadId":"50030","inReplyTo":"1544922308-740-1-git-send-email-eedahlgren@gmail.com","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-18T17:54:18Z","receivedAt":"2018-12-18T17:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2018 at 05:05:08PM -0800, Erin Dahlgren wrote:\n\n> setup_git_directory_gently() expects two types of failures to\n> discover a git directory (e.g. .git/):\n> [...]\n\nWhen I read your subject line, I'll admit to having a funny feeling in\nthe pit of my stomach. This setup code has historically been full of\nsubtle corner cases, and I expected a very tricky review.\n\nHowever, your explanation was so well-written that it was a pleasure to\nread. :)\n\n> Before this change are two misleading additional behaviors:\n> \n> - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n> \tapparent reason. We never had the chance to change directories\n> \tup to this point so chdir(current cwd) is pointless.\n\nThat makes sense. I think this is a holdover from when we used to walk\nbackwards via chdir(), prior to ce9b8aab5d (setup_git_directory_1():\navoid changing global state, 2017-03-13). Back then, we needed to\nrestore the state at this point, but now we don't.\n\n> - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n> \tof a static struct strbuf (cwd). This is unnecessary because the\n> \tstruct is static so its buffer is always reachable. This is also\n> \tmisleading because nowhere else in the function is this buffer\n> \treleased.\n\nMakes sense.\n\nI do have one question, though:\n\n> \tcase GIT_DIR_HIT_CEILING:\n> -\t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n> -\t\tbreak;\n> +\t\tif (!nongit_ok)\n> +\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n>a +\t\t\t\t\tDEFAULT_GIT_DIR_ENVIRONMENT);\n> +\t\t*nongit_ok = 1;\n> +\t\tstrbuf_release(&dir);\n> +\t\treturn NULL;\n\nThis case used to break out of the switch, and now returns early.\n\nSo we do not execute the later code which clears GIT_PREFIX_ENVIRONMENT,\nand zeroes the fields in startup_info. Those probably don't matter in\nmost cases, but I wonder if there are some obscure ones where it might.\n\nMight it make sense to make GIT_DIR_HIT_MOUNT_POINT more like\nGIT_DIR_HIT_CEILING currently is, rather than the other way around?\nI.e., something like:\n\n case GIT_DIR_HIT_CEILING:\n\tif (!nongit_ok)\n\t\tdie(...);\n\t*nongit_ok = 1;\n\tprefix = NULL;\n\tbreak;\n case GIT_DIR_HIT_MOUNT_POINT:\n\tif (!nongit_ok)\n\t\tdie(...);\n\t*nongit_ok = 1;\n\tprefix = NULL;\n\tbreak;\n\n?\n\n-Peff\n"},{"id":"365550","messageId":"CAP_Smy0sW0-DiUQD1vKbbjrCDRo+6WchwHqGu+sDXH8fsbiYEw@mail.gmail.com","threadId":"50030","inReplyTo":"nycvar.QRO.7.76.6.1812181332370.43@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-18T19:50:10Z","receivedAt":"2018-12-18T19:50:25Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"Sorry Johannes for the repeat message, I failed to send to all.\n\nOn Tue, Dec 18, 2018 at 4:35 AM Johannes Schindelin\n wrote:\n>\n> Hi Erin,\n>\n> On Sat, 15 Dec 2018, Erin Dahlgren wrote:\n>\n> > diff --git a/setup.c b/setup.c\n> > index 1be5037..e1a9e17 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n> > return NULL;\n> > }\n> >\n> > -static const char *setup_nongit(const char *cwd, int *nongit_ok)\n> > -{\n> > - if (!nongit_ok)\n> > - die(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n> > - if (chdir(cwd))\n> > - die_errno(_(\"cannot come back to cwd\"));\n> > - *nongit_ok = 1;\n> > - return NULL;\n> > -}\n> > -\n> > static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n> > {\n> > struct stat buf;\n> > @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> > prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n> > break;\n> > case GIT_DIR_HIT_CEILING:\n> > - prefix = setup_nongit(cwd.buf, nongit_ok);\n> > - break;\n> > + if (!nongit_ok)\n> > + die(_(\"not a git repository (or any of the parent directories): %s\"),\n> > + DEFAULT_GIT_DIR_ENVIRONMENT);\n>\n> I am terribly sorry to bother you about formatting issues (in my mind, it\n> is quite an annoying thing that we still have no fully automatic way to\n> format Git's source code according to Git's preferred coding style, but\n> there you go...): this `DEFAULT_GIT_DIR_ENVIRONMENT` should be aligned\n> with the first parameter of `die()`, i.e.\n>\n> + if (!nongit_ok)\n> + die(_(\"not a git repository (or any of the parent directories): %s\"),\n> + DEFAULT_GIT_DIR_ENVIRONMENT);\n>\n> > + *nongit_ok = 1;\n> > + strbuf_release(&dir);\n> > + return NULL;\n> > case GIT_DIR_HIT_MOUNT_POINT:\n> > - if (nongit_ok) {\n> > - *nongit_ok = 1;\n> > - strbuf_release(&cwd);\n> > - strbuf_release(&dir);\n> > - return NULL;\n> > - }\n> > - die(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> > - \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> > - dir.buf);\n> > + if (!nongit_ok)\n> > + die(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n> > + \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n> > + dir.buf);\n>\n> Likewise, `dir.buf` should be aligned with the `_` two lines above.\n\nHi Johannes,\n\nNo problem, I'll make those changes.\n\nI'd be interested to hear the arguments against a \"fully automatic way\nto format Git's source code according to Git's preferred coding\nstyle\". If there aren't any then I'd be willing to take a crack at it.\nShould we start a separate \"discussion\" thread?\n\n>\n> Otherwise I think this patch is good to go!\n>\n> Thank you,\n> Johannes\n>\n> > + *nongit_ok = 1;\n> > + strbuf_release(&dir);\n> > + return NULL;\n> > default:\n> > BUG(\"unhandled setup_git_directory_1() result\");\n> > }\n> > --\n> > 2.7.4\n> >\n> >\n"},{"id":"365551","messageId":"CAP_Smy14j4WK-mkqdKTKue=j7YoNjfaZVCBA-7S8xwNqX2rKhQ@mail.gmail.com","threadId":"50030","inReplyTo":"20181218175418.GB31070@sigill.intra.peff.net","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-18T20:54:02Z","receivedAt":"2018-12-18T20:54:17Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"Hi Peff,\n\nOn Tue, Dec 18, 2018 at 9:54 AM Jeff King wrote:\n>\n> On Sat, Dec 15, 2018 at 05:05:08PM -0800, Erin Dahlgren wrote:\n>\n> > setup_git_directory_gently() expects two types of failures to\n> > discover a git directory (e.g. .git/):\n> > [...]\n>\n> When I read your subject line, I'll admit to having a funny feeling in\n> the pit of my stomach. This setup code has historically been full of\n> subtle corner cases, and I expected a very tricky review.\n>\n> However, your explanation was so well-written that it was a pleasure to\n> read. :)\n\nThanks :)\n\n>\n> > Before this change are two misleading additional behaviors:\n> >\n> > - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n> > apparent reason. We never had the chance to change directories\n> > up to this point so chdir(current cwd) is pointless.\n>\n> That makes sense. I think this is a holdover from when we used to walk\n> backwards via chdir(), prior to ce9b8aab5d (setup_git_directory_1():\n> avoid changing global state, 2017-03-13). Back then, we needed to\n> restore the state at this point, but now we don't.\n>\n> > - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n> > of a static struct strbuf (cwd). This is unnecessary because the\n> > struct is static so its buffer is always reachable. This is also\n> > misleading because nowhere else in the function is this buffer\n> > released.\n>\n> Makes sense.\n>\n> I do have one question, though:\n>\n> > case GIT_DIR_HIT_CEILING:\n> > - prefix = setup_nongit(cwd.buf, nongit_ok);\n> > - break;\n> > + if (!nongit_ok)\n> > + die(_(\"not a git repository (or any of the parent directories): %s\"),\n> >a + DEFAULT_GIT_DIR_ENVIRONMENT);\n> > + *nongit_ok = 1;\n> > + strbuf_release(&dir);\n> > + return NULL;\n>\n> This case used to break out of the switch, and now returns early.\n>\n> So we do not execute the later code which clears GIT_PREFIX_ENVIRONMENT,\n> and zeroes the fields in startup_info. Those probably don't matter in\n> most cases, but I wonder if there are some obscure ones where it might.\n>\n> Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like\n> GIT_DIR_HIT_CEILING currently is, rather than the other way around?\n> I.e., something like:\n>\n> case GIT_DIR_HIT_CEILING:\n> if (!nongit_ok)\n> die(...);\n> *nongit_ok = 1;\n> prefix = NULL;\n> break;\n> case GIT_DIR_HIT_MOUNT_POINT:\n> if (!nongit_ok)\n> die(...);\n> *nongit_ok = 1;\n> prefix = NULL;\n> break;\n>\n> ?\n\nAfter some digging, there seems to be a documented guarantee that\n\"`GIT_PREFIX` is set as returned by running 'git rev-parse\n--show-prefix'\". See Documentation/config/alias.txt. If the\nenvironment variable GIT_PREFIX is already set to something and we\ndon't clear it when prefix is NULL, then the two can be out of sync.\nSo that's a problem with my patch and the current handling of\nGIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it\nis, but we should respect what's documented.\n\nSide note: One of the secondary goals of my patch was to make it\nreally obvious that neither the GIT_DIR_HIT_CEILING nor the\nGIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by\n(startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With\nyour suggestion (and it's a fair one) I don't think that's feasible\nwithout more significant refactoring. Should I just settle with a\ncomment that explains this?\n\nThanks,\nErin\n\n>\n> -Peff\n"},{"id":"365581","messageId":"20181219155928.GE14802@sigill.intra.peff.net","threadId":"50030","inReplyTo":"CAP_Smy14j4WK-mkqdKTKue=j7YoNjfaZVCBA-7S8xwNqX2rKhQ@mail.gmail.com","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-19T15:59:28Z","receivedAt":"2018-12-19T15:59:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote:\n\n> > Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like\n> > GIT_DIR_HIT_CEILING currently is, rather than the other way around?\n> > I.e., something like:\n> >\n> > case GIT_DIR_HIT_CEILING:\n> > if (!nongit_ok)\n> > die(...);\n> > *nongit_ok = 1;\n> > prefix = NULL;\n> > break;\n> > case GIT_DIR_HIT_MOUNT_POINT:\n> > if (!nongit_ok)\n> > die(...);\n> > *nongit_ok = 1;\n> > prefix = NULL;\n> > break;\n> >\n> > ?\n> \n> After some digging, there seems to be a documented guarantee that\n> \"`GIT_PREFIX` is set as returned by running 'git rev-parse\n> --show-prefix'\". See Documentation/config/alias.txt. If the\n> environment variable GIT_PREFIX is already set to something and we\n> don't clear it when prefix is NULL, then the two can be out of sync.\n> So that's a problem with my patch and the current handling of\n> GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it\n> is, but we should respect what's documented.\n\nYeah, agreed.\n\nAnother benefit of avoiding the early return is that we hit the cleanup\ncode at the end of the function. That saves us having to release \"dir\"\nhere. I assume we don't have to care about \"gitdir\" in these cases, but\nhitting the cleanup code means we don't even have to think about it.\n\nOver in:\n\n https://public-inbox.org/git/20181219154853.GC14802@sigill.intra.peff.net/\n\nI suggested adding more cleanup to that block, too (though I _think_ in\nthese cases it would also be a noop, it's again nice to not have to\nworry about it).\n\n> Side note: One of the secondary goals of my patch was to make it\n> really obvious that neither the GIT_DIR_HIT_CEILING nor the\n> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by\n> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With\n> your suggestion (and it's a fair one) I don't think that's feasible\n> without more significant refactoring. Should I just settle with a\n> comment that explains this?\n\nYeah, I think a comment would probably be sufficient. Though we could\nalso perhaps make the two cases (i.e., we found a repo or not) more\nclear. Something like:\n\n if (!*nongit_ok || !*nongit_ok) {\n\tstartup_info->have_repository = 1;\n\tstartup_info->prefix = prefix;\n\tif (prefix) ...etc...\n } else {\n\tstart_info->have_repository = 0;\n\tstartup_info->prefix = NULL;\n\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n }\n\nThat may introduce some slight repetition, but I think it's probably\nclearer to deal with the cases separately (and it saves earlier code\nfrom make-work like setting prefix=NULL when there's no repo).\n\nThe condition would also be a lot less confusing if nongit_ok were\nflipped, but I think that would be a big refactor due to the way we pass\nit around.\n\n-Peff\n"},{"id":"365819","messageId":"xmqqftukq66o.fsf@gitster-ct.c.googlers.com","threadId":"50030","inReplyTo":"20181219155928.GE14802@sigill.intra.peff.net","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-26T22:22:39Z","receivedAt":"2018-12-26T22:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote:\n> ...\n>> GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it\n>> is, but we should respect what's documented.\n>\n> Yeah, agreed.\n>\n> Another benefit of avoiding the early return is that we hit the cleanup\n> code at the end of the function. That saves us having to release \"dir\"\n> here. I assume we don't have to care about \"gitdir\" in these cases, but\n> hitting the cleanup code means we don't even have to think about it.\n>\n> Over in:\n>\n> https://public-inbox.org/git/20181219154853.GC14802@sigill.intra.peff.net/\n>\n> I suggested adding more cleanup to that block, too (though I _think_ in\n> these cases it would also be a noop, it's again nice to not have to\n> worry about it).\n>\n>> Side note: One of the secondary goals of my patch was to make it\n>> really obvious that neither the GIT_DIR_HIT_CEILING nor the\n>> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by\n>> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With\n>> your suggestion (and it's a fair one) I don't think that's feasible\n>> without more significant refactoring. Should I just settle with a\n>> comment that explains this?\n>\n> Yeah, I think a comment would probably be sufficient. Though we could\n> also perhaps make the two cases (i.e., we found a repo or not) more\n> clear. Something like:\n>\n> if (!*nongit_ok || !*nongit_ok) {\n\nWTH is (A || A)?\n\n> \tstartup_info->have_repository = 1;\n> \tstartup_info->prefix = prefix;\n> \tif (prefix) ...etc...\n> } else {\n> \tstart_info->have_repository = 0;\n> \tstartup_info->prefix = NULL;\n> \tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n> }\n>\n> That may introduce some slight repetition, but I think it's probably\n> clearer to deal with the cases separately (and it saves earlier code\n> from make-work like setting prefix=NULL when there's no repo).\n\nSounds good.\n\nThanks both.\n"},{"id":"365851","messageId":"20181227162417.GA23147@sigill.intra.peff.net","threadId":"50030","inReplyTo":"xmqqftukq66o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-27T16:24:17Z","receivedAt":"2018-12-27T16:24:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 26, 2018 at 02:22:39PM -0800, Junio C Hamano wrote:\n\n> >> Side note: One of the secondary goals of my patch was to make it\n> >> really obvious that neither the GIT_DIR_HIT_CEILING nor the\n> >> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by\n> >> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With\n> >> your suggestion (and it's a fair one) I don't think that's feasible\n> >> without more significant refactoring. Should I just settle with a\n> >> comment that explains this?\n> >\n> > Yeah, I think a comment would probably be sufficient. Though we could\n> > also perhaps make the two cases (i.e., we found a repo or not) more\n> > clear. Something like:\n> >\n> > if (!*nongit_ok || !*nongit_ok) {\n> \n> WTH is (A || A)?\n\nHeh, I should learn to cut and paste better. This should be:\n\n if (!nongit_ok || !*nongit_ok)\n\n(which comes from the current code).\n\n-Peff\n"},{"id":"365890","messageId":"1545953789-15040-1-git-send-email-eedahlgren@gmail.com","threadId":"50030","inReplyTo":"1544922308-740-1-git-send-email-eedahlgren@gmail.com","subject":"[PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-27T23:36:29Z","receivedAt":"2018-12-27T23:36:53Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"setup_git_directory_gently() expects two types of failures to\ndiscover a git directory (e.g. .git/):\n\n - GIT_DIR_HIT_CEILING: could not find a git directory in any\n\tparent directories of the cwd.\n - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in\n\tany parent directories up to the mount point of the cwd.\n\nBoth cases are handled in a similar way, but there are misleading\nand unimportant differences. In both cases, setup_git_directory_gently()\nshould:\n\n - Die if we are not in a git repository. Otherwise:\n - Set nongit_ok = 1, indicating that we are not in a git repository\n\tbut this is ok.\n - Call strbuf_release() on any non-static struct strbufs that we\n\tallocated.\n\nBefore this change are two misleading additional behaviors:\n\n - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n\tapparent reason. We never had the chance to change directories\n\tup to this point so chdir(current cwd) is pointless.\n - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n\tof a static struct strbuf (cwd). This is unnecessary because the\n\tstruct is static so its buffer is always reachable. This is also\n\tmisleading because nowhere else in the function is this buffer\n\treleased.\n\nThis change eliminates these two misleading additional behaviors and\ndeletes setup_nogit() because the code is clearer without it. The\nresult is that we can see clearly that GIT_DIR_HIT_CEILING and\nGIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the\ndifferent help messages).\n\nDuring review, this change was amended to additionally include:\n\n - Neither GIT_DIR_HIT_CEILING nor GIT_DIR_HIT_MOUNT_POINT may\n\treturn early from setup_git_directory_gently() before the\n\tGIT_PREFIX environment variable is reset. Change both cases to\n\tbreak instead of return. See GIT_PREFIX below for more details.\n\n - GIT_DIR_NONE: setup_git_directory_gently_1() never returns this\n\tvalue, but if it ever did, setup_git_directory_gently() would\n\tincorrectly record that it had found a repository. Explicitly\n\tBUG on this case because it is underspecified.\n\n - GIT_PREFIX: this environment variable must always match the\n\tvalue of startup_info->prefix and the prefix returned from\n\tsetup_git_directory_gently(). Make how we handle this slightly\n\tmore repetitive but also more clear.\n\n - setup_git_env() and repo_set_hash_algo(): Add comments showing\n\tthat only GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, and GIT_DIR_BARE\n\twill cause setup_git_directory_gently() to call these setup\n\tfunctions. This was obvious (but partly incorrect) before this\n\tchange when GIT_DIR_HIT_MOUNT_POINT returned early from\n\tsetup_git_directory_gently().\n---\nChanges in v3:\n\n - Re-aligned arguments to die() calls to match formatting convention.\n\n - Neither GIT_DIR_HIT_CEILING nor GIT_DIR_HIT_MOUNT_POINT may\n return early from setup_git_directory_gently() before the\n GIT_PREFIX environment variable is reset. Change both cases to\n break instead of return. See GIT_PREFIX below for more details.\n\n - GIT_DIR_NONE: setup_git_directory_gently_1() never returns this\n value, but if it ever did, setup_git_directory_gently() would\n incorrectly record that it had found a repository. Explicitly\n BUG on this case because it is underspecified.\n\n - GIT_PREFIX: this environment variable must always match the\n value of startup_info->prefix and the prefix returned from\n setup_git_directory_gently(). Make how we handle this slightly\n more repetitive but also more clear.\n\n - setup_git_env() and repo_set_hash_algo(): Add comments showing\n that only GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, and GIT_DIR_BARE\n will cause setup_git_directory_gently() to call these setup\n functions. This was obvious (but partly incorrect) before this\n change when GIT_DIR_HIT_MOUNT_POINT returned early from\n setup_git_directory_gently().\n\n setup.c | 75 ++++++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 44 insertions(+), 31 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037..eb8332b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n \treturn NULL;\n }\n \n-static const char *setup_nongit(const char *cwd, int *nongit_ok)\n-{\n-\tif (!nongit_ok)\n-\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"), DEFAULT_GIT_DIR_ENVIRONMENT);\n-\tif (chdir(cwd))\n-\t\tdie_errno(_(\"cannot come back to cwd\"));\n-\t*nongit_ok = 1;\n-\treturn NULL;\n-}\n-\n static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)\n {\n \tstruct stat buf;\n@@ -1054,7 +1044,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n {\n \tstatic struct strbuf cwd = STRBUF_INIT;\n \tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;\n-\tconst char *prefix;\n+\tconst char *prefix = NULL;\n \tstruct repository_format repo_fmt;\n \n \t/*\n@@ -1079,9 +1069,6 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tstrbuf_addbuf(&dir, &cwd);\n \n \tswitch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {\n-\tcase GIT_DIR_NONE:\n-\t\tprefix = NULL;\n-\t\tbreak;\n \tcase GIT_DIR_EXPLICIT:\n \t\tprefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);\n \t\tbreak;\n@@ -1097,29 +1084,52 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tprefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n \t\tbreak;\n \tcase GIT_DIR_HIT_CEILING:\n-\t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n+\t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n+\t\t*nongit_ok = 1;\n \t\tbreak;\n \tcase GIT_DIR_HIT_MOUNT_POINT:\n-\t\tif (nongit_ok) {\n-\t\t\t*nongit_ok = 1;\n-\t\t\tstrbuf_release(&cwd);\n-\t\t\tstrbuf_release(&dir);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n-\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n-\t\t dir.buf);\n+\t\tif (!nongit_ok)\n+\t\t\tdie(_(\"not a git repository (or any parent up to mount point %s)\\n\"\n+\t\t\t \"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\"),\n+\t\t\t dir.buf);\n+\t\t*nongit_ok = 1;\n+\t\tbreak;\n+\tcase GIT_DIR_NONE:\n+\t\t/*\n+\t\t * As a safeguard against setup_git_directory_gently_1 returning\n+\t\t * this value, fallthrough to BUG. Otherwise it is possible to\n+\t\t * set startup_info->have_repository to 1 when we did nothing to\n+\t\t * find a repository.\n+\t\t */\n \tdefault:\n \t\tBUG(\"unhandled setup_git_directory_1() result\");\n \t}\n \n-\tif (prefix)\n-\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n-\telse\n+\t/*\n+\t * At this point, nongit_ok is stable. If it is non-NULL and points\n+\t * to a non-zero value, then this means that we haven't found a\n+\t * repository and that the caller expects startup_info to reflect\n+\t * this.\n+\t *\n+\t * Regardless of the state of nongit_ok, startup_info->prefix and\n+\t * the GIT_PREFIX environment variable must always match. For details\n+\t * see Documentation/config/alias.txt.\n+\t */\n+\tif (nongit_ok && *nongit_ok) {\n+\t\tstartup_info->have_repository = 0;\n+\t\tstartup_info->prefix = NULL;\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n-\n-\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n-\tstartup_info->prefix = prefix;\n+\t} else {\n+\t\t// !nongit_ok || !*nongit_ok\n+\t\tstartup_info->have_repository = 1;\n+\t\tstartup_info->prefix = prefix;\n+\t\tif (prefix)\n+\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n+\t\telse\n+\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n+\t}\n \n \t/*\n \t * Not all paths through the setup code will call 'set_git_dir()' (which\n@@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t * the user has set GIT_DIR. It may be beneficial to disallow bogus\n \t * GIT_DIR values at some point in the future.\n \t */\n-\tif (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n+\tif (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n+\t startup_info->have_repository ||\n+\t // GIT_DIR_EXPLICIT\n+\t getenv(GIT_DIR_ENVIRONMENT)) {\n \t\tif (!the_repository->gitdir) {\n \t\t\tconst char *gitdir = getenv(GIT_DIR_ENVIRONMENT);\n \t\t\tif (!gitdir)\n-- \n2.7.4\n\n"},{"id":"365893","messageId":"CAP_Smy0tMe=mq2u6OFBfYzutHvoLETOyRtFEzLVViupjMLVLrA@mail.gmail.com","threadId":"50030","inReplyTo":"20181227162417.GA23147@sigill.intra.peff.net","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2018-12-27T23:46:10Z","receivedAt":"2018-12-27T23:46:26Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"On Thu, Dec 27, 2018 at 8:24 AM Jeff King wrote:\n>\n> On Wed, Dec 26, 2018 at 02:22:39PM -0800, Junio C Hamano wrote:\n>\n> > >> Side note: One of the secondary goals of my patch was to make it\n> > >> really obvious that neither the GIT_DIR_HIT_CEILING nor the\n> > >> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by\n> > >> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With\n> > >> your suggestion (and it's a fair one) I don't think that's feasible\n> > >> without more significant refactoring. Should I just settle with a\n> > >> comment that explains this?\n> > >\n> > > Yeah, I think a comment would probably be sufficient. Though we could\n> > > also perhaps make the two cases (i.e., we found a repo or not) more\n> > > clear. Something like:\n> > >\n> > > if (!*nongit_ok || !*nongit_ok) {\n> >\n> > WTH is (A || A)?\n>\n> Heh, I should learn to cut and paste better. This should be:\n>\n> if (!nongit_ok || !*nongit_ok)\n>\n> (which comes from the current code).\n\nYep, but I think we can benefit from De Morgan's law here, where:\n\n (!nongit_ok || !*nongit_ok) == (!(nongit_ok && *nongit_ok))\n\nPATCH v3 (just sent) uses that transformation like this:\n\nif (nongit_ok && *nongit_ok) {\n ... startup_info->has_repository = 0;\n} else {\n // !nongit_ok || !*nongit_ok\n .. startup_info->has_repository = 1;\n}\n\nBecause IMHO (nongit_ok && *nongit_ok) is easier to read and reason\nabout. Added brief comments as well.\n\nThanks.\n\n- Erin\n\n>\n> -Peff\n"},{"id":"366065","messageId":"20190103045456.GD20047@sigill.intra.peff.net","threadId":"50030","inReplyTo":"CAP_Smy0tMe=mq2u6OFBfYzutHvoLETOyRtFEzLVViupjMLVLrA@mail.gmail.com","subject":"Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-03T04:54:57Z","receivedAt":"2019-01-03T04:55:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 27, 2018 at 03:46:10PM -0800, Erin Dahlgren wrote:\n\n> > Heh, I should learn to cut and paste better. This should be:\n> >\n> > if (!nongit_ok || !*nongit_ok)\n> >\n> > (which comes from the current code).\n> \n> Yep, but I think we can benefit from De Morgan's law here, where:\n> \n> (!nongit_ok || !*nongit_ok) == (!(nongit_ok && *nongit_ok))\n> \n> PATCH v3 (just sent) uses that transformation like this:\n> \n> if (nongit_ok && *nongit_ok) {\n> ... startup_info->has_repository = 0;\n> } else {\n> // !nongit_ok || !*nongit_ok\n> .. startup_info->has_repository = 1;\n> }\n> \n> Because IMHO (nongit_ok && *nongit_ok) is easier to read and reason\n> about. Added brief comments as well.\n\nAh yes, that's much better.\n\n-Peff\n"},{"id":"366066","messageId":"20190103051432.GE20047@sigill.intra.peff.net","threadId":"50030","inReplyTo":"1545953789-15040-1-git-send-email-eedahlgren@gmail.com","subject":"Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-03T05:14:33Z","receivedAt":"2019-01-03T05:14:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 27, 2018 at 03:36:29PM -0800, Erin Dahlgren wrote:\n\n> Before this change are two misleading additional behaviors:\n> \n> - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no\n> \tapparent reason. We never had the chance to change directories\n> \tup to this point so chdir(current cwd) is pointless.\n> - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer\n> \tof a static struct strbuf (cwd). This is unnecessary because the\n> \tstruct is static so its buffer is always reachable. This is also\n> \tmisleading because nowhere else in the function is this buffer\n> \treleased.\n> [..]\n\nOverall this looks good to me, and I'd be fine to see it go in as-is.\n\nA few minor nits/comments, though:\n\n> During review, this change was amended to additionally include:\n> [...]\n\nWhen I find myself writing big bullet lists of changes in a commit\nmessage, that is often a good time to split the commit into several\nsub-parts.\n\nThis patch isn't _too_ big, so I don't think it's worth the effort at\nthis point for this particular case, but just something to think about\nfor the future. A series around this topic would probably be something\nlike:\n\n 1: drop the useless chdir and fold setup_nongit() into the main\n function\n\n 2: stop doing the early return from HIT_MOUNT_POINT, and treat it just\n like HIT_CEILING (and drop the useless strbuf release)\n\n 3: treat GIT_DIR_NONE as a BUG\n\n 4: rearrange the nongit logic at the end of the function for clarity\n\n> +\t/*\n> +\t * At this point, nongit_ok is stable. If it is non-NULL and points\n> +\t * to a non-zero value, then this means that we haven't found a\n> +\t * repository and that the caller expects startup_info to reflect\n> +\t * this.\n\nRight, makes sense.\n\n> +\t *\n> +\t * Regardless of the state of nongit_ok, startup_info->prefix and\n> +\t * the GIT_PREFIX environment variable must always match. For details\n> +\t * see Documentation/config/alias.txt.\n> +\t */\n\nI think this \"must match\" makes sense to comment. I didn't find\nconfig/alias.txt particularly enlightening in that regard, though. :)\n\n> +\tif (nongit_ok && *nongit_ok) {\n> +\t\tstartup_info->have_repository = 0;\n> +\t\tstartup_info->prefix = NULL;\n> \t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n> -\n> -\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n> -\tstartup_info->prefix = prefix;\n> +\t} else {\n> +\t\t// !nongit_ok || !*nongit_ok\n> +\t\tstartup_info->have_repository = 1;\n> +\t\tstartup_info->prefix = prefix;\n> +\t\tif (prefix)\n> +\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> +\t\telse\n> +\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n> +\t}\n\nWe usually avoid \"//\" comments, even for single lines (though that is\nperhaps superstition at this point, as we've started to embrace several\nother C99-isms). IMHO this particular comment is not even really\nnecessary, as the whole conditional is suitably short and obvious after\nyour refactor.\n\n> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t * the user has set GIT_DIR. It may be beneficial to disallow bogus\n> \t * GIT_DIR values at some point in the future.\n> \t */\n> -\tif (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n> +\tif (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n> +\t startup_info->have_repository ||\n> +\t // GIT_DIR_EXPLICIT\n> +\t getenv(GIT_DIR_ENVIRONMENT)) {\n\nSame \"//\" style issue as above. I'm not sure how much value there is in\nrepeating the GIT_DIR_* cases here, as they're likely to grow out of\nsync with the switch() statement above.\n\nAt first I thought this could all be folded into the \"else\" clause of\nthe conditional above (which would make the logic much easier to\nfollow), but that wouldn't cover the case of GIT_DIR=/bogus, which is\nwhat that getenv() is trying to handle here.\n\nSo I think this is the best we can do for now.\n\n-Peff\n"},{"id":"366095","messageId":"xmqqy381haup.fsf@gitster-ct.c.googlers.com","threadId":"50030","inReplyTo":"20190103051432.GE20047@sigill.intra.peff.net","subject":"Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-03T18:09:18Z","receivedAt":"2019-01-03T18:09:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\n>Subject: Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.\n\nPerhaps\n\n\tSubject: setup: simplify setup_git_directory_gently() failure cases\n\nto clarify which part of the entire world this patch is touching.\n\nJeff King writes:\n\n> This patch isn't _too_ big, so I don't think it's worth the effort at\n> this point for this particular case, but just something to think about\n> for the future. A series around this topic would probably be something\n> like:\n>\n> 1: drop the useless chdir and fold setup_nongit() into the main\n> function\n>\n> 2: stop doing the early return from HIT_MOUNT_POINT, and treat it just\n> like HIT_CEILING (and drop the useless strbuf release)\n>\n> 3: treat GIT_DIR_NONE as a BUG\n>\n> 4: rearrange the nongit logic at the end of the function for clarity\n\nYeah, that organization does make sense. And I agree with your rule\nof thumb to use the length and complexity of the log message to\njudge if a single step is getting too big.\n\n> We usually avoid \"//\" comments, even for single lines (though that is\n> perhaps superstition at this point, as we've started to embrace several\n> other C99-isms). IMHO this particular comment is not even really\n> necessary, as the whole conditional is suitably short and obvious after\n> your refactor.\n\nFWIW \"git grep\" for // seems to show that we reserve use of //\ninside commented out code samples.\n\nI also agree the comments behind // in this patch are probably\nunneeded.\n\n>\n>> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>> \t * the user has set GIT_DIR. It may be beneficial to disallow bogus\n>> \t * GIT_DIR values at some point in the future.\n>> \t */\n>> -\tif (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n>> +\tif (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n>> +\t startup_info->have_repository ||\n>> +\t // GIT_DIR_EXPLICIT\n>> +\t getenv(GIT_DIR_ENVIRONMENT)) {\n>\n> Same \"//\" style issue as above. I'm not sure how much value there is in\n> repeating the GIT_DIR_* cases here, as they're likely to grow out of\n> sync with the switch() statement above.\n\nIt is unclear to me if the original code is doing the right thing\nunder one condition, and this patch does not seem to change the\nbehaviour.\n\nWhat happens if GIT_DIR environment is set to an invalid path and\nnongit_ok is non-NULL? setup_explicit_git_dir() would have assigned\n1 to *nongit_ok, so have_repository is false at this point.\n\nWe enter the if() statement in such a case, and end up calling\nsetup_git_env(gitdir) using the bogus path that is not pointing at a\nrepository. We leave have_repository to be false but the paths\nrecorded in the_repository by setup_git_env() would all point at\nbogus places.\n\n> At first I thought this could all be folded into the \"else\" clause of\n> the conditional above (which would make the logic much easier to\n> follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is\n> what that getenv() is trying to handle here.\n\nYes, but should GIT_DIR=/bogus even be touching the_repository? \n\nIt is a separate clean-up and does not affect the validity of this\nsimplification patchd, so I agreee with ...\n\n> So I think this is the best we can do for now.\n\nThanks.\n"},{"id":"366133","messageId":"20190104082558.GB26014@sigill.intra.peff.net","threadId":"50030","inReplyTo":"xmqqy381haup.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-04T08:25:58Z","receivedAt":"2019-01-04T08:26:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 03, 2019 at 10:09:18AM -0800, Junio C Hamano wrote:\n\n> >> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> >> \t * the user has set GIT_DIR. It may be beneficial to disallow bogus\n> >> \t * GIT_DIR values at some point in the future.\n> >> \t */\n> >> -\tif (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n> >> +\tif (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n> >> +\t startup_info->have_repository ||\n> >> +\t // GIT_DIR_EXPLICIT\n> >> +\t getenv(GIT_DIR_ENVIRONMENT)) {\n> >\n> > Same \"//\" style issue as above. I'm not sure how much value there is in\n> > repeating the GIT_DIR_* cases here, as they're likely to grow out of\n> > sync with the switch() statement above.\n> \n> It is unclear to me if the original code is doing the right thing\n> under one condition, and this patch does not seem to change the\n> behaviour.\n>\n> What happens if GIT_DIR environment is set to an invalid path and\n> nongit_ok is non-NULL? setup_explicit_git_dir() would have assigned\n> 1 to *nongit_ok, so have_repository is false at this point.\n> \n> We enter the if() statement in such a case, and end up calling\n> setup_git_env(gitdir) using the bogus path that is not pointing at a\n> repository. We leave have_repository to be false but the paths\n> recorded in the_repository by setup_git_env() would all point at\n> bogus places.\n\nYeah, that matches my analysis of what the code is doing.\n\nLooks like this code (and comment) come from 73f192c991 (setup: don't\nperform lazy initialization of repository state, 2017-06-20). I'm\nguessing this was mostly a hack to quiet some test that complained about\nthe other changes in that commit. And indeed, dropping the getenv() half\nof the conditional and running the tests in 73f192c991 gets me a failure\nin t1050, which does:\n\n GIT_DIR=non-existent git index-pack --strict --verify foo/.git/objects/pack/*.pack\n\nBut that's a bug in index-pack, which should be able to verify a pack\noutside of a repository. And I think (according to bisection) we fixed\nthat in 14a9bd2898 (prepare_commit_graft: treat non-repository as a\nnoop, 2018-05-31), and the tests currently pass even with this patch\napplied:\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037f12..e2c03e9bbc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1132,7 +1132,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t * the user has set GIT_DIR. It may be beneficial to disallow bogus\n \t * GIT_DIR values at some point in the future.\n \t */\n-\tif (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n+\tif (startup_info->have_repository) {\n \t\tif (!the_repository->gitdir) {\n \t\t\tconst char *gitdir = getenv(GIT_DIR_ENVIRONMENT);\n \t\t\tif (!gitdir)\n\n> > At first I thought this could all be folded into the \"else\" clause of\n> > the conditional above (which would make the logic much easier to\n> > follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is\n> > what that getenv() is trying to handle here.\n> \n> Yes, but should GIT_DIR=/bogus even be touching the_repository? \n\nProbably not. And from my poking around above, I think we're probably\nsafe to remove this hackery now.\n\n> It is a separate clean-up and does not affect the validity of this\n> simplification patchd, so I agreee with ...\n> \n> > So I think this is the best we can do for now.\n\nYep, it's definitely orthogonal. But if we do this cleanup as part of\nit, we should be able to simplify further on top of Erin's patch, like\nthis:\n\ndiff --git a/setup.c b/setup.c\nindex eb8332bc02..edf65c44bf 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1125,35 +1125,26 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t// !nongit_ok || !*nongit_ok\n \t\tstartup_info->have_repository = 1;\n \t\tstartup_info->prefix = prefix;\n+\n \t\tif (prefix)\n \t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n \t\telse\n \t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n-\t}\n \n-\t/*\n-\t * Not all paths through the setup code will call 'set_git_dir()' (which\n-\t * directly sets up the environment) so in order to guarantee that the\n-\t * environment is in a consistent state after setup, explicitly setup\n-\t * the environment if we have a repository.\n-\t *\n-\t * NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some\n-\t * code paths so we also need to explicitly setup the environment if\n-\t * the user has set GIT_DIR. It may be beneficial to disallow bogus\n-\t * GIT_DIR values at some point in the future.\n-\t */\n-\tif (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n-\t startup_info->have_repository ||\n-\t // GIT_DIR_EXPLICIT\n-\t getenv(GIT_DIR_ENVIRONMENT)) {\n+\t\t/*\n+\t\t * Not all paths through the setup code will call 'set_git_dir()' (which\n+\t\t * directly sets up the environment) so in order to guarantee that the\n+\t\t * environment is in a consistent state after setup, explicitly setup\n+\t\t * the environment if we have a repository.\n+\t\t */\n \t\tif (!the_repository->gitdir) {\n \t\t\tconst char *gitdir = getenv(GIT_DIR_ENVIRONMENT);\n \t\t\tif (!gitdir)\n \t\t\t\tgitdir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\tsetup_git_env(gitdir);\n \t\t}\n-\t\tif (startup_info->have_repository)\n-\t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n+\n+\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n \n \tstrbuf_release(&dir);\n\nI actually wonder if the \"not all paths...\" bit is even still true these\ndays (and if it is, I think we should consider fixing those code paths).\nCertainly that setup_git_env() needed to trigger for the GIT_DIR=bogus,\nbut with that gone, any $GIT_DIR should have triggered GIT_DIR_EXPLICIT,\nand any \"default to .git\" should be handled as part of discovery, I'd\nthink.\n\nSo what next? Erin, are you interested in using the details of this\nconversation to take the cleanups a bit further? If not, then I can try\nto prepare a few patches on top of yours.\n\n-Peff\n"},{"id":"366177","messageId":"CAP_Smy0Phh83XW-m0_cGg6WuLpDySshP7Ys2OrD8no1hJzohpg@mail.gmail.com","threadId":"50030","inReplyTo":"20190104082558.GB26014@sigill.intra.peff.net","subject":"Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Erin Dahlgren","fromEmail":"eedahlgren@gmail.com","sentAt":"2019-01-05T16:57:32Z","receivedAt":"2019-01-05T16:57:49Z","isPatch":true,"sender":{"key":"eedahlgren@gmail.com","avatar":null},"body":"On Fri, Jan 4, 2019 at 12:26 AM Jeff King wrote:\n>\n> On Thu, Jan 03, 2019 at 10:09:18AM -0800, Junio C Hamano wrote:\n>\n> > >> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> > >> * the user has set GIT_DIR. It may be beneficial to disallow bogus\n> > >> * GIT_DIR values at some point in the future.\n> > >> */\n> > >> - if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n> > >> + if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n> > >> + startup_info->have_repository ||\n> > >> + // GIT_DIR_EXPLICIT\n> > >> + getenv(GIT_DIR_ENVIRONMENT)) {\n> > >\n> > > Same \"//\" style issue as above. I'm not sure how much value there is in\n> > > repeating the GIT_DIR_* cases here, as they're likely to grow out of\n> > > sync with the switch() statement above.\n> >\n> > It is unclear to me if the original code is doing the right thing\n> > under one condition, and this patch does not seem to change the\n> > behaviour.\n> >\n> > What happens if GIT_DIR environment is set to an invalid path and\n> > nongit_ok is non-NULL? setup_explicit_git_dir() would have assigned\n> > 1 to *nongit_ok, so have_repository is false at this point.\n> >\n> > We enter the if() statement in such a case, and end up calling\n> > setup_git_env(gitdir) using the bogus path that is not pointing at a\n> > repository. We leave have_repository to be false but the paths\n> > recorded in the_repository by setup_git_env() would all point at\n> > bogus places.\n>\n> Yeah, that matches my analysis of what the code is doing.\n>\n> Looks like this code (and comment) come from 73f192c991 (setup: don't\n> perform lazy initialization of repository state, 2017-06-20). I'm\n> guessing this was mostly a hack to quiet some test that complained about\n> the other changes in that commit. And indeed, dropping the getenv() half\n> of the conditional and running the tests in 73f192c991 gets me a failure\n> in t1050, which does:\n>\n> GIT_DIR=non-existent git index-pack --strict --verify foo/.git/objects/pack/*.pack\n>\n> But that's a bug in index-pack, which should be able to verify a pack\n> outside of a repository. And I think (according to bisection) we fixed\n> that in 14a9bd2898 (prepare_commit_graft: treat non-repository as a\n> noop, 2018-05-31), and the tests currently pass even with this patch\n> applied:\n>\n> diff --git a/setup.c b/setup.c\n> index 1be5037f12..e2c03e9bbc 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1132,7 +1132,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> * the user has set GIT_DIR. It may be beneficial to disallow bogus\n> * GIT_DIR values at some point in the future.\n> */\n> - if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {\n> + if (startup_info->have_repository) {\n> if (!the_repository->gitdir) {\n> const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);\n> if (!gitdir)\n>\n> > > At first I thought this could all be folded into the \"else\" clause of\n> > > the conditional above (which would make the logic much easier to\n> > > follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is\n> > > what that getenv() is trying to handle here.\n> >\n> > Yes, but should GIT_DIR=/bogus even be touching the_repository?\n>\n> Probably not. And from my poking around above, I think we're probably\n> safe to remove this hackery now.\n>\n> > It is a separate clean-up and does not affect the validity of this\n> > simplification patchd, so I agreee with ...\n> >\n> > > So I think this is the best we can do for now.\n>\n> Yep, it's definitely orthogonal. But if we do this cleanup as part of\n> it, we should be able to simplify further on top of Erin's patch, like\n> this:\n>\n> diff --git a/setup.c b/setup.c\n> index eb8332bc02..edf65c44bf 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1125,35 +1125,26 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> // !nongit_ok || !*nongit_ok\n> startup_info->have_repository = 1;\n> startup_info->prefix = prefix;\n> +\n> if (prefix)\n> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> else\n> setenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n> - }\n>\n> - /*\n> - * Not all paths through the setup code will call 'set_git_dir()' (which\n> - * directly sets up the environment) so in order to guarantee that the\n> - * environment is in a consistent state after setup, explicitly setup\n> - * the environment if we have a repository.\n> - *\n> - * NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some\n> - * code paths so we also need to explicitly setup the environment if\n> - * the user has set GIT_DIR. It may be beneficial to disallow bogus\n> - * GIT_DIR values at some point in the future.\n> - */\n> - if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE\n> - startup_info->have_repository ||\n> - // GIT_DIR_EXPLICIT\n> - getenv(GIT_DIR_ENVIRONMENT)) {\n> + /*\n> + * Not all paths through the setup code will call 'set_git_dir()' (which\n> + * directly sets up the environment) so in order to guarantee that the\n> + * environment is in a consistent state after setup, explicitly setup\n> + * the environment if we have a repository.\n> + */\n> if (!the_repository->gitdir) {\n> const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);\n> if (!gitdir)\n> gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;\n> setup_git_env(gitdir);\n> }\n> - if (startup_info->have_repository)\n> - repo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n> +\n> + repo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n> }\n>\n> strbuf_release(&dir);\n>\n> I actually wonder if the \"not all paths...\" bit is even still true these\n> days (and if it is, I think we should consider fixing those code paths).\n> Certainly that setup_git_env() needed to trigger for the GIT_DIR=bogus,\n> but with that gone, any $GIT_DIR should have triggered GIT_DIR_EXPLICIT,\n> and any \"default to .git\" should be handled as part of discovery, I'd\n> think.\n>\n> So what next? Erin, are you interested in using the details of this\n> conversation to take the cleanups a bit further?\n\nSure, no problem. If this is urgent, then I would probably be more\ninclined to keep this small and do more cleanup in followup patches.\nBut if it's not urgent (that is my understanding), I'd be happy to\ntake the cleanups further. I'm traveling today through next week but\nI'll try to post another version addressing the comments in the next\ncouple of days.\n\nThanks all for the comments so far.\n\n- Erin\n\n> If not, then I can try to prepare a few patches on top of yours.\n>\n> -Peff\n"},{"id":"366203","messageId":"20190106062229.GB4207@sigill.intra.peff.net","threadId":"50030","inReplyTo":"CAP_Smy0Phh83XW-m0_cGg6WuLpDySshP7Ys2OrD8no1hJzohpg@mail.gmail.com","subject":"Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-06T06:22:30Z","receivedAt":"2019-01-06T06:27:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 05, 2019 at 08:57:32AM -0800, Erin Dahlgren wrote:\n\n> > So what next? Erin, are you interested in using the details of this\n> > conversation to take the cleanups a bit further?\n> \n> Sure, no problem. If this is urgent, then I would probably be more\n> inclined to keep this small and do more cleanup in followup patches.\n> But if it's not urgent (that is my understanding), I'd be happy to\n> take the cleanups further. I'm traveling today through next week but\n> I'll try to post another version addressing the comments in the next\n> couple of days.\n\nNope, I don't think it's urgent at all. I just didn't want to dump work\non you if you were losing interest in the topic. :)\n\n-Peff\n"}]}