Re: [PATCH v8 8/8] ls-tree.c: introduce "--format" option

[Teng Long <dyroneteng@gmail.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@gmx.de> 于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, &quoted, NULL, 0);
> -               strbuf_addstr(line, quoted.buf);
> +               strbuf_addbuf(line, &quoted);
>         } 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, &quoted, 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
> >
> >
> >