* [RFC v2] cat-file: add a --stdin-cmd mode
@ 2022-01-25 22:50 John Cai
2022-01-25 23:00 ` John Cai
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: John Cai @ 2022-01-25 22:50 UTC (permalink / raw)
To: git; +Cc: avarab, me, phillip.wood123, John Cai
This RFC patch proposes a new flag --stdin-cmd that works with
git-cat-file --batch. Similar to git-update-ref --stdin, it will accept
commands and arguments from stdin.
The start of this idea was discussed in [1], where the original
motivation was to be able to control when the buffer was flushed to
stdout in --buffer mode.
However, this can actually be much more useful in situations when
git-cat-file --batch is being used as a long lived backend query
process. At GitLab, we use a pair of cat-file processes. One for
iterating over object metadata with --batch-check, and the other to grab
object contents with --batch. However, if we had --stdin-cmd, we could
get rid of the second --batch-check process, and just have one progress
where we can flip between getting object info, and getting object contents.
This can lead to huge savings.
git cat-file --batch --stdin-cmd
$ <command> [arg1] [arg2] NL
We can also add a -z mode to allow for NUL-terminated lines
$ <command> [arg1] [arg2] NUL
This patch adds three commands: object, info, fflush
$ object <sha1> NL
$ info <sha1> NL
$ fflush NL
These three would be immediately useful in GitLab's context, but one can
imagine this mode to be further extended for other things.
For instance, a non-trivial part of "cat-file --batch" time is spent
on parsing its argument and seeing if it's a revision, ref etc. So we
could add a command that only accepts a full-length 40
character SHA-1.
This would be the first step in adding such an interface to
git-cat-file.
[1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
Changes from v1:
- changed option name to batch-command.
- changed command function interface to receive the whole line after the command
name to put the onus of parsing arguments to each individual command function.
- pass in whole line to batch_one_object in both parse_cmd_object and
parse_cmd_info to support spaces in the object reference.
- removed addition of -z to include in a separate patch series
- added documentation.
---
Documentation/git-cat-file.txt | 15 +++++
builtin/cat-file.c | 114 ++++++++++++++++++++++++++++++++-
strvec.c | 23 +++++++
strvec.h | 8 +++
t/t1006-cat-file.sh | 32 +++++++++
5 files changed, 191 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index bef76f4dd0..8aefa45e4c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -96,6 +96,21 @@ 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 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.
+
+object <object>::
+ Print object contents for object reference <object>
+
+info <object>::
+ Print object info for object reference <object>
+
+flush::
+ Flush to stdout immediately when used with --buffer
+
--batch-all-objects::
Instead of reading a list of objects on stdin, perform the
requested batch operation on all objects in the repository and
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7b3f42950e..30794284d5 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -16,6 +16,7 @@
#include "packfile.h"
#include "object-store.h"
#include "promisor-remote.h"
+#include "strvec.h"
struct batch_options {
int enabled;
@@ -26,7 +27,10 @@ struct batch_options {
int unordered;
int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
const char *format;
+ int stdin_cmd;
+ int end_null;
};
+static char line_termination = '\n';
static const char *force_path;
@@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,
data);
}
+enum batch_state {
+ /* Non-transactional state open for commands. */
+ BATCH_STATE_OPEN,
+};
+
+static void parse_cmd_object(struct batch_options *opt,
+ const char *line,
+ struct strbuf *output,
+ struct expand_data *data)
+{
+ opt->print_contents = 1;
+ batch_one_object(line, output, opt, data);
+}
+
+static void parse_cmd_info(struct batch_options *opt,
+ const char *line,
+ struct strbuf *output,
+ struct expand_data *data)
+{
+ opt->print_contents = 0;
+ batch_one_object(line, output, opt, data);
+}
+
+static void parse_cmd_fflush(struct batch_options *opt,
+ const char *line,
+ struct strbuf *output,
+ struct expand_data *data)
+{
+ fflush(stdout);
+}
+
+typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
+ struct strbuf *, struct expand_data *);
+
+static const struct parse_cmd {
+ const char *prefix;
+ parse_cmd_fn_t fn;
+ unsigned args;
+ enum batch_state state;
+} commands[] = {
+ { "object", parse_cmd_object, 1, BATCH_STATE_OPEN },
+ { "info", parse_cmd_info, 1, BATCH_STATE_OPEN },
+ { "fflush", parse_cmd_fflush, 0, BATCH_STATE_OPEN },
+};
+
+static void batch_objects_stdin_cmd(struct batch_options *opt,
+ struct strbuf *output,
+ struct expand_data *data)
+{
+ struct strbuf input = STRBUF_INIT;
+ enum batch_state state = BATCH_STATE_OPEN;
+
+ /* Read each line dispatch its command */
+ while (!strbuf_getwholeline(&input, stdin, line_termination)) {
+ int i;
+ const struct parse_cmd *cmd = NULL;
+ const char *p;
+
+ if (*input.buf == line_termination)
+ die("empty command in input");
+ else if (isspace(*input.buf))
+ die("whitespace before command: %s", input.buf);
+
+ for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ const char *prefix = commands[i].prefix;
+ char c;
+ const char *cmd_name;
+ if (!skip_prefix(input.buf, prefix, &cmd_name))
+ 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].args ? ' ' : line_termination;
+ if (input.buf[strlen(prefix)] != c)
+ continue;
+
+ cmd = &commands[i];
+ break;
+ }
+ if (!cmd)
+ die("unknown command: %s", input.buf);
+
+ p = input.buf + strlen(cmd->prefix) + 1;
+ const char *pos = strstr(p, &line_termination);
+
+ switch (state) {
+ case BATCH_STATE_OPEN:
+ break;
+ }
+ cmd->fn(opt, xstrndup(p, pos-p), output, data);
+ }
+ strbuf_release(&input);
+}
+
static int batch_objects(struct batch_options *opt)
{
struct strbuf input = STRBUF_INIT;
@@ -515,6 +615,7 @@ static int batch_objects(struct batch_options *opt)
struct expand_data data;
int save_warning;
int retval = 0;
+ const int stdin_cmd = opt->stdin_cmd;
if (!opt->format)
opt->format = "%(objectname) %(objecttype) %(objectsize)";
@@ -590,7 +691,8 @@ 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) {
+ while (!stdin_cmd &&
+ strbuf_getline(&input, stdin) != EOF) {
if (data.split_on_whitespace) {
/*
* Split at first whitespace, tying off the beginning
@@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)
batch_one_object(input.buf, &output, opt, &data);
}
+ if (stdin_cmd)
+ batch_objects_stdin_cmd(opt, &output, &data);
+
strbuf_release(&input);
strbuf_release(&output);
warn_on_object_refname_ambiguity = save_warning;
@@ -636,6 +741,7 @@ static int batch_option_callback(const struct option *opt,
bo->enabled = 1;
bo->print_contents = !strcmp(opt->long_name, "batch");
+ bo->stdin_cmd = !strcmp(opt->long_name, "batch-command");
bo->format = arg;
return 0;
@@ -683,6 +789,10 @@ 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,
batch_option_callback),
+ OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
+ N_("enters batch mode that accepts commands"),
+ PARSE_OPT_NOARG | 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 */
@@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
/* Batch defaults */
if (batch.buffer_output < 0)
batch.buffer_output = batch.all_objects;
+ if (batch.end_null)
+ line_termination = '\0';
/* Return early if we're in batch mode? */
if (batch.enabled) {
diff --git a/strvec.c b/strvec.c
index 61a76ce6cb..7dca04bf7a 100644
--- a/strvec.c
+++ b/strvec.c
@@ -85,6 +85,29 @@ void strvec_split(struct strvec *array, const char *to_split)
}
}
+size_t strvec_split_delim(struct strvec *array, const char *string,
+ int delim, int maxsplit)
+{
+ size_t count = 0;
+ const char *p = string, *end;
+
+ for (;;) {
+ count++;
+ if (maxsplit >= 0 && count > maxsplit) {
+ strvec_push(array, p);
+ return count;
+ }
+ end = strchr(p, delim);
+ if (end) {
+ strvec_push_nodup(array, xmemdupz(p, end - p));
+ p = end + 1;
+ } else {
+ strvec_push(array, p);
+ return count;
+ }
+ }
+}
+
void strvec_clear(struct strvec *array)
{
if (array->v != empty_strvec) {
diff --git a/strvec.h b/strvec.h
index 9f55c8766b..c474918b91 100644
--- a/strvec.h
+++ b/strvec.h
@@ -73,6 +73,14 @@ void strvec_pop(struct strvec *);
/* Splits by whitespace; does not handle quoted arguments! */
void strvec_split(struct strvec *, const char *);
+/**
+ * strvec_split_delim() is a split function that behaves more like its
+ * string_list_split() cousin than the whitespace-splitting
+ * strvec_split().
+ */
+size_t strvec_split_delim(struct strvec *array, const char *string,
+ int delim, int maxsplit);
+
/**
* Free all memory associated with the array and return it to the
* initial, empty state.
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 145eee11df..935ab1cd34 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -964,4 +964,36 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'
test_cmp expect actual
'
+F='%s\0'
+
+test_expect_success 'batch-command unknown command' '
+ echo unknown_command >cmd &&
+ test_expect_code 128 git cat-file --batch-command < cmd 2>err &&
+ grep -E "^fatal:.*unknown command.*" err
+'
+
+test_expect_success 'setup object data' '
+ content="Object Data" &&
+ size=$(strlen "$content") &&
+ sha1=$(echo_without_newline "$content" | git hash-object -w --stdin)
+'
+
+test_expect_success 'batch-command calling object works' '
+ echo "object $sha1" | git cat-file --batch-command >actual &&
+ echo "$sha1 blob $size" >expect &&
+ echo `git cat-file -p "$sha1"` >>expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'batch-command calling info works' '
+ echo "info $sha1" | git cat-file --batch-command >actual &&
+ echo "$sha1 blob $size" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'batch-command fflush works' '
+ printf "fflush\n" > cmd &&
+ test_expect_code 0 git cat-file --batch-command < cmd 2>err
+'
+
test_done
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC v2] cat-file: add a --stdin-cmd mode
2022-01-25 22:50 [RFC v2] cat-file: add a --stdin-cmd mode John Cai
@ 2022-01-25 23:00 ` John Cai
2022-01-27 11:25 ` Phillip Wood
2022-01-28 6:07 ` Eric Wong
2 siblings, 0 replies; 6+ messages in thread
From: John Cai @ 2022-01-25 23:00 UTC (permalink / raw)
To: git; +Cc: avarab, me, phillip.wood123, John Cai
On 25 Jan 2022, at 17:50, John Cai wrote:
> This RFC patch proposes a new flag --stdin-cmd that works with
> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept
> commands and arguments from stdin.
Sorry I forgot to change this summary. s/--stdin-cmd/--batch-command
>
> The start of this idea was discussed in [1], where the original
> motivation was to be able to control when the buffer was flushed to
> stdout in --buffer mode.
>
> However, this can actually be much more useful in situations when
> git-cat-file --batch is being used as a long lived backend query
> process. At GitLab, we use a pair of cat-file processes. One for
> iterating over object metadata with --batch-check, and the other to grab
> object contents with --batch. However, if we had --stdin-cmd, we could
> get rid of the second --batch-check process, and just have one progress
> where we can flip between getting object info, and getting object contents.
> This can lead to huge savings.
>
> git cat-file --batch --stdin-cmd
>
> $ <command> [arg1] [arg2] NL
>
> We can also add a -z mode to allow for NUL-terminated lines
>
> $ <command> [arg1] [arg2] NUL
>
> This patch adds three commands: object, info, fflush
>
> $ object <sha1> NL
> $ info <sha1> NL
> $ fflush NL
>
> These three would be immediately useful in GitLab's context, but one can
> imagine this mode to be further extended for other things.
>
> For instance, a non-trivial part of "cat-file --batch" time is spent
> on parsing its argument and seeing if it's a revision, ref etc. So we
> could add a command that only accepts a full-length 40
> character SHA-1.
>
> This would be the first step in adding such an interface to
> git-cat-file.
>
> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> Changes from v1:
>
> - changed option name to batch-command.
> - changed command function interface to receive the whole line after the command
> name to put the onus of parsing arguments to each individual command function.
> - pass in whole line to batch_one_object in both parse_cmd_object and
> parse_cmd_info to support spaces in the object reference.
> - removed addition of -z to include in a separate patch series
> - added documentation.
> ---
> Documentation/git-cat-file.txt | 15 +++++
> builtin/cat-file.c | 114 ++++++++++++++++++++++++++++++++-
> strvec.c | 23 +++++++
> strvec.h | 8 +++
> t/t1006-cat-file.sh | 32 +++++++++
> 5 files changed, 191 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index bef76f4dd0..8aefa45e4c 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -96,6 +96,21 @@ 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 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.
> +
> +object <object>::
> + Print object contents for object reference <object>
> +
> +info <object>::
> + Print object info for object reference <object>
> +
> +flush::
> + Flush to stdout immediately when used with --buffer
> +
> --batch-all-objects::
> Instead of reading a list of objects on stdin, perform the
> requested batch operation on all objects in the repository and
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 7b3f42950e..30794284d5 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -16,6 +16,7 @@
> #include "packfile.h"
> #include "object-store.h"
> #include "promisor-remote.h"
> +#include "strvec.h"
>
> struct batch_options {
> int enabled;
> @@ -26,7 +27,10 @@ struct batch_options {
> int unordered;
> int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> const char *format;
> + int stdin_cmd;
> + int end_null;
> };
> +static char line_termination = '\n';
>
> static const char *force_path;
>
> @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,
> data);
> }
>
> +enum batch_state {
> + /* Non-transactional state open for commands. */
> + BATCH_STATE_OPEN,
> +};
> +
> +static void parse_cmd_object(struct batch_options *opt,
> + const char *line,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + opt->print_contents = 1;
> + batch_one_object(line, output, opt, data);
> +}
> +
> +static void parse_cmd_info(struct batch_options *opt,
> + const char *line,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + opt->print_contents = 0;
> + batch_one_object(line, output, opt, data);
> +}
> +
> +static void parse_cmd_fflush(struct batch_options *opt,
> + const char *line,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + fflush(stdout);
> +}
> +
> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
> + struct strbuf *, struct expand_data *);
> +
> +static const struct parse_cmd {
> + const char *prefix;
> + parse_cmd_fn_t fn;
> + unsigned args;
> + enum batch_state state;
> +} commands[] = {
> + { "object", parse_cmd_object, 1, BATCH_STATE_OPEN },
> + { "info", parse_cmd_info, 1, BATCH_STATE_OPEN },
> + { "fflush", parse_cmd_fflush, 0, BATCH_STATE_OPEN },
> +};
> +
> +static void batch_objects_stdin_cmd(struct batch_options *opt,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + struct strbuf input = STRBUF_INIT;
> + enum batch_state state = BATCH_STATE_OPEN;
> +
> + /* Read each line dispatch its command */
> + while (!strbuf_getwholeline(&input, stdin, line_termination)) {
> + int i;
> + const struct parse_cmd *cmd = NULL;
> + const char *p;
> +
> + if (*input.buf == line_termination)
> + die("empty command in input");
> + else if (isspace(*input.buf))
> + die("whitespace before command: %s", input.buf);
> +
> + for (i = 0; i < ARRAY_SIZE(commands); i++) {
> + const char *prefix = commands[i].prefix;
> + char c;
> + const char *cmd_name;
> + if (!skip_prefix(input.buf, prefix, &cmd_name))
> + 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].args ? ' ' : line_termination;
> + if (input.buf[strlen(prefix)] != c)
> + continue;
> +
> + cmd = &commands[i];
> + break;
> + }
> + if (!cmd)
> + die("unknown command: %s", input.buf);
> +
> + p = input.buf + strlen(cmd->prefix) + 1;
> + const char *pos = strstr(p, &line_termination);
> +
> + switch (state) {
> + case BATCH_STATE_OPEN:
> + break;
> + }
> + cmd->fn(opt, xstrndup(p, pos-p), output, data);
> + }
> + strbuf_release(&input);
> +}
> +
> static int batch_objects(struct batch_options *opt)
> {
> struct strbuf input = STRBUF_INIT;
> @@ -515,6 +615,7 @@ static int batch_objects(struct batch_options *opt)
> struct expand_data data;
> int save_warning;
> int retval = 0;
> + const int stdin_cmd = opt->stdin_cmd;
>
> if (!opt->format)
> opt->format = "%(objectname) %(objecttype) %(objectsize)";
> @@ -590,7 +691,8 @@ 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) {
> + while (!stdin_cmd &&
> + strbuf_getline(&input, stdin) != EOF) {
> if (data.split_on_whitespace) {
> /*
> * Split at first whitespace, tying off the beginning
> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)
> batch_one_object(input.buf, &output, opt, &data);
> }
>
> + if (stdin_cmd)
> + batch_objects_stdin_cmd(opt, &output, &data);
> +
> strbuf_release(&input);
> strbuf_release(&output);
> warn_on_object_refname_ambiguity = save_warning;
> @@ -636,6 +741,7 @@ static int batch_option_callback(const struct option *opt,
>
> bo->enabled = 1;
> bo->print_contents = !strcmp(opt->long_name, "batch");
> + bo->stdin_cmd = !strcmp(opt->long_name, "batch-command");
> bo->format = arg;
>
> return 0;
> @@ -683,6 +789,10 @@ 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,
> batch_option_callback),
> + OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
> + N_("enters batch mode that accepts commands"),
> + PARSE_OPT_NOARG | 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 */
> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
> /* Batch defaults */
> if (batch.buffer_output < 0)
> batch.buffer_output = batch.all_objects;
> + if (batch.end_null)
> + line_termination = '\0';
>
> /* Return early if we're in batch mode? */
> if (batch.enabled) {
> diff --git a/strvec.c b/strvec.c
> index 61a76ce6cb..7dca04bf7a 100644
> --- a/strvec.c
> +++ b/strvec.c
> @@ -85,6 +85,29 @@ void strvec_split(struct strvec *array, const char *to_split)
> }
> }
>
> +size_t strvec_split_delim(struct strvec *array, const char *string,
> + int delim, int maxsplit)
> +{
> + size_t count = 0;
> + const char *p = string, *end;
> +
> + for (;;) {
> + count++;
> + if (maxsplit >= 0 && count > maxsplit) {
> + strvec_push(array, p);
> + return count;
> + }
> + end = strchr(p, delim);
> + if (end) {
> + strvec_push_nodup(array, xmemdupz(p, end - p));
> + p = end + 1;
> + } else {
> + strvec_push(array, p);
> + return count;
> + }
> + }
> +}
> +
> void strvec_clear(struct strvec *array)
> {
> if (array->v != empty_strvec) {
> diff --git a/strvec.h b/strvec.h
> index 9f55c8766b..c474918b91 100644
> --- a/strvec.h
> +++ b/strvec.h
> @@ -73,6 +73,14 @@ void strvec_pop(struct strvec *);
> /* Splits by whitespace; does not handle quoted arguments! */
> void strvec_split(struct strvec *, const char *);
>
> +/**
> + * strvec_split_delim() is a split function that behaves more like its
> + * string_list_split() cousin than the whitespace-splitting
> + * strvec_split().
> + */
> +size_t strvec_split_delim(struct strvec *array, const char *string,
> + int delim, int maxsplit);
> +
> /**
> * Free all memory associated with the array and return it to the
> * initial, empty state.
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index 145eee11df..935ab1cd34 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -964,4 +964,36 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'
> test_cmp expect actual
> '
>
> +F='%s\0'
> +
> +test_expect_success 'batch-command unknown command' '
> + echo unknown_command >cmd &&
> + test_expect_code 128 git cat-file --batch-command < cmd 2>err &&
> + grep -E "^fatal:.*unknown command.*" err
> +'
> +
> +test_expect_success 'setup object data' '
> + content="Object Data" &&
> + size=$(strlen "$content") &&
> + sha1=$(echo_without_newline "$content" | git hash-object -w --stdin)
> +'
> +
> +test_expect_success 'batch-command calling object works' '
> + echo "object $sha1" | git cat-file --batch-command >actual &&
> + echo "$sha1 blob $size" >expect &&
> + echo `git cat-file -p "$sha1"` >>expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'batch-command calling info works' '
> + echo "info $sha1" | git cat-file --batch-command >actual &&
> + echo "$sha1 blob $size" >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'batch-command fflush works' '
> + printf "fflush\n" > cmd &&
> + test_expect_code 0 git cat-file --batch-command < cmd 2>err
> +'
> +
> test_done
> --
> 2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] cat-file: add a --stdin-cmd mode
2022-01-25 22:50 [RFC v2] cat-file: add a --stdin-cmd mode John Cai
2022-01-25 23:00 ` John Cai
@ 2022-01-27 11:25 ` Phillip Wood
2022-01-27 21:04 ` John Cai
2022-01-28 6:07 ` Eric Wong
2 siblings, 1 reply; 6+ messages in thread
From: Phillip Wood @ 2022-01-27 11:25 UTC (permalink / raw)
To: John Cai, git; +Cc: avarab, me
Hi John
On 25/01/2022 22:50, John Cai wrote:
> This RFC patch proposes a new flag --stdin-cmd that works with
> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept
> commands and arguments from stdin.
>
> The start of this idea was discussed in [1], where the original
> motivation was to be able to control when the buffer was flushed to
> stdout in --buffer mode.
>
> However, this can actually be much more useful in situations when
> git-cat-file --batch is being used as a long lived backend query
> process. At GitLab, we use a pair of cat-file processes. One for
> iterating over object metadata with --batch-check, and the other to grab
> object contents with --batch. However, if we had --stdin-cmd, we could
> get rid of the second --batch-check process, and just have one progress
> where we can flip between getting object info, and getting object contents.
> This can lead to huge savings.
>
> git cat-file --batch --stdin-cmd
>
> $ <command> [arg1] [arg2] NL
>
> We can also add a -z mode to allow for NUL-terminated lines
>
> $ <command> [arg1] [arg2] NUL
>
> This patch adds three commands: object, info, fflush
>
> $ object <sha1> NL
> $ info <sha1> NL
> $ fflush NL
>
> These three would be immediately useful in GitLab's context, but one can
> imagine this mode to be further extended for other things.
>
> For instance, a non-trivial part of "cat-file --batch" time is spent
> on parsing its argument and seeing if it's a revision, ref etc. So we
> could add a command that only accepts a full-length 40
> character SHA-1.
>
> This would be the first step in adding such an interface to
> git-cat-file.
>
> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> Changes from v1:
>
> - changed option name to batch-command.
> - changed command function interface to receive the whole line after the command
> name to put the onus of parsing arguments to each individual command function.
> - pass in whole line to batch_one_object in both parse_cmd_object and
> parse_cmd_info to support spaces in the object reference.
> - removed addition of -z to include in a separate patch series
> - added documentation.
I've left some comments below, they're mostly small details, I like the
new option name and the changes you've made to the command parsing.
> ---
> Documentation/git-cat-file.txt | 15 +++++
> builtin/cat-file.c | 114 ++++++++++++++++++++++++++++++++-
> strvec.c | 23 +++++++
> strvec.h | 8 +++
> t/t1006-cat-file.sh | 32 +++++++++
> 5 files changed, 191 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index bef76f4dd0..8aefa45e4c 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -96,6 +96,21 @@ OPTIONS
> need to specify the path, separated by whitespace. See the
> section `BATCH OUTPUT` below for details.
>
> +-batch-command::
is this missing a '-'?
> + Enter a command mode that reads 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.
> +
> +object <object>::
> + Print object contents for object reference <object>
> +
> +info <object>::
> + Print object info for object reference <object>
> +
> +flush::
> + Flush to stdout immediately when used with --buffer
> +
> --batch-all-objects::
> Instead of reading a list of objects on stdin, perform the
> requested batch operation on all objects in the repository and
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 7b3f42950e..30794284d5 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -16,6 +16,7 @@
> #include "packfile.h"
> #include "object-store.h"
> #include "promisor-remote.h"
> +#include "strvec.h"
>
> struct batch_options {
> int enabled;
> @@ -26,7 +27,10 @@ struct batch_options {
> int unordered;
> int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> const char *format;
> + int stdin_cmd;
Now that the option has been renamed it would be nice to rename the
corresponding variable to match
> + int end_null;
If you're not adding '-z' here then you don't need this or the addition
below.
> };
> +static char line_termination = '\n';
>
> static const char *force_path;
>
> @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,
> data);
> }
>
> +enum batch_state {
> + /* Non-transactional state open for commands. */
> + BATCH_STATE_OPEN,
> +};
I forgot to ask what the idea behind the batch state is last time -
what's it for?
> +static void parse_cmd_object(struct batch_options *opt,
> + const char *line,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + opt->print_contents = 1;
> + batch_one_object(line, output, opt, data);
> +}
> +
> +static void parse_cmd_info(struct batch_options *opt,
> + const char *line,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + opt->print_contents = 0;
> + batch_one_object(line, output, opt, data);
> +}
> +
> +static void parse_cmd_fflush(struct batch_options *opt,
> + const char *line,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + fflush(stdout);
> +}
> +
> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
> + struct strbuf *, struct expand_data *);
> +
> +static const struct parse_cmd {
> + const char *prefix;
> + parse_cmd_fn_t fn;
> + unsigned args;
This is now a flag so maybe 'takes_args' would better describe its purpose.
> + enum batch_state state;
> +} commands[] = {
> + { "object", parse_cmd_object, 1, BATCH_STATE_OPEN },
> + { "info", parse_cmd_info, 1, BATCH_STATE_OPEN },
> + { "fflush", parse_cmd_fflush, 0, BATCH_STATE_OPEN },
> +};
> +
> +static void batch_objects_stdin_cmd(struct batch_options *opt,
> + struct strbuf *output,
> + struct expand_data *data)
> +{
> + struct strbuf input = STRBUF_INIT;
> + enum batch_state state = BATCH_STATE_OPEN;
> +
> + /* Read each line dispatch its command */
> + while (!strbuf_getwholeline(&input, stdin, line_termination)) {
> + int i;
> + const struct parse_cmd *cmd = NULL;
> + const char *p;
> +
> + if (*input.buf == line_termination)
> + die("empty command in input");
> + else if (isspace(*input.buf))
> + die("whitespace before command: %s", input.buf);
> +
> + for (i = 0; i < ARRAY_SIZE(commands); i++) {
> + const char *prefix = commands[i].prefix;
> + char c;
> + const char *cmd_name;
skip_prefix() sets this to the end of the name so maybe 'cmd_end' would
be clearer?
> + if (!skip_prefix(input.buf, prefix, &cmd_name))
> + 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].args ? ' ' : line_termination;
> + if (input.buf[strlen(prefix)] != c)
Now that you're using skip_prefix() you can write this as
if (*cmd_end != c)
> + continue;
> +
> + cmd = &commands[i];
> + break;
> + }
> + if (!cmd)
> + die("unknown command: %s", input.buf);
> +
> + p = input.buf + strlen(cmd->prefix) + 1;
This can be simplified to
p = cmd_end + 1;
> + const char *pos = strstr(p, &line_termination);
This isn't needed without '-z'. If it were required then using
strchrnul() would prevent a NULL pointer dereference when the last input
line does not end with a terminator. I think we typically call a pointer
to the end of the line 'eol' or 'end'. Also variables should be declared
at the top of the function.
> + switch (state) {
> + case BATCH_STATE_OPEN:
> + break;
> + }
> + cmd->fn(opt, xstrndup(p, pos-p), output, data);
Is there a reason this is passing a copy of the string?
> + }
> + strbuf_release(&input);
> +}
> +
> static int batch_objects(struct batch_options *opt)
> {
> struct strbuf input = STRBUF_INIT;
> @@ -515,6 +615,7 @@ static int batch_objects(struct batch_options *opt)
> struct expand_data data;
> int save_warning;
> int retval = 0;
> + const int stdin_cmd = opt->stdin_cmd;
>
> if (!opt->format)
> opt->format = "%(objectname) %(objecttype) %(objectsize)";
> @@ -590,7 +691,8 @@ 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) {
> + while (!stdin_cmd &&
If you moved the 'if (stdin_cmd)' block above this block we could loose
this change. I'm not sure if that is possible without looking at the
whole function though.
> + strbuf_getline(&input, stdin) != EOF) {
> if (data.split_on_whitespace) {
> /*
> * Split at first whitespace, tying off the beginning
> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)
> batch_one_object(input.buf, &output, opt, &data);
> }
>
> + if (stdin_cmd)
> + batch_objects_stdin_cmd(opt, &output, &data);
> +
> strbuf_release(&input);
> strbuf_release(&output);
> warn_on_object_refname_ambiguity = save_warning;
> @@ -636,6 +741,7 @@ static int batch_option_callback(const struct option *opt,
>
> bo->enabled = 1;
> bo->print_contents = !strcmp(opt->long_name, "batch");
> + bo->stdin_cmd = !strcmp(opt->long_name, "batch-command");
> bo->format = arg;
>
> return 0;
> @@ -683,6 +789,10 @@ 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,
> batch_option_callback),
> + OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
> + N_("enters batch mode that accepts commands"),
> + PARSE_OPT_NOARG | 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 */
> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
> /* Batch defaults */
> if (batch.buffer_output < 0)
> batch.buffer_output = batch.all_objects;
> + if (batch.end_null)
> + line_termination = '\0';
>
> /* Return early if we're in batch mode? */
> if (batch.enabled) {
> diff --git a/strvec.c b/strvec.c
> index 61a76ce6cb..7dca04bf7a 100644
> --- a/strvec.c
> +++ b/strvec.c
>[...]
We don't need any strvec changes now that we don't split the input lines
to --bactch-command
> +F='%s\0'
This isn't used now
> +test_expect_success 'batch-command unknown command' '
> + echo unknown_command >cmd &&
> + test_expect_code 128 git cat-file --batch-command < cmd 2>err &&
> + grep -E "^fatal:.*unknown command.*" err
> +'
> +
> +test_expect_success 'setup object data' '
> + content="Object Data" &&
> + size=$(strlen "$content") &&
> + sha1=$(echo_without_newline "$content" | git hash-object -w --stdin)
> +'
> +
> +test_expect_success 'batch-command calling object works' '
> + echo "object $sha1" | git cat-file --batch-command >actual &&
> + echo "$sha1 blob $size" >expect &&
> + echo `git cat-file -p "$sha1"` >>expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'batch-command calling info works' '
> + echo "info $sha1" | git cat-file --batch-command >actual &&
> + echo "$sha1 blob $size" >expect &&
> + test_cmp expect actual
> +'
I had a quick look at this test file and there is a loop at the top that
runs some --batch tests on various inputs, I wonder if these two tests
could go in there.
> +test_expect_success 'batch-command fflush works' '
> + printf "fflush\n" > cmd &&
> + test_expect_code 0 git cat-file --batch-command < cmd 2>err
> +'
It'd be nice to check this actually flushes the output.
Best Wishes
Phillip
> test_done
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] cat-file: add a --stdin-cmd mode
2022-01-27 11:25 ` Phillip Wood
@ 2022-01-27 21:04 ` John Cai
2022-01-28 4:16 ` John Cai
0 siblings, 1 reply; 6+ messages in thread
From: John Cai @ 2022-01-27 21:04 UTC (permalink / raw)
To: phillip.wood; +Cc: git, avarab, me
Hey Phillip,
First of all thank you for the thorough review—it really helps someone who’s learning how to contribute!
On 27 Jan 2022, at 6:25, Phillip Wood wrote:
> Hi John
>
> On 25/01/2022 22:50, John Cai wrote:
>> This RFC patch proposes a new flag --stdin-cmd that works with
>> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept
>> commands and arguments from stdin.
>>
>> The start of this idea was discussed in [1], where the original
>> motivation was to be able to control when the buffer was flushed to
>> stdout in --buffer mode.
>>
>> However, this can actually be much more useful in situations when
>> git-cat-file --batch is being used as a long lived backend query
>> process. At GitLab, we use a pair of cat-file processes. One for
>> iterating over object metadata with --batch-check, and the other to grab
>> object contents with --batch. However, if we had --stdin-cmd, we could
>> get rid of the second --batch-check process, and just have one progress
>> where we can flip between getting object info, and getting object contents.
>> This can lead to huge savings.
>>
>> git cat-file --batch --stdin-cmd
>>
>> $ <command> [arg1] [arg2] NL
>>
>> We can also add a -z mode to allow for NUL-terminated lines
>>
>> $ <command> [arg1] [arg2] NUL
>>
>> This patch adds three commands: object, info, fflush
>>
>> $ object <sha1> NL
>> $ info <sha1> NL
>> $ fflush NL
>>
>> These three would be immediately useful in GitLab's context, but one can
>> imagine this mode to be further extended for other things.
>>
>> For instance, a non-trivial part of "cat-file --batch" time is spent
>> on parsing its argument and seeing if it's a revision, ref etc. So we
>> could add a command that only accepts a full-length 40
>> character SHA-1.
>>
>> This would be the first step in adding such an interface to
>> git-cat-file.
>>
>> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/
>>
>> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>> Changes from v1:
>>
>> - changed option name to batch-command.
>> - changed command function interface to receive the whole line after the command
>> name to put the onus of parsing arguments to each individual command function.
>> - pass in whole line to batch_one_object in both parse_cmd_object and
>> parse_cmd_info to support spaces in the object reference.
>> - removed addition of -z to include in a separate patch series
>> - added documentation.
>
> I've left some comments below, they're mostly small details, I like the new option name and the changes you've made to the command parsing.
>
>> ---
>> Documentation/git-cat-file.txt | 15 +++++
>> builtin/cat-file.c | 114 ++++++++++++++++++++++++++++++++-
>> strvec.c | 23 +++++++
>> strvec.h | 8 +++
>> t/t1006-cat-file.sh | 32 +++++++++
>> 5 files changed, 191 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
>> index bef76f4dd0..8aefa45e4c 100644
>> --- a/Documentation/git-cat-file.txt
>> +++ b/Documentation/git-cat-file.txt
>> @@ -96,6 +96,21 @@ OPTIONS
>> need to specify the path, separated by whitespace. See the
>> section `BATCH OUTPUT` below for details.
>> +-batch-command::
>
> is this missing a '-'?
>
>> + Enter a command mode that reads 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.
>> +
>> +object <object>::
>> + Print object contents for object reference <object>
>> +
>> +info <object>::
>> + Print object info for object reference <object>
>> +
>> +flush::
>> + Flush to stdout immediately when used with --buffer
>> +
>> --batch-all-objects::
>> Instead of reading a list of objects on stdin, perform the
>> requested batch operation on all objects in the repository and
>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>> index 7b3f42950e..30794284d5 100644
>> --- a/builtin/cat-file.c
>> +++ b/builtin/cat-file.c
>> @@ -16,6 +16,7 @@
>> #include "packfile.h"
>> #include "object-store.h"
>> #include "promisor-remote.h"
>> +#include "strvec.h"
>> struct batch_options {
>> int enabled;
>> @@ -26,7 +27,10 @@ struct batch_options {
>> int unordered;
>> int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
>> const char *format;
>> + int stdin_cmd;
>
> Now that the option has been renamed it would be nice to rename the corresponding variable to match
I was trying to find a good name. There is already a cmdmode variable. I’m thinking stdin_cmd is not such a
bad name since we are receiving commands from stdin. suggestions welcome!
>
>> + int end_null;
>
> If you're not adding '-z' here then you don't need this or the addition below.
>
>> };
>> +static char line_termination = '\n';
>> static const char *force_path;
>> @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,
>> data);
>> }
>> +enum batch_state {
>> + /* Non-transactional state open for commands. */
>> + BATCH_STATE_OPEN,
>> +};
>
> I forgot to ask what the idea behind the batch state is last time - what's it for?
This is to support transactional semantics for commands we want to support in the future, but
since this is already a biggish change, we can leave this out of this series.
>
>> +static void parse_cmd_object(struct batch_options *opt,
>> + const char *line,
>> + struct strbuf *output,
>> + struct expand_data *data)
>> +{
>> + opt->print_contents = 1;
>> + batch_one_object(line, output, opt, data);
>> +}
>> +
>> +static void parse_cmd_info(struct batch_options *opt,
>> + const char *line,
>> + struct strbuf *output,
>> + struct expand_data *data)
>> +{
>> + opt->print_contents = 0;
>> + batch_one_object(line, output, opt, data);
>> +}
>> +
>> +static void parse_cmd_fflush(struct batch_options *opt,
>> + const char *line,
>> + struct strbuf *output,
>> + struct expand_data *data)
>> +{
>> + fflush(stdout);
>> +}
>> +
>> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
>> + struct strbuf *, struct expand_data *);
>> +
>> +static const struct parse_cmd {
>> + const char *prefix;
>> + parse_cmd_fn_t fn;
>> + unsigned args;
>
> This is now a flag so maybe 'takes_args' would better describe its purpose.
>
>> + enum batch_state state;
>> +} commands[] = {
>> + { "object", parse_cmd_object, 1, BATCH_STATE_OPEN },
>> + { "info", parse_cmd_info, 1, BATCH_STATE_OPEN },
>> + { "fflush", parse_cmd_fflush, 0, BATCH_STATE_OPEN },
>> +};
>> +
>> +static void batch_objects_stdin_cmd(struct batch_options *opt,
>> + struct strbuf *output,
>> + struct expand_data *data)
>> +{
>> + struct strbuf input = STRBUF_INIT;
>> + enum batch_state state = BATCH_STATE_OPEN;
>> +
>> + /* Read each line dispatch its command */
>> + while (!strbuf_getwholeline(&input, stdin, line_termination)) {
>> + int i;
>> + const struct parse_cmd *cmd = NULL;
>> + const char *p;
>> +
>> + if (*input.buf == line_termination)
>> + die("empty command in input");
>> + else if (isspace(*input.buf))
>> + die("whitespace before command: %s", input.buf);
>> +
>> + for (i = 0; i < ARRAY_SIZE(commands); i++) {
>> + const char *prefix = commands[i].prefix;
>> + char c;
>> + const char *cmd_name;
>
> skip_prefix() sets this to the end of the name so maybe 'cmd_end' would be clearer?
>
>> + if (!skip_prefix(input.buf, prefix, &cmd_name))
>> + 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].args ? ' ' : line_termination;
>> + if (input.buf[strlen(prefix)] != c)
>
> Now that you're using skip_prefix() you can write this as
> if (*cmd_end != c)
>
>> + continue;
>> +
>> + cmd = &commands[i];
>> + break;
>> + }
>> + if (!cmd)
>> + die("unknown command: %s", input.buf);
>> +
>> + p = input.buf + strlen(cmd->prefix) + 1;
>
> This can be simplified to
> p = cmd_end + 1;
>
>> + const char *pos = strstr(p, &line_termination);
>
> This isn't needed without '-z'. If it were required then using strchrnul() would prevent a NULL pointer dereference when the last input line does not end with a terminator. I think we typically call a pointer to the end of the line 'eol' or 'end'. Also variables should be declared at the top of the function.
>
>> + switch (state) {
>> + case BATCH_STATE_OPEN:
>> + break;
>> + }
>> + cmd->fn(opt, xstrndup(p, pos-p), output, data);
>
> Is there a reason this is passing a copy of the string?
>
>> + }
>> + strbuf_release(&input);
>> +}
>> +
>> static int batch_objects(struct batch_options *opt)
>> {
>> struct strbuf input = STRBUF_INIT;
>> @@ -515,6 +615,7 @@ static int batch_objects(struct batch_options *opt)
>> struct expand_data data;
>> int save_warning;
>> int retval = 0;
>> + const int stdin_cmd = opt->stdin_cmd;
>> if (!opt->format)
>> opt->format = "%(objectname) %(objecttype) %(objectsize)";
>> @@ -590,7 +691,8 @@ 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) {
>> + while (!stdin_cmd &&
>
> If you moved the 'if (stdin_cmd)' block above this block we could loose this change. I'm not sure if that is possible without looking at the whole function though.
>
>> + strbuf_getline(&input, stdin) != EOF) {
>> if (data.split_on_whitespace) {
>> /*
>> * Split at first whitespace, tying off the beginning
>> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)
>> batch_one_object(input.buf, &output, opt, &data);
>> }
>> + if (stdin_cmd)
>> + batch_objects_stdin_cmd(opt, &output, &data);
>> +
>> strbuf_release(&input);
>> strbuf_release(&output);
>> warn_on_object_refname_ambiguity = save_warning;
>> @@ -636,6 +741,7 @@ static int batch_option_callback(const struct option *opt,
>> bo->enabled = 1;
>> bo->print_contents = !strcmp(opt->long_name, "batch");
>> + bo->stdin_cmd = !strcmp(opt->long_name, "batch-command");
>> bo->format = arg;
>> return 0;
>> @@ -683,6 +789,10 @@ 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,
>> batch_option_callback),
>> + OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
>> + N_("enters batch mode that accepts commands"),
>> + PARSE_OPT_NOARG | 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 */
>> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>> /* Batch defaults */
>> if (batch.buffer_output < 0)
>> batch.buffer_output = batch.all_objects;
>> + if (batch.end_null)
>> + line_termination = '\0';
>> /* Return early if we're in batch mode? */
>> if (batch.enabled) {
>> diff --git a/strvec.c b/strvec.c
>> index 61a76ce6cb..7dca04bf7a 100644
>> --- a/strvec.c
>> +++ b/strvec.c
>> [...]
>
> We don't need any strvec changes now that we don't split the input lines to --bactch-command
>
>> +F='%s\0'
>
> This isn't used now
>
>> +test_expect_success 'batch-command unknown command' '
>> + echo unknown_command >cmd &&
>> + test_expect_code 128 git cat-file --batch-command < cmd 2>err &&
>> + grep -E "^fatal:.*unknown command.*" err
>> +'
>> +
>> +test_expect_success 'setup object data' '
>> + content="Object Data" &&
>> + size=$(strlen "$content") &&
>> + sha1=$(echo_without_newline "$content" | git hash-object -w --stdin)
>> +'
>> +
>> +test_expect_success 'batch-command calling object works' '
>> + echo "object $sha1" | git cat-file --batch-command >actual &&
>> + echo "$sha1 blob $size" >expect &&
>> + echo `git cat-file -p "$sha1"` >>expect &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'batch-command calling info works' '
>> + echo "info $sha1" | git cat-file --batch-command >actual &&
>> + echo "$sha1 blob $size" >expect &&
>> + test_cmp expect actual
>> +'
>
> I had a quick look at this test file and there is a loop at the top that runs some --batch tests on various inputs, I wonder if these two tests could go in there.
>
>> +test_expect_success 'batch-command fflush works' '
>> + printf "fflush\n" > cmd &&
>> + test_expect_code 0 git cat-file --batch-command < cmd 2>err
>> +'
>
> It'd be nice to check this actually flushes the output.
could you give me some ideas on how to do this?
>
> Best Wishes
>
> Phillip
>
>> test_done
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] cat-file: add a --stdin-cmd mode
2022-01-27 21:04 ` John Cai
@ 2022-01-28 4:16 ` John Cai
0 siblings, 0 replies; 6+ messages in thread
From: John Cai @ 2022-01-28 4:16 UTC (permalink / raw)
To: phillip.wood; +Cc: git, avarab, me
Resending this as my last message accidentally included some html and got rejected by the listserv
On 27 Jan 2022, at 16:04, John Cai wrote:
> Hey Phillip,
>
> First of all thank you for the thorough review—it really helps someone who’s learning how to contribute!
>
> On 27 Jan 2022, at 6:25, Phillip Wood wrote:
>
>> Hi John
>>
>> On 25/01/2022 22:50, John Cai wrote:
>>> This RFC patch proposes a new flag --stdin-cmd that works with
>>> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept
>>> commands and arguments from stdin.
>>>
>>> The start of this idea was discussed in [1], where the original
>>> motivation was to be able to control when the buffer was flushed to
>>> stdout in --buffer mode.
>>>
>>> However, this can actually be much more useful in situations when
>>> git-cat-file --batch is being used as a long lived backend query
>>> process. At GitLab, we use a pair of cat-file processes. One for
>>> iterating over object metadata with --batch-check, and the other to grab
>>> object contents with --batch. However, if we had --stdin-cmd, we could
>>> get rid of the second --batch-check process, and just have one progress
>>> where we can flip between getting object info, and getting object contents.
>>> This can lead to huge savings.
>>>
>>> git cat-file --batch --stdin-cmd
>>>
>>> $ <command> [arg1] [arg2] NL
>>>
>>> We can also add a -z mode to allow for NUL-terminated lines
>>>
>>> $ <command> [arg1] [arg2] NUL
>>>
>>> This patch adds three commands: object, info, fflush
>>>
>>> $ object <sha1> NL
>>> $ info <sha1> NL
>>> $ fflush NL
>>>
>>> These three would be immediately useful in GitLab's context, but one can
>>> imagine this mode to be further extended for other things.
>>>
>>> For instance, a non-trivial part of "cat-file --batch" time is spent
>>> on parsing its argument and seeing if it's a revision, ref etc. So we
>>> could add a command that only accepts a full-length 40
>>> character SHA-1.
>>>
>>> This would be the first step in adding such an interface to
>>> git-cat-file.
>>>
>>> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/
>>>
>>> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> Signed-off-by: John Cai <johncai86@gmail.com>
>>> ---
>>> Changes from v1:
>>>
>>> - changed option name to batch-command.
>>> - changed command function interface to receive the whole line after the command
>>> name to put the onus of parsing arguments to each individual command function.
>>> - pass in whole line to batch_one_object in both parse_cmd_object and
>>> parse_cmd_info to support spaces in the object reference.
>>> - removed addition of -z to include in a separate patch series
>>> - added documentation.
>>
>> I've left some comments below, they're mostly small details, I like the new option name and the changes you've made to the command parsing.
>>
>>> ---
>>> Documentation/git-cat-file.txt | 15 +++++
>>> builtin/cat-file.c | 114 ++++++++++++++++++++++++++++++++-
>>> strvec.c | 23 +++++++
>>> strvec.h | 8 +++
>>> t/t1006-cat-file.sh | 32 +++++++++
>>> 5 files changed, 191 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
>>> index bef76f4dd0..8aefa45e4c 100644
>>> --- a/Documentation/git-cat-file.txt
>>> +++ b/Documentation/git-cat-file.txt
>>> @@ -96,6 +96,21 @@ OPTIONS
>>> need to specify the path, separated by whitespace. See the
>>> section `BATCH OUTPUT` below for details.
>>> +-batch-command::
>>
>> is this missing a '-'?
>>
>>> + Enter a command mode that reads 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.
>>> +
>>> +object <object>::
>>> + Print object contents for object reference <object>
>>> +
>>> +info <object>::
>>> + Print object info for object reference <object>
>>> +
>>> +flush::
>>> + Flush to stdout immediately when used with --buffer
>>> +
>>> --batch-all-objects::
>>> Instead of reading a list of objects on stdin, perform the
>>> requested batch operation on all objects in the repository and
>>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>>> index 7b3f42950e..30794284d5 100644
>>> --- a/builtin/cat-file.c
>>> +++ b/builtin/cat-file.c
>>> @@ -16,6 +16,7 @@
>>> #include "packfile.h"
>>> #include "object-store.h"
>>> #include "promisor-remote.h"
>>> +#include "strvec.h"
>>> struct batch_options {
>>> int enabled;
>>> @@ -26,7 +27,10 @@ struct batch_options {
>>> int unordered;
>>> int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
>>> const char *format;
>>> + int stdin_cmd;
>>
>> Now that the option has been renamed it would be nice to rename the corresponding variable to match
>
> I was trying to find a good name. There is already a cmdmode variable. I’m thinking stdin_cmd is not such a
> bad name since we are receiving commands from stdin. suggestions welcome!
>
>>
>>> + int end_null;
>>
>> If you're not adding '-z' here then you don't need this or the addition below.
>>
>>> };
>>> +static char line_termination = '\n';
>>> static const char *force_path;
>>> @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,
>>> data);
>>> }
>>> +enum batch_state {
>>> + /* Non-transactional state open for commands. */
>>> + BATCH_STATE_OPEN,
>>> +};
>>
>> I forgot to ask what the idea behind the batch state is last time - what's it for?
>
> This is to support transactional semantics for commands we want to support in the future, but
> since this is already a biggish change, we can leave this out of this series.
>
>>
>>> +static void parse_cmd_object(struct batch_options *opt,
>>> + const char *line,
>>> + struct strbuf *output,
>>> + struct expand_data *data)
>>> +{
>>> + opt->print_contents = 1;
>>> + batch_one_object(line, output, opt, data);
>>> +}
>>> +
>>> +static void parse_cmd_info(struct batch_options *opt,
>>> + const char *line,
>>> + struct strbuf *output,
>>> + struct expand_data *data)
>>> +{
>>> + opt->print_contents = 0;
>>> + batch_one_object(line, output, opt, data);
>>> +}
>>> +
>>> +static void parse_cmd_fflush(struct batch_options *opt,
>>> + const char *line,
>>> + struct strbuf *output,
>>> + struct expand_data *data)
>>> +{
>>> + fflush(stdout);
>>> +}
>>> +
>>> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
>>> + struct strbuf *, struct expand_data *);
>>> +
>>> +static const struct parse_cmd {
>>> + const char *prefix;
>>> + parse_cmd_fn_t fn;
>>> + unsigned args;
>>
>> This is now a flag so maybe 'takes_args' would better describe its purpose.
>>
>>> + enum batch_state state;
>>> +} commands[] = {
>>> + { "object", parse_cmd_object, 1, BATCH_STATE_OPEN },
>>> + { "info", parse_cmd_info, 1, BATCH_STATE_OPEN },
>>> + { "fflush", parse_cmd_fflush, 0, BATCH_STATE_OPEN },
>>> +};
>>> +
>>> +static void batch_objects_stdin_cmd(struct batch_options *opt,
>>> + struct strbuf *output,
>>> + struct expand_data *data)
>>> +{
>>> + struct strbuf input = STRBUF_INIT;
>>> + enum batch_state state = BATCH_STATE_OPEN;
>>> +
>>> + /* Read each line dispatch its command */
>>> + while (!strbuf_getwholeline(&input, stdin, line_termination)) {
>>> + int i;
>>> + const struct parse_cmd *cmd = NULL;
>>> + const char *p;
>>> +
>>> + if (*input.buf == line_termination)
>>> + die("empty command in input");
>>> + else if (isspace(*input.buf))
>>> + die("whitespace before command: %s", input.buf);
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(commands); i++) {
>>> + const char *prefix = commands[i].prefix;
>>> + char c;
>>> + const char *cmd_name;
>>
>> skip_prefix() sets this to the end of the name so maybe 'cmd_end' would be clearer?
>>
>>> + if (!skip_prefix(input.buf, prefix, &cmd_name))
>>> + 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].args ? ' ' : line_termination;
>>> + if (input.buf[strlen(prefix)] != c)
>>
>> Now that you're using skip_prefix() you can write this as
>> if (*cmd_end != c)
>>
>>> + continue;
>>> +
>>> + cmd = &commands[i];
>>> + break;
>>> + }
>>> + if (!cmd)
>>> + die("unknown command: %s", input.buf);
>>> +
>>> + p = input.buf + strlen(cmd->prefix) + 1;
>>
>> This can be simplified to
>> p = cmd_end + 1;
>>
>>> + const char *pos = strstr(p, &line_termination);
>>
>> This isn't needed without '-z'. If it were required then using strchrnul() would prevent a NULL pointer dereference when the last input line does not end with a terminator. I think we typically call a pointer to the end of the line 'eol' or 'end'. Also variables should be declared at the top of the function.
>>
>>> + switch (state) {
>>> + case BATCH_STATE_OPEN:
>>> + break;
>>> + }
>>> + cmd->fn(opt, xstrndup(p, pos-p), output, data);
>>
>> Is there a reason this is passing a copy of the string?
>>
>>> + }
>>> + strbuf_release(&input);
>>> +}
>>> +
>>> static int batch_objects(struct batch_options *opt)
>>> {
>>> struct strbuf input = STRBUF_INIT;
>>> @@ -515,6 +615,7 @@ static int batch_objects(struct batch_options *opt)
>>> struct expand_data data;
>>> int save_warning;
>>> int retval = 0;
>>> + const int stdin_cmd = opt->stdin_cmd;
>>> if (!opt->format)
>>> opt->format = "%(objectname) %(objecttype) %(objectsize)";
>>> @@ -590,7 +691,8 @@ 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) {
>>> + while (!stdin_cmd &&
>>
>> If you moved the 'if (stdin_cmd)' block above this block we could loose this change. I'm not sure if that is possible without looking at the whole function though.
>>
>>> + strbuf_getline(&input, stdin) != EOF) {
>>> if (data.split_on_whitespace) {
>>> /*
>>> * Split at first whitespace, tying off the beginning
>>> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)
>>> batch_one_object(input.buf, &output, opt, &data);
>>> }
>>> + if (stdin_cmd)
>>> + batch_objects_stdin_cmd(opt, &output, &data);
>>> +
>>> strbuf_release(&input);
>>> strbuf_release(&output);
>>> warn_on_object_refname_ambiguity = save_warning;
>>> @@ -636,6 +741,7 @@ static int batch_option_callback(const struct option *opt,
>>> bo->enabled = 1;
>>> bo->print_contents = !strcmp(opt->long_name, "batch");
>>> + bo->stdin_cmd = !strcmp(opt->long_name, "batch-command");
>>> bo->format = arg;
>>> return 0;
>>> @@ -683,6 +789,10 @@ 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,
>>> batch_option_callback),
>>> + OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
>>> + N_("enters batch mode that accepts commands"),
>>> + PARSE_OPT_NOARG | 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 */
>>> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>>> /* Batch defaults */
>>> if (batch.buffer_output < 0)
>>> batch.buffer_output = batch.all_objects;
>>> + if (batch.end_null)
>>> + line_termination = '\0';
>>> /* Return early if we're in batch mode? */
>>> if (batch.enabled) {
>>> diff --git a/strvec.c b/strvec.c
>>> index 61a76ce6cb..7dca04bf7a 100644
>>> --- a/strvec.c
>>> +++ b/strvec.c
>>> [...]
>>
>> We don't need any strvec changes now that we don't split the input lines to --bactch-command
>>
>>> +F='%s\0'
>>
>> This isn't used now
>>
>>> +test_expect_success 'batch-command unknown command' '
>>> + echo unknown_command >cmd &&
>>> + test_expect_code 128 git cat-file --batch-command < cmd 2>err &&
>>> + grep -E "^fatal:.*unknown command.*" err
>>> +'
>>> +
>>> +test_expect_success 'setup object data' '
>>> + content="Object Data" &&
>>> + size=$(strlen "$content") &&
>>> + sha1=$(echo_without_newline "$content" | git hash-object -w --stdin)
>>> +'
>>> +
>>> +test_expect_success 'batch-command calling object works' '
>>> + echo "object $sha1" | git cat-file --batch-command >actual &&
>>> + echo "$sha1 blob $size" >expect &&
>>> + echo `git cat-file -p "$sha1"` >>expect &&
>>> + test_cmp expect actual
>>> +'
>>> +
>>> +test_expect_success 'batch-command calling info works' '
>>> + echo "info $sha1" | git cat-file --batch-command >actual &&
>>> + echo "$sha1 blob $size" >expect &&
>>> + test_cmp expect actual
>>> +'
>>
>> I had a quick look at this test file and there is a loop at the top that runs some --batch tests on various inputs, I wonder if these two tests could go in there.
>>
>>> +test_expect_success 'batch-command fflush works' '
>>> + printf "fflush\n" > cmd &&
>>> + test_expect_code 0 git cat-file --batch-command < cmd 2>err
>>> +'
>>
>> It'd be nice to check this actually flushes the output.
>
> could you give me some ideas on how to do this?
Ok, after filling around I think it can be something like this (copied over from an example in t9300-fast-import.sh
run_buffer_test () {
type=$1
sha1=$2
size=$3
mkfifo V.input
exec 8<>V.input
rm V.input
mkfifo V.output
exec 9<>V.output
rm V.output
(
git cat-file --buffer --batch-command <&8 >&9 &
echo $! >&9 &&
wait $!
) &
sh_pid=$!
read fi_pid <&9
test_when_finished "
exec 8>&-; exec 9>&-;
kill $sh_pid && wait $sh_pid
kill $fi_pid && wait $fi_pid
true"
expect=$(echo "$sha1 $type $size")
echo "info $sha1" >&8
echo "fflush" >&8
read actual <&9
test "$actual" = "$expect"
}
Not sure if there's a simpler way since we need to simulate writing to and reading from the process.
Thanks
>
>>
>> Best Wishes
>>
>> Phillip
>>
>>> test_done
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] cat-file: add a --stdin-cmd mode
2022-01-25 22:50 [RFC v2] cat-file: add a --stdin-cmd mode John Cai
2022-01-25 23:00 ` John Cai
2022-01-27 11:25 ` Phillip Wood
@ 2022-01-28 6:07 ` Eric Wong
2 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2022-01-28 6:07 UTC (permalink / raw)
To: John Cai; +Cc: git, avarab, me, phillip.wood123
John Cai <johncai86@gmail.com> wrote:
> This RFC patch proposes a new flag --stdin-cmd that works with
> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept
> commands and arguments from stdin.
>
> The start of this idea was discussed in [1], where the original
> motivation was to be able to control when the buffer was flushed to
> stdout in --buffer mode.
>
> However, this can actually be much more useful in situations when
> git-cat-file --batch is being used as a long lived backend query
> process. At GitLab, we use a pair of cat-file processes. One for
> iterating over object metadata with --batch-check, and the other to grab
> object contents with --batch. However, if we had --stdin-cmd, we could
> get rid of the second --batch-check process, and just have one progress
> where we can flip between getting object info, and getting object contents.
> This can lead to huge savings.
Cool. I wish I had this back when I worked on a project which uses git.
In case I ever continue, having 2-3 features in git itself would be useful:
1) ability to add new alternates / object directories on-the-fly
2) ability to deal with unlinked packs
And maybe:
3) ability to remove alternates
I've implemented 1 and 2 via libgit2; but not yet 3 (not really
important, atm).
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-28 6:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 22:50 [RFC v2] cat-file: add a --stdin-cmd mode John Cai
2022-01-25 23:00 ` John Cai
2022-01-27 11:25 ` Phillip Wood
2022-01-27 21:04 ` John Cai
2022-01-28 4:16 ` John Cai
2022-01-28 6:07 ` Eric Wong
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).