[PATCH] diff-lib: honor override_submodule_config flag bit

[PATCH] diff-lib: honor override_submodule_config flag bit

[Josip Sokcevic]

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.
---
 diff-lib.c                  |  7 ++++++-
 t/t7406-submodule-update.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

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
-- 
2.41.0.162.gfafddb0af9-goog

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

[Eric Sunshine]

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.

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

[Junio C Hamano]

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.

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

[Josip Sokcevic]

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,

-- 
Josip Sokcevic