From: Eric Sunshine <sunshine@sunshineco.com> To: Andrzej Hunt <andrzej@ahunt.org> Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>, Git List <git@vger.kernel.org>, 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 13:01:42 -0400 [thread overview] Message-ID: <CAPig+cToeuq7S1176ze7kX9krjK1rGMOM-JDznF_Fr_qV09qWw@mail.gmail.com> (raw) In-Reply-To: <cda4f200-2400-e915-e995-36eea2a27c11@ahunt.org> I'm not Stolee and hadn't even looked at this code before, but I'll jump in with a couple comments... On Mon, May 3, 2021 at 12:11 PM Andrzej Hunt <andrzej@ahunt.org> wrote: > On 15/10/2020 19:21, Derrick Stolee via GitGitGadget wrote: > > +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? Agreed, the `void*` cbdata doesn't make sense here. > > + 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. ... (dot dot dot) > > +int cmd_for_each_repo(int argc, const char **argv, const char *prefix) > > +{ > > + struct strvec args = STRVEC_INIT; > > + 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. An alternative to all this copying would be to take advantage of child_process.argv which is owned by the caller, thus does not get freed automatically by run_command(). This would allow you to re-use the same argument vector for all calls. And you don't need run_command_on_repo() at all. Something like this in cmd_for_each_repo(), untested and typed directly in email: struct child_process child = CHILD_PROCESS_INIT; for (i = 0; !result && i < values->nr; i++) { const char *d = chdir(values->items[i].string); if (chdir(d)) die_errno(_("cannot chdir to '%s'"), d); child.git_cmd = 1; child.argv = argv; result = run_command(&child); } This assumes that argv[] is correctly NULL-terminated after parse_options() -- I didn't check, but expect it to be so. If not, it's easy enough to copy argv[] into `args` once and then strvec_clear(&args) at the end of the function. The one downside is that trace output wouldn't be as helpful (I think) because you wouldn't see an explicit "-C <dir>", but I suppose the tracing machinery can be invoked manually to address this.
next prev parent reply other threads:[~2021-05-03 17: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 2021-05-03 17:01 ` Eric Sunshine [this message] 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=CAPig+cToeuq7S1176ze7kX9krjK1rGMOM-JDznF_Fr_qV09qWw@mail.gmail.com \ --to=sunshine@sunshineco.com \ --cc=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
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).