On Wed, Jun 14, 2023 at 12:39 AM Josip Sokcevic <sokcevic@xxxxxxxxxx> 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.
