Re: [PATCH] config: complain about --local outside of a git repo
On Sun, May 14, 2017 at 08:31:54PM -0400, Josh Hagins wrote: > For context, the "user.name" bit was purely notional; I just wanted to > give a sample reproduction. Where I've actually been running into this > in real life is with oh-my-git, a GitHub-themed Powerline bash prompt: > https://github.com/arialdomartini/oh-my-git/blob/42c11f08540949b75bd513e6880a4ce6824a2bcc/base.sh#L52. > > Since this command is invoked every time the prompt is build, I see > the error message after every single command I run outside of a Git > repository. While this is more indicative of a code smell in > oh-my-git, I figured that, as you said, BUG assertions should never > get hit in the wild. Thanks for the patch! Ah, thanks for the context. That makes a lot more sense. I do think oh-my-git should just drop the "--local", which will cause git-config to follow the usual precedence rules (and it will skip repo-level config automatically when we are not in a repo). Since the oh-my-git code only looks for "false", nobody would ever need to default the value to true in their ~/.gitconfig. But respecting the usual config rules would mean you could set it to "false" in ~/.gitconfig to turn it off everywhere, but then opt back into it for specific repos by setting it to "true" there. Probably not a use case anybody cares that much about, but it's how "git config" was meant to be used. :) Anyway, thanks again for reporting the BUG assertion. I'm glad to be weeding out more cases. -Peff
Re: [PATCH] config: complain about --local outside of a git repo
Hi Jeff, For context, the "user.name" bit was purely notional; I just wanted to give a sample reproduction. Where I've actually been running into this in real life is with oh-my-git, a GitHub-themed Powerline bash prompt: https://github.com/arialdomartini/oh-my-git/blob/42c11f08540949b75bd513e6880a4ce6824a2bcc/base.sh#L52. Since this command is invoked every time the prompt is build, I see the error message after every single command I run outside of a Git repository. While this is more indicative of a code smell in oh-my-git, I figured that, as you said, BUG assertions should never get hit in the wild. Thanks for the patch! Cheers, Josh On Fri, May 12, 2017 at 4:34 PM, Jeff King wrote: > On Fri, May 12, 2017 at 10:19:59AM -0400, Josh Hagins wrote: > >> Since upgrading to Git 2.13.0 I'm seeing this error message whenever >> `git config --local ` is called outside a Git repository. >> For example, note the difference in behavior between Git 2.13 and >> Apple Git: >> >> $ pwd >> /Users/jhagins >> $ /usr/bin/git --version >> git version 2.11.0 (Apple Git-81) >> $ /usr/bin/git config --local --get user.name >> $ /usr/local/bin/git --version >> git version 2.13.0 >> $ /usr/local/bin/git config --local --get user.name >> fatal: BUG: setup_git_env called without repository >> >> Apple Git outputs nothing, as expected. The summarized release notes >> published by GitHub specifically mentioned that instances of this >> error message should be reported, so here you go! > > Thanks for reporting. All the developers have been running with this > change for months, but I knew as soon as it was released into the wild > that somebody would find a new corner case. :) > > I think dying is the right thing here; you are asking for "--local" but > there is no local repository. But we should never hit a BUG assertion. > Patch is below. > > I'm not sure exactly what you wanted to accomplish with --local. If you > just want to know if user.name is set anywhere (and you may or may not > be in a git repo), then just "git config --get user.name" would work. If > you want to know if you're in a local repo and if so whether the > variable is set, you'd need to use two commands, like: > > git rev-parse --git-dir >/dev/null 2>&1 && > git config --local --get user.name > > -- >8 -- > Subject: [PATCH] config: complain about --local outside of a git repo > > The "--local" option instructs git-config to read or modify > the repository-level config. This doesn't make any sense if > you're not actually in a repository. > > Older versions of Git would blindly try to read or write > ".git/config". For reading, this would result in a quiet > failure, since there was no config to read (and thus no > matching config value). Writing would generally fail > noisily, since ".git" was unlikely to exist. But since > b1ef400ee (setup_git_env: avoid blind fall-back to ".git", > 2016-10-20), we catch this in the call to git_pathdup() and > die("BUG"). > > Dying is the right thing to do, but we should catch the > problem early and give a more human-friendly error message. > > Note that even without --local, git-config will sometimes > default to using local repository config. These cases are > already protected by a similar check. > > Signed-off-by: Jeff King > --- > builtin/config.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/builtin/config.c b/builtin/config.c > index 3a554ad50..ad7c6a19c 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -496,6 +496,9 @@ int cmd_config(int argc, const char **argv, const char > *prefix) > usage_with_options(builtin_config_usage, > builtin_config_options); > } > > + if (use_local_config && nongit) > + die(_("--local only be used inside a git repository")); > + > if (given_config_source.file && > !strcmp(given_config_source.file, "-")) { > given_config_source.file = NULL; > -- > 2.13.0.452.g0afc8e12b > -- Josh Hagins
Re: [PATCH] config: complain about --local outside of a git repo
On Fri, May 12, 2017 at 05:04:42PM -0700, Jonathan Nieder wrote: > > diff --git a/builtin/config.c b/builtin/config.c > > index 3a554ad50..ad7c6a19c 100644 > > --- a/builtin/config.c > > +++ b/builtin/config.c > > @@ -496,6 +496,9 @@ int cmd_config(int argc, const char **argv, const char > > *prefix) > > usage_with_options(builtin_config_usage, > > builtin_config_options); > > } > > > > + if (use_local_config && nongit) > > + die(_("--local only be used inside a git repository")); > > + > > The output would be > > fatal: --local only be used inside a git repository > > Is that missing a "should" before "only"? Urgh, I meant "can" (I had originally written "only makes sense" but changed it at the list minute). I'll re-roll after thinking about the test issues raised in the other part of the thread. -Peff
Re: [PATCH] config: complain about --local outside of a git repo
On Sat, May 13, 2017 at 12:31:31AM +0200, Ævar Arnfjörð Bjarmason wrote: > > + if (use_local_config && nongit) > > + die(_("--local only be used inside a git repository")); > > + > > It would be better to have a test for edge cases that are currently > only being discovered by users in the wild. I actually started on one earlier, but what would it check? We already die() in this case. Should we be grepping for the message? It seems more likely to me that we would change the message and cause a false positive than that there would be an actual regression. What I think might be more interesting is if die("BUG") could learn to exit with some error code that the test suite considered invalid. Like calling abort(), which would kill us with SIGABRT and cause test_must_fail to complain. On many systems that would also generate a coredump. Which is handy sometimes, but I wonder if it would be inconvenient for others. I guess that is no different than what a raised assert() would do. But if we were to do that, then the test could easily demonstrate that we expect a clean die(). -Peff
Re: [PATCH] config: complain about --local outside of a git repo
Jeff King wrote: > Subject: [PATCH] config: complain about --local outside of a git repo > > The "--local" option instructs git-config to read or modify > the repository-level config. This doesn't make any sense if > you're not actually in a repository. > > Older versions of Git would blindly try to read or write > ".git/config". For reading, this would result in a quiet > failure, since there was no config to read (and thus no > matching config value). Writing would generally fail > noisily, since ".git" was unlikely to exist. But since > b1ef400ee (setup_git_env: avoid blind fall-back to ".git", > 2016-10-20), we catch this in the call to git_pathdup() and > die("BUG"). > > Dying is the right thing to do, but we should catch the > problem early and give a more human-friendly error message. > > Note that even without --local, git-config will sometimes > default to using local repository config. These cases are > already protected by a similar check. > > Signed-off-by: Jeff King > --- > builtin/config.c | 3 +++ > 1 file changed, 3 insertions(+) Makes sense. > diff --git a/builtin/config.c b/builtin/config.c > index 3a554ad50..ad7c6a19c 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -496,6 +496,9 @@ int cmd_config(int argc, const char **argv, const char > *prefix) > usage_with_options(builtin_config_usage, > builtin_config_options); > } > > + if (use_local_config && nongit) > + die(_("--local only be used inside a git repository")); > + The output would be fatal: --local only be used inside a git repository Is that missing a "should" before "only"? With that change, Reviewed-by: Jonathan Nieder
Re: [PATCH] config: complain about --local outside of a git repo
On Fri, May 12, 2017 at 10:34 PM, Jeff King wrote: > On Fri, May 12, 2017 at 10:19:59AM -0400, Josh Hagins wrote: > >> Since upgrading to Git 2.13.0 I'm seeing this error message whenever >> `git config --local ` is called outside a Git repository. >> For example, note the difference in behavior between Git 2.13 and >> Apple Git: >> >> $ pwd >> /Users/jhagins >> $ /usr/bin/git --version >> git version 2.11.0 (Apple Git-81) >> $ /usr/bin/git config --local --get user.name >> $ /usr/local/bin/git --version >> git version 2.13.0 >> $ /usr/local/bin/git config --local --get user.name >> fatal: BUG: setup_git_env called without repository >> >> Apple Git outputs nothing, as expected. The summarized release notes >> published by GitHub specifically mentioned that instances of this >> error message should be reported, so here you go! > > Thanks for reporting. All the developers have been running with this > change for months, but I knew as soon as it was released into the wild > that somebody would find a new corner case. :) > > I think dying is the right thing here; you are asking for "--local" but > there is no local repository. But we should never hit a BUG assertion. > Patch is below. > > I'm not sure exactly what you wanted to accomplish with --local. If you > just want to know if user.name is set anywhere (and you may or may not > be in a git repo), then just "git config --get user.name" would work. If > you want to know if you're in a local repo and if so whether the > variable is set, you'd need to use two commands, like: > > git rev-parse --git-dir >/dev/null 2>&1 && > git config --local --get user.name > > -- >8 -- > Subject: [PATCH] config: complain about --local outside of a git repo > > The "--local" option instructs git-config to read or modify > the repository-level config. This doesn't make any sense if > you're not actually in a repository. > > Older versions of Git would blindly try to read or write > ".git/config". For reading, this would result in a quiet > failure, since there was no config to read (and thus no > matching config value). Writing would generally fail > noisily, since ".git" was unlikely to exist. But since > b1ef400ee (setup_git_env: avoid blind fall-back to ".git", > 2016-10-20), we catch this in the call to git_pathdup() and > die("BUG"). > > Dying is the right thing to do, but we should catch the > problem early and give a more human-friendly error message. > > Note that even without --local, git-config will sometimes > default to using local repository config. These cases are > already protected by a similar check. > > Signed-off-by: Jeff King > --- > builtin/config.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/builtin/config.c b/builtin/config.c > index 3a554ad50..ad7c6a19c 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -496,6 +496,9 @@ int cmd_config(int argc, const char **argv, const char > *prefix) > usage_with_options(builtin_config_usage, > builtin_config_options); > } > > + if (use_local_config && nongit) > + die(_("--local only be used inside a git repository")); > + > if (given_config_source.file && > !strcmp(given_config_source.file, "-")) { > given_config_source.file = NULL; > -- > 2.13.0.452.g0afc8e12b It would be better to have a test for edge cases that are currently only being discovered by users in the wild.