| Message ID | 20200103120613.1063828-1-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: commit: add a commit.all-ignore-submodules config option | expand |
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
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
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
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 >
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