[PATCH] wt-status: actually ignore submodules when requested

[PATCH] wt-status: actually ignore submodules when requested

[Brandon Williams]

Since ff6f1f564 (submodule-config: lazy-load a repository's .gitmodules
file, 2017-08-03) rebase interactive fails if there are any submodules
with unstaged changes which have been configured with a value for
'submodule.<name>.ignore' in the repository's config.

This is due to how configured values of 'submodule.<name>.ignore' are
handled in addition to a change in how the submodule config is loaded.
When the diff machinery hits a submodule (gitlink as well as a
corresponding entry in the submodule subsystem) it will read the value
of 'submodule.<name>.ignore' stored in the repository's config and if
the config is present it will clear the 'IGNORE_SUBMODULES' (which is
the flag explicitly requested by rebase interactive),
'IGNORE_UNTRACKED_IN_SUBMODULES', and 'IGNORE_DIRTY_SUBMODULES' diff
flags and then set one of them based on the configured value.

Historically this wasn't a problem because the submodule subsystem
wasn't initialized because the .gitmodules file wasn't explicitly loaded
by the rebase interactive command.  So when the diff machinery hit a
submodule it would skip over reading any configured values of
'submodule.<name>.ignore'.

In order to preserve the behavior of submodules being ignored by rebase
interactive, also set the 'OVERRIDE_SUBMODULE_CONFIG' diff flag when
submodules are requested to be ignored when checking for unstaged
changes.

