git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Teng Long <dyroneteng@gmail.com>
Date
Jan 5, 2022, 09:58 UTC
Message-ID
<CADMgQSSjoxqzBDyGXiNC4wHqYGK7z4O0SG0zai85D-DtDHem=w@mail.gmail.com>
In-Reply-To
<nycvar.QRO.7.76.6.2201041533540.7076@tvgsbejvaqbjf.bet>
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写道：
Show 362 quoted lines
>
> 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
> >
> >
> >
Previous: Teng LongNext: Johannes Schindelin
Message 19 of 74 in “ls-tree: "--object-only" and "--format" opts”
  1. 0/8 ls-tree: "--object-only" and "--format" optsTeng Long, Jan 1, 2022
  2. 1/8 ls-tree: remove commented-out codeTeng Long, Jan 1, 2022
  3. 2/8 ls-tree: add missing braces to "else" armsTeng Long, Jan 1, 2022
  4. 3/8 ls-tree: use "enum object_type", not {blob,tree,commit}_typeTeng Long, Jan 1, 2022
  5. 4/8 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"Teng Long, Jan 1, 2022
  6. 5/8 ls-tree: split up the "init" part of show_tree()Teng Long, Jan 1, 2022
  7. Junio C HamanoJan 4, 2022
  8. Teng LongJan 4, 2022
  9. 6/8 ls-tree.c: support --object-only option for "git-ls-tree"Teng Long, Jan 1, 2022
  10. Junio C HamanoJan 4, 2022
  11. Teng LongJan 4, 2022
  12. 7/8 ls-tree.c: introduce struct "shown_data"Teng Long, Jan 1, 2022
  13. Junio C HamanoJan 3, 2022
  14. Teng LongJan 4, 2022
  15. 8/8 ls-tree.c: introduce "--format" optionTeng Long, Jan 1, 2022
  16. Johannes SchindelinJan 4, 2022
  17. Johannes SchindelinJan 4, 2022
  18. Teng LongJan 5, 2022
  19. Teng LongJan 5, 2022
  20. Johannes SchindelinJan 5, 2022
  21. Teng LongJan 5, 2022
  22. 0/9 ls-tree.c: introduce "--format" optionTeng Long, Jan 6, 2022
  23. 1/9 ls-tree: remove commented-out codeTeng Long, Jan 6, 2022
  24. 2/9 ls-tree: add missing braces to "else" armsTeng Long, Jan 6, 2022
  25. 3/9 ls-tree: use "enum object_type", not {blob,tree,commit}_typeTeng Long, Jan 6, 2022
  26. 4/9 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"Teng Long, Jan 6, 2022
  27. 5/9 ls-tree: optimize naming and handling of "return" in show_tree()Teng Long, Jan 6, 2022
  28. Junio C HamanoJan 6, 2022
  29. Teng LongJan 11, 2022
  30. 6/9 ls-tree.c: support --object-only option for "git-ls-tree"Teng Long, Jan 6, 2022
  31. 7/9 ls-tree.c: introduce struct "show_tree_data"Teng Long, Jan 6, 2022
  32. 8/9 ls-tree.c: introduce "--format" optionTeng Long, Jan 6, 2022
  33. Martin ÅgrenJan 10, 2022
  34. Teng LongJan 11, 2022
  35. 9/9 cocci: allow padding with `strbuf_addf()`Teng Long, Jan 6, 2022
  36. Johannes SchindelinJan 7, 2022
  37. Teng LongJan 10, 2022
  38. Johannes SchindelinJan 10, 2022
  39. Teng LongJan 10, 2022
  40. Junio C HamanoJan 10, 2022
  41. Ævar Arnfjörð BjarmasonJan 10, 2022
  42. Junio C HamanoJan 10, 2022
  43. Ævar Arnfjörð BjarmasonJan 10, 2022
  44. Teng LongJan 11, 2022
  45. Taylor BlauJan 11, 2022
  46. René ScharfeJan 11, 2022
  47. Taylor BlauJan 11, 2022
  48. Teng LongJan 13, 2022
  49. Ævar Arnfjörð BjarmasonJan 11, 2022
  50. Teng LongJan 13, 2022
  51. Teng LongJan 13, 2022
  52. 0/9 ls-tree: "--object-only" and "--format" optsTeng Long, Jan 13, 2022
  53. 1/9 ls-tree: remove commented-out codeTeng Long, Jan 13, 2022
  54. 2/9 ls-tree: add missing braces to "else" armsTeng Long, Jan 13, 2022
  55. 3/9 ls-tree: use "enum object_type", not {blob,tree,commit}_typeTeng Long, Jan 13, 2022
  56. 4/9 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"Teng Long, Jan 13, 2022
  57. 5/9 ls-tree: optimize naming and handling of "return" in show_tree()Teng Long, Jan 13, 2022
  58. Ævar Arnfjörð BjarmasonJan 13, 2022
  59. Teng LongJan 14, 2022
  60. Ævar Arnfjörð BjarmasonJan 14, 2022
  61. 6/9 ls-tree.c: support --object-only option for "git-ls-tree"Teng Long, Jan 13, 2022
  62. Ævar Arnfjörð BjarmasonJan 13, 2022
  63. Teng LongJan 14, 2022
  64. Ævar Arnfjörð BjarmasonJan 14, 2022
  65. Teng LongJan 18, 2022
  66. Ævar Arnfjörð BjarmasonFeb 4, 2022
  67. Teng LongFeb 7, 2022
  68. 7/9 ls-tree.c: introduce struct "show_tree_data"Teng Long, Jan 13, 2022
  69. Ævar Arnfjörð BjarmasonJan 13, 2022
  70. Teng LongJan 14, 2022
  71. 8/9 cocci: allow padding with `strbuf_addf()`Teng Long, Jan 13, 2022
  72. 9/9 ls-tree.c: introduce "--format" optionTeng Long, Jan 13, 2022
  73. Ævar Arnfjörð BjarmasonJan 13, 2022
  74. Teng LongJan 18, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.