From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 30 Nov 2021 07:51:40 GMT
Subject: Re: [PATCH v4 0/2] Conditional config includes based on remote URL
Message-ID: <xmqq4k7u2i9f.fsf@gitster.g>
In-Reply-To: <211129.864k7ug02c.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Nov 29 2021, Jonathan Tan wrote:
>
>> Thanks everyone for your comments. Here's an update.
>
> Just from skimming this (minor) feedback on v3 still applies:
> https://lore.kernel.org/git/211123.86pmqrwtf2.gmgdl@evledraar.gmail.com/
>
> I.e. s/hasremoteurl/hasRemoteURL/ etc. in appropriate places.

Is there any appropriate place, though?

"hasremoteurl" is a new directive to be used as the leading part of
<condition> in the name of `includeIf.<condition>.path` variable.
The <condtion> part is case sensitive, and we do not want people to
spell it, and the existing "gitdir", "gitdir/i", and "onbranch", in
mixed cases.

See config.c::include_condition_is_true() function and its use of
skip_prefix_mem() to locate these existing conditions.

It is troubling that this patch is *NOT* extend the implementation
of include_condition_is_true() function (which gives a very clean
abstraction and makes the caller very readable); it instead mucks
with the caller of include_condition_is_true() and adds a parallel
logic that include_condition_is_true() does not know about.  It may
have been an expedite way to implement this, and the result may not
seem to hurt when include_condition_is_true() is called by only one
caller, but I find the resulting code structure unnecessarily ugly.

Can't the body of if (skip_prefix_mem(..."hasremoteurl:", ...)) block
become include_by_remoteurl() function, similar to include_by_foo()
functions include_condition_is_true() already calls?