diff mbox series

[5/9] builtin/repack.c: move `.idx` files into place last

Message ID 3b10a97ec0e7c9e672904e6415909a1b8cea872e.1631157880.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series packfile: avoid .idx rename races | expand

Commit Message

Taylor Blau Sept. 9, 2021, 3:25 a.m. UTC
In a similar spirit as the previous patch, fix the identical problem
from `git repack` (which invokes `pack-objects` with a temporary
location for output, and then moves the files into their final locations
itself).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Sept. 9, 2021, 7:38 p.m. UTC | #1
Taylor Blau <me@ttaylorr.com> writes:

> In a similar spirit as the previous patch, fix the identical problem
> from `git repack` (which invokes `pack-objects` with a temporary
> location for output, and then moves the files into their final locations
> itself).

OK.  This array is used in cmd_repack() to call rename() in the
order in which its elements appear, so moving the entry to the last
means it will be renamed the last.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/repack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 5f9bc74adc..c3e4771609 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -208,10 +208,10 @@ static struct {
>  	unsigned optional:1;
>  } exts[] = {
>  	{".pack"},
> -	{".idx"},
>  	{".rev", 1},
>  	{".bitmap", 1},
>  	{".promisor", 1},
> +	{".idx"},
>  };

And the .idx entry MUST stay the last.  I wonder if dropping the
trailing comma and replace it with a "/* must be at the end */"
comment would be an effective way to ensure that.
Taylor Blau Sept. 9, 2021, 9:08 p.m. UTC | #2
On Thu, Sep 09, 2021 at 12:38:52PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index 5f9bc74adc..c3e4771609 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -208,10 +208,10 @@ static struct {
> >  	unsigned optional:1;
> >  } exts[] = {
> >  	{".pack"},
> > -	{".idx"},
> >  	{".rev", 1},
> >  	{".bitmap", 1},
> >  	{".promisor", 1},
> > +	{".idx"},
> >  };
>
> And the .idx entry MUST stay the last.  I wonder if dropping the
> trailing comma and replace it with a "/* must be at the end */"
> comment would be an effective way to ensure that.

Heh, Ævar even wrote that up, and I responded that I did not think it
helped much. Again, I'm happy to add it back if anybody feels strongly.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/builtin/repack.c b/builtin/repack.c
index 5f9bc74adc..c3e4771609 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -208,10 +208,10 @@  static struct {
 	unsigned optional:1;
 } exts[] = {
 	{".pack"},
-	{".idx"},
 	{".rev", 1},
 	{".bitmap", 1},
 	{".promisor", 1},
+	{".idx"},
 };
 
 static unsigned populate_pack_exts(char *name)