{"thread":{"id":"47127","subject":"[PATCH] wt-status: actually ignore submodules when requested","startedAt":"2017-11-06T22:08:29Z","lastAt":"2017-11-06T22:52:20Z","messageCount":3,"participants":["Brandon Williams","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"331947","messageId":"20171106220819.178881-1-bmwill@google.com","threadId":"47127","inReplyTo":"CAGHpTB+jKiXr45tKVEVTtszN7OBTW7W_FqKu7aAjsB8Tmx9N3Q@mail.gmail.com","subject":"[PATCH] wt-status: actually ignore submodules when requested","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-06T22:08:19Z","receivedAt":"2017-11-06T22:08:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Since ff6f1f564 (submodule-config: lazy-load a repository's .gitmodules\nfile, 2017-08-03) rebase interactive fails if there are any submodules\nwith unstaged changes which have been configured with a value for\n'submodule.<name>.ignore' in the repository's config.\n\nThis is due to how configured values of 'submodule.<name>.ignore' are\nhandled in addition to a change in how the submodule config is loaded.\nWhen the diff machinery hits a submodule (gitlink as well as a\ncorresponding entry in the submodule subsystem) it will read the value\nof 'submodule.<name>.ignore' stored in the repository's config and if\nthe config is present it will clear the 'IGNORE_SUBMODULES' (which is\nthe flag explicitly requested by rebase interactive),\n'IGNORE_UNTRACKED_IN_SUBMODULES', and 'IGNORE_DIRTY_SUBMODULES' diff\nflags and then set one of them based on the configured value.\n\nHistorically this wasn't a problem because the submodule subsystem\nwasn't initialized because the .gitmodules file wasn't explicitly loaded\nby the rebase interactive command.  So when the diff machinery hit a\nsubmodule it would skip over reading any configured values of\n'submodule.<name>.ignore'.\n\nIn order to preserve the behavior of submodules being ignored by rebase\ninteractive, also set the 'OVERRIDE_SUBMODULE_CONFIG' diff flag when\nsubmodules are requested to be ignored when checking for unstaged\nchanges.\n\nReported-by: Orgad Shaneh <orgads@gmail.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n t/t3426-rebase-submodule.sh | 16 ++++++++++++++++\n wt-status.c                 |  4 +++-\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh\nindex ebf4f5e4b..760c872e2 100755\n--- a/t/t3426-rebase-submodule.sh\n+++ b/t/t3426-rebase-submodule.sh\n@@ -40,4 +40,20 @@ git_rebase_interactive () {\n \n test_submodule_switch \"git_rebase_interactive\"\n \n+test_expect_success 'rebase interactive ignores modified submodules' '\n+\ttest_when_finished \"rm -rf super sub\" &&\n+\tgit init sub &&\n+\tgit -C sub commit --allow-empty -m \"Initial commit\" &&\n+\tgit init super &&\n+\tgit -C super submodule add ../sub &&\n+\tgit -C super config submodule.sub.ignore dirty &&\n+\t> super/foo &&\n+\tgit -C super add foo &&\n+\tgit -C super commit -m \"Initial commit\" &&\n+\ttest_commit -C super a &&\n+\ttest_commit -C super b &&\n+\ttest_commit -C super/sub c &&\n+\tgit -C super rebase -i HEAD^^\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 29bc64cc0..94e5ebaf8 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2262,8 +2262,10 @@ int has_unstaged_changes(int ignore_submodules)\n \tint result;\n \n \tinit_revisions(&rev_info, NULL);\n-\tif (ignore_submodules)\n+\tif (ignore_submodules) {\n \t\tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n+\t\tDIFF_OPT_SET(&rev_info.diffopt, OVERRIDE_SUBMODULE_CONFIG);\n+\t}\n \tDIFF_OPT_SET(&rev_info.diffopt, QUICK);\n \tdiff_setup_done(&rev_info.diffopt);\n \tresult = run_diff_files(&rev_info, 0);\n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"331948","messageId":"CAGZ79kY5d3hhBCstY+UPpjhDbq=v_QXL+vtKz1fLdxo2Qhj5DA@mail.gmail.com","threadId":"47127","inReplyTo":"20171106220819.178881-1-bmwill@google.com","subject":"Re: [PATCH] wt-status: actually ignore submodules when requested","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-06T22:44:54Z","receivedAt":"2017-11-06T22:45:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 6, 2017 at 2:08 PM, Brandon Williams <bmwill@google.com> wrote:\n> Since ff6f1f564 (submodule-config: lazy-load a repository's .gitmodules\n> file, 2017-08-03) rebase interactive fails if there are any submodules\n> with unstaged changes which have been configured with a value for\n> 'submodule.<name>.ignore' in the repository's config.\n>\n> This is due to how configured values of 'submodule.<name>.ignore' are\n> handled in addition to a change in how the submodule config is loaded.\n> When the diff machinery hits a submodule (gitlink as well as a\n> corresponding entry in the submodule subsystem) it will read the value\n> of 'submodule.<name>.ignore' stored in the repository's config and if\n> the config is present it will clear the 'IGNORE_SUBMODULES' (which is\n> the flag explicitly requested by rebase interactive),\n> 'IGNORE_UNTRACKED_IN_SUBMODULES', and 'IGNORE_DIRTY_SUBMODULES' diff\n> flags and then set one of them based on the configured value.\n>\n> Historically this wasn't a problem because the submodule subsystem\n> wasn't initialized because the .gitmodules file wasn't explicitly loaded\n> by the rebase interactive command.  So when the diff machinery hit a\n> submodule it would skip over reading any configured values of\n> 'submodule.<name>.ignore'.\n>\n> In order to preserve the behavior of submodules being ignored by rebase\n> interactive, also set the 'OVERRIDE_SUBMODULE_CONFIG' diff flag when\n> submodules are requested to be ignored when checking for unstaged\n> changes.\n>\n> Reported-by: Orgad Shaneh <orgads@gmail.com>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  t/t3426-rebase-submodule.sh | 16 ++++++++++++++++\n>  wt-status.c                 |  4 +++-\n>  2 files changed, 19 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh\n> index ebf4f5e4b..760c872e2 100755\n> --- a/t/t3426-rebase-submodule.sh\n> +++ b/t/t3426-rebase-submodule.sh\n> @@ -40,4 +40,20 @@ git_rebase_interactive () {\n>\n>  test_submodule_switch \"git_rebase_interactive\"\n>\n> +test_expect_success 'rebase interactive ignores modified submodules' '\n> +       test_when_finished \"rm -rf super sub\" &&\n> +       git init sub &&\n> +       git -C sub commit --allow-empty -m \"Initial commit\" &&\n> +       git init super &&\n> +       git -C super submodule add ../sub &&\n> +       git -C super config submodule.sub.ignore dirty &&\n> +       > super/foo &&\n> +       git -C super add foo &&\n> +       git -C super commit -m \"Initial commit\" &&\n> +       test_commit -C super a &&\n> +       test_commit -C super b &&\n> +       test_commit -C super/sub c &&\n> +       git -C super rebase -i HEAD^^\n\nThe previous test `set_fake_editor` already, though I am unsure\nabout the current directory. It turns out that the setting of the fake\neditor is done via environment variable using absolute path to\nthe generated `fake_editor.sh`, hence it works even when\ninvoking a rebase in another directory/repo. Spooky.\n\nAlso we do want to be able to skip previous tests,\nwhich this might be a problem for. Can we have a 'setup'\nthat sets up the fake editor instead of repeating it here or\nhoping the previous tests has run? (Calling for a refactoring\nduring a regression fix is bad taste, so maybe just take this\npatch as is and put it to the #leftoverbits ?)\n\n> +'\n> +\n>  test_done\n> diff --git a/wt-status.c b/wt-status.c\n> index 29bc64cc0..94e5ebaf8 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -2262,8 +2262,10 @@ int has_unstaged_changes(int ignore_submodules)\n>         int result;\n>\n>         init_revisions(&rev_info, NULL);\n> -       if (ignore_submodules)\n> +       if (ignore_submodules) {\n>                 DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n> +               DIFF_OPT_SET(&rev_info.diffopt, OVERRIDE_SUBMODULE_CONFIG);\n> +       }\n\nThere are a couple of submodule related flags:\n\n#define DIFF_OPT_IGNORE_SUBMODULES   (1 << 18)\n...\n#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)\n#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)\n#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)\n#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)\n\nWe don't need to pay attention to 24,25,26 here, because\nDIRTY_SUBMODULES and IGN_* are only used to specify further\ninterest, the generic INORE_SUBMODULES turns off any submodule\nhandling. (so if we were to turn them on as well, it would still be correct,\nthough it may have performance impact -- I shortly wondered if we'd rather\nwant to have a mask covering all submodule related flags, specifically\nall flags invented in the future ;) )\n\nThe patch looks good to me (i.e. I am convinced by review it fixes the\nregression), so maybe we can put the test refactor on top of it, which\nthen doesn't need fast-tracking down to the maintenance track?\n\nThanks,\nStefan\n"},{"id":"331949","messageId":"20171106225212.GB131418@google.com","threadId":"47127","inReplyTo":"CAGZ79kY5d3hhBCstY+UPpjhDbq=v_QXL+vtKz1fLdxo2Qhj5DA@mail.gmail.com","subject":"Re: [PATCH] wt-status: actually ignore submodules when requested","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-06T22:52:12Z","receivedAt":"2017-11-06T22:52:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/06, Stefan Beller wrote:\n> On Mon, Nov 6, 2017 at 2:08 PM, Brandon Williams <bmwill@google.com> wrote:\n> > Since ff6f1f564 (submodule-config: lazy-load a repository's .gitmodules\n> > file, 2017-08-03) rebase interactive fails if there are any submodules\n> > with unstaged changes which have been configured with a value for\n> > 'submodule.<name>.ignore' in the repository's config.\n> >\n> > This is due to how configured values of 'submodule.<name>.ignore' are\n> > handled in addition to a change in how the submodule config is loaded.\n> > When the diff machinery hits a submodule (gitlink as well as a\n> > corresponding entry in the submodule subsystem) it will read the value\n> > of 'submodule.<name>.ignore' stored in the repository's config and if\n> > the config is present it will clear the 'IGNORE_SUBMODULES' (which is\n> > the flag explicitly requested by rebase interactive),\n> > 'IGNORE_UNTRACKED_IN_SUBMODULES', and 'IGNORE_DIRTY_SUBMODULES' diff\n> > flags and then set one of them based on the configured value.\n> >\n> > Historically this wasn't a problem because the submodule subsystem\n> > wasn't initialized because the .gitmodules file wasn't explicitly loaded\n> > by the rebase interactive command.  So when the diff machinery hit a\n> > submodule it would skip over reading any configured values of\n> > 'submodule.<name>.ignore'.\n> >\n> > In order to preserve the behavior of submodules being ignored by rebase\n> > interactive, also set the 'OVERRIDE_SUBMODULE_CONFIG' diff flag when\n> > submodules are requested to be ignored when checking for unstaged\n> > changes.\n> >\n> > Reported-by: Orgad Shaneh <orgads@gmail.com>\n> > Signed-off-by: Brandon Williams <bmwill@google.com>\n> > ---\n> >  t/t3426-rebase-submodule.sh | 16 ++++++++++++++++\n> >  wt-status.c                 |  4 +++-\n> >  2 files changed, 19 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh\n> > index ebf4f5e4b..760c872e2 100755\n> > --- a/t/t3426-rebase-submodule.sh\n> > +++ b/t/t3426-rebase-submodule.sh\n> > @@ -40,4 +40,20 @@ git_rebase_interactive () {\n> >\n> >  test_submodule_switch \"git_rebase_interactive\"\n> >\n> > +test_expect_success 'rebase interactive ignores modified submodules' '\n> > +       test_when_finished \"rm -rf super sub\" &&\n> > +       git init sub &&\n> > +       git -C sub commit --allow-empty -m \"Initial commit\" &&\n> > +       git init super &&\n> > +       git -C super submodule add ../sub &&\n> > +       git -C super config submodule.sub.ignore dirty &&\n> > +       > super/foo &&\n> > +       git -C super add foo &&\n> > +       git -C super commit -m \"Initial commit\" &&\n> > +       test_commit -C super a &&\n> > +       test_commit -C super b &&\n> > +       test_commit -C super/sub c &&\n    +       set_fake_editor &&\n> > +       git -C super rebase -i HEAD^^\n> \n> The previous test `set_fake_editor` already, though I am unsure\n> about the current directory. It turns out that the setting of the fake\n> editor is done via environment variable using absolute path to\n> the generated `fake_editor.sh`, hence it works even when\n> invoking a rebase in another directory/repo. Spooky.\n> \n> Also we do want to be able to skip previous tests,\n> which this might be a problem for. Can we have a 'setup'\n> that sets up the fake editor instead of repeating it here or\n> hoping the previous tests has run? (Calling for a refactoring\n> during a regression fix is bad taste, so maybe just take this\n> patch as is and put it to the #leftoverbits ?)\n\nThanks for catching that.  'set_fake_editor' definitely needs to be set\n(like what i did above in-line).\n\n> \n> > +'\n> > +\n> >  test_done\n> > diff --git a/wt-status.c b/wt-status.c\n> > index 29bc64cc0..94e5ebaf8 100644\n> > --- a/wt-status.c\n> > +++ b/wt-status.c\n> > @@ -2262,8 +2262,10 @@ int has_unstaged_changes(int ignore_submodules)\n> >         int result;\n> >\n> >         init_revisions(&rev_info, NULL);\n> > -       if (ignore_submodules)\n> > +       if (ignore_submodules) {\n> >                 DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n> > +               DIFF_OPT_SET(&rev_info.diffopt, OVERRIDE_SUBMODULE_CONFIG);\n> > +       }\n> \n> There are a couple of submodule related flags:\n> \n> #define DIFF_OPT_IGNORE_SUBMODULES   (1 << 18)\n> ...\n> #define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)\n> #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)\n> #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)\n> #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)\n> \n> We don't need to pay attention to 24,25,26 here, because\n> DIRTY_SUBMODULES and IGN_* are only used to specify further\n> interest, the generic INORE_SUBMODULES turns off any submodule\n> handling. (so if we were to turn them on as well, it would still be correct,\n> though it may have performance impact -- I shortly wondered if we'd rather\n> want to have a mask covering all submodule related flags, specifically\n> all flags invented in the future ;) )\n> \n> The patch looks good to me (i.e. I am convinced by review it fixes the\n> regression), so maybe we can put the test refactor on top of it, which\n> then doesn't need fast-tracking down to the maintenance track?\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"}]}