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

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

[karsten.blees]

On some systems (e.g. Windows XP), directories cannot be deleted while
they're open. Both git-prune and git-repack (and thus, git-gc) try to
rmdir while holding a DIR* handle on the directory, leaving dangling
empty directories in the .git/objects store.

Fix it by swapping the rmdir / closedir calls.

Reported-by: John Chen <john0312@gmail.com>
Reported-by: Stefan Naewe <stefan.naewe@gmail.com>
Signed-off-by: Karsten Blees <blees@dcon.de>
---

Erik Faye-Lund <kusmabite@gmail.com> wrote on 05.03.2012 11:26:08:
> On Mon, Mar 5, 2012 at 10:57 AM,  <karsten.blees@dcon.de> wrote:
[...]
> > I thought the delete / rename problems were a Windows illness (at
> > least the retry-ask-user-yes-no logic is exclusive to mingw.c)?
> 
> POSIX allows for this behavior:
> 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/rmdir.html
> 
> (EBUSY: "The directory to be removed is currently in use by the system
> or some process and the implementation considers this to be an
> error.")
> 
> I've found traces of similar issues on exotic unices, so I don't think
> it's completely hypothetical either...
> 
> > And as all mingw stuff
> > is eventually sent upstream (I hope :-)), I don't see a particular
> > reason to rush this one.
> >
> 
> There's no automatic system for this. I tend to send all my patches
> that cleanly fix problems present in git.git directly upstream, so
> people on Windows who doesn't use our branch can benefit from them as
> well.

So what about this one? It applies cleanly to git master, and when
merged / cherry-picked to msysgit devel, the prune-packed.c fix we
already have in b4cb824d is silently ignored.

Note: the is_dir_empty() function in mingw.c is broken in upstream git
as well, but I'm reluctant to backport this as it will clash with the
Unicode patches.

The upstream patch can be pulled from here:
https://github.com/kblees/git/commit/56e1bc62

And cherry-picked to msysgit devel from here:
https://github.com/kblees/git/commit/b07bfd51

Regards,
Karsten


 builtin/prune-packed.c |    2 +-
 builtin/prune.c        |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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);
 }
 
 void prune_packed_objects(int opts)
@@ -65,6 +64,7 @@ void prune_packed_objects(int opts)
                        continue;
                prune_dir(i, d, pathname, len + 3, opts);
                closedir(d);
+               rmdir(pathname);
        }
        stop_progress(&progress);
 }
diff --git a/builtin/prune.c b/builtin/prune.c
index 58d7cb8..b99b635 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -85,9 +85,9 @@ static int prune_dir(int i, char *path)
                }
                fprintf(stderr, "bad sha1 file: %s/%s\n", path, 
de->d_name);
        }
+       closedir(dir);
        if (!show_only)
                rmdir(path);
-       closedir(dir);
        return 0;
 }
 
-- 
1.7.9.msysgit.1.364.g3e2096

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

[Johannes Sixt]

Am 06.03.2012 10:18, schrieb karsten.blees@dcon.de:
> On some systems (e.g. Windows XP), directories cannot be deleted while
> they're open. Both git-prune and git-repack (and thus, git-gc) try to
> rmdir while holding a DIR* handle on the directory, leaving dangling
> empty directories in the .git/objects store.
> 
> Fix it by swapping the rmdir / closedir calls.

The reasoning makes a lot of sense. I wonder why object directories are
pruned nevertheless when I run git gc --prune (I run git master plus a
few topics from pu).

> 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.

>  }
>  
>  void prune_packed_objects(int opts)
> @@ -65,6 +64,7 @@ void prune_packed_objects(int opts)
>                         continue;
>                 prune_dir(i, d, pathname, len + 3, opts);
>                 closedir(d);
> +               rmdir(pathname);
>         }
>         stop_progress(&progress);
>  }
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 58d7cb8..b99b635 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -85,9 +85,9 @@ static int prune_dir(int i, char *path)
>                 }
>                 fprintf(stderr, "bad sha1 file: %s/%s\n", path, 
> de->d_name);
>         }
> +       closedir(dir);
>         if (!show_only)
>                 rmdir(path);
> -       closedir(dir);
>         return 0;
>  }
>  

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

[Junio C Hamano]

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.

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?

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

[Johannes Sixt]

Am 06.03.2012 21:19, schrieb Junio C Hamano:
> 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.
> 
> 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?

This patch is fairly independent from other topics that are queued in
msysgit, IIRC. Please take it with the mentioned fixup.

Thanks,
-- Hannes

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

[karsten.blees]

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:

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.

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

[karsten.blees]

Johannes Sixt <j6t@kdbg.org> wrote on 06.03.2012 19:32:41:
> Am 06.03.2012 10:18, schrieb karsten.blees@dcon.de:
> > On some systems (e.g. Windows XP), directories cannot be deleted while
> > they're open. Both git-prune and git-repack (and thus, git-gc) try to
> > rmdir while holding a DIR* handle on the directory, leaving dangling
> > empty directories in the .git/objects store.
> > 
> > Fix it by swapping the rmdir / closedir calls.
> 
> The reasoning makes a lot of sense. I wonder why object directories are
> pruned nevertheless when I run git gc --prune (I run git master plus a
> few topics from pu).
> 

You're right...in upstream git, closedir() is essentially a NOOP. The OS
handle is closed in readdir() on reading the last entry. The dirent.c
'improvements' that move FindFirstFile() to opendir() and FindClose() to
closedir() are in the msysgit fork only [1] (and entirely my fault, I
guess :-)).

So this patch actually isn't currently required for Windows XP (probably
other platforms, I don't know).

[1] https://github.com/msysgit/git/commit/e2b3f70b

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***

*** Please avoid top-posting. ***

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

[Junio C Hamano]

karsten.blees@dcon.de writes:

> 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.

> 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.

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

[Junio C Hamano]

karsten.blees@dcon.de writes:

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

I think the only thing that is guaranteeing that pathname[len+38] is
NUL is that we do not hop around repositories, so once we fill the
static char pathname[PATH_MAX] in prune_packed_objects(), nobody
writes to that location, because the length of the leading part
(i.e. "len" given to prune_dir()) will stay constant during the
lifetime of the process.

So it is not currently a problem, but it would be better to clear
that byte here.

Good eyes.

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

[karsten.blees]

Junio C Hamano <gitster@pobox.com> wrote on 06.03.2012 22:49:57:
> karsten.blees@dcon.de writes:
> 
> > 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."
> 

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)