fix(config): enable file include in scoped queries - #2345
Conversation
|
@becm Thanks for identifying this key piece that connects to several issues. Could you add a test to demonstrate the behavior change to prevent regressions in the future? |
|
@derrickstolee sadly only the 1st step, since:
|
|
@derrickstolee since the Will try to implement a test for (abstract) config data with the (hopefully) resolved corner case of |
|
Reverted PR to primary goal, will tackle @derrickstolee I'll try to add a test for file inclusion (global scope). |
This comment was marked as spam.
This comment was marked as spam.
|
@mjcheetham any further actions to be taken to accept this? This change will essentially harmonizes behavior before and after |
|
@derrickstolee it seems Would it be possible to introduce a further code path for This would be an extension or (for non-ancient Git versions) a replacement for the current PR. |
|
The way I read this change, it constitutes a change of behavior. A good one, granted, but one that has a high potential of affecting existing users' setups (especially complex setups). As such, I'd be much more comfortable to make this behavioral change part of a major version bump. Seeing as v3.x is starting to take concrete shape, I would consider that version to be the natural inflection point to integrate this here PR. |
|
@dscho behavior change already happened for This combination effectively uses |
@becm sure, but the way I see it was that this was an inadvertent behavioral change. Here, we are aware of the change. That makes a huge difference. |
|
@dscho the problem is inconsistent behavior when different Git versions are involved. So a "bugfix" is to intentionally lean into the (unintended) change. If adding config cache did not enforce |
|
Now that |
|
If there is serious intent to merge, I'd invest work to submit a better (more extensive) approach.
Only using The approach using |
|
Revised for
|
|
@becm thank you for rebasing! @mjcheetham and I should be able to focus on getting this merged this coming week. |
mjcheetham
left a comment
There was a problem hiding this comment.
Looking good so far, but a few issues at the moment.
redirect global config location test special conditional global include based on local Git remote value verify unconditional include directive for local scope
split Git version checks for support of "raw" and "typed" config listing use compatible argument to create config listing in older Git versions drop `--no-type` for Raw values (default) to support older Git versions
match query results of `ConfigCache` as close as possible
c9cf891 to
228be3d
Compare
mjcheetham
left a comment
There was a problem hiding this comment.
Thanks for persisting with this!
Avoid duplication of entries already present in included files:
Rawvalues in all Git versions supporting--show-scopeImproves consistency regarding include behavior and
Fixes #2342
Fixes #2328
Fixes #1696
Fixes #1337