Re: [PATCH] Build in merge

Miklos Vajna [off-list ref] writes:

Mentored-by: Johannes Schindelin [off-list ref]
Signed-off-by: Miklos Vajna <redacted>
---

On Mon, Jul 07, 2008 at 11:15:09AM -0700, Junio C Hamano [off-list ref] wrote:

quoted

I do not get you on this point.  Which one is nicer?

 (1) Have two lists, perhaps all_* and user_*.  The logic that finds a
     strategy searches in two lists.  The logic that checks if a given
     strategy is built-in checks if it is on all_* list.

 (2) Have a single list, but add a boolean "unsigned is_builtin:1" to
 each
     element of it.  The logic that finds a strategy looks in this
     single
     list.  The logic that checks if a given strategy is built-in
     looks at
     the strategy instance and it has the bit already.

You seem to be advocating (1) but I do not understand why...

Ah, OK. For now, I just added an "unsigned enabled:1;". Later we can add
an "unsigned is_buildin:1;" as well, but currently we die with earlier
with a "Could not find merge strategy" error message, so is_builtin
would be always true.

So here is a version, this time without the use_strategies list.

That is not what I meant.  I am afraid perhaps I misunderstood what you
were talking about.

When/if you allow user defined new strategies, then you have a choice:

 (1) find "git-merge-*" in path, add them to the single all_strategies[]
     list (but you will do the ALLOC_GROW() business so you would need to
     use the one you currently have as static form to prime the real list),
     and look for "foo" strategy when "-s foo" is given from that single
     list, or

 (2) find "git-merge-*" in path, add them to a separate user_strategies[]
     list, and look for "foo" strategy when "-s foo" is given from the
     user_strategies[] list and all_strategies[] list (all_strategies[]
     should perhaps be renamed to builtin_strategies[] if you go that
     route).

The comparison I gave was between the above two.  But the change you are
talking about is completely different, isn't it?

The part that records which strategies were specified from the command
line *in what order* via "-s foo" switches should remain list of pointers
into "struct strategy", which is called "struct strategy **use_strategies"
in the code and corresponds to the $use_strategies variable in the
scripted version.  The order of these is important, as that defines in
which order the strategies are tried [*1*].  If you go route (1), these
pointers will all be pointing at elements in all_strategies[]; with route
(2) they may be pointing at either all_strageties[] element or
user_strategies[] element.

If you are never going to say "available strategies are these" after you
start supporting user-defined strategy, then you do not necessarily need
to do the "find 'git-merge-*' in path, add them to ..." step above, in
which case it would be Ok not to scan the path and add them to
all_strategies[] (in route (1)) nor user_strategies[] (in route (2)).
Instead, you would just create a new "struct strategy" instance lazily
when the user gave "-s foo" and "foo" is not one of the built-in strategy.
You would put that at the tail of "struct strategy **use_strategy" array,
and iterate over use_strategy in the order they are given on the command
line.


[Footnote]

*1* Personally, I find the importance of this dubious in practice, as I
said earlier, I do not think it would work well to try different
strategies and pick the best one --- evaluating which result is the *best*
is difficult.  If you want to stay compatible with the scripted version,
however, you cannot just mark entries in all_strategies[] with boolean and
iterate over them in the order that all_strageties[] define them.  You
need to try them in the order the user specified.