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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 14, 2023, 04:56 UTC
Message-ID: <CAPig+cQiwkwzVNHpBnXEAKo91LCUs-qgxbiZVbmGK1CbPVEQXg@mail.gmail.com>
In-Reply-To: <20230614041626.2979067-2-sokcevic@google.com>

On Wed, Jun 14, 2023 at 12:39 AM Josip Sokcevic <sokcevic@google.com> wrote:

Show 14 quoted lines

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

Show 7 quoted lines

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

Show 9 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_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.

Show 7 quoted lines

> +
> +
> +test_expect_success 'commit with staged submodule change' '
> +       add_submodule_commits_and_validate
> +'
> +
> +

Separate the tests by a single blank line rather than two.

Show 5 quoted lines

> +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
    '

Show 5 quoted lines

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