From: Claus Schneider Date: Fri, 14 Nov 2025 13:53:50 GMT Subject: Re: [PATCH v2 0/5] git-add : Respect submodule ignore=all and only add changes with --force Message-ID: In-Reply-To: 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: > 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..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..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 wrote: > > "Claus Schneider via GitGitGadget" 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..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..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) > > > > ## 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) > > > > ## 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) > > > > ## 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) > > > > @@ 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 " 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 " 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