Hi,
On Sun, 6 Jul 2008, Junio C Hamano wrote:
> Miklos Vajna <vmiklos@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > + *
> > + * 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.
> > +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
--
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
