Re: [PATCH v4 0/2] Conditional config includes based on remote URL

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 30, 2021, 07:51 UTC
Message-ID: <xmqq4k7u2i9f.fsf@gitster.g>
In-Reply-To: <211129.864k7ug02c.gmgdl@evledraar.gmail.com>

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

Show 8 quoted lines

> 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?