On Tue, Sep 07 2021, Taylor Blau wrote: > On Tue, Sep 07, 2021 at 10:50:58PM -0400, Taylor Blau wrote: >> Of the two, I think the former is more appealing (since no other >> functions called by finish_tmp_packfile() are guarded like that; they >> conditionally behave as noops depending on `flags`). > > Sorry; this is nonsensical. The only other function we call is > write_idx_file() which merely changes its behavior based on flags, but > it never behaves as a noop. > > That doesn't change my thinking about preferring the former of my two > suggestions, but just wanted to correct my error. I agree that this code is very confusing overall, but would prefer to wait on refactoring further until the two topics in flight (this and the other pack-write topic) settle. As shown in https://lore.kernel.org/git/87v93bidhn.fsf@xxxxxxxxxxxxxxxxxxx/ I think the best thing to do is neither of the narrow fixes you suggest, but to more deeply untangle the whole mess around how we choose to write these files & with what options. A lot of it is bit-twiddling back and forth for no real reason. Once we do that it becomes impossible to land in a mode where these functions need to in principle deal with writing a "real" file and the "verify" mode, which as noted in the linked E-Mail is the case now, we just need/want these "is more than one set?" checks & assertions because we've made the interface overly confusing/general.
