Re: [PATCH v2 0/5] git-add : Respect submodule ignore=all and only add changes with --force
- From
Claus Schneider <claus.schneider@eficode.com>
- Date
- Nov 14, 2025, 13:53 UTC
- Message-ID
- <CA+GP4bob2A+GsVUo5vy+Mw0qJHDD5g+pyo2Ka1726ouUuS_=Wg@mail.gmail.com>
- In-Reply-To
- <xmqqzf8pln62.fsf@gitster.g>
Thanks Junio - well received and noted. I have updated the PR description accordingly, but I have not changed the "--force" in the description even though I have implemented ''--include-ignored-submodules' as Philip had the input not to use "--force". He suggested using a new option. Philips comment:
Show 8 quoted lines
> I'm not convinced that the approach of using "--force" is a good idea as > it conflates ignoring changes to tracked paths (which is what > submodule.<name>.ignore" does) with ignoring untracked paths (which is > what ".gitignore" does). If we're happy to break existing uses that rely > on the current behavior then having a new option to override > submodule.<name>.ignore strikes me as a better way forward. I don't have > much experience of using submodules so I can't comment on whether > changing the behavior is a good idea or not.
I think it will be more simple to use the '--force' option though and keep the amount of options lower and less to remember. Given your comments about more usages of bytes for option also becomes obsolete if we stick to "--force". I am happy to do so.
I am investigating your other comments on the patches in the meantime.
On Thu, Nov 13, 2025 at 8:58 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 465 quoted lines
>
> "Claus Schneider via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > The feature of configuring a submodule to "ignore=all" is nicely respected
> > in commands "status" and "diff".
>
> "nicely respected" is not very informative for those who do not know
> what the setting does. Saying something like
>
> "git status" and "git diff" will not report modified submodules
> with submodule.<name>.ignore set to "all".
>
> would not waste significantly more bytes than what you wrote, and is
> more helpful.
>
> > However the "add" command does not respect
> > the configuration the same way.
>
> Again, "does not respect" and then what? Running "git add" on a
> submodule with submodule.<name>.ignore set to "all" does what?
> Complains that it has changes but because .ignore is set it won't
> add? Adds it silently? Something else?
>
> > The behavior is problematic for the logic
> > between status/diff and add.
>
> After this sentence, "because ..." is missing. Please help readers
> understand the issue you perceive as problematic more easily.
>
> I am guessing that you are assuming that an "add", after "diff" or
> "status" said there is no change, is expected to be a no-op, but I
> cannot be sure if that is what you are referring to here with the
> reason left unsaid like the above.
>
> > Secondly it makes it problematic to track
> > branches in the submodule configuration as developers unintentionally keeps
> > add submodule updates and get conflicts for no intentional reason. Both adds
> > unnecessary friction to the usage of submodules.
> >
> > The patches implement the same logical behavior for ignore=all submodules as
> > regular ignored files. The status now does not show any diff - nor will the
> > add command update the reference submodule reference. If you add the
> > submodule path which is ignore=all then you are presented with a message
> > that you need to use the --force option.
>
> I vaguely recall that an earlier discussion between you and Phillip
> were concluding against "--force"? I personally feel it is in line
> with "git add foo.o" (when '*.o' is in .gitignore) gets rejected and
> "git add -f foo.o" is a way to override it, but in the list of
> patches below, I see --include-ignored-submodules (no, our command
> line option names do not use underscore for inter-word-gaps), so I
> suspect the description in the cover letter around here is stale?
>
>
> > The branch=, ignore=all (and
> > update=none) now works great with update --remote,
>
> Again, "great" is not very informative, and as bad as "nicely
> respected". Avoid using these adjectives loaded with unnecessary
> value judgements, and instead trust your readers. They are
> intelligent to judge if the updated behaviour is great or not for
> themselves. Try to use the same bytes on helping readers understand
> what actually happens.
>
> > but developers does not
>
> "do not".
>
> > have to consider changes in the updates of the submodule sha1. The
> > implementation removes a friction of working with submodules and can be used
> > like the repo tool with branches configured. The submodule status report
> > could be used for build/release documentation for reproduction of a setup.
> >
> > A few tests used the adding of submodules without --force, hence they have
> > been updated to use the --force option.
> >
> > Claus Schneider(Eficode) (5):
> > read-cache: update add_files_to_cache take param
> > include_ignored_submodules
> > read-cache: add/read-cache respect submodule ignore=all
> > tests: add new t2206-add-submodule-ignored.sh to test ignore=all
> > scenario
> > tests: fix existing tests when add an ignore=all submodule
> > Documentation: add --include_ignored_submodules + ignore=all config
> >
> > .devcontainer/Dockerfile | 70 +++++++++++++++
> > .devcontainer/Dockerfile.standalone | 76 ++++++++++++++++
> > .devcontainer/devcontainer.json | 25 ++++++
> > Documentation/config/submodule.adoc | 13 +--
> > Documentation/git-add.adoc | 5 ++
> > Documentation/gitmodules.adoc | 5 +-
> > builtin/add.c | 4 +-
> > builtin/checkout.c | 2 +-
> > builtin/commit.c | 2 +-
> > read-cache-ll.h | 2 +-
> > read-cache.c | 54 ++++++++++-
> > t/lib-submodule-update.sh | 6 +-
> > t/meson.build | 1 +
> > t/t2206-add-submodule-ignored.sh | 134 ++++++++++++++++++++++++++++
> > t/t7508-status.sh | 2 +-
> > 15 files changed, 384 insertions(+), 17 deletions(-)
> > create mode 100644 .devcontainer/Dockerfile
> > create mode 100644 .devcontainer/Dockerfile.standalone
> > create mode 100644 .devcontainer/devcontainer.json
> > create mode 100755 t/t2206-add-submodule-ignored.sh
> >
> >
> > base-commit: 81f86aacc4eb74cdb9c2c8082d36d2070c666045
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1987%2FPraqma%2Frespect-submodule-ignore-v2
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1987/Praqma/respect-submodule-ignore-v2
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1987
> >
> > Range-diff vs v1:
> >
> > 1: d98cca698d ! 1: 5796009122 read-cache: update add_files_to_cache to take param ignored_too(--force)
> > @@ Metadata
> > Author: Claus Schneider(Eficode) <claus.schneider@eficode.com>
> >
> > ## Commit message ##
> > - read-cache: update add_files_to_cache to take param ignored_too(--force)
> > + read-cache: update add_files_to_cache take param include_ignored_submodules
> >
> > - The ignored_too parameter is added to the function add_files_to_cache for
> > - usage of explicit updating the index for the updated submodule using the
> > - explicit patchspec to the submodule.
> > + The include_ignored_submodules parameter is added to the function
> > + add_files_to_cache for usage of explicit updating the index for the updated
> > + submodule using the explicit patchspec to the submodule.
> >
> > Signed-off-by: Claus Schneider(Eficode) <claus.schneider@eficode.com>
> >
> > ## builtin/add.c ##
> > +@@ builtin/add.c: N_("The following paths are ignored by one of your .gitignore files:\n");
> > + static int verbose, show_only, ignored_too, refresh_only;
> > + static int ignore_add_errors, intent_to_add, ignore_missing;
> > + static int warn_on_embedded_repo = 1;
> > ++static int include_ignored_submodules;
> > +
> > + #define ADDREMOVE_DEFAULT 1
> > + static int addremove = ADDREMOVE_DEFAULT;
> > +@@ builtin/add.c: static struct option builtin_add_options[] = {
> > + OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
> > + OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
> > + OPT_BOOL(0, "sparse", &include_sparse, N_("allow updating entries outside of the sparse-checkout cone")),
> > ++ OPT_BOOL(0, "include-ignored-submodules", &include_ignored_submodules, N_("add submodules even if they has configuration ignore=all")),
> > + OPT_STRING(0, "chmod", &chmod_arg, "(+|-)x",
> > + N_("override the executable bit of the listed files")),
> > + OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
> > @@ builtin/add.c: int cmd_add(int argc,
> > else
> > exit_status |= add_files_to_cache(repo, prefix,
> > &pathspec, ps_matched,
> > - include_sparse, flags);
> > -+ include_sparse, flags, ignored_too);
> > ++ include_sparse, flags, include_ignored_submodules);
> >
> > if (take_worktree_changes && !add_renormalize && !ignore_add_errors &&
> > report_path_error(ps_matched, &pathspec))
> > @@ read-cache.c: void overlay_tree_on_index(struct index_state *istate,
> > int include_sparse;
> > int flags;
> > int add_errors;
> > -+ int ignored_too;
> > ++ int include_ignored_submodules;
> > };
> >
> > static int fix_unmerged_status(struct diff_filepair *p,
> > @@ read-cache.c: static void update_callback(struct diff_queue_struct *q,
> > + default:
> > + die(_("unexpected diff status %c"), p->status);
> > + case DIFF_STATUS_MODIFIED:
> > +- case DIFF_STATUS_TYPE_CHANGED:
> > ++ case DIFF_STATUS_TYPE_CHANGED: {
> > ++ struct stat st;
> > ++ if (!lstat(path, &st) && S_ISDIR(st.st_mode)) { // only consider submodule if it is a directory
> > ++ const struct submodule *sub = submodule_from_path(data->repo, null_oid(the_hash_algo), path);
> > ++ if (sub && sub->name && sub->ignore && !strcmp(sub->ignore, "all")) {
> > ++ int pathspec_matches = 0;
> > ++ char *norm_pathspec = NULL;
> > ++ int ps_i;
> > ++ trace_printf("ignore=all %s\n", path);
> > ++ trace_printf("pathspec %s\n",
> > ++ (data->pathspec && data->pathspec->nr) ? "has pathspec" : "no pathspec");
> > ++ /* Safely scan all pathspec items (q->nr may exceed pathspec->nr). */
> > ++ if (data->pathspec) {
> > ++ for (ps_i = 0; ps_i < data->pathspec->nr; ps_i++) {
> > ++ const char *m = data->pathspec->items[ps_i].match;
> > ++ if (!m)
> > ++ continue;
> > ++ norm_pathspec = xstrdup(m);
> > ++ strip_dir_trailing_slashes(norm_pathspec);
> > ++ if (!strcmp(path, norm_pathspec)) {
> > ++ pathspec_matches = 1;
> > ++ FREE_AND_NULL(norm_pathspec);
> > ++ break;
> > ++ }
> > ++ FREE_AND_NULL(norm_pathspec);
> > ++ }
> > ++ }
> > ++ if (pathspec_matches) {
> > ++ if (data->include_ignored_submodules && data->include_ignored_submodules > 0) {
> > ++ trace_printf("Add ignored=all submodule due to --include_ignored_submodules: %s\n", path);
> > ++ } else {
> > ++ printf(_("Skipping submodule due to ignore=all: %s"), path);
> > ++ printf(_("Use --include_ignored_submodules, if you really want to add them.") );
> > ++ continue;
> > ++ }
> > ++ } else {
> > ++ /* No explicit pathspec match -> skip silently (or with trace). */
> > ++ trace_printf("pathspec does not match %s\n", path);
> > ++ continue;
> > ++ }
> > ++ }
> > ++ }
> > + if (add_file_to_index(data->index, path, data->flags)) {
> > + if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
> > + die(_("updating files failed"));
> > +@@ read-cache.c: static void update_callback(struct diff_queue_struct *q,
> >
> > int add_files_to_cache(struct repository *repo, const char *prefix,
> > const struct pathspec *pathspec, char *ps_matched,
> > - int include_sparse, int flags)
> > -+ int include_sparse, int flags, int ignored_too )
> > ++ int include_sparse, int flags, int include_ignored_submodules )
> > {
> > struct update_callback_data data;
> > struct rev_info rev;
> > @@ read-cache.c: int add_files_to_cache(struct repository *repo, const char *prefix
> > data.include_sparse = include_sparse;
> > data.flags = flags;
> > + data.repo = repo;
> > -+ data.ignored_too = ignored_too;
> > ++ data.include_ignored_submodules = include_ignored_submodules;
> > + data.pathspec = (struct pathspec *)pathspec;
> >
> > repo_init_revisions(repo, &rev, prefix);
> > 2: d1b02617e6 ! 2: 9ec79b9a11 read-cache: let read-cache respect submodule ignore=all and --force
> > @@ Metadata
> > Author: Claus Schneider(Eficode) <claus.schneider@eficode.com>
> >
> > ## Commit message ##
> > - read-cache: let read-cache respect submodule ignore=all and --force
> > + read-cache: add/read-cache respect submodule ignore=all
> >
> > - Given the submdule configuration is ignore=all then only update the
> > - submdule if the --force option is given and the submodule is explicit
> > - given in the pathspec.
> > + Submodules configured with ignore=all are now skipped during add operations
> > + unless overridden by --include-ignored-submodules and the submodule path is
> > + explicitly specified.
> >
> > A message is printed (like ignored files) guiding the user to use the
> > - --force flag if the user has explicitely want to update the submodule
> > - reference.
> > + --include-ignored-submodules flag if the user has explicitely want to update
> > + the submodule reference.
> >
> > The reason for the change is support submodule branch tracking or
> > similar and git status state nothing and git add should not add either.
> > @@ Commit message
> > the submodule is already tracked.
> >
> > The change opens up a lot of possibilities for submodules to be used
> > - more freely and a like the repo tool. A submodule can be added for many
> > + more freely and simular to the repo tool. A submodule can be added for many
> > more reason and loosely coupled dependencies to the super repo which often
> > gives the friction of handle the explicit commits and updates without
> > the need for tracking the submodule sha1 by sha1.
> > @@ read-cache.c
> > /* Mask for the name length in ce_flags in the on-disk index */
> >
> > @@ read-cache.c: static void update_callback(struct diff_queue_struct *q,
> > - default:
> > - die(_("unexpected diff status %c"), p->status);
> > - case DIFF_STATUS_MODIFIED:
> > -- case DIFF_STATUS_TYPE_CHANGED:
> > -+ case DIFF_STATUS_TYPE_CHANGED: {
> > -+ struct stat st;
> > -+ if (!lstat(path, &st) && S_ISDIR(st.st_mode)) { // only consider submodule if it is a directory
> > -+ const struct submodule *sub = submodule_from_path(data->repo, null_oid(the_hash_algo), path);
> > -+ if (sub && sub->name && sub->ignore && !strcmp(sub->ignore, "all")) {
> > -+ int pathspec_matches = 0;
> > -+ char *norm_pathspec = NULL;
> > -+ int ps_i;
> > -+ trace_printf("ignore=all %s\n", path);
> > -+ trace_printf("pathspec %s\n",
> > -+ (data->pathspec && data->pathspec->nr) ? "has pathspec" : "no pathspec");
> > -+ /* Safely scan all pathspec items (q->nr may exceed pathspec->nr). */
> > -+ if (data->pathspec) {
> > -+ for (ps_i = 0; ps_i < data->pathspec->nr; ps_i++) {
> > -+ const char *m = data->pathspec->items[ps_i].match;
> > -+ if (!m)
> > -+ continue;
> > -+ norm_pathspec = xstrdup(m);
> > -+ strip_dir_trailing_slashes(norm_pathspec);
> > -+ if (!strcmp(path, norm_pathspec)) {
> > -+ pathspec_matches = 1;
> > -+ FREE_AND_NULL(norm_pathspec);
> > -+ break;
> > -+ }
> > -+ FREE_AND_NULL(norm_pathspec);
> > -+ }
> > -+ }
> > -+ if (pathspec_matches) {
> > -+ if (data->ignored_too && data->ignored_too > 0) {
> > -+ trace_printf("Forcing add of submodule ignored=all due to --force: %s\n", path);
> > -+ } else {
> > -+ printf(_("Skipping submodule due to ignore=all: %s"), path);
> > -+ printf(_("Use -f if you really want to add them.") );
> > -+ continue;
> > -+ }
> > -+ } else {
> > -+ /* No explicit pathspec match -> skip silently (or with trace). */
> > -+ trace_printf("pathspec does not match %s\n", path);
> > -+ continue;
> > -+ }
> > -+ }
> > -+ }
> > - if (add_file_to_index(data->index, path, data->flags)) {
> > - if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
> > - die(_("updating files failed"));
> > + }
> > + if (pathspec_matches) {
> > + if (data->include_ignored_submodules && data->include_ignored_submodules > 0) {
> > +- trace_printf("Add ignored=all submodule due to --include_ignored_submodules: %s\n", path);
> > ++ trace_printf("Add submodule due to --include_ignored_submodules: %s\n", path);
> > + } else {
> > + printf(_("Skipping submodule due to ignore=all: %s"), path);
> > + printf(_("Use --include_ignored_submodules, if you really want to add them.") );
> > +@@ read-cache.c: static void update_callback(struct diff_queue_struct *q,
> > + }
> > + } else {
> > + /* No explicit pathspec match -> skip silently (or with trace). */
> > +- trace_printf("pathspec does not match %s\n", path);
> > ++ trace_printf("Pathspec to submodule does not match explicitly: %s\n", path);
> > + continue;
> > + }
> > + }
> > +@@ read-cache.c: static void update_callback(struct diff_queue_struct *q,
> > data->add_errors++;
> > }
> > break;
> > 3: 8f3d5f7ec1 ! 3: 399a153b95 tests: add new t2206-add-submodule-ignored.sh to test ignore=all scenario
> > @@ Commit message
> > config with ignore=all also behaves as intended with configuration in
> > .gitmodules and configuration given on the command line.
> >
> > - Testfile is added to meson.build for execution.
> > + The usage of --include_ignored_submodules is showcased and tested in the
> > + test suite.
> > +
> > + The test file is added to meson.build for execution.
> >
> > Signed-off-by: Claus Schneider(Eficode) <claus.schneider@eficode.com>
> >
> > @@ t/t2206-add-submodule-ignored.sh (new)
> > +# This test covers the behavior of "git add", "git status" and "git log" when
> > +# dealing with submodules that have the ignore=all setting in
> > +# .gitmodules. It ensures that changes in such submodules are
> > -+# ignored by default, but can be staged with "git add --force".
> > ++# ignored by default, but can be staged with "git add --include-ignored-submodules".
> > +
> > +# shellcheck disable=SC1091
> > +. ./test-lib.sh
> > @@ t/t2206-add-submodule-ignored.sh (new)
> > +'
> > +
> > +#6
> > -+# check that 'git add --force .' does not stage the change in the submodule
> > ++# check that 'git add --include-ignored-submodules .' does not stage the change in the submodule
> > +# and that 'git status' does not show it as modified
> > -+test_expect_success 'main: check --force add . and status' '
> > ++test_expect_success 'main: check --include-ignored-submodules add . and status' '
> > + cd "${base_path}" &&
> > + cd main &&
> > -+ GIT_TRACE=1 git add --force . &&
> > ++ GIT_TRACE=1 git add --include-ignored-submodules . &&
> > + ! git status --porcelain | grep "^M sub$" &&
> > + echo
> > +'
> > @@ t/t2206-add-submodule-ignored.sh (new)
> > +'
> > +
> > +#8
> > -+# check that 'git add --force sub' does stage the change in the submodule
> > -+# check that 'git add --force ./sub/' does stage the change in the submodule
> > ++# check that 'git add --include-ignored-submodules sub' does stage the change in the submodule
> > ++# check that 'git add --include-ignored-submodules ./sub/' does stage the change in the submodule
> > +# and that 'git status --porcelain' does show it as modified
> > +# commit it..
> > +# check that 'git log --ignore-submodules=none' shows the submodule change
> > @@ t/t2206-add-submodule-ignored.sh (new)
> > +test_expect_success 'main: check force add sub and ./sub/ and status' '
> > + cd "${base_path}" &&
> > + cd main &&
> > -+ echo "Adding with --force should work: git add --force sub" &&
> > -+ GIT_TRACE=1 git add --force sub &&
> > ++ echo "Adding with --include-ignored-submodules should work: git add --include-ignored-submodules sub" &&
> > ++ GIT_TRACE=1 git add --include-ignored-submodules sub &&
> > + git status --porcelain | grep "^M sub$" &&
> > + git restore --staged sub &&
> > + ! git status --porcelain | grep "^M sub$" &&
> > -+ echo "Adding with --force should work: git add --force ./sub/" &&
> > -+ GIT_TRACE=1 git add --force ./sub/ &&
> > ++ echo "Adding with --include-ignored-submodules should work: git add --include-ignored-submodules ./sub/" &&
> > ++ GIT_TRACE=1 git add --include-ignored-submodules ./sub/ &&
> > + git status --porcelain | grep "^M sub$" &&
> > + git commit -m "update submodule pointer" &&
> > + ! git status --porcelain | grep "^ M sub$" &&
> > 4: 58563a7b90 ! 4: 93c95954f1 tests: fix existing tests when add an ignore=all submodule
> > @@ Metadata
> > ## Commit message ##
> > tests: fix existing tests when add an ignore=all submodule
> >
> > - There are tests that rely on "git add <submodule>" also adds it. A --force
> > - is needed with this enhancement hence they are added accordingly in these
> > - tests.
> > + There are tests that rely on "git add <submodule>" to add updates in the
> > + parent repository. A new option --include-ignored-submodules is introduced
> > + as it is now needed with this enhancement.
> >
> > Updated tests:
> > - t1013-read-tree-submodule.sh ( fixed in: t/lib-submodule-update.sh )
> > + - t2013-checkout-submodule.sh ( fixed in: t/lib-submodule-update.sh )
> > - t7406-submodule-update.sh
> > - t7508-status.sh
> >
> > @@ t/lib-submodule-update.sh: create_lib_submodule_repo () {
> > git push origin modifications
> > ) &&
> > - git add sub1 &&
> > -+ git add --force sub1 &&
> > ++ git add --include-ignored-submodules sub1 &&
> > git commit -m "Modify sub1" &&
> >
> > git checkout -b add_nested_sub modify_sub1 &&
> > @@ t/lib-submodule-update.sh: create_lib_submodule_repo () {
> > git -C sub1 submodule add --branch no_submodule ../submodule_update_sub2 sub2 &&
> > git -C sub1 commit -a -m "add a nested submodule" &&
> > - git add sub1 &&
> > -+ git add --force sub1 &&
> > ++ git add --include-ignored-submodules sub1 &&
> > git commit -a -m "update submodule, that updates a nested submodule" &&
> > git checkout -b modify_sub1_recursively &&
> > git -C sub1 checkout -b modify_sub1_recursively &&
> > @@ t/lib-submodule-update.sh: create_lib_submodule_repo () {
> > git -C sub1 add sub2 &&
> > git -C sub1 commit -m "update nested sub" &&
> > - git add sub1 &&
> > -+ git add --force sub1 &&
> > ++ git add --include-ignored-submodules sub1 &&
> > git commit -m "update sub1, that updates nested sub" &&
> > git -C sub1 push origin modify_sub1_recursively &&
> > git -C sub1/sub2 push origin modify_sub1_recursively &&
> > @@ t/t7508-status.sh: test_expect_success 'git commit will commit a staged but igno
> > test_expect_success 'git commit --dry-run will show a staged but ignored submodule' '
> > git reset HEAD^ &&
> > - git add sm &&
> > -+ git add --force sm &&
> > ++ git add --include-ignored-submodules sm &&
> > cat >expect << EOF &&
> > On branch main
> > Your branch and '\''upstream'\'' have diverged,
> > 5: 416695f439 < -: ---------- Documentation: update add --force and submodule ignore=all config
> > -: ---------- > 5: ee84190cd8 Documentation: add --include_ignored_submodules + ignore=all config