{"thread":{"id":"57358","subject":"git-checkout doesn't seem to respect config from include.path","startedAt":"2022-02-02T16:04:53Z","lastAt":"2022-02-08T01:07:26Z","messageCount":10,"participants":["Greg Hurrell","brian m. carlson","Phillip Wood","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"447541","messageId":"ee1dd453-e698-440a-911b-d14389e33715@beta.fastmail.com","threadId":"57358","inReplyTo":null,"subject":"git-checkout doesn't seem to respect config from include.path","fromName":"Greg Hurrell","fromEmail":"greg@hurrell.net","sentAt":"2022-02-02T16:04:26Z","receivedAt":"2022-02-02T16:04:53Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"Hi,\n\nNot sure if this is confined specifically to `git-checkout`, but that's\nthe command I noticed the issue with:\n\nWith the release of the v2.35.0 and the \"zdiff3\" setting for\n\"merge.conflictStyle\", I find myself wanting to use \"zdiff3\" on machines\nrunning the new version, and falling back to \"diff3\" on machines with an\nolder version.\n\nTo this end, I have a ~/.gitconfig that contains:\n\n    [merge]\n    \tconflictStyle = zdiff3\n    [include]\n    \tpath = ~/.gitconfig.local\n\nThe idea is that I can use the same `~/.gitconfig` on every machine I\nuse, but on machines that only have an older Git version, I drop in a\n~/.gitconfig.local with overrides like this:\n\n    [merge]\n    \tconflictStyle = diff3\n\n`git config --get merge.conflictStyle` correctly reports that my setting is\n\"diff3\" on such machines, and `git config --get-all merge.conflictStyle`\nshows:\n\n    diff3\n    zdiff3\n\nIn other words, it knows that I have multiple values set, but it uses\na last-one-wins policy.\n\nHowever, when I try to run a command like `git checkout -b something`,\nGit dies with:\n\n    fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'\n\nSo, it looks like something in `git-checkout`'s option processing is\ncausing it to disregard the override set via \"include.path\". In fact, it\neven disregards a value passed in with `-c` like this:\n\n    git -c merge.conflictStyle=diff3 checkout -b something\n\nDoes this sound like a bug, or are my expectations off? I'd be happy to\nlook into fixing this, but first would like to know whether it is\nexpected behavior.\n\nCheers,\nGreg\n"},{"id":"447578","messageId":"YfsMYTo1tND5JKNx@camp.crustytoothpaste.net","threadId":"57358","inReplyTo":"ee1dd453-e698-440a-911b-d14389e33715@beta.fastmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-02-02T22:57:37Z","receivedAt":"2022-02-02T22:57:43Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-02-02 at 16:04:26, Greg Hurrell wrote:\n> However, when I try to run a command like `git checkout -b something`,\n> Git dies with:\n> \n>     fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'\n> \n> So, it looks like something in `git-checkout`'s option processing is\n> causing it to disregard the override set via \"include.path\". In fact, it\n> even disregards a value passed in with `-c` like this:\n> \n>     git -c merge.conflictStyle=diff3 checkout -b something\n> \n> Does this sound like a bug, or are my expectations off? I'd be happy to\n> look into fixing this, but first would like to know whether it is\n> expected behavior.\n\nThis definitely doesn't sound like the expected behavior here.  It's not\nclear to me why this is happening, but it probably shouldn't be.  It\ndoesn't appear that we fail to call the config callback, since we've\nbeen doing that since 2010.\n\nWhat version of Git are you using that's not working?\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"447589","messageId":"6a898459-ee74-49b1-96cb-98fd766d082d@beta.fastmail.com","threadId":"57358","inReplyTo":"YfsMYTo1tND5JKNx@camp.crustytoothpaste.net","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Greg Hurrell","fromEmail":"greg@hurrell.net","sentAt":"2022-02-03T07:48:42Z","receivedAt":"2022-02-03T07:49:04Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"On Wed, Feb 2, 2022, at 11:57 PM, brian m. carlson wrote:\n> This definitely doesn't sound like the expected behavior here.  It's not\n> clear to me why this is happening, but it probably shouldn't be.  It\n> doesn't appear that we fail to call the config callback, since we've\n> been doing that since 2010.\n> \n> What version of Git are you using that's not working?\n\nInitially noticed on v2.30.2, but you can reproduce it to similar\neffect on v2.35.0 (ie. add an invalid \"merge.conflictStyle\" value\nlike \"fizzbuzz\" to ~/.gitconfig, and a valid one like \"diff3\" to\na file pulled in via \"include.path\", and you'll see the same\nbehavior).\n\nCheers,\nGreg\n"},{"id":"447615","messageId":"0b8222c2-7337-7e8f-33d1-7926462daac1@gmail.com","threadId":"57358","inReplyTo":"ee1dd453-e698-440a-911b-d14389e33715@beta.fastmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-03T15:54:23Z","receivedAt":"2022-02-03T15:54:32Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Greg\n\nOn 02/02/2022 16:04, Greg Hurrell wrote:\n> Hi,\n> \n> Not sure if this is confined specifically to `git-checkout`, but that's\n> the command I noticed the issue with:\n> \n> With the release of the v2.35.0 and the \"zdiff3\" setting for\n> \"merge.conflictStyle\", I find myself wanting to use \"zdiff3\" on machines\n> running the new version, and falling back to \"diff3\" on machines with an\n> older version.\n> \n> To this end, I have a ~/.gitconfig that contains:\n> \n>      [merge]\n>      \tconflictStyle = zdiff3\n>      [include]\n>      \tpath = ~/.gitconfig.local\n> \n> The idea is that I can use the same `~/.gitconfig` on every machine I\n> use, but on machines that only have an older Git version, I drop in a\n> ~/.gitconfig.local with overrides like this:\n> \n>      [merge]\n>      \tconflictStyle = diff3\n> \n> `git config --get merge.conflictStyle` correctly reports that my setting is\n> \"diff3\" on such machines, and `git config --get-all merge.conflictStyle`\n> shows:\n> \n>      diff3\n>      zdiff3\n> \n> In other words, it knows that I have multiple values set, but it uses\n> a last-one-wins policy.\n> \n> However, when I try to run a command like `git checkout -b something`,\n> Git dies with:\n> \n>      fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'\n\nI think what is happening is that git parses each line of the config \nfile as it reads it so the old version of git sees \"zdiff3\" and errors \nout before it reads the include line. I'm afraid I don't have any useful \nsuggestions for avoiding this other than switching the include around so \nthat it contains zdiff3 and is only included by newer versions of git.\n\n> So, it looks like something in `git-checkout`'s option processing is\n> causing it to disregard the override set via \"include.path\". In fact, it\n> even disregards a value passed in with `-c` like this:\n> \n>      git -c merge.conflictStyle=diff3 checkout -b something\n\nI think the values passed with -c are parsed after all the config files \nso the override works. What we really want in this case is to store the \nstring value for each config option as we read each config source and \nthen parse those values at the end, unfortunately I think that would \nbreak multi-valued config keys.\n\nBest Wishes\n\nPhillip\n\n> Does this sound like a bug, or are my expectations off? I'd be happy to\n> look into fixing this, but first would like to know whether it is\n> expected behavior.\n> \n> Cheers,\n> Greg\n"},{"id":"447624","messageId":"3f1972f3-c764-41e9-9853-8f1c303d4f6b@beta.fastmail.com","threadId":"57358","inReplyTo":"0b8222c2-7337-7e8f-33d1-7926462daac1@gmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Greg Hurrell","fromEmail":"greg@hurrell.net","sentAt":"2022-02-03T17:39:58Z","receivedAt":"2022-02-03T17:40:23Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"On Thu, Feb 3, 2022, at 4:54 PM, Phillip Wood wrote:\n> Hi Greg\n> \n> On 02/02/2022 16:04, Greg Hurrell wrote:\n> \n> > `git config --get merge.conflictStyle` correctly reports that my setting is\n> > \"diff3\" on such machines, and `git config --get-all merge.conflictStyle`\n> > shows:\n> > \n> >      diff3\n> >      zdiff3\n> > \n> > In other words, it knows that I have multiple values set, but it uses\n> > a last-one-wins policy.\n> > \n> > However, when I try to run a command like `git checkout -b something`,\n> > Git dies with:\n> > \n> >      fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'\n> \n> I think what is happening is that git parses each line of the config \n> file as it reads it so the old version of git sees \"zdiff3\" and errors \n> out before it reads the include line.\n\nThat gave me the idea of moving the `include.path` setting higher up in\nthe file, to see if `git checkout` would consult that value first, but\nit doesn't work; `git config merge.conflictStyle` shows the value from\nthe file indicated in `include.path`, but a command like `git checkout`\nstill dies based on the value in ~/.gitconfig.\n\nOverall this points to the general problem that it is not only hard to\nmake a single config that works on different machines, but it's hard to\nmake a _combination_ of files that works on different machines.\n\nFor now, I think my workaround is going to be templating out\nmachine-specific files.\n\nGreg\n\n"},{"id":"447625","messageId":"606851c7-110d-486e-9e39-0a00444caad8@beta.fastmail.com","threadId":"57358","inReplyTo":"3f1972f3-c764-41e9-9853-8f1c303d4f6b@beta.fastmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Greg Hurrell","fromEmail":"greg@hurrell.net","sentAt":"2022-02-03T17:42:40Z","receivedAt":"2022-02-03T17:43:06Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"Minor correction to what I said here:\n\nOn Thu, Feb 3, 2022, at 6:39 PM, Greg Hurrell wrote:\n> \n> That gave me the idea of moving the `include.path` setting higher up in\n> the file, to see if `git checkout` would consult that value first, but\n> it doesn't work; `git config merge.conflictStyle` shows the value from\n> the file indicated in `include.path`\n\nIt only did that because I forgot to remove the original `include.path`\nfrom the bottom of the config. Once I removed that, `git config` showed\nthe value from ~/.gitconfig. `git checkout` behaved the same either way.\n\nGreg\n\n"},{"id":"447628","messageId":"xmqq1r0jx1qm.fsf@gitster.g","threadId":"57358","inReplyTo":"0b8222c2-7337-7e8f-33d1-7926462daac1@gmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-03T18:07:45Z","receivedAt":"2022-02-03T18:07:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> ... What we really want in this case is to\n> store the string value for each config option as we read each config\n> source and then parse those values at the end, unfortunately I think\n> that would break multi-valued config keys.\n\nThanks for raising, and looking into, the issue.\n\nWhile the original \"callback functions are called for each and every\nconfiguration item defined in the files and it is the responsibility\nfor these callback functions to implement the semantics like the\nlast one wins\" design that uses git_config() makes it harder, but I\nthink we are already halfway there, with the more recent API update\nin 2014 (!) that allows config_get_value() to go directly get a\nvalue given a key without writing callback functions.\n\nI think builtin/add.c predates the configset API work (of course, it\nis natural that we can \"git add\" way before 2014), and mostly uses\ngit_config(add_config) callback as a way to parse its configuration,\nbecause it needs to tell other subsystems (like diff, merge, etc.)\nthat are even older to pay attention to the configuration variables\nthey care about.\n\nSo it may be a major surgery to switch to the newer\nconfig_get_value() API.\n\nFor a \"last one wins\" variable, config_get_value() will only look at\nthe last item, so any garbage value Git does not recognize would not\ntrigger a fatal error.\n\nSuch an update is both good and bad.  Surely it makes the scenario\nthat triggered this discussion more pleasant by not dying, but it\nmakes it too pleasant by not even giving the user a chance to notice\na possible typo.\n\nA incremental improvement that we can immediately make is probably\nto teach the current xdiff-interface.c::git_xmerge_config() parser\nto react to an unknown value differently.  It should not die() but\njust ignore the unknown value, and issue a warning.  This should be\ndoable with minimum impact to the code.\n\nCompletely untested.  The first test that would be interesting to\nrun is how many tests this changes breaks to gauge how good test\ncoverage we have ;-)\n\n xdiff-interface.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/xdiff-interface.c w/xdiff-interface.c\nindex 2e3a5a2943..523b04960a 100644\n--- c/xdiff-interface.c\n+++ w/xdiff-interface.c\n@@ -322,8 +322,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n \t\t * git-completion.bash when you add new merge config\n \t\t */\n \t\telse\n-\t\t\tdie(\"unknown style '%s' given for '%s'\",\n-\t\t\t    value, var);\n+\t\t\twarning(\"ignored unknown style '%s' given for '%s'\",\n+\t\t\t\tvalue, var);\n \t\treturn 0;\n \t}\n \treturn git_default_config(var, value, cb);\n"},{"id":"447884","messageId":"e557db22-038b-cb78-9518-3873d7d69ee8@gmail.com","threadId":"57358","inReplyTo":"3f1972f3-c764-41e9-9853-8f1c303d4f6b@beta.fastmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-07T14:05:16Z","receivedAt":"2022-02-07T14:28:38Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Greg\n\nOn 03/02/2022 17:39, Greg Hurrell wrote:\n> On Thu, Feb 3, 2022, at 4:54 PM, Phillip Wood wrote:\n>> Hi Greg\n>>\n>> On 02/02/2022 16:04, Greg Hurrell wrote:\n>>\n>>> `git config --get merge.conflictStyle` correctly reports that my setting is\n>>> \"diff3\" on such machines, and `git config --get-all merge.conflictStyle`\n>>> shows:\n>>>\n>>>       diff3\n>>>       zdiff3\n>>>\n>>> In other words, it knows that I have multiple values set, but it uses\n>>> a last-one-wins policy.\n>>>\n>>> However, when I try to run a command like `git checkout -b something`,\n>>> Git dies with:\n>>>\n>>>       fatal: unknown style 'zdiff3' given for 'merge.conflictstyle'\n>>\n>> I think what is happening is that git parses each line of the config\n>> file as it reads it so the old version of git sees \"zdiff3\" and errors\n>> out before it reads the include line.\n> \n> That gave me the idea of moving the `include.path` setting higher up in\n> the file, to see if `git checkout` would consult that value first, but\n> it doesn't work; `git config merge.conflictStyle` shows the value from\n> the file indicated in `include.path`, but a command like `git checkout`\n> still dies based on the value in ~/.gitconfig.\n\nYes, you need to move the \"zdiff3\" setting into an include file that is \nonly read by recent versions of git.\n\n> Overall this points to the general problem that it is not only hard to\n> make a single config that works on different machines, but it's hard to\n> make a _combination_ of files that works on different machines.\n\nIf you make sure that you are only including files containing recent \nconfig keys on machines running recent git versions then it should work \nbut arranging for that to happen is not necessarily easy\n\nBest Wishes\n\nPhillip\n\n> For now, I think my workaround is going to be templating out\n> machine-specific files.\n> \n> Greg\n> \n\n"},{"id":"447887","messageId":"bb0532ca-f718-15d1-7328-fd0e062eae06@gmail.com","threadId":"57358","inReplyTo":"xmqq1r0jx1qm.fsf@gitster.g","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-07T14:01:26Z","receivedAt":"2022-02-07T14:32:23Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 03/02/2022 18:07, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> ... What we really want in this case is to\n>> store the string value for each config option as we read each config\n>> source and then parse those values at the end, unfortunately I think\n>> that would break multi-valued config keys.\n> \n> Thanks for raising, and looking into, the issue.\n> \n> While the original \"callback functions are called for each and every\n> configuration item defined in the files and it is the responsibility\n> for these callback functions to implement the semantics like the\n> last one wins\" design that uses git_config() makes it harder, but I\n> think we are already halfway there, with the more recent API update\n> in 2014 (!) that allows config_get_value() to go directly get a\n> value given a key without writing callback functions.\n> \n> I think builtin/add.c predates the configset API work (of course, it\n> is natural that we can \"git add\" way before 2014), and mostly uses\n> git_config(add_config) callback as a way to parse its configuration,\n> because it needs to tell other subsystems (like diff, merge, etc.)\n> that are even older to pay attention to the configuration variables\n> they care about.\n> \n> So it may be a major surgery to switch to the newer\n> config_get_value() API.\n> \n> For a \"last one wins\" variable, config_get_value() will only look at\n> the last item, so any garbage value Git does not recognize would not\n> trigger a fatal error.\n> \n> Such an update is both good and bad.  Surely it makes the scenario\n> that triggered this discussion more pleasant by not dying, but it\n> makes it too pleasant by not even giving the user a chance to notice\n> a possible typo.\n> \n> A incremental improvement that we can immediately make is probably\n> to teach the current xdiff-interface.c::git_xmerge_config() parser\n> to react to an unknown value differently.  It should not die() but\n> just ignore the unknown value, and issue a warning.  This should be\n> doable with minimum impact to the code.\n\nI think that would be worthwhile, the warning is potentially confusing \nthough if a bad value is followed by a good value then we will warn \nabout the bad value but use the good one.\n\nBest Wishes\n\nPhillip\n\n> Completely untested.  The first test that would be interesting to\n> run is how many tests this changes breaks to gauge how good test\n> coverage we have ;-)\n> \n>   xdiff-interface.c | 4 ++--\n>   1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git c/xdiff-interface.c w/xdiff-interface.c\n> index 2e3a5a2943..523b04960a 100644\n> --- c/xdiff-interface.c\n> +++ w/xdiff-interface.c\n> @@ -322,8 +322,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n>   \t\t * git-completion.bash when you add new merge config\n>   \t\t */\n>   \t\telse\n> -\t\t\tdie(\"unknown style '%s' given for '%s'\",\n> -\t\t\t    value, var);\n> +\t\t\twarning(\"ignored unknown style '%s' given for '%s'\",\n> +\t\t\t\tvalue, var);\n>   \t\treturn 0;\n>   \t}\n>   \treturn git_default_config(var, value, cb);\n\n"},{"id":"447931","messageId":"xmqqsfsufd83.fsf@gitster.g","threadId":"57358","inReplyTo":"bb0532ca-f718-15d1-7328-fd0e062eae06@gmail.com","subject":"Re: git-checkout doesn't seem to respect config from include.path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-07T23:50:36Z","receivedAt":"2022-02-08T01:07:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I think that would be worthwhile, the warning is potentially confusing\n> though if a bad value is followed by a good value then we will warn \n> about the bad value but use the good one.\n\nI dunno.  That is exactly why the new message is crafted to convey:\n\"you have an entry with an unsupported value in your configuration\nfile, which you may want to inspect and possibly correct it; in the\nmeantime we've ignored that entry\".  \"ignored\" is the key word.\n\nIf we say \"we later found this good value so we'd use it\", it may\nbecome confusing, as we'd never issue such a notice for a\nlast-one-wins variable that do not use any unsupported values, but\nwe are not doing that, so I think there is no room for confusion.\n\n\n>> Completely untested.  The first test that would be interesting to\n>> run is how many tests this changes breaks to gauge how good test\n>> coverage we have ;-)\n>>   xdiff-interface.c | 4 ++--\n>>   1 file changed, 2 insertions(+), 2 deletions(-)\n>> diff --git c/xdiff-interface.c w/xdiff-interface.c\n>> index 2e3a5a2943..523b04960a 100644\n>> --- c/xdiff-interface.c\n>> +++ w/xdiff-interface.c\n>> @@ -322,8 +322,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n>>   \t\t * git-completion.bash when you add new merge config\n>>   \t\t */\n>>   \t\telse\n>> -\t\t\tdie(\"unknown style '%s' given for '%s'\",\n>> -\t\t\t    value, var);\n>> +\t\t\twarning(\"ignored unknown style '%s' given for '%s'\",\n>> +\t\t\t\tvalue, var);\n>>   \t\treturn 0;\n>>   \t}\n>>   \treturn git_default_config(var, value, cb);\n"}]}