[PATCH v4 0/4] for-each-repo: work correctly in a worktree

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

This was reported by Matthew [1] and I thought it would be a quick fix. It
turns out to have had a lot of implications to get exactly right.

[1]
https://lore.kernel.org/git/CABpCjbY=wpStuhxqRJ5TSNV3A-CmN-g-xZGJOQGSSv3GYhs2fQ@mail.gmail.com/


Updates in v3
=============

v2 included some important changes that apparently didn't save in my cover
letter. Sorry. The most important one was that the first patch was replaced
with one that tests the command outside of a Git repo so the first patch in
v1 can be caught as a bug.

But v2 also included a bug! The filtering of GIT_CONFIG* environment
variables was incorrect. Further, duplicating this logic from
prepare_other_repo_env() was a new bug waiting to happen.

Here is the new setup:

 * Patch 1 is the same as in v2.
 * A new Patch 2 extracts clear_local_repo_env() from
   prepare_other_repo_env().
 * The old Patch 2 is now Patch 3 with a much simpler implementation that
   calls clear_local_repo_env().
 * Tests are added to check that these config-related environment variables
   are preserved. The NO_REPLACE_OBJECTS case seemed like it would take
   longer than necessary to set up for the value it provides.
 * A fourth patch is added that simplifies the use of argv. It's separate
   because it modifies the original implementation in a way that is a "nice
   to have" but isn't necessary for solving the bug that motivates the
   series.


Updates in V4
=============

Minor updates from Peff's review:

 1. Update the comment of prepare_other_repo_env() to avoid duplication.
 2. Rename the new method to sanitize_repo_env().
 3. Move incorrect removal of 'int i;' to correct patch.

Thanks, -Stolee

Derrick Stolee (4):
  for-each-repo: test outside of repo context
  run-command: extract sanitize_repo_env helper
  for-each-repo: work correctly in a worktree
  for-each-repo: simplify passing of parameters

 builtin/for-each-repo.c  | 12 +++----
 run-command.c            |  7 ++++-
 run-command.h            | 15 ++++++---
 t/t0068-for-each-repo.sh | 67 +++++++++++++++++++++++++++++++++-------
 4 files changed, 77 insertions(+), 24 deletions(-)


base-commit: 67ad42147a7acc2af6074753ebd03d904476118f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2056%2Fderrickstolee%2Ffor-each-repo-in-gitdir-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2056/derrickstolee/for-each-repo-in-gitdir-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2056

Range-diff vs v3:

 1:  6e9d4f3029 = 1:  6e9d4f3029 for-each-repo: test outside of repo context
 2:  13d783dbbd ! 2:  24398664c2 run-command: extract clear_local_repo_env helper
     @@ Metadata
      Author: Derrick Stolee <stolee@gmail.com>
      
       ## Commit message ##
     -    run-command: extract clear_local_repo_env helper
     +    run-command: extract sanitize_repo_env helper
      
          The current prepare_other_repo_env() does two distinct things:
      
     @@ Commit message
          item is required as the GIT_DIR discovery should happen naturally from
          the '-C' parameter in the child process.
      
     +    Helped-by: Jeff King <peff@peff.net>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
       ## run-command.c ##
     @@ run-command.c: int run_auto_maintenance(int quiet)
       }
       
      -void prepare_other_repo_env(struct strvec *env, const char *new_git_dir)
     -+void clear_local_repo_env(struct strvec *env)
     ++void sanitize_repo_env(struct strvec *env)
       {
       	const char * const *var;
       
     @@ run-command.c: void prepare_other_repo_env(struct strvec *env, const char *new_g
      +
      +void prepare_other_repo_env(struct strvec *env, const char *new_git_dir)
      +{
     -+	clear_local_repo_env(env);
     ++	sanitize_repo_env(env);
       	strvec_pushf(env, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
       }
       
     @@ run-command.h: struct run_process_parallel_opts
      + * environment.h. GIT_CONFIG_PARAMETERS and GIT_CONFIG_COUNT are preserved
      + * to pass -c and --config-env options from the parent process.
      + */
     -+void clear_local_repo_env(struct strvec *env);
     ++void sanitize_repo_env(struct strvec *env);
      +
       /**
        * Convenience function which prepares env for a command to be run in a
     -  * new repo. This adds all GIT_* environment variables to env with the
     +- * new repo. This adds all GIT_* environment variables to env with the
     +- * exception of GIT_CONFIG_PARAMETERS and GIT_CONFIG_COUNT (which cause the
     +- * corresponding environment variables to be unset in the subprocess) and adds
     +- * an environment variable pointing to new_git_dir. See local_repo_env in
     +- * environment.h for more information.
     ++ * new repo. This removes variables pointing to the local repository (using
     ++ * sanitize_repo_env() above), and adds an environment variable pointing to
     ++ * new_git_dir.
     +  */
     + void prepare_other_repo_env(struct strvec *env, const char *new_git_dir);
     + 
 3:  2a6091095f ! 3:  e759b35069 for-each-repo: work correctly in a worktree
     @@ Commit message
          We need to be careful to unset the local Git environment variables and
          let the child process rediscover them, while also reinstating those
          variables in the parent process afterwards. Update run_command_on_repo()
     -    to use the new clear_local_repo_env() helper method to erase these
     +    to use the new sanitize_repo_env() helper method to erase these
          environment variables.
      
          During review of this bug fix, there were several incorrect patches
          demonstrating different bad behaviors. Most of these are covered by
          tests, when it is not too expensive to set it up. One case that would be
          expensive to set up is the GIT_NO_REPLACE_OBJECTS environment variable,
     -    but we trust that using clear_local_repo_env() will be sufficient to
     +    but we trust that using sanitize_repo_env() will be sufficient to
          capture these uncovered cases by using the common code for resetting
          environment variables.
      
     @@ builtin/for-each-repo.c
       #include "gettext.h"
       #include "parse-options.h"
       #include "path.h"
     -@@ builtin/for-each-repo.c: static const char * const for_each_repo_usage[] = {
     - 
     - static int run_command_on_repo(const char *path, int argc, const char ** argv)
     - {
     --	int i;
     +@@ builtin/for-each-repo.c: static int run_command_on_repo(const char *path, int argc, const char ** argv)
       	struct child_process child = CHILD_PROCESS_INIT;
       	char *abspath = interpolate_path(path, 0);
       
     -+	clear_local_repo_env(&child.env);
     ++	sanitize_repo_env(&child.env);
      +
       	child.git_cmd = 1;
       	strvec_pushl(&child.args, "-C", abspath, NULL);
 4:  f6582e9402 ! 4:  8b1f083da1 for-each-repo: simplify passing of parameters
     @@ builtin/for-each-repo.c: static const char * const for_each_repo_usage[] = {
      -static int run_command_on_repo(const char *path, int argc, const char ** argv)
      +static int run_command_on_repo(const char *path, const char **argv)
       {
     +-	int i;
       	struct child_process child = CHILD_PROCESS_INIT;
       	char *abspath = interpolate_path(path, 0);
     + 
      @@ builtin/for-each-repo.c: static int run_command_on_repo(const char *path, int argc, const char ** argv)
       
       	child.git_cmd = 1;

-- 
gitgitgadget