[PATCH v15 00/26] Issue 80: Unicode support on Windows
karste...@dcon.de
here is v15 of the unicode patch series:
git: https://github.com/kblees/git/tree/kb/unicode-v15
less-444 (unchanged):
https://github.com/kblees/msysgit/commits/kb/msys/less
Unicode msys.dll (unchanged):
https://github.com/kblees/msysgit/commits/kb/msys/unicode
Recodetree script (unchanged):
https://github.com/kblees/msysgit/commits/kb/recodetree
When merging, beware that the less build script and msys.dll patch go to
the msys branch of the msysgit repo. And Atsushi Nakagawa's vim-patch
(an/vim-utf-8) should be merged, too.
An updated Unicode Howto in MediaWiki format is included below (also
available here: https://github.com/kblees/git/wiki).
Again, I'll only include the interdiff (pretty small this time), if you'd
like the 26 patches via email, please speak up.
A git installer that bundles the Unicode msys.dll, less-444, recodetree
script and git v1.7.9 with unicode-v15 patches can be found here:
https://docs.google.com/uc?id=0BxXUoUg2r8ftMzJiNDRlNWEtYjMxZi00YWMzLThiNjktMjcyZTQ1MzlhZDFj&export=download
Feel free to upload to the msysgit downloads area.
And TortoiseGit V1.7.5.0 built with CP_ACP replaced by CP_UTF8
(unchanged):
32 bit:
https://docs.google.com/uc?id=0BxXUoUg2r8ftYjFmMzg0MmItYjZhMy00MjM4LWFkYjktN2RiOTUxNDdiMzdk&export=download
Bye,
Karsten
Changes since kb/unicode-v14:
* Unicode file name support (gitk and git-gui)
- fix git-gui startup if absolute worktree path contains non-ascii chars
- fix encoding in git-gui options dialog (e.g. user name)
Commit logs ('+': new/rewritten in v15, '-': removed, '!': modified)
---
[01/26] git-gui: fix encoding in git-gui file browser
[02/26] gitk: fix file name encoding in diff hunk headers
[03/26] Revert "Disable test on MinGW that challenges its bash quoting"
[04/26] Revert "Windows: teach getenv to do a case-sensitive search"
[05/26] Revert "mingw.c: move definition of mingw_getenv down"
[06/26] Win32: Thread-safe windows console output
[07/26] Win32: add Unicode conversion functions
[08/26] Win32: Unicode file name support (except dirent)
[09/26] Win32: Unicode file name support (dirent)
! [10/26] Unicode file name support (gitk and git-gui)
[11/26] Win32: Unicode arguments (outgoing)
[12/26] Win32: Unicode arguments (incoming)
[13/26] Win32: sync Unicode console output and file system
[14/26] Win32: Unicode environment (outgoing)
[15/26] Win32: Unicode environment (incoming)
[16/26] MinGW: disable legacy encoding tests
[17/26] Win32: fix environment memory leaks
[18/26] Win32: unify environment case-sensitivity
[19/26] Win32: simplify internal mingw_spawn* APIs
[20/26] Win32: move environment functions
[21/26] Win32: unify environment function names
[22/26] Win32: factor out environment block creation
[23/26] Win32: don't copy the environment twice when spawning child
processes
[24/26] Win32: reduce environment array reallocations
[25/26] Win32: keep the environment sorted
[26/26] Win32: patch Windows environment on startup
Makefile | 2 -
compat/mingw.c | 680
+++++++++++++++++++++++++++++++----------------
compat/mingw.h | 134 +++++++++-
compat/win32/dirent.c | 30 +--
compat/win32/dirent.h | 2 +-
compat/winansi.c | 397 ++++++++++++++++++----------
git-gui/git-gui.sh | 11 +-
git-gui/lib/browser.tcl | 2 +-
git-gui/lib/index.tcl | 6 +-
gitk-git/gitk | 15 +-
run-command.c | 10 +-
t/t3901-i18n-patch.sh | 19 +-
t/t4201-shortlog.sh | 6 +-
t/t5505-remote.sh | 5 +-
t/t8005-blame-i18n.sh | 8 +-
15 files changed, 883 insertions(+), 444 deletions(-)
---
Diff between v14 and v15...
---
$ git diff -p --stat kb/unicode-v14-4efc677a..kb/unicode-v15
git-gui/git-gui.sh | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 9988746..e5038dd 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -533,6 +533,9 @@ proc git {args} {
_trace_exec [concat $opt $cmdp $args]
set result [eval exec $opt $cmdp $args]
+ if {[encoding system] != "utf-8"} {
+ set result [encoding convertfrom utf-8 [encoding convertto
$result]]
+ }
if {$::_trace} {
puts stderr "< $result"
}
@@ -1087,7 +1090,7 @@ git-version proc _parse_config {arr_name args} {
[list git_read config] \
$args \
[list --null --list]]
- fconfigure $fd_rc -translation binary
+ fconfigure $fd_rc -translation binary -encoding
utf-8
set buf [read $fd_rc]
close $fd_rc
}
@@ -1273,7 +1276,7 @@ apply_config
# v1.7.0 introduced --show-toplevel to return the canonical work-tree
if {[package vsatisfies $_git_version 1.7.0]} {
- set _gitworktree [encoding convertfrom utf-8 [git rev-parse
--show-toplevel]]
+ set _gitworktree [git rev-parse --show-toplevel]
} else {
# try to set work tree from environment, core.worktree or use
# cdup to obtain a relative path to the top of the worktree. If
---
Unicode Howto
---
= Git for Windows Unicode Support =
As of V1.7.8, a Unicode-aware Version of Git for Windows is available.
Here are a few things to consider when using the Unicode version.
== Editor ==
If you want to use a custom text editor to enter commit messages or to
edit config files (instead of vim/gvim that are installed with Git for
Windows), find one that supports Unix line breaks (LF only) and can save
UTF-8 without BOM (i.e. Windows notepad.exe is a bad choice).
== Settings ==
=== Windows settings ===
==== Console font (per user) ====
The default console font does not support Unicode. Change the console font
to a TrueType font such as Lucida Console or Consolas. The setup program
can do this automatically, but only for the installing user.
=== Git settings ===
These can be set per user (with the --global option) or per repository,
the repository settings take precedence.
==== Disable quoted file names ====
By default, git will print non-ASCII file names in quoted octal notation,
i.e. "\nnn\nnn...". This can be disabled with
git config [--global] core.quotepath off
==== Disable commit message transcoding ====
Previous Git for Windows required to set the
<tt>i18n.logoutputencoding</tt> to your Windows system's default OEM
encoding for proper console output of non-ASCII commit messages. This is
no longer necessary. Remove this or set it to 'utf-8':
git config [--global] --unset i18n.logoutputencoding
The i18n.commitencoding setting should also be removed or set to 'utf-8'
to support commit messages on the command line (<tt>git commit -m
"..."</tt> from cmd.exe, MSYS bash won't let you enter non-ASCII
characters):
git config [--global] --unset i18n.commitencoding
== Migrating old Git for Windows repositories ==
This is only relevant if you used non-ASCII file names with non-Unicode
Git for Windows versions.
Previous Git for Windows versions stored file names in the default
encoding of the originating Windows system, making these repositories
incompatible with other Windows language-versions and other Git versions
(including Cygwin-git and JGit / EGit on Windows).
The Unicode-enabled Git for Windows stores file names UTF-8 encoded.
=== Checking if a repository contains non-ASCII file names ===
The <tt>recodetree check</tt> command scans the entire history of a git
repository and prints all non-ASCII file names. If the output is empty, no
migration is necessary.
=== Migration with previous Git for Windows version available ===
The simplest way to convert old repositories is by keeping an old Git for
Windows version around:
# With the old Git for Windows version: check out a completely clean state
of the working copy (so <tt>git status</tt> reports nothing, not even
untracked files). This can be achieved by manually deleting everything
except the .git directory, and then doing a <tt>git reset --hard</tt>.
# With the new Git for Windows version: <tt>git status</tt> with the new
version should now report non-ASCII file names as deleted (with mangled
file name) as well as untracked (with correct file name). Add the changes
to the repository with <tt>git add --all & git commit -m "UTF-8
conversion"</tt>.
# Repeat for every branch of interest
=== Migration without previous Git for Windows available ===
==== Manually ====
This requires renaming all non-ASCII file names manually.
# <tt>git status</tt> will report non-ASCII file names as deleted (with
mangled names) as well as untracked (also with mangled names).
# Fix the file names in the working copy manually.
# Add the changes to the repository, e.g. with <tt>git add --all & git
commit -m "UTF-8 conversion"</tt>.
# Repeat for every branch of interest
==== Using the recodetree script (experimental) ====
This requires iconv.exe on the path.
# <tt>git status</tt> will report non-ASCII file names as deleted (with
mangled names) as well as untracked (also with mangled names).
# Use <tt>recodetree head</tt> to convert non-ASCII file names to UTF-8
and stage them for commit. Check the results with <tt>git status</tt>.
# Commit the changes to the repository with <tt>git commit -m "UTF-8
conversion"</tt>.
# Clean up the working copy (remove untracked files with mangled names,
e.g. delete everything except .git, then <tt>git reset --hard</tt>).
# Repeat for every branch of interest
=== Convert config files ===
Git config files with non-ASCII content need to be converted to UTF-8, for
example your name in %HOME%/.gitconfig, or non-ASCII file names in
.gitattributes / .gitignore / .gitmodules files.
=== Migrating the entire history (experimental) ===
The <tt>recodetree history</tt> command can be used to convert the entire
history of the repository (requires iconv.exe). Beware that rewriting
history changes all the object hashes in the repository, which has quite
severe implications on other users if the repository is published (see
"RECOVERING FROM UPSTREAM REBASE" in git help rebase). The <tt>recodetree
history</tt> script currently does not convert config files such as
.gitattributes / .gitignore / .gitmodules.
---
Sebastian Schuberth
> less-444 (unchanged):
> https://github.com/kblees/msysgit/commits/kb/msys/less
In an effort to reduce the diff to Karsten's branches I was thinking
about merging the above. Will this just work fine with the current
non-Unicode code, or do you expect any problems due to the "Update
less settings for UTF-8" patch, Karsten?
--
Sebastian Schuberth
karste...@dcon.de
Without the LESSCHARSET=utf-8 setting, I think less-444 will work pretty much the same as the current version (I haven't tested this, though).
Less uses the msys.dll for output, and the current msys.dll is still based on WriteConsoleA/WriteFile (i.e. expects Windows system encoding, not UTF-8).
The LESSCHARSET setting tells less which byte sequences form valid characters, i.e. it controls escaping invalid chars ("<xx>") and horizontal scrolling.
Karsten
Erik Faye-Lund
> Hi there,
>
> here is v15 of the unicode patch series:
>
> git: https://github.com/kblees/git/tree/kb/unicode-v15
Awesome work! I've reviewed the patches again, and everything looks good to me!
> less-444 (unchanged):
> https://github.com/kblees/msysgit/commits/kb/msys/less
> Unicode msys.dll (unchanged):
> https://github.com/kblees/msysgit/commits/kb/msys/unicode
> Recodetree script (unchanged):
> https://github.com/kblees/msysgit/commits/kb/recodetree
>
> When merging, beware that the less build script and msys.dll patch go to
> the msys branch of the msysgit repo. And Atsushi Nakagawa's vim-patch
> (an/vim-utf-8) should be merged, too.
>
I would have preferred to get that as "for-msys" and "for-devel"
branches including an/vim-utf-8, but thanks to your instructions +
some common sense I think I figured out how to merge it all :)
Merged, and pushed! If I messed up something, I'm sure we can fix that later...
karste...@dcon.de
This is SO COOL, thanks :-)
To be honest, it wasn't quite clear to me how I should publish a
'multi-branch' patch. As I understand it, msys binaries are built in the
msys branch, then cherry-picked to devel [1]. So, could you please
cherry-pick these three (binaries and settings), the rest looks good to
me:
c82c0ff Updated msys-1.0.dll to MSYS-g7ebac74
e1fba54 Updated less.exe to less-444
50f2c54 Update less settings for UTF-8
[1] http://groups.google.com/group/msysgit/msg/f32c9c13a528b156
PS: I hope that links are OK now (I double-checked the "Plain text only"
setting), otherwise I'll have to mangle URLs...
Johannes Schindelin
On Mon, 6 Feb 2012, Erik Faye-Lund wrote:
> On Sun, Feb 5, 2012 at 7:39 PM, <karste...@dcon.de> wrote:
>
> > here is v15 of the unicode patch series:
> >
> > git: https://github.com/kblees/git/tree/kb/unicode-v15
>
> Awesome work! I've reviewed the patches again, and everything looks good
> to me!
>
> [...]
>
> Merged, and pushed!
Awesome! Thank you both for your persistence and hard work.
Ciao,
Dscho
Erik Faye-Lund
> Erik Faye-Lund <kusm...@gmail.com> wrote on 06.02.2012 17:14:34:
>
>> On Sun, Feb 5, 2012 at 7:39 PM, <karste...@dcon.de> wrote:
>> > Hi there,
>> >
>> > here is v15 of the unicode patch series:
>> >
>> > git: https://github.com/kblees/git/tree/kb/unicode-v15
>>
>> Awesome work! I've reviewed the patches again, and everything looks
>> good to me!
>>
>> > less-444 (unchanged):
>> > https://github.com/kblees/msysgit/commits/kb/msys/less
>> > Unicode msys.dll (unchanged):
>> > https://github.com/kblees/msysgit/commits/kb/msys/unicode
>> > Recodetree script (unchanged):
>> > https://github.com/kblees/msysgit/commits/kb/recodetree
>> >
>> > When merging, beware that the less build script and msys.dll patch go
> to
>> > the msys branch of the msysgit repo. And Atsushi Nakagawa's vim-patch
>> > (an/vim-utf-8) should be merged, too.
>> >
>>
>> I would have preferred to get that as "for-msys" and "for-devel"
>> branches including an/vim-utf-8, but thanks to your instructions +
>> some common sense I think I figured out how to merge it all :)
>>
>> Merged, and pushed! If I messed up something, I'm sure we can fix
>> that later...
>
> This is SO COOL, thanks :-)
>
Hey, you were the one doing the heavy lifting, thank YOU! :)
> To be honest, it wasn't quite clear to me how I should publish a
> 'multi-branch' patch.
Neither was I, it would just have been a bit easier to just have two
branches ready to pull. But this works fine as well :)
> As I understand it, msys binaries are built in the
> msys branch, then cherry-picked to devel [1]. So, could you please
> cherry-pick these three (binaries and settings), the rest looks good to
> me:
>
> c82c0ff Updated msys-1.0.dll to MSYS-g7ebac74
> e1fba54 Updated less.exe to less-444
> 50f2c54 Update less settings for UTF-8
>
Done, pushed. Thanks again!
Erik Faye-Lund
Hmm, I'm seeing an annoying little issue; when there's a lot of data
waiting for the pager, it doesn't quit right away on "ESC" any more.
It seems Git rather spends whatever time it takes to complete the
action it was doing. A simple "git log" in git.git shows what I mean
very clearly...
Kirill Smelkov
Karsten, Erik, thanks for finally merging this. It's a great step
forward!
Thanks a lot!
Kirill
Erik Faye-Lund
Never mind, this is not a problem your series introduced, it was in
the 'devel'-branch before I applied your patches as well, it seems.
Atsushi Nakagawa
Erik Faye-Lund <kusm...@gmail.com> wrote:
> On Sun, Feb 5, 2012 at 7:39 PM, <karste...@dcon.de> wrote:
> > Hi there,
> >
> > here is v15 of the unicode patch series:
> >
>
> Merged, and pushed! If I messed up something, I'm sure we can fix that later...
A big fat congratulations! to Karsten and Eric on finally getting this
merged! It couldn't have been done without Karsten's persistence to see
it through and Eric's vigilant and invaluable efforts!
Regards,
--
Atsushi Nakagawa
<at...@chejz.com>
Changes are made when there is inconvenience.
Atsushi Nakagawa
Erik Faye-Lund <kusm...@gmail.com> wrote:
> On Mon, Feb 6, 2012 at 6:46 PM, <karste...@dcon.de> wrote:
> > Erik Faye-Lund <kusm...@gmail.com> wrote on 06.02.2012 17:14:34:
> >
> >> On Sun, Feb 5, 2012 at 7:39 PM, <karste...@dcon.de> wrote:
> >> > When merging, beware that the less build script and msys.dll patch go
> > to
> >> > the msys branch of the msysgit repo. And Atsushi Nakagawa's vim-patch
> >> > (an/vim-utf-8) should be merged, too.
> >> >
> >>
> >> I would have preferred to get that as "for-msys" and "for-devel"
> >> branches including an/vim-utf-8, but thanks to your instructions +
> >> some common sense I think I figured out how to merge it all :)
> >>
> >> Merged, and pushed! If I messed up something, I'm sure we can fix
> >> that later...
>
> > 'multi-branch' patch.
>
> Neither was I, it would just have been a bit easier to just have two
> branches ready to pull. But this works fine as well :)
>
> > As I understand it, msys binaries are built in the
> > msys branch, then cherry-picked to devel [1]. So, could you please
> > cherry-pick these three (binaries and settings), the rest looks good to
> > me:
> >
> > c82c0ff Updated msys-1.0.dll to MSYS-g7ebac74
> > e1fba54 Updated less.exe to less-444
> > 50f2c54 Update less settings for UTF-8
Oops, I just noticed that the version of "an/vim-utf-8" that was merged
is an older version in msysgit/msysgit[1] which I couldn't work out how
to push over.
The last version that I pushed was to atnak/msysgit[2] and this list[3].
The only delta (aside from the commit message) is an addition of
"git-rebase-todo" to the file name pattern (for `rebase -i').
Is there any way you could get this one merged instead (or evil merged
to the current HEAD)?
Regards,
[1] https://github.com/msysgit/msysgit/tree/an/vim-utf-8
[2] https://github.com/atnak/msysgit/commits/an/vim-utf-8
[3] http://groups.google.com/group/msysgit/browse_thread/thread/ab5ad659831b0fe0
Johannes Schindelin
On Wed, 8 Feb 2012, Atsushi Nakagawa wrote:
> On Mon, 6 Feb 2012 17:14:34 +0100
> Erik Faye-Lund <kusm...@gmail.com> wrote:
> > On Sun, Feb 5, 2012 at 7:39 PM, <karste...@dcon.de> wrote:
> > > Hi there,
> > >
> > > here is v15 of the unicode patch series:
> > >
> > [...]
> >
> > Merged, and pushed! If I messed up something, I'm sure we can fix that later...
>
> A big fat congratulations! to Karsten and Eric on finally getting this
> merged! It couldn't have been done without Karsten's persistence to see
> it through and Eric's vigilant and invaluable efforts!
Yep, there cannot be enough thanks for that. It was a major undertaking, a
lot of convincing needed to be done (yours truly was one of the people who
needed convincing).
Karsten, Eric, if I ever come close to any of you: I buy you dinner.
Ciao,
Dscho
Johannes Schindelin
On Wed, 8 Feb 2012, Atsushi Nakagawa wrote:
Could you do a "git merge -s ours <wrongly-merged-commit>" with exactly
what you wrote here for the record? I'd gladly merge this.
Thanks,
Dscho
Atsushi Nakagawa
I wasn't familiar with this workflow so it took me a while to work it
out but I think I've got it and have updated atnak/msysgit/an/vim-utf-8
by merging a nullified msysgit/msysgit/an/vim-utf-8 into it.
I've also sent a "Pull Request" to msysgit/msysgit for the sake of
trying it out.
Regards,
Sebastian Schuberth
> I've also sent a "Pull Request" to msysgit/msysgit for the sake of
> trying it out.
Hmm, although I received a personal notification about this merge
requests, it seems my forwarding filter to the mailing list did not
work. Or did anyone see the merge requests on the mailing list?
--
Sebastian Schuberth
John Chen
On Feb 6, 2:39 am, karsten.bl...@dcon.de wrote:
> A git installer that bundles the Unicode msys.dll, less-444, recodetree
I think I've found a bug in the version Karsten uploaded a few days
ago. The bug can be reproduced by creating an empty repository and
commit something into it, then running git gc, as shown below:
-------------------------------------------------------------
Welcome to Git (version 1.7.9-unicode-20120204)
Run 'git help git' to display the help index.
Run 'git help <command>' to display help for specific commands.
x@SMTW-41 /c/tmp/git/1
$ git version
git version 1.7.9.msysgit.1
x@SMTW-41 /c/tmp/git/1
$ git init
Initialized empty Git repository in C:/tmp/git/1/.git/
x@SMTW-41 /c/tmp/git/1 (master)
$ echo "aoeu" > testing.txt
x@SMTW-41 /c/tmp/git/1 (master)
$ git add testing.txt
x@SMTW-41 /c/tmp/git/1 (master)
$ git commit -m "Just testing"
[master (root-commit) a31615a] Just testing
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 testing.txt
x@SMTW-41 /c/tmp/git/1 (master)
$ git gc
Counting objects: 3, done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0)
Deletion of directory '.git/objects/8c/' failed. Should I try again?
(y/n) y
Deletion of directory '.git/objects/8c/' failed. Should I try again?
(y/n) y
Deletion of directory '.git/objects/8c/' failed. Should I try again?
(y/n) y
Deletion of directory '.git/objects/8c/' failed. Should I try again?
(y/n)
-------------------------------------------------------------
After some debugging with OllyDbg, I think that
this bug seemed to be related to the function
int mingw_rmdir(const char *pathname) in compat/mingw.c.
Within the debugger, rmdir() is mapped to _wrmdir() in msvcrt.dll,
which takes const wchar_t * as parameter, but debugging shows that
an ASCII string is passed to it instead. Other functions such as
unlink(), which is mapped to _wunlink(), might have similiar problem,
but I'm not sure as I haven't seen it in action within the debugger.
As I'm new to git development, I'm unable to create a patch for this,
therefore I reported it here.
Thank you.
Chen John L.
Erik Faye-Lund
A similar issue have already been reported, thanks for looking into it.
However, as with the other report, I cannot reproduce it. I'm not
using Karsten's installer, though. Instead, I'm using his v15-series,
as is what is currently applied to 'devel'. This is what will probably
go into the next release.
> After some debugging with OllyDbg, I think that
> this bug seemed to be related to the function
> int mingw_rmdir(const char *pathname) in compat/mingw.c.
>
> Within the debugger, rmdir() is mapped to _wrmdir() in msvcrt.dll,
> which takes const wchar_t * as parameter, but debugging shows that
> an ASCII string is passed to it instead.
Interesting. I can't see how the code I have in my repo could get into
that situation. The code does
---8<---
wchar_t wpathname[MAX_PATH];
if (xutftowcs_path(wpathname, pathname) < 0)
return -1;
---8<---
and pass wpathname to _wrmdir.
Karsten, did you fix a bug matching these symptoms?
Erik Faye-Lund
> In the non-unicode build, _rmdir() fails with ERROR_SHARING_VIOLATION as
> well. However, the call to:
> is_dir_empty(pathname)
> actually erroneously returns 0, the reason why it returns 0 when the
> directory is in fact actually empty will be detailed later. Also,
> is_dir_empty() modified the last error, so the call to GetLastError()
> returns ERROR_SUCCESS. Therefore, in the next while loop:
> is_file_in_use_error(GetLastError())
> will return 0 because GetLastError() returns ERROR_SUCCESS, and the prompt
> does not appear, then the function fails silently, leaving the directory to
> be removed dangling there.
>
> On the issue with is_dir_empty(), the problem also lies with GetLastError().
> -----------------------------------------------
> if (!FindNextFile(handle, &findbuf)) {
> // Here, Last error is ERROR_NO_MORE_FILES
> strbuf_release(&buf); // calls free(), which changes last error to
> ERROR_SUCCESS
> return GetLastError() == ERROR_NO_MORE_FILES; // return 0, because
> GetLastError() returns ERROR_SUCCESS
> }
> -----------------------------------------------
>
> So the problem isn't introduced by the unicode patch, but this problem
> already exists, but is hidden by the is_dir_empty() bug.
>
Aha, this makes sense, and it's easy to fix. Thanks for spotting it!
> However, I haven't found the reason why this is_dir_empty() bug didn't hide
> the rmdir() failure in unicode build. In the unicode build, is_dir_empty()
> is inlined into mingw_rmdir(), and I haven't traced it properly yet, I'll
> report again when I find out the reason why it didn't hide rmdir()'s
> failure.
>
> Meanwhile, I've a suggestion on why rmdir() fails
> with ERROR_SHARING_VIOLATION. mingw_rmdir() is called from prune_dir() in
> builtin/prune-packed.c, which is called by prune_packed_objects().
>
> In prune_packed_objects():
> -------------------------------------------------
> d = opendir(pathname);
> if (!d)
> continue;
> prune_dir(i, d, pathname, len + 3, opts);
> closedir(d);
> -------------------------------------------------
> Directory is opened, and prune_dir() is called, that means when execution
> reaches rmdir(), the directory to remove is actually opened, and that's why
> the deletion fails. To solve the problem, simply move closedir() into
> prune_dir(), right before rmdir():
> -------------------------------------------------
> pathname[len] = 0;
> closedir(dir);
> rmdir(pathname);
> ...
> if (!d)
> continue;
> prune_dir(i, d, pathname, len + 3, opts);
> }
> -------------------------------------------------
> However, I'm not sure if this builtin/prune-packed.c belongs to the original
> git, that we cannot touch with it.
>
Sure, we can change it. Every file "belongs" to the "original git" ;)
But we can also send a patch upstream, because IMO it's an improvement
nevertheless.
> I'm sorry for not being able to submit a patch, as I'm new to git
> development, but I'll try to learn the basics.
>
No problem, your debugging has been very useful so far, thanks a lot!
Erik Faye-Lund
> However, I haven't found the reason why this is_dir_empty() bug didn't hide
> the rmdir() failure in unicode build. In the unicode build, is_dir_empty()
> is inlined into mingw_rmdir(), and I haven't traced it properly yet, I'll
> report again when I find out the reason why it didn't hide rmdir()'s
> failure.
This is because Karsten patched the function to use Unicode paths, and
removed the strbuf-usage. So the patch really only just revealed the
already existing bug, which is good :)
John Chen
Johannes Schindelin
On Sat, 11 Feb 2012, John Chen wrote:
> I'm not sure if this builtin/prune-packed.c belongs to the original git,
> that we cannot touch with it.
If you installed the development environment (which I think you mentioned
in another mail that you did), you can actually compare the version we
have versus the upstream version by calling
git fetch junio
git diff junio/next... -- builtin/prune-packed.c
> I'm sorry for not being able to submit a patch, as I'm new to git
> development, but I'll try to learn the basics.
You are really close: just check your changes with
git diff
(e.g. to remove debug printf()s or other things that do not belong to the
fix you are about to commit)
Then stage your changes for commit; I recommend doing
git gui
for that and clicking on the blue file symbols in the upper file list.
Then you can commit the change directly in the GUI by writing a very short
summary of the fix as first line, then an empty line, and then a bit more
explanation. Then click on the Sign-off button. Then on the Commit button.
If you need inspiration how commit messages typically look, just fire up
gitk
and try to imitate the style.
Once you have done that, you can export the patch with
git format-patch -1
Or you can publish your change by forking on GitHub
(https://github.com/msysgit/git/fork_select) and pushing your branch.
Ciao,
Johannes
karste...@dcon.de
handle
is open, causing "Deletion of directory '...' failed. Should I try again?"
prompts in git-repack and git-gc.
Prior to 19d1e75d "Win32: Unicode file name support (except dirent)",
these failures were silently ignored due to strbuf_free in is_dir_empty
resetting GetLastError to ERROR_SUCCESS.
Close find handles properly before trying to delete directories.
Reported-by: John Chen <john...@gmail.com>
Signed-off-by: Karsten Blees <bl...@dcon.de>
---
Erik Faye-Lund <kusm...@gmail.com> wrote on 10.02.2012 18:02:36:
> On Fri, Feb 10, 2012 at 5:36 PM, John Chen <john...@gmail.com> wrote:
> > In the non-unicode build, _rmdir() fails with ERROR_SHARING_VIOLATION
as
> > well. However, the call to:
> > is_dir_empty(pathname)
> > actually erroneously returns 0, the reason why it returns 0 when the
[...]
> > Directory is opened, and prune_dir() is called, that means when
execution
> > reaches rmdir(), the directory to remove is actually opened, and
that's why
> > the deletion fails. To solve the problem, simply move closedir() into
> > prune_dir(), right before rmdir():
[...]
>
> No problem, your debugging has been very useful so far, thanks a lot!
Indeed, thanks for this very detailed analysis.
I couldn't reproduce the problem until I tried the unicode version on an
old XP box...seems that on XP, FindFirstFile locks the directory, while on
Win7 it does not.
Incidentally, is_dir_empty didn't close its find handle either, so there
were actually three bugs, one fixed by accident, exposing the other two
:-)
This patch fixes it (tested on XP and 7).
builtin/prune-packed.c | 2 +-
compat/mingw.c | 4 +++-
2 files changed, 4 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/compat/mingw.c b/compat/mingw.c
index 183a2a4..6daffa7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -243,8 +243,10 @@ static int is_dir_empty(const wchar_t *wpath)
while (!wcscmp(findbuf.cFileName, L".") ||
!wcscmp(findbuf.cFileName, L".."))
- if (!FindNextFileW(handle, &findbuf))
+ if (!FindNextFileW(handle, &findbuf)) {
+ FindClose(handle);
return GetLastError() == ERROR_NO_MORE_FILES;
+ }
FindClose(handle);
return 0;
}
--
1.7.9.msysgit.1.1.g50c36
Erik Faye-Lund
> I couldn't reproduce the problem until I tried the unicode version on an
> old XP box...seems that on XP, FindFirstFile locks the directory, while on
> Win7 it does not.
>
> Incidentally, is_dir_empty didn't close its find handle either, so there
> were actually three bugs, one fixed by accident, exposing the other two
> :-)
>
> This patch fixes it (tested on XP and 7).
>
Unfortunately, it seems to be white-space corrupted (all tabs turned
to spaces before it reached my inbox), so I'll have to either re-roll
it or have you push it out yourself.
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 183a2a4..6daffa7 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -243,8 +243,10 @@ static int is_dir_empty(const wchar_t *wpath)
>
> while (!wcscmp(findbuf.cFileName, L".") ||
> !wcscmp(findbuf.cFileName, L".."))
> - if (!FindNextFileW(handle, &findbuf))
> + if (!FindNextFileW(handle, &findbuf)) {
> + FindClose(handle);
> return GetLastError() == ERROR_NO_MORE_FILES;
Don't we risk that FindClose changes the return code for GetLastError
here, like free (probably backed by VirtualFree) did on WinXP? Perhaps
we should do this instead?
diff --git a/compat/mingw.c b/compat/mingw.c
index 183a2a4..d10734b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -243,8 +243,11 @@ static int is_dir_empty(const wchar_t *wpath)
while (!wcscmp(findbuf.cFileName, L".") ||
!wcscmp(findbuf.cFileName, L".."))
- if (!FindNextFileW(handle, &findbuf))
- return GetLastError() == ERROR_NO_MORE_FILES;
+ if (!FindNextFileW(handle, &findbuf)) {
+ DWORD err = GetLastError();
+ FindClose(handle);
+ return err == ERROR_NO_MORE_FILES;
+ }
FindClose(handle);
return 0;
}
karste...@dcon.de
g'night
Karsten
Erik Faye-Lund
Looks good to me, pushed!
Erik Faye-Lund
Whoops, seems my push failed. Pushed for real this time!
Stefan Naewe
>
> On Sat, Feb 11, 2012 at 1:11 AM, Erik Faye-Lund <kusmabite <at> gmail.com>
wrote:
> > On Sat, Feb 11, 2012 at 1:08 AM, <karsten.blees <at> dcon.de> wrote:
> >>
> >> Right...I guess it's too late for plain C ...fixed and pushed to:
> >> https://github.com/kblees/git/tree/kb/fix-rmdir-in-repack
> >>
> >> g'night
> >> Karsten
> >
> > Looks good to me, pushed!
>
> Whoops, seems my push failed. Pushed for real this time!
I built an installer from the latest devel branch of '/git' (01fcafb8) and
now I'm getting the 'Deletion of directory...' prompts from 'git gc' when it
runs 'git prune --expire 2.weeks.ago'.
Stefan
Stefan Naewe
Stefan
--
----------------------------------------------------------------
python -c "print '73746566616e2e6e6165776540676d61696c2e636f6d'.decode('hex')"
karste...@dcon.de
sub-directories
in git-repack". There is another rmdir-before-closedir in builtin/prune.c
(i.e. producing "Deletion of directory '...' failed..." prompts in
git-prune on Windows XP).
Fix it by swapping closedir / rmdir.
The other closedir calls look fine at first glance.
Reported-by: Stefan Naewe <stefan...@gmail.com>
Signed-off-by: Karsten Blees <bl...@dcon.de>
---
https://github.com/kblees/git/commit/9db94146
Can I just push this or do we want to squash it with the previous patch
(b4cb824d) on next rebase?
builtin/prune.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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
Erik Faye-Lund
On Fri, Mar 2, 2012 at 1:43 PM, <karste...@dcon.de> wrote:
> This is a follow-up to "Win32: fix deletion of .git/objects
> sub-directories
> in git-repack". There is another rmdir-before-closedir in builtin/prune.c
> (i.e. producing "Deletion of directory '...' failed..." prompts in
> git-prune on Windows XP).
>
> Fix it by swapping closedir / rmdir.
>
> The other closedir calls look fine at first glance.
>
> Reported-by: Stefan Naewe <stefan...@gmail.com>
> Signed-off-by: Karsten Blees <bl...@dcon.de>
> ---
>
> https://github.com/kblees/git/commit/9db94146
>
> Can I just push this or do we want to squash it with the previous patch
> (b4cb824d) on next rebase?
>
I think these should be separate commits. I also suspect that the
rmdir-moving from b4cb824d should be split out into a separate patch
(perhaps squashed with this one, I don't know) and sent upstream, as
it might be an improvement for other systems as well.
>
> builtin/prune.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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;
> }
>
Looks obviously correct to me, nice :)
karste...@dcon.de
Erik Faye-Lund <kusm...@gmail.com> wrote on 02.03.2012 13:57:31:
> (Corrected the CC-list, it somehow got corrupted)
>
> On Fri, Mar 2, 2012 at 1:43 PM, <karste...@dcon.de> wrote:
> > This is a follow-up to "Win32: fix deletion of .git/objects
> > sub-directories
> > in git-repack". There is another rmdir-before-closedir in builtin/prune.c
> > (i.e. producing "Deletion of directory '...' failed..." prompts in
> > git-prune on Windows XP).
> >
> > Fix it by swapping closedir / rmdir.
> >
> > The other closedir calls look fine at first glance.
> >
> > Reported-by: Stefan Naewe <stefan...@gmail.com>
> > Signed-off-by: Karsten Blees <bl...@dcon.de>
> > ---
> >
> > https://github.com/kblees/git/commit/9db94146
> >
> > Can I just push this or do we want to squash it with the previous patch
> > (b4cb824d) on next rebase?
> >
>
> I think these should be separate commits. I also suspect that the
> rmdir-moving from b4cb824d should be split out into a separate patch
> (perhaps squashed with this one, I don't know) and sent upstream, as
> it might be an improvement for other systems as well.
>
Erik Faye-Lund
>> > Can I just push this or do we want to squash it with the previous patch
>> > (b4cb824d) on next rebase?
>> >
>>
>> I think these should be separate commits. I also suspect that the
>> rmdir-moving from b4cb824d should be split out into a separate patch
>> (perhaps squashed with this one, I don't know) and sent upstream, as
>> it might be an improvement for other systems as well.
>>
>
> 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.
karste...@dcon.de
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 <john...@gmail.com>
Reported-by: Stefan Naewe <stefan...@gmail.com>
Signed-off-by: Karsten Blees <bl...@dcon.de>
---
Erik Faye-Lund <kusm...@gmail.com> wrote on 05.03.2012 11:26:08:
> On Mon, Mar 5, 2012 at 10:57 AM, <karste...@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
Johannes Sixt
> 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.
Junio C Hamano
>> 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
This patch is fairly independent from other topics that are queued in
msysgit, IIRC. Please take it with the mentioned fixup.
Thanks,
-- Hannes
karste...@dcon.de
> Johannes Sixt <j...@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.
karste...@dcon.de
> Am 06.03.2012 10:18, schrieb karste...@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).
Junio C Hamano
> Junio C Hamano <git...@pobox.com> wrote on 06.03.2012 21:19:06:
>> Johannes Sixt <j...@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.
Junio C Hamano
> 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.
karste...@dcon.de
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)