[0/9] packfile: avoid .idx rename races

Message

Taylor Blau Sept. 9, 2021, 3:24 a.m. UTC

This series is a unification of [1] and [2] which together prevent a handful of
races when code that writes packfiles moves the `.idx` into place before all
other auxiliary files are properly renamed.

This can lead to races like not reading the `.rev` file even if one was
generated and is about to be moved into place.

Credit goes to Ævar for preparing what is more-or-less sent here. I polished a
few of the commit messages, and added the second patch on top of his result. It
isn't necessary, but felt like good hygiene when I was reading the surrounding
code.

Thanks in advance for reviewing.

[1]: https://lore.kernel.org/git/[e-mail]/
[2]: https://lore.kernel.org/git/[e-mail]/

Taylor Blau (4):
  bulk-checkin.c: store checksum directly
  pack-write.c: rename `.idx` files after `*.rev`
  builtin/repack.c: move `.idx` files into place last
  builtin/index-pack.c: move `.idx` files into place last

Ævar Arnfjörð Bjarmason (5):
  pack.h: line-wrap the definition of finish_tmp_packfile()
  pack-write: refactor renaming in finish_tmp_packfile()
  index-pack: refactor renaming in final()
  pack-write: split up finish_tmp_packfile() function
  pack-objects: rename .idx files into place after .bitmap files

 builtin/index-pack.c   | 48 +++++++++++++++++------------------
 builtin/pack-objects.c | 15 ++++++++---
 builtin/repack.c       |  2 +-
 bulk-checkin.c         | 31 +++++++++++++++++------
 pack-write.c           | 57 +++++++++++++++++++++---------------------
 pack.h                 | 10 +++++++-
 6 files changed, 96 insertions(+), 67 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Sept. 9, 2021, 8:06 a.m. UTC | #1

On Wed, Sep 08 2021, Taylor Blau wrote:

> This series is a unification of [1] and [2] which together prevent a handful of
> races when code that writes packfiles moves the `.idx` into place before all
> other auxiliary files are properly renamed.
>
> This can lead to races like not reading the `.rev` file even if one was
> generated and is about to be moved into place.
>
> Credit goes to Ævar for preparing what is more-or-less sent here. I polished a
> few of the commit messages, and added the second patch on top of his result. It
> isn't necessary, but felt like good hygiene when I was reading the surrounding
> code.
>
> Thanks in advance for reviewing.

Thanks a lot!

I think it's probably redundant to note it at this point but I've given
this a thorough review and it all looks good to me. I left some
comments/musings on minor points in the series, but none of those IMO
require a re-roll except perhaps the duplicate Signed-off-by headers,
depending on what Junio thinks & if he's going to fix those while
queuing.

Taylor Blau Sept. 9, 2021, 2:40 p.m. UTC | #2

On Thu, Sep 09, 2021 at 10:06:24AM +0200, Ævar Arnfjörð Bjarmason wrote:
> I think it's probably redundant to note it at this point but I've given
> this a thorough review and it all looks good to me. I left some
> comments/musings on minor points in the series, but none of those IMO
> require a re-roll except perhaps the duplicate Signed-off-by headers,
> depending on what Junio thinks & if he's going to fix those while
> queuing.

Thanks, and likewise your parts of this series look good to me
(obviously, or I wouldn't have sent it otherwise ;)).

The duplicate s-o-b's are intentional, but see my response in 2/9 (I'd
link to it, but vger seems to be a little sluggish for me this morning)
for why. If I made an error there, I'm happy to fix it up for Junio.

Thanks,
Taylor

Junio C Hamano Sept. 9, 2021, 7:52 p.m. UTC | #3

Taylor Blau <[e-mail]> writes:

> The duplicate s-o-b's are intentional, but see my response in 2/9 (I'd
> link to it, but vger seems to be a little sluggish for me this morning)

Looks good to me.  Thanks.