git commit vs. ignore-submodules

git commit vs. ignore-submodules

[Ronald Weiss]

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.

Thanks in advance for any reply,
Ronald Weiss

Re: git commit vs. ignore-submodules

[Jens Lehmann]

Am 28.03.2014 00:36, schrieb Ronald Weiss:
> 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.

Welcome and thanks for the feedback!

> 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.

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.

> 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 ;-).

Re: git commit vs. ignore-submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>> .. 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

That definitely smells like a bug to me.  Excluding modified
submodules when "git add -u" is run with ignore=all would be fine
and most likely the right thing to do, but once the user actually
adds the change to the index, it should not be ignored.

> ...
>> 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 ;-).

Thanks.

Re: git commit vs. ignore-submodules

[Ronald Weiss]

On Fri, Mar 28, 2014 at 5:47 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> 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.

OK, I managed to create a patch, will post it as separate email right
after this one. I also had to touch builtin/add.c, to make the new
--ignore-submodes option work with "git commit -a" too.

>> 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 ;-).

While I was playing with that now, I discovered that the problem is
actually not in commit, but only in status. Commit really commits the
submodule, but it's not visible in the commit message (generated by
status), that confused me before. And if there are no other changes in
the index, then commit fails immediately with "no changes". A small
change in wt-status.c fixes that for me, but I'm not sure about any
undesired side effects. I'll post a patch for that too, in a few
moments.

[PATCH 1/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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 <weiss.ronald@gmail.com>
---
 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(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..c8839c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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
diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..1086294 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..22a4b48 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 26b2986..121c185 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/cache.h b/cache.h
index ebe9a40..5ef8dd6 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
-- 
1.9.0
 

Re: [PATCH 1/2] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 29.03.2014 23:50, schrieb Ronald Weiss:
> 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.

Thanks, looking good from first glance, will look deeper into that
tomorrow.

> Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
> ---
>  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(-)
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 1a7616c..c8839c8 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -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
> diff --git a/builtin/add.c b/builtin/add.c
> index 672adc0..1086294 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -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)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index ada51fa..22a4b48 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 26b2986..121c185 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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);
> diff --git a/cache.h b/cache.h
> index ebe9a40..5ef8dd6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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;
> 

Re: [PATCH 1/2] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 29.03.2014 23:50, schrieb Ronald Weiss:
> 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.

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.

> Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
> ---
>  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(-)
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 1a7616c..c8839c8 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -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
> diff --git a/builtin/add.c b/builtin/add.c
> index 672adc0..1086294 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -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)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index ada51fa..22a4b48 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 26b2986..121c185 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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);
> diff --git a/cache.h b/cache.h
> index ebe9a40..5ef8dd6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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;
> 

[PATCH v2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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 <weiss.ronald@gmail.com>
---
On 30. 3. 2014 21:48, Jens Lehmann wrote:
> 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.

> 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

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..c8839c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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
diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..1086294 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..22a4b48 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 26b2986..b3fb28e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/cache.h b/cache.h
index ebe9a40..5ef8dd6 100644
--- a/cache.h
+++ b/cache.h
@@ -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 --git a/diff-lib.c b/diff-lib.c
index 2eddc66..fec5ad4 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -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 --git a/diff.h b/diff.h
index ce123fa..0ac869b 100644
--- a/diff.h
+++ b/diff.h
@@ -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,
diff --git a/sequencer.c b/sequencer.c
index 90cac7b..de9edec 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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);
diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
new file mode 100644
index 0000000..a3a68ce
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -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 -m "Update sm" &&
+	git diff-index --exit-code --cached --ignore-submodules=none HEAD &&
+	git diff-files --exit-code --ignore-submodules=none
+'
+
+test_done
-- 
1.9.0
 

[PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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 <weiss.ronald@gmail.com>
---
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.

On 30. 3. 2014 21:48, Jens Lehmann wrote:
> 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.

> 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

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..c8839c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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
diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..1086294 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..22a4b48 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 26b2986..b3fb28e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/cache.h b/cache.h
index ebe9a40..5ef8dd6 100644
--- a/cache.h
+++ b/cache.h
@@ -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 --git a/diff-lib.c b/diff-lib.c
index 2eddc66..fec5ad4 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -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 --git a/diff.h b/diff.h
index ce123fa..0ac869b 100644
--- a/diff.h
+++ b/diff.h
@@ -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,
diff --git a/sequencer.c b/sequencer.c
index 90cac7b..de9edec 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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);
diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
new file mode 100644
index 0000000..a7148ce
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -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
-- 
1.9.0
 

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 31.03.2014 02:07, schrieb Ronald Weiss:
> 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 <weiss.ronald@gmail.com>
> ---
> 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.
> 
> On 30. 3. 2014 21:48, Jens Lehmann wrote:
>> 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.

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.

>> 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.

You might be right as I didn't really check the actual codepaths. I
was deducing this from the observation that most changes were made
to builtin/add.c. And seeing a function like add_files_to_cache()
being modified made me expect that plain add would use the same
function to stage the files.

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.

Thanks for digging into this!

