Re: [PATCH] config: complain about --local outside of a git repo

[Jeff King]

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

[Josh Hagins]

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

[Jeff King]

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

[Jeff King]

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

[Jonathan Nieder]

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

[Ævar Arnfjörð Bjarmason]

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.