includeIf not matching during `git rebase`

* What did you do before the bug happened? (Steps to reproduce your issue)

With ~/.gitconfig having:

[includeIf "onbranch:pr/"]
        path = .gitconfig.for-upstream

where the included config changes commit-msg hook,

git checkout pr/zzz # has multiple commits over upstream git rebase -i

(modify it) git rebase --continue

* What did you expect to happen? (Expected behavior)

The commit messages are unchanged.

* What happened instead? (Actual behavior)

The rebased commits had messages modified by default commit-msg hook.

* What's different between what you expected and what actually happened?

I'd expect includeIf to match and in effect prevent the default commit message modifications.

[System Info] git version: git version 2.41.0.487.g6d72f3e995-goog cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh compiler info: gnuc: 12.2 libc info: glibc: 2.36 $SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks] commit-msg pre-commit prepare-commit-msg

On Tue, Jul 25, 2023 at 12:49 PM Michał Mirosław <emmir@google.com> wrote:

Hm, I would guess this is why - in the middle of the rebase, the branch ref doesn't move, it's only moved to the new tip when the rebase is completed. However, it seems that we do have some knowledge of which branch we are trying to rebase:

emilyshaffer@podkayne:~/git [libification-style|REBASE 3/4]$ git
branch | head -n1
* (no branch, rebasing libification-style)

It looks like to log that error, we're using a cached branch name (and pick that up from wt_status_get_state and eventually wt_status_check_rebase, which checks .git/rebase-(merge|apply)/head-name).

However, when we check the current branch for includeif.onbranch (config.c:include_by_branch()) we're using resolve_ref_unsafe("HEAD"), which doesn't check the current rebase state the way that the wt_status_* stuff does.

Does that mean that the config machinery should also be using wt_status to determine which branch to use? The use case Michał is describing sounds perfectly reasonable to me - is there reason to think that doing conditional includes during a rebase based on "the branch we were in the middle of rebasing" would negatively impact someone's existing workflow?

 - Emily

Emily Shaffer <nasamuffin@google.com> writes:

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).

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.

Thanks.

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

"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...

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.