[BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`

[Richard Hartmann]

Dear all,

I am not sure if this is an actual bug or just a corner case that's
not worth to be fixed.

This was not tested with HEAD or even 2.1.2, but 2.1.1.

Notwithstanding if the setting is correct, shouldn't rev-parse be
resilient enough to at least be able to tell if we're in a work tree?
I understand why `git status` and the like would need to parse the
full config, but determining if you're in a work tree should be
possible in most if not all cases.
Unless detached work trees get you into a situation where you really
need to parse the whole config...

So this is not a real bug report, more of a "is this intended this way?"

As you can see, my custom prompt (via vcs_info in Zsh) breaks due to
that which is how I noticed.


***@titanium ~ % mkdir git_test
***@titanium ~ % cd git_test
***@titanium ~/git_test % git init
Initialized empty Git repository in /home/richih/git_test/.git/
***@titanium (git)-[master] ~/git_test % git rev-parse --is-inside-work-tree
true
***@titanium (git)-[master] ~/git_test % git config
branch.autosetuprebase true
***@titanium ~/git_test % git rev-parse --is-inside-work-tree
error: Malformed value for branch.autosetuprebase
fatal: bad config file line 8 in .git/config
***@titanium ~/git_test % cat .git/config
[core]
repositoryformatversion = 0
filemode = true
bare = false
logallrefupdates = true
[branch]
autosetuprebase = true
***@titanium ~/git_test % git --version
git version 2.1.1
***@titanium ~/git_test %


Thanks,
Richard

[Junio C Hamano]

Post by Richard Hartmann
So this is not a real bug report, more of a "is this intended this way?"
error: Malformed value for branch.autosetuprebase
fatal: bad config file line 8 in .git/config
...
[branch]
autosetuprebase = true

It does not seem to be limited to rev-parse but having a malformed
configuration for that variable would break everything Git, which
certainly is not how it is supposed to work. It also seems that the
breakage dates back very far into the past (I checked 1.7.0 and it
seems to be broken the same way).

The same breakage exists for branch.autosetupmerge, I think, e.g.

[branch]
autosetupmerge = garbage

In config.c, git_default_branch_config() must be corrected to set
git_branch_track and autorebase to BRANCH_TRACK_MALFORMED and
AUTOREBASE_MALFORMED and the users of these two variables must be
fixed to deal with the "malformed in the configuration" cases, I
think. The error should happen only in the codepath where we need
the value, and no other places.

[Junio C Hamano]

Post by Junio C Hamano
In config.c, git_default_branch_config() must be corrected to set
git_branch_track and autorebase to BRANCH_TRACK_MALFORMED and
AUTOREBASE_MALFORMED and the users of these two variables must be
fixed to deal with the "malformed in the configuration" cases, I
think. The error should happen only in the codepath where we need
the value, and no other places.

Having said that, given that any call git_config_bool() inside a
callback function given to the git_config() will stop Git from doing
anything even if the variable with a malformed value in quesiton is
not used by the operation at all, and there are very many of them
(e.g. setting core.filemode to "treu" would break everything), it
appears to me that:

(1) it could be argued that catching obvious typos in the
configuration file as early as possible, even if the variables
with typos are not used for the particular operation, is even a
feature, as long as you can fix the brekage with "git config"
(and/or your editor);

(2) it is too much pain to shift the error checking to the site of
their use from the site of their parsing anyway ;-)

And I suspect Tanay and Matthieu's recent work is taking us to a
direction where many code paths do not use the config callbacks
(which is what leads us to detect errors at parse time even for
variables that are not used) and instead allow the callers that care
about the individual variables to diagnose errors at the site of
use. So as you stated originally, this may not be something we want
to patch up in the current callback based config system.

[Tanay Abhra]

Post by Junio C Hamano

Post by Richard Hartmann
So this is not a real bug report, more of a "is this intended this way?"
error: Malformed value for branch.autosetuprebase
fatal: bad config file line 8 in .git/config
...
[branch]
autosetuprebase = true

It does not seem to be limited to rev-parse but having a malformed
configuration for that variable would break everything Git, which
certainly is not how it is supposed to work. It also seems that the
breakage dates back very far into the past (I checked 1.7.0 and it
seems to be broken the same way).
The same breakage exists for branch.autosetupmerge, I think, e.g.
[branch]
autosetupmerge = garbage
In config.c, git_default_branch_config() must be corrected to set
git_branch_track and autorebase to BRANCH_TRACK_MALFORMED and
AUTOREBASE_MALFORMED and the users of these two variables must be
fixed to deal with the "malformed in the configuration" cases, I
think. The error should happen only in the codepath where we need
the value, and no other places.

Supporting Junio's claim, there is a function called git_default_config()
which checks and sets a whole load of config values which may or maynot
be relevant to the codepath that called it. (branch.autosetuprebase is a
part of it) So an error may occur printing a seemingly unrelated config value
as the malformed variable as happened in your case.

There are currently 72 callers of git_default_config() in the codebase,
so a malformed config value breaks most of git commands. The only path
to correct this behavior would be either correct the config variable in
the file or we could decouple the huge monolithic function that
git_default_config() has become and use the git_config_get_value() in the
code paths that really need them. This part is doable, albeit slowly. All
the config variables in git_default_config() can be rewritten using the
new non callback based functions easily as demonstrated in an earlier
RFC patch.

[brian m. carlson]

Post by Richard Hartmann
Notwithstanding if the setting is correct, shouldn't rev-parse be
resilient enough to at least be able to tell if we're in a work tree?
I understand why `git status` and the like would need to parse the
full config, but determining if you're in a work tree should be
possible in most if not all cases.
Unless detached work trees get you into a situation where you really
need to parse the whole config...

I have seen similar problems where various git commands fail in the
middle if a config file is the subject of merge conflicts. For example:

vauxhall ok % git pull . master
From .
* branch master -> FETCH_HEAD
Auto-merging .zshrc
CONFLICT (content): Merge conflict in .zshrc
Auto-merging .zshenv
CONFLICT (content): Merge conflict in .zshenv
Auto-merging .vimrc
Auto-merging .gitconfig
CONFLICT (content): Merge conflict in .gitconfig
fatal: bad config file line 9 in .gitconfig

This tends to be one of the downsides of storing one's dotfiles in git.
I usually work around it by specifying HOME=/tmp before a command I
think might cause a conflict in .gitconfig. I'm not sure there's any
good way around it, though.