git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Michael Forney <mforney@mforney.org>
To: Stefan Beller <sbeller@google.com>
Cc: git <git@vger.kernel.org>
Subject: Re: Confusing behavior with ignored submodules and `git commit -a`
Date: Thu, 15 Nov 2018 13:33:57 -0800
Message-ID: <CAGw6cBvLGZKcf1em0d47hcCuKau2QVbX4wfb0yN+m4umbNLaRg@mail.gmail.com> (raw)
In-Reply-To: <CAGZ79kYiWnciitwTQCXR5bHOj7nhHWr40xBiS5sPCH5W4_yQ5w@mail.gmail.com>

On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Nov 14, 2018 at 10:05 PM Michael Forney <mforney@mforney.org>
> wrote:
>> 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).

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.

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;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 	clear_pathspec(&rev.prune_data);
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)
 {
 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
 		struct diff_flags orig_flags = diffopt->flags;
-		if (!diffopt->flags.override_submodule_config)
+		if (!diffopt->flags.override_submodule_config && !exact)
 			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
 		if (diffopt->flags.ignore_submodules)
 			changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct
diff_options *diffopt,

 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
-	int entries, i;
+	int entries, i, matched;
 	int diff_unmerged_stage = revs->max_count;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
 		if (diff_can_quit_early(&revs->diffopt))
 			break;

-		if (!ce_path_match(istate, ce, &revs->prune_data, NULL))
+		matched = ce_path_match(istate, ce, &revs->prune_data, NULL);
+		if (!matched)
 			continue;

 		if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
 			}

 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-							    ce_option, &dirty_submodule);
+							    ce_option, &dirty_submodule,
+							    matched == MATCHED_EXACTLY);
 			newmode = ce_mode_from_stat(ce, st.st_mode);
 		}

@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,
 			return -1;
 		}
 		changed = match_stat_with_submodule(diffopt, ce, &st,
-						    0, dirty_submodule);
+						    0, dirty_submodule, 0);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			oid = &null_oid;
diff --git a/diff.c b/diff.c
index e38d1ecaf..73dc75286 100644
--- a/diff.c
+++ b/diff.c
@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)
 		opt->flags.has_changes);
 }

-/*
- * Shall changes to this submodule be ignored?
- *
- * Submodule changes can be configured to be ignored separately for each path,
- * but that configuration can be overridden from the command line.
- */
-static int is_submodule_ignored(const char *path, struct diff_options *options)
-{
-	int ignored = 0;
-	struct diff_flags orig_flags = options->flags;
-	if (!options->flags.override_submodule_config)
-		set_diffopt_flags_from_submodule_config(options, path);
-	if (options->flags.ignore_submodules)
-		ignored = 1;
-	options->flags = orig_flags;
-	return ignored;
-}
-
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const struct object_id *oid,
@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options,
 {
 	struct diff_filespec *one, *two;

-	if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))
+	if (S_ISGITLINK(mode) && options->flags.ignore_submodules)
 		return;

 	/* This may look odd, but it is a preparation for
@@ -6285,7 +6267,7 @@ void diff_change(struct diff_options *options,
 	struct diff_filepair *p;

 	if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&
-	    is_submodule_ignored(concatpath, options))
+	    options->flags.ignore_submodules)
 		return;

 	if (options->flags.reverse_diff) {

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-17  3:57 Michael Forney
2018-10-25 18:03 ` Michael Forney
2018-10-25 18:26   ` Stefan Beller
2018-11-15  5:12     ` Michael Forney
2018-11-15  6:05       ` Michael Forney
2018-11-15 20:03         ` Stefan Beller
2018-11-15 21:33           ` Michael Forney [this message]
2018-11-15 21:53             ` Michael Forney
2018-11-15 22:23             ` Stefan Beller
2018-11-16  0:31               ` Michael Forney
2018-11-27  0:03                 ` Stefan Beller
2018-11-15 19:39       ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGw6cBvLGZKcf1em0d47hcCuKau2QVbX4wfb0yN+m4umbNLaRg@mail.gmail.com \
    --to=mforney@mforney.org \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox