Miklos Vajna <vmiklos@xxxxxxxxxxxxxx> writes:
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> Signed-off-by: Miklos Vajna <vmiklos@xxxxxxxxxxxxxx>
> ---
>
> On Mon, Jul 07, 2008 at 11:15:09AM -0700, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> 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.
--
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
