git commit vs. ignore-submodules

Hello.

As this is my first post to this list, let me first thank all the people involved in Git development - it's really a great tool.

Now to the point. Since Git 1.8 (I think), git commit command honours the submodules' ignore settings, configured either in .gitmodules, or in .git/config. That's very nice and certainly correct for "git commit -a", but it's less clear if one explicitely stages an updated submodule using git add. Git commit will ignore it anyway, if ignore=all is configured in .gitmodules. Maybe that's correct too, I'm not sure about that, but it's inconvenient in our use case, especially combined with the lack of --ignore-submodule parameter to git commit, as git status and git diff have.

We use submodules in such a way that normally we don't ever want to see changes in them in output of git diff and git status. So we set ignore=all in .gitmodules for each submodule. But occasionally, we need to add a new submodule, and sometimes also commit changed submodule. This got harder with Git 1.8, we have to "git config submodule.<name>.ignore none" before the commit, and "git config --unset ..." after.

I'd like to at least add an --ignore-submodules parameter to git commit. I though about posting a patch, but as I looked into the commit source file, I didn't see any straightforward way to implement it. I don't have enough free time for a deeper analysis of the sources, I'm sorry.

So please, let me first know, whether you could possibly accept such patch, and if so, then I'd really appreciate some hints on how to do it.

And also, I'd like to know git folks' opinion on whether it's OK that git commit with ignore=all in .gitmodules ignores submodules even when they are explicitely staged with git add.

Quoted 3 lines

.. but it's less clear if one explicitely stages an updated submodule using git add. Git commit will ignore it anyway, if ignore=all is configured in .gitmodules. Maybe that's correct too

...

Quoted 3 lines

And also, I'd like to know git folks' opinion on whether it's OK that git commit with ignore=all in .gitmodules ignores submodules even when they are explicitely staged with git add.

No, they should be visible in status and commit when the user chose to add them. I see if I can hack something up (as I've been bitten myself by that recently ;-).

Am 28.03.2014 00:36, schrieb Ronald Weiss:

Quoted 3 lines

And also, I'd like to know git folks' opinion on whether it's OK that git commit with ignore=all in .gitmodules ignores submodules even when they are explicitely staged with git add.

No, they should be visible in status and commit when the user chose to add them. I see if I can hack something up (as I've been bitten myself by that recently ;-).

Such a patch would be very much appreciated. You might want to look into other commands that already have the --ignore-submodules option to get an idea how to do that. cmd_status() in builtin/commit.c should be a good starting point.

Quoted 3 lines

And also, I'd like to know git folks' opinion on whether it's OK that git commit with ignore=all in .gitmodules ignores submodules even when they are explicitely staged with git add.

No, they should be visible in status and commit when the user chose to add them. I see if I can hack something up (as I've been bitten myself by that recently ;-).

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line, like other commands do.

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Changes in add.c and cache.h (and related compilo fix in checkout.c) are needed to make it work for "commit -a" too.

Signed-off-by Ronald Weiss <[email protected]>

---
 Documentation/git-commit.txt |  6 ++++++
 builtin/add.c                | 16 ++++++++++++----
 builtin/checkout.c           |  2 +-
 builtin/commit.c             |  8 +++++++-
 cache.h                      |  2 +-
 5 files changed, 27 insertions(+), 7 deletions(-)

Documentation/git-commit.txt +6 -0

index 1a7616c..c8839c8 100644
@@ -13,6 +13,7 @@ SYNOPSIS
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
+	   [--ignore-submodules[=<when>]]
 	   [-i | -o] [-S[<keyid>]] [--] [<file>...]
 
 DESCRIPTION

@@ -271,6 +272,11 @@ The possible options are:
 The default can be changed using the status.showUntrackedFiles
 configuration variable documented in linkgit:git-config[1].
 
+--ignore-submodules[=<when>]::
+	Can be used to override any settings of the 'ignore' option
+	in linkgit:git-config[1] or linkgit:gitmodules[5].
+	<when> can be either "none" or "all", which is the default.
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what

builtin/add.c +12 -4

index 672adc0..1086294 100644
@@ -168,7 +168,8 @@ static void update_callback(struct diff_queue_struct *q,
 
 static void update_files_in_cache(const char *prefix,
 				  const struct pathspec *pathspec,
-				  struct update_callback_data *data)
+				  struct update_callback_data *data,
+				  const char *ignore_submodules_arg)
 {
 	struct rev_info rev;
 

@@ -180,17 +181,24 @@ static void update_files_in_cache(const char *prefix,
 	rev.diffopt.format_callback = update_callback;
 	rev.diffopt.format_callback_data = data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
+
+	if (ignore_submodules_arg) {
+		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+		handle_ignore_submodules_arg(&rev.diffopt, ignore_submodules_arg);
+	}
+
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 }
 
 int add_files_to_cache(const char *prefix,
-		       const struct pathspec *pathspec, int flags)
+		       const struct pathspec *pathspec, int flags,
+		       const char *ignore_submodules_arg)
 {
 	struct update_callback_data data;
 
 	memset(&data, 0, sizeof(data));
 	data.flags = flags;
-	update_files_in_cache(prefix, pathspec, &data);
+	update_files_in_cache(prefix, pathspec, &data, ignore_submodules_arg);
 	return !!data.add_errors;
 }
 

@@ -576,7 +584,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		memset(&pathspec, 0, sizeof(pathspec));
 	}
 	update_data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
-	update_files_in_cache(prefix, &pathspec, &update_data);
+	update_files_in_cache(prefix, &pathspec, &update_data, NULL);
 
 	exit_status |= !!update_data.add_errors;
 	if (add_new_files)

builtin/checkout.c +1 -1

index ada51fa..22a4b48 100644
@@ -525,7 +525,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 * entries in the index.
 			 */
 
-			add_files_to_cache(NULL, NULL, 0);
+			add_files_to_cache(NULL, NULL, 0, NULL);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line

builtin/commit.c +7 -1

index 26b2986..121c185 100644
@@ -360,7 +360,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	 */
 	if (all || (also && pathspec.nr)) {
 		fd = hold_locked_index(&index_lock, 1);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, ignore_submodule_arg);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||

@@ -1492,6 +1492,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		{ OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
+		  N_("ignore changes to submodules, optional when: all, none. (Default: all)"),
+		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		/* end commit contents options */
 
 		OPT_HIDDEN_BOOL(0, "allow-empty", &allow_empty,

@@ -1531,6 +1534,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);
+
+	s.ignore_submodule_arg = ignore_submodule_arg;
+
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);

cache.h +1 -1

index ebe9a40..5ef8dd6 100644
@@ -1282,7 +1282,7 @@ void packet_trace_identity(const char *prog);
  * return 0 if success, 1 - if addition of a file failed and
  * ADD_FILES_IGNORE_ERRORS was specified in flags
  */
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, const char *ignore_submodules_arg);
 
 /* diff.c */
 extern int diff_auto_refresh_index;

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line, like other commands do.

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Changes in add.c and cache.h (and related compilo fix in checkout.c) are needed to make it work for "commit -a" too.

Signed-off-by Ronald Weiss <[email protected]>

---
 Documentation/git-commit.txt |  6 ++++++
 builtin/add.c                | 16 ++++++++++++----
 builtin/checkout.c           |  2 +-
 builtin/commit.c             |  8 +++++++-
 cache.h                      |  2 +-
 5 files changed, 27 insertions(+), 7 deletions(-)

Documentation/git-commit.txt +6 -0

index 1a7616c..c8839c8 100644
@@ -13,6 +13,7 @@ SYNOPSIS
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
+	   [--ignore-submodules[=<when>]]
 	   [-i | -o] [-S[<keyid>]] [--] [<file>...]
 
 DESCRIPTION

@@ -271,6 +272,11 @@ The possible options are:
 The default can be changed using the status.showUntrackedFiles
 configuration variable documented in linkgit:git-config[1].
 
+--ignore-submodules[=<when>]::
+	Can be used to override any settings of the 'ignore' option
+	in linkgit:git-config[1] or linkgit:gitmodules[5].
+	<when> can be either "none" or "all", which is the default.
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what

builtin/add.c +12 -4

index 672adc0..1086294 100644
@@ -168,7 +168,8 @@ static void update_callback(struct diff_queue_struct *q,
 
 static void update_files_in_cache(const char *prefix,
 				  const struct pathspec *pathspec,
-				  struct update_callback_data *data)
+				  struct update_callback_data *data,
+				  const char *ignore_submodules_arg)
 {
 	struct rev_info rev;
 

@@ -180,17 +181,24 @@ static void update_files_in_cache(const char *prefix,
 	rev.diffopt.format_callback = update_callback;
 	rev.diffopt.format_callback_data = data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
+
+	if (ignore_submodules_arg) {
+		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+		handle_ignore_submodules_arg(&rev.diffopt, ignore_submodules_arg);
+	}
+
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 }
 
 int add_files_to_cache(const char *prefix,
-		       const struct pathspec *pathspec, int flags)
+		       const struct pathspec *pathspec, int flags,
+		       const char *ignore_submodules_arg)
 {
 	struct update_callback_data data;
 
 	memset(&data, 0, sizeof(data));
 	data.flags = flags;
-	update_files_in_cache(prefix, pathspec, &data);
+	update_files_in_cache(prefix, pathspec, &data, ignore_submodules_arg);
 	return !!data.add_errors;
 }
 

@@ -576,7 +584,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		memset(&pathspec, 0, sizeof(pathspec));
 	}
 	update_data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
