From: Derrick Stolee <stolee@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes.Schindelin@gmx.de,
sandals@crustytoothpaste.net, steadmon@google.com,
jrnieder@gmail.com, peff@peff.net, congdanhqx@gmail.com,
phillip.wood123@gmail.com, emilyshaffer@google.com,
sluongng@gmail.com, jonathantanmy@google.com,
Derrick Stolee <derrickstolee@github.com>,
Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 03/18] maintenance: replace run_auto_gc()
Date: Thu, 30 Jul 2020 09:29:09 -0400 [thread overview]
Message-ID: <9aadb889-007a-c973-c59c-a471436368bb@gmail.com> (raw)
In-Reply-To: <xmqqblk6q8ws.fsf@gitster.c.googlers.com>
On 7/23/2020 4:21 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +--[no-]maintenance::
>> --[no-]auto-gc::
>> - Run `git gc --auto` at the end to perform garbage collection
>> - if needed. This is enabled by default.
>> + Run `git maintenance run --auto` at the end to perform garbage
>> + collection if needed. This is enabled by default.
>
> Shouldn't the new synonym be called --auto-maintenance or an
> abbreviation thereof? It is not like we will run the full
> maintenance suite when "--no-maintenance" is omitted, which
> certainly is not the impression we want to give our readers.
Adding "auto-" to the argument can be informative.
I would think that abbreviating the option may make understanding
the argument more difficult for users where English is not their
first language. Of course, I'm a bad judge of that.
I also don't think this option is called often by users at the
command-line and instead is done by scripts and third-party tools.
Abbreviating the word may have less value there.
(This reminds me that is would be good to have a config option that
prevents running this process _at all_. Sure, gc.auto=0 prevents the
command from doing anything, but there is still an extra cost of
starting a process. This is more of a problem on Windows. Making a
note to self here.)
>> These objects may be removed by normal Git operations (such as `git commit`)
>> -which automatically call `git gc --auto`. (See linkgit:git-gc[1].)
>> -If these objects are removed and were referenced by the cloned repository,
>> -then the cloned repository will become corrupt.
>> +which automatically call `git maintenance run --auto` and `git gc --auto`.
>
> Hmph. Perhaps the picture may change in the end of the series but I
> got an impression that "gc --auto" would eventually become just part
> of "maintenance --auto" and the users won't have to be even aware of
> its existence? Wouldn't we rather want to say something like
>
> --[no-]auto-maintenance::
> --[no-]auto-gc::
> Run `git maintenance run --auto` at the end to perform
> garbage collection if needed (`--[no-]auto-gc` is a
> synonym). This is enabled by default.
I don't completely eliminate 'git gc' at the end of this series, but
instead hope that we can peel it apart slowly in follow-up series.
However, you are correct that I should be more careful to obliterate
it from the documentation instead of keeping references around.
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 82ac4be8a5..49a4d727d4 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -196,8 +196,10 @@ static struct option builtin_fetch_options[] = {
>> OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>> N_("report that we have only objects reachable from this object")),
>> OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
>
>> + OPT_BOOL(0, "maintenance", &enable_auto_gc,
>> + N_("run 'maintenance --auto' after fetching")),
>> OPT_BOOL(0, "auto-gc", &enable_auto_gc,
>> + N_("run 'maintenance --auto' after fetching")),
>
> OK, so this is truly a backward-compatible synonym at this point.
>
>> diff --git a/run-command.c b/run-command.c
>> index 9b3a57d1e3..82ad241638 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -1865,14 +1865,17 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
>> return result;
>> }
>>
>> -int run_auto_gc(int quiet)
>> +int run_auto_maintenance(int quiet)
>> {
>> struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
>> int status;
>>
>> - argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
>> + argv_array_pushl(&argv_gc_auto, "maintenance", "run", "--auto", NULL);
>> if (quiet)
>> argv_array_push(&argv_gc_auto, "--quiet");
>> + else
>> + argv_array_push(&argv_gc_auto, "--no-quiet");
>> +
>> status = run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
>> argv_array_clear(&argv_gc_auto);
>> return status;
>
> Don't we want to replace all _gc_ with _maintenance_ in this
> function? I think the first business before we can do so would be
> to rethink if spelling out "maintenance" fully in code is a good
> idea in the first space. It would make names for variables,
> structures and fields unnecessarily long without contributing to
> ease of understanding an iota, and a easy-to-remember short-form or
> an abbreviation may be needed. Using a short-form/abbreviation
> wouldn't worsen the end-user experience, and not the developer
> experience for that matter.
>
> If we choose "gc" as the short-hand, most of the change in this step
> would become unnecessary. I also do not mind if we some other words
> or word-fragment (perhaps "maint"???) is chosen.
Yes, I should have noticed that. Also, with Peff's feedback from
another thread, the method can look a bit simpler this way:
int run_auto_maintenance(int quiet)
{
struct child_process maint = CHILD_PROCESS_INIT;
maint.git_cmd = 1;
argv_array_pushl(&maint.argv, "maintenance", "run", "--auto", NULL);
if (quiet)
argv_array_push(&maint.argv, "--quiet");
else
argv_array_push(&maint.argv, "--no-quiet");
return run_command(&maint.argv);
}
(I will update it again to work on Peff's argv_array work, but
hopefully it is clear how this is simpler.)
Thanks,
-Stolee
next prev parent reply other threads:[~2020-07-30 13:29 UTC|newest]
Thread overview: 164+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-07 14:21 [PATCH 00/21] Maintenance builtin, allowing 'gc --auto' customization Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 01/21] gc: use the_repository less often Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 02/21] gc: use repository in too_many_loose_objects() Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 03/21] gc: use repo config Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 04/21] gc: drop the_repository in log location Derrick Stolee via GitGitGadget
2020-07-09 2:22 ` Jonathan Tan
2020-07-09 11:13 ` Derrick Stolee
2020-07-07 14:21 ` [PATCH 05/21] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 06/21] maintenance: add --quiet option Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 07/21] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 08/21] maintenance: initialize task array and hashmap Derrick Stolee via GitGitGadget
2020-07-09 2:25 ` Jonathan Tan
2020-07-09 13:15 ` Derrick Stolee
2020-07-09 13:51 ` Junio C Hamano
2020-07-07 14:21 ` [PATCH 09/21] maintenance: add commit-graph task Derrick Stolee via GitGitGadget
2020-07-09 2:29 ` Jonathan Tan
2020-07-09 11:14 ` Derrick Stolee
2020-07-09 22:52 ` Jeff King
2020-07-09 23:41 ` Derrick Stolee
2020-07-07 14:21 ` [PATCH 10/21] maintenance: add --task option Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 11/21] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 12/21] maintenance: add fetch task Derrick Stolee via GitGitGadget
2020-07-09 2:35 ` Jonathan Tan
2020-07-07 14:21 ` [PATCH 13/21] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 14/21] maintenance: add pack-files task Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 15/21] maintenance: auto-size pack-files batch Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 16/21] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 17/21] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 18/21] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 19/21] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 20/21] maintenance: add pack-files auto condition Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 21/21] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-07-08 23:57 ` [PATCH 00/21] Maintenance builtin, allowing 'gc --auto' customization Emily Shaffer
2020-07-09 11:21 ` Derrick Stolee
2020-07-09 12:43 ` Derrick Stolee
2020-07-09 23:16 ` Jeff King
2020-07-09 23:45 ` Derrick Stolee
2020-07-10 18:46 ` Emily Shaffer
2020-07-10 19:30 ` Son Luong Ngoc
2020-07-09 14:05 ` Junio C Hamano
2020-07-09 15:54 ` Derrick Stolee
2020-07-09 16:26 ` Junio C Hamano
2020-07-09 16:56 ` Derrick Stolee
2020-07-23 17:56 ` [PATCH v2 00/18] " Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 01/18] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget
2020-07-25 1:26 ` Taylor Blau
2020-07-25 1:47 ` Đoàn Trần Công Danh
2020-07-29 22:19 ` Jonathan Nieder
2020-07-30 13:12 ` Derrick Stolee
2020-07-31 0:30 ` Jonathan Nieder
2020-08-03 17:37 ` Derrick Stolee
2020-08-03 17:46 ` Jonathan Nieder
2020-08-03 22:46 ` Taylor Blau
2020-08-03 23:01 ` Jonathan Nieder
2020-08-03 23:08 ` Taylor Blau
2020-08-03 23:17 ` Jonathan Nieder
2020-08-04 0:07 ` Junio C Hamano
2020-08-04 13:32 ` Derrick Stolee
2020-08-04 14:42 ` Jonathan Nieder
2020-08-04 16:32 ` Derrick Stolee
2020-08-04 17:02 ` Jonathan Nieder
2020-08-04 17:51 ` Derrick Stolee
2020-08-05 15:02 ` Derrick Stolee
2020-07-31 16:40 ` Jonathan Nieder
2020-08-03 23:52 ` Jonathan Nieder
2020-07-23 17:56 ` [PATCH v2 02/18] maintenance: add --quiet option Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 03/18] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget
2020-07-23 20:21 ` Junio C Hamano
2020-07-25 1:33 ` Taylor Blau
2020-07-30 13:29 ` Derrick Stolee [this message]
2020-07-30 13:31 ` Derrick Stolee
2020-07-30 19:00 ` Eric Sunshine
2020-07-30 20:21 ` Derrick Stolee
2020-07-23 17:56 ` [PATCH v2 04/18] maintenance: initialize task array Derrick Stolee via GitGitGadget
2020-07-23 19:57 ` Junio C Hamano
2020-07-24 12:23 ` Derrick Stolee
2020-07-24 12:51 ` Derrick Stolee
2020-07-24 19:39 ` Junio C Hamano
2020-07-25 1:46 ` Taylor Blau
2020-07-29 22:19 ` Emily Shaffer
2020-07-23 17:56 ` [PATCH v2 05/18] maintenance: add commit-graph task Derrick Stolee via GitGitGadget
2020-07-23 20:22 ` Junio C Hamano
2020-07-24 13:09 ` Derrick Stolee
2020-07-24 19:47 ` Junio C Hamano
2020-07-25 1:52 ` Taylor Blau
2020-07-30 13:59 ` Derrick Stolee
2020-07-29 0:22 ` Jeff King
2020-07-23 17:56 ` [PATCH v2 06/18] maintenance: add --task option Derrick Stolee via GitGitGadget
2020-07-23 20:21 ` Junio C Hamano
2020-07-23 22:18 ` Junio C Hamano
2020-07-24 13:36 ` Derrick Stolee
2020-07-24 19:50 ` Junio C Hamano
2020-07-23 17:56 ` [PATCH v2 07/18] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 08/18] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-07-23 20:53 ` Junio C Hamano
2020-07-24 14:25 ` Derrick Stolee
2020-07-24 20:47 ` Junio C Hamano
2020-07-25 1:37 ` Đoàn Trần Công Danh
2020-07-25 1:48 ` Junio C Hamano
2020-07-27 14:07 ` Derrick Stolee
2020-07-27 16:13 ` Junio C Hamano
2020-07-27 18:27 ` Derrick Stolee
2020-07-28 16:37 ` [PATCH v2] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano
2020-07-29 9:12 ` Phillip Wood
2020-07-29 9:17 ` Phillip Wood
2020-07-30 15:17 ` Derrick Stolee
2020-07-23 17:56 ` [PATCH v2 09/18] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-07-23 20:59 ` Junio C Hamano
2020-07-24 14:50 ` Derrick Stolee
2020-07-24 19:57 ` Junio C Hamano
2020-07-29 22:21 ` Emily Shaffer
2020-07-30 15:38 ` Derrick Stolee
2020-07-23 17:56 ` [PATCH v2 10/18] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-07-23 22:00 ` Junio C Hamano
2020-07-24 15:03 ` Derrick Stolee
2020-07-29 22:22 ` Emily Shaffer
2020-07-23 17:56 ` [PATCH v2 11/18] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-07-23 22:15 ` Junio C Hamano
2020-07-23 23:09 ` Eric Sunshine
2020-07-23 23:24 ` Junio C Hamano
2020-07-24 16:09 ` Derrick Stolee
2020-07-24 19:51 ` Derrick Stolee
2020-07-24 20:17 ` Junio C Hamano
2020-07-29 22:23 ` Emily Shaffer
2020-07-30 16:57 ` Derrick Stolee
2020-07-30 19:02 ` Derrick Stolee
2020-07-30 19:24 ` Chris Torek
2020-08-05 12:37 ` Đoàn Trần Công Danh
2020-08-06 13:54 ` Derrick Stolee
2020-07-23 17:56 ` [PATCH v2 12/18] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 13/18] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 14/18] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 15/18] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 16/18] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 17/18] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-07-23 17:56 ` [PATCH v2 18/18] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget
2020-07-29 22:03 ` [PATCH v2 00/18] Maintenance builtin, allowing 'gc --auto' customization Emily Shaffer
2020-07-30 22:24 ` [PATCH v3 00/20] " Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 01/20] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 02/20] maintenance: add --quiet option Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 03/20] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 04/20] maintenance: initialize task array Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 05/20] maintenance: add commit-graph task Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 06/20] maintenance: add --task option Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 07/20] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 08/20] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 09/20] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 10/20] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 11/20] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 12/20] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 13/20] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-07-30 23:36 ` Chris Torek
2020-08-03 17:43 ` Derrick Stolee
2020-07-30 22:24 ` [PATCH v3 14/20] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 15/20] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 16/20] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 17/20] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 18/20] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 19/20] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-07-30 22:24 ` [PATCH v3 20/20] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget
2020-07-30 23:06 ` [PATCH v3 00/20] Maintenance builtin, allowing 'gc --auto' customization Junio C Hamano
2020-07-30 23:31 ` Junio C Hamano
2020-07-31 2:58 ` Junio C Hamano
2020-08-06 17:58 ` Derrick Stolee
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=9aadb889-007a-c973-c59c-a471436368bb@gmail.com \
--to=stolee@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=congdanhqx@gmail.com \
--cc=derrickstolee@github.com \
--cc=dstolee@microsoft.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
--cc=phillip.wood123@gmail.com \
--cc=sandals@crustytoothpaste.net \
--cc=sluongng@gmail.com \
--cc=steadmon@google.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public 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).