threads / patch / 29855

patchfix deletion of .git/objects sub-directories in git-prune/repack

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

## tl;dr

9 messages between Mar 6, 2012 and Mar 7, 2012. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

karsten.blees@dcon.de· Mar 6, 2012, 09:18 UTC · lore

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:
[...]
Show 23 quoted lines
> > 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(-)
Show changes to 2 files +2 −2

builtin/prune-packed.c, builtin/prune.c

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
Johannes Sixt· Mar 6, 2012, 18:32 UTC · re: karsten.blees@dcon.de · lore

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

Am 06.03.2012 10:18, schrieb karsten.blees@dcon.de:
Show 6 quoted lines
> 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).

Show 10 quoted lines
> 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.

Show 27 quoted lines
>  }
>  
>  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;
>  }
>
Junio C Hamano· Mar 6, 2012, 20:19 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:
Show 14 quoted lines
>> 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?

Johannes Sixt· Mar 6, 2012, 21:19 UTC · re: Junio C Hamano · lore

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

Am 06.03.2012 21:19, schrieb Junio C Hamano:
Show 22 quoted lines
> 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

karsten.blees@dcon.de· Mar 6, 2012, 21:30 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Mar 6, 2012, 21:49 UTC · re: karsten.blees@dcon.de · lore

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

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.

karsten.blees@dcon.de· Mar 7, 2012, 10:50 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote on 06.03.2012 22:49:57:
Show 18 quoted lines
> 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
Show 8 quoted lines
> > 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
Show 19 quoted lines
> > 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)

Junio C Hamano· Mar 6, 2012, 21:57 UTC · re: karsten.blees@dcon.de · lore

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

karsten.blees@dcon.de writes:
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')?

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.
karsten.blees@dcon.de· Mar 6, 2012, 21:47 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> wrote on 06.03.2012 19:32:41:
Show 12 quoted lines
> 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

← back to recent threads