Re: [PATCH 3/3] vcs-svn: Refactor dump_export code into dispatch table

Ramkumar Ramachandra wrote:

[...]

+static state_fn *const dispatch_table[NODE_KIND_COUNT][NODE_ACTION_COUNT] = {
+	/* NODE_KIND_UNKNOWN */
+	{abort, abort, abort, Adelete, abort},
+	/* NODE_KIND_NORMAL */
+	{abort, Nchange, Nadd, Adelete, Nreplace},
+	/* NODE_KIND_EXECUTABLE */
+	{abort, Echange, Eadd, Adelete, Ereplace},
+	/* NODE_KIND_SYMLINK */
+	{abort, Schange, Sadd, Adelete, Sreplace},
+	/* NODE_KIND_GITLINK */
+	{abort, abort, abort, abort, abort},
+	/* NODE_KIND_DIR */
+	{abort, Dchange, Dadd, Adelete, Dreplace},
+	/* NODE_KIND_SUBDIR */
+	{abort, abort, abort, abort, abort}
+};

Heh.  I think that Junio was suggesting making the _parser_
table-driven, meaning something like

	... node_kinds[] = {
		{ "100644", sizeof("100644"), "file" },
		{ "100755", sizeof("100755"), "file", "svn:executable" },
		{ "120000", sizeof("120000"), "file", "svn:special" },
		{ "160000", sizeof("160000"), "file" },	/* NEEDSWORK: seems wrong" */
		{ "040000", sizeof("040000"), "dir" }
	};

(Side note: remember that 644 and 755 are permitted synonyms for
100644 and 100755!)

I personally think that simple state machines tend to be easier to
follow if the current state is represented by the instruction pointer
rather than a variable, as in the current fast-import.c.  But maybe
that's a matter of taste?

Anyway, my other complaints about this dispatch_table are that the
function names leave me rubbing my head and I can't keep the list
of states you're transitioning between straight in my head.  I guess
"Adelete" is an abbreviation for print_delete_node_action?  Is a
callback needed at all (rather than just a string) for such actions?

Hope that helps,
Jonathan