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 08/18] maintenance: add prefetch task
Date: Fri, 24 Jul 2020 10:25:52 -0400 [thread overview]
Message-ID: <52fc3f5a-281c-a0cb-d897-3866315913a1@gmail.com> (raw)
In-Reply-To: <xmqqwo2uosvz.fsf@gitster.c.googlers.com>
On 7/23/2020 4:53 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
>> index 9204762e21..0927643247 100644
>> --- a/Documentation/git-maintenance.txt
>> +++ b/Documentation/git-maintenance.txt
>> @@ -53,6 +53,18 @@ since it will not expire `.graph` files that were in the previous
>> `commit-graph-chain` file. They will be deleted by a later run based on
>> the expiration delay.
>>
>> +prefetch::
>> + The `fetch` task updates the object directory with the latest objects
>
> s/fetch/prefetch/ most likely.
>
>> + from all registered remotes. For each remote, a `git fetch` command
>> + is run. The refmap is custom to avoid updating local or remote
>
> s/remote/remote-tracking/ definitely. Do not forget the hyphen
> between the two words.
>
> I think it made the above unnecessarily confusing that you ended a
> sentence after "is run". It gives a wrong impression that you'd be
> doing a "real fetch", which you need to dispel with a follow up
> description of the refmap.
>
> For each remote, a `git fetch` command is run with a refspec
> to fetch their branches (those in their `refs/heads`) into
> our `refs/prefetch/<remote>/` hierarchy and without auto
> following tags (the configured refspec in the repository is
> ignored).
>
>> + branches (those in `refs/heads` or `refs/remotes`). Instead, the
>> + remote refs are stored in `refs/prefetch/<remote>/`. Also, tags are
>> + not updated.
>> ++
>> +This means that foreground fetches are still required to update the
>> +remote refs, but the users is notified when the branches and tags are
>
> s/is notified/are notified/???
>
>> +updated on the remote.
>
> Often, when one needs to say "X. This means Y.", X is a suboptimal
> way to explain what needs to be conveyed to the readers. But this
> is not such a case. Unlike the "This means" that is often an
> attempt to rephrasing a poor explanation given first, this gives an
> implication.
>
> But let's not start with a negative impression (i.e. even with
> prefetch, I still have to do X? What's the point???), but let them
> feel why it is a good thing. Perhaps (continuing my previous
> rewrite):
>
> This is done to avoid disrupting the remote-tracking
> branches---the end users expect them to stay unmoved unless
> they initiate a fetch. With prefetch task, however, the
> objects necessary to complete a later real fetch would
> already be obtained, so the real fetch would go faster. In
> the ideal case, it will just become an update to bunch of
> remote-tracking branches without any object transfer.
>
> or something like that?
I like this clarification and have adapted it with minimal edits.
>> + argv_array_pushl(&cmd, "fetch", remote, "--prune",
>> + "--no-tags", "--refmap=", NULL);
>> + strbuf_addf(&refmap, "+refs/heads/*:refs/prefetch/%s/*", remote);
>> + argv_array_push(&cmd, refmap.buf);
>
> The command line looks somewhat fishy, but I think it is correct.
> At first glance it looks like a mistake to pass "--refmap=" and the
> refspec "+refs/heads/*:refs/prefetch/origin/*" as separate arguments,
> but I think that is exactly what you want here, i.e.
>
> - defeat any refspec in the configuration with "--refmap=<empty>"
>
> - give explicit refspec "+refs/heads/*:...", with "--no-tags" to
> decline auto-following, to tell what exactly are to be fetched
> and stored where.
>
> The description in the log message about refmap needs to be
> clarified, though (I've already done so in the above suggested
> rewrite).
I could have made your life easier by referring to b40a50264ac
(fetch: document and test --refmap="", 2020-01-21) in my commit
message. It includes this sentence in Documentation/fetch-options.txt:
Providing an empty `<refspec>` to the `--refmap` option causes
Git to ignore the configured refspecs and rely entirely on the
refspecs supplied as command-line arguments.
>> +static int maintenance_task_prefetch(void)
>> +{
>> + int result = 0;
>> + struct string_list_item *item;
>> + struct string_list remotes = STRING_LIST_INIT_DUP;
>> +
>> + if (for_each_remote(fill_each_remote, &remotes)) {
>> + error(_("failed to fill remotes"));
>> + result = 1;
>> + goto cleanup;
>> + }
>> +
>> + /*
>> + * Do not modify the result based on the success of the 'fetch'
>> + * operation, as a loss of network could cause 'fetch' to fail
>> + * quickly. We do not want that to stop the rest of our
>> + * background operations.
>> + */
>
> The loop that runs different tasks abort at the first failure,
> though. Perhaps that loop needs to be rethought as well?
You're right. These maintenance tasks are intended to be
independent of each other, so let's try all of them and
report a failure after all have been given an opportunity
to run. That makes this failure behavior unnecessary.
>> + for (item = remotes.items;
>> + item && item < remotes.items + remotes.nr;
>> + item++)
>> + fetch_remote(item->string);
>> +
>> +cleanup:
>> + string_list_clear(&remotes, 0);
>> + return result;
>> +}
>> +
>> static int maintenance_task_gc(void)
>> {
>> int result;
>> @@ -871,6 +929,10 @@ static void initialize_tasks(void)
>> for (i = 0; i < MAX_NUM_TASKS; i++)
>> tasks[i] = xcalloc(1, sizeof(struct maintenance_task));
>>
>> + tasks[num_tasks]->name = "prefetch";
>> + tasks[num_tasks]->fn = maintenance_task_prefetch;
>> + num_tasks++;
>> +
>> tasks[num_tasks]->name = "gc";
>> tasks[num_tasks]->fn = maintenance_task_gc;
>> tasks[num_tasks]->enabled = 1;
>
> Two things.
>
> - As I said upfront, I do not see the point of preparing the table
> with code.
>
> - The reason why prefetch is placed in front is probably because
> you do not want to repack before you add more objects to the
> object store. But doesn't that imply that there is an inherent
> ordering that we, as those who are more expert on Git than the
> end users, prefer? Is it a wise decision to let the users affect
> the order of the tasks run by giving command line options in
> different order in the previous step?
I don't anticipate users specifying --task=<task> very often, as
it requires deep knowledge of the tasks. If a user _does_ use the
option, then we should trust their order as they might have a
good reason to choose that order.
Generally, my philosophy is to provide expert users with flexible
choices while creating sensible defaults for non-expert users.
That said, if this sort is more of a problem than the value it
provides, then I can drop the sort and the duplicate --task=<task>
check.
Thanks,
-Stolee
next prev parent reply other threads:[~2020-07-24 14:26 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
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 [this message]
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=52fc3f5a-281c-a0cb-d897-3866315913a1@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).