Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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?
>
> 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.
Oops. I obviously meant "the callee" here.
> static void munmap_temporary_stapshot(struct snapshot *snapshot
> {
> if (!snapshot)
> return;
> ... the rest of the helper function ...
> }
>
> Thanks.
