Re: [PATCH 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`

[shejialuo <shejialuo@gmail.com>]

On Tue, May 06, 2025 at 11:52:02AM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> 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.