Re: [PATCH 2/2] catfile.c: add --batch-command mode
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-04 12:22:47
On Thu, Feb 03 2022, John Cai via GitGitGadget wrote:
From: John Cai <redacted>
[Trying not to pile on and mentioning some things others haven't, but maybe there'll be duplications]
quoted hunk ↗ jump to hunk
requested batch operation on all objects in the repository anddiff --git a/builtin/cat-file.c b/builtin/cat-file.c index 858bca208ff..29d5cd6857b 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c@@ -26,6 +26,7 @@ struct batch_options { int unordered; int mode; /* may be 'w' or 'c' for --filters or --textconv */ const char *format; + int command; };
Not sure why it's not added to "int mode", but in any case post-ab/cat-file that might be clearer...
+ /* Read each line dispatch its command */
+ while (!strbuf_getline(&input, stdin)) {I think comments that are obvious from the code are probably best dropped. We're just doing a fairly obvious consume stdin pattern here.
+ int i;
+ const struct parse_cmd *cmd = NULL;
+ const char *p, *cmd_end;
+
+ 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);These should use _() to mark strings for translation, and let's quote %s like '%s'
+ }
+
+ for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ const char *prefix = commands[i].prefix;
+ char c;nit: add a \n between variable decls and code.
+ if (!skip_prefix(input.buf, 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. + */
I'd say ditto on the "the code says that" comments...
+ 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)
+ p = cmd_end + 1;
+ break;
+ }
+
+ if (input.buf[input.len - 1] == '\n')
+ input.buf[--input.len] = '\0';Don't we mean strbuf_trim_trailing_newline() here, or do we not want Windows newlines to be accepted? But more generally doesn't one of the strbuf_getline_*() functions do the right thing here already?
+
+ if (state == BATCH_STATE_INPUT && !cmd){
+ string_list_append(&revs, input.buf);
Nit: You can save yourself some malloc() churn here with:
string_list_append_nodup(..., strbuf_detach(&input, NULL));
I.e. we're looping over the input, here we're done, so we might as well
steal the already alloc'd string....
+ continue;
+ }
+
+ if (!cmd)
+ die("unknown command: %s", input.buf);
+
+ state = cmd->next_state;
+ cmd->fn(opt, p, output, data, revs);
+ }
+ strbuf_release(&input);
+ string_list_clear(&revs, 0);...and these will do the right thing, as strbuf will notice the string is stolen (it'll be the slopbuf again), and due to the combination of *_DUP and *_nodup() we'll properly free it here too.
quoted hunk ↗ jump to hunk
+} + static int batch_objects(struct batch_options *opt) { struct strbuf input = STRBUF_INIT;@@ -519,6 +665,7 @@ static int batch_objects(struct batch_options *opt) struct expand_data data; int save_warning; int retval = 0; + const int command = opt->command; if (!opt->format) opt->format = "%(objectname) %(objecttype) %(objectsize)";@@ -594,22 +741,25 @@ 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) + batch_objects_command(opt, &output, &data); + else {
Style: {} braces for all arms if one requires it.
+ while (strbuf_getline(&input, stdin) != EOF) {
+ if (data.split_on_whitespace) {
diff nit: maybe we can find some way to not require re-indenting the existing code. E.g.:
if (command) {
batch_objects_command(...);
goto cleanup;
}
...
+ /*
+ * 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);
}
...and then just add a "cleanup:" label here.
quoted hunk ↗ jump to hunk
strbuf_release(&input);@@ -646,6 +796,7 @@ 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"); bo->format = arg; return 0;@@ -682,6 +833,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) N_("show info about objects fed from the standard input"), PARSE_OPT_OPTARG | PARSE_OPT_NONEG, batch_option_callback), + OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
You're either missing a string here in "", or we don't need N_() to mark it for translation.
quoted hunk ↗ jump to hunk
+ 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,diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 39382fa1958..7360d049113 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh@@ -85,6 +85,34 @@ $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 "$(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 && + 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_cmp expect actual + ' + test_expect_success "custom --batch-check format" ' echo "$type $sha1" >expect && echo $sha1 | git cat-file --batch-check="%(objecttype) %(objectname)" >actual &&@@ -141,6 +169,7 @@ test_expect_success '--batch-check without %(rest) considers whole line' ' ' tree_sha1=$(git write-tree) +
stray newline addition.
quoted hunk ↗ jump to hunk
tree_size=$(($(test_oid rawsz) + 13)) tree_pretty_content="100644 blob $hello_sha1 hello"@@ -175,7 +204,7 @@ test_expect_success \ "Reach a blob from a tag pointing to it" \ "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\"" -for batch in batch batch-check +for batch in batch batch-check batch-command do for opt in t s e p do@@ -281,6 +310,15 @@ test_expect_success "--batch-check with multiple sha1s gives correct format" ' "$(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 +'
indentation with spaces, \t correctly used for the rest.