-	update_files_in_cache(prefix, &pathspec, &update_data);
+	update_files_in_cache(prefix, &pathspec, &update_data, NULL);
 
 	exit_status |= !!update_data.add_errors;
 	if (add_new_files)

builtin/checkout.c +1 -1

index ada51fa..22a4b48 100644
@@ -525,7 +525,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 * entries in the index.
 			 */
 
-			add_files_to_cache(NULL, NULL, 0);
+			add_files_to_cache(NULL, NULL, 0, NULL);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line

builtin/commit.c +7 -1

index 26b2986..121c185 100644
@@ -360,7 +360,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	 */
 	if (all || (also && pathspec.nr)) {
 		fd = hold_locked_index(&index_lock, 1);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, ignore_submodule_arg);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||

@@ -1492,6 +1492,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		{ OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
+		  N_("ignore changes to submodules, optional when: all, none. (Default: all)"),
+		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		/* end commit contents options */
 
 		OPT_HIDDEN_BOOL(0, "allow-empty", &allow_empty,

@@ -1531,6 +1534,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);
+
+	s.ignore_submodule_arg = ignore_submodule_arg;
+
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);

cache.h +1 -1

index ebe9a40..5ef8dd6 100644
@@ -1282,7 +1282,7 @@ void packet_trace_identity(const char *prog);
  * return 0 if success, 1 - if addition of a file failed and
  * ADD_FILES_IGNORE_ERRORS was specified in flags
  */
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, const char *ignore_submodules_arg);
 
 /* diff.c */
 extern int diff_auto_refresh_index;

Looking good so far, but we definitely need tests for this new option.

But I wonder if it would make more sense to start by teaching the --ignore-submodules option to git add. Then this patch could reuse that for commit -a.

Looking good so far, but we definitely need tests for this new option.

But I wonder if it would make more sense to start by teaching the --ignore-submodules option to git add. Then this patch could reuse that for commit -a.

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line, like other commands do.

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Changes in add.c and cache.h (and related compilo fix in checkout.c) are needed to make it work for "commit -a" too.

Signed-off-by Ronald Weiss <[email protected]>

---
The previous patch version (v2) contained bug in the test, by mistake I have sent older version than I was testing with, sorry for that.

Quoted 1 line · Jens Lehmann

Looking good so far, but we definitely need tests for this new option.

I added two simple tests (one for --ignore-submodules=all, another one for =none). But I am sure the tests could be written better, by someone more proficient in Git than I am.

The tests immediately revealed, that the patch was not complete. It didn't work with commit message given on command line (-m). To make that work, I had to also patch the index_differs_from function in diff-lib.c.

Quoted 3 lines

But I wonder if it would make more sense to start by teaching the --ignore-submodules option to git add. Then this patch could reuse that for commit -a.

That sounds reasonable, however I don't think that any code from my patch would be affected, or would it? IOW, would commit really "reuse" anything? If not (or not much), there is probably no point in postponing this patch, support for git add may be added later.

 Documentation/git-commit.txt        |  6 ++++++
 builtin/add.c                       | 16 +++++++++++----
 builtin/checkout.c                  |  2 +-
 builtin/commit.c                    | 10 ++++++++--
 cache.h                             |  2 +-
 diff-lib.c                          |  7 ++++++-
 diff.h                              |  3 ++-
 sequencer.c                         |  4 ++--
 t/t7513-commit-ignore-submodules.sh | 39 +++++++++++++++++++++++++++++++++++++
 9 files changed, 77 insertions(+), 12 deletions(-)
 create mode 100644 t/t7513-commit-ignore-submodules.sh

Documentation/git-commit.txt +6 -0

index 1a7616c..c8839c8 100644
@@ -13,6 +13,7 @@ SYNOPSIS
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
+	   [--ignore-submodules[=<when>]]
 	   [-i | -o] [-S[<keyid>]] [--] [<file>...]
 
 DESCRIPTION

@@ -271,6 +272,11 @@ The possible options are:
 The default can be changed using the status.showUntrackedFiles
 configuration variable documented in linkgit:git-config[1].
 
+--ignore-submodules[=<when>]::
+	Can be used to override any settings of the 'ignore' option
+	in linkgit:git-config[1] or linkgit:gitmodules[5].
+	<when> can be either "none" or "all", which is the default.
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what

builtin/add.c +12 -4

index 672adc0..1086294 100644
@@ -168,7 +168,8 @@ static void update_callback(struct diff_queue_struct *q,
 
 static void update_files_in_cache(const char *prefix,
 				  const struct pathspec *pathspec,
-				  struct update_callback_data *data)
+				  struct update_callback_data *data,
+				  const char *ignore_submodules_arg)
 {
 	struct rev_info rev;
 

@@ -180,17 +181,24 @@ static void update_files_in_cache(const char *prefix,
 	rev.diffopt.format_callback = update_callback;
 	rev.diffopt.format_callback_data = data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
+
+	if (ignore_submodules_arg) {
+		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+		handle_ignore_submodules_arg(&rev.diffopt, ignore_submodules_arg);
+	}
+
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 }
 
 int add_files_to_cache(const char *prefix,
-		       const struct pathspec *pathspec, int flags)
+		       const struct pathspec *pathspec, int flags,
+		       const char *ignore_submodules_arg)
 {
 	struct update_callback_data data;
 
 	memset(&data, 0, sizeof(data));
 	data.flags = flags;
-	update_files_in_cache(prefix, pathspec, &data);
+	update_files_in_cache(prefix, pathspec, &data, ignore_submodules_arg);
 	return !!data.add_errors;
 }
 

