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

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

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?

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.