[PATCH v2] diff-lib: honor override_submodule_config flag bit

Josip Sokcevic· Jun 14, 2023, 15:31 UTC · lore

When `diff.ignoreSubmodules = all` is set and submodule commits are manually staged (e.g. via `git-update-index`), `git-commit` should record the commit with updated submodules.

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

Signed-off-by: Josip Sokcevic <sokcevic@google.com>
---
 diff-lib.c                  |  9 ++++++++-
 t/t7406-submodule-update.sh | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

Show changes to 2 files +31 −1

diff-lib.c, t/t7406-submodule-update.sh

diff --git a/diff-lib.c b/diff-lib.c
index 60e979dc1b..1918517ebd 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -669,8 +669,15 @@ 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..d5167d9ee6 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1179,4 +1179,27 @@ 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_when_finished "rm -f output" &&
+	grep "$HASH" 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' '
+	test_config diff.ignoreSubmodules dirty &&
+	add_submodule_commits_and_validate
+'
+
+test_expect_success 'commit with staged submodule change with ignoreSubmodules all' '
+	test_config diff.ignoreSubmodules all &&
+	add_submodule_commits_and_validate
+'
+
 test_done

-- 
2.41.0.162.gfafddb0af9-goog

Junio C Hamano· Jun 14, 2023, 16:14 UTC · re: Josip Sokcevic · lore

Re: [PATCH v2] diff-lib: honor override_submodule_config flag bit

Josip Sokcevic <sokcevic@google.com> writes:

Show 7 quoted lines

> +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_when_finished "rm -f output" &&
> +	grep "$HASH" output

If ls-tree exits with non-zero status, test_when_finished will not be seen, and will not arrange output to be removed. If we are going to use test_when_finished to remove the file, we should do so before we do anything that potentially creates the file.

The test with "grep" is overly loose, as I suspect that you won't be happy to see the $HASH (i.e. the commit object name) just anywhere in the output, but exactly where you placed it, i.e. at path "sub".

So it may make more sense to do the test like so:

	...
	git commit -m "create" &&

	echo "160000 commit $HASH	sub" >expect &&
	git ls-tree HEAD -- sub >actual &&
	test_cmp expect actual

If we were to add

	test_when_finished "rm -f expect actual" &&

we would do so immediately after "git commit" step, but I personally do not think it is worth doing in this case. These two files are what many if not most of our test pieces use and having them as untracked files is a norm. Unless we have a need to have strict control of what untracked files are in the working tree in later tests, it is OK to leave these files around.

Other than that, looking much better.

Thanks.

Show 17 quoted lines

> +}
> +
> +test_expect_success 'commit with staged submodule change' '
> +	add_submodule_commits_and_validate
> +'
> +
> +test_expect_success 'commit with staged submodule change with ignoreSubmodules dirty' '
> +	test_config diff.ignoreSubmodules dirty &&
> +	add_submodule_commits_and_validate
> +'
> +
> +test_expect_success 'commit with staged submodule change with ignoreSubmodules all' '
> +	test_config diff.ignoreSubmodules all &&
> +	add_submodule_commits_and_validate
> +'
> +
>  test_done

v2diff-lib: honor override_submodule_config flag bit