@@ -576,7 +584,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		memset(&pathspec, 0, sizeof(pathspec));
 	}
 	update_data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
-	update_files_in_cache(prefix, &pathspec, &update_data);
+	update_files_in_cache(prefix, &pathspec, &update_data, NULL);
 
 	exit_status |= !!update_data.add_errors;
 	if (add_new_files)

builtin/checkout.c +1 -1

index ada51fa..22a4b48 100644
@@ -525,7 +525,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 * entries in the index.
 			 */
 
-			add_files_to_cache(NULL, NULL, 0);
+			add_files_to_cache(NULL, NULL, 0, NULL);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line

builtin/commit.c +8 -2

index 26b2986..b3fb28e 100644
@@ -360,7 +360,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	 */
 	if (all || (also && pathspec.nr)) {
 		fd = hold_locked_index(&index_lock, 1);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, ignore_submodule_arg);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||

@@ -827,7 +827,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (get_sha1(parent, sha1))
 			commitable = !!active_nr;
 		else
-			commitable = index_differs_from(parent, 0);
+			commitable = index_differs_from(parent, 0, ignore_submodule_arg);
 	}
 	strbuf_release(&committer_ident);
 

@@ -1492,6 +1492,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		{ OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
+		  N_("ignore changes to submodules, optional when: all, none. (Default: all)"),
+		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		/* end commit contents options */
 
 		OPT_HIDDEN_BOOL(0, "allow-empty", &allow_empty,

@@ -1531,6 +1534,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);
+
+	s.ignore_submodule_arg = ignore_submodule_arg;
+
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);

cache.h +1 -1

index ebe9a40..5ef8dd6 100644
@@ -1282,7 +1282,7 @@ void packet_trace_identity(const char *prog);
  * return 0 if success, 1 - if addition of a file failed and
  * ADD_FILES_IGNORE_ERRORS was specified in flags
  */
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, const char *ignore_submodules_arg);
 
 /* diff.c */
 extern int diff_auto_refresh_index;

diff-lib.c +6 -1

index 2eddc66..fec5ad4 100644
@@ -508,7 +508,8 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags)
+int index_differs_from(const char *def, int diff_flags,
+		       const char *ignore_submodules_arg)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;

@@ -520,6 +521,10 @@ int index_differs_from(const char *def, int diff_flags)
 	DIFF_OPT_SET(&rev.diffopt, QUICK);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	rev.diffopt.flags |= diff_flags;
+	if (ignore_submodules_arg) {
+		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+		handle_ignore_submodules_arg(&rev.diffopt, ignore_submodules_arg);
+	}
 	run_diff_index(&rev, 1);
 	if (rev.pending.alloc)
 		free(rev.pending.objects);

diff.h +2 -1

index ce123fa..0ac869b 100644
@@ -334,7 +334,8 @@ extern int diff_result_code(struct diff_options *, int);
 
 extern void diff_no_index(struct rev_info *, int, const char **, const char *);
 
-extern int index_differs_from(const char *def, int diff_flags);
+extern int index_differs_from(const char *def, int diff_flags,
+			      const char *ignore_submodules_arg);
 
 extern size_t fill_textconv(struct userdiff_driver *driver,
 			    struct diff_filespec *df,

sequencer.c +2 -2

index 90cac7b..de9edec 100644
@@ -496,7 +496,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		unborn = get_sha1("HEAD", head);
 		if (unborn)
 			hashcpy(head, EMPTY_TREE_SHA1_BIN);
-		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
+		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0, NULL))
 			return error_dirty_index(opts);
 	}
 	discard_cache();

@@ -1042,7 +1042,7 @@ static int sequencer_continue(struct replay_opts *opts)
 		if (ret)
 			return ret;
 	}
-	if (index_differs_from("HEAD", 0))
+	if (index_differs_from("HEAD", 0, NULL))
 		return error_dirty_index(opts);
 	todo_list = todo_list->next;
 	return pick_commits(todo_list, opts);

t/t7513-commit-ignore-submodules.sh new file +39 -0

new file mode 100644
index 0000000..a7148ce
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='git commit --ignore-submodules'
+
+. ./test-lib.sh
+
+test_expect_success 'create submodule' '
+	test_create_repo sm && (
+		cd sm &&
+		>foo &&
+		git add foo &&
+		git commit -m "Add foo"
+	) &&
+	git submodule add ./sm &&
+	git commit -m "Add sm"
+'
+
+update_sm () {
+	(cd sm &&
+		echo bar >> foo &&
+		git add foo &&
+		git commit -m "Updated foo"
+	)
+}
+
+test_expect_success 'commit -a --ignore-submodules=all ignores dirty submodule' '
+	update_sm &&
+	test_must_fail git commit -a --ignore-submodules=all -m "Update sm"
+'
+
+test_expect_success 'commit -a --ignore-submodules=none overrides ignore=all setting' '
+	update_sm &&
+	git config submodule.sm.ignore all &&
+	git commit -a --ignore-submodules=none -m "Update sm" &&
+	git diff-index --exit-code --cached --ignore-submodules=none HEAD &&
+	git diff-files --exit-code --ignore-submodules=none
+'
+
+test_done

Am 31.03.2014 02:07, schrieb Ronald Weiss:

Quoted 4 lines

The tests immediately revealed, that the patch was not complete. It didn't work with commit message given on command line (-m). To make that work, I had to also patch the index_differs_from function in diff-lib.c.

Which is exactly the same function I have to tweak to make my "status/commit: always show staged submodules regardless of ignore config" patch work for "git commit -m" too ;-)

I was doing that slightly differently but it seems that your way of adding the "ignore_submodules_arg" parameter could serve us both. Will update my upcoming patch accordingly.

Quoted 8 lines

diff-lib.c +0 -0

index 2eddc66..fec5ad4 100644
@@ -508,7 +508,8 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	return 0;
 }
 

diff-lib.c int index_differs_from(const char *def, int diff_flags) · line 525

Quoted 12 lines

diff-lib.c +4 -1

-int index_differs_from(const char *def, int diff_flags)
+int index_differs_from(const char *def, int diff_flags,
+		       const char *ignore_submodules_arg)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;

@@ -520,6 +521,10 @@ int index_differs_from(const char *def, int diff_flags)
 	DIFF_OPT_SET(&rev.diffopt, QUICK);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	rev.diffopt.flags |= diff_flags;
+	if (ignore_submodules_arg) {
+		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);

diff-lib.c int index_differs_from(const char *def, int diff_flags) · line 530

Quoted 5 lines

diff-lib.c +2 -0

+		handle_ignore_submodules_arg(&rev.diffopt, ignore_submodules_arg);
+	}
 	run_diff_index(&rev, 1);
 	if (rev.pending.alloc)
 		free(rev.pending.objects);

... maybe the best way is to leave index_differs_from() unchanged and call that function with the correct diff_flags instead:

(quoted diff) +5 -0

+               int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+               if (ignore_submodule_arg &&
+                   !strcmp(ignore_submodule_arg, "all"))
+                       diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
+               commitable = index_differs_from(parent, diff_flags);

