Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP

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

"Shawn O. Pearce" <spearce@spearce.org> writes:

> +extern void close_pack_windows(struct packed_git *, int);

Nobody seems to pass anything but true in retain_fd parameter.
Is it worth it?

> +void close_pack_windows(struct packed_git *p, int retain_fd)
> +{
> +	struct pack_window *tail_var = NULL, *n = p->windows;
> +	struct pack_window **tail = &tail_var;
> +	while (n) {
> +		struct pack_window *w = p->windows;
> +
> +		if (w->inuse_cnt) {
> +			*tail = w;
> +			tail = &w->next;
> +			continue;
> +		}
> +
> +		munmap(w->base, w->len);
> +		pack_mapped -= w->len;
> +		pack_open_windows--;
> +		n = w->next;
> +		free(w);
> +	}
> +
> +	p->windows = tail_var;
> +	if (p->windows)
> +		warning("pack windows still in-use during close attempt");
> +	else if (!retain_fd && p->pack_fd != -1) {
> +		close(p->pack_fd);
> +		p->pack_fd = -1;
> +	}
> +}

I am not sure about this inuse_cnt business.

The only caller we know that needs this function is fast-import
that wants to drop all windows into a pack while keeping the
file descriptor and it should not have an in-use windows.

It is unclear what semantics is the right one for callers that
do want to retain some windows but still want to call this
function.  If somebody is in the middle of chasing a delta chain
and still calls this function, *why* is the call being made, and
what is the right behaviour if not all the windows can be closed
because of these open windows?

What about the case the value passed in ratain_fd is 0, which
presumably means the caller is asking us to close the file
descriptor?  If we close the packfile, later accesses through
the unclosed windows will obviously barf and I understand that
is why you are ignoring retain_fd in that case, but maybe for
the caller it was of higher priority that the packfile to get
closed than the remaining windows to be still usable.  Or maybe
the caller wants to be notified of such an error, in which case
it probably is not enough to just call warning().

IOW, I think the patch is trying to be too flexible without
having a clear definition of what that flexibility is trying to
achieve.

Maybe we would need more flexible version later, but I am not
convinced if the semantics the above patch implements is the
right one.

So I'd prefer something much simpler like this one instead...

void close_pack_windows(struct packed_git *p)
{
	while (p->windows) {
		struct pack_window *w = p->windows;

		if (w->inuse_cnt)
			die("pack '%s' still has outstanding windows",
				p->pack_name)
		munmap(w->base, w->len);
		pack_mapped -= w->len;
		pack_open_windows--;
		p->windows = w->next;
		free(w);
	}
}