Re: Confusing behavior with ignored submodules and `git commit -a`
From: Stefan Beller <hidden>
Date: 2018-11-15 22:23:16
On Thu, Nov 15, 2018 at 1:33 PM Michael Forney [off-list ref] wrote:
On 2018-11-15, Stefan Beller [off-list ref] wrote:quoted
On Wed, Nov 14, 2018 at 10:05 PM Michael Forney [off-list ref] wrote:quoted
Looking at ff6f1f564c, I don't really see anything that might be related to git-add, git-reset, or git-diff, so I'm guessing that this only worked before because the submodule config wasn't getting loaded during `git add` or `git reset`. Now that the config is loaded automatically, submodule.<name>.ignore started taking effect where it shouldn't. Unfortunately, this doesn't really get me much closer to finding a fix.Maybe selectively unloading or overwriting the config? Or we can change is_submodule_ignored() in diff.c to be only applied selectively whether we are running the right command? For this approach we'd have to figure out the set of commands to which the ignore config should apply or not (and come up with a more concise documentation then) This approach sounds appealing to me as it would cover new commands as well and we'd only have a central point where the decision for ignoring is made.Well, currently the submodule config can be disabled in diff_flags by setting override_submodule_config=1. However, I'm thinking it may be simpler to selectively *enable* the submodule config in diff_flags where it is needed instead of disabling it everywhere else (i.e. use_submodule_config instead of override_submodule_config).
This sounds like undoing the good(?) part of the series that introduced this regression, as before that we selectively loaded the submodule config, which lead to confusion when you forgot it. Selectively *enabling* the submodule config sounds like that state before? Or do we *only* talk about enabling the ignore flag, while loading the rest of the submodule config automatic?
I'm also starting to see why this is tricky. The only difference that diff.c:run_diff_files sees between `git add inner` and `git add --all` is whether the index entry matched the pathspec exactly or not.
Unrelated to the trickiness, I think we'd need to document the behavior of the -a flag in git-add and git-commit better as adding the diff below will depart from the "all" rule again, which I thought was a strong motivator for Brandons series (IIRC).
quoted hunk ↗ jump to hunk
Here is a work-in-progress diff that seems to have the correct behavior in all cases I tried. Can you think of any cases that it breaks? I'm not quite sure of the consequences of having diff_change and diff_addremove always ignore the submodule config; git-diff and git-status still seem to work correctly.diff --git a/builtin/add.c b/builtin/add.c index f65c17229..9902f7742 100644 --- a/builtin/add.c +++ b/builtin/add.c@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix, rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = update_callback; rev.diffopt.format_callback_data = &data; - rev.diffopt.flags.override_submodule_config = 1;
This line partially reverts 5556808, taking 02f2f56bc377c28 into account.
quoted hunk ↗ jump to hunk
diff --git a/diff-lib.c b/diff-lib.c index 83fce5151..fbb048cca 100644 --- a/diff-lib.c +++ b/diff-lib.c@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry*ce, struct stat *st) static int match_stat_with_submodule(struct diff_options *diffopt, const struct cache_entry *ce, struct stat *st, unsigned ce_option, - unsigned *dirty_submodule) + unsigned *dirty_submodule, + int exact) [...];
This is an interesting take so far as it is all about *detecting* change here via stat information and not like the previous (before the regression) where it was about correcting output. match_stat_with_submodule would grow its documentation to be slightly more complicated as a result.
quoted hunk ↗ jump to hunk
diff --git a/diff.c b/diff.c index e38d1ecaf..73dc75286 100644 --- a/diff.c +++ b/diff.c[...] -static int is_submodule_ignored(const char *path, struct diff_options *options) -{ [...] - if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options)) + if (S_ISGITLINK(mode) && options->flags.ignore_submodules) return;
This basically inlines the function is_submodule_ignored,
except for the part:
if (!options->flags.override_submodule_config)
set_diffopt_flags_from_submodule_config(options, path);
but that was taken care off in match_stat_with_submodule in diff-lib?
This WIP looks really promising, thanks for looking into this!
Stefan