Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 5, 2015, 20:57 UTC
- Message-ID
- <xmqqio6lxcci.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <a1f0d9339a9de95ba3f5eaaaf2e1a712629ad5d4.1444076827.git.johannes.schindelin@gmx.de>
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 ...
Show 22 quoted lines
> + 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.
Show 11 quoted lines
> 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?
Show 8 quoted lines
> 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 ;-)