>  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
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 1a7616c..c8839c8 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -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
> diff --git a/builtin/add.c b/builtin/add.c
> index 672adc0..1086294 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -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)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index ada51fa..22a4b48 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 26b2986..b3fb28e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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);
> diff --git a/cache.h b/cache.h
> index ebe9a40..5ef8dd6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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 --git a/diff-lib.c b/diff-lib.c
> index 2eddc66..fec5ad4 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -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 --git a/diff.h b/diff.h
> index ce123fa..0ac869b 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -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,
> diff --git a/sequencer.c b/sequencer.c
> index 90cac7b..de9edec 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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);
> diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
> new file mode 100644
> index 0000000..a7148ce
> --- /dev/null
> +++ b/t/t7513-commit-ignore-submodules.sh
> @@ -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
> 

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 31.03.2014 20:58, schrieb Jens Lehmann:
> Am 31.03.2014 02:07, schrieb Ronald Weiss:
>> 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.

I've been hacking on that some more (I'll post it as soon as I
have all new tests up and running) and think we might be able to
handle that even easier. Please see below:

>> diff --git a/diff-lib.c b/diff-lib.c
>> index 2eddc66..fec5ad4 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -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);

You'll have to do this unconditionally, as this option tells the
diff machinery to ignore any submodule configurations, which is
what we want in either case. But ...

>> +		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:

+		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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On Mon, Mar 31, 2014 at 10:37 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> ... maybe the best way is to leave index_differs_from() unchanged
> and call that function with the correct diff_flags instead:
>
> +               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.

Thanks for this hint, that is really much nicer. I'll test that, and
post updated patch if it works.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> 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,
and nobody actually complains about lack of submodule ignoring
facility in git add, while I know that the current behavior of commit
really causes trouble.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 31. 3. 2014 23:47, Ronald Weiss wrote:
> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> 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,

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.


diff --git a/builtin/add.c b/builtin/add.c
index 1086294..9f70327 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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)

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 1. 4. 2014 0:50, Ronald Weiss wrote:
> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> 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,
>
> 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?

>
> 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.
>
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 1086294..9f70327 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -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)
>

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 01.04.2014 01:35, schrieb Ronald Weiss:
> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>> 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,
>>
>> 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?

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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 1. 4. 2014 22:23, Jens Lehmann wrote:
> Am 01.04.2014 01:35, schrieb Ronald Weiss:
>> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>>> 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,
>>>
>>> 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?
> 
> 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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 01.04.2014 23:59, schrieb Ronald Weiss:
> On 1. 4. 2014 22:23, Jens Lehmann wrote:
>> Am 01.04.2014 01:35, schrieb Ronald Weiss:
>>> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>>>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>>>> 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,
>>>>
>>>> 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?
>>
>> 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.

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 ;-)

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 2. 4. 2014 20:53, Jens Lehmann wrote:
> Am 01.04.2014 23:59, schrieb Ronald Weiss:
>> On 1. 4. 2014 22:23, Jens Lehmann wrote:
>>> Am 01.04.2014 01:35, schrieb Ronald Weiss:
>>>> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>>>>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>>>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>>>>> 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,
>>>>>
>>>>> 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?
>>>
>>> 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.
> 
> 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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 02.04.2014 21:56, schrieb Ronald Weiss:
> On 2. 4. 2014 20:53, Jens Lehmann wrote:
>> Am 01.04.2014 23:59, schrieb Ronald Weiss:
>>> On 1. 4. 2014 22:23, Jens Lehmann wrote:
>>>> Am 01.04.2014 01:35, schrieb Ronald Weiss:
>>>>> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>>>>>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>>>>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>>>>>> 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,
>>>>>>
>>>>>> 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?
>>>>
>>>> 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.
>>
>> 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.

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

> 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?

> 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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 6. 4. 2014 18:28, Jens Lehmann wrote:
> Am 02.04.2014 21:56, schrieb Ronald Weiss:
>> On 2. 4. 2014 20:53, Jens Lehmann wrote:
>>> Am 01.04.2014 23:59, schrieb Ronald Weiss:
>>>> On 1. 4. 2014 22:23, Jens Lehmann wrote:
>>>>> Am 01.04.2014 01:35, schrieb Ronald Weiss:
>>>>>> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>>>>>>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>>>>>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>>>>>>> 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,
>>>>>>>
>>>>>>> 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?
>>>>>
>>>>> 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.
>>>
>>> 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.
> 
> 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?

>> 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?

>> 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.

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.

[PATCH v3 1/2] add: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

Allow overriding the ignore setting from config, using the command line
parameter like diff and status have. Git add currently doesn't honor
ignore from .gitmodules, but it does honor it from .git/config. And
support for .gitmodules will come in another patch.

Useful <when> values are 'none' and 'all' (the default), the other values
('dirty' and 'untracked') being equivalent to 'none' for add's purposes.

Also add ignore_submodules_arg parameter to function add_files_to_cache,
which will allow implementing --ignore-submodules also for other commands
using this function (for commit command, in particular, coming in
subsequent commit). This requires compilo fixes in checkout.c and commit.c

Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
---
I have changed order of commits, from what Jens proposed, to avoid the patch
for commit (coming right after this one) having to mess too much with add's
source code.

If anyone disagrees, let me know, and I will redo it as needed.

