ab/run-command (was: What's cooking in git.git (Dec 2021, #04; Wed, 15))

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 16, 2021, 11:51 UTC
Message-ID: <211216.868rwkbv1b.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq8rwlz3cq.fsf@gitster.g>

On Wed, Dec 15 2021, Junio C Hamano wrote:

Show 18 quoted lines

> [Graduated to 'master']
> [...]
> * ab/run-command (2021-11-25) 9 commits
>   (merged to 'next' on 2021-12-07 at 5c0bedf853)
>  + run-command API: remove "env" member, always use "env_array"
>  + difftool: use "env_array" to simplify memory management
>  + run-command API: remove "argv" member, always use "args"
>  + run-command API users: use strvec_push(), not argv construction
>  + run-command API users: use strvec_pushl(), not argv construction
>  + run-command tests: use strvec_pushv(), not argv assignment
>  + run-command API users: use strvec_pushv(), not argv assignment
>  + upload-archive: use regular "struct child_process" pattern
>  + worktree: stop being overly intimate with run_command() internals
>
>  API clean-up.
>  cf. <211130.86k0gpcpy2.gmgdl@evledraar.gmail.com>
>  cf. <CAPig+cSvFgJTiq8pXrWy_7ukQwE1y9ZGwMgKmyjWuQHdNGxiDQ@mail.gmail.com>
>  source: <cover-v3-0.9-00000000000-20211125T224833Z-avarab@gmail.com>

Thanks! FWIW the tip commit mentioned that renaming "env_array" to "env"
probably wasn't worth it, but having tried it just now I think it's
probably a worthwhile follow-up at some point. The diff is just:
	
	diff --git a/run-command.h b/run-command.h
	index 2be5f5d6422..c5d44cc9a5c 100644
	--- a/run-command.h
	+++ b/run-command.h
	@@ -73,7 +73,7 @@ struct child_process {
	 	 * The memory in .env_array will be cleaned up automatically during
	 	 * `finish_command` (or during `start_command` when it is unsuccessful).
	 	 */
	-	struct strvec env_array;
	+	struct strvec env;
	 	pid_t pid;
	 
	 	int trace2_child_id;
	@@ -147,7 +147,7 @@ struct child_process {
	 
	 #define CHILD_PROCESS_INIT { \
	 	.args = STRVEC_INIT, \
	-	.env_array = STRVEC_INIT, \
	+	.env = STRVEC_INIT, \
	 }
	 
	 /**

Followed by a rather large mechanical change, but that change can almost
entirely be done with this cocci rule:
    
    @@
    struct child_process E;
    @@
    - E.env_array
    + E.env
    
    @@
    struct child_process *E;
    @@
    - E->env_array
    + E->env

The "almost" being that we'll also need this on top:
    
     builtin/submodule--helper.c |  2 +-
     run-command.c               |  6 +++---
     run-command.h               | 10 +++++-----
     sequencer.c                 |  6 +++---
     submodule.h                 |  4 ++--
     5 files changed, 14 insertions(+), 14 deletions(-)

Which changes comments and function/variable names that refer to "env_array" to refer to "env".

I won't submit it now because we've got some "env_array" in master..seen, but there'll probably be a time when there'll be none which might make it trivial.

Or maybe you're not interested in either case, which is also fine. It's really not needed for anything other than to resolve the minor oddity that we have two "struct strvec"'s, "args" and "env_array". It's just a bit odd to have one with "_array" in the name, which is only because the now-removed "env" was taken at the time.