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

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

This was reported by Matthew [1] and is a quick fix.

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

I also took the liberty of removing the_repository as I wanted to make sure
that wasn't involved here.


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.

Thanks, -Stolee

Derrick Stolee (4):
  for-each-repo: test outside of repo context
  run-command: extract clear_local_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            |  7 +++++
 t/t0068-for-each-repo.sh | 67 +++++++++++++++++++++++++++++++++-------
 4 files changed, 74 insertions(+), 19 deletions(-)


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

Range-diff vs v2:

 1:  6e9d4f3029 = 1:  6e9d4f3029 for-each-repo: test outside of repo context
 -:  ---------- > 2:  13d783dbbd run-command: extract clear_local_repo_env helper
 2:  4e3f4aa6cd ! 3:  2a6091095f 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 store, unset, then reset the non-NULL variables.
     +    to use the new clear_local_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
     +    capture these uncovered cases by using the common code for resetting
     +    environment variables.
      
          Reported-by: Matthew Gabeler-Lee <fastcat@gmail.com>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
     @@ 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;
     -+	int res;
       	struct child_process child = CHILD_PROCESS_INIT;
     -+	char **envvars;
     -+	size_t envvar_nr = 0;
       	char *abspath = interpolate_path(path, 0);
       
     -+	while (local_repo_env[envvar_nr])
     -+		envvar_nr++;
     -+
     -+	CALLOC_ARRAY(envvars, envvar_nr);
     -+
     -+	for (size_t i = 0; i < envvar_nr; i++) {
     -+		envvars[i] = getenv(local_repo_env[i]);
     -+
     -+		if (envvars[i]) {
     -+			unsetenv(local_repo_env[i]);
     -+			envvars[i] = xstrdup(envvars[i]);
     -+		}
     -+	}
     ++	clear_local_repo_env(&child.env);
      +
       	child.git_cmd = 1;
       	strvec_pushl(&child.args, "-C", abspath, NULL);
       
     --	for (i = 0; i < argc; i++)
     -+	for (int i = 0; i < argc; i++)
     - 		strvec_push(&child.args, argv[i]);
     - 
     - 	free(abspath);
     - 
     --	return run_command(&child);
     -+	res = run_command(&child);
     -+
     -+	for (size_t i = 0; i < envvar_nr; i++) {
     -+		if (envvars[i]) {
     -+			setenv(local_repo_env[i], envvars[i], 1);
     -+			free(envvars[i]);
     -+		}
     -+	}
     -+
     -+	free(envvars);
     -+	return res;
     - }
     - 
     - int cmd_for_each_repo(int argc,
      
       ## t/t0068-for-each-repo.sh ##
      @@ t/t0068-for-each-repo.sh: TEST_NO_CREATE_REPO=1
     + . ./test-lib.sh
     + 
       test_expect_success 'run based on configured value' '
     - 	git init one &&
     - 	git init two &&
     +-	git init one &&
     +-	git init two &&
      -	git init three &&
     +-	git init ~/four &&
     ++	git init --initial-branch=one one &&
     ++	git init --initial-branch=two two &&
      +	git -C two worktree add --orphan ../three &&
     - 	git init ~/four &&
     ++	git -C three checkout -b three &&
     ++	git init --initial-branch=four ~/four &&
     ++
       	git -C two commit --allow-empty -m "DID NOT RUN" &&
       	git config --global run.key "$TRASH_DIRECTORY/one" &&
     + 	git config --global --add run.key "$TRASH_DIRECTORY/three" &&
      @@ t/t0068-for-each-repo.sh: test_expect_success 'run based on configured value' '
       	git -C three log -1 --pretty=format:%s >message &&
       	grep again message &&
     @@ t/t0068-for-each-repo.sh: test_expect_success 'run based on configured value' '
      -	grep again message
      +	grep again message &&
      +
     -+	git -C three for-each-repo --config=run.key -- commit --allow-empty -m "ran from worktree" &&
     ++	git -C three for-each-repo --config=run.key -- \
     ++		commit --allow-empty -m "ran from worktree" &&
      +	git -C one log -1 --pretty=format:%s >message &&
     -+	grep worktree message &&
     ++	test_grep "ran from worktree" message &&
      +	git -C two log -1 --pretty=format:%s >message &&
     -+	! grep worktree message &&
     ++	test_grep ! "ran from worktree" message &&
      +	git -C three log -1 --pretty=format:%s >message &&
     -+	grep worktree message &&
     ++	test_grep "ran from worktree" message &&
      +	git -C ~/four log -1 --pretty=format:%s >message &&
     -+	grep worktree message
     ++	test_grep "ran from worktree" message &&
     ++
     ++	# Test running with config values set by environment
     ++	cat >expect <<-EOF &&
     ++	ran from worktree (HEAD -> refs/heads/one)
     ++	ran from worktree (HEAD -> refs/heads/three)
     ++	ran from worktree (HEAD -> refs/heads/four)
     ++	EOF
     ++
     ++	GIT_CONFIG_PARAMETERS="${SQ}log.decorate=full${SQ}" \
     ++		git -C three for-each-repo --config=run.key -- log --format="%s%d" -1 >out &&
     ++	test_cmp expect out &&
     ++
     ++	cat >test-config <<-EOF &&
     ++	[run]
     ++		key = $(pwd)/one
     ++		key = $(pwd)/three
     ++		key = $(pwd)/four
     ++
     ++	[log]
     ++		decorate = full
     ++	EOF
     ++
     ++	GIT_CONFIG_GLOBAL="$(pwd)/test-config" \
     ++		git -C three for-each-repo --config=run.key -- log --format="%s%d" -1 >out &&
     ++	test_cmp expect out
       '
       
       test_expect_success 'do nothing on empty config' '
 -:  ---------- > 4:  f6582e9402 for-each-repo: simplify passing of parameters

-- 
gitgitgadget