{"thread":{"id":"49819","subject":"[PATCH 0/1] Some left-over add-on for bw/config-h","startedAt":"2018-11-13T15:05:46Z","lastAt":"2018-11-14T21:42:16Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"363161","messageId":"pull.78.git.gitgitgadget@gmail.com","threadId":"49819","inReplyTo":null,"subject":"[PATCH 0/1] Some left-over add-on for bw/config-h","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-13T15:05:43Z","receivedAt":"2018-11-13T15:05:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Back when bw/config-h was developed (and backported to Git for Windows), I\ncame up with this patch. It seems to not be strictly necessary, but I like\nthe safety of falling back to the Git directory when no common directory is\nconfigured (for whatever reason).\n\nJohannes Schindelin (1):\n do_git_config_sequence(): fall back to git_dir if commondir is NULL\n\n config.c | 2 ++\n 1 file changed, 2 insertions(+)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-78/dscho/bw/config-h-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/78\n-- \ngitgitgadget\n"},{"id":"363162","messageId":"a3854e4ed8108c8fde5cd4e2107e90ccffafd49b.1542121541.git.gitgitgadget@gmail.com","threadId":"49819","inReplyTo":"pull.78.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] do_git_config_sequence(): fall back to git_dir if commondir is NULL","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-13T15:05:44Z","receivedAt":"2018-11-13T15:05:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nJust some defensive programming.\n\nSigned-off-by: Johannes Schindelin \n---\n config.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 4051e38823..279d0d7077 100644\n--- a/config.c\n+++ b/config.c\n@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct config_options *opts,\n \n \tif (opts->commondir)\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n+\telse if (opts->git_dir)\n+\t\trepo_config = mkpathdup(\"%s/config\", opts->git_dir);\n \telse\n \t\trepo_config = NULL;\n \n-- \ngitgitgadget\n"},{"id":"363320","messageId":"xmqqo9asnr3b.fsf@gitster-ct.c.googlers.com","threadId":"49819","inReplyTo":"pull.78.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] Some left-over add-on for bw/config-h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-14T05:52:24Z","receivedAt":"2018-11-14T05:52:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Back when bw/config-h was developed (and backported to Git for Windows), I\n> came up with this patch. It seems to not be strictly necessary, but I like\n> the safety of falling back to the Git directory when no common directory is\n> configured (for whatever reason).\n\nShouldn't that be diagnosed as an error with BUG()? That is, if we\nknow there is the current repository, and if commondir is not set,\nisn't it a bug that we want to look into in the start-up sequence?\n\nThe phrase \"for whatever reason\" makes me wonder if this is truly\n\"defensive programming\", rather than just sweeping the bug under the\nrug.\n\nFWIW, with this toy patch applied on top of this patch, all tests\nthat I usually run seem to pass.\n\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex c5726af14d..e667e2ebce 100644\n--- a/config.c\n+++ b/config.c\n@@ -1669,7 +1669,7 @@ static int do_git_config_sequence(const struct config_options *opts,\n \tif (opts->commondir)\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n \telse if (opts->git_dir)\n-\t\trepo_config = mkpathdup(\"%s/config\", opts->git_dir);\n+\t\tBUG(\"git-dir exists but no commondir?\");\n \telse\n \t\trepo_config = NULL;\n \n"},{"id":"363330","messageId":"20181114073158.GE19904@sigill.intra.peff.net","threadId":"49819","inReplyTo":"xmqqo9asnr3b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/1] Some left-over add-on for bw/config-h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-14T07:31:58Z","receivedAt":"2018-11-14T07:32:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2018 at 02:52:24PM +0900, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > Back when bw/config-h was developed (and backported to Git for Windows), I\n> > came up with this patch. It seems to not be strictly necessary, but I like\n> > the safety of falling back to the Git directory when no common directory is\n> > configured (for whatever reason).\n> \n> Shouldn't that be diagnosed as an error with BUG()? That is, if we\n> know there is the current repository, and if commondir is not set,\n> isn't it a bug that we want to look into in the start-up sequence?\n> \n> The phrase \"for whatever reason\" makes me wonder if this is truly\n> \"defensive programming\", rather than just sweeping the bug under the\n> rug.\n> \n> FWIW, with this toy patch applied on top of this patch, all tests\n> that I usually run seem to pass.\n\nHeh, I independently made the same change after reading the patch and\ncame here to report similar results.\n\nI think the key thing here is that git_dir is never meant to be used as\na source for config. It is there to let the \"includeIf gitdir\" directive\nwork. So it would indeed be surprising and a bug if we had one but not\nthe other.\n\n-Peff\n"},{"id":"363362","messageId":"pull.78.v2.git.gitgitgadget@gmail.com","threadId":"49819","inReplyTo":"pull.78.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Some left-over add-on for bw/config-h","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T13:59:00Z","receivedAt":"2018-11-14T13:59:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Back when bw/config-h was developed (and backported to Git for Windows), I\ncame up with a patch to use git_dir if commondir is NULL, and contributed\nthat as v1 of this patch. However, it was deemed a bug if that happens, so\nlet's instead detect that condition and report it.\n\nChange since v1:\n\n * Be loud about this bug instead of papering over it.\n\nJohannes Schindelin (1):\n config: report a bug if git_dir exists without commondir\n\n config.c | 2 ++\n 1 file changed, 2 insertions(+)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-78/dscho/bw/config-h-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/78\n\nRange-diff vs v1:\n\n 1: a3854e4ed8 ! 1: 0767f98378 do_git_config_sequence(): fall back to git_dir if commondir is NULL\n @@ -1,8 +1,9 @@\n Author: Johannes Schindelin \n \n - do_git_config_sequence(): fall back to git_dir if commondir is NULL\n + config: report a bug if git_dir exists without commondir\n \n - Just some defensive programming.\n + This did happen at some stage, and was fixed relatively quickly. Make\n + sure that we detect very quickly, too, should that happen again.\n \n Signed-off-by: Johannes Schindelin \n \n @@ -14,7 +15,7 @@\n \tif (opts->commondir)\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n +\telse if (opts->git_dir)\n -+\t\trepo_config = mkpathdup(\"%s/config\", opts->git_dir);\n ++\t\tBUG(\"git_dir without commondir\");\n \telse\n \t\trepo_config = NULL;\n \n\n-- \ngitgitgadget\n"},{"id":"363363","messageId":"0767f9837811c922c49c5aa148252f9e66320f18.1542203938.git.gitgitgadget@gmail.com","threadId":"49819","inReplyTo":"pull.78.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] config: report a bug if git_dir exists without commondir","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T13:59:02Z","receivedAt":"2018-11-14T13:59:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis did happen at some stage, and was fixed relatively quickly. Make\nsure that we detect very quickly, too, should that happen again.\n\nSigned-off-by: Johannes Schindelin \n---\n config.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 4051e38823..db6b0167c6 100644\n--- a/config.c\n+++ b/config.c\n@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct config_options *opts,\n \n \tif (opts->commondir)\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n+\telse if (opts->git_dir)\n+\t\tBUG(\"git_dir without commondir\");\n \telse\n \t\trepo_config = NULL;\n \n-- \ngitgitgadget\n"},{"id":"363397","messageId":"20181114214212.GB2960@sigill.intra.peff.net","threadId":"49819","inReplyTo":"0767f9837811c922c49c5aa148252f9e66320f18.1542203938.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] config: report a bug if git_dir exists without commondir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-14T21:42:12Z","receivedAt":"2018-11-14T21:42:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2018 at 05:59:02AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> This did happen at some stage, and was fixed relatively quickly. Make\n> sure that we detect very quickly, too, should that happen again.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> config.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/config.c b/config.c\n> index 4051e38823..db6b0167c6 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct config_options *opts,\n> \n> \tif (opts->commondir)\n> \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n> +\telse if (opts->git_dir)\n> +\t\tBUG(\"git_dir without commondir\");\n\nYeah, I think this is the right thing to do. Thanks!\n\n-Peff\n"}]}