Add cat-file --batch-command flag

The feature proposal of adding a command interface to cat-file was first discussed in [A]. In [B], Taylor expressed the need for a fuller proposal before moving forward with a new flag. An RFC was created [C] and the idea was discussed more thoroughly, and overall it seemed like it was headed in the right direction.

This patch series consolidates the feedback from these different threads.

The latest change since [C] was replacing the flush command with a series of commands to allow a read "session", so we can queue up a bunch of objects and then get either their info or contents, then end by flushing. This is to address Phillip's concern about deadlocks if we allow the caller to call flush whenever they want.

This patch series has two parts:

1. preparation patch to rename a variable
2. logic to handle --batch-command flag, and adding different commands

A. https://lore.kernel.org/git/[email protected]/ B.
https://lore.kernel.org/git/[email protected]/ C.
https://lore.kernel.org/git/[email protected]/

John Cai (2):
cat-file.c: rename cmdmode to mode
catfile.c: add --batch-command mode

 Documentation/git-cat-file.txt |  27 +++++
 builtin/cat-file.c             | 203 +++++++++++++++++++++++++++++----
 t/t1006-cat-file.sh            |  46 +++++++-
 3 files changed, 251 insertions(+), 25 deletions(-)

base-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1212%2Fjohn-cai%2Fjc-cat-file-batch-command-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1212/john-cai/jc-cat-file-batch-command-v1
Pull-Request: https://github.com/git/git/pull/1212

gitgitgadget

From: John Cai <[email protected]>

To prepare for a new flag --batch-command, we will add a flag that indicates whether or not an interactive command mode will be used that reads commands and arguments off of stdin.

An intuitive name for this flag would be "command", which can get confusing with the already existing cmdmode.

Rename cmdmode->mode to prepare for this change.

---
 builtin/cat-file.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

