* [PATCH] wt-status: actually ignore submodules when requested [not found] <CAGHpTB+jKiXr45tKVEVTtszN7OBTW7W_FqKu7aAjsB8Tmx9N3Q@mail.gmail.com> @ 2017-11-06 22:08 ` Brandon Williams 2017-11-06 22:44 ` Stefan Beller 0 siblings, 1 reply; 3+ messages in thread From: Brandon Williams @ 2017-11-06 22:08 UTC (permalink / raw) To: git; +Cc: orgads, Johannes.Schindelin, sbeller, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] wt-status: actually ignore submodules when requested 2017-11-06 22:08 ` [PATCH] wt-status: actually ignore submodules when requested Brandon Williams @ 2017-11-06 22:44 ` Stefan Beller 2017-11-06 22:52 ` Brandon Williams 0 siblings, 1 reply; 3+ messages in thread From: Stefan Beller @ 2017-11-06 22:44 UTC (permalink / raw) To: Brandon Williams; +Cc: git, Orgad Shaneh, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] wt-status: actually ignore submodules when requested 2017-11-06 22:44 ` Stefan Beller @ 2017-11-06 22:52 ` Brandon Williams 0 siblings, 0 replies; 3+ messages in thread From: Brandon Williams @ 2017-11-06 22:52 UTC (permalink / raw) To: Stefan Beller; +Cc: git, Orgad Shaneh, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-06 22:52 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAGHpTB+jKiXr45tKVEVTtszN7OBTW7W_FqKu7aAjsB8Tmx9N3Q@mail.gmail.com> 2017-11-06 22:08 ` [PATCH] wt-status: actually ignore submodules when requested Brandon Williams 2017-11-06 22:44 ` Stefan Beller 2017-11-06 22:52 ` Brandon Williams
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).