On Tue, May 06, 2025 at 11:52:02AM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> writes:
>
> > +static void munmap_snapshot_if_temporary(struct snapshot *snapshot)
> > +{
> > + if (mmap_strategy != MMAP_OK && snapshot->mmapped) {
>
> In general, a refactoring tomove conditionals like this to the
> callee and make the callers unconditionally call the helper is an
> antipattern from maintainability's point of view.
>
> Imagine what would happen when we acquire a different mmap_strategy
> in the future, and by that time, there are callers in the codebase
> other than what we currently have (which is just one below after
> this patch). Do you have to verify if all existing callers that
> trusted "if_temporary" really meant MMAP_TEMPORARY, as the name of
> the helper function suggested, or do some of the callers meant "any
> strategy other than MMAP_OK"? What if some callers want the former
> and others want the latter semantics?
>
That's right. Actually when I wake up in the morning, I suddenly find
out I made a mistake here. We should try to make the function as pure as
possible. So, this function would just do one thing, call `mumap` when
the mmap strategy is "MMAP_TEMPORARY".
> Without even talking about longer term maintainability, at the
> callsite, _if_temporary in the name is a much weaker sign than an
> explicit if() condition that says what is going on.
>
> I'd prefer the caller to be more like
>
> if (mmap_strategy == MMAP_TEMPORARY)
> munmap_temporary_stapshot(snapshot)
>
> and make the caller to return immediately when !snapshot, i.e.
>
> static void munmap_temporary_stapshot(struct snapshot *snapshot
> {
> if (!snapshot)
> return;
> ... the rest of the helper function ...
> }
>
> Thanks.