index d94050e6c18..858bca208ff 100644
@@ -24,7 +24,7 @@ struct batch_options {
 	int buffer_output;
 	int all_objects;
 	int unordered;
-	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
+	int mode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 

@@ -306,19 +306,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (opt->cmdmode) {
+		if (opt->mode) {
 			char *contents;
 			unsigned long size;
 
 			if (!data->rest)
 				die("missing path for '%s'", oid_to_hex(oid));
 
-			if (opt->cmdmode == 'w') {
+			if (opt->mode == 'w') {
 				if (filter_object(data->rest, 0100644, oid,
 						  &contents, &size))
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
-			} else if (opt->cmdmode == 'c') {
+			} else if (opt->mode == 'c') {
 				enum object_type type;
 				if (!textconv_object(the_repository,
 						     data->rest, 0100644, oid,

@@ -330,7 +330,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
 			} else
-				BUG("invalid cmdmode: %c", opt->cmdmode);
+				BUG("invalid mode: %c", opt->mode);
 			batch_write(opt, contents, size);
 			free(contents);
 		} else {

@@ -533,7 +533,7 @@ static int batch_objects(struct batch_options *opt)
 	strbuf_expand(&output, opt->format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
-	if (opt->cmdmode)
+	if (opt->mode)
 		data.split_on_whitespace = 1;
 
 	/*

@@ -695,10 +695,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 
 	batch.buffer_output = -1;
 	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
-
 	if (opt) {
 		if (batch.enabled && (opt == 'c' || opt == 'w'))
-			batch.cmdmode = opt;
+			batch.mode = opt;
 		else if (argc == 1)
 			obj_name = argv[0];
 		else

@@ -712,9 +711,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 			usage_with_options(cat_file_usage, options);
 	}
 	if (batch.enabled) {
-		if (batch.cmdmode != opt || argc)
+		if (batch.mode != opt || argc)
 			usage_with_options(cat_file_usage, options);
-		if (batch.cmdmode && batch.all_objects)
+		if (batch.mode && batch.all_objects)
 			die("--batch-all-objects cannot be combined with "
 			    "--textconv nor with --filters");
 	}

gitgitgadget

I am moderately against this change. "mode" is too vague a word (i.e. it does not answer "mode of what?" question with the name), and renaming it to "mode" is probably a regression in readability.

The original name "cmdmode" is not all that better, either, as I am sure that it was named after the irrelevant implementation detail that it is originally read using the OPT_CMDMODE() feature of the parse-options machinery, and did not mean to express mode of doing "what" with the name.

So, let's think aloud what this "mode" is about. The feature lets the batch operation to choose how the blob objects are mangled [*], so "mangle-mode", "filter-mode", etc. (or find and use a better verb than these) would be an improvement over the original "cmdmode".

[Footnote]

* 'w' to apply the smudge filter to make them as if they were written to the working tree, 'c' to apply the textconv filter, or 0 to stream them as-is.

From: John Cai <[email protected]>

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.

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) 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. 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.

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.

---
 Documentation/git-cat-file.txt |  27 +++++
 builtin/cat-file.c             | 184 ++++++++++++++++++++++++++++++---
 t/t1006-cat-file.sh            |  46 ++++++++-
 3 files changed, 242 insertions(+), 15 deletions(-)

index 27b27e2b300..d1ba0b12e54 100644
@@ -90,6 +90,33 @@ OPTIONS
 	need to specify the path, separated by whitespace.  See the
 	section `BATCH OUTPUT` below for details.
 
+--batch-command::
+	Enter a command mode that reads commands and arguments from stdin.
+	May not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also need to
+	specify the path, separated by whitespace.  See the section
+	`BATCH OUTPUT` below for details.
+
+contents <object>::
+	Print object contents for object reference <object>
+
+info <object>::
+	Print object info for object reference <object>
+
+begin::
+	Begins a session to read object names off of stdin. A session can be
+	terminated with `get contents` or `get info`.
+
+get contents::
+	After a read session is begun with the `begin` command, and object
+	names have been fed into stdin, end the session and retrieve contents of
+	all the objects requested.
+
+get info::
+	After a read session is begun with the `begin` command, and object
+	names have been fed into stdin, end the session and retrieve info of
+	all the objects requested.
+
 --batch-all-objects::
 	Instead of reading a list of objects on stdin, perform the
 	requested batch operation on all objects in the repository and

gitgitgadget

"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.

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"?

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.

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.

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?

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?

Hi Junio, thanks for the review!

Yes, exactly. I'll reword this in the next version to be more clear.

Yes this understanding is correct. Will fix wording in next version

Correct, "know" is not exactly right. _control_ would be the more accurate description.

Yes

yes, that's the idea.

Good point. Now I'm wondering if we can simplify where commands get queued up and a "get" will execute them along with an implicit flush.

<cmd> <parameter>
<cmd> <parameter>
<cmd> <parameter>
get

Hi John

I think that would be an improvement (I'd suggest calling "get" "flush" instead), the begin ... get <cmd> sequence seems unnecessarily complex compared the the RFC of this series. If the user gives --buffer on the command line then we can buffer the input until we see a "flush" command and if the user does not give --buffer on the command line then we should flush the output after each command.

Best Wishes

Phillip

()()On Thu, Feb 3, 2022 at 7:20 PM John Cai via GitGitGadget

I had the same reaction to this new command set as Junio expressed upstream[1], and was prepared to suggest an alternative but Junio said everything I wanted to say, so I won't say anything more about it here.

That aside, the implementation presented here is overly loose about malformed input and accepts all sorts of bogus invocations which it should be reporting as errors. For instance, a lone:

get info

without a preceding begin should be an error but such bogus input is not diagnosed. get contents is likewise affected. Another example:

    begin
    <oid>
    <EOF>

which lacks a closing get info or get contents is silently ignored. Similarly, malformed:

    begin
    begin
    ...

should be reported as an error but is not.

There is also a bug in which the accumulated list of OID's is never cleared. Thus:

    begin
    <oid>
    get info
    get info

emits info about <oid> twice, once for each get info invocation. Similarly:

    begin
    <oid1>
    get info
    <oid2>
    get info

emits information about <oid1> twice and <oid2> once. Invoking begin between the get info commands doesn't help because begin is a no-op. Thus:

    begin
    <oid1>
    get info
    begin
    <oid2>
    get info

likewise emits information about <oid1> twice and <oid2> once.

It also incorrectly accepts non-"session" commands in the middle of a session. For instance:

    begin
    <oid1>
    info <oid2>
    <oid3>

immediately emits information about <oid2> and then bombs out claiming that <oid3> is an "unknown command" because the info command -- which should not have been allowed within a session -- prematurely ended the "session".

The info and contents commands neglect to do any sort of validation of their arguments, thus any and all bogus invocations are accepted. Thus:

    info
    info <arg1> <arg2>
    info <non-oid>

are all accepted as valid invocations, misleadingly producing "<foo> missing" messages, rather than erroring out as they should. The "<oid> missing" message should be reserved for the case when the lone argument to info or contents is something which looks like a legitimate OID.

The above is a long-winded way of saying that it is important not only to check the obvious "expect success" cases when implementing a new feature, but it's also important to add checks for the "expect failure" cases, such as all the above malformed inputs.

It's subjective, but it feels like this implementation is trying to be too clever by handling all these cases via a single strbuf_getline() loop in batch_objects_command(). It would be easier to reason about, and would have avoided some of the above problems, for instance, if handling of begin was split out to its own function which looped over strbuf_getline() itself, thus could easily have detected premature EOF (lacking get info or get contents) and likewise would not have allowed info or contents commands to be executed within a "session".

Similarly (again subjective), the generic command dispatcher seems over-engineered and perhaps contributed to the oversight of info and contents failing to perform validation of their arguments. A simpler hand-rolled command-response loop is more common on this project and often easier to reason about. Perhaps something like this (pseudo-code):

    while (strbuf_getline(&input, stdin)) {
        char *end_cmd = strchrnul(&input.buf, ' ');
        const char *argstart = *end_cmd ? end_cmd + 1 : end_cmd;
        *end_cmd = '\0';
        if (strcmp(input.buf, "info"))
            show_info(argstart);
        else if (strcmp(input.buf, "contents"))
            show_contents(argstart);
        else if (strcmp(input.buf, "begin"))
            begin_session(argstart);
        else
            die(...);
    }

and each of the command-handler functions would perform its own argument validation (including the case when no argument should be present).

[1]: https://lore.kernel.org/git/[email protected]/

    strbuf_setlen(&input, input.len - 1);

Hi Eric,

I appreciate the feedback and the thorough review!

So actually the argument to info and contents doesn't have to be an OID but an object name that eventually gets passed to get_oid_with_context() to resolve to an actual oid. This is the same behavior as git cat-file --batch and --batch-check, neither of which throws an error. My goal was to maintain this behavior in --batch-command.

I overall agree and will add test coverage for invalid input.

I think I agree that the code in this patch is a little hard to follow, but now I'm planning to simplify the design by getting rid of "begin"[2].

I'm hoping this change will make the code easier to reason about.

2. https://lore.kernel.org/git/[email protected]/

yes, this was an oversight. thanks for catching it!

I realize we actually don't need this if block at all because strubf_getline() strips the line terminator for us already.

Okay, that makes sense. I was misled by the commit message talking about "<sha1>" in its examples, and didn't do my due diligence by digging more deeply into the existing documentation and code.

[Trying not to pile on and mentioning some things others haven't, but maybe there'll be duplications]

    string_list_append_nodup(..., strbuf_detach(&input, NULL));

	if (command) {
		batch_objects_command(...);
	        goto cleanup;
	}

Hi John/Ævar

If we do that then the strbuf will have to reallocate its buffer when it reads the next input line on the next iteration of the loop. As the strbuf will have likely over allocated the buffer using the string_list_append_nodup() + strbuf_detach() will will be less efficient overall than using string_list_append().

Best Wishes

Phillip

The feature proposal of adding a command interface to cat-file was first discussed in [A]. In [B], Taylor expressed the need for a fuller proposal before moving forward with a new flag. An RFC was created [C] and the idea was discussed more thoroughly, and overall it seemed like it was headed in the right direction.

This patch series consolidates the feedback from these different threads.

This patch series has two parts:

1. preparation patch to rename a variable
2. logic to handle --batch-command flag, adding contents, info, flush commands

Changes since v1:

* simplified "session" mechanism. "flush" will execute all commands that were entered in since the last "flush" when in --buffer mode
* when not in --buffer mode, each command is executed and flushed each time
* rename cmdmode to transform_mode instead of just mode
* simplified command parsing logic
* changed rename of cmdmode to transform_mode
* clarified verbiage in commit messages

A. https://lore.kernel.org/git/[email protected]/ B.
https://lore.kernel.org/git/[email protected]/ C.
https://lore.kernel.org/git/[email protected]/

John Cai (2):
cat-file: rename cmdmode to transform_mode
cat-file: add --batch-command mode

 Documentation/git-cat-file.txt |  19 ++++
 builtin/cat-file.c             | 138 +++++++++++++++++++++--
 t/t1006-cat-file.sh            | 197 ++++++++++++++++++++++++++++++++-
 3 files changed, 346 insertions(+), 8 deletions(-)

base-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1212%2Fjohn-cai%2Fjc-cat-file-batch-command-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1212/john-cai/jc-cat-file-batch-command-v2
Pull-Request: https://github.com/git/git/pull/1212

Range-diff vs v1:

1: 86df0c9e4df ! 1: 2d9a0b06ce5 cat-file.c: rename cmdmode to mode

     @@ Metadata
      Author: John Cai <[email protected]>
      
       ## Commit message ##
     -    cat-file.c: rename cmdmode to mode
     +    cat-file: rename cmdmode to transform_mode
      
     -    To prepare for a new flag --batch-command, we will add a flag that
     -    indicates whether or not an interactive command mode will be used
     -    that reads commands and arguments off of stdin.
     +    When introducing a new flag --batch-command, we will add a flag on the
     +    batch_options struct that indicates whether or not an interactive
     +    command mode will be used that reads commands and arguments off of
     +    stdin.
      
          An intuitive name for this flag would be "command", which can get
          confusing with the already existing cmdmode.
      
     -    Rename cmdmode->mode to prepare for this change.
     +    cmdmode refers to how the result output of the blob will be transformed,
     +    either according to --filter or --textconv. So transform_mode is a more
     +    descriptive name for the flag, and will not get confused with the new
     +    command flag to be added in the next commit.
     +
     +    Rename cmdmode to transform_mode in cat-file.c
      
          Signed-off-by: John Cai <[email protected]>
      
     @@ builtin/cat-file.c: struct batch_options {
       	int all_objects;
       	int unordered;
      -	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
     -+	int mode; /* may be 'w' or 'c' for --filters or --textconv */
     ++	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
       	const char *format;
       };
       
     @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
       		if (opt->buffer_output)
       			fflush(stdout);
      -		if (opt->cmdmode) {
     -+		if (opt->mode) {
     ++		if (opt->transform_mode) {
       			char *contents;
       			unsigned long size;
       
     @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
       				die("missing path for '%s'", oid_to_hex(oid));
       
      -			if (opt->cmdmode == 'w') {
     -+			if (opt->mode == 'w') {
     ++			if (opt->transform_mode == 'w') {
       				if (filter_object(data->rest, 0100644, oid,
       						  &contents, &size))
       					die("could not convert '%s' %s",
       					    oid_to_hex(oid), data->rest);
      -			} else if (opt->cmdmode == 'c') {
     -+			} else if (opt->mode == 'c') {
     ++			} else if (opt->transform_mode == 'c') {
       				enum object_type type;
       				if (!textconv_object(the_repository,
       						     data->rest, 0100644, oid,
     @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
       					    oid_to_hex(oid), data->rest);
       			} else
      -				BUG("invalid cmdmode: %c", opt->cmdmode);
     -+				BUG("invalid mode: %c", opt->mode);
     ++				BUG("invalid transform_mode: %c", opt->transform_mode);
       			batch_write(opt, contents, size);
       			free(contents);
       		} else {
     @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	data.mark_query = 0;
       	strbuf_release(&output);
      -	if (opt->cmdmode)
     -+	if (opt->mode)
     ++	if (opt->transform_mode)
       		data.split_on_whitespace = 1;
       
       	/*
      @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *prefix)
     - 
     - 	batch.buffer_output = -1;
     - 	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
     --
     - 	if (opt) {
     - 		if (batch.enabled && (opt == 'c' || opt == 'w'))
     --			batch.cmdmode = opt;
     -+			batch.mode = opt;
     - 		else if (argc == 1)
     - 			obj_name = argv[0];
     - 		else
     -@@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *prefix)
     - 			usage_with_options(cat_file_usage, options);
     - 	}
     + 	/* Return early if we're in batch mode? */
       	if (batch.enabled) {
     --		if (batch.cmdmode != opt || argc)
     -+		if (batch.mode != opt || argc)
     - 			usage_with_options(cat_file_usage, options);
     --		if (batch.cmdmode && batch.all_objects)
     -+		if (batch.mode && batch.all_objects)
     - 			die("--batch-all-objects cannot be combined with "
     - 			    "--textconv nor with --filters");
     - 	}
     + 		if (opt_cw)
     +-			batch.cmdmode = opt;
     ++			batch.transform_mode = opt;
     + 		else if (opt && opt != 'b')
     + 			usage_msg_optf(_("'-%c' is incompatible with batch mode"),
     + 				       usage, options, opt);
 2:  ebd2a135601 ! 2:  1b63164ad4d catfile.c: add --batch-command mode
     @@ Metadata
      Author: John Cai <[email protected]>
      
       ## Commit message ##
     -    catfile.c: add --batch-command mode
     +    cat-file: add --batch-command mode
      
     -    Add new flag --batch-command that will accept commands and arguments
     +    Add a new flag --batch-command that accepts 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.
     +    However, if we had --batch-command, we wouldn't need to keep both
     +    processes around, and instead just have one --batch-command process
     +    where we can flip between getting object info, and getting object
     +    contents. Since we have a pair of cat-file processes per repository,
     +    this means we can get rid of roughly half of long lived git cat-file
     +    processes. Given there are many repositories being accessed at any given
     +    time, this can lead to huge savings since on a given server.
      
          git cat-file --batch-command
      
     -    would enter an interactive command mode whereby the user can enter in
     -    commands and their arguments:
     +    will enter an interactive command mode whereby the user can enter in
     +    commands and their arguments that get queued in memory:
      
     -    <command> [arg1] [arg2] NL
     +    <command1> [arg1] [arg2] NL
     +    <command2> [arg1] [arg2] NL
      
     -    This patch adds the basic structure for add command which can be
     -    extended in the future to add more commands.
     +    When --buffer mode is used, commands will be queued in memory until a
     +    flush command is issued that execute them:
      
     -    This patch also adds the following commands:
     +    flush NL
     +
     +    The reason for a flush command is that when a consumer process (A)
     +    talks to a git cat-file process (B) and interactively writes to and
     +    reads from it in --buffer mode, (A) needs to be able to control when
     +    the buffer is flushed to stdout.
     +
     +    Currently, from (A)'s perspective, the only way is to either
     +
     +    1. kill (B)'s process
     +    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.
     +
     +    With this mechanism of queueing up commands and letting (A) issue a
     +    flush command, process (A) can control when the buffer is flushed and
     +    can guarantee it will receive all of the output when in --buffer mode.
     +
     +    This patch adds the basic structure for adding command which can be
     +    extended in the future to add more commands. It also adds the following
     +    two commands (on top of the flush command):
      
          contents <object> NL
          info <object> NL
     @@ Commit message
          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)
     -    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.
     -    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.
     +    These can be used in the following way with --buffer:
      
     -    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.
     +    contents <sha1> NL
     +    object <sha1> NL
     +    object <sha1> NL
     +    contents <sha1> NL
     +    flush
     +    contents <sha1> NL
     +    flush
      
     -    begin NL
     -    get contents NL
     -    get info NL
     +    When used without --buffer:
      
     -    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.
     +    contents <sha1> NL
     +    object <sha1> NL
     +    object <sha1> NL
     +    contents <sha1> NL
     +    contents <sha1> NL
      
          Helped-by: Ævar Arnfjörð Bjarmason <[email protected]>
          Signed-off-by: John Cai <[email protected]>
     @@ Documentation/git-cat-file.txt: OPTIONS
      +info <object>::
      +	Print object info for object reference <object>
      +
     -+begin::
     -+	Begins a session to read object names off of stdin. A session can be
     -+	terminated with `get contents` or `get info`.
     -+
     -+get contents::
     -+	After a read session is begun with the `begin` command, and object
     -+	names have been fed into stdin, end the session and retrieve contents of
     -+	all the objects requested.
     -+
     -+get info::
     -+	After a read session is begun with the `begin` command, and object
     -+	names have been fed into stdin, end the session and retrieve info of
     -+	all the objects requested.
     ++flush::
     ++	Execute all preceding commands that were issued since the beginning or
     ++	since the last flush command was issued. Only used with --buffer. When
     ++	--buffer is not used, commands are flushed each time without issuing
     ++	`flush`.
      +
       --batch-all-objects::
       	Instead of reading a list of objects on stdin, perform the
     @@ Documentation/git-cat-file.txt: OPTIONS
       ## builtin/cat-file.c ##
      @@ builtin/cat-file.c: struct batch_options {
       	int unordered;
     - 	int mode; /* may be 'w' or 'c' for --filters or --textconv */
     + 	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
       	const char *format;
      +	int command;
       };
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
       				      data);
       }
       
     -+static void parse_cmd_object(struct batch_options *opt,
     ++typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
     ++			       struct strbuf *, struct expand_data *);
     ++
     ++struct queued_cmd {
     ++	parse_cmd_fn_t fn;
     ++	const char *line;
     ++};
     ++
     ++static void parse_cmd_contents(struct batch_options *opt,
      +			     const char *line,
      +			     struct strbuf *output,
     -+			     struct expand_data *data,
     -+			     struct string_list revs)
     ++			     struct expand_data *data)
      +{
      +	opt->print_contents = 1;
      +	batch_one_object(line, output, opt, data);
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +static void parse_cmd_info(struct batch_options *opt,
      +			   const char *line,
      +			   struct strbuf *output,
     -+			   struct expand_data *data,
     -+			   struct string_list revs)
     ++			   struct expand_data *data)
      +{
      +	opt->print_contents = 0;
      +	batch_one_object(line, output, opt, data);
      +}
      +
     -+static void parse_cmd_begin(struct batch_options *opt,
     -+			   const char *line,
     -+			   struct strbuf *output,
     -+			   struct expand_data *data,
     -+			   struct string_list revs)
     ++static void flush_batch_calls(struct batch_options *opt,
     ++		struct strbuf *output,
     ++		struct expand_data *data,
     ++		struct queued_cmd *cmds,
     ++		int queued)
      +{
     -+	/* nothing needs to be done here */
     -+}
     -+
     -+static void parse_cmd_get(struct batch_options *opt,
     -+			   const char *line,
     -+			   struct strbuf *output,
     -+			   struct expand_data *data,
     -+			   struct string_list revs)
     -+{
     -+	struct string_list_item *item;
     -+	for_each_string_list_item(item, &revs) {
     -+		batch_one_object(item->string, output, opt, data);
     ++	int i;
     ++	for(i = 0; i < queued; i++){
     ++		cmds[i].fn(opt, cmds[i].line, output, data);
      +	}
     -+	if (opt->buffer_output)
     -+		fflush(stdout);
     -+}
     -+static void parse_cmd_get_info(struct batch_options *opt,
     -+			   const char *line,
     -+			   struct strbuf *output,
     -+			   struct expand_data *data,
     -+			   struct string_list revs)
     -+{
     -+	opt->print_contents = 0;
     -+	parse_cmd_get(opt, line, output, data, revs);
     -+}
     -+
     -+static void parse_cmd_get_objects(struct batch_options *opt,
     -+			   const char *line,
     -+			   struct strbuf *output,
     -+			   struct expand_data *data,
     -+			   struct string_list revs)
     -+{
     -+	opt->print_contents = 1;
     -+	parse_cmd_get(opt, line, output, data, revs);
     -+	if (opt->buffer_output)
     -+		fflush(stdout);
     ++	fflush(stdout);
      +}
      +
     -+enum batch_state {
     -+	BATCH_STATE_COMMAND,
     -+	BATCH_STATE_INPUT,
     -+};
     -+
     -+typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
     -+			       struct strbuf *, struct expand_data *,
     -+			       struct string_list revs);
     -+
      +static const struct parse_cmd {
      +	const char *prefix;
      +	parse_cmd_fn_t fn;
      +	unsigned takes_args;
     -+	enum batch_state next_state;
      +} commands[] = {
     -+	{ "contents", parse_cmd_object, 1, BATCH_STATE_COMMAND},
     -+	{ "info", parse_cmd_info, 1, BATCH_STATE_COMMAND},
     -+	{ "begin", parse_cmd_begin, 0, BATCH_STATE_INPUT},
     -+	{ "get info", parse_cmd_get_info, 0, BATCH_STATE_COMMAND},
     -+	{ "get contents", parse_cmd_get_objects, 0, BATCH_STATE_COMMAND},
     ++	{ "contents", parse_cmd_contents, 1},
     ++	{ "info", parse_cmd_info, 1},
      +};
      +
      +static void batch_objects_command(struct batch_options *opt,
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +				    struct expand_data *data)
      +{
      +	struct strbuf input = STRBUF_INIT;
     -+	enum batch_state state = BATCH_STATE_COMMAND;
     -+	struct string_list revs = STRING_LIST_INIT_DUP;
     ++	struct queued_cmd *cmds = NULL;
     ++	size_t alloc = 0, nr = 0;
     ++	int queued = 0;
      +
     -+	/* Read each line dispatch its command */
      +	while (!strbuf_getline(&input, stdin)) {
      +		int i;
      +		const struct parse_cmd *cmd = NULL;
      +		const char *p, *cmd_end;
     ++		struct queued_cmd call = {0};
     ++
     ++		if (!input.len)
     ++			die(_("empty command in input"));
     ++		if (isspace(*input.buf))
     ++			die(_("whitespace before command: '%s'"), input.buf);
      +
     -+		if (state == BATCH_STATE_COMMAND) {
     -+			if (*input.buf == '\n')
     -+				die("empty command in input");
     -+			else if (isspace(*input.buf))
     -+				die("whitespace before command: %s", input.buf);
     ++		if (skip_prefix(input.buf, "flush", &cmd_end)) {
     ++			if (!opt->buffer_output)
     ++				die(_("flush is only for --buffer mode"));
     ++			if (*cmd_end)
     ++				die(_("flush takes no arguments"));
     ++			if (!queued)
     ++				die(_("nothing to flush"));
     ++			flush_batch_calls(opt, output, data, cmds, queued);
     ++			queued = 0;
     ++			continue;
      +		}
      +
      +		for (i = 0; i < ARRAY_SIZE(commands); i++) {
     -+			const char *prefix = commands[i].prefix;
     -+			char c;
     -+			if (!skip_prefix(input.buf, prefix, &cmd_end))
     ++			if (!skip_prefix(input.buf, commands[i].prefix, &cmd_end))
      +				continue;
     -+			/*
     -+			 * If the command has arguments, verify that it's
     -+			 * followed by a space. Otherwise, it shall be followed
     -+			 * by a line terminator.
     -+			 */
     -+			c = commands[i].takes_args ? ' ' : '\n';
     -+			if (*cmd_end && *cmd_end != c)
     -+				die("arguments invalid for command: %s", commands[i].prefix);
      +
      +			cmd = &commands[i];
      +			if (cmd->takes_args)
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +			break;
      +		}
      +
     -+		if (input.buf[input.len - 1] == '\n')
     -+			input.buf[--input.len] = '\0';
     ++		if (!cmd)
     ++			die(_("unknown command: '%s'"), input.buf);
      +
     -+		if (state == BATCH_STATE_INPUT && !cmd){
     -+			string_list_append(&revs, input.buf);
     ++		if (!opt->buffer_output) {
     ++			cmd->fn(opt, p, output, data);
      +			continue;
      +		}
      +
     -+		if (!cmd)
     -+			die("unknown command: %s", input.buf);
     ++		queued++;
     ++		if (queued > nr) {
     ++			ALLOC_GROW(cmds, nr+1, alloc);
     ++			nr++;
     ++		}
      +
     -+		state = cmd->next_state;
     -+		cmd->fn(opt, p, output, data, revs);
     ++		call.fn = cmd->fn;
     ++		call.line = xstrdup(p);
     ++		cmds[queued-1] = call;
      +	}
     ++	free(cmds);
      +	strbuf_release(&input);
     -+	string_list_clear(&revs, 0);
      +}
      +
       static int batch_objects(struct batch_options *opt)
     @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	save_warning = warn_on_object_refname_ambiguity;
       	warn_on_object_refname_ambiguity = 0;
       
     --	while (strbuf_getline(&input, stdin) != EOF) {
     --		if (data.split_on_whitespace) {
     --			/*
     --			 * Split at first whitespace, tying off the beginning
     --			 * of the string and saving the remainder (or NULL) in
     --			 * data.rest.
     --			 */
     --			char *p = strpbrk(input.buf, " \t");
     --			if (p) {
     --				while (*p && strchr(" \t", *p))
     --					*p++ = '\0';
     -+	if (command)
     ++	if (command) {
      +		batch_objects_command(opt, &output, &data);
     -+	else {
     -+		while (strbuf_getline(&input, stdin) != EOF) {
     -+			if (data.split_on_whitespace) {
     -+				/*
     -+				 * Split at first whitespace, tying off the beginning
     -+				 * of the string and saving the remainder (or NULL) in
     -+				 * data.rest.
     -+				 */
     -+				char *p = strpbrk(input.buf, " \t");
     -+				if (p) {
     -+					while (*p && strchr(" \t", *p))
     -+						*p++ = '\0';
     -+				}
     -+				data.rest = p;
     - 			}
     --			data.rest = p;
     -+			batch_one_object(input.buf, &output, opt, &data);
     - 		}
     --
     --		batch_one_object(input.buf, &output, opt, &data);
     ++		goto cleanup;
     ++	}
     + 	while (strbuf_getline(&input, stdin) != EOF) {
     + 		if (data.split_on_whitespace) {
     + 			/*
     +@@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
     + 		batch_one_object(input.buf, &output, opt, &data);
       	}
       
     ++ cleanup:
       	strbuf_release(&input);
     + 	strbuf_release(&output);
     + 	warn_on_object_refname_ambiguity = save_warning;
      @@ builtin/cat-file.c: static int batch_option_callback(const struct option *opt,
       
       	bo->enabled = 1;
     @@ builtin/cat-file.c: static int batch_option_callback(const struct option *opt,
       
       	return 0;
      @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *prefix)
     - 			N_("show info about objects fed from the standard input"),
     + 			N_("like --batch, but don't emit <contents>"),
       			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
       			batch_option_callback),
     -+		OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
     -+			 N_("enters batch mode that accepts commands"),
     -+			 PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
     -+			 batch_option_callback),
     -+
     - 		OPT_BOOL(0, "follow-symlinks", &batch.follow_symlinks,
     - 			 N_("follow in-tree symlinks (used with --batch or --batch-check)")),
     - 		OPT_BOOL(0, "batch-all-objects", &batch.all_objects,
     ++		OPT_CALLBACK_F(0, "batch-command", &batch, N_("format"),
     ++			N_("read commands from stdin"),
     ++			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
     ++			batch_option_callback),
     + 		OPT_CMDMODE(0, "batch-all-objects", &opt,
     + 			    N_("with --batch[-check]: ignores stdin, batches all known objects"), 'b'),
     + 		/* Batch-specific options */
      
       ## t/t1006-cat-file.sh ##
      @@ t/t1006-cat-file.sh: $content"
     @@ t/t1006-cat-file.sh: $content"
      +    test -z "$content" ||
      +    test_expect_success "--batch-command output of $type content is correct" '
      +	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
     -+	maybe_remove_timestamp "$(echo contents $sha1 | git cat-file --batch-command)" $no_ts >actual &&
     -+	test_cmp expect actual
     -+    '
     -+
     -+    test -z "$content" ||
     -+    test_expect_success "--batch-command session for $type content is correct" '
     -+	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
     -+	maybe_remove_timestamp \
     -+		"$(test_write_lines "begin" "$sha1" "get contents" | git cat-file --batch-command)" \
     -+		$no_ts >actual &&
     ++	maybe_remove_timestamp "$(test_write_lines "contents $sha1" \
     ++	| git cat-file --batch-command)" $no_ts >actual &&
      +	test_cmp expect actual
      +    '
      +
      +    test_expect_success "--batch-command output of $type info is correct" '
      +	echo "$sha1 $type $size" >expect &&
     -+	echo "info $sha1" | git cat-file --batch-command >actual &&
     -+	test_cmp expect actual
     -+    '
     -+
     -+    test_expect_success "--batch-command session for $type info is correct" '
     -+	echo "$sha1 $type $size" >expect &&
     -+	test_write_lines "begin" "$sha1" "get info" | git cat-file --batch-command >actual &&
     ++	test_write_lines "info $sha1" | git cat-file --batch-command >actual &&
      +	test_cmp expect actual
      +    '
      +
           test_expect_success "custom --batch-check format" '
       	echo "$type $sha1" >expect &&
       	echo $sha1 | git cat-file --batch-check="%(objecttype) %(objectname)" >actual &&
     -@@ t/t1006-cat-file.sh: test_expect_success '--batch-check without %(rest) considers whole line' '
     - '
     +@@ t/t1006-cat-file.sh: $content"
     +     '
     + }
       
     - tree_sha1=$(git write-tree)
     ++run_buffer_test_flush () {
     ++	type=$1
     ++	sha1=$2
     ++	size=$3
     ++
     ++	mkfifo input &&
     ++	test_when_finished 'rm input; exec 8<&-' &&
     ++	mkfifo output &&
     ++	exec 9<>output &&
     ++	test_when_finished 'rm output; exec 9<&-'
     ++	(
     ++		git cat-file --buffer --batch-command <input 2>err &
     ++		echo $! &&
     ++		wait $!
     ++	) >&9 &
     ++	sh_pid=$! &&
     ++	read cat_file_pid <&9 &&
     ++	test_when_finished "kill $cat_file_pid
     ++			    kill $sh_pid; wait $sh_pid; :" &&
     ++	echo "$sha1 $type $size" >expect &&
     ++	test_write_lines "info $sha1" flush "info $sha1" >input
     ++	# TODO - consume all available input, not just one
     ++	# line (see above).
     ++	# check output is flushed on exit
     ++	read actual <&9 &&
     ++	echo "$actual" >actual &&
     ++	test_cmp expect actual &&
     ++	test_must_be_empty err
     ++}
     ++
     ++run_buffer_test_no_flush () {
     ++	type=$1
     ++	sha1=$2
     ++	size=$3
     ++
     ++	touch output &&
     ++	test_when_finished 'rm output' &&
     ++	mkfifo input &&
     ++	test_when_finished 'rm input' &&
     ++	mkfifo pid &&
     ++	exec 9<>pid &&
     ++	test_when_finished 'rm pid; exec 9<&-'
     ++	(
     ++		git cat-file --buffer --batch-command <input >output &
     ++		echo $! &&
     ++		wait $!
     ++		echo $?
     ++	) >&9 &
     ++	sh_pid=$! &&
     ++	read cat_file_pid <&9 &&
     ++	test_when_finished "kill $cat_file_pid
     ++			    kill $sh_pid; wait $sh_pid; :" &&
     ++	test_write_lines "info $sha1" "info $sha1" &&
     ++	kill $cat_file_pid &&
     ++	read status <&9 &&
     ++	test_must_be_empty output
     ++}
      +
     - tree_size=$(($(test_oid rawsz) + 13))
     - tree_pretty_content="100644 blob $hello_sha1	hello"
     + hello_content="Hello World"
     + hello_size=$(strlen "$hello_content")
     + hello_sha1=$(echo_without_newline "$hello_content" | git hash-object --stdin)
     +@@ t/t1006-cat-file.sh: test_expect_success "setup" '
       
     -@@ t/t1006-cat-file.sh: test_expect_success \
     + run_tests 'blob' $hello_sha1 $hello_size "$hello_content" "$hello_content"
     + 
     ++test_expect_success PIPE '--batch-command --buffer with flush for blob info' '
     ++       run_buffer_test_flush blob $hello_sha1 $hello_size
     ++'
     ++
     ++test_expect_success PIPE '--batch-command --buffer without flush for blob info' '
     ++       run_buffer_test_no_flush blob $hello_sha1 $hello_size false
     ++'
     ++
     + test_expect_success '--batch-check without %(rest) considers whole line' '
     + 	echo "$hello_sha1 blob $hello_size" >expect &&
     + 	git update-index --add --cacheinfo 100644 $hello_sha1 "white space" &&
     +@@ t/t1006-cat-file.sh: tree_pretty_content="100644 blob $hello_sha1	hello"
     + 
     + run_tests 'tree' $tree_sha1 $tree_size "" "$tree_pretty_content"
     + 
     ++test_expect_success PIPE '--batch-command --buffer with flush for tree info' '
     ++       run_buffer_test_flush tree $tree_sha1 $tree_size
     ++'
     ++
     ++test_expect_success PIPE '--batch-command --buffer without flush for tree info' '
     ++       run_buffer_test_no_flush tree $tree_sha1 $tree_size false
     ++'
     ++
     + commit_message="Initial commit"
     + commit_sha1=$(echo_without_newline "$commit_message" | git commit-tree $tree_sha1)
     + commit_size=$(($(test_oid hexsz) + 137))
     +@@ t/t1006-cat-file.sh: $commit_message"
     + 
     + run_tests 'commit' $commit_sha1 $commit_size "$commit_content" "$commit_content" 1
     + 
     ++test_expect_success PIPE '--batch-command --buffer with flush for commit info' '
     ++       run_buffer_test_flush commit $commit_sha1 $commit_size
     ++'
     ++
     ++test_expect_success PIPE '--batch-command --buffer without flush for commit info' '
     ++       run_buffer_test_no_flush commit $commit_sha1 $commit_size false
     ++'
     ++
     + tag_header_without_timestamp="object $hello_sha1
     + type blob
     + tag hellotag
     +@@ t/t1006-cat-file.sh: tag_size=$(strlen "$tag_content")
     + 
     + run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_content" 1
     + 
     ++test_expect_success PIPE '--batch-command --buffer with flush for tag info' '
     ++       run_buffer_test_flush tag $tag_sha1 $tag_size
     ++'
     ++
     ++test_expect_success PIPE '--batch-command --buffer without flush for tag info' '
     ++       run_buffer_test_no_flush tag $tag_sha1 $tag_size false
     ++'
     ++
     + test_expect_success \
           "Reach a blob from a tag pointing to it" \
           "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
       
     @@ t/t1006-cat-file.sh: test_expect_success "--batch-check with multiple sha1s give
           "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
       '
       
     -+test_expect_success "--batch-command with multiple sha1s gives correct format" '
     -+    echo "$batch_check_output" >expect &&
     -+    echo begin >input &&
     -+    echo_without_newline "$batch_check_input" >>input &&
     -+    echo "get info" >>input &&
     -+    git cat-file --batch-command <input >actual &&
     -+    test_cmp expect actual
     ++batch_command_info_input="info $hello_sha1
     ++info $tree_sha1
     ++info $commit_sha1
     ++info $tag_sha1
     ++info deadbeef
     ++info 
     ++flush
     ++"
     ++
     ++test_expect_success "--batch-command with multiple info calls gives correct format" '
     ++	test "$batch_check_output" = "$(echo_without_newline \
     ++	"$batch_command_info_input" | git cat-file --batch-command --buffer)"
     ++'
     ++
     ++batch_command_contents_input="contents $hello_sha1
     ++contents $commit_sha1
     ++contents $tag_sha1
     ++contents deadbeef
     ++contents 
     ++flush
     ++"
     ++
     ++test_expect_success "--batch-command with multiple contents calls gives correct format" '
     ++	test "$(maybe_remove_timestamp "$batch_output" 1)" = \
     ++	"$(maybe_remove_timestamp "$(echo_without_newline "$batch_command_contents_input" | git cat-file --batch-command)" 1)"
     ++'
     ++
     ++batch_command_mixed_input="info $hello_sha1
     ++contents $hello_sha1
     ++info $commit_sha1
     ++contents $commit_sha1
     ++info $tag_sha1
     ++contents $tag_sha1
     ++contents deadbeef
     ++info 
     ++flush
     ++"
     ++
     ++batch_command_mixed_output="$hello_sha1 blob $hello_size
     ++$hello_sha1 blob $hello_size
     ++$hello_content
     ++$commit_sha1 commit $commit_size
     ++$commit_sha1 commit $commit_size
     ++$commit_content
     ++$tag_sha1 tag $tag_size
     ++$tag_sha1 tag $tag_size
     ++$tag_content
     ++deadbeef missing
     ++ missing"
     ++
     ++test_expect_success "--batch-command with mixed calls gives correct format" '
     ++	test "$(maybe_remove_timestamp "$batch_command_mixed_output" 1)" = \
     ++	"$(maybe_remove_timestamp "$(echo_without_newline \
     ++	"$batch_command_mixed_input" | git cat-file --batch-command --buffer)" 1)"
      +'
      +
       test_expect_success 'setup blobs which are likely to delta' '
       	test-tool genrandom foo 10240 >foo &&
       	{ cat foo && echo plus; } >foo-plus &&
      @@ t/t1006-cat-file.sh: test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'
     + 	echo "$orig commit $orig_size" >expect &&
       	test_cmp expect actual
       '
     - 
     ++test_expect_success 'batch-command empty command' '
     ++	echo "" >cmd &&
     ++	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
     ++	grep -E "^fatal:.*empty command in input.*" err
     ++'
     ++
     ++test_expect_success 'batch-command whitespace before command' '
     ++	echo " info deadbeef" >cmd &&
     ++	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
     ++	grep -E "^fatal:.*whitespace before command.*" err
     ++'
     ++
      +test_expect_success 'batch-command unknown command' '
      +	echo unknown_command >cmd &&
     -+	test_expect_code 128 git cat-file --batch-command < cmd 2>err &&
     ++	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
      +	grep -E "^fatal:.*unknown command.*" err
      +'
      +
     ++test_expect_success 'batch-command flush with arguments' '
     ++	echo "flush arg" >cmd &&
     ++	test_expect_code 128 git cat-file --batch-command --buffer <cmd 2>err &&
     ++	grep -E "^fatal:.*flush takes no arguments.*" err
     ++'
     ++
     ++test_expect_success 'batch-command flush without --buffer' '
     ++	echo "flush arg" >cmd &&
     ++	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
     ++	grep -E "^fatal:.*flush is only for --buffer mode.*" err
     ++'
     ++
     ++test_expect_success 'batch-command flush empty queue' '
     ++	echo flush >cmd &&
     ++	test_expect_code 128 git cat-file --batch-command --buffer <cmd 2>err &&
     ++	grep -E "^fatal:.*nothing to flush.*" err
     ++'
     + 
       test_done

gitgitgadget

From: John Cai <[email protected]>

When introducing a new flag --batch-command, we will add a flag on the batch_options struct that indicates whether or not an interactive command mode will be used that reads commands and arguments off of stdin.

An intuitive name for this flag would be "command", which can get confusing with the already existing cmdmode.

cmdmode refers to how the result output of the blob will be transformed, either according to --filter or --textconv. So transform_mode is a more descriptive name for the flag, and will not get confused with the new command flag to be added in the next commit.

Rename cmdmode to transform_mode in cat-file.c

---
 builtin/cat-file.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 7b3f42950ec..5f015e71096 100644
@@ -24,7 +24,7 @@ struct batch_options {
 	int buffer_output;
 	int all_objects;
 	int unordered;
-	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
+	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 

@@ -302,19 +302,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (opt->cmdmode) {
+		if (opt->transform_mode) {
 			char *contents;
 			unsigned long size;
 
 			if (!data->rest)
 				die("missing path for '%s'", oid_to_hex(oid));
 
-			if (opt->cmdmode == 'w') {
+			if (opt->transform_mode == 'w') {
 				if (filter_object(data->rest, 0100644, oid,
 						  &contents, &size))
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
-			} else if (opt->cmdmode == 'c') {
+			} else if (opt->transform_mode == 'c') {
 				enum object_type type;
 				if (!textconv_object(the_repository,
 						     data->rest, 0100644, oid,

@@ -326,7 +326,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
 			} else
-				BUG("invalid cmdmode: %c", opt->cmdmode);
+				BUG("invalid transform_mode: %c", opt->transform_mode);
 			batch_write(opt, contents, size);
 			free(contents);
 		} else {

@@ -529,7 +529,7 @@ static int batch_objects(struct batch_options *opt)
 	strbuf_expand(&output, opt->format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
-	if (opt->cmdmode)
+	if (opt->transform_mode)
 		data.split_on_whitespace = 1;
 
 	/*

@@ -742,7 +742,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	/* Return early if we're in batch mode? */
 	if (batch.enabled) {
 		if (opt_cw)
-			batch.cmdmode = opt;
+			batch.transform_mode = opt;
 		else if (opt && opt != 'b')
 			usage_msg_optf(_("'-%c' is incompatible with batch mode"),
 				       usage, options, opt);

gitgitgadget

Is "command" truly an intuitive name for "were we given '--batch-command' option?" bit? I dunno.

But I agree that "transform_mode" is much better name than "cmdmode" for what this member is about.

From: John Cai <[email protected]>

Add a new flag --batch-command that accepts 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 wouldn't need to keep both processes around, and instead just have one --batch-command process where we can flip between getting object info, and getting object contents. Since we have a pair of cat-file processes per repository, this means we can get rid of roughly half of long lived git cat-file processes. Given there are many repositories being accessed at any given time, this can lead to huge savings since on a given server.

git cat-file --batch-command

will enter an interactive command mode whereby the user can enter in commands and their arguments that get queued in memory:

<command1> [arg1] [arg2] NL
<command2> [arg1] [arg2] NL

When --buffer mode is used, commands will be queued in memory until a flush command is issued that execute them:

flush NL

The reason for a flush command is that when a consumer process (A) talks to a git cat-file process (B) and interactively writes to and reads from it in --buffer mode, (A) needs to be able to control when the buffer is flushed to stdout.

Currently, from (A)'s perspective, the only way is to either

1. kill (B)'s process
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.

With this mechanism of queueing up commands and letting (A) issue a flush command, process (A) can control when the buffer is flushed and can guarantee it will receive all of the output when in --buffer mode.

This patch adds the basic structure for adding command which can be extended in the future to add more commands. It also adds the following two commands (on top of the flush command):

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.

These can be used in the following way with --buffer:

contents <sha1> NL
object <sha1> NL
object <sha1> NL
contents <sha1> NL
flush
contents <sha1> NL
flush

When used without --buffer:

contents <sha1> NL
object <sha1> NL
object <sha1> NL
contents <sha1> NL
contents <sha1> NL

---
 Documentation/git-cat-file.txt |  19 ++++
 builtin/cat-file.c             | 124 +++++++++++++++++++++
 t/t1006-cat-file.sh            | 197 ++++++++++++++++++++++++++++++++-
 3 files changed, 339 insertions(+), 1 deletion(-)

index bef76f4dd06..618dbd15338 100644
@@ -96,6 +96,25 @@ OPTIONS
 	need to specify the path, separated by whitespace.  See the
 	section `BATCH OUTPUT` below for details.
 
+--batch-command::
+	Enter a command mode that reads commands and arguments from stdin.
+	May not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also need to
+	specify the path, separated by whitespace.  See the section
+	`BATCH OUTPUT` below for details.
+
+contents <object>::
+	Print object contents for object reference <object>
+
+info <object>::
+	Print object info for object reference <object>
+
+flush::
+	Execute all preceding commands that were issued since the beginning or
+	since the last flush command was issued. Only used with --buffer. When
+	--buffer is not used, commands are flushed each time without issuing
+	`flush`.
+
 --batch-all-objects::
 	Instead of reading a list of objects on stdin, perform the
 	requested batch operation on all objects in the repository and

gitgitgadget

One other benefit is that with explicit flushes, in a partial clone, this makes it possible to batch prefetch objects.

Hi Jonathan and John

Jonathan is there any overlap between what this series is trying to do and your proposal for a batch command[1]? For example would extending this series to get blob sizes be useful to you?

Best Wishes

Phillip

[1] https://lore.kernel.org/git/[email protected]/

There is overlap, yes. I'm not sure of the best way to resolve it, though. John mentions a substantial reduction ("roughly half") of Git processes [1], and if they foresee needing to access things other than object info and contents, it might be better to start with something more extensible, like my proposal for a specific batch command. (If not, they will encounter another increase in the number of processes.) If they think that they can make do with this patch for the time being, I think that's fine too: once this is merged (which will be earlier than any extensible batch command, for sure), they (and anyone else who needs batched object info and contents without the overhead of initializing all the data structures in Git) can make use of this improvement.

As for getting blob sizes, I think that --batch-check can already give it to us. If that is the case, the series is fine as-is (at least in that aspect).

[1] https://lore.kernel.org/git/1b63164ad4d9ec6b5fa6f733b6095b2779298b36.1644251611.git.gitgitgadget@gmail.com/

I agree that it would be ideal to have just one way generic and extensible enough. I do not know if there are much difference in that area between the two approaches, though. The RFC I saw did look more complex and rigidly specified with framing and such, but that is only the syntax part---in the way in which interaction between two processes happen, I didn't quite see fundamental differences. I'd expect it wouldn't be too much trouble to add new commands to code written using either approach (although I haven't seen yours yet ;-).

Thanks.

It is similar to this approach, except:
- the approach I sent out uses pkt-line, which might be difficult to retrofit to cat-file if we need it
- in the future, we want the Git side to be able to initiate requests
- (possibly minor) it may be confusing if we add functionality to cat-file that is not about reading objects

Other than that, yes, they are similar.

If you mean you take one command with its args per line, use LF not NL.

    $ git grep '\<NL\>' Documentation
    $ git grep '\<LF\>' Documentation

We may want to fix the sole offender in Documentation/config.txt file (#leftoverbits).

Are we giving them guarantee when output will *not* come? If (B) is allowed to flush when it thinks it has too much in-core, it would mean that (A) cannot keep issuing commands forever without reading the response from (B), because (B) will eventually be blocked when it tries to flush to a pipe that (A) is not reading. I think there should be some discussion on that, too. IOW, --batch-command does not allow (B) to flush until it gets "flush", or something like that.

		ALLOC_GROW(cmds, nr + 1, alloc);
		cmds[nr++] = call;
		call.fn = cmd->fn;
		call.line = xstrdup_or_null(p);

    --batch
    --batch-check
    --batch-command

Hi John

There is no object command

The feature proposal of adding a command interface to cat-file was first discussed in [A]. In [B], Taylor expressed the need for a fuller proposal before moving forward with a new flag. An RFC was created [C] and the idea was discussed more thoroughly, and overall it seemed like it was headed in the right direction.

This patch series consolidates the feedback from these different threads.

This patch series has three parts:

1. preparation patch to rename a variable
2. adding an enum to keep track of batch modes
3. logic to handle --batch-command flag, adding contents, info, flush commands

Changes since v2:

* added enum to keep track of which batch mode we are in (thanks to Junio's feedback)
* fixed array allocation logic (thanks to Junio's feedback)
* added code to flush commands when --batch-commands receives an EOF and exits (thanks to Phillip's feedback)
* fixed docs formatting (thanks to Jonathan's feedback)

Changes since v1:

* simplified "session" mechanism. "flush" will execute all commands that were entered in since the last "flush" when in --buffer mode
* when not in --buffer mode, each command is executed and flushed each time
* rename cmdmode to transform_mode instead of just mode
* simplified command parsing logic
* changed rename of cmdmode to transform_mode
* clarified verbiage in commit messages

A. https://lore.kernel.org/git/[email protected]/ B.
https://lore.kernel.org/git/[email protected]/ C.
https://lore.kernel.org/git/[email protected]/

John Cai (3):
cat-file: rename cmdmode to transform_mode
cat-file: introduce batch_command enum to replace print_contents
cat-file: add --batch-command mode

 Documentation/git-cat-file.txt |  24 ++++
 builtin/cat-file.c             | 154 ++++++++++++++++++++++--
 t/t1006-cat-file.sh            | 207 ++++++++++++++++++++++++++++++++-
 3 files changed, 373 insertions(+), 12 deletions(-)

base-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1212%2Fjohn-cai%2Fjc-cat-file-batch-command-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1212/john-cai/jc-cat-file-batch-command-v3
Pull-Request: https://github.com/git/git/pull/1212

Range-diff vs v2:

1: 2d9a0b06ce5 ! 1: fa6294387ab cat-file: rename cmdmode to transform_mode
@@ Metadata
## Commit message ##
cat-file: rename cmdmode to transform_mode

- When introducing a new flag --batch-command, we will add a flag on the
- batch_options struct that indicates whether or not an interactive
- command mode will be used that reads commands and arguments off of
- stdin.
+ In the next patch, we will add an enum on the batch_options struct that
+ indicates which type of batch operation will be used: --batch,
+ --batch-check and the soon to be --batch-command that will read
+ commands from stdin. --batch-command mode might get confused with
+ the cmdmode flag.

- An intuitive name for this flag would be "command", which can get
- confusing with the already existing cmdmode.
-
- cmdmode refers to how the result output of the blob will be transformed,
- either according to --filter or --textconv. So transform_mode is a more
- descriptive name for the flag, and will not get confused with the new
- command flag to be added in the next commit.
+ There is value in renaming cmdmode in any case. cmdmode refers to how
+ the result output of the blob will be transformed, either according to
+ --filter or --textconv. So transform_mode is a more descriptive name
+ for the flag.

Rename cmdmode to transform_mode in cat-file.c

 -:  ----------- > 2:  ae2dfa512a7 cat-file: introduce batch_command enum to replace print_contents
 2:  1b63164ad4d ! 3:  1ab5524ee87 cat-file: add --batch-command mode
     @@ Commit message
          contents. Since we have a pair of cat-file processes per repository,
          this means we can get rid of roughly half of long lived git cat-file
          processes. Given there are many repositories being accessed at any given
     -    time, this can lead to huge savings since on a given server.
     +    time, this can lead to huge savings.
      
          git cat-file --batch-command
      
          will enter an interactive command mode whereby the user can enter in
          commands and their arguments that get queued in memory:
      
     -    <command1> [arg1] [arg2] NL
     -    <command2> [arg1] [arg2] NL
     +    <command1> [arg1] [arg2] LF
     +    <command2> [arg1] [arg2] LF
      
          When --buffer mode is used, commands will be queued in memory until a
          flush command is issued that execute them:
      
     -    flush NL
     +    flush LF
      
          The reason for a flush command is that when a consumer process (A)
          talks to a git cat-file process (B) and interactively writes to and
     @@ Commit message
          With this mechanism of queueing up commands and letting (A) issue a
          flush command, process (A) can control when the buffer is flushed and
          can guarantee it will receive all of the output when in --buffer mode.
     +    --batch-command also will not allow (B) to flush to stdout until a flush
     +    is received.
      
          This patch adds the basic structure for adding command which can be
          extended in the future to add more commands. It also adds the following
          two commands (on top of the flush command):
      
     -    contents <object> NL
     -    info <object> NL
     +    contents <object> LF
     +    info <object> LF
      
          The contents command takes an <object> argument and prints out the object
          contents.
     @@ Commit message
      
          These can be used in the following way with --buffer:
      
     -    contents <sha1> NL
     -    object <sha1> NL
     -    object <sha1> NL
     -    contents <sha1> NL
     +    info <sha1> LF
     +    contents <sha1> LF
     +    contents <sha1> LF
     +    info <sha1> LF
          flush
     -    contents <sha1> NL
     +    info <sha1> LF
          flush
      
          When used without --buffer:
      
     -    contents <sha1> NL
     -    object <sha1> NL
     -    object <sha1> NL
     -    contents <sha1> NL
     -    contents <sha1> NL
     +    info <sha1> LF
     +    contents <sha1> LF
     +    contents <sha1> LF
     +    info <sha1> LF
     +    info <sha1> LF
      
          Helped-by: Ævar Arnfjörð Bjarmason <[email protected]>
          Signed-off-by: John Cai <[email protected]>
     @@ Documentation/git-cat-file.txt: OPTIONS
      +	`--textconv` or `--filters`, in which case the input lines also need to
      +	specify the path, separated by whitespace.  See the section
      +	`BATCH OUTPUT` below for details.
     -+
     +++
     ++--
      +contents <object>::
     -+	Print object contents for object reference <object>
     ++	Print object contents for object reference <object>. This corresponds to
     ++	the output of --batch.
      +
      +info <object>::
     -+	Print object info for object reference <object>
     ++	Print object info for object reference <object>. This corresponds to the
     ++	output of --batch-check.
      +
      +flush::
     -+	Execute all preceding commands that were issued since the beginning or
     -+	since the last flush command was issued. Only used with --buffer. When
     -+	--buffer is not used, commands are flushed each time without issuing
     -+	`flush`.
     ++	Used in --buffer mode to execute all preceding commands that were issued
     ++	since the beginning or since the last flush was issued. When --buffer
     ++	is used, no output will come until flush is issued. When --buffer is not
     ++	used, commands are flushed each time without issuing `flush`.
     ++--
     +++
      +
       --batch-all-objects::
       	Instead of reading a list of objects on stdin, perform the
       	requested batch operation on all objects in the repository and
      
       ## builtin/cat-file.c ##
     -@@ builtin/cat-file.c: struct batch_options {
     - 	int unordered;
     - 	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
     - 	const char *format;
     -+	int command;
     +@@
     + #include "object-store.h"
     + #include "promisor-remote.h"
     + 
     +-enum batch_command {
     +-	BATCH_COMMAND_CONTENTS,
     +-	BATCH_COMMAND_INFO,
     ++enum batch_mode {
     ++	BATCH_MODE_CONTENTS,
     ++	BATCH_MODE_INFO,
     ++	BATCH_MODE_PARSE_CMDS,
       };
       
     - static const char *force_path;
     + struct batch_options {
     + 	int enabled;
     + 	int follow_symlinks;
     +-	enum batch_command command_mode;
     ++	enum batch_mode batch_mode;
     + 	int buffer_output;
     + 	int all_objects;
     + 	int unordered;
     +@@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
     + 	strbuf_addch(scratch, '\n');
     + 	batch_write(opt, scratch->buf, scratch->len);
     + 
     +-	if (opt->command_mode == BATCH_COMMAND_CONTENTS) {
     ++	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
     + 		print_object_or_die(opt, data);
     + 		batch_write(opt, "\n", 1);
     + 	}
      @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oid,
       				      data);
       }
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +			     struct strbuf *output,
      +			     struct expand_data *data)
      +{
     -+	opt->print_contents = 1;
     ++	opt->batch_mode = BATCH_MODE_CONTENTS;
      +	batch_one_object(line, output, opt, data);
      +}
      +
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +			   struct strbuf *output,
      +			   struct expand_data *data)
      +{
     -+	opt->print_contents = 0;
     ++	opt->batch_mode = BATCH_MODE_INFO;
      +	batch_one_object(line, output, opt, data);
      +}
      +
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +		struct strbuf *output,
      +		struct expand_data *data,
      +		struct queued_cmd *cmds,
     -+		int queued)
     ++		int nr)
      +{
      +	int i;
     -+	for(i = 0; i < queued; i++){
     ++	for (i = 0; i < nr; i++)
      +		cmds[i].fn(opt, cmds[i].line, output, data);
     -+	}
     ++
      +	fflush(stdout);
      +}
      +
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +	struct strbuf input = STRBUF_INIT;
      +	struct queued_cmd *cmds = NULL;
      +	size_t alloc = 0, nr = 0;
     -+	int queued = 0;
      +
      +	while (!strbuf_getline(&input, stdin)) {
      +		int i;
      +		const struct parse_cmd *cmd = NULL;
     -+		const char *p, *cmd_end;
     ++		const char *p = NULL, *cmd_end;
      +		struct queued_cmd call = {0};
      +
      +		if (!input.len)
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +				die(_("flush is only for --buffer mode"));
      +			if (*cmd_end)
      +				die(_("flush takes no arguments"));
     -+			if (!queued)
     -+				die(_("nothing to flush"));
     -+			flush_batch_calls(opt, output, data, cmds, queued);
     -+			queued = 0;
     ++			if (!nr)
     ++				error(_("nothing to flush"));
     ++
     ++			flush_batch_calls(opt, output, data, cmds, nr);
     ++			nr = 0;
      +			continue;
      +		}
      +
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +			cmd->fn(opt, p, output, data);
      +			continue;
      +		}
     -+
     -+		queued++;
     -+		if (queued > nr) {
     -+			ALLOC_GROW(cmds, nr+1, alloc);
     -+			nr++;
     -+		}
     -+
     ++		
     ++		ALLOC_GROW(cmds, nr + 1, alloc);
      +		call.fn = cmd->fn;
      +		call.line = xstrdup(p);
     -+		cmds[queued-1] = call;
     ++		cmds[nr++] = call;
      +	}
     ++
     ++	if (opt->buffer_output && nr)
     ++		flush_batch_calls(opt, output, data, cmds, nr);
     ++
      +	free(cmds);
      +	strbuf_release(&input);
      +}
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
       {
       	struct strbuf input = STRBUF_INIT;
      @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
     - 	struct expand_data data;
     - 	int save_warning;
     - 	int retval = 0;
     -+	const int command = opt->command;
     + 	 * If we are printing out the object, then always fill in the type,
     + 	 * since we will want to decide whether or not to stream.
     + 	 */
     +-	if (opt->command_mode == BATCH_COMMAND_CONTENTS)
     ++	if (opt->batch_mode == BATCH_MODE_CONTENTS)
     + 		data.info.typep = &data.type;
       
     - 	if (!opt->format)
     - 		opt->format = "%(objectname) %(objecttype) %(objectsize)";
     + 	if (opt->all_objects) {
      @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	save_warning = warn_on_object_refname_ambiguity;
       	warn_on_object_refname_ambiguity = 0;
       
     -+	if (command) {
     ++	if (opt->batch_mode == BATCH_MODE_PARSE_CMDS) {
      +		batch_objects_command(opt, &output, &data);
      +		goto cleanup;
      +	}
     @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	strbuf_release(&output);
       	warn_on_object_refname_ambiguity = save_warning;
      @@ builtin/cat-file.c: static int batch_option_callback(const struct option *opt,
     + 	}
       
       	bo->enabled = 1;
     - 	bo->print_contents = !strcmp(opt->long_name, "batch");
     -+	bo->command = !strcmp(opt->long_name, "batch-command");
     +-
     + 	if (!strcmp(opt->long_name, "batch"))
     +-		bo->command_mode = BATCH_COMMAND_CONTENTS;
     ++		bo->batch_mode = BATCH_MODE_CONTENTS;
     + 	if (!strcmp(opt->long_name, "batch-check"))
     +-		bo->command_mode = BATCH_COMMAND_INFO;
     ++		bo->batch_mode = BATCH_MODE_INFO;
     ++	if (!strcmp(opt->long_name, "batch-command"))
     ++		bo->batch_mode = BATCH_MODE_PARSE_CMDS;
     + 
       	bo->format = arg;
       
     - 	return 0;
      @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *prefix)
       			N_("like --batch, but don't emit <contents>"),
       			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
     @@ t/t1006-cat-file.sh: $content"
       	test_cmp expect actual
           '
       
     -+    test -z "$content" ||
     -+    test_expect_success "--batch-command output of $type content is correct" '
     -+	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
     -+	maybe_remove_timestamp "$(test_write_lines "contents $sha1" \
     -+	| git cat-file --batch-command)" $no_ts >actual &&
     -+	test_cmp expect actual
     -+    '
     -+
     -+    test_expect_success "--batch-command output of $type info is correct" '
     -+	echo "$sha1 $type $size" >expect &&
     -+	test_write_lines "info $sha1" | git cat-file --batch-command >actual &&
     -+	test_cmp expect actual
     -+    '
     ++    for opt in --buffer --no-buffer
     ++    do
     ++	test -z "$content" ||
     ++		test_expect_success "--batch-command $opt output of $type content is correct" '
     ++		maybe_remove_timestamp "$batch_output" $no_ts >expect &&
     ++		maybe_remove_timestamp "$(test_write_lines "contents $sha1" \
     ++		| git cat-file --batch-command $opt)" $no_ts >actual &&
     ++		test_cmp expect actual
     ++	'
     ++
     ++	test_expect_success "--batch-command $opt output of $type info is correct" '
     ++		echo "$sha1 $type $size" >expect &&
     ++		test_write_lines "info $sha1" \
     ++		| git cat-file --batch-command $opt >actual &&
     ++		test_cmp expect actual
     ++	'
     ++    done
      +
           test_expect_success "custom --batch-check format" '
       	echo "$type $sha1" >expect &&
     @@ t/t1006-cat-file.sh: $content"
      +	exec 9<>output &&
      +	test_when_finished 'rm output; exec 9<&-'
      +	(
     ++		# TODO - Ideally we'd pipe the output of cat-file
     ++		# through "sed s'/$/\\/'" to make sure that that read
     ++		# would consume all the available
     ++		# output. Unfortunately we cannot do this as we cannot
     ++		# control when sed flushes its output. We could write
     ++		# a test helper in C that appended a '\' to the end of
     ++		# each line and flushes its output after every line.
      +		git cat-file --buffer --batch-command <input 2>err &
      +		echo $! &&
      +		wait $!
     @@ t/t1006-cat-file.sh: $content"
      +	test_write_lines "info $sha1" flush "info $sha1" >input
      +	# TODO - consume all available input, not just one
      +	# line (see above).
     -+	# check output is flushed on exit
      +	read actual <&9 &&
      +	echo "$actual" >actual &&
      +	test_cmp expect actual &&
     @@ t/t1006-cat-file.sh: test_expect_success 'cat-file --batch-all-objects --batch-c
      +
      +test_expect_success 'batch-command flush empty queue' '
      +	echo flush >cmd &&
     -+	test_expect_code 128 git cat-file --batch-command --buffer <cmd 2>err &&
     -+	grep -E "^fatal:.*nothing to flush.*" err
     ++	test_expect_code 0 git cat-file --batch-command --buffer <cmd 2>err &&
     ++	grep -E "^error:.*nothing to flush.*" err
      +'
       
       test_done

gitgitgadget

From: John Cai <[email protected]>

In the next patch, we will add an enum on the batch_options struct that indicates which type of batch operation will be used: --batch, --batch-check and the soon to be --batch-command that will read commands from stdin. --batch-command mode might get confused with the cmdmode flag.

There is value in renaming cmdmode in any case. cmdmode refers to how the result output of the blob will be transformed, either according to --filter or --textconv. So transform_mode is a more descriptive name for the flag.

Rename cmdmode to transform_mode in cat-file.c

---
 builtin/cat-file.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 7b3f42950ec..5f015e71096 100644
@@ -24,7 +24,7 @@ struct batch_options {
 	int buffer_output;
 	int all_objects;
 	int unordered;
-	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
+	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 

@@ -302,19 +302,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (opt->cmdmode) {
+		if (opt->transform_mode) {
 			char *contents;
 			unsigned long size;
 
 			if (!data->rest)
 				die("missing path for '%s'", oid_to_hex(oid));
 
-			if (opt->cmdmode == 'w') {
+			if (opt->transform_mode == 'w') {
 				if (filter_object(data->rest, 0100644, oid,
 						  &contents, &size))
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
-			} else if (opt->cmdmode == 'c') {
+			} else if (opt->transform_mode == 'c') {
 				enum object_type type;
 				if (!textconv_object(the_repository,
 						     data->rest, 0100644, oid,

@@ -326,7 +326,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
 			} else
-				BUG("invalid cmdmode: %c", opt->cmdmode);
+				BUG("invalid transform_mode: %c", opt->transform_mode);
 			batch_write(opt, contents, size);
 			free(contents);
 		} else {

@@ -529,7 +529,7 @@ static int batch_objects(struct batch_options *opt)
 	strbuf_expand(&output, opt->format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
-	if (opt->cmdmode)
+	if (opt->transform_mode)
 		data.split_on_whitespace = 1;
 
 	/*

@@ -742,7 +742,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	/* Return early if we're in batch mode? */
 	if (batch.enabled) {
 		if (opt_cw)
-			batch.cmdmode = opt;
+			batch.transform_mode = opt;
 		else if (opt && opt != 'b')
 			usage_msg_optf(_("'-%c' is incompatible with batch mode"),
 				       usage, options, opt);

gitgitgadget

From: John Cai <[email protected]>

The next patch introduces a new --batch-command flag. Including --batch and --batch-check, we will have a total of three batch modes. Currently, from the batch_options struct's perspective, print_options is the only member used to distinguish between the different modes. This makes the code harder to read.

To reduce potential confusion, replace print_contents with an enum to help readability and clarity.

---
 builtin/cat-file.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

index 5f015e71096..1c673385868 100644
@@ -17,10 +17,15 @@
 #include "object-store.h"
 #include "promisor-remote.h"
 
+enum batch_command {
+	BATCH_COMMAND_CONTENTS,
+	BATCH_COMMAND_INFO,
+};
+
 struct batch_options {
 	int enabled;
 	int follow_symlinks;
-	int print_contents;
+	enum batch_command command_mode;
 	int buffer_output;
 	int all_objects;
 	int unordered;

@@ -386,7 +391,7 @@ static void batch_object_write(const char *obj_name,
 	strbuf_addch(scratch, '\n');
 	batch_write(opt, scratch->buf, scratch->len);
 
-	if (opt->print_contents) {
+	if (opt->command_mode == BATCH_COMMAND_CONTENTS) {
 		print_object_or_die(opt, data);
 		batch_write(opt, "\n", 1);
 	}

@@ -536,7 +541,7 @@ static int batch_objects(struct batch_options *opt)
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
 	 */
-	if (opt->print_contents)
+	if (opt->command_mode == BATCH_COMMAND_CONTENTS)
 		data.info.typep = &data.type;
 
 	if (opt->all_objects) {

@@ -635,7 +640,12 @@ static int batch_option_callback(const struct option *opt,
 	}
 
 	bo->enabled = 1;
-	bo->print_contents = !strcmp(opt->long_name, "batch");
+
+	if (!strcmp(opt->long_name, "batch"))
+		bo->command_mode = BATCH_COMMAND_CONTENTS;
+	if (!strcmp(opt->long_name, "batch-check"))
+		bo->command_mode = BATCH_COMMAND_INFO;
+
 	bo->format = arg;
 
 	return 0;

gitgitgadget

From: John Cai <[email protected]>

Add a new flag --batch-command that accepts 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 wouldn't need to keep both processes around, and instead just have one --batch-command process where we can flip between getting object info, and getting object contents. Since we have a pair of cat-file processes per repository, this means we can get rid of roughly half of long lived git cat-file processes. Given there are many repositories being accessed at any given time, this can lead to huge savings.

git cat-file --batch-command

will enter an interactive command mode whereby the user can enter in commands and their arguments that get queued in memory:

<command1> [arg1] [arg2] LF
<command2> [arg1] [arg2] LF

When --buffer mode is used, commands will be queued in memory until a flush command is issued that execute them:

flush LF

The reason for a flush command is that when a consumer process (A) talks to a git cat-file process (B) and interactively writes to and reads from it in --buffer mode, (A) needs to be able to control when the buffer is flushed to stdout.

Currently, from (A)'s perspective, the only way is to either

1. kill (B)'s process
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.

With this mechanism of queueing up commands and letting (A) issue a flush command, process (A) can control when the buffer is flushed and can guarantee it will receive all of the output when in --buffer mode. --batch-command also will not allow (B) to flush to stdout until a flush is received.

This patch adds the basic structure for adding command which can be extended in the future to add more commands. It also adds the following two commands (on top of the flush command):

contents <object> LF
info <object> LF

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.

These can be used in the following way with --buffer:

info <sha1> LF
contents <sha1> LF
contents <sha1> LF
info <sha1> LF
flush
info <sha1> LF
flush

When used without --buffer:

info <sha1> LF
contents <sha1> LF
contents <sha1> LF
info <sha1> LF
info <sha1> LF

---
 Documentation/git-cat-file.txt |  24 ++++
 builtin/cat-file.c             | 140 ++++++++++++++++++++--
 t/t1006-cat-file.sh            | 207 ++++++++++++++++++++++++++++++++-
 3 files changed, 361 insertions(+), 10 deletions(-)

index bef76f4dd06..d77a61c47de 100644
@@ -96,6 +96,30 @@ OPTIONS
 	need to specify the path, separated by whitespace.  See the
 	section `BATCH OUTPUT` below for details.
 
+--batch-command::
+	Enter a command mode that reads commands and arguments from stdin.
+	May not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also need to
+	specify the path, separated by whitespace.  See the section
+	`BATCH OUTPUT` below for details.
++
+--
+contents <object>::
+	Print object contents for object reference <object>. This corresponds to
+	the output of --batch.
+
+info <object>::
+	Print object info for object reference <object>. This corresponds to the
+	output of --batch-check.
+
+flush::
+	Used in --buffer mode to execute all preceding commands that were issued
+	since the beginning or since the last flush was issued. When --buffer
+	is used, no output will come until flush is issued. When --buffer is not
+	used, commands are flushed each time without issuing `flush`.
+--
++
+
 --batch-all-objects::
 	Instead of reading a list of objects on stdin, perform the
 	requested batch operation on all objects in the repository and

gitgitgadget

Does t1006-cat-file.sh pass the --stress test? I have no time to test it for you but I've seen "make test" got stuck and this is the only cat-file related change in flight.

Hi Junio

Yes it looks like there are some failures. Thanks for pointing this out. It looks like the flush test is getting stuck. I can actually reproduce it on my end when I do a make clean in t/ and then run the test. Will investigate.

thanks!

I believe this was the culprit, as the stress tests that failed passed once I removed this:

index 9428a04482..a20c8dae85 100755
@@ -237,7 +237,7 @@ run_buffer_test_flush () {
        size=$3
 
        mkfifo input &&
-       test_when_finished 'rm input; exec 8<&-' &&
+       test_when_finished 'rm input' &&
        mkfifo output &&
        exec 9<>output &&
        test_when_finished 'rm output; exec 9<&-'

I was closing a file descriptor 8 that was never opened. But, I don't fully understand why that would create problems.

The feature proposal of adding a command interface to cat-file was first discussed in [A]. In [B], Taylor expressed the need for a fuller proposal before moving forward with a new flag. An RFC was created [C] and the idea was discussed more thoroughly, and overall it seemed like it was headed in the right direction.

This patch series consolidates the feedback from these different threads.

This patch series has three parts:

1. preparation patch to rename a variable
2. adding an enum to keep track of batch modes
3. logic to handle --batch-command flag, adding contents, info, flush commands

Changes since v3 (thanks to Junio's feedback):

* added cascading logic in batch_options_callback()
* free memory for queued call input lines
* do not throw error when flushing an empty queue
* renamed cmds array to singular queued_cmd
* fixed flaky test that failed --stress

Changes since v2:

* added enum to keep track of which batch mode we are in (thanks to Junio's feedback)
* fixed array allocation logic (thanks to Junio's feedback)
* added code to flush commands when --batch-commands receives an EOF and exits (thanks to Phillip's feedback)
* fixed docs formatting (thanks to Jonathan's feedback)

Changes since v1:

* simplified "session" mechanism. "flush" will execute all commands that were entered in since the last "flush" when in --buffer mode
* when not in --buffer mode, each command is executed and flushed each time
* rename cmdmode to transform_mode instead of just mode
* simplified command parsing logic
* changed rename of cmdmode to transform_mode
* clarified verbiage in commit messages

A. https://lore.kernel.org/git/[email protected]/ B.
https://lore.kernel.org/git/[email protected]/ C.
https://lore.kernel.org/git/[email protected]/

John Cai (3):
cat-file: rename cmdmode to transform_mode
cat-file: introduce batch_mode enum to replace print_contents
cat-file: add --batch-command mode

 Documentation/git-cat-file.txt |  24 ++++
 builtin/cat-file.c             | 159 +++++++++++++++++++++++--
 t/t1006-cat-file.sh            | 211 ++++++++++++++++++++++++++++++++-
 3 files changed, 382 insertions(+), 12 deletions(-)

base-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1212%2Fjohn-cai%2Fjc-cat-file-batch-command-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1212/john-cai/jc-cat-file-batch-command-v4
Pull-Request: https://github.com/git/git/pull/1212

Range-diff vs v3:

 1:  fa6294387ab = 1:  fa6294387ab cat-file: rename cmdmode to transform_mode
 2:  ae2dfa512a7 ! 2:  81bc5ae1fc1 cat-file: introduce batch_command enum to replace print_contents
     @@ Metadata
      Author: John Cai <[email protected]>
      
       ## Commit message ##
     -    cat-file: introduce batch_command enum to replace print_contents
     +    cat-file: introduce batch_mode enum to replace print_contents
      
          The next patch introduces a new --batch-command flag. Including --batch
          and --batch-check, we will have a total of three batch modes. Currently,
     @@ builtin/cat-file.c
       #include "object-store.h"
       #include "promisor-remote.h"
       
     -+enum batch_command {
     -+	BATCH_COMMAND_CONTENTS,
     -+	BATCH_COMMAND_INFO,
     ++enum batch_mode {
     ++	BATCH_MODE_CONTENTS,
     ++	BATCH_MODE_INFO,
      +};
      +
       struct batch_options {
       	int enabled;
       	int follow_symlinks;
      -	int print_contents;
     -+	enum batch_command command_mode;
     ++	enum batch_mode batch_mode;
       	int buffer_output;
       	int all_objects;
       	int unordered;
     @@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
       	batch_write(opt, scratch->buf, scratch->len);
       
      -	if (opt->print_contents) {
     -+	if (opt->command_mode == BATCH_COMMAND_CONTENTS) {
     ++	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
       		print_object_or_die(opt, data);
       		batch_write(opt, "\n", 1);
       	}
     @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	 * since we will want to decide whether or not to stream.
       	 */
      -	if (opt->print_contents)
     -+	if (opt->command_mode == BATCH_COMMAND_CONTENTS)
     ++	if (opt->batch_mode == BATCH_MODE_CONTENTS)
       		data.info.typep = &data.type;
       
       	if (opt->all_objects) {
     @@ builtin/cat-file.c: static int batch_option_callback(const struct option *opt,
       	bo->enabled = 1;
      -	bo->print_contents = !strcmp(opt->long_name, "batch");
      +
     -+	if (!strcmp(opt->long_name, "batch"))
     -+		bo->command_mode = BATCH_COMMAND_CONTENTS;
     -+	if (!strcmp(opt->long_name, "batch-check"))
     -+		bo->command_mode = BATCH_COMMAND_INFO;
     ++	if (!strcmp(opt->long_name, "batch")) {
     ++		bo->batch_mode = BATCH_MODE_CONTENTS;
     ++	} else if (!strcmp(opt->long_name, "batch-check")) {
     ++		bo->batch_mode = BATCH_MODE_INFO;
     ++	} else {
     ++		BUG("%s given to batch-option-callback", opt->long_name);
     ++	}
      +
       	bo->format = arg;

3: 1ab5524ee87 ! 3: 6c51324a662 cat-file: add --batch-command mode

     @@ Documentation/git-cat-file.txt: OPTIONS
      
       ## builtin/cat-file.c ##
      @@
     - #include "object-store.h"
     - #include "promisor-remote.h"
     - 
     --enum batch_command {
     --	BATCH_COMMAND_CONTENTS,
     --	BATCH_COMMAND_INFO,
     -+enum batch_mode {
     -+	BATCH_MODE_CONTENTS,
     -+	BATCH_MODE_INFO,
     -+	BATCH_MODE_PARSE_CMDS,
     + enum batch_mode {
     + 	BATCH_MODE_CONTENTS,
     + 	BATCH_MODE_INFO,
     ++	BATCH_MODE_QUEUE_AND_DISPATCH,
       };
       
       struct batch_options {
     - 	int enabled;
     - 	int follow_symlinks;
     --	enum batch_command command_mode;
     -+	enum batch_mode batch_mode;
     - 	int buffer_output;
     - 	int all_objects;
     - 	int unordered;
     -@@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
     - 	strbuf_addch(scratch, '\n');
     - 	batch_write(opt, scratch->buf, scratch->len);
     - 
     --	if (opt->command_mode == BATCH_COMMAND_CONTENTS) {
     -+	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
     - 		print_object_or_die(opt, data);
     - 		batch_write(opt, "\n", 1);
     - 	}
      @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oid,
       				      data);
       }
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +
      +struct queued_cmd {
      +	parse_cmd_fn_t fn;
     -+	const char *line;
     ++	char *line;
      +};
      +
      +static void parse_cmd_contents(struct batch_options *opt,
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +	batch_one_object(line, output, opt, data);
      +}
      +
     -+static void flush_batch_calls(struct batch_options *opt,
     ++static void dispatch_calls(struct batch_options *opt,
      +		struct strbuf *output,
      +		struct expand_data *data,
     -+		struct queued_cmd *cmds,
     ++		struct queued_cmd *cmd,
      +		int nr)
      +{
      +	int i;
     -+	for (i = 0; i < nr; i++)
     -+		cmds[i].fn(opt, cmds[i].line, output, data);
     ++
     ++	for (i = 0; i < nr; i++){
     ++		cmd[i].fn(opt, cmd[i].line, output, data);
     ++		free(cmd[i].line);
     ++	}
      +
      +	fflush(stdout);
      +}
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +				    struct expand_data *data)
      +{
      +	struct strbuf input = STRBUF_INIT;
     -+	struct queued_cmd *cmds = NULL;
     ++	struct queued_cmd *queued_cmd = NULL;
      +	size_t alloc = 0, nr = 0;
      +
      +	while (!strbuf_getline(&input, stdin)) {
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +				die(_("flush is only for --buffer mode"));
      +			if (*cmd_end)
      +				die(_("flush takes no arguments"));
     -+			if (!nr)
     -+				error(_("nothing to flush"));
      +
     -+			flush_batch_calls(opt, output, data, cmds, nr);
     ++			dispatch_calls(opt, output, data, queued_cmd, nr);
      +			nr = 0;
      +			continue;
      +		}
     @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oi
      +			cmd->fn(opt, p, output, data);
      +			continue;
      +		}
     -+		
     -+		ALLOC_GROW(cmds, nr + 1, alloc);
     ++
     ++		ALLOC_GROW(queued_cmd, nr + 1, alloc);
      +		call.fn = cmd->fn;
     -+		call.line = xstrdup(p);
     -+		cmds[nr++] = call;
     ++		call.line = xstrdup_or_null(p);
     ++		queued_cmd[nr++] = call;
      +	}
      +
      +	if (opt->buffer_output && nr)
     -+		flush_batch_calls(opt, output, data, cmds, nr);
     ++		dispatch_calls(opt, output, data, queued_cmd, nr);
      +
     -+	free(cmds);
     ++	free(queued_cmd);
      +	strbuf_release(&input);
      +}
      +
       static int batch_objects(struct batch_options *opt)
       {
       	struct strbuf input = STRBUF_INIT;
     -@@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
     - 	 * If we are printing out the object, then always fill in the type,
     - 	 * since we will want to decide whether or not to stream.
     - 	 */
     --	if (opt->command_mode == BATCH_COMMAND_CONTENTS)
     -+	if (opt->batch_mode == BATCH_MODE_CONTENTS)
     - 		data.info.typep = &data.type;
     - 
     - 	if (opt->all_objects) {
      @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	save_warning = warn_on_object_refname_ambiguity;
       	warn_on_object_refname_ambiguity = 0;
       
     -+	if (opt->batch_mode == BATCH_MODE_PARSE_CMDS) {
     ++	if (opt->batch_mode == BATCH_MODE_QUEUE_AND_DISPATCH) {
      +		batch_objects_command(opt, &output, &data);
      +		goto cleanup;
      +	}
     @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
       	strbuf_release(&output);
       	warn_on_object_refname_ambiguity = save_warning;
      @@ builtin/cat-file.c: static int batch_option_callback(const struct option *opt,
     + 		bo->batch_mode = BATCH_MODE_CONTENTS;
     + 	} else if (!strcmp(opt->long_name, "batch-check")) {
     + 		bo->batch_mode = BATCH_MODE_INFO;
     ++	} else if (!strcmp(opt->long_name, "batch-command")) {
     ++		bo->batch_mode = BATCH_MODE_QUEUE_AND_DISPATCH;
     + 	} else {
     + 		BUG("%s given to batch-option-callback", opt->long_name);
       	}
     - 
     - 	bo->enabled = 1;
     --
     - 	if (!strcmp(opt->long_name, "batch"))
     --		bo->command_mode = BATCH_COMMAND_CONTENTS;
     -+		bo->batch_mode = BATCH_MODE_CONTENTS;
     - 	if (!strcmp(opt->long_name, "batch-check"))
     --		bo->command_mode = BATCH_COMMAND_INFO;
     -+		bo->batch_mode = BATCH_MODE_INFO;
     -+	if (!strcmp(opt->long_name, "batch-command"))
     -+		bo->batch_mode = BATCH_MODE_PARSE_CMDS;
     - 
     - 	bo->format = arg;
     - 
      @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *prefix)
       			N_("like --batch, but don't emit <contents>"),
       			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
     @@ t/t1006-cat-file.sh: $content"
      +	size=$3
      +
      +	mkfifo input &&
     -+	test_when_finished 'rm input; exec 8<&-' &&
     ++	test_when_finished 'rm input' &&
      +	mkfifo output &&
      +	exec 9<>output &&
      +	test_when_finished 'rm output; exec 9<&-'
     @@ t/t1006-cat-file.sh: test_expect_success "--batch-check with multiple sha1s give
      +info $commit_sha1
      +info $tag_sha1
      +info deadbeef
     -+info 
      +flush
      +"
      +
     ++batch_command_info_output="$hello_sha1 blob $hello_size
     ++$tree_sha1 tree $tree_size
     ++$commit_sha1 commit $commit_size
     ++$tag_sha1 tag $tag_size
     ++deadbeef missing"
     ++
      +test_expect_success "--batch-command with multiple info calls gives correct format" '
     -+	test "$batch_check_output" = "$(echo_without_newline \
     ++	test "$batch_command_info_output" = "$(echo_without_newline \
      +	"$batch_command_info_input" | git cat-file --batch-command --buffer)"
      +'
      +
     @@ t/t1006-cat-file.sh: test_expect_success "--batch-check with multiple sha1s give
      +contents $commit_sha1
      +contents $tag_sha1
      +contents deadbeef
     -+contents 
      +flush
      +"
      +
     ++batch_command_output="$hello_sha1 blob $hello_size
     ++$hello_content
     ++$commit_sha1 commit $commit_size
     ++$commit_content
     ++$tag_sha1 tag $tag_size
     ++$tag_content
     ++deadbeef missing"
     ++
      +test_expect_success "--batch-command with multiple contents calls gives correct format" '
     -+	test "$(maybe_remove_timestamp "$batch_output" 1)" = \
     ++	test "$(maybe_remove_timestamp "$batch_command_output" 1)" = \
      +	"$(maybe_remove_timestamp "$(echo_without_newline "$batch_command_contents_input" | git cat-file --batch-command)" 1)"
      +'
      +
     @@ t/t1006-cat-file.sh: test_expect_success "--batch-check with multiple sha1s give
      +info $tag_sha1
      +contents $tag_sha1
      +contents deadbeef
     -+info 
      +flush
      +"
      +
     @@ t/t1006-cat-file.sh: test_expect_success "--batch-check with multiple sha1s give
      +$tag_sha1 tag $tag_size
      +$tag_sha1 tag $tag_size
      +$tag_content
     -+deadbeef missing
     -+ missing"
     ++deadbeef missing"
      +
      +test_expect_success "--batch-command with mixed calls gives correct format" '
      +	test "$(maybe_remove_timestamp "$batch_command_mixed_output" 1)" = \
     @@ t/t1006-cat-file.sh: test_expect_success 'cat-file --batch-all-objects --batch-c
      +	echo "flush arg" >cmd &&
      +	test_expect_code 128 git cat-file --batch-command <cmd 2>err &&
      +	grep -E "^fatal:.*flush is only for --buffer mode.*" err
     -+'
     -+
     -+test_expect_success 'batch-command flush empty queue' '
     -+	echo flush >cmd &&
     -+	test_expect_code 0 git cat-file --batch-command --buffer <cmd 2>err &&
     -+	grep -E "^error:.*nothing to flush.*" err
      +'
       
       test_done

gitgitgadget

From: John Cai <[email protected]>

In the next patch, we will add an enum on the batch_options struct that indicates which type of batch operation will be used: --batch, --batch-check and the soon to be --batch-command that will read commands from stdin. --batch-command mode might get confused with the cmdmode flag.

There is value in renaming cmdmode in any case. cmdmode refers to how the result output of the blob will be transformed, either according to --filter or --textconv. So transform_mode is a more descriptive name for the flag.

Rename cmdmode to transform_mode in cat-file.c

---
 builtin/cat-file.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 7b3f42950ec..5f015e71096 100644
@@ -24,7 +24,7 @@ struct batch_options {
 	int buffer_output;
 	int all_objects;
 	int unordered;
-	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
+	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
 	const char *format;
 };
 

@@ -302,19 +302,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
-		if (opt->cmdmode) {
+		if (opt->transform_mode) {
 			char *contents;
 			unsigned long size;
 
 			if (!data->rest)
 				die("missing path for '%s'", oid_to_hex(oid));
 
-			if (opt->cmdmode == 'w') {
+			if (opt->transform_mode == 'w') {
 				if (filter_object(data->rest, 0100644, oid,
 						  &contents, &size))
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
-			} else if (opt->cmdmode == 'c') {
+			} else if (opt->transform_mode == 'c') {
 				enum object_type type;
 				if (!textconv_object(the_repository,
 						     data->rest, 0100644, oid,

@@ -326,7 +326,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 					die("could not convert '%s' %s",
 					    oid_to_hex(oid), data->rest);
 			} else
-				BUG("invalid cmdmode: %c", opt->cmdmode);
+				BUG("invalid transform_mode: %c", opt->transform_mode);
 			batch_write(opt, contents, size);
 			free(contents);
 		} else {

@@ -529,7 +529,7 @@ static int batch_objects(struct batch_options *opt)
 	strbuf_expand(&output, opt->format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
-	if (opt->cmdmode)
+	if (opt->transform_mode)
 		data.split_on_whitespace = 1;
 
 	/*

@@ -742,7 +742,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	/* Return early if we're in batch mode? */
 	if (batch.enabled) {
 		if (opt_cw)
-			batch.cmdmode = opt;
+			batch.transform_mode = opt;
 		else if (opt && opt != 'b')
 			usage_msg_optf(_("'-%c' is incompatible with batch mode"),
 				       usage, options, opt);

gitgitgadget

From: John Cai <[email protected]>

The next patch introduces a new --batch-command flag. Including --batch and --batch-check, we will have a total of three batch modes. Currently, from the batch_options struct's perspective, print_options is the only member used to distinguish between the different modes. This makes the code harder to read.

To reduce potential confusion, replace print_contents with an enum to help readability and clarity.

---
 builtin/cat-file.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

index 5f015e71096..709510c6564 100644
@@ -17,10 +17,15 @@
 #include "object-store.h"
 #include "promisor-remote.h"
 
+enum batch_mode {
+	BATCH_MODE_CONTENTS,
+	BATCH_MODE_INFO,
+};
+
 struct batch_options {
 	int enabled;
 	int follow_symlinks;
-	int print_contents;
+	enum batch_mode batch_mode;
 	int buffer_output;
 	int all_objects;
 	int unordered;

@@ -386,7 +391,7 @@ static void batch_object_write(const char *obj_name,
 	strbuf_addch(scratch, '\n');
 	batch_write(opt, scratch->buf, scratch->len);
 
-	if (opt->print_contents) {
+	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
 		print_object_or_die(opt, data);
 		batch_write(opt, "\n", 1);
 	}

@@ -536,7 +541,7 @@ static int batch_objects(struct batch_options *opt)
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
 	 */
-	if (opt->print_contents)
+	if (opt->batch_mode == BATCH_MODE_CONTENTS)
 		data.info.typep = &data.type;
 
 	if (opt->all_objects) {

@@ -635,7 +640,15 @@ static int batch_option_callback(const struct option *opt,
 	}
 
 	bo->enabled = 1;
-	bo->print_contents = !strcmp(opt->long_name, "batch");
+
+	if (!strcmp(opt->long_name, "batch")) {
+		bo->batch_mode = BATCH_MODE_CONTENTS;
+	} else if (!strcmp(opt->long_name, "batch-check")) {
+		bo->batch_mode = BATCH_MODE_INFO;
+	} else {
+		BUG("%s given to batch-option-callback", opt->long_name);
+	}
+
 	bo->format = arg;
 
 	return 0;

gitgitgadget

Here you talk about "print_options"...

...but here it's "print_contents".

Also it would perhaps be a bit clearer if you introduced it saying something like "the print_contents flag (or boolean?) is the only member..."

       if (!strcmp(opt->long_name, "batch"))
               bo->batch_mode = BATCH_MODE_CONTENTS;
       else if (!strcmp(opt->long_name, "batch-check"))
               bo->batch_mode = BATCH_MODE_INFO;
       else
               BUG("%s given to batch-option-callback", opt->long_name);

From: John Cai <[email protected]>

Add a new flag --batch-command that accepts 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 wouldn't need to keep both processes around, and instead just have one --batch-command process where we can flip between getting object info, and getting object contents. Since we have a pair of cat-file processes per repository, this means we can get rid of roughly half of long lived git cat-file processes. Given there are many repositories being accessed at any given time, this can lead to huge savings.

git cat-file --batch-command

will enter an interactive command mode whereby the user can enter in commands and their arguments that get queued in memory:

<command1> [arg1] [arg2] LF
<command2> [arg1] [arg2] LF

When --buffer mode is used, commands will be queued in memory until a flush command is issued that execute them:

flush LF

The reason for a flush command is that when a consumer process (A) talks to a git cat-file process (B) and interactively writes to and reads from it in --buffer mode, (A) needs to be able to control when the buffer is flushed to stdout.

Currently, from (A)'s perspective, the only way is to either

1. kill (B)'s process
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.

With this mechanism of queueing up commands and letting (A) issue a flush command, process (A) can control when the buffer is flushed and can guarantee it will receive all of the output when in --buffer mode. --batch-command also will not allow (B) to flush to stdout until a flush is received.

This patch adds the basic structure for adding command which can be extended in the future to add more commands. It also adds the following two commands (on top of the flush command):

contents <object> LF
info <object> LF

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.

These can be used in the following way with --buffer:

info <sha1> LF
contents <sha1> LF
contents <sha1> LF
info <sha1> LF
flush
info <sha1> LF
flush

When used without --buffer:

info <sha1> LF
contents <sha1> LF
contents <sha1> LF
info <sha1> LF
info <sha1> LF

---
 Documentation/git-cat-file.txt |  24 ++++
 builtin/cat-file.c             | 124 +++++++++++++++++++
 t/t1006-cat-file.sh            | 211 ++++++++++++++++++++++++++++++++-
 3 files changed, 358 insertions(+), 1 deletion(-)

index bef76f4dd06..d77a61c47de 100644
@@ -96,6 +96,30 @@ OPTIONS
 	need to specify the path, separated by whitespace.  See the
 	section `BATCH OUTPUT` below for details.
 
+--batch-command::
+	Enter a command mode that reads commands and arguments from stdin.
+	May not be combined with any other options or arguments except
+	`--textconv` or `--filters`, in which case the input lines also need to
+	specify the path, separated by whitespace.  See the section
+	`BATCH OUTPUT` below for details.
++
+--
+contents <object>::
+	Print object contents for object reference <object>. This corresponds to
+	the output of --batch.
+
+info <object>::
+	Print object info for object reference <object>. This corresponds to the
+	output of --batch-check.
+
+flush::
+	Used in --buffer mode to execute all preceding commands that were issued
+	since the beginning or since the last flush was issued. When --buffer
+	is used, no output will come until flush is issued. When --buffer is not
+	used, commands are flushed each time without issuing `flush`.
+--
++
+
 --batch-all-objects::
 	Instead of reading a list of objects on stdin, perform the
 	requested batch operation on all objects in the repository and

gitgitgadget

Hi John

I've concentrated on the tests as others have commented on the implementation

	batch_command_info_input="info $hello_sha1\
	info $tree_sha1\
	info $commit_sha1\
	info $tag_sha1\
	info deadbeef\
	flush"
	
	cat >expect <<-EOF &&
	$hello_sha1 blob $hello_size
	$tree_sha1 tree $tree_size
	$commit_sha1 commit $commit_size
	$tag_sha1 tag $tag_size
	deadbeef missing
	EOF

	echo_without_newline "$batch_command_info_input" | git cat-file 

Yeah, very good point.

I admit I do not offhand recall what your tests did but help with this (and more) level of detail with an offer to collaborate is something I am very happy to see. Thanks for working well together.

One thing that I wasn't quite sure was how well failure cases are tested. If we ask, in a batch mode, "info" for two objects and then "flush", does the asker get enough clue when to read and when to stop reading with all four combinations of states, i.e. asking for two missing objects, one good object and one bad object, one bad object and one good object, two good objects, for example?

Testing such combinations reliably is tricky---if the asker needs to react to different response differently, a test that expects good and then bad may not just fail but can get into deadlock, for example if the reaction to good response has to read a lot but the reaction to bad response is to just consume the "bad object" notice, when a bug in the program being tested makes it issue the response for a bad case when the asker is expecting a response for a good object, because the asker will keep waiting for more response to read which may not come.

Hi Junio

This is a good point. We currently don't have tests that exercise these combinations.

Let me see if I understand you. What I'm hearing is that it's hard to test a git processes (A) that read/write from/to pipes without knowing exactly how (A) will behave. By necessity, the test logic will have embedded some logic in it that assumes certain behavior from (A), which might or might not be the case.

This can lead to a hanging test if, say, it is waiting around for (A) to output data when due to a bug in the code, it never does. Did I get that right?

I still see value in having a test that hangs when it doesn't receive expected output from the git process. If we had something that detected timeout on tests then this could catch such a case. But since we don't, then that means having tests like run_buffer_test_flush() and run_buffer_test_no_flush() will run the risk of being a deadlocked test if there is a regression of the code in the future. While still providing value in showing that something is wrong, these deadlocked tests can be inconvenient to debug.

Exactly. And we've seen such tests that are designed to hang, when they detect bugs, which made us very unhappy and we fixed them not to hang but reliably fail. Otherwise, such tests weren't very useful in unattended CI environment, which we do not want to wait for 3 hours to timeout and leave later steps in the same script untested.

Thanks.

Hi Junio

That makes sense. Do you have an example of one of these tests? I'd like to see how it was converted from a test that hung to a test that failed reliably. As I'm thinking about converting run_buffer_test_flush() and run_buffer_test_no_flush() into tests that fail rather than hang, I'm having a hard time avoiding the pattern of A writes to B and waits for B to respond.

Hi Phillip,

Thanks again for helping with this! a few comments/questions below:

I've restored the tests in the form you suggested. I had removed some lines to simplify the test but as it turns out I removed some of the important aspects of the test.

Here are my modifications to the tests you helped me with. Let me know if these changes make sense, or if I'm missing something.

yeah, that makes sense

true, we don't actually need it

sounds good, will adjust

Some comments not offered by other reviewers...

s/<sha1>/<object>/ for consistency with the usage information earlier in the commit message, and since Git is migrating to SHA-256, and to avoid reviewer confusion as occurred earlier[1].

Also: s/flush$/flush LF/

Ditto.

[1]: https://lore.kernel.org/git/CAPig+cTeqhOYTu9WBiY=LnZtt35hAp3Qa5RduC2yLut6p01_1w@mail.gmail.com/

This unconditional +1 is going to make p point beyond the NUL character if the input is just a bare command, such as "contents" or "info" without any space or any argument...