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

karsten.blees@dcon.de writes:

Junio C Hamano [off-list ref] wrote on 06.03.2012 21:19:06:

quoted

Johannes Sixt [off-list ref] writes:

quoted

quoted

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, 

quoted

quoted

quoted

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

quoted

quoted

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.

Junio C Hamano [off-list ref] wrote on 06.03.2012 22:49:57:

karsten.blees@dcon.de writes:

quoted

Junio C Hamano [off-list ref] wrote on 06.03.2012 21:19:06:

quoted

Johannes Sixt [off-list ref] writes:

quoted

quoted

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, 

quoted

quoted

quoted

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

quoted

quoted

quoted

pathname does not logically belong here anymore. It should be moved 

quoted

with

quoted

quoted

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

quoted

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)