Not thoroughly tested yet, but that'd also prevent any fallout for the two callsites of index_differs_from() in sequencer.c.

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 5 lines · Jens Lehmann

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 7 lines · Ronald Weiss

Quoted 5 lines · Jens Lehmann

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

So I'm going to do some more testing, write tests for git add with ignoring submodules the various ways, and then post two patches rearranged, one for git add, and second for git commit on top of that, as you guys suggested. Including the change suggested by Jens, to not mess with index_differs_from() in diff-lib.c, that works fine too.

builtin/add.c +7 -1

index 1086294..9f70327 100644
@@ -350,6 +350,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing;
  static int addremove = ADDREMOVE_DEFAULT;
  static int addremove_explicit = -1; /* unspecified */
 
+static char *ignore_submodule_arg;
+
  static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
  {
         /* if we are told to ignore, we are not adding removals */

@@ -375,6 +377,9 @@ static struct option builtin_add_options[] = {
         OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
         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")),
+       { OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
+         N_("ignore changes to submodules, optional when: all, none. (Default: all)"),
+         PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
         OPT_END(),
  };
 

@@ -422,6 +427,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
         int implicit_dot = 0;
         struct update_callback_data update_data;
 
+       gitmodules_config();
         git_config(add_config, NULL);
 
         argc = parse_options(argc, argv, prefix, builtin_add_options,

@@ -584,7 +590,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
                 memset(&pathspec, 0, sizeof(pathspec));
         }
         update_data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
-       update_files_in_cache(prefix, &pathspec, &update_data, NULL);
+       update_files_in_cache(prefix, &pathspec, &update_data, ignore_submodule_arg);
 
         exit_status |= !!update_data.add_errors;
         if (add_new_files)

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Am 01.04.2014 01:35, schrieb Ronald Weiss:

Quoted 26 lines · Ronald Weiss

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Quoted 1 line

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Me thinks git add should require the '-f' option to add an ignored submodule (just like it does for files) unless the user uses the '--ignore-submodules=none' option. And if neither of these are given it should "fail with a list of ignored files" as the documentation states.

Quoted 27 lines

Am 01.04.2014 01:35, schrieb Ronald Weiss:

Quoted 26 lines · Ronald Weiss

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Quoted 7 lines

Quoted 1 line

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Me thinks git add should require the '-f' option to add an ignored submodule (just like it does for files) unless the user uses the '--ignore-submodules=none' option. And if neither of these are given it should "fail with a list of ignored files" as the documentation states.

It's still not clear, at least not to me. Should '-f' suppress the ignore setting of all involved submodules? That would make it a synonyme (or a superset) of --ignore-submodules=none. Or only if the submodule is explicitly named on command line? That seems fuzzy to me, and also more tricky to implement.

Am 01.04.2014 23:59, schrieb Ronald Weiss:

Quoted 39 lines · Jens Lehmann

Quoted 29 lines

Am 01.04.2014 01:35, schrieb Ronald Weiss:

Quoted 28 lines · Ronald Weiss

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Quoted 8 lines

Quoted 2 lines

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Me thinks git add should require the '-f' option to add an ignored submodule (just like it does for files) unless the user uses the '--ignore-submodules=none' option. And if neither of these are given it should "fail with a list of ignored files" as the documentation states.

Quoted 5 lines

It's still not clear, at least not to me. Should '-f' suppress the ignore setting of all involved submodules? That would make it a synonyme (or a superset) of --ignore-submodules=none. Or only if the submodule is explicitly named on command line? That seems fuzzy to me, and also more tricky to implement.

Maybe my impression that doing "add" together with "commit" would be easy wasn't correct after all. I won't object if you try to tackle commit first (but I have the slight suspicion that similar questions will arise concerning the "add"ish functionality in commit too. So maybe after resolving those things might look clearer ;-)

Quoted 46 lines

Am 01.04.2014 23:59, schrieb Ronald Weiss:

Quoted 39 lines · Jens Lehmann

Quoted 29 lines

Am 01.04.2014 01:35, schrieb Ronald Weiss:

Quoted 28 lines · Ronald Weiss

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Quoted 8 lines

Quoted 2 lines

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Me thinks git add should require the '-f' option to add an ignored submodule (just like it does for files) unless the user uses the '--ignore-submodules=none' option. And if neither of these are given it should "fail with a list of ignored files" as the documentation states.

Quoted 5 lines

It's still not clear, at least not to me. Should '-f' suppress the ignore setting of all involved submodules? That would make it a synonyme (or a superset) of --ignore-submodules=none. Or only if the submodule is explicitly named on command line? That seems fuzzy to me, and also more tricky to implement.

Quoted 5 lines

Maybe my impression that doing "add" together with "commit" would be easy wasn't correct after all. I won't object if you try to tackle commit first (but I have the slight suspicion that similar questions will arise concerning the "add"ish functionality in commit too. So maybe after resolving those things might look clearer ;-)

There is one big distinction. My patch for commit doesn't add any new problems. It just adds the --ignore-submodules argument, which is easy to implement and no unclear behavior decisions are needed.

You are right that when specifying ignored submodules on commit's command line, there is the same problem as with git add. However, it's already there anyway. I don't feel in position to solve it, I'd just like to have "git commit --ignore-submodules=none".

With git add however, changing it to honor settings from .gitmodules would change behavior people might be used to, so I would be afraid to do that. Btw add also has the problem already, but only if somebody configures the submodule's ignore setting in .git/config, rather than .gitmodules. I don't know how much real use case that is.

As I see it, there are now these rather easy possibilities (sorted from the easiest):

1) Just teach commit the --ignore-submodules argument, as I proposed.

2) Teach both add and commit to --ignore-submodules, but dont add that problematic gitmodules_config() in add.c.

3) Teach both add and commit to --ignore-submodules, and also let add honor settings from .gitmodules, to make it more consistent with other commands. And, make add --force imply --ignore-submodules=none.

I like both 1) and 2). I don't like 3), the problem of add with submodules' ignore setting is a bug IMHO (ignore=all in .git/config causes strange behavior, while ignore=all in .gitmodules is ignored), but not directly related to the --ignore-submodules param, and should be solved separately.

Am 02.04.2014 21:56, schrieb Ronald Weiss:

Quoted 68 lines · Jens Lehmann

Quoted 46 lines

Am 01.04.2014 23:59, schrieb Ronald Weiss:

Quoted 39 lines · Jens Lehmann

Quoted 29 lines

Am 01.04.2014 01:35, schrieb Ronald Weiss:

Quoted 28 lines · Ronald Weiss

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Quoted 8 lines

Quoted 2 lines

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Me thinks git add should require the '-f' option to add an ignored submodule (just like it does for files) unless the user uses the '--ignore-submodules=none' option. And if neither of these are given it should "fail with a list of ignored files" as the documentation states.

Quoted 5 lines

It's still not clear, at least not to me. Should '-f' suppress the ignore setting of all involved submodules? That would make it a synonyme (or a superset) of --ignore-submodules=none. Or only if the submodule is explicitly named on command line? That seems fuzzy to me, and also more tricky to implement.

Quoted 5 lines

Maybe my impression that doing "add" together with "commit" would be easy wasn't correct after all. I won't object if you try to tackle commit first (but I have the slight suspicion that similar questions will arise concerning the "add"ish functionality in commit too. So maybe after resolving those things might look clearer ;-)

