Re: includeIf not matching during `git rebase`

From: Glen Choo <chooglen@google.com>
Date: Jul 27, 2023, 18:08 UTC
Message-ID: <kl6lcz0dz0qg.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqila7h9bl.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

> Emily Shaffer <nasamuffin@google.com> writes:
>
>> Does that mean that the config machinery should also be using
>> wt_status to determine which branch to use?
>
> Not really.  The low-level config machinery shouldn't rely on a
> piece of information from so high a layer (making call to
> wt_status.c or spawning "git status" is an absolute no-no).

"includes" are surprisingly high-level and tacked-on compared to the rest of config parsing. Includes are implemented using just config callbacks; config_with_options() does the preparation and git_config_include() evaluates the includes. And, "includeIf"s already use all sorts of higher-level information (onbranch: is evaluated by resolving HEAD, gitdir knows about the repo setup). So I don't think it would be the worst thing to introduce such a check into config...

Show 12 quoted lines

> But "we are not exactly on branch X, but doing work on behalf of
> branch X" is a common situation during rebase and possibly bisect,
> and I agree that it is a good future direction to introduce a
> reliable low-level primitive to notice that condition.
>
> I however am hesitant to fully support such an idea, because I
> suspect that there may be cases such as "we are technically on
> branch Y, but actually doing work on behalf of branch X" or worse
> yet "we are on branch Z, but actually doing work on behalf of both
> branches X and Y", where there are more than one plausible branch,
> which is different from what HEAD points at, that
> include_by_branch() could use.

But I fully agree that this shouldn't be a one-off config-only change, and I don't think we fully understand the ramifications of how this change will affect _other_ parts of Git. I don't think we'll figure that out without testing, though. Perhaps we could put this behind a feature flag, probably even feature.experimental.