[4/9] pack-write.c: rename `.idx` files after `*.rev`

Commit Message

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

We treat the presence of an `.idx` file as the indicator of whether or
not it's safe to use a packfile. But `finish_tmp_packfile()` (which is
responsible for writing the pack and moving the temporary versions of
all of its auxiliary files into place) is inconsistent about the write
order.

Specifically, it moves the `.rev` file into place after the `.idx`,
leaving open the possibility to open a pack which looks "ready" (because
the `.idx` file exists and is readable) but appears momentarily to not
have a `.rev` file. This causes Git to fall back to generating the
pack's reverse index in memory.

Though racy, this amounts to an unnecessary slow-down at worst, and
doesn't affect the correctness of the resulting reverse index.

Close this race by moving the .rev file into place before moving the
.idx file into place.

This still leaves the issue of `.idx` files being renamed into place
before the auxiliary `.bitmap` file is renamed when in pack-object.c's
write_pack_file() "write_bitmap_index" is true. That race will be
addressed in subsequent commits.

Signed-off-by: Taylor Blau <[e-mail]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Taylor Blau <[e-mail]>
---
 pack-write.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

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

On Wed, Sep 08 2021, Taylor Blau wrote:

> [...]
>
> Signed-off-by: Taylor Blau <[e-mail]>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Taylor Blau <[e-mail]>

git commit --amend --signed-off-by-harder ? :)

I.e. the duplicate header entry can go, maybe something Junio will fix
up while queuing...

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

On Thu, Sep 09, 2021 at 09:46:57AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Sep 08 2021, Taylor Blau wrote:
>
> > [...]
> >
> > Signed-off-by: Taylor Blau <[e-mail]>
> > Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> > Signed-off-by: Taylor Blau <[e-mail]>
>
> git commit --amend --signed-off-by-harder ? :)

I don't think so, though I had to check myself before sending.
Documentation/SubmittingPatches says:

    Notice that you can place your own `Signed-off-by` trailer when
    forwarding somebody else's patch with the above rules for
    D-C-O.  Indeed you are encouraged to do so.  Do not forget to
    place an in-body "From: " line at the beginning to properly attribute
    the change to its true author (see (2) above).

Here it's awkward because you modified my original patch and then I sent
your modified result out. But I think the s-o-b chain still makes sense:
I wrote the original patch, and signed it off. Then you modified it,
signing that off. Finally, I sent it to the list, signing off on your
modification.

> I.e. the duplicate header entry can go, maybe something Junio will fix
> up while queuing...

If I misread SubmittingPatches then I'm happy to fix it myself, but this
was the intended outcome.

Thanks,
Taylor

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

Taylor Blau <[e-mail]> writes:

> This still leaves the issue of `.idx` files being renamed into place
> before the auxiliary `.bitmap` file is renamed when in pack-object.c's
> write_pack_file() "write_bitmap_index" is true. That race will be
> addressed in subsequent commits.

OK.  I was about to suggest s/after .*rev/last/ on the title, but
the above makes it clear that we are not ready for such a title.

>
> Signed-off-by: Taylor Blau <[e-mail]>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Taylor Blau <[e-mail]>
> ---
>  pack-write.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pack-write.c b/pack-write.c
> index 95b063be94..077710090e 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -491,9 +491,9 @@ void finish_tmp_packfile(struct strbuf *name_buffer,
>  				      pack_idx_opts->flags);
>  
>  	rename_tmp_packfile(name_buffer, pack_tmp_name, "pack");
> -	rename_tmp_packfile(name_buffer, idx_tmp_name, "idx");
>  	if (rev_tmp_name)
>  		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
> +	rename_tmp_packfile(name_buffer, idx_tmp_name, "idx");
>  
>  	free((void *)idx_tmp_name);
>  }

Patch

diff --git a/pack-write.c b/pack-write.c
index 95b063be94..077710090e 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -491,9 +491,9 @@  void finish_tmp_packfile(struct strbuf *name_buffer,
 				      pack_idx_opts->flags);
 
 	rename_tmp_packfile(name_buffer, pack_tmp_name, "pack");
-	rename_tmp_packfile(name_buffer, idx_tmp_name, "idx");
 	if (rev_tmp_name)
 		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
+	rename_tmp_packfile(name_buffer, idx_tmp_name, "idx");
 
 	free((void *)idx_tmp_name);
 }