{"thread":{"id":"60027","subject":"includeIf not matching during `git rebase`","startedAt":"2023-07-25T19:49:31Z","lastAt":"2023-07-27T18:08:26Z","messageCount":4,"participants":["Michał Mirosław","Emily Shaffer","Junio C Hamano","Glen Choo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"479851","messageId":"CABb0KFF1vqMLa5DLYd_c9sQeZbhkhQ=Q0bE7W41nmMFmNWB4tg@mail.gmail.com","threadId":"60027","inReplyTo":null,"subject":"includeIf not matching during `git rebase`","fromName":"Michał Mirosław","fromEmail":"emmir@google.com","sentAt":"2023-07-25T19:49:15Z","receivedAt":"2023-07-25T19:49:31Z","isPatch":false,"sender":{"key":"emmir@google.com","avatar":null},"body":"* What did you do before the bug happened? (Steps to reproduce your issue)\n\nWith ~/.gitconfig having:\n\n[includeIf \"onbranch:pr/\"]\n        path = .gitconfig.for-upstream\n\nwhere the included config changes commit-msg hook,\n\ngit checkout pr/zzz # has multiple commits over upstream\ngit rebase -i\n> 'edit' first commit\n(modify it)\ngit rebase --continue\n\n* What did you expect to happen? (Expected behavior)\n\nThe commit messages are unchanged.\n\n* What happened instead? (Actual behavior)\n\nThe rebased commits had messages modified by default commit-msg hook.\n\n* What's different between what you expected and what actually happened?\n\nI'd expect includeIf to match and in effect prevent the default commit\nmessage modifications.\n\n[System Info]\ngit version:\ngit version 2.41.0.487.g6d72f3e995-goog\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\ncompiler info: gnuc: 12.2\nlibc info: glibc: 2.36\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\ncommit-msg\npre-commit\nprepare-commit-msg\n"},{"id":"479852","messageId":"CAJoAoZnuLxyQ7ufUTrK4mBJ_4sQoyPCqJD9eeS8XfquWue1xQA@mail.gmail.com","threadId":"60027","inReplyTo":"CABb0KFF1vqMLa5DLYd_c9sQeZbhkhQ=Q0bE7W41nmMFmNWB4tg@mail.gmail.com","subject":"Re: includeIf not matching during `git rebase`","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2023-07-25T20:28:08Z","receivedAt":"2023-07-25T20:28:24Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 25, 2023 at 12:49 PM Michał Mirosław <emmir@google.com> wrote:\n>\n> * What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> With ~/.gitconfig having:\n>\n> [includeIf \"onbranch:pr/\"]\n>         path = .gitconfig.for-upstream\n>\n> where the included config changes commit-msg hook,\n>\n> git checkout pr/zzz # has multiple commits over upstream\n> git rebase -i\n> > 'edit' first commit\n> (modify it)\n> git rebase --continue\n\nHm, I would guess this is why - in the middle of the rebase, the\nbranch ref doesn't move, it's only moved to the new tip when the\nrebase is completed. However, it seems that we do have some knowledge\nof which branch we are trying to rebase:\n\nemilyshaffer@podkayne:~/git [libification-style|REBASE 3/4]$ git\nbranch | head -n1\n* (no branch, rebasing libification-style)\n\nIt looks like to log that error, we're using a cached branch name (and\npick that up from wt_status_get_state and eventually\nwt_status_check_rebase, which checks\n.git/rebase-(merge|apply)/head-name).\n\nHowever, when we check the current branch for includeif.onbranch\n(config.c:include_by_branch()) we're using resolve_ref_unsafe(\"HEAD\"),\nwhich doesn't check the current rebase state the way that the\nwt_status_* stuff does.\n\nDoes that mean that the config machinery should also be using\nwt_status to determine which branch to use? The use case Michał is\ndescribing sounds perfectly reasonable to me - is there reason to\nthink that doing conditional includes during a rebase based on \"the\nbranch we were in the middle of rebasing\" would negatively impact\nsomeone's existing workflow?\n\n - Emily\n"},{"id":"479877","messageId":"xmqqila7h9bl.fsf@gitster.g","threadId":"60027","inReplyTo":"CAJoAoZnuLxyQ7ufUTrK4mBJ_4sQoyPCqJD9eeS8XfquWue1xQA@mail.gmail.com","subject":"Re: includeIf not matching during `git rebase`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-25T23:15:26Z","receivedAt":"2023-07-25T23:15:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <nasamuffin@google.com> writes:\n\n> emilyshaffer@podkayne:~/git [libification-style|REBASE 3/4]$ git\n> branch | head -n1\n> * (no branch, rebasing libification-style)\n>\n> It looks like to log that error, we're using a cached branch name (and\n> pick that up from wt_status_get_state and eventually\n> wt_status_check_rebase, which checks\n> .git/rebase-(merge|apply)/head-name).\n>\n> However, when we check the current branch for includeif.onbranch\n> (config.c:include_by_branch()) we're using resolve_ref_unsafe(\"HEAD\"),\n> which doesn't check the current rebase state the way that the\n> wt_status_* stuff does.\n>\n> Does that mean that the config machinery should also be using\n> wt_status to determine which branch to use?\n\nNot really.  The low-level config machinery shouldn't rely on a\npiece of information from so high a layer (making call to\nwt_status.c or spawning \"git status\" is an absolute no-no).\n\nBut \"we are not exactly on branch X, but doing work on behalf of\nbranch X\" is a common situation during rebase and possibly bisect,\nand I agree that it is a good future direction to introduce a\nreliable low-level primitive to notice that condition.\n\nI however am hesitant to fully support such an idea, because I\nsuspect that there may be cases such as \"we are technically on\nbranch Y, but actually doing work on behalf of branch X\" or worse\nyet \"we are on branch Z, but actually doing work on behalf of both\nbranches X and Y\", where there are more than one plausible branch,\nwhich is different from what HEAD points at, that\ninclude_by_branch() could use.\n\nThanks.\n"},{"id":"479934","messageId":"kl6lcz0dz0qg.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"60027","inReplyTo":"xmqqila7h9bl.fsf@gitster.g","subject":"Re: includeIf not matching during `git rebase`","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-07-27T18:08:07Z","receivedAt":"2023-07-27T18:08:26Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Emily Shaffer <nasamuffin@google.com> writes:\n>\n>> Does that mean that the config machinery should also be using\n>> wt_status to determine which branch to use?\n>\n> Not really.  The low-level config machinery shouldn't rely on a\n> piece of information from so high a layer (making call to\n> wt_status.c or spawning \"git status\" is an absolute no-no).\n\n\"includes\" are surprisingly high-level and tacked-on compared to the\nrest of config parsing. Includes are implemented using just config\ncallbacks; config_with_options() does the preparation and\ngit_config_include() evaluates the includes. And, \"includeIf\"s already\nuse all sorts of higher-level information (onbranch: is evaluated by\nresolving HEAD, gitdir knows about the repo setup). So I don't think it\nwould be the worst thing to introduce such a check into config...\n\n> But \"we are not exactly on branch X, but doing work on behalf of\n> branch X\" is a common situation during rebase and possibly bisect,\n> and I agree that it is a good future direction to introduce a\n> reliable low-level primitive to notice that condition.\n>\n> I however am hesitant to fully support such an idea, because I\n> suspect that there may be cases such as \"we are technically on\n> branch Y, but actually doing work on behalf of branch X\" or worse\n> yet \"we are on branch Z, but actually doing work on behalf of both\n> branches X and Y\", where there are more than one plausible branch,\n> which is different from what HEAD points at, that\n> include_by_branch() could use.\n\nBut I fully agree that this shouldn't be a one-off config-only change,\nand I don't think we fully understand the ramifications of how this\nchange will affect _other_ parts of Git. I don't think we'll figure that\nout without testing, though. Perhaps we could put this behind a feature\nflag, probably even feature.experimental.\n"}]}