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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 26, 2012, 17:29 UTC
Message-ID: <7vlii68m7k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120726090842.GA4999@burratino>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 10 quoted lines

> [...]
>>>> +
>>>> +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.