{"thread":{"id":"37689","subject":"[BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`","startedAt":"2014-10-08T11:22:33Z","lastAt":"2014-10-08T20:09:36Z","messageCount":5,"participants":["Richard Hartmann","Junio C Hamano","Tanay Abhra","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"250355","messageId":"CAD77+gTuqbZimEK-=gXY8o4wFqOicFvnQ5MnV6Fq1npuckwYFQ@mail.gmail.com","threadId":"37689","inReplyTo":null,"subject":"[BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`","fromName":"Richard Hartmann","fromEmail":"richih.mailinglist@gmail.com","sentAt":"2014-10-08T11:22:33Z","receivedAt":"2014-10-08T11:22:33Z","isPatch":false,"sender":{"key":"richih.mailinglist@gmail.com","avatar":"https://avatars.githubusercontent.com/u/754723?v=4"},"body":"Dear all,\n\nI am not sure if this is an actual bug or just a corner case that's\nnot worth to be fixed.\n\nThis was not tested with HEAD or even 2.1.2, but 2.1.1.\n\nNotwithstanding if the setting is correct, shouldn't rev-parse be\nresilient enough to at least be able to tell if we're in a work tree?\nI understand why `git status` and the like would need to parse the\nfull config, but determining if you're in a work tree should be\npossible in most if not all cases.\nUnless detached work trees get you into a situation where you really\nneed to parse the whole config...\n\nSo this is not a real bug report, more of a \"is this intended this way?\"\n\nAs you can see, my custom prompt (via vcs_info in Zsh) breaks due to\nthat which is how I noticed.\n\n\nrichih@titanium ~ % mkdir git_test\nrichih@titanium ~ % cd git_test\nrichih@titanium ~/git_test % git init\nInitialized empty Git repository in /home/richih/git_test/.git/\nrichih@titanium (git)-[master] ~/git_test % git rev-parse --is-inside-work-tree\ntrue\nrichih@titanium (git)-[master] ~/git_test % git config\nbranch.autosetuprebase true\nrichih@titanium ~/git_test % git rev-parse --is-inside-work-tree\nerror: Malformed value for branch.autosetuprebase\nfatal: bad config file line 8 in .git/config\nrichih@titanium ~/git_test % cat .git/config\n[core]\nrepositoryformatversion = 0\nfilemode = true\nbare = false\nlogallrefupdates = true\n[branch]\nautosetuprebase = true\nrichih@titanium ~/git_test % git --version\ngit version 2.1.1\nrichih@titanium ~/git_test %\n\n\nThanks,\nRichard\n"},{"id":"250364","messageId":"xmqqppe2bh1p.fsf@gitster.dls.corp.google.com","threadId":"37689","inReplyTo":"CAD77+gTuqbZimEK-=gXY8o4wFqOicFvnQ5MnV6Fq1npuckwYFQ@mail.gmail.com","subject":"Re: [BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-08T17:35:46Z","receivedAt":"2014-10-08T17:35:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hartmann writes:\n\n> So this is not a real bug report, more of a \"is this intended this way?\"\n> richih@titanium ~/git_test % git rev-parse --is-inside-work-tree\n> error: Malformed value for branch.autosetuprebase\n> fatal: bad config file line 8 in .git/config\n> richih@titanium ~/git_test % cat .git/config\n> ...\n> [branch]\n> autosetuprebase = true\n\nIt does not seem to be limited to rev-parse but having a malformed\nconfiguration for that variable would break everything Git, which\ncertainly is not how it is supposed to work. It also seems that the\nbreakage dates back very far into the past (I checked 1.7.0 and it\nseems to be broken the same way).\n\nThe same breakage exists for branch.autosetupmerge, I think, e.g.\n\n\t[branch]\n autosetupmerge = garbage\n\nIn config.c, git_default_branch_config() must be corrected to set\ngit_branch_track and autorebase to BRANCH_TRACK_MALFORMED and\nAUTOREBASE_MALFORMED and the users of these two variables must be\nfixed to deal with the \"malformed in the configuration\" cases, I\nthink. The error should happen only in the codepath where we need\nthe value, and no other places.\n"},{"id":"250366","messageId":"xmqqlhoqbgab.fsf@gitster.dls.corp.google.com","threadId":"37689","inReplyTo":"xmqqppe2bh1p.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-08T17:52:12Z","receivedAt":"2014-10-08T17:52:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In config.c, git_default_branch_config() must be corrected to set\n> git_branch_track and autorebase to BRANCH_TRACK_MALFORMED and\n> AUTOREBASE_MALFORMED and the users of these two variables must be\n> fixed to deal with the \"malformed in the configuration\" cases, I\n> think. The error should happen only in the codepath where we need\n> the value, and no other places.\n\nHaving said that, given that any call git_config_bool() inside a\ncallback function given to the git_config() will stop Git from doing\nanything even if the variable with a malformed value in quesiton is\nnot used by the operation at all, and there are very many of them\n(e.g. setting core.filemode to \"treu\" would break everything), it\nappears to me that:\n\n (1) it could be argued that catching obvious typos in the\n configuration file as early as possible, even if the variables\n with typos are not used for the particular operation, is even a\n feature, as long as you can fix the brekage with \"git config\"\n (and/or your editor);\n\n (2) it is too much pain to shift the error checking to the site of\n their use from the site of their parsing anyway ;-)\n\nAnd I suspect Tanay and Matthieu's recent work is taking us to a\ndirection where many code paths do not use the config callbacks\n(which is what leads us to detect errors at parse time even for\nvariables that are not used) and instead allow the callers that care\nabout the individual variables to diagnose errors at the site of\nuse. So as you stated originally, this may not be something we want\nto patch up in the current callback based config system.\n"},{"id":"250384","messageId":"54358AB0.3060002@gmail.com","threadId":"37689","inReplyTo":"xmqqppe2bh1p.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-10-08T19:04:16Z","receivedAt":"2014-10-08T19:04:16Z","isPatch":false,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 10/8/2014 11:05 PM, Junio C Hamano wrote:\n> Richard Hartmann writes:\n> \n>> So this is not a real bug report, more of a \"is this intended this way?\"\n>> richih@titanium ~/git_test % git rev-parse --is-inside-work-tree\n>> error: Malformed value for branch.autosetuprebase\n>> fatal: bad config file line 8 in .git/config\n>> richih@titanium ~/git_test % cat .git/config\n>> ...\n>> [branch]\n>> autosetuprebase = true\n> \n> It does not seem to be limited to rev-parse but having a malformed\n> configuration for that variable would break everything Git, which\n> certainly is not how it is supposed to work. It also seems that the\n> breakage dates back very far into the past (I checked 1.7.0 and it\n> seems to be broken the same way).\n> \n> The same breakage exists for branch.autosetupmerge, I think, e.g.\n> \n> \t[branch]\n> autosetupmerge = garbage\n> \n> In config.c, git_default_branch_config() must be corrected to set\n> git_branch_track and autorebase to BRANCH_TRACK_MALFORMED and\n> AUTOREBASE_MALFORMED and the users of these two variables must be\n> fixed to deal with the \"malformed in the configuration\" cases, I\n> think. The error should happen only in the codepath where we need\n> the value, and no other places.\n\nSupporting Junio's claim, there is a function called git_default_config()\nwhich checks and sets a whole load of config values which may or maynot\nbe relevant to the codepath that called it. (branch.autosetuprebase is a\npart of it) So an error may occur printing a seemingly unrelated config value\nas the malformed variable as happened in your case.\n\nThere are currently 72 callers of git_default_config() in the codebase,\nso a malformed config value breaks most of git commands. The only path\nto correct this behavior would be either correct the config variable in\nthe file or we could decouple the huge monolithic function that\ngit_default_config() has become and use the git_config_get_value() in the\ncode paths that really need them. This part is doable, albeit slowly. All\nthe config variables in git_default_config() can be rewritten using the\nnew non callback based functions easily as demonstrated in an earlier\nRFC patch.\n"},{"id":"250394","messageId":"20141008200936.GA19492@vauxhall.crustytoothpaste.net","threadId":"37689","inReplyTo":"CAD77+gTuqbZimEK-=gXY8o4wFqOicFvnQ5MnV6Fq1npuckwYFQ@mail.gmail.com","subject":"Re: [BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-10-08T20:09:36Z","receivedAt":"2014-10-08T20:09:36Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Oct 08, 2014 at 01:22:33PM +0200, Richard Hartmann wrote:\n> Notwithstanding if the setting is correct, shouldn't rev-parse be\n> resilient enough to at least be able to tell if we're in a work tree?\n> I understand why `git status` and the like would need to parse the\n> full config, but determining if you're in a work tree should be\n> possible in most if not all cases.\n> Unless detached work trees get you into a situation where you really\n> need to parse the whole config...\n\nI have seen similar problems where various git commands fail in the\nmiddle if a config file is the subject of merge conflicts. For example:\n\n vauxhall ok % git pull . master\n From .\n * branch master -> FETCH_HEAD\n Auto-merging .zshrc\n CONFLICT (content): Merge conflict in .zshrc\n Auto-merging .zshenv\n CONFLICT (content): Merge conflict in .zshenv\n Auto-merging .vimrc\n Auto-merging .gitconfig\n CONFLICT (content): Merge conflict in .gitconfig\n fatal: bad config file line 9 in .gitconfig\n\nThis tends to be one of the downsides of storing one's dotfiles in git.\nI usually work around it by specifying HOME=/tmp before a command I\nthink might cause a conflict in .gitconfig. I'm not sure there's any\ngood way around it, though.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"}]}