Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx/ > > 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?
