Re: [PATCH 14/14] Build in merge

[Junio C Hamano <gitster@pobox.com>]

Miklos Vajna <vmiklos@frugalware.org> writes:

> diff --git a/builtin-merge.c b/builtin-merge.c
> new file mode 100644
> index 0000000..b261993
> --- /dev/null
> +++ b/builtin-merge.c
> @@ -0,0 +1,1158 @@
> +/*
> + * Builtin "git merge"
> + *
> + * Copyright (c) 2008 Miklos Vajna <vmiklos@frugalware.org>
> + *
> + * Based on git-merge.sh by Junio C Hamano.
> + */
> +
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "builtin.h"
> +#include "run-command.h"
> +#include "path-list.h"
> +#include "diff.h"
> +#include "refs.h"
> +#include "commit.h"
> +#include "diffcore.h"
> +#include "revision.h"
> +#include "unpack-trees.h"
> +#include "cache-tree.h"
> +#include "dir.h"
> +#include "utf8.h"
> +#include "log-tree.h"
> +#include "color.h"
> +
> +enum strategy {
> +	DEFAULT_TWOHEAD = 1,
> +	DEFAULT_OCTOPUS = 2,
> +	NO_FAST_FORWARD = 4,
> +	NO_TRIVIAL = 8
> +};

Usually "enum foo" consists of possible values of "foo".  But this is
not a list of strategies.  These are possible attributes to strategies.

> +static const char * const builtin_merge_usage[] = {
> +	"git-merge [options] <remote>...",
> +	"git-merge [options] <msg> HEAD <remote>",
> +	NULL
> +};
> +
> +static int show_diffstat = 1, option_log, squash;
> +static int option_commit = 1, allow_fast_forward = 1;
> +static int allow_trivial = 1, have_message;
> +static struct strbuf merge_msg;
> +static struct commit_list *remoteheads;
> +static unsigned char head[20], stash[20];
> +static struct path_list use_strategies;
> +static const char *branch;
> +
> +static struct path_list_item strategy_items[] = {
> +	{ "recur",      (void *)NO_TRIVIAL },
> +	{ "recursive",  (void *)(DEFAULT_TWOHEAD | NO_TRIVIAL) },
> +	{ "octopus",    (void *)DEFAULT_OCTOPUS },
> +	{ "resolve",    (void *)0 },
> +	{ "stupid",     (void *)0 },
> +	{ "ours",       (void *)(NO_FAST_FORWARD | NO_TRIVIAL) },
> +	{ "subtree",    (void *)(NO_FAST_FORWARD | NO_TRIVIAL) },
> +};
> +static struct path_list strategies = { strategy_items,
> +	ARRAY_SIZE(strategy_items), 0, 0 };

This declaration is funnily line-wrapped.

        static struct path_list strategies = {
                strategy_items, ARRAY_SIZE(strategy_items), 0, 0,
        };

But more problematic is that a path_list is inherently a dynamic data
structure (you can add and it reallocs), and this use of relying on the
knowledge that you happen to never add anything (nor subtract anything)
from the list is a mere hack.  If on the other hand you (and more
importantly other people who touch this implementation later) will never
add or remove items from this "strategies" array, you should make sure at
the interface level that nobody can -- one way to do so is not to abuse
path_list for something like this.

Come to think of it, wasn't the reason why the earlier "Why do you need
such distracting casts all over the place?" issue came up in another patch
because of this kind of (ab)use of path_list, which is an inappropriate
data structure for the job?

You would perhaps define:

	#define DEFAULT_TWOHEAD (1<<0)
	#define DEFAULT_OCTOPUS (1<<1)
	#define NO_FAST_FORWARD (1<<2)
	#define NO_TRIVIAL	(1<<3)

	static struct strategy {
        	char *name;
                unsigned attr;
	} all_strategy[] = {
                { "octopus",    DEFAULT_OCTOPUS },
                { "ours",       (NO_FAST_FORWARD | NO_TRIVIAL) },
                { "recur",      NO_TRIVIAL },
                { "recursive",  (DEFAULT_TWOHEAD | NO_TRIVIAL) },
                { "resolve",    0 },
                { "stupid",     0 },
                { "subtree",    (NO_FAST_FORWARD | NO_TRIVIAL) },
        };

And "unsorted_path_list_lookup()" can now become much more natural,
perhaps:

	static struct strategy *get_strategy(const char *name);

which has a more natural function signature and much better name.

Then, you would keep an array of pointers into all_strategy[] array to
represent the list of "-s strategy" given by the user:

	static struct strategy *use_strategy;
	static int use_strategy_alloc, use_strategy_nr;

and have a function that use s the standard ALLOC_GROW() and friends to
grow this.  The function will be named and written more naturally
(i.e. path_list_append_strategy() can go) --- this does not have anything
to do with path_list, but it is about "merge strategy".

But 99.9% of the time you would not have more than one elements ;-).