Re: [PATCH] Build in merge
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 8, 2008, 00:32 UTC
- Message-ID
- <7v63rhz03x.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1215474140-11220-1-git-send-email-vmiklos@frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> writes:
Show 27 quoted lines
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> > --- > > On Mon, Jul 07, 2008 at 11:15:09AM -0700, Junio C Hamano <gitster@pobox.com> 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.