Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.

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

Jonathan Nieder <jrnieder@gmail.com> writes:

> [...]
>>>> +
>>>> +enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };
> [...]
>> Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.
>
> Much nicer.
>
> I think this tristate return value could be avoided entirely because...
> ... it isn't needed at the moment.

I am not sure what you mean by that.

The command dispatcher loop in [Patch v2 1/16] seems to call every
possible input handler with the first line of the input and expect
them to answer "This is not for me", so NOT_HANDLED is needed.

An alternative dispatcher could be written in such a way that the
dispatcher inspects the first line and decide what to call, and in
such a scheme, you do not need NOT_HANDLED. My intuition tells me
that such an arrangement is in general a better organization.

Looking at what cmd_import() does, however, I think the approach the
patch takes might make sense for this application.  Unlike other
handlers like "capabilities" that do not want to handle anything
other than "capabilities", it wants to handle two:

 - "import" that starts an import batch;
 - "" (an empty line), but only when an import batch is in effect.

A centralized dispatcher that does not use NOT_HANDLED could be
written for such an input stream, but then the state information
(i.e. "are we in an import batch?") needs to be global, which may or
may not be desirable (I haven't thought things through on this).

In any case, if you are going to use dispatching based on
NOT_HANDLED, the result may have to be (at least) quadri-state.  In
addition to "I am done successfully, please go back and dispatch
another command" (SUCCESS), "This is not for me" (NOT_HANDLED), and
"I am done successfully, and there is no need to dispatch and
process another command further" (TERMINATE), you may want to be
able to say "This was for me, but I found an error" (ERROR).

Of course, if the dispatch loop has to be rewritten so that a
central dispatcher decides what to call, individual input handlers
do not need to say NOT_HANDLED nor TERMINATE, as the central
dispatcher should keep track of the overall state of the system, and
the usual "0 on success, negative on error" may be sufficient.

One thing I wondered was how an input "capability" (or "list")
should be handled after "import" was issued (hence batch_active
becomes true).  The dispatcher loop in the patch based on
NOT_HANDLED convention will happily call cmd_capabilities(), which
does not have any notion of the batch_active state (because it is a
function scope static inside cmd_import()), and will say "Ah, that
is mine, and let me do my thing."  If we want to diagnose such an
input stream as an error, the dispatch loop needs to become aware of
the overall state of the system _anyway_, so that may be an argument
against the NOT_HANDLED based dispatch system the patch series uses.