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

On Thu, Nov 26, 2009 at 1:15 AM, Junio C Hamano [off-list ref] wrote:

 - The original series was done over a few weeks in 'pu' and this
  intermediate step was done before a better alternative of not using
  these two extra merge strategies were discovered ("...may have been an
  easy way to experiment, but we should bite the bullet", in the next
  patch).

  As the second round to seriously polish the series for inclusion, it
  would make much more sense to squash this with the next patch to erase
  this failed approach that has already been shown as clearly inferiour.

ok.

 - 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, but I had to redo a bunch of the flag stuff
anyway since a few other flags had been added in the meantime.

I actually tried it both ways (with and without an extra parameter),
but I observed that:

- There are more lines of code (and more confusion) if you use an
all-in-one flags vs. what I did.

- Several functions have the same signature with all-in-one flags vs.
their current boolean parameter, so the code would compile (and then
subtly not work) if I forgot to modify a particular function.

- When we go to add a third flag parameter, it wouldn't be any harder
to join them together at that time, and because it would *again*
modify the function signatures (from two flag params back down to
one), the compiler would *again* be able to catch any functions we
forgot to adjust.

If you think this logic doesn't work, I can redo it with all-in-one
flags as you request.

Avery