I'll look in to the related "add [-f] vs .gitmodules:ignore=all" problem
soon.

 Documentation/git-add.txt        |  7 ++++++-
 builtin/add.c                    | 21 +++++++++++++++----
 builtin/checkout.c               |  2 +-
 builtin/commit.c                 |  2 +-
 cache.h                          |  2 +-
 t/t3704-add-ignore-submodules.sh | 45 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 71 insertions(+), 8 deletions(-)
 create mode 100644 t/t3704-add-ignore-submodules.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 48754cb..be2e7b5 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -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
 -----------
@@ -160,6 +160,11 @@ today's "git add <pathspec>...", ignoring removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--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.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..72ef792 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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;
 }
 
@@ -342,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 */
@@ -367,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(),
 };
 
@@ -576,7 +589,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, ignore_submodule_arg);
 
 	exit_status |= !!update_data.add_errors;
 	if (add_new_files)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..22a4b48 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 26b2986..0db215b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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, NULL);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||
diff --git a/cache.h b/cache.h
index ebe9a40..5ef8dd6 100644
--- a/cache.h
+++ b/cache.h
@@ -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 --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
new file mode 100644
index 0000000..db58f0c
--- /dev/null
+++ b/t/t3704-add-ignore-submodules.sh
@@ -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
-- 
1.9.1.3.gef38fe4
 

[PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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 <weiss.ronald@gmail.com>
---
 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

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..8d3b2db 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 0db215b..121c185 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
new file mode 100644
index 0000000..83ce04c
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -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
-- 
1.9.1.3.gef38fe4
 

Re: [PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 08.04.2014 01:03, schrieb Ronald Weiss:
> 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.

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.

> 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 <weiss.ronald@gmail.com>
> ---
>  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
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 1a7616c..8d3b2db 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -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
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0db215b..121c185 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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);
> diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
> new file mode 100644
> index 0000000..83ce04c
> --- /dev/null
> +++ b/t/t7513-commit-ignore-submodules.sh
> @@ -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
> 

Re: [PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 8. 4. 2014 20:43, Jens Lehmann wrote:
> Am 08.04.2014 01:03, schrieb Ronald Weiss:
>> 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.
> 
> 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.

Hm, You mean the status message, which is pre-inserted as comment into
the commit message, when opening an editor to write the commit message?

OK, that really makes a difference, although really small and actually
affecting nothing. I'll take it into account. But are You sure the tests
for this would be actually useful? If only effect of them would be
increasing time needed to run the full test suite, then it's better to
not have them :-). But I can do that, if You still think it's useful.

Re: [PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 8. 4. 2014 20:43, Jens Lehmann wrote:
>> 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?

[PATCH v4 1/2] add: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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

Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
---
 Documentation/git-add.txt        |  7 ++++++-
 builtin/add.c                    | 16 ++++++++++++--
 builtin/checkout.c               |  2 +-
 builtin/commit.c                 |  2 +-
 cache.h                          |  2 +-
 t/t3704-add-ignore-submodules.sh | 45 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 68 insertions(+), 6 deletions(-)
 create mode 100644 t/t3704-add-ignore-submodules.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9631526..b2c936f 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -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
 -----------
@@ -164,6 +164,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--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.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
diff --git a/builtin/add.c b/builtin/add.c
index 459208a..85f2110 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -83,7 +83,8 @@ static void update_callback(struct diff_queue_struct *q,
 }
 
 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;
 	struct rev_info rev;
@@ -99,6 +100,12 @@ int add_files_to_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);
 	return !!data.add_errors;
 }
@@ -237,6 +244,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 */
@@ -262,6 +271,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(),
 };
 
@@ -434,7 +446,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags, ignore_submodule_arg);
 
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..607af47 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..a148e28 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,7 +361,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, NULL);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||
diff --git a/cache.h b/cache.h
index 107ac61..a6cedc0 100644
--- a/cache.h
+++ b/cache.h
@@ -1370,7 +1370,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 --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
new file mode 100644
index 0000000..db58f0c
--- /dev/null
+++ b/t/t3704-add-ignore-submodules.sh
@@ -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
-- 
1.9.1.3.g7790cde.dirty
 

Re: [PATCH v4 1/2] add: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 13.04.2014 00:45, schrieb Ronald Weiss:
> 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.

> 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.

> That's why signature
> of function add_files_to_cache was changed.

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

> 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.

> Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
> ---
>  Documentation/git-add.txt        |  7 ++++++-
>  builtin/add.c                    | 16 ++++++++++++--
>  builtin/checkout.c               |  2 +-
>  builtin/commit.c                 |  2 +-
>  cache.h                          |  2 +-
>  t/t3704-add-ignore-submodules.sh | 45 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 68 insertions(+), 6 deletions(-)
>  create mode 100644 t/t3704-add-ignore-submodules.sh
> 
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 9631526..b2c936f 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -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
>  -----------
> @@ -164,6 +164,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
>  	be ignored, no matter if they are already present in the work
>  	tree or not.
>  
> +--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.
> +
>  \--::
>  	This option can be used to separate command-line options from
>  	the list of files, (useful when filenames might be mistaken
> diff --git a/builtin/add.c b/builtin/add.c
> index 459208a..85f2110 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -83,7 +83,8 @@ static void update_callback(struct diff_queue_struct *q,
>  }
>  
>  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;
>  	struct rev_info rev;
> @@ -99,6 +100,12 @@ int add_files_to_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);
>  	return !!data.add_errors;
>  }
> @@ -237,6 +244,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 */
> @@ -262,6 +271,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(),
>  };
>  
> @@ -434,7 +446,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	plug_bulk_checkin();
>  
> -	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
> +	exit_status |= add_files_to_cache(prefix, &pathspec, flags, ignore_submodule_arg);
>  
>  	if (add_new_files)
>  		exit_status |= add_files(&dir, flags);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 07cf555..607af47 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -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
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..a148e28 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -361,7 +361,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, NULL);
>  		refresh_cache_or_die(refresh_flags);
>  		update_main_cache_tree(WRITE_TREE_SILENT);
>  		if (write_cache(fd, active_cache, active_nr) ||
> diff --git a/cache.h b/cache.h
> index 107ac61..a6cedc0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1370,7 +1370,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 --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
> new file mode 100644

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

> index 0000000..db58f0c
> --- /dev/null
> +++ b/t/t3704-add-ignore-submodules.sh
> @@ -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
> 

Re: [PATCH v4 1/2] add: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 18. 4. 2014 13:53, Jens Lehmann wrote:
> Am 13.04.2014 00:45, schrieb Ronald Weiss:
>> 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.
> 
>> 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.
> 
>> That's why signature
>> of function add_files_to_cache was changed.
> 
> But that's necessary for this patch too, no?

No, for this patch alone, we could just use the global variable, which
is set up by parse_options(), without changing the public function and
breaking compilation in other files.

> 
>> 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.

OK, I'll update the commit message, and fix the file mode for the added
test script. Will repost once we sort out the problems (failing tests)
You have with the second patch (for commit).

>> diff --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
>> new file mode 100644
> 
> The flags should be 100755 here, currently "make test" fails because
> of this.

I'm sorry for this, I was testing it on Windows, where the file mode
doesn't matter, that's why I missed it.

[PATCH v4 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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.

Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
---
 Documentation/git-commit.txt        |  7 ++++
 builtin/commit.c                    |  8 +++-
 t/t7513-commit-ignore-submodules.sh | 78 +++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 t/t7513-commit-ignore-submodules.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..de0e8fe 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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[<key-id>]] [--] [<file>...]
 
 DESCRIPTION
@@ -277,6 +278,12 @@ 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", "dirty, "untracked" or "all", which
+	is the default.
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what
diff --git a/builtin/commit.c b/builtin/commit.c
index a148e28..8c4d05e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,7 +361,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) ||
@@ -1526,6 +1526,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,
@@ -1564,6 +1567,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);
diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
new file mode 100644
index 0000000..52ab37d
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -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
-- 
1.9.1.3.g7790cde.dirty
 

Re: [PATCH v4 2/2] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 13.04.2014 00:49, schrieb Ronald Weiss:
> 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?

> Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
> ---
>  Documentation/git-commit.txt        |  7 ++++
>  builtin/commit.c                    |  8 +++-
>  t/t7513-commit-ignore-submodules.sh | 78 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 1 deletion(-)
>  create mode 100644 t/t7513-commit-ignore-submodules.sh
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 0bbc8f5..de0e8fe 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -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[<key-id>]] [--] [<file>...]
>  
>  DESCRIPTION
> @@ -277,6 +278,12 @@ 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", "dirty, "untracked" or "all", which
> +	is the default.
> +
>  -v::
>  --verbose::
>  	Show unified diff between the HEAD commit and what
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a148e28..8c4d05e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -361,7 +361,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) ||
> @@ -1526,6 +1526,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,
> @@ -1564,6 +1567,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);
> diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
> new file mode 100644

Flags: 100755

> index 0000000..52ab37d
> --- /dev/null
> +++ b/t/t7513-commit-ignore-submodules.sh
> @@ -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
> 

Re: [PATCH v4 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 18. 4. 2014 14:09, Jens Lehmann wrote:
> Am 13.04.2014 00:49, schrieb Ronald Weiss:
>> 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?

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.

Re: [PATCH v4 2/2] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 22.04.2014 00:08, schrieb Ronald Weiss:
> On 18. 4. 2014 14:09, Jens Lehmann wrote:
>> Am 13.04.2014 00:49, schrieb Ronald Weiss:
>>> 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?
> 
> 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.

I'd rather have them fail because nothing is to be committed (and not
because the commit message is empty) and document we expect that to
happen by using test_must_fail (and that for example will catch a later
change which accidentally makes commit create empty commits here).

> 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?

Silly me, I forgot to do that (even though you explicitly mention
this dependency in the commit message). Sorry for the noise, all
tests run fine after rebasing your changes on top of mine.

> 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.
> 

[PATCH v5 1/2] add: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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 <weiss.ronald@gmail.com>
---
Git add currently doesn't honor ignore setting from .gitmodules or
.git/config, which is related functionality, however I'd like to
change that in another patch, coming soon.