There is one big distinction. My patch for commit doesn't add any new problems. It just adds the --ignore-submodules argument, which is easy to implement and no unclear behavior decisions are needed.

You are right that when specifying ignored submodules on commit's command line, there is the same problem as with git add. However, it's already there anyway. I don't feel in position to solve it, I'd just like to have "git commit --ignore-submodules=none".

With git add however, changing it to honor settings from .gitmodules would change behavior people might be used to, so I would be afraid to do that. Btw add also has the problem already, but only if somebody configures the submodule's ignore setting in .git/config, rather than .gitmodules. I don't know how much real use case that is.

Quoted 4 lines

As I see it, there are now these rather easy possibilities (sorted from the easiest):

1) Just teach commit the --ignore-submodules argument, as I proposed.

1a) Teach commit to honor ignore from .git/config.

Quoted 2 lines

2) Teach both add and commit to --ignore-submodules, but dont add that problematic gitmodules_config() in add.c.

Why is that problematic after add learned --ignore-submodules=none?

Quoted 9 lines

3) Teach both add and commit to --ignore-submodules, and also let add honor settings from .gitmodules, to make it more consistent with other commands. And, make add --force imply --ignore-submodules=none.

I like both 1) and 2). I don't like 3), the problem of add with submodules' ignore setting is a bug IMHO (ignore=all in .git/config causes strange behavior, while ignore=all in .gitmodules is ignored), but not directly related to the --ignore-submodules param, and should be solved separately.

I think the ignore config options and --ignore-submodules parameter are directly related, as you need the latter to override the former. In the long run commit should honor ignore=all in .git/config for unstaged submodules like add should honor the settings from the .gitmodules file. But we should always add the --ignore-submodules parameter first so that the user can override the configuration when needed. So I see these steps:

1) Teach commit the --ignore-submodules option; then make it honor ignore=all in .git/config in another commit.

2) Teach add --ignore-submodules (which is implied by -f, but only for the submodules given on the command line); then make it honor the submodule.<name>.ignore option in another commit.

After that we'd have consistent ignore and override behavior. But it looks like getting -f right is not easy, so I'd prefer having
1) without 2) if the alternative is to get neither.

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line, like other commands do.

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

This patch depends on Jens Lehmann's patch "commit -m: commit staged submodules regardless of ignore config". Without it, "commit -m --ignore-submodules" would not work and tests introduced here would fail.

Signed-off-by Ronald Weiss <[email protected]>

---
 Documentation/git-commit.txt        |  6 ++++++
 builtin/commit.c                    |  8 ++++++-
 t/t7513-commit-ignore-submodules.sh | 42 +++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 t/t7513-commit-ignore-submodules.sh

Documentation/git-commit.txt +6 -0

index 1a7616c..8d3b2db 100644
@@ -13,6 +13,7 @@ SYNOPSIS
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
+	   [--ignore-submodules[=<when>]]
 	   [-i | -o] [-S[<keyid>]] [--] [<file>...]
 
 DESCRIPTION

@@ -271,6 +272,11 @@ The possible options are:
 The default can be changed using the status.showUntrackedFiles
 configuration variable documented in linkgit:git-config[1].
 
+--ignore-submodules[=<when>]::
+	Can be used to override any settings of the 'submodule.*.ignore'
+	option in linkgit:git-config[1] or linkgit:gitmodules[5].
+	<when> can be either "none" or "all", which is the default.
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what

builtin/commit.c +7 -1

index 0db215b..121c185 100644
@@ -360,7 +360,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	 */
 	if (all || (also && pathspec.nr)) {
 		fd = hold_locked_index(&index_lock, 1);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, NULL);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, ignore_submodule_arg);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||

@@ -1492,6 +1492,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		{ OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
+		  N_("ignore changes to submodules, optional when: all, none. (Default: all)"),
+		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		/* end commit contents options */
 
 		OPT_HIDDEN_BOOL(0, "allow-empty", &allow_empty,

@@ -1531,6 +1534,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);
+
+	s.ignore_submodule_arg = ignore_submodule_arg;
+
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);

t/t7513-commit-ignore-submodules.sh new file +42 -0

new file mode 100644
index 0000000..83ce04c
@@ -0,0 +1,42 @@
+#!/bin/sh
+#
+# Copyright (c) 2014 Ronald Weiss
+#
+
+test_description='Test of git commit --ignore-submodules'
+
+. ./test-lib.sh
+
+test_expect_success 'create submodule' '
+	test_create_repo sm && (
+		cd sm &&
+		>foo &&
+		git add foo &&
+		git commit -m "Add foo"
+	) &&
+	git submodule add ./sm &&
+	git commit -m "Add sm"
+'
+
+update_sm () {
+	(cd sm &&
+		echo bar >> foo &&
+		git add foo &&
+		git commit -m "Updated foo"
+	)
+}
+
+test_expect_success 'commit -a --ignore-submodules=all ignores dirty submodule' '
+	update_sm &&
+	test_must_fail git commit -a --ignore-submodules=all -m "Update sm"
+'
+
+test_expect_success 'commit -a --ignore-submodules=none overrides ignore=all setting' '
+	update_sm &&
+	git config submodule.sm.ignore all &&
+	git commit -a --ignore-submodules=none -m "Update sm" &&
+	git diff --exit-code --ignore-submodules=none &&
+	git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_done

Am 08.04.2014 01:03, schrieb Ronald Weiss:

Quoted 3 lines

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line, like other commands do.

Quoted 4 lines

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Unless it outputs a status message, then 'dirty' and 'untracked' do influence what is shown there. Apart from that (and maybe tests for these two cases ;-) this is looking good to me.

Quoted 4 lines

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Unless it outputs a status message, then 'dirty' and 'untracked' do influence what is shown there. Apart from that (and maybe tests for these two cases ;-) this is looking good to me.

Allow ignoring submodules (or not) by command line switch, like diff and status do.

Git add currently doesn't honor ignore from .gitmodules or .git/config, which is related functionality, however I'd like to change that in another patch, coming soon.

This commit is also a prerequisite for the next one in series, which adds the --ignore-submodules switch to git commit.

That's why signature
of function add_files_to_cache was changed.

That also requires compilo fixes in checkout.c and commit.c

index 0000000..db58f0c

t/t3704-add-ignore-submodules.sh new file +45 -0

@@ -0,0 +1,45 @@
+#!/bin/sh
+#
+# Copyright (c) 2014 Ronald Weiss
+#
+
+test_description='Test of git add with ignoring submodules'
+
+. ./test-lib.sh
+
+test_expect_success 'create dirty submodule' '
+	test_create_repo sm && (
+		cd sm &&
+		>foo &&
+		git add foo &&
+		git commit -m "Add foo"
+	) &&
+	git submodule add ./sm &&
+	git commit -m "Add sm" && (
+		cd sm &&
+		echo bar >> foo &&
+		git add foo &&
+		git commit -m "Update foo"
+	)
+'
+
+test_expect_success 'add --ignore-submodules ignores submodule' '
+	git reset &&
+	git add -u --ignore-submodules &&
+	git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_expect_success 'add --ignore-submodules=all ignores submodule' '
+	git reset &&
+	git add -u --ignore-submodules=all &&
+	git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_expect_success 'add --ignore-submodules=none overrides ignore=all from config' '
+	git reset &&
+	git config submodule.sm.ignore all &&
+	git add -u --ignore-submodules=none &&
+	test_must_fail git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_done

