git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Andrzej Hunt <andrzej@ahunt.org>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v4 3/8] for-each-repo: run subcommands on configured repos
Date: Mon, 3 May 2021 18:10:47 +0200	[thread overview]
Message-ID: <cda4f200-2400-e915-e995-36eea2a27c11@ahunt.org> (raw)
In-Reply-To: <dd9237927395ef69663ab376a2da74da4654c495.1602782524.git.gitgitgadget@gmail.com>


On 15/10/2020 19:21, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
[... snip ...]
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> new file mode 100644
> index 0000000000..5bba623ff1
> --- /dev/null
> +++ b/builtin/for-each-repo.c
> @@ -0,0 +1,58 @@
> +#include "cache.h"
> +#include "config.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +#include "string-list.h"
> +
> +static const char * const for_each_repo_usage[] = {
> +	N_("git for-each-repo --config=<config> <command-args>"),
> +	NULL
> +};
> +
> +static int run_command_on_repo(const char *path,
> +			       void *cbdata)
> +{
> +	int i;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strvec *args = (struct strvec *)cbdata;

I was curious there's a strong reason for declaring args as void * 
followed by this cast? The most obvious answer seems to be that this 
probably evolved from a callback - and we could simplify it now?
> +
> +	child.git_cmd = 1;
> +	strvec_pushl(&child.args, "-C", path, NULL);
> +
> +	for (i = 0; i < args->nr; i++)
> +		strvec_push(&child.args, args->v[i]);
So here we're copying all of args - and I don't see any way of avoiding 
it since we're adding it to child's arg list.

> +
> +	return run_command(&child);
> +}
> +
> +int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
> +{
> +	static const char *config_key = NULL;
> +	int i, result = 0;
> +	const struct string_list *values;
> +	struct strvec args = STRVEC_INIT;
> +
> +	const struct option options[] = {
> +		OPT_STRING(0, "config", &config_key, N_("config"),
> +			   N_("config key storing a list of repository paths")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, for_each_repo_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	if (!config_key)
> +		die(_("missing --config=<config>"));
> +
> +	for (i = 0; i < argc; i++)
> +		strvec_push(&args, argv[i]);

But why do we have to copy all of argv 1:1 into args here, only to later 
pass it to run_command_on_repo() which, as described above, copies the 
entire input again? I suspect this was done to comply with 
run_command_on_repo()'s API (which takes strvec) - does that seem 
plausible, or did I miss something?

Which brings me to the real reason for my questions: I noticed we "leak" 
args (this leak is of no significance since it happens in cmd_*, but 
LSAN still complains, and I'm trying to get tests running leak-free). My 
initial inclination was to strvec_clear() or UNLEAK() args - but if we 
can avoid creating args in the first place we also wouldn't need to 
clear it later.

My current proposal is therefore to completely remove args and pass 
argc+argv into run_command_on_repo() - but I wanted to be sure that I 
didn't miss some important reason to stick with the current approach.

> +
> +	values = repo_config_get_value_multi(the_repository,
> +					     config_key);
> +
> +	for (i = 0; !result && i < values->nr; i++)
> +		result = run_command_on_repo(values->items[i].string, &args);
> +
> +	return result;
> +}
[... snip ...]

(I hope this doesn't come across as useless necroposting - I figured it 
would be easier to clarify these questions on the original thread as 
opposed to potentially discussing it as part of my next leak-fixing 
series :) .)

ATB,

   Andrzej

  reply	other threads:[~2021-05-03 16:10 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04 15:41 [PATCH 0/7] Maintenance III: Background maintenance Derrick Stolee via GitGitGadget
2020-09-04 15:42 ` [PATCH 1/7] maintenance: optionally skip --auto process Derrick Stolee via GitGitGadget
2020-09-04 15:42 ` [PATCH 2/7] maintenance: add --schedule option and config Derrick Stolee via GitGitGadget
2020-09-08 13:07   ` Đoàn Trần Công Danh
2020-09-09 12:14     ` Derrick Stolee
2020-09-04 15:42 ` [PATCH 3/7] for-each-repo: run subcommands on configured repos Derrick Stolee via GitGitGadget
2020-09-04 15:42 ` [PATCH 4/7] maintenance: add [un]register subcommands Derrick Stolee via GitGitGadget
2020-09-04 15:42 ` [PATCH 5/7] maintenance: add start/stop subcommands Derrick Stolee via GitGitGadget
2020-09-08  6:29   ` SZEDER Gábor
2020-09-08 12:43     ` Derrick Stolee
2020-09-08 19:31     ` Junio C Hamano
2020-09-04 15:42 ` [PATCH 6/7] maintenance: recommended schedule in register/start Derrick Stolee via GitGitGadget
2020-09-04 15:42 ` [PATCH 7/7] maintenance: add troubleshooting guide to docs Derrick Stolee via GitGitGadget
2020-09-11 17:49 ` [PATCH v2 0/7] Maintenance III: Background maintenance Derrick Stolee via GitGitGadget
2020-09-11 17:49   ` [PATCH v2 1/7] maintenance: optionally skip --auto process Derrick Stolee via GitGitGadget
2020-09-11 17:49   ` [PATCH v2 2/7] maintenance: add --schedule option and config Derrick Stolee via GitGitGadget
2020-09-11 17:49   ` [PATCH v2 3/7] for-each-repo: run subcommands on configured repos Derrick Stolee via GitGitGadget
2020-09-11 17:49   ` [PATCH v2 4/7] maintenance: add [un]register subcommands Derrick Stolee via GitGitGadget
2020-09-17 14:05     ` Đoàn Trần Công Danh
2020-09-11 17:49   ` [PATCH v2 5/7] maintenance: add start/stop subcommands Derrick Stolee via GitGitGadget
2020-09-11 17:49   ` [PATCH v2 6/7] maintenance: recommended schedule in register/start Derrick Stolee via GitGitGadget
2020-09-29 19:48     ` Martin Ågren
2020-09-30 20:11       ` Derrick Stolee
2020-10-01 20:38         ` Derrick Stolee
2020-10-02  0:38           ` Đoàn Trần Công Danh
2020-10-02  1:55             ` Derrick Stolee
2020-10-05 13:16               ` Đoàn Trần Công Danh
2020-10-05 18:17                 ` Derrick Stolee
2020-09-11 17:49   ` [PATCH v2 7/7] maintenance: add troubleshooting guide to docs Derrick Stolee via GitGitGadget
2020-10-05 12:57   ` [PATCH v3 0/7] Maintenance III: Background maintenance Derrick Stolee via GitGitGadget
2020-10-05 12:57     ` [PATCH v3 1/7] maintenance: optionally skip --auto process Derrick Stolee via GitGitGadget
2020-10-05 12:57     ` [PATCH v3 2/7] maintenance: add --schedule option and config Derrick Stolee via GitGitGadget
2020-10-05 12:57     ` [PATCH v3 3/7] for-each-repo: run subcommands on configured repos Derrick Stolee via GitGitGadget
2020-10-05 12:57     ` [PATCH v3 4/7] maintenance: add [un]register subcommands Derrick Stolee via GitGitGadget
2020-10-05 12:57     ` [PATCH v3 5/7] maintenance: add start/stop subcommands Derrick Stolee via GitGitGadget
2020-10-05 12:57     ` [PATCH v3 6/7] maintenance: use default schedule if not configured Derrick Stolee via GitGitGadget
2020-10-05 19:57       ` Martin Ågren
2020-10-08 13:32         ` Derrick Stolee
2020-10-05 12:57     ` [PATCH v3 7/7] maintenance: add troubleshooting guide to docs Derrick Stolee via GitGitGadget
2020-10-15 17:21     ` [PATCH v4 0/8] Maintenance III: Background maintenance Derrick Stolee via GitGitGadget
2020-10-15 17:21       ` [PATCH v4 1/8] maintenance: optionally skip --auto process Derrick Stolee via GitGitGadget
2020-10-15 17:21       ` [PATCH v4 2/8] maintenance: add --schedule option and config Derrick Stolee via GitGitGadget
2021-02-09 14:06         ` Ævar Arnfjörð Bjarmason
2021-02-09 16:54           ` Derrick Stolee
2021-05-10 12:16             ` Ævar Arnfjörð Bjarmason
2021-05-10 18:42               ` Junio C Hamano
2020-10-15 17:21       ` [PATCH v4 3/8] for-each-repo: run subcommands on configured repos Derrick Stolee via GitGitGadget
2021-05-03 16:10         ` Andrzej Hunt [this message]
2021-05-03 17:01           ` Eric Sunshine
2021-05-03 19:26             ` Eric Sunshine
2021-05-03 19:43           ` Derrick Stolee
2020-10-15 17:22       ` [PATCH v4 4/8] maintenance: add [un]register subcommands Derrick Stolee via GitGitGadget
2020-10-15 17:22       ` [PATCH v4 5/8] maintenance: add start/stop subcommands Derrick Stolee via GitGitGadget
2020-12-09 18:51         ` Josh Steadmon
2020-12-09 19:16           ` Josh Steadmon
2020-12-09 21:59             ` Derrick Stolee
2020-12-10  0:13             ` Junio C Hamano
2020-12-10  1:52               ` Derrick Stolee
2020-12-10  6:54                 ` Junio C Hamano
2020-10-15 17:22       ` [PATCH v4 6/8] maintenance: create maintenance.strategy config Derrick Stolee via GitGitGadget
2020-10-15 17:22       ` [PATCH v4 7/8] maintenance: use 'incremental' strategy by default Derrick Stolee via GitGitGadget
2020-10-15 17:22       ` [PATCH v4 8/8] maintenance: add troubleshooting guide to docs Derrick Stolee via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cda4f200-2400-e915-e995-36eea2a27c11@ahunt.org \
    --to=andrzej@ahunt.org \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --subject='Re: [PATCH v4 3/8] for-each-repo: run subcommands on configured repos' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git