Changes against v4 of this patch:
* fixed file mode of added test script (644 -> 755)
* cleaned up commit message

 Documentation/git-add.txt        |  7 ++++++-
 builtin/add.c                    | 16 ++++++++++++--
 builtin/checkout.c               |  2 +-
 builtin/commit.c                 |  2 +-
 cache.h                          |  2 +-
 t/t3704-add-ignore-submodules.sh | 45 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 68 insertions(+), 6 deletions(-)
 create mode 100755 t/t3704-add-ignore-submodules.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9631526..b2c936f 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -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
 -----------
@@ -164,6 +164,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--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.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
diff --git a/builtin/add.c b/builtin/add.c
index 459208a..85f2110 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -83,7 +83,8 @@ static void update_callback(struct diff_queue_struct *q,
 }
 
 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;
 	struct rev_info rev;
@@ -99,6 +100,12 @@ int add_files_to_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);
 	return !!data.add_errors;
 }
@@ -237,6 +244,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 */
@@ -262,6 +271,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(),
 };
 
@@ -434,7 +446,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags, ignore_submodule_arg);
 
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..607af47 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..a148e28 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,7 +361,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, NULL);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||
diff --git a/cache.h b/cache.h
index 107ac61..a6cedc0 100644
--- a/cache.h
+++ b/cache.h
@@ -1370,7 +1370,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 --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
new file mode 100755
index 0000000..db58f0c
--- /dev/null
+++ b/t/t3704-add-ignore-submodules.sh
@@ -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
-- 
1.9.1.3.g7790cde.dirty 

Re: [PATCH v5 1/2] add: add --ignore-submodules[=<when>] parameter

[Eric Sunshine]

On Tue, Apr 22, 2014 at 5:12 PM, Ronald Weiss <weiss.ronald@gmail.com> wrote:
> 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

s/it's/its/

> call sites are updated to pass NULL.
>
> Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
> ---
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 9631526..b2c936f 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -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
>  -----------
> @@ -164,6 +164,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
>         be ignored, no matter if they are already present in the work
>         tree or not.
>
> +--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.

The "which is the default" clause reads ambiguously. It's not clear
whether you mean "none" is the default or "all" is the default.

>  \--::
>         This option can be used to separate command-line options from
>         the list of files, (useful when filenames might be mistaken
> diff --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
> new file mode 100755
> index 0000000..db58f0c
> --- /dev/null
> +++ b/t/t3704-add-ignore-submodules.sh
> @@ -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 && (

It's conventional for the opening '(' to be placed on a line by itself.

> +               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 &&

s/>> />>/

> +               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

[PATCH v6 1/2] add: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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 its
call sites are updated to pass NULL.

Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
---
Git add currently doesn't honor ignore setting from .gitmodules or
.git/config, which is related functionality, however I'd like to
change that in another patch, coming soon.

Patch changelog:
v6
* corrected wording and formatting errors (as pointed out by Eric Sunshine)
v5
* fixed file mode of added test script (644 -> 755)
* cleaned up commit message

 Documentation/git-add.txt        |  7 +++++-
 builtin/add.c                    | 16 ++++++++++++--
 builtin/checkout.c               |  2 +-
 builtin/commit.c                 |  2 +-
 cache.h                          |  2 +-
 t/t3704-add-ignore-submodules.sh | 47 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 70 insertions(+), 6 deletions(-)
 create mode 100755 t/t3704-add-ignore-submodules.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9631526..ef69f8b 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -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
 -----------
@@ -164,6 +164,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--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", defaulting to "all".
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
diff --git a/builtin/add.c b/builtin/add.c
index 459208a..85f2110 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -83,7 +83,8 @@ static void update_callback(struct diff_queue_struct *q,
 }
 
 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;
 	struct rev_info rev;
@@ -99,6 +100,12 @@ int add_files_to_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);
 	return !!data.add_errors;
 }
@@ -237,6 +244,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 */
@@ -262,6 +271,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(),
 };
 
@@ -434,7 +446,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags, ignore_submodule_arg);
 
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..607af47 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/commit.c b/builtin/commit.c
index 0ad4d0b..5444111 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,7 +361,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, NULL);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||
diff --git a/cache.h b/cache.h
index 107ac61..a6cedc0 100644
--- a/cache.h
+++ b/cache.h
@@ -1370,7 +1370,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 --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
new file mode 100755
index 0000000..a6d8517
--- /dev/null
+++ b/t/t3704-add-ignore-submodules.sh
@@ -0,0 +1,47 @@
+#!/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
-- 
2.0.0.rc0.3.gd50de04 

[PATCH v6 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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.

Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
---
Patch changelog:
v6
* corrected wording and formatting errors (as pointed out by Eric Sunshine)
v5
* fixed file mode of added test script (644 -> 755)
* replaced test_might_fail with test_must_fail in test script

 Documentation/git-commit.txt        |  7 ++++
 builtin/commit.c                    |  8 +++-
 t/t7513-commit-ignore-submodules.sh | 80 +++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 1 deletion(-)
 create mode 100755 t/t7513-commit-ignore-submodules.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..55995be 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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[<key-id>]] [--] [<file>...]
 
 DESCRIPTION