Am 13.04.2014 00:45, schrieb Ronald Weiss:

Quoted 6 lines

Allow ignoring submodules (or not) by command line switch, like diff and status do.

Git add currently doesn't honor ignore from .gitmodules or .git/config, which is related functionality, however I'd like to change that in another patch, coming soon.

I think we should drop this paragraph from this commit message. Though I believe it's helpful to add this information after the "---" below to inform readers of the list of your future plans.

Quoted 2 lines

This commit is also a prerequisite for the next one in series, which adds the --ignore-submodules switch to git commit.

But this information definitely belongs here.

Quoted 2 lines

That's why signature
of function add_files_to_cache was changed.

But that's necessary for this patch too, no?

Quoted 2 lines

That also requires compilo fixes in checkout.c and commit.c

Sorry, but I don't know what a "compilo fix" is ;-) .. I suspect you mean that add_files_to_cache() needs a new NULL argument in some places. What about dropping the last two sentences and adding something like "Add a new argument to add_files_to_cache() to pass the command line option and update all other call sites to pass NULL instead." to the first paragraph?

Apart from that and the flags of the test (see below) this patch is looking good to me.

t/t3704-add-ignore-submodules.sh

Quoted 2 lines

t/t3704-add-ignore-submodules.sh new file +0 -0

new file mode 100644

The flags should be 100755 here, currently "make test" fails because of this.

Allow ignoring submodules (or not) by command line switch, like diff and status do.

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line.

This patch depends on Jens Lehmann's patch "commit -m: commit staged submodules regardless of ignore config". Without it, "commit -m --ignore-submodules" would not work and tests introduced here would fail.

index 0000000..52ab37d

t/t7513-commit-ignore-submodules.sh new file +78 -0

@@ -0,0 +1,78 @@
+#!/bin/sh
+#
+# Copyright (c) 2014 Ronald Weiss
+#
+
+test_description='Test of git commit --ignore-submodules'
+
+. ./test-lib.sh
+
+test_expect_success 'create submodule' '
+	test_create_repo sm && (
+		cd sm &&
+		>foo &&
+		git add foo &&
+		git commit -m "Add foo"
+	) &&
+	git submodule add ./sm &&
+	git commit -m "Add sm"
+'
+
+update_sm () {
+	(cd sm &&
+		echo bar >> foo &&
+		git add foo &&
+		git commit -m "Updated foo"
+	)
+}
+
+test_expect_success 'commit -a --ignore-submodules=all ignores dirty submodule' '
+	update_sm &&
+	test_must_fail git commit -a --ignore-submodules=all -m "Update sm"
+'
+
+test_expect_success 'commit -a --ignore-submodules=none overrides ignore=all setting' '
+	update_sm &&
+	git config submodule.sm.ignore all &&
+	git commit -a --ignore-submodules=none -m "Update sm" &&
+	git diff --exit-code --ignore-submodules=none &&
+	git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_expect_success 'commit --ignore-submodules status of submodule with untracked content' '
+	GIT_EDITOR=cat &&
+	export GIT_EDITOR &&
+	echo untracked > sm/untracked &&
+
+	test_might_fail git commit --ignore-submodules=none > output &&
+	test_i18ngrep modified output &&
+
+	test_might_fail git commit --ignore-submodules=untracked > output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_might_fail git commit --ignore-submodules=dirty > output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_might_fail git commit --ignore-submodules=all > output &&
+	test_must_fail test_i18ngrep modified output
+'
+
+test_expect_success 'commit --ignore-submodules status of dirty submodule' '
+	GIT_EDITOR=cat &&
+	export GIT_EDITOR &&
+	echo dirty > sm/foo &&
+
+	test_might_fail git commit --ignore-submodules=none > output &&
+	test_i18ngrep modified output &&
+
+	test_might_fail git commit --ignore-submodules=untracked > output &&
+	test_i18ngrep modified output &&
+
+	test_might_fail git commit --ignore-submodules=dirty > output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_might_fail git commit --ignore-submodules=all > output &&
+	test_must_fail test_i18ngrep modified output
+'
+
+test_done

Am 13.04.2014 00:49, schrieb Ronald Weiss:

Quoted 10 lines

Allow ignoring submodules (or not) by command line switch, like diff and status do.

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line.

This patch depends on Jens Lehmann's patch "commit -m: commit staged submodules regardless of ignore config". Without it, "commit -m --ignore-submodules" would not work and tests introduced here would fail.

Apart from the flags of the test (see below) I get three failures when running t7513. And I'm wondering why you are using "test_might_fail" there, shouldn't we know exactly if a commit should succeed or not and test exactly for that?

Quoted 12 lines

Am 13.04.2014 00:49, schrieb Ronald Weiss:

Quoted 10 lines

Allow ignoring submodules (or not) by command line switch, like diff and status do.

Git commit honors the 'ignore' setting from .gitmodules or .git/config, but didn't allow to override it from command line.

This patch depends on Jens Lehmann's patch "commit -m: commit staged submodules regardless of ignore config". Without it, "commit -m --ignore-submodules" would not work and tests introduced here would fail.

Quoted 4 lines

Apart from the flags of the test (see below) I get three failures when running t7513. And I'm wondering why you are using "test_might_fail" there, shouldn't we know exactly if a commit should succeed or not and test exactly for that?

I used "test_might_fail" instead of "test_must_fail" to denote that this test doesn't care whether "git commit" fails or not due to empty commit message. I found it more appropriate. However, if you disagree, I can change it to "test_must_fail", no problem for me.

Unfortunately I don't know why the test fails for you, they pass for me. Did you apply it on top of your own patch for "commit -m", which is a prerequisite?

I tried it again, on top of current master (cc29195 [tag: v2.0.0-rc0]). First, I have applied your patch from here:

http://article.gmane.org/gmane.comp.version-control.git/245783

On top of that, I applied my two patches, and the tests pass fine here. Until now I was testing on Windows, but now I tested on a Linux box, and that makes no difference.

Allow ignoring submodules (or not) by command line switch, like diff and status do.

This commit is also a prerequisite for the next one in series, which adds the --ignore-submodules switch to git commit. That's why a new argument is added to public function add_files_to_cache(), and it's

call sites are updated to pass NULL.

Signed-off-by Ronald Weiss <[email protected]>

---

Documentation/git-add.txt +1 -1

