{"thread":{"id":"51564","subject":"[PATCH 0/1] Make the includeif:onbranch feature more robust","startedAt":"2019-07-31T19:53:55Z","lastAt":"2019-08-08T19:48:49Z","messageCount":17,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Johannes Schindelin","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"379705","messageId":"pull.300.git.gitgitgadget@gmail.com","threadId":"51564","inReplyTo":null,"subject":"[PATCH 0/1] Make the includeif:onbranch feature more robust","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-31T19:53:51Z","receivedAt":"2019-07-31T19:53:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I actually stumbled over this while rebasing the VFS for Git patches, in\nrelation with the pre-command hook that we still support there (we're slowly\nmigrating toward Trace2, of course).\n\nOnce I figured out what the problem was, I hunted for a way to trigger this\nbug in plain Git, and git help -a is the one I settled on. It had to be a\ncommand that uses the early config machinery (and git help -a uses it to\nlist all aliases), and it had to be a command that does not discover a .git \ndirectory automatically (cmd_help is listed without any flags in git.c, so:\ncheck).\n\nOf course, part of me wants to just go and dig into the refs part of the\ncode to introduce an equivalent to the \"early config\" machinery (calling it\n\"early ref store\"), but:\n\n 1. We're really in feature freeze, and I want this bug fix to go into\n    v2.23.0.\n 2. It is actually a pretty obscure thing to want: a branch-dependent config\n    that is used that early that the Git directory was not yet discovered. I \n    could imagine that some power user wants to play some games at some\n    stage, say, with the pager depending on the name of the current branch,\n    but even then, to run into the issue with this here patch where it\n    simply ignores the includeif.onbranch: setting in the early config code\n    path, a command has to be run that does not immediately set up \n    the_repository->gitdir but still wants to use the configured pager.\n\nSo yes, this patch introduces a known issue, but it does fix a BUG() where\nno bug should be reported.\n\nJohannes Schindelin (1):\n  config: work around bug with includeif:onbranch and early config\n\n config.c                | 3 ++-\n t/t1309-early-config.sh | 5 +++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\n\nbase-commit: 026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-300%2Fdscho%2Fonbranch-and-early-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-300/dscho/onbranch-and-early-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/300\n-- \ngitgitgadget\n"},{"id":"379706","messageId":"4aa88340132b03c9480c781cb5cb3c5b5513e944.1564602830.git.gitgitgadget@gmail.com","threadId":"51564","inReplyTo":"pull.300.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-31T19:53:51Z","receivedAt":"2019-07-31T19:53:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johasc@microsoft.com>\n\nSince 07b2c0eacac (config: learn the \"onbranch:\" includeIf condition,\n2019-06-05), there is a potential catch-22 in the early config path: if\nthe `include.onbranch:` feature is used, Git assumes that the Git\ndirectory has been initialized already. However, in the early config\ncode path that is not true.\n\nOne way to trigger this is to call the following commands in any\nrepository:\n\n\tgit config includeif.onbranch:refs/heads/master.path broken\n\tgit help -a\n\nThe symptom triggered by the `git help -a` invocation reads like this:\n\nBUG: refs.c:1851: attempting to get main_ref_store outside of repository\n\nLet's work around this, simply by ignoring the `includeif.onbranch:`\nsetting when parsing the config when the ref store has not been\ninitialized (yet).\n\nTechnically, there is a way to solve this properly: teach the refs\nmachinery to initialize the ref_store from a given gitdir/commondir pair\n(which we _do_ have in the early config code path), and then use that in\n`include_by_branch()`. This, however, is a pretty involved project, and\nwe're already in the feature freeze for Git v2.23.0.\n\nNote: when calling above-mentioned two commands _outside_ of any Git\nworktree (passing the `--global` flag to `git config`, as there is\nobviously no repository config available), at the point when\n`include_by_branch()` is called, `the_repository` is `NULL`, therefore\nwe have to be extra careful not to dereference it in that case.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n config.c                | 3 ++-\n t/t1309-early-config.sh | 5 +++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex ed7f58e0fc..3900e4947b 100644\n--- a/config.c\n+++ b/config.c\n@@ -275,7 +275,8 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \tint flags;\n \tint ret;\n \tstruct strbuf pattern = STRBUF_INIT;\n-\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n+\tconst char *refname = !the_repository || !the_repository->gitdir ?\n+\t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n \tconst char *shortname;\n \n \tif (!refname || !(flags & REF_ISSYMREF)\t||\ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex 413642aa56..0c37e7180d 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -89,4 +89,9 @@ test_expect_failure 'ignore .git/ with invalid config' '\n \ttest_with_config \"[\"\n '\n \n+test_expect_success 'early config and onbranch' '\n+\techo \"[broken\" >broken &&\n+\ttest_with_config \"[includeif \\\"onbranch:refs/heads/master\\\"]path=../broken\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"379711","messageId":"pull.300.v2.git.gitgitgadget@gmail.com","threadId":"51564","inReplyTo":"pull.300.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Make the includeif:onbranch feature more robust","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-31T20:06:41Z","receivedAt":"2019-07-31T20:06:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I actually stumbled over this while rebasing the VFS for Git patches, in\nrelation with the pre-command hook that we still support there (we're slowly\nmigrating toward Trace2, of course).\n\nOnce I figured out what the problem was, I hunted for a way to trigger this\nbug in plain Git, and git help -a is the one I settled on. It had to be a\ncommand that uses the early config machinery (and git help -a uses it to\nlist all aliases), and it had to be a command that does not discover a .git \ndirectory automatically (cmd_help is listed without any flags in git.c, so:\ncheck).\n\nOf course, part of me wants to just go and dig into the refs part of the\ncode to introduce an equivalent to the \"early config\" machinery (calling it\n\"early ref store\"), but:\n\n 1. We're really in feature freeze, and I want this bug fix to go into\n    v2.23.0.\n 2. It is actually a pretty obscure thing to want: a branch-dependent config\n    that is used that early that the Git directory was not yet discovered. I \n    could imagine that some power user wants to play some games at some\n    stage, say, with the pager depending on the name of the current branch,\n    but even then, to run into the issue with this here patch where it\n    simply ignores the includeif.onbranch: setting in the early config code\n    path, a command has to be run that does not immediately set up \n    the_repository->gitdir but still wants to use the configured pager.\n\nSo yes, this patch introduces a known issue, but it does fix a BUG() where\nno bug should be reported.\n\nChanges since v1:\n\n * Now using my correct email address for Git contributions.\n\nJohannes Schindelin (1):\n  config: work around bug with includeif:onbranch and early config\n\n config.c                | 3 ++-\n t/t1309-early-config.sh | 5 +++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\n\nbase-commit: 026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-300%2Fdscho%2Fonbranch-and-early-config-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-300/dscho/onbranch-and-early-config-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/300\n\nRange-diff vs v1:\n\n 1:  4aa8834013 ! 1:  ea1a746113 config: work around bug with includeif:onbranch and early config\n     @@ -1,4 +1,4 @@\n     -Author: Johannes Schindelin <johasc@microsoft.com>\n     +Author: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n          config: work around bug with includeif:onbranch and early config\n      \n\n-- \ngitgitgadget\n"},{"id":"379712","messageId":"ea1a746113b85bde5319c410f68fe3dc75f8a328.1564603600.git.gitgitgadget@gmail.com","threadId":"51564","inReplyTo":"pull.300.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-31T20:06:42Z","receivedAt":"2019-07-31T20:06:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nSince 07b2c0eacac (config: learn the \"onbranch:\" includeIf condition,\n2019-06-05), there is a potential catch-22 in the early config path: if\nthe `include.onbranch:` feature is used, Git assumes that the Git\ndirectory has been initialized already. However, in the early config\ncode path that is not true.\n\nOne way to trigger this is to call the following commands in any\nrepository:\n\n\tgit config includeif.onbranch:refs/heads/master.path broken\n\tgit help -a\n\nThe symptom triggered by the `git help -a` invocation reads like this:\n\nBUG: refs.c:1851: attempting to get main_ref_store outside of repository\n\nLet's work around this, simply by ignoring the `includeif.onbranch:`\nsetting when parsing the config when the ref store has not been\ninitialized (yet).\n\nTechnically, there is a way to solve this properly: teach the refs\nmachinery to initialize the ref_store from a given gitdir/commondir pair\n(which we _do_ have in the early config code path), and then use that in\n`include_by_branch()`. This, however, is a pretty involved project, and\nwe're already in the feature freeze for Git v2.23.0.\n\nNote: when calling above-mentioned two commands _outside_ of any Git\nworktree (passing the `--global` flag to `git config`, as there is\nobviously no repository config available), at the point when\n`include_by_branch()` is called, `the_repository` is `NULL`, therefore\nwe have to be extra careful not to dereference it in that case.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n config.c                | 3 ++-\n t/t1309-early-config.sh | 5 +++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex ed7f58e0fc..3900e4947b 100644\n--- a/config.c\n+++ b/config.c\n@@ -275,7 +275,8 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \tint flags;\n \tint ret;\n \tstruct strbuf pattern = STRBUF_INIT;\n-\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n+\tconst char *refname = !the_repository || !the_repository->gitdir ?\n+\t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n \tconst char *shortname;\n \n \tif (!refname || !(flags & REF_ISSYMREF)\t||\ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex 413642aa56..0c37e7180d 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -89,4 +89,9 @@ test_expect_failure 'ignore .git/ with invalid config' '\n \ttest_with_config \"[\"\n '\n \n+test_expect_success 'early config and onbranch' '\n+\techo \"[broken\" >broken &&\n+\ttest_with_config \"[includeif \\\"onbranch:refs/heads/master\\\"]path=../broken\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"379717","messageId":"xmqq7e7xq41u.fsf@gitster-ct.c.googlers.com","threadId":"51564","inReplyTo":"4aa88340132b03c9480c781cb5cb3c5b5513e944.1564602830.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-31T21:37:01Z","receivedAt":"2019-07-31T21:37:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> Let's work around this, simply by ignoring the `includeif.onbranch:`\n> setting when parsing the config when the ref store has not been\n> initialized (yet).\n\nInteresting chicken-and-egg problem.\n\n> Technically, there is a way to solve this properly: teach the refs\n> machinery to initialize the ref_store from a given gitdir/commondir pair\n> (which we _do_ have in the early config code path), and then use that in\n> `include_by_branch()`. This, however, is a pretty involved project, and\n> we're already in the feature freeze for Git v2.23.0.\n\nPostponing the proper fix to the next cycle (or later) is good.\n"},{"id":"379719","messageId":"20190731220204.GA1933@sigill.intra.peff.net","threadId":"51564","inReplyTo":"ea1a746113b85bde5319c410f68fe3dc75f8a328.1564603600.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T22:02:05Z","receivedAt":"2019-07-31T22:02:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2019 at 01:06:42PM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> Since 07b2c0eacac (config: learn the \"onbranch:\" includeIf condition,\n> 2019-06-05), there is a potential catch-22 in the early config path: if\n> the `include.onbranch:` feature is used, Git assumes that the Git\n> directory has been initialized already. However, in the early config\n> code path that is not true.\n> \n> One way to trigger this is to call the following commands in any\n> repository:\n> \n> \tgit config includeif.onbranch:refs/heads/master.path broken\n> \tgit help -a\n> \n> The symptom triggered by the `git help -a` invocation reads like this:\n> \n> BUG: refs.c:1851: attempting to get main_ref_store outside of repository\n> \n> Let's work around this, simply by ignoring the `includeif.onbranch:`\n> setting when parsing the config when the ref store has not been\n> initialized (yet).\n\nI think this \"fix the BUG() now, consider making more uses cases work\nlater\" is the right thing to do, and it matches many of the other\n\"oops, we are looking at repository bits while not in a repo\" fixes\nwe've done over the past few years.\n\n> Technically, there is a way to solve this properly: teach the refs\n> machinery to initialize the ref_store from a given gitdir/commondir pair\n> (which we _do_ have in the early config code path), and then use that in\n> `include_by_branch()`. This, however, is a pretty involved project, and\n> we're already in the feature freeze for Git v2.23.0.\n\nThis gets tricky, because some commands are intentionally avoiding the\nnormal lookup procedure (e.g., clone or init, and probably things like\nupload-pack that want to enter another repo). So I think it is OK as\nlong as the early-config code is explicitly saying \"and please look at\nthe refs in this specific direectory now\", and it doesn't affect other\npossible code paths that might look at refs. I _think_ that's what\nyou're suggesting above, but I just want to make sure (not that it\nmatters either way for this patch).\n\nAs a workaround, I suspect in many cases it might work to simply do a\ngentle setup (e.g., in \"help\"), but we simply weren't doing it before\nbecause there was no use case. That obviously wouldn't work for\nsomething like clone, though.\n\n> diff --git a/config.c b/config.c\n> index ed7f58e0fc..3900e4947b 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -275,7 +275,8 @@ static int include_by_branch(const char *cond, size_t cond_len)\n>  \tint flags;\n>  \tint ret;\n>  \tstruct strbuf pattern = STRBUF_INIT;\n> -\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n> +\tconst char *refname = !the_repository || !the_repository->gitdir ?\n> +\t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n\nI think the_repository is always non-NULL. The way similar sites check\nthis is with \"!startup_info->have_repository\" or have_git_dir(). The\nearly-config code uses the latter, so we should probably match it here.\n\n  Side note: I suspect there are some cleanup opportunities. IIRC, I had\n  to add have_git_dir() to cover some cases where $GIT_DIR was set but\n  we hadn't explicitly done a setup step, but there's been a lot of\n  refactoring and cleanup in the initialization code since then. I'm not\n  sure if it's still necessary.\n\n> diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\n> index 413642aa56..0c37e7180d 100755\n> --- a/t/t1309-early-config.sh\n> +++ b/t/t1309-early-config.sh\n> @@ -89,4 +89,9 @@ test_expect_failure 'ignore .git/ with invalid config' '\n>  \ttest_with_config \"[\"\n>  '\n>  \n> +test_expect_success 'early config and onbranch' '\n> +\techo \"[broken\" >broken &&\n> +\ttest_with_config \"[includeif \\\"onbranch:refs/heads/master\\\"]path=../broken\"\n> +'\n\nOK, so we know we didn't see the broken config because we'd have barfed\nif we actually included it. Makes sense.\n\n-Peff\n"},{"id":"379720","messageId":"nycvar.QRO.7.76.6.1908010004130.21907@tvgsbejvaqbjf.bet","threadId":"51564","inReplyTo":"20190731220204.GA1933@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-31T22:13:19Z","receivedAt":"2019-07-31T22:13:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 31 Jul 2019, Jeff King wrote:\n\n> On Wed, Jul 31, 2019 at 01:06:42PM -0700, Johannes Schindelin via GitGitGadget wrote:\n>\n> > Technically, there is a way to solve this properly: teach the refs\n> > machinery to initialize the ref_store from a given gitdir/commondir pair\n> > (which we _do_ have in the early config code path), and then use that in\n> > `include_by_branch()`. This, however, is a pretty involved project, and\n> > we're already in the feature freeze for Git v2.23.0.\n>\n> This gets tricky, because some commands are intentionally avoiding the\n> normal lookup procedure (e.g., clone or init, and probably things like\n> upload-pack that want to enter another repo). So I think it is OK as\n> long as the early-config code is explicitly saying \"and please look at\n> the refs in this specific direectory now\", and it doesn't affect other\n> possible code paths that might look at refs. I _think_ that's what\n> you're suggesting above, but I just want to make sure (not that it\n> matters either way for this patch).\n\nI think we already have the `git clone` problem with\n`includeif.gitdir:`. AFAICT we _will_ discover a Git directory when\ncloning inside an existing Git worktree.\n\n> As a workaround, I suspect in many cases it might work to simply do a\n> gentle setup (e.g., in \"help\"), but we simply weren't doing it before\n> because there was no use case. That obviously wouldn't work for\n> something like clone, though.\n\nRight.\n\nAnd as you say, there was no use case, and I would even contend that\nthere still is no use case. In the cover letter, I tried to concoct\nsomething (using a branch-dependent pager) that sounds _really_\nfar-fetched to even me.\n>\n> > diff --git a/config.c b/config.c\n> > index ed7f58e0fc..3900e4947b 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -275,7 +275,8 @@ static int include_by_branch(const char *cond, size_t cond_len)\n> >  \tint flags;\n> >  \tint ret;\n> >  \tstruct strbuf pattern = STRBUF_INIT;\n> > -\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n> > +\tconst char *refname = !the_repository || !the_repository->gitdir ?\n> > +\t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n>\n> I think the_repository is always non-NULL.\n\nNo, it totally can be `NULL`. I know because my first version of the\npatch did not have that extra check, and `git help -a` would segfault\noutside a Git worktree when I had an `includeif.onbranch:` in my\n`~/.gitconfig`.\n\nI tried to explain that in the commit message, but obviously did a poor\njob:\n\n    Note: when calling above-mentioned two commands _outside_ of any Git\n    worktree (passing the `--global` flag to `git config`, as there is\n    obviously no repository config available), at the point when\n    `include_by_branch()` is called, `the_repository` is `NULL`, therefore\n    we have to be extra careful not to dereference it in that case.\n\n> The way similar sites check this is withV\n> \"!startup_info->have_repository\" or have_git_dir(). The early-config\n> code uses the latter, so we should probably match it here.\n\nQuite frankly, I'd rather not. At this point, it is not important\nwhether or not we discovered a Git directory, but whether or not we have\npopulated a dereference'able `the_repository`. Those are two different\nthings.\n\n>   Side note: I suspect there are some cleanup opportunities. IIRC, I had\n>   to add have_git_dir() to cover some cases where $GIT_DIR was set but\n>   we hadn't explicitly done a setup step, but there's been a lot of\n>   refactoring and cleanup in the initialization code since then. I'm not\n>   sure if it's still necessary.\n\nYeah, well, I am not necessarily certain that we always ask the right\nquestions, such as asking whether we found a startup repository when we\nneed, in fact, to know whether `the_repository->refs` would cause a\nsegmentation fault because we would dereference a `NULL` pointer ;-)\n>\n> > diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\n> > index 413642aa56..0c37e7180d 100755\n> > --- a/t/t1309-early-config.sh\n> > +++ b/t/t1309-early-config.sh\n> > @@ -89,4 +89,9 @@ test_expect_failure 'ignore .git/ with invalid config' '\n> >  \ttest_with_config \"[\"\n> >  '\n> >\n> > +test_expect_success 'early config and onbranch' '\n> > +\techo \"[broken\" >broken &&\n> > +\ttest_with_config \"[includeif \\\"onbranch:refs/heads/master\\\"]path=../broken\"\n> > +'\n>\n> OK, so we know we didn't see the broken config because we'd have barfed\n> if we actually included it. Makes sense.\n\nThat was the idea :-)\n\nThanks for the review!\nDscho\n"},{"id":"379722","messageId":"20190731231257.GB1933@sigill.intra.peff.net","threadId":"51564","inReplyTo":"nycvar.QRO.7.76.6.1908010004130.21907@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T23:12:57Z","receivedAt":"2019-07-31T23:13:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2019 at 12:13:19AM +0200, Johannes Schindelin wrote:\n\n> > This gets tricky, because some commands are intentionally avoiding the\n> > normal lookup procedure (e.g., clone or init, and probably things like\n> > upload-pack that want to enter another repo). So I think it is OK as\n> > long as the early-config code is explicitly saying \"and please look at\n> > the refs in this specific direectory now\", and it doesn't affect other\n> > possible code paths that might look at refs. I _think_ that's what\n> > you're suggesting above, but I just want to make sure (not that it\n> > matters either way for this patch).\n> \n> I think we already have the `git clone` problem with\n> `includeif.gitdir:`. AFAICT we _will_ discover a Git directory when\n> cloning inside an existing Git worktree.\n\nYeah, I could well believe that. I think it's hard for the config code\nto say what's the right think to do here. If I'm running \"git clone\"\nfrom inside another repository, should I respect, say, an alias defined\nin that repository's config? Probably. But should I find that alias\nbehind \"includeif.gitdir\"? I dunno. Maybe?\n\nSo I'm not 100% sure the current behavior is buggy. And mostly I'd be\nhappy to ignore it until somebody comes up with a compelling\n(real-world) example either way.\n\n> And as you say, there was no use case, and I would even contend that\n> there still is no use case. In the cover letter, I tried to concoct\n> something (using a branch-dependent pager) that sounds _really_\n> far-fetched to even me.\n\nYeah. I'd be totally fine if we left it with your fix here and nobody\never found time to work on this. :)\n\n> > > -\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n> > > +\tconst char *refname = !the_repository || !the_repository->gitdir ?\n> > > +\t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n> >\n> > I think the_repository is always non-NULL.\n> \n> No, it totally can be `NULL`. I know because my first version of the\n> patch did not have that extra check, and `git help -a` would segfault\n> outside a Git worktree when I had an `includeif.onbranch:` in my\n> `~/.gitconfig`.\n\nHrm. But common-main calls initialize_the_repository(), which points it\nat &the_repo. And I can't find any other assignments. So how does it\nbecome NULL? And is every caller of have_git_dir() at risk of\nsegfaulting?\n\nAh, I see. I think it is that trace2 reads the configuration very early.\nI think we ought to do this:\n\ndiff --git a/common-main.c b/common-main.c\nindex 582a7b1886..89fd415e55 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -39,14 +39,14 @@ int main(int argc, const char **argv)\n \n \tgit_resolve_executable_dir(argv[0]);\n \n+\tinitialize_the_repository();\n+\n \ttrace2_initialize();\n \ttrace2_cmd_start(argv);\n \ttrace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n \n \tgit_setup_gettext();\n \n-\tinitialize_the_repository();\n-\n \tattr_start();\n \n \tresult = cmd_main(argc, argv);\n\nor possibly even move the trace2 bits to the very end of that function.\nThe point of common-main is to do very basic setup. Doing tentative repo\ndiscovery and config reading there at all is surprising to me, to say\nthe least. But I think we can at least make sure the library code is\ninitialized first.\n\n> > The way similar sites check this is withV\n> > \"!startup_info->have_repository\" or have_git_dir(). The early-config\n> > code uses the latter, so we should probably match it here.\n> \n> Quite frankly, I'd rather not. At this point, it is not important\n> whether or not we discovered a Git directory, but whether or not we have\n> populated a dereference'able `the_repository`. Those are two different\n> things.\n\nWhat I'm concerned about it is whether there are cases where\nthe_repository->gitdir is NULL, but we _could_ still look up refs. I.e.,\nwhy is the rest of the config code using have_git_dir(), and why is this\ncode path special?\n\nAgain, I _think_ we might be able to get rid of have_git_dir() now. Back\nwhen it was introduced get_git_dir() did lazy setup, and these days it\nlooks like it's just peeking at the_repository->gitdir. But it makes\nsense to me for this fix to be consistent with the surrounding code, and\nthen to investigate have_git_dir() separately.\n\n> >   Side note: I suspect there are some cleanup opportunities. IIRC, I had\n> >   to add have_git_dir() to cover some cases where $GIT_DIR was set but\n> >   we hadn't explicitly done a setup step, but there's been a lot of\n> >   refactoring and cleanup in the initialization code since then. I'm not\n> >   sure if it's still necessary.\n> \n> Yeah, well, I am not necessarily certain that we always ask the right\n> questions, such as asking whether we found a startup repository when we\n> need, in fact, to know whether `the_repository->refs` would cause a\n> segmentation fault because we would dereference a `NULL` pointer ;-)\n\nIf there are cases where startup_info->have_repository is non-zero but\nwe'd segfault, then I think that's a bug that is going to affect more\nspots than this, and we need to investigate and fix. But I don't think\nthat is the case. We should only be setting it after calling\nset_git_dir(), and poking at the current sites which set that leads me\nto believe this is true.\n\n-Peff\n"},{"id":"379727","messageId":"20190801004928.GA6351@sigill.intra.peff.net","threadId":"51564","inReplyTo":"20190731231257.GB1933@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-01T00:49:28Z","receivedAt":"2019-08-01T00:49:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2019 at 07:12:57PM -0400, Jeff King wrote:\n\n> Hrm. But common-main calls initialize_the_repository(), which points it\n> at &the_repo. And I can't find any other assignments. So how does it\n> become NULL? And is every caller of have_git_dir() at risk of\n> segfaulting?\n> \n> Ah, I see. I think it is that trace2 reads the configuration very early.\n> I think we ought to do this:\n> \n> diff --git a/common-main.c b/common-main.c\n> index 582a7b1886..89fd415e55 100644\n> --- a/common-main.c\n> +++ b/common-main.c\n> @@ -39,14 +39,14 @@ int main(int argc, const char **argv)\n>  \n>  \tgit_resolve_executable_dir(argv[0]);\n>  \n> +\tinitialize_the_repository();\n> +\n>  \ttrace2_initialize();\n>  \ttrace2_cmd_start(argv);\n>  \ttrace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n>  \n>  \tgit_setup_gettext();\n>  \n> -\tinitialize_the_repository();\n> -\n>  \tattr_start();\n>  \n>  \tresult = cmd_main(argc, argv);\n\nBy the way, I wondered why trace2's existing config reading did not\ncause us to segfault because of this. It is because it invented the\n\"very early config\" function which always ignores some config sources\n(working around this problem, but also making it weirdly unlike most\nother config).\n\n-Peff\n"},{"id":"379760","messageId":"7f4257d3-5ae0-3a83-84f1-bee83682fa36@jeffhostetler.com","threadId":"51564","inReplyTo":"20190801004928.GA6351@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-08-01T17:24:17Z","receivedAt":"2019-08-01T17:24:21Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 7/31/2019 8:49 PM, Jeff King wrote:\n> On Wed, Jul 31, 2019 at 07:12:57PM -0400, Jeff King wrote:\n> \n>> Hrm. But common-main calls initialize_the_repository(), which points it\n>> at &the_repo. And I can't find any other assignments. So how does it\n>> become NULL? And is every caller of have_git_dir() at risk of\n>> segfaulting?\n>>\n>> Ah, I see. I think it is that trace2 reads the configuration very early.\n>> I think we ought to do this:\n>>\n>> diff --git a/common-main.c b/common-main.c\n>> index 582a7b1886..89fd415e55 100644\n>> --- a/common-main.c\n>> +++ b/common-main.c\n>> @@ -39,14 +39,14 @@ int main(int argc, const char **argv)\n>>   \n>>   \tgit_resolve_executable_dir(argv[0]);\n>>   \n>> +\tinitialize_the_repository();\n>> +\n>>   \ttrace2_initialize();\n>>   \ttrace2_cmd_start(argv);\n>>   \ttrace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n>>   \n>>   \tgit_setup_gettext();\n>>   \n>> -\tinitialize_the_repository();\n>> -\n>>   \tattr_start();\n>>   \n>>   \tresult = cmd_main(argc, argv);\n> \n> By the way, I wondered why trace2's existing config reading did not\n> cause us to segfault because of this. It is because it invented the\n> \"very early config\" function which always ignores some config sources\n> (working around this problem, but also making it weirdly unlike most\n> other config).\n\nYes, I added the \"very early config\" to try to work around some of\nthe chicken-n-egg problems.  I can't say that I was completely happy\nwith having to do that.  I haven't had time to play with your patch\nsuggestion here, but I think it would be fine to do if it will help\nwith the original problem.\n\nIn [1] I added code to just start the clock in isolation (rather than\nbeing part of the trace2_initialize() -- which does all the config\nloading and subsystem initialization).  So it is OK to let the\ntrace2_initialize() run a little later.  (Part of the reason for that\nsplit was to allow git_resolve_executable_dir() to run first, since\nthat data was needed to find the location of the system config relative\nto the exe path (sigh).)\n\n[1] a089724958a trace2: refactor setting process starting time\n\n\nSo, as you suggested in your previous response, something like\nthis would/should be fine.\n\n$ git diff\ndiff --git a/common-main.c b/common-main.c\nindex 582a7b1886..71e21dd20a 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -39,16 +39,16 @@ int main(int argc, const char **argv)\n\n         git_resolve_executable_dir(argv[0]);\n\n-       trace2_initialize();\n-       trace2_cmd_start(argv);\n-       trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n-\n         git_setup_gettext();\n\n         initialize_the_repository();\n\n         attr_start();\n\n+       trace2_initialize();\n+       trace2_cmd_start(argv);\n+       trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n+\n         result = cmd_main(argc, argv);\n\n         trace2_cmd_exit(result);\n\n"},{"id":"379991","messageId":"20190806122601.GA21475@sigill.intra.peff.net","threadId":"51564","inReplyTo":"7f4257d3-5ae0-3a83-84f1-bee83682fa36@jeffhostetler.com","subject":"[PATCH 0/3] the_repository initialization cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T12:26:02Z","receivedAt":"2019-08-06T12:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2019 at 01:24:17PM -0400, Jeff Hostetler wrote:\n\n> > By the way, I wondered why trace2's existing config reading did not\n> > cause us to segfault because of this. It is because it invented the\n> > \"very early config\" function which always ignores some config sources\n> > (working around this problem, but also making it weirdly unlike most\n> > other config).\n> \n> Yes, I added the \"very early config\" to try to work around some of\n> the chicken-n-egg problems.  I can't say that I was completely happy\n> with having to do that.  I haven't had time to play with your patch\n> suggestion here, but I think it would be fine to do if it will help\n> with the original problem.\n> \n> In [1] I added code to just start the clock in isolation (rather than\n> being part of the trace2_initialize() -- which does all the config\n> loading and subsystem initialization).  So it is OK to let the\n> trace2_initialize() run a little later.  (Part of the reason for that\n> split was to allow git_resolve_executable_dir() to run first, since\n> that data was needed to find the location of the system config relative\n> to the exe path (sigh).)\n> \n> [1] a089724958a trace2: refactor setting process starting time\n> \n> So, as you suggested in your previous response, something like\n> this would/should be fine.\n\nThanks, that pointer helped me write the commit message. So here are a\nfew cleanups that can go on top (of master now, since Dscho's patch\ngraduated). There's no rush to get these into v2.23.\n\nNote that there _is_ still a funny corner case with the way the original\npatch checks the_repository->git_dir, but I don't think it's worth\nfixing at this point. I'll send a followup email with more details.\n\n  [1/3]: t1309: use short branch name in includeIf.onbranch test\n  [2/3]: common-main: delay trace2 initialization\n  [3/3]: config: stop checking whether the_repository is NULL\n\n common-main.c           | 8 ++++----\n config.c                | 2 +-\n t/t1309-early-config.sh | 7 ++++++-\n 3 files changed, 11 insertions(+), 6 deletions(-)\n\n-Peff\n"},{"id":"379992","messageId":"20190806122645.GA13513@sigill.intra.peff.net","threadId":"51564","inReplyTo":"20190806122601.GA21475@sigill.intra.peff.net","subject":"[PATCH 1/3] t1309: use short branch name in includeIf.onbranch test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T12:26:45Z","receivedAt":"2019-08-06T12:26:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 85fe0e800c (config: work around bug with includeif:onbranch and\nearly config, 2019-07-31) tests that our early config-reader does not\naccess the file mentioned by includeIf.onbranch:refs/heads/master.path.\nBut it would never do so even if the feature were implemented, since the\nonbranch matching code uses the short refname \"master\".\n\nThe test still serves its purpose, since the bug fixed by 85fe0e800c is\nactually that we hit a BUG() before even deciding whether to match the\nref. But let's use the correct name to avoid confusion (and which we'll\neventually want to trigger once we do the \"real\" fix described in that\ncommit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1309-early-config.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex 0c37e7180d..eeb60e4143 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -91,7 +91,7 @@ test_expect_failure 'ignore .git/ with invalid config' '\n \n test_expect_success 'early config and onbranch' '\n \techo \"[broken\" >broken &&\n-\ttest_with_config \"[includeif \\\"onbranch:refs/heads/master\\\"]path=../broken\"\n+\ttest_with_config \"[includeif \\\"onbranch:master\\\"]path=../broken\"\n '\n \n test_done\n-- \n2.23.0.rc1.436.g24d2e81391\n\n"},{"id":"379993","messageId":"20190806122725.GB13513@sigill.intra.peff.net","threadId":"51564","inReplyTo":"20190806122601.GA21475@sigill.intra.peff.net","subject":"[PATCH 2/3] common-main: delay trace2 initialization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T12:27:26Z","receivedAt":"2019-08-06T12:27:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We initialize the trace2 system in the common main() function so that\nall programs (even ones that aren't builtins) will enable tracing. But\ntrace2 startup is relatively heavy-weight, as we have to actually read\non-disk config to decide whether to trace. This can cause unexpected\ninteractions with other common-main initialization. For instance, we'll\nend up in the config code before calling initialize_the_repository(),\nand the usual invariant that the_repository is never NULL will not hold.\n\nLet's push the trace2 initialization further down in common-main, to\njust before we execute cmd_main(). The other parts of the initialization\nare much more self-contained and less likely to call library code that\ndepends on those kinds of invariants.\n\nOriginally the trace2 code tried to start as early as possible to get\naccurate timings. But the timer initialization was split out from the\nconfig reading in a089724958 (trace2: refactor setting process starting\ntime, 2019-04-15), so there shouldn't be any impact from this patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n common-main.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/common-main.c b/common-main.c\nindex 582a7b1886..71e21dd20a 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -39,16 +39,16 @@ int main(int argc, const char **argv)\n \n \tgit_resolve_executable_dir(argv[0]);\n \n-\ttrace2_initialize();\n-\ttrace2_cmd_start(argv);\n-\ttrace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n-\n \tgit_setup_gettext();\n \n \tinitialize_the_repository();\n \n \tattr_start();\n \n+\ttrace2_initialize();\n+\ttrace2_cmd_start(argv);\n+\ttrace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n+\n \tresult = cmd_main(argc, argv);\n \n \ttrace2_cmd_exit(result);\n-- \n2.23.0.rc1.436.g24d2e81391\n\n"},{"id":"379994","messageId":"20190806122758.GC13513@sigill.intra.peff.net","threadId":"51564","inReplyTo":"20190806122601.GA21475@sigill.intra.peff.net","subject":"[PATCH 3/3] config: stop checking whether the_repository is NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T12:27:58Z","receivedAt":"2019-08-06T12:28:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the previous commit, our invariant that the_repository is never\nNULL is restored, and we can stop being defensive in include_by_branch().\n\nWe can confirm the fix by showing that an onbranch config include will\nnot cause a segfault when run outside a git repository. I've put this in\nt1309-early-config since it's related to the case added by 85fe0e800c\n(config: work around bug with includeif:onbranch and early config,\n2019-07-31), though technically the issue was with\nread_very_early_config() and not read_early_config().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c                | 2 +-\n t/t1309-early-config.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 3900e4947b..cc637363bb 100644\n--- a/config.c\n+++ b/config.c\n@@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \tint flags;\n \tint ret;\n \tstruct strbuf pattern = STRBUF_INIT;\n-\tconst char *refname = !the_repository || !the_repository->gitdir ?\n+\tconst char *refname = !the_repository->gitdir ?\n \t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n \tconst char *shortname;\n \ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex eeb60e4143..3a0de0ddaa 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -94,4 +94,9 @@ test_expect_success 'early config and onbranch' '\n \ttest_with_config \"[includeif \\\"onbranch:master\\\"]path=../broken\"\n '\n \n+test_expect_success 'onbranch config outside of git repo' '\n+\ttest_config_global includeIf.onbranch:master.path non-existent &&\n+\tnongit git help\n+'\n+\n test_done\n-- \n2.23.0.rc1.436.g24d2e81391\n"},{"id":"379995","messageId":"20190806124954.GA13649@sigill.intra.peff.net","threadId":"51564","inReplyTo":"20190806122758.GC13513@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] config: stop checking whether the_repository is NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T12:49:55Z","receivedAt":"2019-08-06T12:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2019 at 08:27:58AM -0400, Jeff King wrote:\n\n> diff --git a/config.c b/config.c\n> index 3900e4947b..cc637363bb 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)\n>  \tint flags;\n>  \tint ret;\n>  \tstruct strbuf pattern = STRBUF_INIT;\n> -\tconst char *refname = !the_repository || !the_repository->gitdir ?\n> +\tconst char *refname = !the_repository->gitdir ?\n>  \t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n\nI stopped here, even though I argued earlier that this probably ought to\nbe have_git_dir() to match the rest of the config code. What I found\nwhen I started poking at it is that there are a few odd cases still in\nthe startup code, and that IMHO the final form we want to end up with\nactually matches what's written here (but I'm not quite ready to sink\nthe time into taking us there just yet). Read on for the gory details.\n\nThe most immediate issue I saw was that the_repository->gitdir doesn't\nactually tell you whether or not we have a repository! I.e., that entry\nmay be non-NULL even though startup_info->have_repository is 0.\n\nSo these two tests fail:\n\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex d20b4d150d..7ec91c4c1c 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -348,6 +348,18 @@ test_expect_success 'conditional include, onbranch, implicit /** for /' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'onbranch include not fooled by fake GIT_DIR' '\n+\tmkdir not-a-git-dir &&\n+\techo \"ref: refs/heads/master\" >not-a-git-dir/HEAD &&\n+\tgit config --file=inner the.value inner &&\n+\tgit config --file=outer the.value outer &&\n+\tgit config --file=outer includeIf.onbranch:master.path inner &&\n+\tGIT_DIR=not-a-git-dir \\\n+\t\tgit config --file=outer --includes the.value >actual &&\n+\techo outer >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'include cycles are detected' '\n \tgit init --bare cycle &&\n \tgit -C cycle config include.path cycle &&\ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex 3a0de0ddaa..91af2c2f89 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -99,4 +99,12 @@ test_expect_success 'onbranch config outside of git repo' '\n \tnongit git help\n '\n \n+test_expect_success 'early config is not fooled by bogus GIT_DIR' '\n+\tmkdir not-a-git-dir &&\n+\tgit config --file=not-a-git-dir/config \\\n+\t\talias.should-not-run \\\n+\t\t\"!echo should-not-run\" &&\n+\ttest_must_fail env GIT_DIR=\"not-a-git-dir\" git should-not-run\n+'\n+\n test_done\n\nAs you can see from that case, it's a curiosity that we still set the\ninternal gitdir variable from $GIT_DIR, even if we found that it's not a\nvalid repo.\n\nThe onbranch case is fooled by our direct check of gitdir, but likewise\nhave_git_dir() allows either have_repository or a non-NULL gitdir (as I\nmentioned earlier, I think this is vestigial at this point).\n\nSo doing this fixes the onbranch case:\n\ndiff --git a/config.c b/config.c\nindex cc637363bb..134637f3ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \tint flags;\n \tint ret;\n \tstruct strbuf pattern = STRBUF_INIT;\n-\tconst char *refname = !the_repository->gitdir ?\n+\tconst char *refname = !have_git_dir() ?\n \t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n \tconst char *shortname;\n \ndiff --git a/environment.c b/environment.c\nindex 89af47cb85..08bdf79d6b 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -203,8 +203,7 @@ int is_bare_repository(void)\n \n int have_git_dir(void)\n {\n-\treturn startup_info->have_repository\n-\t\t|| the_repository->gitdir;\n+\treturn startup_info->have_repository;\n }\n \n const char *get_git_dir(void)\n\n\nbut curiously it _doesn't_ fix the alias one, because after seeing that\nwe have no configured git dir, we then fall back to discover_git_dir(),\nwhich happily reports back the same bogus $GIT_DIR.\n\nSo I think there'd be some more cleanup required there. But the general\ndirection is that in the near future we probably ought to be checking\nstartup_info->have_repository and not the_repository->gitdir.\n\nBut I think in the long-term, we probably ought to be getting rid of\nstartup_info->have_repository itself. It's really just hiding a subtle\ndependency on whether the_repository is valid, and we're better off\nactually seeing all the spots that depend on the_repository (and\neventually converting them to take a \"struct repository\" as\nappropriate).\n\nWhere we want to end up is (I think):\n\n  - stop setting the_repository->gitdir when we know that there's no\n    repo (and possibly clear $GIT_DIR as well to avoid confusion). That\n    should make it a robust way to check whether a \"struct repository\"\n    is valid.\n\n    This is the step I've stalled on, because I'm a bit worried that\n    there's some subtle case that depends on it working this way.\n\n  - drop startup_info->have_repository entirely. At that point it would\n    be redundant with checking the_repository->gitdir.\n\n    If you do this without the first step, t4201 will notice and fail\n    (because it expects GIT_DIR=not-a-repo to notice that there is no\n    repo).\n\n  - optionally, drop have_git_dir() in favor of just checking\n    the_repository->gitdir. This loses a layer of abstraction, but I\n    think it makes it much more clear where we'd depending on\n    the_repository.\n\nAnd so in the end, the current state of include_by_branch() is what we\nwant, and all the other call sites should change to match it. Whether it\nshould be changed in the interim (until we fix the discrepancy between\ngitdir/have_repository), I don't feel strongly about. There are\nuser-visible cases that I believe we get wrong today, but as you can see\nfrom the tests above, they're pretty ridiculous and unlikely in\npractice.\n\n-Peff\n"},{"id":"379996","messageId":"20190806125659.GA18442@sigill.intra.peff.net","threadId":"51564","inReplyTo":"7f4257d3-5ae0-3a83-84f1-bee83682fa36@jeffhostetler.com","subject":"Re: [PATCH v2 1/1] config: work around bug with includeif:onbranch and early config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-06T12:56:59Z","receivedAt":"2019-08-06T12:57:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2019 at 01:24:17PM -0400, Jeff Hostetler wrote:\n\n> > By the way, I wondered why trace2's existing config reading did not\n> > cause us to segfault because of this. It is because it invented the\n> > \"very early config\" function which always ignores some config sources\n> > (working around this problem, but also making it weirdly unlike most\n> > other config).\n> \n> Yes, I added the \"very early config\" to try to work around some of\n> the chicken-n-egg problems.  I can't say that I was completely happy\n> with having to do that.\n\nI meant to comment a little further on this earlier. While I do think\nit's unfortunate to have yet another set of special rules, I think what\nyou ended up with is probably the least-bad thing.  This trace2 config\nread is happening so early (even after my movement patch) that it makes\nme nervous to do anything at all complicated.\n\nNot just for performance reasons, which you cited in the original\ncommit, but also for correctness and even security reasons.  This is\ncode we're running for every single binary before we even hit main(),\nand it's probably a good thing that we are not loading .git/config\nvalues from a potentially untrusted repository (e.g., upload-pack is\nsupposed to be safe to run in an untrusted repo).\n\n-Peff\n"},{"id":"380162","messageId":"nycvar.QRO.7.76.6.1908082147190.46@tvgsbejvaqbjf.bet","threadId":"51564","inReplyTo":"20190806124954.GA13649@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] config: stop checking whether the_repository is NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-08T19:48:31Z","receivedAt":"2019-08-08T19:48:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nACK on the three patches, and I think your outline is valid (post\nv2.23.0, possibly an Outreachy project?).\n\nThanks,\nDscho\n\nP.S.: Sorry for top-posting, but I felt that I did not even respond to\nanything you said, concretely, so...\n\nOn Tue, 6 Aug 2019, Jeff King wrote:\n\n> On Tue, Aug 06, 2019 at 08:27:58AM -0400, Jeff King wrote:\n>\n> > diff --git a/config.c b/config.c\n> > index 3900e4947b..cc637363bb 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)\n> >  \tint flags;\n> >  \tint ret;\n> >  \tstruct strbuf pattern = STRBUF_INIT;\n> > -\tconst char *refname = !the_repository || !the_repository->gitdir ?\n> > +\tconst char *refname = !the_repository->gitdir ?\n> >  \t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n>\n> I stopped here, even though I argued earlier that this probably ought to\n> be have_git_dir() to match the rest of the config code. What I found\n> when I started poking at it is that there are a few odd cases still in\n> the startup code, and that IMHO the final form we want to end up with\n> actually matches what's written here (but I'm not quite ready to sink\n> the time into taking us there just yet). Read on for the gory details.\n>\n> The most immediate issue I saw was that the_repository->gitdir doesn't\n> actually tell you whether or not we have a repository! I.e., that entry\n> may be non-NULL even though startup_info->have_repository is 0.\n>\n> So these two tests fail:\n>\n> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n> index d20b4d150d..7ec91c4c1c 100755\n> --- a/t/t1305-config-include.sh\n> +++ b/t/t1305-config-include.sh\n> @@ -348,6 +348,18 @@ test_expect_success 'conditional include, onbranch, implicit /** for /' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'onbranch include not fooled by fake GIT_DIR' '\n> +\tmkdir not-a-git-dir &&\n> +\techo \"ref: refs/heads/master\" >not-a-git-dir/HEAD &&\n> +\tgit config --file=inner the.value inner &&\n> +\tgit config --file=outer the.value outer &&\n> +\tgit config --file=outer includeIf.onbranch:master.path inner &&\n> +\tGIT_DIR=not-a-git-dir \\\n> +\t\tgit config --file=outer --includes the.value >actual &&\n> +\techo outer >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'include cycles are detected' '\n>  \tgit init --bare cycle &&\n>  \tgit -C cycle config include.path cycle &&\n> diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\n> index 3a0de0ddaa..91af2c2f89 100755\n> --- a/t/t1309-early-config.sh\n> +++ b/t/t1309-early-config.sh\n> @@ -99,4 +99,12 @@ test_expect_success 'onbranch config outside of git repo' '\n>  \tnongit git help\n>  '\n>\n> +test_expect_success 'early config is not fooled by bogus GIT_DIR' '\n> +\tmkdir not-a-git-dir &&\n> +\tgit config --file=not-a-git-dir/config \\\n> +\t\talias.should-not-run \\\n> +\t\t\"!echo should-not-run\" &&\n> +\ttest_must_fail env GIT_DIR=\"not-a-git-dir\" git should-not-run\n> +'\n> +\n>  test_done\n>\n> As you can see from that case, it's a curiosity that we still set the\n> internal gitdir variable from $GIT_DIR, even if we found that it's not a\n> valid repo.\n>\n> The onbranch case is fooled by our direct check of gitdir, but likewise\n> have_git_dir() allows either have_repository or a non-NULL gitdir (as I\n> mentioned earlier, I think this is vestigial at this point).\n>\n> So doing this fixes the onbranch case:\n>\n> diff --git a/config.c b/config.c\n> index cc637363bb..134637f3ad 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -275,7 +275,7 @@ static int include_by_branch(const char *cond, size_t cond_len)\n>  \tint flags;\n>  \tint ret;\n>  \tstruct strbuf pattern = STRBUF_INIT;\n> -\tconst char *refname = !the_repository->gitdir ?\n> +\tconst char *refname = !have_git_dir() ?\n>  \t\tNULL : resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n>  \tconst char *shortname;\n>\n> diff --git a/environment.c b/environment.c\n> index 89af47cb85..08bdf79d6b 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -203,8 +203,7 @@ int is_bare_repository(void)\n>\n>  int have_git_dir(void)\n>  {\n> -\treturn startup_info->have_repository\n> -\t\t|| the_repository->gitdir;\n> +\treturn startup_info->have_repository;\n>  }\n>\n>  const char *get_git_dir(void)\n>\n>\n> but curiously it _doesn't_ fix the alias one, because after seeing that\n> we have no configured git dir, we then fall back to discover_git_dir(),\n> which happily reports back the same bogus $GIT_DIR.\n>\n> So I think there'd be some more cleanup required there. But the general\n> direction is that in the near future we probably ought to be checking\n> startup_info->have_repository and not the_repository->gitdir.\n>\n> But I think in the long-term, we probably ought to be getting rid of\n> startup_info->have_repository itself. It's really just hiding a subtle\n> dependency on whether the_repository is valid, and we're better off\n> actually seeing all the spots that depend on the_repository (and\n> eventually converting them to take a \"struct repository\" as\n> appropriate).\n>\n> Where we want to end up is (I think):\n>\n>   - stop setting the_repository->gitdir when we know that there's no\n>     repo (and possibly clear $GIT_DIR as well to avoid confusion). That\n>     should make it a robust way to check whether a \"struct repository\"\n>     is valid.\n>\n>     This is the step I've stalled on, because I'm a bit worried that\n>     there's some subtle case that depends on it working this way.\n>\n>   - drop startup_info->have_repository entirely. At that point it would\n>     be redundant with checking the_repository->gitdir.\n>\n>     If you do this without the first step, t4201 will notice and fail\n>     (because it expects GIT_DIR=not-a-repo to notice that there is no\n>     repo).\n>\n>   - optionally, drop have_git_dir() in favor of just checking\n>     the_repository->gitdir. This loses a layer of abstraction, but I\n>     think it makes it much more clear where we'd depending on\n>     the_repository.\n>\n> And so in the end, the current state of include_by_branch() is what we\n> want, and all the other call sites should change to match it. Whether it\n> should be changed in the interim (until we fix the discrepancy between\n> gitdir/have_repository), I don't feel strongly about. There are\n> user-visible cases that I believe we get wrong today, but as you can see\n> from the tests above, they're pretty ridiculous and unlikely in\n> practice.\n>\n> -Peff\n>\n"}]}