@@ -277,6 +278,12 @@ 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", "dirty, "untracked" or "all",
+	defaulting to "all".
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what
diff --git a/builtin/commit.c b/builtin/commit.c
index 5444111..dc1d8d0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,7 +361,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) ||
@@ -1540,6 +1540,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,
@@ -1578,6 +1581,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);
diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
new file mode 100755
index 0000000..10ae178
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -0,0 +1,80 @@
+#!/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_must_fail git commit --ignore-submodules=none >output &&
+	test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=untracked >output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=dirty >output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_must_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_must_fail git commit --ignore-submodules=none >output &&
+	test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=untracked >output &&
+	test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=dirty >output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=all >output &&
+	test_must_fail test_i18ngrep modified output
+'
+
+test_done
-- 
2.0.0.rc0.3.gd50de04

[PATCH v5 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

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.

Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
---
Changes against v4 of this patch:
* fixed file mode of added test script (644 -> 755)
* replaced test_might_fail with test_must_fail in test script

 Documentation/git-commit.txt        |  7 ++++
 builtin/commit.c                    |  8 +++-
 t/t7513-commit-ignore-submodules.sh | 78 +++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100755 t/t7513-commit-ignore-submodules.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..de0e8fe 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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[<key-id>]] [--] [<file>...]
 
 DESCRIPTION
@@ -277,6 +278,12 @@ 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", "dirty, "untracked" or "all", which
+	is the default.
+
 -v::
 --verbose::
 	Show unified diff between the HEAD commit and what
diff --git a/builtin/commit.c b/builtin/commit.c
index a148e28..8c4d05e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,7 +361,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) ||
@@ -1526,6 +1526,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,
@@ -1564,6 +1567,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);
diff --git a/t/t7513-commit-ignore-submodules.sh b/t/t7513-commit-ignore-submodules.sh
new file mode 100755
index 0000000..52ab37d
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -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_must_fail git commit --ignore-submodules=none > output &&
+	test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=untracked > output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=dirty > output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_must_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_must_fail git commit --ignore-submodules=none > output &&
+	test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=untracked > output &&
+	test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=dirty > output &&
+	test_must_fail test_i18ngrep modified output &&
+
+	test_must_fail git commit --ignore-submodules=all > output &&
+	test_must_fail test_i18ngrep modified output
+'
+
+test_done
-- 
1.9.1.3.g7790cde.dirty 

Re: [PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Junio C Hamano]

Ronald Weiss <weiss.ronald@gmail.com> writes:

> On 8. 4. 2014 20:43, Jens Lehmann wrote:
>>> 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?

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".

Thanks.

Re: [PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 14. 4. 2014 20:30, Junio C Hamano wrote:
> Ronald Weiss <weiss.ronald@gmail.com> writes:
> 
>> On 8. 4. 2014 20:43, Jens Lehmann wrote:
>>>> 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?
> 
> 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.

> 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.

Re: [PATCH v3 2/2] commit: add --ignore-submodules[=<when>] parameter

[Junio C Hamano]

Ronald Weiss <weiss.ronald@gmail.com> writes:

> On 14. 4. 2014 20:30, Junio C Hamano wrote:
>> Ronald Weiss <weiss.ronald@gmail.com> writes:
>> 
>>> On 8. 4. 2014 20:43, Jens Lehmann wrote:
>>>>> 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?
>> 
>> 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.

No, that wasn't harsh at all.  I just did not want to get a patch
with Acked-by with somebody's name on it, when there is not yet an
Ack, as that will confuse me greatly.  My mental bandwidth is not
wide enough to keep track of all the in-flight topics I haven't yet
picked up.

I should be the one to say sorry if my message sounded harsh.

Thanks.

>> 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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 07.04.2014 23:46, schrieb Ronald Weiss:
> On 6. 4. 2014 18:28, Jens Lehmann wrote:
>> Am 02.04.2014 21:56, schrieb Ronald Weiss:
>>> On 2. 4. 2014 20:53, Jens Lehmann wrote:
>>>> Am 01.04.2014 23:59, schrieb Ronald Weiss:
>>>>> On 1. 4. 2014 22:23, Jens Lehmann wrote:
>>>>>> Am 01.04.2014 01:35, schrieb Ronald Weiss:
>>>>>>> On 1. 4. 2014 0:50, Ronald Weiss wrote:
>>>>>>>> On 31. 3. 2014 23:47, Ronald Weiss wrote:
>>>>>>>>> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>>>>>>>> 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,
>>>>>>>>
>>>>>>>> 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?
>>>>>>
>>>>>> 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.
>>>>
>>>> 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.
>>
>> 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?

No, I missed that, so please forget my comment.

>>> 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.

I believe we should be consistent here, but the overriding of that
option is the tricky part. So we need to solve that first before we
can add gitmodules_config() to add.c.

> 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?

That should behave just like an ignored file is contained in that
directory me thinks. But I agree this is non trivial.

>>> 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.
> 
> 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.

Good to hear that!

> 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.

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 8. 4. 2014 20:26, Jens Lehmann wrote:
> Am 07.04.2014 23:46, schrieb Ronald Weiss:
>> 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.


diff --git a/builtin/add.c b/builtin/add.c
index 85f2110..f19e6c8 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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);
 }
 
@@ -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();
 
 	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);
 			}
