| Message ID | 20230614041626.2979067-2-sokcevic@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff-lib: honor override_submodule_config flag bit | expand |
On Wed, Jun 14, 2023 at 12:39 AM Josip Sokcevic <sokcevic@google.com> wrote: > When `diff.ignoreSubmodules = all` is set and a submodule commit is > manually staged, `git-commit` should accept it. > > `index_differs_from` is called from `prepare_to_commit` with flags set to > `override_submodule_config = 1`. `index_differs_from` then merges the > default diff flags and passed flags. > > When `diff.ignoreSubmodules` is set to "all", `flags` ends up having > both `override_submodule_config` and `ignore_submodules` set to 1. This > results in `git-commit` ignoring staged commits. > > This patch restores original `flags.ignore_submodule` if > `flags.override_submodule_config` is set. > --- Thanks for the patch. I'm not a submodule user, so I can't comment on the functional changes made by the patch, but instead will provide a few superficial comments to help move your patch along. Please add a Signed-off-by: before the "---" line above. See Documentation/SubmittingPatches. > diff --git a/diff-lib.c b/diff-lib.c > @@ -669,8 +669,13 @@ int index_differs_from(struct repository *r, > - if (flags) > + if (flags) { > diff_flags_or(&rev.diffopt.flags, flags); > + // Now that flags are merged, honor override_submodule_config > + // and ignore_submodules from passed flags. This project still uses old-style /*...*/ comments; //-style are avoided. A multi-line comment is formatted like this: /* * This is a multi-line * comment. */ > + if ((*flags).override_submodule_config) > + rev.diffopt.flags.ignore_submodules = (*flags).ignore_submodules; It would be more idiomatic to use the `->` operator to access members of `flags`: if (flags->override_submodule_config) rev.diffopt.flags.ignore_submodules = flags->ignore_submodules; > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > @@ -1179,4 +1179,32 @@ test_expect_success 'submodule update --recursive skip submodules with strategy= Nice to see new tests accompanying the code change. > +add_submodule_commits_and_validate () { > + HASH=$(git rev-parse HEAD) && > + git update-index --add --cacheinfo 160000,$HASH,sub && > + git commit -m "create submodule" && > + git ls-tree HEAD >output && > + test_i18ngrep "$HASH" output && > + > + rm output > +} "output" won't get cleaned up if a command earlier in the &&-chain fails. To ensure that it does get cleaned up regardless of whether the test succeeds or fails, use test_when_finished() before the file is created. For instance: add_submodule_commits_and_validate () { HASH=$(git rev-parse HEAD) && ... test_when_finished "rm -f output" && git ls-tree HEAD >output && ... } These days, test_i18ngrep() is deprecated. Just use plain old `grep` instead. > + > + > +test_expect_success 'commit with staged submodule change' ' > + add_submodule_commits_and_validate > +' > + > + Separate the tests by a single blank line rather than two. > +test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' ' > + git config diff.ignoreSubmodules dirty && > + add_submodule_commits_and_validate && > + git config --unset diff.ignoreSubmodules > +' The same observation as above regarding cleaning up: The `git config --unset` won't be invoked if a command earlier in the &&-chain fails. Instead, use test_config() which will ensure cleanup regardless of whether the test succeeds or fails: test_expect_success 'commit ...' ' test_config diff.ignoreSubmodules dirty && add_submodule_commits_and_validate ' > +test_expect_success 'commit with staged submodule change with ignoreSubmodules all' ' > + git config diff.ignoreSubmodules all && > + add_submodule_commits_and_validate && > + git config --unset diff.ignoreSubmodules > +' Likewise.
Josip Sokcevic <sokcevic@google.com> writes: > When `diff.ignoreSubmodules = all` is set and a submodule commit is > manually staged, `git-commit` should accept it. What does "it" refer to in this sentence? Does "accept"ing mean "Even if the configuration tells us to ignore all submodules, the command should record the commit with updated submodule"? Or does it mean "as the configuration tells us to ignore all submodules, the command should honor that wish and the command should record the commit with the same commit at the submodule path as the parent commit, ignoring the manual addition"? The sentence needs to be rewritten to clarify so that readers won't have to ask the above question. Everything else I wrote in my draft review I notice was adequately covered by Eric's review, so I've removed them from this message. Thanks.
Thanks for everyone's input, in this thread and two others! I realized each patch version has its own thread so apologies for that - it's my first time contributing to git and I didn't realize I'm using the send-email wrong. Best,
diff --git a/diff-lib.c b/diff-lib.c index 60e979dc1b..75859bd159 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -669,8 +669,13 @@ int index_differs_from(struct repository *r, setup_revisions(0, NULL, &rev, &opt); rev.diffopt.flags.quick = 1; rev.diffopt.flags.exit_with_status = 1; - if (flags) + if (flags) { diff_flags_or(&rev.diffopt.flags, flags); + // Now that flags are merged, honor override_submodule_config + // and ignore_submodules from passed flags. + if ((*flags).override_submodule_config) + rev.diffopt.flags.ignore_submodules = (*flags).ignore_submodules; + } rev.diffopt.ita_invisible_in_index = ita_invisible_in_index; run_diff_index(&rev, 1); has_changes = rev.diffopt.flags.has_changes; diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index f094e3d7f3..0e3fa642dd 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1179,4 +1179,32 @@ test_expect_success 'submodule update --recursive skip submodules with strategy= test_cmp expect.err actual.err ' +add_submodule_commits_and_validate () { + HASH=$(git rev-parse HEAD) && + git update-index --add --cacheinfo 160000,$HASH,sub && + git commit -m "create submodule" && + git ls-tree HEAD >output && + test_i18ngrep "$HASH" output && + + rm output +} + + +test_expect_success 'commit with staged submodule change' ' + add_submodule_commits_and_validate +' + + +test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' ' + git config diff.ignoreSubmodules dirty && + add_submodule_commits_and_validate && + git config --unset diff.ignoreSubmodules +' + +test_expect_success 'commit with staged submodule change with ignoreSubmodules all' ' + git config diff.ignoreSubmodules all && + add_submodule_commits_and_validate && + git config --unset diff.ignoreSubmodules +' + test_done