wt-status: actually ignore submodules when requested

PATCH In review Brandon Williams 2 participants 3 messages Nov 6, 2017 1 message not in the archive

Share & tools

A message that is not in the archive (sent off-list or not yet received): CAGHpTB+jKiXr45tKVEVTtszN7OBTW7W_FqKu7aAjsB8Tmx9N3Q@mail.gmail.com

Brandon Williams

author

2017-11-06 22:08 UTC #1 lore raw

PATCH 1/1 wt-status: actually ignore submodules when requested

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 <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---
 t/t3426-rebase-submodule.sh | 16 ++++++++++++++++
 wt-status.c                 |  4 +++-
 2 files changed, 19 insertions(+), 1 deletion(-)

t/t3426-rebase-submodule.sh +16 -0

index ebf4f5e4b..760c872e2 100755
@@ -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

wt-status.c +3 -1

index 29bc64cc0..94e5ebaf8 100644
@@ -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);

Signature · 1 line

2.15.0.448.gf294e3d99a-goog

Stefan Beller

2017-11-06 22:45 UTC #2 lore raw

Quoted 45 lines · 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 <[email protected]>
Signed-off-by Brandon Williams <[email protected]>
---
 t/t3426-rebase-submodule.sh | 16 ++++++++++++++++
 wt-status.c                 |  4 +++-
 2 files changed, 19 insertions(+), 1 deletion(-)
t/t3426-rebase-submodule.sh +4 -0
index ebf4f5e4b..760c872e2 100755
@@ -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" &&

t/t3426-rebase-submodule.sh git_rebase_interactive () { · line 56

Quoted 10 lines

t/t3426-rebase-submodule.sh +10 -0

+       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 ?)

Quoted 8 lines

t/t3426-rebase-submodule.sh +2 -0
+'
+
 test_done
wt-status.c +0 -0
index 29bc64cc0..94e5ebaf8 100644

wt-status.c int has_unstaged_changes(int ignore_submodules) · line 2268

Quoted 8 lines

wt-status.c +3 -1

@@ -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

author

2017-11-06 22:52 UTC #3 lore raw

Quoted 42 lines · Stefan Beller

Quoted 41 lines · 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 <[email protected]>
Signed-off-by Brandon Williams <[email protected]>
---
 t/t3426-rebase-submodule.sh | 16 ++++++++++++++++
 wt-status.c                 |  4 +++-
 2 files changed, 19 insertions(+), 1 deletion(-)
t/t3426-rebase-submodule.sh +0 -0
index ebf4f5e4b..760c872e2 100755
@@ -40,4 +40,20 @@ git_rebase_interactive () {
 
 test_submodule_switch "git_rebase_interactive"
 

Quoted 13 lines

t/t3426-rebase-submodule.sh git_rebase_interactive () { · line 55

Quoted 13 lines

t/t3426-rebase-submodule.sh +13 -0

+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 &&

Quoted 8 lines

t/t3426-rebase-submodule.sh git_rebase_interactive () { · line 56
Quoted 1 line
t/t3426-rebase-submodule.sh +1 -0
+       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.

Quoted 6 lines

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

Quoted 41 lines

Quoted 9 lines
t/t3426-rebase-submodule.sh +2 -0
+'
+
 test_done
wt-status.c +0 -0
index 29bc64cc0..94e5ebaf8 100644
wt-status.c int has_unstaged_changes(int ignore_submodules) · line 2268
Quoted 8 lines
wt-status.c +3 -1
@@ -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

Signature · 1 line

Brandon Williams