karsten.blees@xxxxxxx writes:
> Junio C Hamano <gitster@xxxxxxxxx> wrote on 06.03.2012 21:19:06:
>> Johannes Sixt <j6t@xxxxxxxx> 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.
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html