git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4 4/6] config: correctly read worktree configs in submodules

From
Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date
Sep 1, 2020, 21:44 UTC
Message-ID
<CAHd-oW6LA7Ovnu-e9c+WapF-e+JMichWZEetxmnbjCT_H6D0AQ@mail.gmail.com>
In-Reply-To
<20200901024130.GA3332286@google.com>
Hi, Jonathan
On Mon, Aug 31, 2020 at 11:41 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 18 quoted lines
>
> Hi,
>
> Matheus Tavares wrote:
>
> > One of the steps in do_git_config_sequence() is to load the
> > worktree-specific config file. Although the function receives a git_dir
> > string, it relies on git_pathdup(), which uses the_repository->git_dir,
> > to make the path to the file. Furthermore, it also checks that
> > extensions.worktreeConfig is set through the
> > repository_format_worktree_config variable, which refers to
> > the_repository only. Thus, when a submodule has worktree-specific
> > settings, a command executed in the superproject that recurses into the
> > submodule won't find the said settings.
>
> I think the above goes out of order: it states the "how" before the
> "what".  Instead, a commit message should lead with the problem the
> change aims to solve.
Thanks. I will reorder these two sections in the commit message.
Show 8 quoted lines
> Is the idea here that until this patch, we're only able to read
> worktree config from a repository when extensions.worktreeConfig is
> set in the_repository, meaning that
>
> - when examining submodule config in a process where the_repository
>   represents the superproject, we do not read the submodule's worktree
>   config even if extensions.worktreeConfig is set in the submodule,
>   unless the superproject has extensions.worktreeConfig set, and
Right.
Show 6 quoted lines
> - when examining submodule config in a process where the_repository
>   represents the superproject, we *do* read the submodule's worktree
>   config even if extensions.worktreeConfig is not set in the submodule,
>   if the superproject has extensions.worktreeConfig set, and
>
> ?

Right, but with one change: if extensions.worktreeConfig is not set in the submodule and it is set in the superproject, the *superproject's* worktree config is read (independently of which git_dir was given as argument).

