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.
So I would like to skip this:
> diff --git a/git.c b/git.c
> index 5feba41..ad34680 100644
> --- a/git.c
> +++ b/git.c
> @@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int
> argc, const char **argv)
> trace_argv_printf(argv, "trace: built-in: git");
>
> status = p->fn(argc, argv, prefix);
> + close_all_packs();
> if (status)
> return status;
>
Also, I would move the new declaration directly before `close_pack_windows()`:
> diff --git a/cache.h b/cache.h
> index 79066e5..153bc46 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);
> extern void free_pack_by_name(const char *);
> extern void clear_delta_base_cache(void);
> extern struct packed_git *add_packed_git(const char *, int, int);
> +extern void close_all_packs(void);
>
> /*
> * Return the SHA-1 of the nth object within the specified packfile.
The convention in Git seems to call things _gently rather than _nodie:
> diff --git a/sha1_file.c b/sha1_file.c
> index 08302f5..62f1dad 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,
> return ret;
> }
>
> -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`?
There is another problem: when we cannot close the pack window, we cannot really continue, can we? Because if we do, we *still* have the lock, and we'll just fail later, most likely with an unhelpful error message because we do not know where the pack closing failed. I do not think that the warning really helps...
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html