Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > This, along with two other similar instances, triggers the > `static-analysis` job in the CI failure of `seen`. The suggested diff is: The second and third I will optimize in the next patch. The first one. Actually I am a little puzzled from this : > - strbuf_addf(line, "%7s", "-"); > + strbuf_addstr(line, "-"); > But I think that the first hunk indicates a deeper issue, as `%7s` > probably meant to pad the dash to seven dashes (which that format won't > accomplish, but `strbuf_addchars()` would)? "strbuf_addf(line, "%7s", "-");" here is used to align the columns with a width of seven chars, not repeat one DASH to seven. A little weird about the fix recommendation of "strbuf_addstr(line, "-");" , because it will only add a single DASH here. It's the identical result which compares to the "master"[1] I think with the current codes and I tested the "strbuf_addf()" simply and it seems to work fine. [1] https://github.com/git/git/blob/master/builtin/ls-tree.c#L106 Thanks. Johannes Schindelin <Johannes.Schindelin@xxxxxx> 于2022年1月4日周二 22:38写道： > > Hi Teng, > > On Sat, 1 Jan 2022, Teng Long wrote: > > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > > index 009ffeb15d..6e3e5a4d06 100644 > > --- a/builtin/ls-tree.c > > +++ b/builtin/ls-tree.c > > @@ -56,23 +56,75 @@ enum { > > > > static int cmdmode = MODE_UNSPECIFIED; > > > > -static int parse_shown_fields(void) > > +static const char *format; > > +static const char *default_format = "%(mode) %(type) %(object)%x09%(file)"; > > +static const char *long_format = "%(mode) %(type) %(object) %(size:padded)%x09%(file)"; > > +static const char *name_only_format = "%(file)"; > > +static const char *object_only_format = "%(object)"; > > + > > +static void expand_objectsize(struct strbuf *line, const struct object_id *oid, > > + const enum object_type type, unsigned int padded) > > { > > - if (cmdmode == MODE_NAME_ONLY) { > > - shown_bits = SHOW_FILE_NAME; > > - return 0; > > + if (type == OBJ_BLOB) { > > + unsigned long size; > > + if (oid_object_info(the_repository, oid, &size) < 0) > > + die(_("could not get object info about '%s'"), > > + oid_to_hex(oid)); > > + if (padded) > > + strbuf_addf(line, "%7" PRIuMAX, (uintmax_t)size); > > + else > > + strbuf_addf(line, "%" PRIuMAX, (uintmax_t)size); > > + } else if (padded) { > > + strbuf_addf(line, "%7s", "-"); > > This, along with two other similar instances, triggers the > `static-analysis` job in the CI failure of `seen`. The suggested diff is: > > > -- snip -- > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 6e3e5a4d0634..8301d1a15f9a 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -75,7 +75,7 @@ static void expand_objectsize(struct strbuf *line, const struct object_id *oid, > else > strbuf_addf(line, "%" PRIuMAX, (uintmax_t)size); > } else if (padded) { > - strbuf_addf(line, "%7s", "-"); > + strbuf_addstr(line, "-"); > } else { > strbuf_addstr(line, "-"); > } > @@ -110,7 +110,7 @@ static size_t expand_show_tree(struct strbuf *line, const char *start, > } else if (skip_prefix(start, "(size)", &p)) { > expand_objectsize(line, data->oid, data->type, 0); > } else if (skip_prefix(start, "(object)", &p)) { > - strbuf_addstr(line, find_unique_abbrev(data->oid, abbrev)); > + strbuf_add_unique_abbrev(line, data->oid, abbrev); > } else if (skip_prefix(start, "(file)", &p)) { > const char *name = data->base->buf; > const char *prefix = chomp_prefix ? ls_tree_prefix : NULL; > @@ -119,7 +119,7 @@ static size_t expand_show_tree(struct strbuf *line, const char *start, > strbuf_addstr(data->base, data->pathname); > name = relative_path(data->base->buf, prefix, &sb); > quote_c_style(name, "ed, NULL, 0); > - strbuf_addstr(line, quoted.buf); > + strbuf_addbuf(line, "ed); > } else { > errlen = (unsigned long)len; > die(_("bad ls-tree format: %%%.*s"), errlen, start); > -- snap -- > > But I think that the first hunk indicates a deeper issue, as `%7s` > probably meant to pad the dash to seven dashes (which that format won't > accomplish, but `strbuf_addchars()` would)? > > Ciao, > Dscho > > > + } else { > > + strbuf_addstr(line, "-"); > > } > > - if (cmdmode == MODE_OBJECT_ONLY) { > > - shown_bits = SHOW_OBJECT_NAME; > > - return 0; > > +} > > + > > +static size_t expand_show_tree(struct strbuf *line, const char *start, > > + void *context) > > +{ > > + struct shown_data *data = context; > > + const char *end; > > + const char *p; > > + unsigned int errlen; > > + size_t len; > > + len = strbuf_expand_literal_cb(line, start, NULL); > > + if (len) > > + return len; > > + > > + if (*start != '(') > > + die(_("bad ls-tree format: as '%s'"), start); > > + > > + end = strchr(start + 1, ')'); > > + if (!end) > > + die(_("bad ls-tree format: element '%s' does not end in ')'"), start); > > + > > + len = end - start + 1; > > + if (skip_prefix(start, "(mode)", &p)) { > > + strbuf_addf(line, "%06o", data->mode); > > + } else if (skip_prefix(start, "(type)", &p)) { > > + strbuf_addstr(line, type_name(data->type)); > > + } else if (skip_prefix(start, "(size:padded)", &p)) { > > + expand_objectsize(line, data->oid, data->type, 1); > > + } else if (skip_prefix(start, "(size)", &p)) { > > + expand_objectsize(line, data->oid, data->type, 0); > > + } else if (skip_prefix(start, "(object)", &p)) { > > + strbuf_addstr(line, find_unique_abbrev(data->oid, abbrev)); > > + } else if (skip_prefix(start, "(file)", &p)) { > > + const char *name = data->base->buf; > > + const char *prefix = chomp_prefix ? ls_tree_prefix : NULL; > > + struct strbuf quoted = STRBUF_INIT; > > + struct strbuf sb = STRBUF_INIT; > > + strbuf_addstr(data->base, data->pathname); > > + name = relative_path(data->base->buf, prefix, &sb); > > + quote_c_style(name, "ed, NULL, 0); > > + strbuf_addstr(line, quoted.buf); > > + } else { > > + errlen = (unsigned long)len; > > + die(_("bad ls-tree format: %%%.*s"), errlen, start); > > } > > - if (!ls_options || (ls_options & LS_RECURSIVE) > > - || (ls_options & LS_SHOW_TREES) > > - || (ls_options & LS_TREE_ONLY)) > > - shown_bits = SHOW_DEFAULT; > > - if (cmdmode == MODE_LONG) > > - shown_bits = SHOW_DEFAULT | SHOW_SIZE; > > - return 1; > > + return len; > > } > > > > static int show_recursive(const char *base, size_t baselen, > > @@ -106,6 +158,75 @@ static int show_recursive(const char *base, size_t baselen, > > return 0; > > } > > > > +static int show_tree_init(enum object_type *type, struct strbuf *base, > > + const char *pathname, unsigned mode, int *retval) > > +{ > > + if (S_ISGITLINK(mode)) { > > + *type = OBJ_COMMIT; > > + } else if (S_ISDIR(mode)) { > > + if (show_recursive(base->buf, base->len, pathname)) { > > + *retval = READ_TREE_RECURSIVE; > > + if (!(ls_options & LS_SHOW_TREES)) > > + return 1; > > + } > > + *type = OBJ_TREE; > > + } > > + else if (ls_options & LS_TREE_ONLY) > > + return 1; > > + return 0; > > +} > > + > > +static int show_tree_fmt(const struct object_id *oid, struct strbuf *base, > > + const char *pathname, unsigned mode, void *context) > > +{ > > + size_t baselen; > > + int retval = 0; > > + struct strbuf line = STRBUF_INIT; > > + struct shown_data data = { > > + .mode = mode, > > + .type = OBJ_BLOB, > > + .oid = oid, > > + .pathname = pathname, > > + .base = base, > > + }; > > + > > + if (show_tree_init(&data.type, base, pathname, mode, &retval)) > > + return retval; > > + > > + baselen = base->len; > > + strbuf_expand(&line, format, expand_show_tree, &data); > > + strbuf_addch(&line, line_termination); > > + fwrite(line.buf, line.len, 1, stdout); > > + strbuf_setlen(base, baselen); > > + return retval; > > +} > > + > > +static int parse_shown_fields(void) > > +{ > > + if (cmdmode == MODE_NAME_ONLY || > > + (format && !strcmp(format, name_only_format))) { > > + shown_bits = SHOW_FILE_NAME; > > + return 1; > > + } > > + > > + if (cmdmode == MODE_OBJECT_ONLY || > > + (format && !strcmp(format, object_only_format))) { > > + shown_bits = SHOW_OBJECT_NAME; > > + return 1; > > + } > > + > > + if (!ls_options || (ls_options & LS_RECURSIVE) > > + || (ls_options & LS_SHOW_TREES) > > + || (ls_options & LS_TREE_ONLY) > > + || (format && !strcmp(format, default_format))) > > + shown_bits = SHOW_DEFAULT; > > + > > + if (cmdmode == MODE_LONG || > > + (format && !strcmp(format, long_format))) > > + shown_bits = SHOW_DEFAULT | SHOW_SIZE; > > + return 1; > > +} > > + > > static int show_default(struct shown_data *data) > > { > > size_t baselen = data->base->len; > > @@ -137,24 +258,6 @@ static int show_default(struct shown_data *data) > > return 1; > > } > > > > -static int show_tree_init(enum object_type *type, struct strbuf *base, > > - const char *pathname, unsigned mode, int *retval) > > -{ > > - if (S_ISGITLINK(mode)) { > > - *type = OBJ_COMMIT; > > - } else if (S_ISDIR(mode)) { > > - if (show_recursive(base->buf, base->len, pathname)) { > > - *retval = READ_TREE_RECURSIVE; > > - if (!(ls_options & LS_SHOW_TREES)) > > - return 1; > > - } > > - *type = OBJ_TREE; > > - } > > - else if (ls_options & LS_TREE_ONLY) > > - return 1; > > - return 0; > > -} > > - > > static int show_tree(const struct object_id *oid, struct strbuf *base, > > const char *pathname, unsigned mode, void *context) > > { > > @@ -196,6 +299,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > > struct object_id oid; > > struct tree *tree; > > int i, full_tree = 0; > > + read_tree_fn_t fn = show_tree; > > const struct option ls_tree_options[] = { > > OPT_BIT('d', NULL, &ls_options, N_("only show trees"), > > LS_TREE_ONLY), > > @@ -218,6 +322,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > > OPT_BOOL(0, "full-tree", &full_tree, > > N_("list entire tree; not just current directory " > > "(implies --full-name)")), > > + OPT_STRING_F(0, "format", &format, N_("format"), > > + N_("format to use for the output"), > > + PARSE_OPT_NONEG), > > OPT__ABBREV(&abbrev), > > OPT_END() > > }; > > @@ -238,6 +345,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > > ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) > > ls_options |= LS_SHOW_TREES; > > > > + if (format && cmdmode) > > + usage_msg_opt( > > + _("--format can't be combined with other format-altering options"), > > + ls_tree_usage, ls_tree_options); > > if (argc < 1) > > usage_with_options(ls_tree_usage, ls_tree_options); > > if (get_oid(argv[0], &oid)) > > @@ -261,6 +372,18 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > > tree = parse_tree_indirect(&oid); > > if (!tree) > > die("not a tree object"); > > - return !!read_tree(the_repository, tree, > > - &pathspec, show_tree, NULL); > > + > > + /* > > + * The generic show_tree_fmt() is slower than show_tree(), so > > + * take the fast path if possible. > > + */ > > + if (format && (!strcmp(format, default_format) || > > + !strcmp(format, long_format) || > > + !strcmp(format, name_only_format) || > > + !strcmp(format, object_only_format))) > > + fn = show_tree; > > + else if (format) > > + fn = show_tree_fmt; > > + > > + return !!read_tree(the_repository, tree, &pathspec, fn, NULL); > > } > > diff --git a/t/t3105-ls-tree-format.sh b/t/t3105-ls-tree-format.sh > > new file mode 100755 > > index 0000000000..92b4d240e8 > > --- /dev/null > > +++ b/t/t3105-ls-tree-format.sh > > @@ -0,0 +1,55 @@ > > +#!/bin/sh > > + > > +test_description='ls-tree --format' > > + > > +TEST_PASSES_SANITIZE_LEAK=true > > +. ./test-lib.sh > > + > > +test_expect_success 'ls-tree --format usage' ' > > + test_expect_code 129 git ls-tree --format=fmt -l && > > + test_expect_code 129 git ls-tree --format=fmt --name-only && > > + test_expect_code 129 git ls-tree --format=fmt --name-status && > > + test_expect_code 129 git ls-tree --format=fmt --object-only > > +' > > + > > +test_expect_success 'setup' ' > > + mkdir dir && > > + test_commit dir/sub-file && > > + test_commit top-file > > +' > > + > > +test_ls_tree_format () { > > + format=$1 && > > + opts=$2 && > > + shift 2 && > > + git ls-tree $opts -r HEAD >expect.raw && > > + sed "s/^/> /" >expect <expect.raw && > > + git ls-tree --format="> $format" -r HEAD >actual && > > + test_cmp expect actual > > +} > > + > > +test_expect_success 'ls-tree --format=<default-like>' ' > > + test_ls_tree_format \ > > + "%(mode) %(type) %(object)%x09%(file)" \ > > + "" > > +' > > + > > +test_expect_success 'ls-tree --format=<long-like>' ' > > + test_ls_tree_format \ > > + "%(mode) %(type) %(object) %(size:padded)%x09%(file)" \ > > + "--long" > > +' > > + > > +test_expect_success 'ls-tree --format=<name-only-like>' ' > > + test_ls_tree_format \ > > + "%(file)" \ > > + "--name-only" > > +' > > + > > +test_expect_success 'ls-tree --format=<object-only-like>' ' > > + test_ls_tree_format \ > > + "%(object)" \ > > + "--object-only" > > +' > > + > > +test_done > > -- > > 2.33.0.rc1.1802.gbb1c3936fb.dirty > > > > > >
