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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 2, 2015, 10:05 UTC
Message-ID: <33b74e875c7298f67640f5850e88c152@dscho.org>
In-Reply-To: <1443670163-31193-2-git-send-email-max@max630.net>

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:

Show 13 quoted lines

> 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()`:

Show 12 quoted lines

> 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:

Show 28 quoted lines

> 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