Junio C Hamano <gitster@xxxxxxxxx> wrote on 06.03.2012 22:49:57: > 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." > Well, I just don't like that a function designed to prune a directory modifies its input parameters as a side effect (it is bad enough that prune_dir uses the caller's buffer at all). You know, trying to limit side effects as a general programming principle. In my opinion, it doesn't matter at all if the caller actually depends on unmodified parameters or not, it just makes robust APIs that encourage reuse. Just my 2 cents, though, prune_packed_objects and prune_dir are so tightly coupled that it probably doesn't make any difference...(on the other hand, we have near identical code in prune.c, so thinking about reuse is not so far fetched) -- 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