Re: [PATCH 0/1] Some left-over add-on for bw/config-h

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2018, 05:52 UTC
Message-ID: <xmqqo9asnr3b.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <pull.78.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Back when bw/config-h was developed (and backported to Git for Windows), I
> came up with this patch. It seems to not be strictly necessary, but I like
> the safety of falling back to the Git directory when no common directory is
> configured (for whatever reason).

Shouldn't that be diagnosed as an error with BUG()? That is, if we know there is the current repository, and if commondir is not set, isn't it a bug that we want to look into in the start-up sequence?

The phrase "for whatever reason" makes me wonder if this is truly "defensive programming", rather than just sweeping the bug under the rug.

FWIW, with this toy patch applied on top of this patch, all tests that I usually run seem to pass.

 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index c5726af14d..e667e2ebce 100644
--- a/config.c
+++ b/config.c
@@ -1669,7 +1669,7 @@ static int do_git_config_sequence(const struct config_options *opts,
 	if (opts->commondir)
 		repo_config = mkpathdup("%s/config", opts->commondir);
 	else if (opts->git_dir)
-		repo_config = mkpathdup("%s/config", opts->git_dir);
+		BUG("git-dir exists but no commondir?");
 	else
 		repo_config = NULL;