Re: [PATCH 2/2] catfile.c: add --batch-command mode

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Subject: Re: [PATCH 2/2] catfile.c: add --batch-command mode
>
> "cat-file: add --batch-command mode" perhaps.  The patch touching
> the file "catfile.c" (which does not even exist) is an irrelevant
> implementation detail to spend 2 extra bytes in "git shortlog"
> output.
>
>> From: John Cai <johncai86@gmail.com>
>>
>> Add new flag --batch-command that will accept commands and arguments
>> from stdin, similar to git-update-ref --stdin.
>>
>> At GitLab, we use a pair of long running cat-file processes when
>> accessing object content. One for iterating over object metadata with
>> --batch-check, and the other to grab object contents with --batch.
>>
>> However, if we had --batch-command, we wouldnt need to keep both
>> processes around, and instead just have one process where we can flip
>> between getting object info, and getting object contents. This means we
>> can get rid of roughly half of long lived git cat-file processes. This
>> can lead to huge savings since on a given server there could be hundreds
>> of git cat-file processes running.
>
> Hmph, why hundreds, not two you listed?
>
> Do you mean "we have two per repository, but by combining, we can do
> with just one per repository, halving the number of processes"?

>
>> git cat-file --batch-command
>>
>> would enter an interactive command mode whereby the user can enter in
>> commands and their arguments:
>>
>> <command> [arg1] [arg2] NL
>>
>> This patch adds the basic structure for add command which can be
>> extended in the future to add more commands.
>>
>> This patch also adds the following commands:
>>
>> contents <object> NL
>> info <object> NL
>>
>> The contents command takes an <object> argument and prints out the object
>> contents.
>>
>> The info command takes a <object> argument and prints out the object
>> metadata.
>>
>> In addition, we need a set of commands that enable a "read session".
>>
>> When a separate process (A) is connected to a git cat-file process (B)
>
> This description misleads readers into thinking as if we have just
> created a daemon process that is running, and an unrelated process
> can connect to it, which obviously poses a question about securing
> the connection.  It is my understanding that what this creates is
> just a consumer process (A) starts the cat-file process (B) locally
> on its behalf under process (A)'s privilege, and they talk over pipe
> without allowing any third-party to participate in the exchange, so
> we should avoid misleading users by saying "is connected to" here.

>
>> and is interactively writing to and reading from it in --buffer mode,
>> (A) needs to be able to know when the buffer is flushed to stdout.
>
> If A and B are talking over a pair pipes, in order to avoid
> deadlocking, both ends need to be able to control whose turn it is
> to speak (and it is turn for the other side to listen).  A needs to
> be able to _control_ (not "know") when the buffer it uses to write
> to B gets flushed, in order to reliably say "I am done for now, it
> is your turn to speak" and be assured that it reaches B.  The story
> is the same for the other side.  When a request by A needs to be
> responded with multiple lines of output, B needs to be able to say
> "And that concludes my response, and I am ready to accept a new
> request from you" and make sure it reaches A.  "know when..." is
> probably a wrong phrase here.

>
>> Currently, from (A)'s perspective, the only way is to either 1. exit
>> (B)'s process or 2. send an invalid object to stdin. 1. is not ideal
>> from a performance perspective as it will require spawning a new cat-file
>> process each time, and 2. is hacky and not a good long term solution.
>
> Writing enumeration as bulletted or enumerated list would make it
> much easier to read, I would think.
>
>     From (A)'s perspective, the only way is to either
>
>     1. exit (B)'s process or
>     2. send an invalid object to stdin.
>
>     1. is not ideal from a performance perspective, as it will
>     require spawning a new cat-file process each time, and 2. is
>     hacky and not a good long term solution.
>
> I am not sure what you exactly mean by "exit" in the above.  Do you
> mean "kill" instead?

>
>> With the following commands, process (A) can begin a "session" and
>> send a list of object names over stdin. When "get contents" or "get info"
>> is issued, this list of object names will be fed into batch_one_object()
>> to retrieve either info or contents. Finally an fflush() will be called
>> to end the session.
>>
>> begin NL
>> get contents NL
>> get info NL
>>
>> These can be used in the following way:
>>
>> begin
>> <sha1>
>> <sha1>
>> <sha1>
>> <sha1>
>> get info
>>
>> begin
>> <sha1>
>> <sha1>
>> <sha1>
>> <sha1>
>> get contents
>>
>> With this mechanism, process (A) can be guaranteed to receive all of the
>> output even in --buffer mode.
>
> OK, so do these "get blah" serve both as command and an implicit
> "flush"?

>
> With an implicit "flush", do we really need "begin"?
>
> Also, from the point of view of extensibility, not saying what kind
> of operation is started when given "begin" is probably not a good
> idea.  "get info" and "get contents" may happen to be the only
> commands that are supported right now, and the parameters to them
> may happen to be just list of object names and nothing else, but
> what happens when a new "git frotz" command is added and its
> operation is extended with something other than object names and
> pathnames?  The way to parse these parameter lines for the "get"
> would be different for different commands, and if "cat-file" knows
> upfront what is to be done to these parameters, it can even start
> prefetching and precomputing to reduce latency observed by the
> client before the final "get info" command is given.
>
> So, from that point of view,
>
>     begin <cmd>
>     <parameter>
>     <parameter>
>     ...
>     end
>
> may be a better design, no?