Re: [PATCH 14/14] Build in merge

Hi,

On Sun, 6 Jul 2008, Junio C Hamano wrote:

Miklos Vajna [off-list ref] writes:

quoted

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.

My fault.  I avoid ugly #defines, so I suggested using an enum.

quoted

+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?

Well, it is not really an abuse if you think of path_list as a 
string_list, which it really is, and which should have happened a long 
time ago, but I gave up.

Anyway, the use of string_list here was originally to make lookup slightly 
more convenient, using an API, instead of reinventing the wheel over and 
over and over again, as can be seen nicely in some parts of Git's source 
code.

Given that we want to be able to add other strategies, I would have rather 
suggested fixing string_list to heed "alloc == 0" and _not_ realloc() in 
that case.

But given that you seem so sick and tired of string_list, and rather have 
a code duplication, I will not argue to that end anymore.

Tired,
Dscho