index 9631526..b2c936f 100644
@@ -11,7 +11,7 @@ SYNOPSIS
 'git add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
          [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
          [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
-         [--] [<pathspec>...]
+         [--ignore-submodules[=<when>]] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------

Documentation/git-add.txt +0 -0

 \--::
        This option can be used to separate command-line options from
        the list of files, (useful when filenames might be mistaken

t/t3704-add-ignore-submodules.sh new file +1 -0

new file mode 100755
index 0000000..db58f0c
@@ -0,0 +1,45 @@
+#!/bin/sh

t/t3704-add-ignore-submodules.sh new file +25 -1

+               git add foo &&
+               git commit -m "Update foo"
+       )
+'
+
+test_expect_success 'add --ignore-submodules ignores submodule' '
+       git reset &&
+       git add -u --ignore-submodules &&
+       git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_expect_success 'add --ignore-submodules=all ignores submodule' '
+       git reset &&
+       git add -u --ignore-submodules=all &&
+       git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_expect_success 'add --ignore-submodules=none overrides ignore=all from config' '
+       git reset &&
+       git config submodule.sm.ignore all &&
+       git add -u --ignore-submodules=none &&
+       test_must_fail git diff --cached --exit-code --ignore-submodules=none
+'
+
+test_done
--

1.9.1.3.g7790cde.dirty

Quoted 8 lines · Jens Lehmann

Quoted 4 lines

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Unless it outputs a status message, then 'dirty' and 'untracked' do influence what is shown there. Apart from that (and maybe tests for these two cases ;-) this is looking good to me.

OK, I updated the patch for commit to take that into account. Also, I rebased both patches onto current master. Sending them in a moment.

If you don't have any more complaints, can I add "Acked-by: <you>" and resend the patches to Junio?

Quoted 13 lines · Ronald Weiss

Quoted 8 lines · Jens Lehmann

Quoted 4 lines

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Unless it outputs a status message, then 'dirty' and 'untracked' do influence what is shown there. Apart from that (and maybe tests for these two cases ;-) this is looking good to me.

OK, I updated the patch for commit to take that into account. Also, I rebased both patches onto current master. Sending them in a moment.

Quoted 2 lines

If you don't have any more complaints, can I add "Acked-by: <you>" and resend the patches to Junio?

It is not "When I see no more complaints, I'll resend with your Ack". An Ack is a positive thing, not lack of discovery of further issues.

Rather, it is more like "I'll wait for your Acks and then I'll resend with your Ack", or "If they look good, reply with Ack and let the maintainer pick them up".

Quoted 18 lines · Junio C Hamano

Quoted 15 lines · Ronald Weiss

Quoted 8 lines · Jens Lehmann

Quoted 4 lines

Useful <when> values for commit are 'all' (default) or 'none'. The others ('dirty' and 'untracked') have same effect as 'none', as commit is only interested in whether the submodule's HEAD differs from what is commited in the superproject.

Unless it outputs a status message, then 'dirty' and 'untracked' do influence what is shown there. Apart from that (and maybe tests for these two cases ;-) this is looking good to me.

OK, I updated the patch for commit to take that into account. Also, I rebased both patches onto current master. Sending them in a moment.

If you don't have any more complaints, can I add "Acked-by: <you>" and resend the patches to Junio?

Quoted 3 lines

It is not "When I see no more complaints, I'll resend with your Ack". An Ack is a positive thing, not lack of discovery of further issues.

I'm really sorry if the tone of my message sounded harsh to you, it wasn't meant like that at all.

Quoted 3 lines

Rather, it is more like "I'll wait for your Acks and then I'll resend with your Ack", or "If they look good, reply with Ack and let the maintainer pick them up".

OK.

Quoted 72 lines

Am 02.04.2014 21:56, schrieb Ronald Weiss:

Quoted 71 lines · Jens Lehmann

Quoted 46 lines

Am 01.04.2014 23:59, schrieb Ronald Weiss:

Quoted 39 lines · Jens Lehmann

Quoted 29 lines

Am 01.04.2014 01:35, schrieb Ronald Weiss:

Quoted 28 lines · Ronald Weiss

Quoted 17 lines · Ronald Weiss

Quoted 7 lines · Jens Lehmann

Quoted 5 lines

As Junio mentioned it would be great if you could teach the add command also honor the --ignore-submodule command line option in a companion patch. In the course of doing so you'll easily see if I was right or not, then please just order them in the most logical way.

Quoted 8 lines

Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger,

Quoted 4 lines

Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit.

There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right.

Quoted 8 lines

Quoted 2 lines

Any ideas, what to do about that? When exactly should such submodule be actually ignored?

Me thinks git add should require the '-f' option to add an ignored submodule (just like it does for files) unless the user uses the '--ignore-submodules=none' option. And if neither of these are given it should "fail with a list of ignored files" as the documentation states.

Quoted 5 lines

It's still not clear, at least not to me. Should '-f' suppress the ignore setting of all involved submodules? That would make it a synonyme (or a superset) of --ignore-submodules=none. Or only if the submodule is explicitly named on command line? That seems fuzzy to me, and also more tricky to implement.

Quoted 5 lines

Maybe my impression that doing "add" together with "commit" would be easy wasn't correct after all. I won't object if you try to tackle commit first (but I have the slight suspicion that similar questions will arise concerning the "add"ish functionality in commit too. So maybe after resolving those things might look clearer ;-)

There is one big distinction. My patch for commit doesn't add any new problems. It just adds the --ignore-submodules argument, which is easy to implement and no unclear behavior decisions are needed.

You are right that when specifying ignored submodules on commit's command line, there is the same problem as with git add. However, it's already there anyway. I don't feel in position to solve it, I'd just like to have "git commit --ignore-submodules=none".

With git add however, changing it to honor settings from .gitmodules would change behavior people might be used to, so I would be afraid to do that. Btw add also has the problem already, but only if somebody configures the submodule's ignore setting in .git/config, rather than .gitmodules. I don't know how much real use case that is.

As I see it, there are now these rather easy possibilities (sorted from the easiest):

Quoted 3 lines

Quoted 1 line

1) Just teach commit the --ignore-submodules argument, as I proposed.

1a) Teach commit to honor ignore from .git/config.

But commit already honors that. It honors submodule.<name>.ignore from both .git/config and .gitmodules. It's just add which doesn't honor it from .gitmodules, because cmd_add() function lacks a gitmodules_config() call. Or do I miss something?

Quoted 4 lines

Quoted 2 lines

2) Teach both add and commit to --ignore-submodules, but dont add that problematic gitmodules_config() in add.c.

Why is that problematic after add learned --ignore-submodules=none?

First, because it changes current behaviour. Which is obviously inconsistent currently, however I didn't find it easy to tell what's the right thing to do.

And second, because the "-f implies --ignore-submodules=none" proposal, which seems to be the easy cure for those accustomed to the current behavior, seems non-trivial. Below You wrote that --ignore-submodules=none should be implied by -f only for files specified on the command line. OK. And what if a directory containing the submodule is specified?

Quoted 21 lines

Quoted 9 lines

3) Teach both add and commit to --ignore-submodules, and also let add honor settings from .gitmodules, to make it more consistent with other commands. And, make add --force imply --ignore-submodules=none.

I like both 1) and 2). I don't like 3), the problem of add with submodules' ignore setting is a bug IMHO (ignore=all in .git/config causes strange behavior, while ignore=all in .gitmodules is ignored), but not directly related to the --ignore-submodules param, and should be solved separately.

I think the ignore config options and --ignore-submodules parameter are directly related, as you need the latter to override the former. In the long run commit should honor ignore=all in .git/config for unstaged submodules like add should honor the settings from the .gitmodules file. But we should always add the --ignore-submodules parameter first so that the user can override the configuration when needed. So I see these steps:

1) Teach commit the --ignore-submodules option; then make it honor ignore=all in .git/config in another commit.

Quoted 7 lines

2) Teach add --ignore-submodules (which is implied by -f, but only for the submodules given on the command line); then make it honor the submodule.<name>.ignore option in another commit.

After that we'd have consistent ignore and override behavior. But it looks like getting -f right is not easy, so I'd prefer having
1) without 2) if the alternative is to get neither.

