{"thread":{"id":"59863","subject":"[PATCH v2] diff-lib: honor override_submodule_config flag bit","startedAt":"2023-06-14T15:32:48Z","lastAt":"2023-06-14T16:14:09Z","messageCount":2,"participants":["Josip Sokcevic","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"478372","messageId":"20230614153142.3138028-1-sokcevic@google.com","threadId":"59863","inReplyTo":null,"subject":"[PATCH v2] diff-lib: honor override_submodule_config flag bit","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-06-14T15:31:42Z","receivedAt":"2023-06-14T15:32:48Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"When `diff.ignoreSubmodules = all` is set and submodule commits are\nmanually staged (e.g. via `git-update-index`), `git-commit` should\nrecord the commit  with updated submodules.\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\nSigned-off-by: Josip Sokcevic <sokcevic@google.com>\n---\n diff-lib.c                  |  9 ++++++++-\n t/t7406-submodule-update.sh | 23 +++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 60e979dc1b..1918517ebd 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -669,8 +669,15 @@ 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/*\n+\t\t * Now that flags are merged, honor override_submodule_config\n+\t\t * and ignore_submodules from passed flags.\n+\t\t */\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..d5167d9ee6 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1179,4 +1179,27 @@ 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_when_finished \"rm -f output\" &&\n+\tgrep \"$HASH\" output\n+}\n+\n+test_expect_success 'commit with staged submodule change' '\n+\tadd_submodule_commits_and_validate\n+'\n+\n+test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' '\n+\ttest_config diff.ignoreSubmodules dirty &&\n+\tadd_submodule_commits_and_validate\n+'\n+\n+test_expect_success 'commit with staged submodule change with ignoreSubmodules all' '\n+\ttest_config diff.ignoreSubmodules all &&\n+\tadd_submodule_commits_and_validate\n+'\n+\n test_done\n-- \n2.41.0.162.gfafddb0af9-goog\n\n"},{"id":"478378","messageId":"xmqqa5x2xbqr.fsf@gitster.g","threadId":"59863","inReplyTo":"20230614153142.3138028-1-sokcevic@google.com","subject":"Re: [PATCH v2] diff-lib: honor override_submodule_config flag bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T16:14:04Z","receivedAt":"2023-06-14T16:14:09Z","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> +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_when_finished \"rm -f output\" &&\n> +\tgrep \"$HASH\" output\n\nIf ls-tree exits with non-zero status, test_when_finished will not\nbe seen, and will not arrange output to be removed.  If we are going\nto use test_when_finished to remove the file, we should do so before\nwe do anything that potentially creates the file.\n\nThe test with \"grep\" is overly loose, as I suspect that you won't be\nhappy to see the $HASH (i.e. the commit object name) just anywhere\nin the output, but exactly where you placed it, i.e. at path \"sub\".\n\nSo it may make more sense to do the test like so:\n\n\t...\n\tgit commit -m \"create\" &&\n\n\techo \"160000 commit $HASH\tsub\" >expect &&\n\tgit ls-tree HEAD -- sub >actual &&\n\ttest_cmp expect actual\n\nIf we were to add\n\n\ttest_when_finished \"rm -f expect actual\" &&\n\nwe would do so immediately after \"git commit\" step, but I personally\ndo not think it is worth doing in this case.  These two files are\nwhat many if not most of our test pieces use and having them as\nuntracked files is a norm.  Unless we have a need to have strict\ncontrol of what untracked files are in the working tree in later\ntests, it is OK to leave these files around.\n\nOther than that, looking much better.\n\nThanks.\n\n> +}\n> +\n> +test_expect_success 'commit with staged submodule change' '\n> +\tadd_submodule_commits_and_validate\n> +'\n> +\n> +test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' '\n> +\ttest_config diff.ignoreSubmodules dirty &&\n> +\tadd_submodule_commits_and_validate\n> +'\n> +\n> +test_expect_success 'commit with staged submodule change with ignoreSubmodules all' '\n> +\ttest_config diff.ignoreSubmodules all &&\n> +\tadd_submodule_commits_and_validate\n> +'\n> +\n>  test_done\n"}]}