Re: [msysGit] [PATCH] fix deletion of .git/objects sub-directories in git-prune/repack

From: karsten.blees@dcon.de <karsten.blees@dcon.de>
Date: Mar 6, 2012, 21:30 UTC
Message-ID: <OFBC9E02C9.526DB40C-ONC12579B9.00726ECA-C12579B9.00763181@dcon.de>
In-Reply-To: <7v4nu1scsl.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote on 06.03.2012 21:19:06:

Show 7 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> >> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
> >> index f9463de..a834417 100644
> >> --- a/builtin/prune-packed.c
> >> +++ b/builtin/prune-packed.c
> >> @@ -36,7 +36,6 @@ static void prune_dir(int i, DIR *dir, char

*pathname,

Show 8 quoted lines

> >> int len, int opts)
> >>                 display_progress(progress, i + 1);
> >>         }
> >>         pathname[len] = 0;
> >> -       rmdir(pathname);
> >
> > After moving the rmdir() away from prune_dir(), the truncation of the
> > pathname does not logically belong here anymore. It should be moved

with

Show 5 quoted lines

> > the rmdir(). Looks good otherwise.
> 
> I agree that it is better to have the NUL termination close to where
> it actually matters.
>

The pathname is extended in prune_dir, so I think it should be reset there as well; moving it to prune_packed_objects would be quite obscure:

d = opendir(pathname); prune_dir(d, pathname); pathname[len] = 0; rmdir(pathname);

OT: While looking at the code I just stumbled across this immediately
above the patch (prune-packed.c line 32ff):

                memcpy(pathname + len, de->d_name, 38);
                if (opts & DRY_RUN)
                        printf("rm -f %s\n", pathname);
                else
                        unlink_or_warn(pathname);

Shouldn't this be memcpy(..., 39) (i.e. including '\0')?

> Do you want me to take it after locally fixing it up, or do you
> prefer to feed this as part of msysgit related updates to me later?

The msysgit queue is quite long, and I think it makes sense to fast-track this one.