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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2012, 21:49 UTC
Message-ID: <7vmx7tqu0q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <OFBC9E02C9.526DB40C-ONC12579B9.00726ECA-C12579B9.00763181@dcon.de>

karsten.blees@dcon.de writes:

Show 26 quoted lines

> Junio C Hamano <gitster@pobox.com> wrote on 06.03.2012 21:19:06:
>> 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, 
>> >> 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
>> > 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:

This depends entirely on how you look at it.

You can certainly stare at the original code and declare that the contract between the caller and the callee was that the caller gives pathname[] and len (len+3 for the caller) to the callee, and allows the callee to play with the rest of the pathname[] array but expects that pathname[] to be properly NUL-terminated when the callee comes back. From that point of view, "pathname[len] = 0" can belong to the callee.

But while you are staring the original code, notice that "expects that pathname[] to be NUL-terminated when the callee comes back" is not something the caller even depends on. That expectation starts to matter _only_ if you move rmdir(pathname) to the caller.

That is why I said "where it actually matters."

In other words, "The caller allows the callee to play with the rest of the pathname[]; as long as the callee does not touch earlier parts of the array, it can do anything before it returns", without requiring the callee to NUL-terminate the pathname[] to restore to its original state, is equally a valid contract between the caller and the callee in the original code.

And that also holds true for the updated code that has rmdir() in the caller.

Show 10 quoted lines

> 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')?

That is not necessary, I think, as get_sha1_hex() does not look at that NUL in the first place.