| Message ID | c3f16bccd023601bb1d041c36cf5f49011abcb76.1630359290.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Upstreaming the Scalar command | expand |
On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote: > + const char *usagestr[] = { NULL, NULL }; Missing usage strings? > + if (argc == 0) Style nit (per style guide): s/argc == 0/!argc/g. > + if (!strcmp("all", argv[0])) > + i = -1; Style nit (per style guide): missing braces here. (Just noting the style nits once, but more in this patch, and presumably the rest of the series...)
Hi Ævar, On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote: > On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote: > > > + const char *usagestr[] = { NULL, NULL }; > > Missing usage strings? This command will show a generated usage, i.e. a non-static string. It therefore cannot be specified here already. See the `strbuf_*()` calls populating `buf` and the `usagestr[0] = buf.buf;` assignment. > > + if (argc == 0) > > Style nit (per style guide): s/argc == 0/!argc/g. It is true that we often do this, but in this instance it would be misleading: `argc` is a counter, not a Boolean. > > + if (!strcmp("all", argv[0])) > > + i = -1; > > Style nit (per style guide): missing braces here. The style guide specifically allows my preference to leave single-line blocks without curlies. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Ævar, > > On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote: > >> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote: >> >> > + const char *usagestr[] = { NULL, NULL }; >> >> Missing usage strings? > > This command will show a generated usage, i.e. a non-static string. It > therefore cannot be specified here already. See the `strbuf_*()` calls > populating `buf` and the `usagestr[0] = buf.buf;` assignment. > >> > + if (argc == 0) >> >> Style nit (per style guide): s/argc == 0/!argc/g. > > It is true that we often do this, but in this instance it would be > misleading: `argc` is a counter, not a Boolean. That argument could be a plausible excuse to deviate from the style if it were if (argc == 0) do no args case; else if (argc == 1) do one arg case; else if (argc == 2) do two args case; ... Replacing the first one with "if (!argc)" may make it less readable. But I do not think the reasoning applies here if (argc == 0) do a thing that applies only to no args case; without "else". This is talking about "do we have any argument? Yes or no?" Boolean here. >> > + if (!strcmp("all", argv[0])) >> > + i = -1; >> >> Style nit (per style guide): missing braces here. > > The style guide specifically allows my preference to leave single-line > blocks without curlies. Actually, the exception goes the other way, no? We generally want to avoid such an unnecessary braces around a single statement block. But when we have an else clause that has a block with multiple statements (hence braces are required), as an exception, the guide asks you to write braces around the body of the if side for consistency. When you only have just a couple of lines on the "else {}" side, I do not think it matters too much either way for readability, though. I cannot see the "else" side in the above clause, but IIRC it wasn't just a few lines, was it? Thanks.
Hi Junio, On Fri, 3 Sep 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Hi Ævar, > > > > On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote: > > > >> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote: > >> > >> > + const char *usagestr[] = { NULL, NULL }; > >> > >> Missing usage strings? > > > > This command will show a generated usage, i.e. a non-static string. It > > therefore cannot be specified here already. See the `strbuf_*()` calls > > populating `buf` and the `usagestr[0] = buf.buf;` assignment. > > > >> > + if (argc == 0) > >> > >> Style nit (per style guide): s/argc == 0/!argc/g. > > > > It is true that we often do this, but in this instance it would be > > misleading: `argc` is a counter, not a Boolean. > > That argument could be a plausible excuse to deviate from the style > if it were > > if (argc == 0) > do no args case; > else if (argc == 1) > do one arg case; > else if (argc == 2) > do two args case; > ... > > Replacing the first one with "if (!argc)" may make it less readable. > > But I do not think the reasoning applies here > > if (argc == 0) > do a thing that applies only to no args case; > > without "else". This is talking about "do we have any argument? Yes > or no?" Boolean here. Well, I offer a differing opinion. But you're right, we are at least consistent in Git's source code in using `!i` where other projects would use `i == 0`, and consistency is definitely something I'd like to see more in Git, not less. So I changed it as you suggested. > > >> > + if (!strcmp("all", argv[0])) > >> > + i = -1; > >> > >> Style nit (per style guide): missing braces here. > > > > The style guide specifically allows my preference to leave single-line > > blocks without curlies. > > Actually, the exception goes the other way, no? > > We generally want to avoid such an unnecessary braces around a > single statement block. But when we have an else clause that has a > block with multiple statements (hence braces are required), as an > exception, the guide asks you to write braces around the body of the > if side for consistency. You're right. I am somehow still using the previous style where we _required_ single-line blocks _not_ to have curly brackets (see e.g. aa1c48df817 ([PATCH] ls-tree enhancements, 2005-04-15), the `else` part of the added `if (! eltbuf)` block). > > When you only have just a couple of lines on the "else {}" side, I > do not think it matters too much either way for readability, though. > I cannot see the "else" side in the above clause, but IIRC it wasn't > just a few lines, was it? It depends what you count as "just a few lines". There are seven lines enclosed within the curly brackets of the `else` block. But as much as I enjoy thorough reviews of the Scalar code, I am failing at getting excited about code style discussions, therefore I simply went with your suggestion to enclose even the single-line block in curly brackets. Thanks, Dscho
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 908eaa84df1..d5d38a1afeb 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -490,6 +490,69 @@ static int cmd_register(int argc, const char **argv) return register_dir(); } +static int cmd_run(int argc, const char **argv) +{ + struct option options[] = { + OPT_END(), + }; + struct { + const char *arg, *task; + } tasks[] = { + { "config", NULL }, + { "commit-graph", "commit-graph" }, + { "fetch", "prefetch" }, + { "loose-objects", "loose-objects" }, + { "pack-files", "incremental-repack" }, + { NULL, NULL } + }; + struct strbuf buf = STRBUF_INIT; + const char *usagestr[] = { NULL, NULL }; + int i; + + strbuf_addstr(&buf, N_("scalar run <task> [<enlistment>]\nTasks:\n")); + for (i = 0; tasks[i].arg; i++) + strbuf_addf(&buf, "\t%s\n", tasks[i].arg); + usagestr[0] = buf.buf; + + argc = parse_options(argc, argv, NULL, options, + usagestr, 0); + + if (argc == 0) + usage_with_options(usagestr, options); + + if (!strcmp("all", argv[0])) + i = -1; + else { + for (i = 0; tasks[i].arg && strcmp(tasks[i].arg, argv[0]); i++) + ; /* keep looking for the task */ + + if (i > 0 && !tasks[i].arg) { + error(_("no such task: '%s'"), argv[0]); + usage_with_options(usagestr, options); + } + } + + argc--; + argv++; + setup_enlistment_directory(argc, argv, usagestr, options, NULL); + strbuf_release(&buf); + + if (i == 0) + return register_dir(); + + if (i > 0) + return run_git("maintenance", "run", + "--task", tasks[i].task, NULL); + + if (register_dir()) + return -1; + for (i = 1; tasks[i].arg; i++) + if (run_git("maintenance", "run", + "--task", tasks[i].task, NULL)) + return -1; + return 0; +} + static int remove_deleted_enlistment(struct strbuf *path) { int res = 0; @@ -562,6 +625,7 @@ static struct { { "list", cmd_list }, { "register", cmd_register }, { "unregister", cmd_unregister }, + { "run", cmd_run }, { NULL, NULL}, }; diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt index bb9411b38cb..9aadaf6323f 100644 --- a/contrib/scalar/scalar.txt +++ b/contrib/scalar/scalar.txt @@ -12,6 +12,7 @@ scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<e scalar list scalar register [<enlistment>] scalar unregister [<enlistment>] +scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>] DESCRIPTION ----------- @@ -94,6 +95,24 @@ unregister [<enlistment>]:: Remove the specified repository from the list of repositories registered with Scalar and stop the scheduled background maintenance. +Run +~~~ + +scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]:: + Run the given maintenance task (or all tasks, if `all` was specified). + Except for `all` and `config`, this subcommand simply hands off to + linkgit:git-maintenance[1] (mapping `fetch` to `prefetch` and + `pack-files` to `incremental-repack`). ++ +These tasks are run automatically as part of the scheduled maintenance, +as soon as the repository is registered with Scalar. It should therefore +not be necessary to run this subcommand manually. ++ +The `config` task is specific to Scalar and configures all those +opinionated default settings that make Git work more efficiently with +large repositories. As this task is run as part of `scalar clone` +automatically, explicit invocations of this task are rarely needed. + SEE ALSO -------- linkgit:git-clone[1], linkgit:git-maintenance[1].