Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Junio C Hamano <gitster@pobox.com>]

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

> There was a lot of repeated code to close the file descriptor of
> a given pack. Let's just refactor this code into a single function.

That is a very good idea, but...

> +static int close_pack_fd(struct packed_git *p)
> +{
> +	if (p->pack_fd < 0)
> +		return 0;

Is this "return 0" compatible with ...

> +	close(p->pack_fd);
> +	pack_open_fds--;
> +	p->pack_fd = -1;
> +
> +	return 1;
> +}
> +
>  /*
>   * The LRU pack is the one with the oldest MRU window, preferring packs
>   * with no used windows, or the oldest mtime if it has no windows allocated.
> @@ -853,12 +865,8 @@ static int close_one_pack(void)
>  		find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);
>  	}
>  
> -	if (lru_p) {
> -		close(lru_p->pack_fd);
> -		pack_open_fds--;
> -		lru_p->pack_fd = -1;
> -		return 1;
> -	}
> +	if (lru_p)
> +		return close_pack_fd(lru_p);

... what is returned from here?

It seems to me that it would be a bug if (p->pack_fd < 0) in this
codepath, so in practice nobody will receive a newly invented return
value 0 from this function, but it feels wrong.

>  	return 0;
>  }
> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)
>  		if (strcmp(pack_name, p->pack_name) == 0) {
>  			clear_delta_base_cache();
>  			close_pack_windows(p);
> -			if (p->pack_fd != -1) {
> -				close(p->pack_fd);
> -				pack_open_fds--;
> -			}
> +			close_pack_fd(p);

And here, the closer _must_ be (and it currently is) very aware that
the pack chosen may not be open anymore.  By giving a function that
conditionally closes if the pack is still open too bland a name,
that distinction is lost at these two call sites.

Also closing "fd" is not the only thing the new helper does, so in
that sense its name is suboptimal, too.

Perhaps a new helper function that is close_pack(), which is
unconditional, with another close_pack_if_open() around it?

>  			if (!win->offset && win->len == p->pack_size
> -				&& !p->do_not_close) {
> -				close(p->pack_fd);
> -				pack_open_fds--;
> -				p->pack_fd = -1;
> -			}
> +				&& !p->do_not_close)
> +				close_pack_fd(p);

I wonder how this do_not_close bit should interact with "we are
shutting down (or we are spawning another to do the real work, while
we won't do anything useful anymore), so close everything down".

I'd imagine we'll see the answer in the next patch ;-)