[PATCH] RFC: commit: add a commit.all-ignore-submodules config option

[PATCH] RFC: commit: add a commit.all-ignore-submodules config option

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

One of my most frequent mistake is to commit undesired submodules
changes when doing "commit -a", and I have seen a number of people doing
the same mistake in various projects. I wish there would be a config to
change this default behaviour.

submodule.<name>.ignore or diff.ignoreSubmodules have different
tradeoffs, as they change the diff or status behaviour. I just wish the
default behaviour of "commit -a" to be different, to exclude submodules
by default.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 builtin/add.c    |  6 ++++++
 builtin/commit.c | 10 +++++++++-
 cache.h          |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 4c38aff419..4023ee2681 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -82,6 +82,12 @@ static void update_callback(struct diff_queue_struct *q,
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		const char *path = p->one->path;
+
+		if (data->flags & ADD_CACHE_IGNORE_SUBMODULES &&
+		    S_ISGITLINK(p->one->mode)) {
+		    continue;
+		}
+
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
diff --git a/builtin/commit.c b/builtin/commit.c
index aa1332308a..ce37e4e6da 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -110,6 +110,7 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+static int commit_all_ignore_submodules;
 
 /*
  * The default commit message cleanup mode will remove the lines
@@ -415,8 +416,10 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * (B) on failure, rollback the real index.
 	 */
 	if (all || (also && pathspec.nr)) {
+		int flags = commit_all_ignore_submodules ? ADD_CACHE_IGNORE_SUBMODULES : 0;
+
 		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, flags);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_locked_index(&the_index, &index_lock, 0))
@@ -1475,6 +1478,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "commit.all-ignore-submodules")) {
+		commit_all_ignore_submodules = git_config_bool(k, v);
+		return 0;
+	}
+
 	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
diff --git a/cache.h b/cache.h
index 1554488d66..5fb3b18916 100644
--- a/cache.h
+++ b/cache.h
@@ -816,6 +816,7 @@ int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_IGNORE_ERRORS	4
 #define ADD_CACHE_IGNORE_REMOVAL 8
 #define ADD_CACHE_INTENT 16
+#define ADD_CACHE_IGNORE_SUBMODULES 32
 /*
  * These two are used to add the contents of the file at path
  * to the index, marking the working tree up-to-date by storing

base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
-- 
2.25.0.rc1.1.gb0343b22ed

Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option

[Jonathan Nieder]

Hi,

Marc-André Lureau wrote:

> One of my most frequent mistake is to commit undesired submodules
> changes when doing "commit -a", and I have seen a number of people doing
> the same mistake in various projects. I wish there would be a config to
> change this default behaviour.

Can you say more about the overall workflow this is part of?  What
causes the submodules to change state in the first place here?

[...]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
[...]
> @@ -1475,6 +1478,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(k, "commit.all-ignore-submodules")) {
> +		commit_all_ignore_submodules = git_config_bool(k, v);
> +		return 0;
> +	}

nit, less important than the comment above: no other config items use
this naming scheme.  We'd have to come up with a different name if we
want to pursue this.

If I want to disable this setting for a particular "git commit"
invocation, how do I do that?  Typically when adding new settings, we
add them first as command-line options and then as a separate followup
can introduce configuration to change the defaults.

To summarize: I'm interested in hearing more about the overall
workflow so we can make the standard behavior without any special
configuration work better for it, too.

Thanks and hope that helps,
Jonathan

Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option

[Marc-André Lureau]

Hi

On Sat, Jan 4, 2020 at 4:45 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Marc-André Lureau wrote:
>
> > One of my most frequent mistake is to commit undesired submodules
> > changes when doing "commit -a", and I have seen a number of people doing
> > the same mistake in various projects. I wish there would be a config to
> > change this default behaviour.
>
> Can you say more about the overall workflow this is part of?  What
> causes the submodules to change state in the first place here?

The most common case is, I guess, when you work on different branches
that have different (compatible) versions of the submodules. It is
easy to go unnoticed then, although I am usually quite careful what I
include in my commit, and will usually add changes interactively with
add -i instead.

I often rely on git commit -a during an interactive rebase. I check
the project at various points in history, find a small fix, and git
commit -a. At this point, I may have included a submodule change
inadvertently that may happen later in the series for example.

>
> [...]
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> [...]
> > @@ -1475,6 +1478,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> >               return 0;
> >       }
> >
> > +     if (!strcmp(k, "commit.all-ignore-submodules")) {
> > +             commit_all_ignore_submodules = git_config_bool(k, v);
> > +             return 0;
> > +     }
>
> nit, less important than the comment above: no other config items use
> this naming scheme.  We'd have to come up with a different name if we
> want to pursue this.

Sure, I am open to suggestions.

>
> If I want to disable this setting for a particular "git commit"
> invocation, how do I do that?  Typically when adding new settings, we
> add them first as command-line options and then as a separate followup
> can introduce configuration to change the defaults.

--all=no-ignore ?

>
> To summarize: I'm interested in hearing more about the overall
> workflow so we can make the standard behavior without any special
> configuration work better for it, too.
>
> Thanks and hope that helps,
> Jonathan
>

thanks for the feeback

Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option

[Jonathan Nieder]

Marc-André Lureau wrote:
> On Sat, Jan 4, 2020 at 4:45 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Marc-André Lureau wrote:

>>> One of my most frequent mistake is to commit undesired submodules
>>> changes when doing "commit -a", and I have seen a number of people doing
>>> the same mistake in various projects. I wish there would be a config to
>>> change this default behaviour.
>>
>> Can you say more about the overall workflow this is part of?  What
>> causes the submodules to change state in the first place here?
>
> The most common case is, I guess, when you work on different branches
> that have different (compatible) versions of the submodules.

Ah!  This is because "git checkout" defaults to --no-recurse-submodules,
which is a terrible default.

Does "git config submodule.recurse true" help?  If so, we can look
into which it would take to flip that default.

Thanks,
Jonathan

Re: [PATCH] RFC: commit: add a commit.all-ignore-submodules config option

[Marc-André Lureau]

Hi

On Tue, Jan 7, 2020 at 4:05 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Marc-André Lureau wrote:
> > On Sat, Jan 4, 2020 at 4:45 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
> >> Marc-André Lureau wrote:
>
> >>> One of my most frequent mistake is to commit undesired submodules
> >>> changes when doing "commit -a", and I have seen a number of people doing
> >>> the same mistake in various projects. I wish there would be a config to
> >>> change this default behaviour.
> >>
> >> Can you say more about the overall workflow this is part of?  What
> >> causes the submodules to change state in the first place here?
> >
> > The most common case is, I guess, when you work on different branches
> > that have different (compatible) versions of the submodules.
>
> Ah!  This is because "git checkout" defaults to --no-recurse-submodules,
> which is a terrible default.

Thanks for the hint, I'll give it a try for a while and let you know.

> Does "git config submodule.recurse true" help?  If so, we can look
> into which it would take to flip that default.
>
> Thanks,
> Jonathan
>