Reported-by: Orgad Shaneh <orgads@gmail.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 t/t3426-rebase-submodule.sh | 16 ++++++++++++++++
 wt-status.c                 |  4 +++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index ebf4f5e4b..760c872e2 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -40,4 +40,20 @@ git_rebase_interactive () {
 
 test_submodule_switch "git_rebase_interactive"
 
+test_expect_success 'rebase interactive ignores modified submodules' '
+	test_when_finished "rm -rf super sub" &&
+	git init sub &&
+	git -C sub commit --allow-empty -m "Initial commit" &&
+	git init super &&
+	git -C super submodule add ../sub &&
+	git -C super config submodule.sub.ignore dirty &&
+	> super/foo &&
+	git -C super add foo &&
+	git -C super commit -m "Initial commit" &&
+	test_commit -C super a &&
+	test_commit -C super b &&
+	test_commit -C super/sub c &&
+	git -C super rebase -i HEAD^^
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 29bc64cc0..94e5ebaf8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2262,8 +2262,10 @@ int has_unstaged_changes(int ignore_submodules)
 	int result;
 
 	init_revisions(&rev_info, NULL);
-	if (ignore_submodules)
+	if (ignore_submodules) {
 		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+		DIFF_OPT_SET(&rev_info.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+	}
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_files(&rev_info, 0);
-- 
2.15.0.448.gf294e3d99a-goog

Re: [PATCH] wt-status: actually ignore submodules when requested

[Stefan Beller]

On Mon, Nov 6, 2017 at 2:08 PM, Brandon Williams <bmwill@google.com> wrote:
> Since ff6f1f564 (submodule-config: lazy-load a repository's .gitmodules
> file, 2017-08-03) rebase interactive fails if there are any submodules
> with unstaged changes which have been configured with a value for
> 'submodule.<name>.ignore' in the repository's config.
>
> This is due to how configured values of 'submodule.<name>.ignore' are
> handled in addition to a change in how the submodule config is loaded.
> When the diff machinery hits a submodule (gitlink as well as a
> corresponding entry in the submodule subsystem) it will read the value
> of 'submodule.<name>.ignore' stored in the repository's config and if
> the config is present it will clear the 'IGNORE_SUBMODULES' (which is
> the flag explicitly requested by rebase interactive),
> 'IGNORE_UNTRACKED_IN_SUBMODULES', and 'IGNORE_DIRTY_SUBMODULES' diff
> flags and then set one of them based on the configured value.
>
> Historically this wasn't a problem because the submodule subsystem
> wasn't initialized because the .gitmodules file wasn't explicitly loaded
> by the rebase interactive command.  So when the diff machinery hit a
> submodule it would skip over reading any configured values of
> 'submodule.<name>.ignore'.
>
> In order to preserve the behavior of submodules being ignored by rebase
> interactive, also set the 'OVERRIDE_SUBMODULE_CONFIG' diff flag when
> submodules are requested to be ignored when checking for unstaged
> changes.
>
> Reported-by: Orgad Shaneh <orgads@gmail.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  t/t3426-rebase-submodule.sh | 16 ++++++++++++++++
>  wt-status.c                 |  4 +++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
> index ebf4f5e4b..760c872e2 100755
> --- a/t/t3426-rebase-submodule.sh
> +++ b/t/t3426-rebase-submodule.sh
> @@ -40,4 +40,20 @@ git_rebase_interactive () {
>
>  test_submodule_switch "git_rebase_interactive"
>
> +test_expect_success 'rebase interactive ignores modified submodules' '
> +       test_when_finished "rm -rf super sub" &&
> +       git init sub &&
> +       git -C sub commit --allow-empty -m "Initial commit" &&
> +       git init super &&
> +       git -C super submodule add ../sub &&
> +       git -C super config submodule.sub.ignore dirty &&
> +       > super/foo &&
> +       git -C super add foo &&
> +       git -C super commit -m "Initial commit" &&
> +       test_commit -C super a &&
> +       test_commit -C super b &&
> +       test_commit -C super/sub c &&
> +       git -C super rebase -i HEAD^^

The previous test `set_fake_editor` already, though I am unsure
about the current directory. It turns out that the setting of the fake
editor is done via environment variable using absolute path to
the generated `fake_editor.sh`, hence it works even when
invoking a rebase in another directory/repo. Spooky.

Also we do want to be able to skip previous tests,
which this might be a problem for. Can we have a 'setup'
that sets up the fake editor instead of repeating it here or
hoping the previous tests has run? (Calling for a refactoring
during a regression fix is bad taste, so maybe just take this
patch as is and put it to the #leftoverbits ?)

> +'
> +
>  test_done
> diff --git a/wt-status.c b/wt-status.c
> index 29bc64cc0..94e5ebaf8 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2262,8 +2262,10 @@ int has_unstaged_changes(int ignore_submodules)
>         int result;
>
>         init_revisions(&rev_info, NULL);
> -       if (ignore_submodules)
> +       if (ignore_submodules) {
>                 DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
> +               DIFF_OPT_SET(&rev_info.diffopt, OVERRIDE_SUBMODULE_CONFIG);
> +       }

There are a couple of submodule related flags:

#define DIFF_OPT_IGNORE_SUBMODULES   (1 << 18)
...
#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)
#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)

We don't need to pay attention to 24,25,26 here, because
DIRTY_SUBMODULES and IGN_* are only used to specify further
interest, the generic INORE_SUBMODULES turns off any submodule
handling. (so if we were to turn them on as well, it would still be correct,
though it may have performance impact -- I shortly wondered if we'd rather
want to have a mask covering all submodule related flags, specifically
all flags invented in the future ;) )

The patch looks good to me (i.e. I am convinced by review it fixes the
regression), so maybe we can put the test refactor on top of it, which
then doesn't need fast-tracking down to the maintenance track?

Thanks,
Stefan

Re: [PATCH] wt-status: actually ignore submodules when requested

[Brandon Williams]

On 11/06, Stefan Beller wrote:
> On Mon, Nov 6, 2017 at 2:08 PM, Brandon Williams <bmwill@google.com> wrote:
> > Since ff6f1f564 (submodule-config: lazy-load a repository's .gitmodules
> > file, 2017-08-03) rebase interactive fails if there are any submodules
> > with unstaged changes which have been configured with a value for
> > 'submodule.<name>.ignore' in the repository's config.
> >
> > This is due to how configured values of 'submodule.<name>.ignore' are
> > handled in addition to a change in how the submodule config is loaded.
> > When the diff machinery hits a submodule (gitlink as well as a
> > corresponding entry in the submodule subsystem) it will read the value
> > of 'submodule.<name>.ignore' stored in the repository's config and if
> > the config is present it will clear the 'IGNORE_SUBMODULES' (which is
> > the flag explicitly requested by rebase interactive),
> > 'IGNORE_UNTRACKED_IN_SUBMODULES', and 'IGNORE_DIRTY_SUBMODULES' diff
> > flags and then set one of them based on the configured value.
> >
> > Historically this wasn't a problem because the submodule subsystem
> > wasn't initialized because the .gitmodules file wasn't explicitly loaded
> > by the rebase interactive command.  So when the diff machinery hit a
> > submodule it would skip over reading any configured values of
> > 'submodule.<name>.ignore'.
> >
> > In order to preserve the behavior of submodules being ignored by rebase
> > interactive, also set the 'OVERRIDE_SUBMODULE_CONFIG' diff flag when
> > submodules are requested to be ignored when checking for unstaged
> > changes.
> >
> > Reported-by: Orgad Shaneh <orgads@gmail.com>
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  t/t3426-rebase-submodule.sh | 16 ++++++++++++++++
> >  wt-status.c                 |  4 +++-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
> > index ebf4f5e4b..760c872e2 100755
> > --- a/t/t3426-rebase-submodule.sh
> > +++ b/t/t3426-rebase-submodule.sh
> > @@ -40,4 +40,20 @@ git_rebase_interactive () {
> >
> >  test_submodule_switch "git_rebase_interactive"
> >
> > +test_expect_success 'rebase interactive ignores modified submodules' '
> > +       test_when_finished "rm -rf super sub" &&
> > +       git init sub &&
> > +       git -C sub commit --allow-empty -m "Initial commit" &&
> > +       git init super &&
> > +       git -C super submodule add ../sub &&
> > +       git -C super config submodule.sub.ignore dirty &&
> > +       > super/foo &&
> > +       git -C super add foo &&
> > +       git -C super commit -m "Initial commit" &&
> > +       test_commit -C super a &&
> > +       test_commit -C super b &&
> > +       test_commit -C super/sub c &&
    +       set_fake_editor &&
> > +       git -C super rebase -i HEAD^^
> 
> The previous test `set_fake_editor` already, though I am unsure
> about the current directory. It turns out that the setting of the fake
> editor is done via environment variable using absolute path to
> the generated `fake_editor.sh`, hence it works even when
> invoking a rebase in another directory/repo. Spooky.
> 
> Also we do want to be able to skip previous tests,
> which this might be a problem for. Can we have a 'setup'
> that sets up the fake editor instead of repeating it here or
> hoping the previous tests has run? (Calling for a refactoring
> during a regression fix is bad taste, so maybe just take this
> patch as is and put it to the #leftoverbits ?)

Thanks for catching that.  'set_fake_editor' definitely needs to be set
(like what i did above in-line).

> 
> > +'
> > +
> >  test_done
> > diff --git a/wt-status.c b/wt-status.c
> > index 29bc64cc0..94e5ebaf8 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -2262,8 +2262,10 @@ int has_unstaged_changes(int ignore_submodules)
> >         int result;
> >
> >         init_revisions(&rev_info, NULL);
> > -       if (ignore_submodules)
> > +       if (ignore_submodules) {
> >                 DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
> > +               DIFF_OPT_SET(&rev_info.diffopt, OVERRIDE_SUBMODULE_CONFIG);
> > +       }
> 
> There are a couple of submodule related flags:
> 
> #define DIFF_OPT_IGNORE_SUBMODULES   (1 << 18)
> ...
> #define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)
> #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
> #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
> #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
> 
> We don't need to pay attention to 24,25,26 here, because
> DIRTY_SUBMODULES and IGN_* are only used to specify further
> interest, the generic INORE_SUBMODULES turns off any submodule
> handling. (so if we were to turn them on as well, it would still be correct,
> though it may have performance impact -- I shortly wondered if we'd rather
> want to have a mask covering all submodule related flags, specifically
> all flags invented in the future ;) )
> 
> The patch looks good to me (i.e. I am convinced by review it fixes the
> regression), so maybe we can put the test refactor on top of it, which
> then doesn't need fast-tracking down to the maintenance track?
> 
> Thanks,
> Stefan

-- 
Brandon Williams