On Thu, Nov 26, 2009 at 1:15 AM, Junio C Hamano <gitster@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html