{"thread":{"id":"59859","subject":"[PATCH] diff-lib: honor override_submodule_config flag bit","startedAt":"2023-06-14T04:17:49Z","lastAt":"2023-06-14T18:46:17Z","messageCount":4,"participants":["Josip Sokcevic","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478358","messageId":"20230614041626.2979067-2-sokcevic@google.com","threadId":"59859","inReplyTo":null,"subject":"[PATCH] diff-lib: honor override_submodule_config flag bit","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-06-14T04:16:27Z","receivedAt":"2023-06-14T04:17:49Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"When `diff.ignoreSubmodules = all` is set and a submodule commit is\nmanually staged, `git-commit` should accept it.\n\n`index_differs_from` is called from `prepare_to_commit` with flags set to\n`override_submodule_config = 1`. `index_differs_from` then merges the\ndefault diff flags and passed flags.\n\nWhen `diff.ignoreSubmodules` is set to \"all\", `flags` ends up having\nboth `override_submodule_config` and `ignore_submodules` set to 1. This\nresults in `git-commit` ignoring staged commits.\n\nThis patch restores original `flags.ignore_submodule` if\n`flags.override_submodule_config` is set.\n---\n diff-lib.c                  |  7 ++++++-\n t/t7406-submodule-update.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 60e979dc1b..75859bd159 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -669,8 +669,13 @@ int index_differs_from(struct repository *r,\n \tsetup_revisions(0, NULL, &rev, &opt);\n \trev.diffopt.flags.quick = 1;\n \trev.diffopt.flags.exit_with_status = 1;\n-\tif (flags)\n+\tif (flags) {\n \t\tdiff_flags_or(&rev.diffopt.flags, flags);\n+\t\t// Now that flags are merged, honor override_submodule_config\n+\t\t// and ignore_submodules from passed flags.\n+\t\tif ((*flags).override_submodule_config)\n+\t\t\trev.diffopt.flags.ignore_submodules = (*flags).ignore_submodules;\n+\t}\n \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n \trun_diff_index(&rev, 1);\n \thas_changes = rev.diffopt.flags.has_changes;\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f094e3d7f3..0e3fa642dd 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1179,4 +1179,32 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=\n \ttest_cmp expect.err actual.err\n '\n \n+add_submodule_commits_and_validate () {\n+\tHASH=$(git rev-parse HEAD) &&\n+\tgit update-index --add --cacheinfo 160000,$HASH,sub &&\n+\tgit commit -m \"create submodule\" &&\n+\tgit ls-tree HEAD >output &&\n+\ttest_i18ngrep \"$HASH\" output &&\n+\n+\trm output\n+}\n+\n+\n+test_expect_success 'commit with staged submodule change' '\n+\tadd_submodule_commits_and_validate\n+'\n+\n+\n+test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' '\n+\tgit config diff.ignoreSubmodules dirty &&\n+\tadd_submodule_commits_and_validate &&\n+\tgit config --unset diff.ignoreSubmodules\n+'\n+\n+test_expect_success 'commit with staged submodule change with ignoreSubmodules all' '\n+\tgit config diff.ignoreSubmodules all &&\n+\tadd_submodule_commits_and_validate &&\n+\tgit config --unset diff.ignoreSubmodules\n+'\n+\n test_done\n-- \n2.41.0.162.gfafddb0af9-goog\n\n"},{"id":"478359","messageId":"CAPig+cQiwkwzVNHpBnXEAKo91LCUs-qgxbiZVbmGK1CbPVEQXg@mail.gmail.com","threadId":"59859","inReplyTo":"20230614041626.2979067-2-sokcevic@google.com","subject":"Re: [PATCH] diff-lib: honor override_submodule_config flag bit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-06-14T04:56:46Z","receivedAt":"2023-06-14T04:57:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 14, 2023 at 12:39 AM Josip Sokcevic <sokcevic@google.com> wrote:\n> When `diff.ignoreSubmodules = all` is set and a submodule commit is\n> manually staged, `git-commit` should accept it.\n>\n> `index_differs_from` is called from `prepare_to_commit` with flags set to\n> `override_submodule_config = 1`. `index_differs_from` then merges the\n> default diff flags and passed flags.\n>\n> When `diff.ignoreSubmodules` is set to \"all\", `flags` ends up having\n> both `override_submodule_config` and `ignore_submodules` set to 1. This\n> results in `git-commit` ignoring staged commits.\n>\n> This patch restores original `flags.ignore_submodule` if\n> `flags.override_submodule_config` is set.\n> ---\n\nThanks for the patch. I'm not a submodule user, so I can't comment on\nthe functional changes made by the patch, but instead will provide a\nfew superficial comments to help move your patch along.\n\nPlease add a Signed-off-by: before the \"---\" line above. See\nDocumentation/SubmittingPatches.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> @@ -669,8 +669,13 @@ int index_differs_from(struct repository *r,\n> -       if (flags)\n> +       if (flags) {\n>                 diff_flags_or(&rev.diffopt.flags, flags);\n> +               // Now that flags are merged, honor override_submodule_config\n> +               // and ignore_submodules from passed flags.\n\nThis project still uses old-style /*...*/ comments; //-style are\navoided. A multi-line comment is formatted like this:\n\n    /*\n     * This is a multi-line\n     * comment.\n     */\n\n> +               if ((*flags).override_submodule_config)\n> +                       rev.diffopt.flags.ignore_submodules = (*flags).ignore_submodules;\n\nIt would be more idiomatic to use the `->` operator to access members\nof `flags`:\n\n    if (flags->override_submodule_config)\n        rev.diffopt.flags.ignore_submodules = flags->ignore_submodules;\n\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> @@ -1179,4 +1179,32 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=\n\nNice to see new tests accompanying the code change.\n\n> +add_submodule_commits_and_validate () {\n> +       HASH=$(git rev-parse HEAD) &&\n> +       git update-index --add --cacheinfo 160000,$HASH,sub &&\n> +       git commit -m \"create submodule\" &&\n> +       git ls-tree HEAD >output &&\n> +       test_i18ngrep \"$HASH\" output &&\n> +\n> +       rm output\n> +}\n\n\"output\" won't get cleaned up if a command earlier in the &&-chain\nfails. To ensure that it does get cleaned up regardless of whether the\ntest succeeds or fails, use test_when_finished() before the file is\ncreated. For instance:\n\n    add_submodule_commits_and_validate () {\n        HASH=$(git rev-parse HEAD) &&\n        ...\n        test_when_finished \"rm -f output\" &&\n        git ls-tree HEAD >output &&\n        ...\n    }\n\nThese days, test_i18ngrep() is deprecated. Just use plain old `grep` instead.\n\n> +\n> +\n> +test_expect_success 'commit with staged submodule change' '\n> +       add_submodule_commits_and_validate\n> +'\n> +\n> +\n\nSeparate the tests by a single blank line rather than two.\n\n> +test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' '\n> +       git config diff.ignoreSubmodules dirty &&\n> +       add_submodule_commits_and_validate &&\n> +       git config --unset diff.ignoreSubmodules\n> +'\n\nThe same observation as above regarding cleaning up: The `git config\n--unset` won't be invoked if a command earlier in the &&-chain fails.\nInstead, use test_config() which will ensure cleanup regardless of\nwhether the test succeeds or fails:\n\n    test_expect_success 'commit ...' '\n        test_config diff.ignoreSubmodules dirty &&\n        add_submodule_commits_and_validate\n    '\n\n> +test_expect_success 'commit with staged submodule change with ignoreSubmodules all' '\n> +       git config diff.ignoreSubmodules all &&\n> +       add_submodule_commits_and_validate &&\n> +       git config --unset diff.ignoreSubmodules\n> +'\n\nLikewise.\n"},{"id":"478360","messageId":"xmqqedmezfn9.fsf@gitster.g","threadId":"59859","inReplyTo":"20230614041626.2979067-2-sokcevic@google.com","subject":"Re: [PATCH] diff-lib: honor override_submodule_config flag bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T07:06:50Z","receivedAt":"2023-06-14T07:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josip Sokcevic <sokcevic@google.com> writes:\n\n> When `diff.ignoreSubmodules = all` is set and a submodule commit is\n> manually staged, `git-commit` should accept it.\n\nWhat does \"it\" refer to in this sentence?  Does \"accept\"ing mean\n\"Even if the configuration tells us to ignore all submodules, the\ncommand should record the commit with updated submodule\"?  Or does\nit mean \"as the configuration tells us to ignore all submodules, the\ncommand should honor that wish and the command should record the\ncommit with the same commit at the submodule path as the parent\ncommit, ignoring the manual addition\"?  The sentence needs to be\nrewritten to clarify so that readers won't have to ask the above\nquestion.\n\nEverything else I wrote in my draft review I notice was adequately\ncovered by Eric's review, so I've removed them from this message.\n\nThanks.\n"},{"id":"478383","messageId":"CAJiyOii3RDur7fwMfG5LKHdApADqKxtQoJ-nMiKWEtY8L3UxnQ@mail.gmail.com","threadId":"59859","inReplyTo":"xmqqedmezfn9.fsf@gitster.g","subject":"Re: [PATCH] diff-lib: honor override_submodule_config flag bit","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-06-14T18:45:59Z","receivedAt":"2023-06-14T18:46:17Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"Thanks for everyone's input, in this thread and two others! I realized\neach patch version has its own thread so apologies for that - it's my\nfirst time contributing to git and I didn't realize I'm using the\nsend-email wrong.\n\nBest,\n\n-- \nJosip Sokcevic\n"}]}