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, 22:15 UTC
Message-ID: <xmqq1td9x8pz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <b7d0f89bb94f88cb8d600a461dfe7ea1@dscho.org>

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

Show 15 quoted lines

>>> +				&& !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".
>
> The `close_all_packs()` function is meant for the use case where you
> really close all the packs, no question asked.
>
> I cannot think of a use case where it would make sense to try to be
> gentle, yet still ask for *all* of the packs to be closed. Maybe you
> can think of one to convince me that it might make sense to respect
> the `do_not_close` flag in such a function? Because so far, I am
> totally unconvinced.

Do not wait for being convinced forever, as I am not interested in convincing you either way. Decision by made-up examples or lack of imagination is a waste of time ;-).

My earlier "I wonder" leads me to totally different conclusion, which is "we declare that it is a bug for any caller to call close-all-packs while marking any open pack with do_not_close bit".

Which merely means that while iterating over packs in the "close-all" loop, we should throw a die("BUG") at the caller if that bit is on. That way, we won't have to rely on "we did not think of a good use case so we unconditionally closed the packs without telling the remainder of the system, hopefully we broke nothing."

Instead we'd make it absolutely clear what our assumption of this change and the new helper function is, and a caller that may appear in the future that wants to have do_not_close bit can complain. At that point, maybe we can decide to either loosen it if that caller has a good reason, or tell the caller to drop do_not_close bit before calling close-all.