OK, I will try prepare that. However I'd more like to start with two commits just adding the --ignore-submodules param to add and commit. That should be easily acceptable, as there is no risk to break anything, and it adds useful funcionality. Patching add first will avoid having to touch add.c in patch for commit, which makes it more clear and logical IMHO.

Then, on top of that, I'll prepare patches for add to honor ignore from .gitmodules, and -f implying --ignore-submodules. That might need more discussion, let's see.

Am 07.04.2014 23:46, schrieb Ronald Weiss:

Quoted 3 lines

Then, on top of that, I'll prepare patches for add to honor ignore from .gitmodules, and -f implying --ignore-submodules. That might need more discussion, let's see.

Makes sense.

Quoted 6 lines

Am 07.04.2014 23:46, schrieb Ronald Weiss:

Quoted 3 lines

Then, on top of that, I'll prepare patches for add to honor ignore from .gitmodules, and -f implying --ignore-submodules. That might need more discussion, let's see.

Makes sense.

I thought more about that, and also played with the code a bit.

First, I was confused when I wrote that git add doesn't honor submodules' ignore setting only from .gitmodules, but it does from .git/config. It doesn't, from neither. Sorry for the confusion. However, that doesn't change anything on the fact that it would be nice if add would honor the ignore setting, from both places.

Second, there are some differences between adding standard ignored files, and ignored submodules:

1) Already tracked files are never ignored, regardless of .gitignore. However, tracked submodules should be ignored by "add -u", if told so by their ignore setting.

2) So .gitignore seems to only do something when adding new files to the repo. However, when adding new submodules, they are probably never ignored (setting the ignore setting for non existent submodule seems like non-sense, although possible).

3) Ignored files can be ignored less explicitely (in global gitignore, or using a wildcard, or by ignoring parent folder). So it makes sense to warn the user if he tries to explicitely add an ignored file, as he might not be aware that the file is ignored. Submodules, however, can only be ignored explicitely. And when user explicitely specifies the submodule in an add command, he most probably really wants to add it, so I don't see the point in warning him and requiring the -f option.

So, I think that the use cases are completely different, for submodules and ignored files. So trying to make add behave the same for both, might not be that good idea.

I would propose - let's make add honor the ignore setting by just parsing if from config like the other commands do, and pass it to underlying diff invocations. And at the same the, let's override it for submodules explicitely specified on the command line, to never ignore such submodules, without requiring the -f option. That seems to be pretty easy, see below.

builtin/add.c +4 -0

index 85f2110..f19e6c8 100644
@@ -284,6 +284,10 @@ static int add_config(const char *var, const char *value, void *cb)
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
+
+	if (starts_with(var, "submodule."))
+		return parse_submodule_config_option(var, value);
+
 	return git_default_config(var, value, cb);
 }
 

builtin/add.c +1 -0

 	argc = parse_options(argc, argv, prefix, builtin_add_options,
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);

@@ -425,6 +430,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_EXCLUDE);
 
 		for (i = 0; i < pathspec.nr; i++) {
+			int cachepos;
 			const char *path = pathspec.items[i].match;
 			if (pathspec.items[i].magic & PATHSPEC_EXCLUDE)
 				continue;

@@ -440,6 +446,18 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 					die(_("pathspec '%s' did not match any files"),
 					    pathspec.items[i].original);
 			}

builtin/add.c +2 -0

+				free(optname);
+			}
 		}
 		free(seen);
 	}

Signature · 4 lines

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 13.04.2014 01:41, schrieb Ronald Weiss:

Quoted 11 lines

Second, there are some differences between adding standard ignored files, and ignored submodules:

1) Already tracked files are never ignored, regardless of .gitignore. However, tracked submodules should be ignored by "add -u", if told so by their ignore setting.

2) So .gitignore seems to only do something when adding new files to the repo. However, when adding new submodules, they are probably never ignored (setting the ignore setting for non existent submodule seems like non-sense, although possible).

What about "diff.ignoreSubmodules=all"?

Quoted 7 lines

3) Ignored files can be ignored less explicitely (in global gitignore, or using a wildcard, or by ignoring parent folder). So it makes sense to warn the user if he tries to explicitely add an ignored file, as he might not be aware that the file is ignored. Submodules, however, can only be ignored explicitely. And when user explicitely specifies the submodule in an add command, he most probably really wants to add it, so I don't see the point in warning him and requiring the -f option.

But we do have "diff.ignoreSubmodules=all", so I do not agree with your conclusion.

Quoted 10 lines

So, I think that the use cases are completely different, for submodules and ignored files. So trying to make add behave the same for both, might not be that good idea.

I would propose - let's make add honor the ignore setting by just parsing if from config like the other commands do, and pass it to underlying diff invocations. And at the same the, let's override it for submodules explicitely specified on the command line, to never ignore such submodules, without requiring the -f option. That seems to be pretty easy, see below.

What about doing that only when '-f' is given? Would that do what we want?

builtin/add.c int cmd_add(int argc, const char **argv, const char *prefix) · line 327

Quoted 5 lines

builtin/add.c +1 -0

@@ -320,6 +324,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
  	char *seen = NULL;
 
  	git_config(add_config, NULL);
+	gitmodules_config();

Wrong order, gitmodules_config() must be called before git_config() to make the .gitmodules settings overridable by the users config.

Quoted 11 lines

builtin/add.c +7 -0

@@ -440,6 +446,18 @@ int cmd_add(int argc, const char **argv, const char *prefix)
  					die(_("pathspec '%s' did not match any files"),
  					    pathspec.items[i].original);
  			}
+
+			/* disable ignore setting for any submodules specified explicitly in the pathspec */
+			if (path[0] &&
+				(cachepos = cache_name_pos(path, pathspec.items[i].len)) >= 0 &&
+				S_ISGITLINK(active_cache[cachepos]->ce_mode)) {
+				char *optname;
+				int optnamelen = pathspec.items[i].len + 17;

builtin/add.c int cmd_add(int argc, const char **argv, const char *prefix) · line 458

Quoted 3 lines

builtin/add.c +3 -0

+				optname = xcalloc(optnamelen + 1, 1);
+				snprintf(optname, optnamelen + 1, "submodule.%s.ignore", path);
+				parse_submodule_config_option(optname, "none");

We should use "dirty" instead of "none" here. Modifications inside the submodule cannot be added without committing them there first and using "none" might incur an extra preformance penalty for looking through the submodule directories for such changes.

Quoted 2 lines

Changes in add.c and cache.h (and related compilo fix in checkout.c) are needed to make it work for "commit -a" too.

Looking good so far, but we definitely need tests for this new option.

But I wonder if it would make more sense to start by teaching the --ignore-submodules option to git add. Then this patch could reuse that for commit -a.

Submodules explicitly added to index by user should be never hidden in status output.

This also fixes a bug in commit, where submodules with configured ignore setting (in .gitmodules or .git/config), added to index by user, are not displayed in the commit message as being commited, but they still are commited. Unless the changed submodules are the only changes in the index, in such case commit fails immediately with "no changes", which is even worse.

Signed-off-by Ronald Weiss <[email protected]>

---
 wt-status.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

wt-status.c +2 -4

index a452407..108a048 100644
@@ -486,10 +486,8 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
 	setup_revisions(0, NULL, &rev, &opt);
 
-	if (s->ignore_submodule_arg) {
-		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
-		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
-	}
+	DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+	DIFF_OPT_CLR(&rev.diffopt, IGNORE_SUBMODULES);
 
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_collect_updated_cb;