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

From: Jeff King <peff@peff.net>
Date: Nov 14, 2018, 07:31 UTC
Message-ID: <20181114073158.GE19904@sigill.intra.peff.net>
In-Reply-To: <xmqqo9asnr3b.fsf@gitster-ct.c.googlers.com>

On Wed, Nov 14, 2018 at 02:52:24PM +0900, Junio C Hamano wrote:

Show 18 quoted lines

> "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.

Heh, I independently made the same change after reading the patch and came here to report similar results.

I think the key thing here is that git_dir is never meant to be used as a source for config. It is there to let the "includeIf gitdir" directive work. So it would indeed be surprising and a bug if we had one but not the other.

-Peff