Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov <max@max630.net>]

On Fri, Oct 02, 2015 at 12:05:55PM +0200, Johannes Schindelin wrote:
> Hi Max,
> 
> On 2015-10-01 05:29, Max Kirillov wrote:
>> When a builtin has done its job, but waits for pager or not waited
>> by its caller and still hanging it keeps pack files opened.
>> This can cause a number of issues, for example on Windows git gc
>> cannot remove the packs.
> 
> I did not experience that issue. In any case, closing the
> packs after the builtin function has returned might not
> change anything anyway because we are about to `exit()`
> and that's that.

Steps to reproduce:
1. install the latest git-for-windows, 2.6.0.windows.1
2. run the script in the end of message
3. remember the directory name, press the enter as it asks
4. in the less scrool down

Then inspect the processes git.exe and less.exe with Process
Explorer of something you find convenient. Both processes
should have opened the pack file, and git.exe should have it
mapped (I found it in the "View"->"Lower Pane views"->"Dlls").

Then, if you run another bash window, cd to the repository
and run "git repack -a -d", it should print:

----
$ git repack -a -d
Counting objects: 3001, done.
Compressing objects: 100% (1003/1003), done.
Writing objects: 100% (3001/3001), done.
Total 3001 (delta 997), reused 3001 (delta 997)
Unlink of file '.git/objects/pack/pack-e1b0e3ac01ff8d79a77648de3370f49b93c58a8b.pack' failed. Should I try again? (y/n)
----

I would like the git gc to succeed, because I run it as a
scheduled task nightly and feel a bit annoyed by the opened
windows which wait for me to say yes (after exiting pager).

So, the fix is needed approximately in that place, after
running any builtin command.

For your case, I think, the same close_all_packs() should be
invoked before the repacking.

> The convention in Git seems to call things _gently rather
> than _nodie:

Thanks, will change it if the idea is welcomed.

>> -void close_pack_windows(struct packed_git *p)
>> +static int close_pack_windows_nodie(struct packed_git *p)
>>  {
>>  	while (p->windows) {
>>  		struct pack_window *w = p->windows;
>>  
>>  		if (w->inuse_cnt)
>> -			die("pack '%s' still has open windows to it",
>> -			    p->pack_name);
>> +			return 1;
>> +
>>  		munmap(w->base, w->len);
>>  		pack_mapped -= w->len;
>>  		pack_open_windows--;
>>  		p->windows = w->next;
>>  		free(w);
>>  	}
>> +
>> +	return 0;
>> +}
> 
> And while we're at it, why not teach that function a new
> parameter `int close_pack_fd`?

I think "close windows" should close windows, if it also
closes pack fd probably should be another name. But current
code seems quite logical. Close the packs, and run closing
windows from it.

> There is another problem: when we cannot close the pack
> window, we cannot really continue, can we?

Yes we can, unlocking of the window is not needed for the
current process to do what it intended to do, it would just
interfere with concurrent git gc.

For the clone case probably die would be appropriate. If you
feel like it worth complicating the code we might search for
some solution.

-- 
Max

The script:
--------- ./t-windows-pack-close.sh ---------
#!/bin/sh

set -e

TEST_DIR=`mktemp -d`

t_git() {
    git --work-tree="$TEST_DIR" --git-dir="$TEST_DIR/.git" "$@"
}

t_git init

for i in $(seq 1000)
do
    echo "commit$i" >"$TEST_DIR/commit.$i"
    t_git add "commit.$i"
    t_git commit -m "commit$i" -q
done

t_git repack

pack_path=$(find "$TEST_DIR/.git/objects/pack" -name "pack-.pack")

echo "dir: $TEST_DIR"
echo press enter to start git log
read

t_git -c core.packedGitWindowSize=100 log
-----------------------------