[PATCH] prune: close directory earlier during loose-object directory traversal

[Johannes Sixt]

27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16)
introduced a new function for_each_loose_file_in_objdir() with a helper
for_each_file_in_obj_subdir(). The latter calls callbacks for each file
found during a directory traversal and finally also a callback for the
directory itself.

git-prune uses the function to clean up the object directory. In
particular, in the directory callback it calls rmdir(). On Windows XP,
this rmdir call fails, because the directory is still open while the
callback is called. Close the directory before calling the callback.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
Without this fix, t9300-fast-import hans on my Windows XP because
the git prune call is outside test_expect_success and waits for an
answer to the "should I try again" question. On my Windows 8.1
machine, this fix is not required. Does anybody know why this is
Should I send the fix upstream?

sha1_file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4d1b26f..5cecc68 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3473,12 +3473,12 @@ static int for_each_file_in_obj_subdir(int subdir_nr,
break;
}
}
- strbuf_setlen(path, baselen);
+ closedir(dir);

+ strbuf_setlen(path, baselen);
if (!r && subdir_cb)
r = subdir_cb(subdir_nr, path->buf, data);

- closedir(dir);
return r;
}

--
2.3.2.245.gb5bf9d3

[Johannes Sixt]

Here is the --patience version of the patch text:

diff --git a/sha1_file.c b/sha1_file.c
index 4d1b26f..5cecc68 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3473,12 +3473,12 @@ static int for_each_file_in_obj_subdir(int subdir_nr,
break;
}
}

+ closedir(dir);
+
strbuf_setlen(path, baselen);

-

[Johannes Schindelin]

Hi Johannes,

On 2015-08-11 22:44, Johannes Sixt wrote:
> 27e1e22d (prune: factor out loose-object directory traversal,
> 2014-10-16)
> introduced a new function for_each_loose_file_in_objdir() with a helper
> for_each_file_in_obj_subdir(). The latter calls callbacks for each file
> found during a directory traversal and finally also a callback for the
> directory itself.
>
> git-prune uses the function to clean up the object directory. In
> particular, in the directory callback it calls rmdir(). On Windows XP,
> this rmdir call fails, because the directory is still open while the
> callback is called. Close the directory before calling the callback.
>
> Signed-off-by: Johannes Sixt <j...@kdbg.org>
> ---
> Without this fix, t9300-fast-import hans on my Windows XP because
> the git prune call is outside test_expect_success and waits for an
> answer to the "should I try again" question. On my Windows 8.1
> machine, this fix is not required. Does anybody know why this is

I do not.

> Should I send the fix upstream?

Yes, please, with the patch formatted using --patience as you did in the
follow-up.

Thanks,
Johannes

[Johannes Sixt]

27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16)
introduced a new function for_each_loose_file_in_objdir() with a helper
for_each_file_in_obj_subdir(). The latter calls callbacks for each file
found during a directory traversal and finally also a callback for the
directory itself.

git-prune uses the function to clean up the object directory. In
particular, in the directory callback it calls rmdir(). On Windows XP,
this rmdir call fails, because the directory is still open while the
callback is called. Close the directory before calling the callback.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---

My Windows 8.1 machine does not require this fix for some unkonwn
reason. But we still cater for Windows XP users, where this change
is a real improvement.

sha1_file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4d1b26f..5cecc68 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3473,12 +3473,12 @@ static int for_each_file_in_obj_subdir(int subdir_nr,
break;
}
}

+ closedir(dir);
+
strbuf_setlen(path, baselen);

-

[Jeff King]

On Wed, Aug 12, 2015 at 07:43:01PM +0200, Johannes Sixt wrote:

> 27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16)
> introduced a new function for_each_loose_file_in_objdir() with a helper
> for_each_file_in_obj_subdir(). The latter calls callbacks for each file
> found during a directory traversal and finally also a callback for the
> directory itself.
>
> git-prune uses the function to clean up the object directory. In
> particular, in the directory callback it calls rmdir(). On Windows XP,
> this rmdir call fails, because the directory is still open while the
> callback is called. Close the directory before calling the callback.

Makes sense, and the patch looks good to me. Sorry for breaking things
on Windows.

Acked-by: Jeff King <pe...@peff.net>

-Peff

[Johannes Schindelin]

Hi,

On 2015-08-12 19:43, Johannes Sixt wrote:
> 27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16)
> introduced a new function for_each_loose_file_in_objdir() with a helper
> for_each_file_in_obj_subdir(). The latter calls callbacks for each file
> found during a directory traversal and finally also a callback for the
> directory itself.
>
> git-prune uses the function to clean up the object directory. In
> particular, in the directory callback it calls rmdir(). On Windows XP,
> this rmdir call fails, because the directory is still open while the
> callback is called. Close the directory before calling the callback.
>
> Signed-off-by: Johannes Sixt <j...@kdbg.org>
> ---
> My Windows 8.1 machine does not require this fix for some unkonwn
> reason. But we still cater for Windows XP users, where this change
> is a real improvement.

I believe that we have a concrete bug report for that:

https://github.com/git-for-windows/git/issues/231

Ciao,
Johannes

[Junio C Hamano]

Sorry for reviving this old thread, but I noticed that we do not
have this patch in our tree yet. I'll queue to 'pu' for now lest I
forget. If I missed a good argument or concensus against the change
please let me know, otherwise I'll fast track the change to 2.7 final

[Jeff King]

Ah, thanks for doing that. I noticed it when picking through "git branch
--no-merged pu" of your workspace a few weeks ago, but forgot to follow
up. I certainly have no objections.

-Peff

[Johannes Schindelin]

Hi Junio & Peff,

Git for Windows carries this patch since Git for Windows v2.5.0. So: no
objection from my side, either.

Ciao,
Dscho

[Junio C Hamano]

Johannes Schindelin <Johannes....@gmx.de> writes:

>> > Sorry for reviving this old thread, but I noticed that we do not
>> > have this patch in our tree yet. I'll queue to 'pu' for now lest I
>> > forget. If I missed a good argument or concensus against the change
>> > please let me know, otherwise I'll fast track the change to 2.7 final
>>
>> Ah, thanks for doing that. I noticed it when picking through "git branch
>> --no-merged pu" of your workspace a few weeks ago, but forgot to follow
>> up. I certainly have no objections.
>
> Git for Windows carries this patch since Git for Windows v2.5.0. So: no
> objection from my side, either.

Thanks!