list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <>
To: Jeff King <>
Cc: Junio C Hamano <>, Taylor Blau <>,
Subject: Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way
Date: Mon, 16 Nov 2020 19:26:30 -0500	[thread overview]
Message-ID: <X7MYtjLQmmhfyHA5@nand.local> (raw)
In-Reply-To: <>

On Mon, Nov 16, 2020 at 07:02:52PM -0500, Jeff King wrote:
> I think you could make an argument either way:
>   - we have an existing bitmap for free, and bitmaps make things faster,
>     so why not keep it?
>   - the user did not ask for bitmaps, so we should make the outcome
>     consistent whether a pack of the exact name existed before or not
> The second one seems less surprising to me. But I think if we did the
> first, the code would be shorter (it would not need any of this "keep
> track of what pack-objects generated" stuff at all, but would just copy
> whatever files we see into place).

I think that the second one is _far_ less surprising to me, so I'd
prefer that we did that instead of keeping the .bitmap from the last run
regardless of whether or not the user asked for it.

> > >  				if (rename(fname_old, fname))
> > >  					die_errno(_("renaming '%s' failed"), fname_old);
> >
> > OK, so this is where the previous step matters.  We do the same as
> > before (i.e. stat+chmod and rename) only for paths we have created.
> >
> > We don't reuse the old one because we have already written a new
> > file so we won't save anything by doing so, and checking if the
> > target of rename exists already before deciding not to rename cannot
> > be made atomic, so just relying on rename() to do the right thing is
> > a good idea anyway.
> Even though the pack is byte-for-byte identical, the new .idx, etc,
> might not be. And those could be affected by options. E.g.:
>   git -c pack.writeBitmapHashCache=false repack -adb
>   git -c pack.writeBitmapHashCache=true  repack -adb
> should probably end up with a bitmap file that contains a hash cache.

Agreed completely.

> -Peff


  reply	other threads:[~2020-11-17  0:28 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-16 18:41 [PATCH 0/3] repack: don't move existing packs out of the way Taylor Blau
2020-11-16 18:41 ` [PATCH 1/3] repack: make "exts" array available outside cmd_repack() Taylor Blau
2020-11-16 18:41 ` [PATCH 2/3] builtin/repack.c: keep track of what pack-objects wrote Taylor Blau
2020-11-16 18:41 ` [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way Taylor Blau
2020-11-16 23:29   ` Junio C Hamano
2020-11-17  0:02     ` Jeff King
2020-11-17  0:26       ` Taylor Blau [this message]
2020-11-17  0:25     ` Taylor Blau
2020-11-17  0:46       ` Junio C Hamano
2020-11-17 20:15         ` Taylor Blau
2020-11-17 21:28           ` Junio C Hamano

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:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=X7MYtjLQmmhfyHA5@nand.local \ \ \ \ \

* 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

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).