Re: [PATCH 3/8] git-merge-recursive-{ours,theirs}

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 30, 2009, 06:21 UTC
Message-ID: <7vvdgs1qip.fsf@alter.siamese.dyndns.org>
In-Reply-To: <32541b130911261405q6564d8f2o30b7d7fd6f708d05@mail.gmail.com>

Avery Pennarun <apenwarr@gmail.com> writes:

Show 10 quoted lines

>>  - I think we should avoid adding the extra argument to ll_merge_fn() by
>>   combining virtual_ancestor and favor into one "flags" parameter.  If
>>   you do so, we do not have to change the callsites again next time we
>>   need to add new optional features that needs only a few bits.
>>
>>   I vaguely recall that I did the counterpart of this patch that way
>>   exactly for the above reason, but it is more than a year ago, so maybe
>>   I didn't do it that way.
>
> You did do that, in fact,... <<rationale omitted>>

Think of the "flag" parameter as a mini "struct option". When you add a feature to a function at or near the leaf level of call chains that are potentially deep, you add one element to the option structure, and take advantage of the fact that existing callers put a sane default value in the new field, i.e. 0, by doing a "memset(&opt, 0, sizeof(opt))" already, so that the callsites that do not even have to know about the new feature will keep working the same old way without breakage. You saw this exact pattern in the [1/8] patch in your series to cram new "favor this side" information into an existing parameter.

As you mentioned, sometimes changing function signature is preferred to catch semantic differences at compilation time. The report given by the compiler of extra or missing parameter at the call site is a wonderful way to find out that you forgot to convert them to the new semantics of the function. This also helps when there is an in-flight patch that adds a new callsite to the function whose semantics you are changing. The semantic conflict is caught when compiling the result of a merge with a branch with such a patch. It is a trick worth knowing about.

The approach however cuts both ways. When you are adding an optional feature that is used only in a very few call sites, the semantic merge conflict resulting from such a function signature change is rarely worth it.

As long as you choose the default "no-op" value carefully enough so that existing callers will naturally use it without modification, the old code will work the way they did before the new optional feature was added. In other words, "let's implement this as purely an opt-in feature" is often preferrable over "let's force semantic conflict and compilation failure, just in case existing callsites may also want to trigger this new feature".

That is why [1/8] patch in your series uses 0 to mean "don't do the funny 'favor' trick, but just leave the conflicts there".

I've queued the series with minor fixes to 'pu' and pushed it out.