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

On Tue, Dec 07 2021, Jonathan Tan wrote:

quoted

I read through this and came up with the below as a proposed squash-in
just while reading through it. These may or may not help. Changes:

 * There was some needless "$(pwd)" in the tests

Ah, thanks for catching that.

quoted

 * Inlining the "remote_urls" in the struct makes its management easier;
   and the free/NULL checks just check .nr now, and string_list_clear() can be
   unconditional.

I don't think we can do this - nr might still be 0 after a scan if we
don't have remote URLs for some reason, so we still need to distinguish
between not-scanned and scanned-with-zero-URLs.

You mean so that we don't double-free? The way string_list_clear()
protects against that, but maybe there's something else.

Whatever it is (if there's anything) it could use test coverage then :)

quoted

 * Created a include_by_remote_url() function. Makes the overall diff smaller
   since you don't need to add braces to everything in include_condition_is_true()

Ah, good idea. I'll do this.

quoted

Other comments (not related to the below):

 * It would be nice if e.g. the "includeIf.hasconfig:remote.*.url globs" test
   were split up by condition, but maybe that's a hassle (would need a small helper).

   Just something that would have helped while hacking on this, i.e. now most of it
   was an all-or-nothing failure & peek at the trace output

What do you mean by condition? There seems to only be one condition
(whether the URL is there or not), unless you were thinking of smaller
subdivisions.

Maybe I'm just misunderstanding the intent here, but aren't you trying
to guard against the case of having a ~/.gitconfig that includes
~/.gitconfig.d/for-this-url, and *that* file in turns changes the
remote's "url" in its config, followed by another "include if url
matches" condition therein?

I.e. I read (more like skimmed) the documentation & test at the end as
forbidding that, but maybe that's OK?

quoted

 * Your last test appears to entirely forbid recursion. I.e. we die if you include config
   which in turn tries to use this include mechanism, right?

   That's probably wise, and it is explicitly documented.

   But as far as the documentation about this being a forward-compatible facility, do we
   think that this limitation would apply to any future config key? I.e. if I include based
   on "user.email" nothing in that to-be-included can set user.email?

   That's probably OK, just wondering. In any case it can always be expanded later on.

We can decide later what the future facility will be, but I envision
that we will not allow included files to set config that can affect any
include directives in use. So, for example, if I have a user.email-based
include, none of my config-conditionally included files can set user.email.

I didn't look deeply at the implementation at all, but why would this be
a problem?

You parse ~/.gitconfig, it has user.name=foo, then right after in that
file we do:

    [includeIf "hasconfig:user.name:*foo*"]
    path = ~/.gitconfig.d/foo

Now the top of  ~/.gitconfig.d/foo we have:

    [user]
    name = bar
    [includeIf "hasconfig:user.name:*bar*"]
    path = ~/.gitconfig.d/bar

Why would it matter that we included on user.name=foo before?

Doesn't that only matter *while* we process that first "path" line? Once
we move past it we update our configset to user.name=bar once we hit the
"name" line of the included file.

Then when we get another "hasconfig:user.name" we just match it to our
current user.name=*bar*.

No?

Anyway, I think it's fine to punt on it for now or whatever, just
curious...