git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Son Luong Ngoc <sluongng@gmail.com>
To: gitgitgadget@gmail.com
Cc: dstolee@microsoft.com, git@vger.kernel.org, jrnieder@google.com,
	peff@peff.net, stolee@gmail.com
Subject: Re: [PATCH 05/15] run-job: implement pack-files job
Date: Sat, 2 May 2020 09:56:50 +0200	[thread overview]
Message-ID: <CAL3xRKfcKh=XxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f+A@mail.gmail.com> (raw)

Hi Derrick,

Sorry for another late reply on this RFC.
This time is a question on the multi-pack-index repack process.

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/builtin/run-job.c b/builtin/run-job.c
> index cecf9058c51..d3543f7ccb9 100644
> --- a/builtin/run-job.c
> +++ b/builtin/run-job.c

...

> +static int multi_pack_index_repack(void)
> +{
> + int result;
> + struct argv_array cmd = ARGV_ARRAY_INIT;
> + argv_array_pushl(&cmd, "multi-pack-index", "repack",
> + "--no-progress", "--batch-size=0", NULL);
> + result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
> +
> + if (result && multi_pack_index_verify()) {
> + warning(_("multi-pack-index verify failed after repack"));
> + result = rewrite_multi_pack_index();
> + }

Its a bit inconsistent where write() and expire() did not include
verify() within them
but repack does. What make repack() different?

> +
> + return result;
> +}
> +
> +static int run_pack_files_job(void)
> +{
> + if (multi_pack_index_write()) {
> + error(_("failed to write multi-pack-index"));
> + return 1;
> + }
> +
> + if (multi_pack_index_verify()) {
> + warning(_("multi-pack-index verify failed after initial write"));
> + return rewrite_multi_pack_index();
> + }
> +
> + if (multi_pack_index_expire()) {
> + error(_("multi-pack-index expire failed"));
> + return 1;
> + }

Why expire *before* repack and not after?

I thought `core.multiPackIndex=true` would prevent old pack files from
being used thus expiring immediately after repack is safe? (on that
note, are users
required to set this config prior to running the job?)

If expiring immediately after repack()+verify() is not safe, then should
we have a minimum allowed interval set? (although I would preferred to
make expire() safe)

> +
> + if (multi_pack_index_verify()) {
> + warning(_("multi-pack-index verify failed after expire"));
> + return rewrite_multi_pack_index();
> + }
> +
> + if (multi_pack_index_repack()) {
> + error(_("multi-pack-index repack failed"));
> + return 1;
> + }

Again, I just think the decision to include verify() inside repack()
made this part a bit inconsistent.

> +
> + return 0;
> +}
> +

Cheers,
Son Luong

             reply	other threads:[~2020-05-02  7:57 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-02  7:56 Son Luong Ngoc [this message]
2020-05-04 14:34 ` [PATCH 05/15] run-job: implement pack-files job Derrick Stolee
  -- strict thread matches above, loose matches on Subject: below --
2020-04-03 20:47 [PATCH 00/15] [RFC] Maintenance jobs and job runner Derrick Stolee via GitGitGadget
2020-04-03 20:48 ` [PATCH 05/15] run-job: implement pack-files job Derrick Stolee via GitGitGadget
2020-05-27 22:17   ` Josh Steadmon

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='CAL3xRKfcKh=XxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f+A@mail.gmail.com' \
    --to=sluongng@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jrnieder@google.com \
    --cc=peff@peff.net \
    --cc=stolee@gmail.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).