Re: [PATCH 14/14] Build in merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2008, 08:50 UTC
Message-ID: <7vej67jt1e.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <0cde1e7c930589364318b2d0344b345453e23586.1214918017.git.vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 37 quoted lines

> 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.

Show 26 quoted lines

> +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 ;-).