Show 34 quoted lines
> That sounds like a serious problem indeed.  Thanks for fixing it.
>
> > This will be especially important in the next patch: git-grep will learn
> > to honor sparse checkouts and, when running with --recurse-submodules,
> > the submodule's sparse checkout settings must be loaded. As these
> > settings are stored in the config.worktree file, they would be ignored
> > without this patch. So let's fix this by reading the right
> > config.worktree file and extensions.worktreeConfig setting, based on the
> > git_dir and commondir paths given to do_git_config_sequence(). Also
> > add a test to avoid any regressions.
>
> I see.  I'm not sure that's more important than other cases, but I
> can understand if the problem was noticed in this circumstance. :)
>
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> >  config.c                   | 21 +++++++++---
> >  t/helper/test-config.c     | 67 +++++++++++++++++++++++++++++++++-----
> >  t/t2404-worktree-config.sh | 16 +++++++++
> >  3 files changed, 91 insertions(+), 13 deletions(-)
> >
> > diff --git a/config.c b/config.c
> > index 8db9c77098..c2d56309dc 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts,
> >               ret += git_config_from_file(fn, repo_config, data);
> >
> >       current_parsing_scope = CONFIG_SCOPE_WORKTREE;
> > -     if (!opts->ignore_worktree && repository_format_worktree_config) {
> > +     if (!opts->ignore_worktree && repo_config && opts->git_dir) {
>
> Can we eliminate the repository_format_worktree_config global to save
> the next caller from the same problem?
Hmm, I think it's possible, I will investigate it further.
Show 12 quoted lines
> > +             struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
> > +             struct strbuf buf = STRBUF_INIT;
> > +
> > +             read_repository_format(&repo_fmt, repo_config);
> > +
> > +             if (!verify_repository_format(&repo_fmt, &buf) &&
> > +                 repo_fmt.worktree_config) {
>
> This undoes the caching the repository_format_worktree_config means to
> do.  Can we cache the value in "struct repository" instead?  That way,
> in the common case where we're reading the_repository, we wouldn't
> experience a slowdown.

Yeah, that would be the best solution. But, unfortunately, do_git_config_sequence() doesn't receive a complete repository struct, just the 'commondir' and 'git_dir' strings.

Show 5 quoted lines
> > -             char *path = git_pathdup("config.worktree");
> > +                     char *path = mkpathdup("%s/config.worktree", opts->git_dir);
>
> Can this use a helper like repo_git_path or strbuf_repo_git_path
> (preferably one using strbuf like the latter)?

Hmm, here we would have the same problem of not having a 'struct repository' to pass to those functions :(

Show 42 quoted lines
> [...]
> > +             strbuf_release(&buf);
> > +             clear_repository_format(&repo_fmt);
> >       }
> >
> >       current_parsing_scope = CONFIG_SCOPE_COMMAND;
> > diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> > index 61da2574c5..284f83a921 100644
> > --- a/t/helper/test-config.c
> > +++ b/t/helper/test-config.c
> > @@ -2,12 +2,19 @@
> >  #include "cache.h"
> >  #include "config.h"
> >  #include "string-list.h"
> > +#include "submodule-config.h"
> >
> >  /*
> >   * This program exposes the C API of the configuration mechanism
> >   * as a set of simple commands in order to facilitate testing.
> >   *
> > - * Reads stdin and prints result of command to stdout:
> > + * Usage: test-tool config [--submodule=<path>] <cmd> [<args>]
> > + *
> > + * If --submodule=<path> is given, <cmd> will operate on the submodule at the
> > + * given <path>. This option is not valid for the commands: read_early_config,
> > + * configset_get_value and configset_get_value_multi.
>
> Nice!
>
> [...]
> > @@ -93,7 +102,18 @@ int cmd__config(int argc, const char **argv)
> >       if (argc == 0)
> >               goto print_usage_error;
> >
> > +     if (skip_prefix(*argv, "--submodule=", &subrepo_path)) {
> > +             argc--;
> > +             argv++;
> > +             if (argc == 0)
> > +                     goto print_usage_error;
> > +     }
>
> Can this use the parse_options API?

Right, it would make it easier to add more options in the future. There is only one consideration, though, about parse_options()'s exit codes on error, but more on that below...

Show 7 quoted lines
> > +
> >       if (argc == 2 && !strcmp(argv[0], "read_early_config")) {
> > +             if (subrepo_path) {
> > +                     fprintf(stderr, "Cannot use --submodule with read_early_config\n");
> > +                     return TC_USAGE_ERROR;
>
> Should this use die() or BUG()?

The idea of using TC_USAGE_ERROR (129) here and not die() (128), was that some users of the test-config helper want to detect die() errors from the config machinery itself. So by using a different exit code, we can avoid false positives in these tests. Of course they should also be checking stderr/stdout, but there is at least one test which only checks the exit code. Rethinking about that now, instead of using different exit codes in test-config.c, should we adjust the tests to use `test_must_fail` and only check stderr/stdout? Then we could use die() (or BUG()) here, as you suggested, as well as the parse_options API in the snippet above. Does that sound reasonable?

Show 15 quoted lines
> > +             }
> >               read_early_config(early_config_cb, (void *)argv[1]);
> >               return TC_SUCCESS;
> >       }
> > @@ -101,8 +121,23 @@ int cmd__config(int argc, const char **argv)
> >       setup_git_directory();
> >       git_configset_init(&cs);
> >
> > +     if (subrepo_path) {
> > +             const struct submodule *sub;
> > +             struct repository *subrepo = xcalloc(1, sizeof(*repo));
>
> nit: this could be scoped to cmd__config:
>
>         struct repository subrepo = {0};
OK, will do. Thanks
Show 13 quoted lines
> > +
> > +             sub = submodule_from_path(the_repository, &null_oid, subrepo_path);
> > +             if (!sub || repo_submodule_init(subrepo, the_repository, sub)) {
> > +                     fprintf(stderr, "Invalid argument to --submodule: '%s'\n",
> > +                             subrepo_path);
> > +                     free(subrepo);
> > +                     ret = TC_USAGE_ERROR;
>
> Likewise: I think may want to use die() or BUG() (and likewise for other
> USAGE_ERROR cases).
>
> Thanks and hope that helps,
> Jonathan
It did :) Thanks a lot for the comments!
Previous: Jonathan NiederNext: Matheus Tavares
Message 28 of 31 in “grep: honor sparse checkout and add option to ignore it”
  1. 0/5 grep: honor sparse checkout and add option to ignore itMatheus Tavares, May 28, 2020
  2. 1/5 doc: grep: unify info on configuration variablesMatheus Tavares, May 28, 2020
  3. 3/5 config: correctly read worktree configs in submodulesMatheus Tavares, May 28, 2020
  4. Elijah NewrenMay 30, 2020
  5. Matheus Tavares BernardinoJun 1, 2020
  6. 2/5 t/helper/test-config: return exit codes consistentlyMatheus Tavares, May 28, 2020
  7. Elijah NewrenMay 30, 2020
  8. Matheus Tavares BernardinoJun 1, 2020
  9. 4/5 grep: honor sparse checkout patternsMatheus Tavares, May 28, 2020
  10. Elijah NewrenMay 30, 2020
  11. Matheus Tavares BernardinoJun 1, 2020
  12. Elijah NewrenJun 3, 2020
  13. Matheus Tavares BernardinoJun 10, 2020
  14. 5/5 config: add setting to ignore sparsity patterns in some cmdsMatheus Tavares, May 28, 2020
  15. Elijah NewrenMay 30, 2020
  16. Matheus Tavares BernardinoJun 1, 2020
  17. Elijah NewrenJun 3, 2020
  18. Matheus Tavares BernardinoJun 10, 2020
  19. Elijah NewrenJun 11, 2020
  20. 0/6 grep: honor sparse checkout and add option to ignore itMatheus Tavares, Jun 12, 2020
  21. 2/6 t/helper/test-config: return exit codes consistentlyMatheus Tavares, Jun 12, 2020
  22. 1/6 doc: grep: unify info on configuration variablesMatheus Tavares, Jun 12, 2020
  23. 3/6 t/helper/test-config: facilitate addition of new cli optionsMatheus Tavares, Jun 12, 2020
  24. 4/6 config: correctly read worktree configs in submodulesMatheus Tavares, Jun 12, 2020
  25. Elijah NewrenJun 16, 2020
  26. Matheus Tavares BernardinoJun 21, 2020
  27. Jonathan NiederSep 1, 2020
  28. Matheus Tavares BernardinoSep 1, 2020
  29. 5/6 grep: honor sparse checkout patternsMatheus Tavares, Jun 12, 2020
  30. 6/6 config: add setting to ignore sparsity patterns in some cmdsMatheus Tavares, Jun 12, 2020
  31. Elijah NewrenJun 16, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.