+
+			/* 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;
+				optname = xcalloc(optnamelen + 1, 1);
+				snprintf(optname, optnamelen + 1, "submodule.%s.ignore", path);
+				parse_submodule_config_option(optname, "none");
+				free(optname);
+			}
 		}
 		free(seen);
 	}
--  

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Jens Lehmann]

Am 13.04.2014 01:41, schrieb Ronald Weiss:
> On 8. 4. 2014 20:26, Jens Lehmann wrote:
>> Am 07.04.2014 23:46, schrieb Ronald Weiss:
>>> 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.

Ok, thanks for digging deeper here.

> 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"?

> 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.

> 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?

> diff --git a/builtin/add.c b/builtin/add.c
> index 85f2110..f19e6c8 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -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);
>  }
>  
> @@ -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.

>  	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);
>  			}
> +
> +			/* 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;
> +				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.

> +				free(optname);
> +			}
>  		}
>  		free(seen);
>  	}
> --  
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2.1] commit: add --ignore-submodules[=<when>] parameter

[Ronald Weiss]

On 18. 4. 2014 14:28, Jens Lehmann wrote:
> Am 13.04.2014 01:41, schrieb Ronald Weiss:
>> 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"?
> 
>> 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.

Ah, I didn't know about diff.ignoreSubmodules. I agree that it defeats
two of my points :-(.

And how about the point 1), should submodules never be ignored once
already tracked, like standard ignored files? I'm almost sure that in
this case the behavior should be different, otherwise it would drop
the most useful use case of the proposed change.

>> 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?

OK then, seems right with diff.ignoreSubmodules in mind.

But one question stays - what to do with submodules not explicitly
specified on command line (when their parent folder is being added)?
You adviced already to do what we do with standard ignored files. That
seems to be:

A) if the file is already tracked, add it
B) if not tracked, without -f, silently ignore it
C) if not tracked, with -f, add it

A) is same like point 1 above, let's forget it until we settle that one.
But B) is even more problematic, we would probably have to modify the
directory parsing code in dir.c to handle ignored submodules. Is that
what we want? If so, then I'm afraid this is getting overly complicated.

>> @@ -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.

Right. I noticed that too just after sending the patch.

>> @@ -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;
>> +				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.

OK, that's right too.

Re: [PATCH 1/2] commit: add --ignore-submodules[=<when>] parameter

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>> 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.

I am not sure about the code reuse, but from the usability and
consistency point of view, they must go hand in hand, I would
think.

[PATCH 2/2] status: don't ignore submodules added to index

[Ronald Weiss]

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 <weiss.ronald@gmail.com>
---
 wt-status.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index a452407..108a048 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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;
-- 
1.9.0
 

Re: [PATCH 2/2] status: don't ignore submodules added to index

[Jens Lehmann]

Am 29.03.2014 23:56, schrieb Ronald Weiss:
> 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.

Thanks, but I think this patch falls a bit short (I assume you should see
test failures with this patch). I'm currently working on fixing that, will
post that as soon as I finished it.

> Signed-off-by: Ronald Weiss <weiss.ronald@gmail.com>
> ---
>  wt-status.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index a452407..108a048 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -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;
> 

Re: [PATCH 2/2] status: don't ignore submodules added to index

[Ronald Weiss]

On 30. 3. 2014 0:16, Jens Lehmann wrote:
> Thanks, but I think this patch falls a bit short (I assume you should see
> test failures with this patch). I'm currently working on fixing that, will
> post that as soon as I finished it.

You're right, 3 tests from t7508 failed with that, I'm sorry for not
verifying that :-(.

That change was really too aggresive, the one below seems better, all
tests pass with it, and it still works. But I'm not sending it as patch
anymore, knowing that You are already working on better solution.

diff --git a/wt-status.c b/wt-status.c
index a452407..520835e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -489,4 +489,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
        if (s->ignore_submodule_arg) {
                DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
                handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
+       } else {
+               DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+               DIFF_OPT_CLR(&rev.diffopt, IGNORE_SUBMODULES);
        }

Re: [PATCH 2/2] status: don't ignore submodules added to index

[Ronald Weiss]

On 30. 3. 2014 0:40, Ronald Weiss wrote:
> That change was really too aggresive, the one below seems better, all
> tests pass with it, and it still works.

Oops, some tests still fail, sorry for my blindness. Nevermind, I'm 
looking forward to Your fix.

[WIP/PATCH] status/commit: always show staged submodules regardless of ignore config

[Jens Lehmann]

Currently setting submodule.<name>.ignore and/or diff.ignoreSubmodules to
"all" suppresses all output of submodule changes for the diff family,
status and commit. For status and commit this is really confusing, as it
even when the user chooses to record a new commit for an ignored submodule
by adding it manually this change won't show up under the to-be-committed
changes. To add insult to injury, a later "git commit" will error out with
"nothing to commit" when only ignored submodules are staged.

Fix that by making wt_status always print staged submodule changes, no
matter what ignore settings are configured. The only exception is when the
user explicitly uses the "--ignore-submodules=all" command line option, in
that case the submodule output is still suppressed. This also makes "git
commit" work again when only modifications of ignored submodules are
staged, as that command uses the "commitable" member of the wt_status
struct to determine if staged changes are present.

Change t7508 to reflect this new behavior. Also mention it in the
documentation of the ignore config options.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---


Am 28.03.2014 17:47, schrieb Jens Lehmann:
> Am 28.03.2014 00:36, schrieb Ronald Weiss:
>> 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 ;-).

Ok, here we go! As we discussed the same issue last November I
added the people involved back then to the CC.

I still need to fix an issue with this patch: committing a single
ignored submodule sometimes exits with an error code (even though
the commit succeeds), I'm still looking into that and will provide
a test for commit too.

After this we need at least two other patches:

- git gui should show staged ignored submodules

- gitk should show staged ignored submodules

(It'd be great if someone else would tackle these two, just let me
know if you want to to avoid duplicated work)

Did I forget a git command that shows what is to be staged?


 Documentation/config.txt     |  8 ++++++--
 Documentation/gitmodules.txt |  4 +++-
 t/t7508-status.sh            | 29 +++++++++++++++++++++++++++--
 wt-status.c                  | 12 +++++++++++-
 4 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 73c8973..0a2e9ad 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2279,7 +2279,9 @@ status.submodulesummary::
 	--summary-limit option of linkgit:git-submodule[1]). Please note
 	that the summary output command will be suppressed for all
 	submodules when `diff.ignoreSubmodules` is set to 'all' or only
-	for those submodules where `submodule.<name>.ignore=all`. To
+	for those submodules where `submodule.<name>.ignore=all`. The only
+	exception to that rule is that status and commit will always show
+	submodule changes that have been staged. To
 	also view the summary for ignored submodules you can either use
 	the --ignore-submodules=dirty command line option or the 'git
 	submodule summary' command, which shows a similar output but does
@@ -2310,7 +2312,9 @@ submodule.<name>.fetchRecurseSubmodules::
 submodule.<name>.ignore::
 	Defines under what circumstances "git status" and the diff family show
 	a submodule as modified. When set to "all", it will never be considered
-	modified, "dirty" will ignore all changes to the submodules work tree and
+	modified (but will nonetheless show up in the output of status and
++	commit when they have been staged), "dirty" will ignore all changes
+	to the submodules work tree and
 	takes only differences between the HEAD of the submodule and the commit
 	recorded in the superproject into account. "untracked" will additionally
 	let submodules with modified tracked files in their work tree show up.
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index f539e3f..f684963 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -71,7 +71,9 @@ submodule.<name>.fetchRecurseSubmodules::
 submodule.<name>.ignore::
 	Defines under what circumstances "git status" and the diff family show
 	a submodule as modified. When set to "all", it will never be considered
-	modified, "dirty" will ignore all changes to the submodules work tree and
+	modified (but will nonetheless show up in the output of status and
+	commit when they have been staged), "dirty" will ignore all changes
+	to the submodules work tree and
 	takes only differences between the HEAD of the submodule and the commit
 	recorded in the superproject into account. "untracked" will additionally
 	let submodules with modified tracked files in their work tree show up.
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..f2b89e8 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1380,7 +1380,32 @@ EOF
 	test_i18ncmp expect output
 '

-test_expect_success '.gitmodules ignore=all suppresses submodule summary' '
+test_expect_success '.gitmodules ignore=all suppresses unstaged submodule summary' '
+	cat > expect << EOF &&
+On branch master
+Changes to be committed:
+  (use "git reset HEAD <file>..." to unstage)
+
+	modified:   sm
+
+Changes not staged for commit:
+  (use "git add <file>..." to update what will be committed)
+  (use "git checkout -- <file>..." to discard changes in working directory)
+
+	modified:   dir1/modified
+
+Untracked files:
+  (use "git add <file>..." to include in what will be committed)
+
+	.gitmodules
+	dir1/untracked
+	dir2/modified
+	dir2/untracked
+	expect
+	output
+	untracked
+
+EOF
 	git config --add -f .gitmodules submodule.subname.ignore all &&
 	git config --add -f .gitmodules submodule.subname.path sm &&
 	git status > output &&
@@ -1388,7 +1413,7 @@ test_expect_success '.gitmodules ignore=all suppresses submodule summary' '
 	git config -f .gitmodules  --remove-section submodule.subname
 '

-test_expect_success '.git/config ignore=all suppresses submodule summary' '
+test_expect_success '.git/config ignore=all suppresses unstaged submodule summary' '
 	git config --add -f .gitmodules submodule.subname.ignore none &&
 	git config --add -f .gitmodules submodule.subname.path sm &&
 	git config --add submodule.subname.ignore all &&
diff --git a/wt-status.c b/wt-status.c
index e1827fa..821d10c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -519,9 +519,19 @@ 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);

+	DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
 	if (s->ignore_submodule_arg) {
-		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
+	} else {
+		/*
+		 * Unless the user did explicitly request a submodule ignore
+		 * mode by passing a command line option we do not ignore any
+		 * changed submodule SHA-1s when comparing index and HEAD, no
+		 * matter what is configured. Otherwise the user won't be
+		 * shown any submodules she manually added (and which are
+		 * staged to be committed), which would be really confusing.
+		 */
+		handle_ignore_submodules_arg(&rev.diffopt, "dirty");
 	}

 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
-- 
1.9.1.378.g5fbabd4