Re: [PATCH v2] ls-files: introduce "--format" option

>
> Hi ZheNing
>
> On 19/06/2022 10:13, ZheNing Hu via GitGitGadget wrote:
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Add a new option --format that output index enties
> > informations with custom format, taking inspiration
> > from the option with the same name in the `git ls-tree`
> > command.
> >
> > --format cannot used with -s, -o, -k, --resolve-undo,
> > --deduplicate and --debug.
>
> I think this is an interesting feature that provides functionality that
> is not available by feeding index entries into cat-file.
>

> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> >   Documentation/git-ls-files.txt |  51 ++++++++++++-
> >   builtin/ls-files.c             | 130 ++++++++++++++++++++++++++++++++-
> >   t/t3013-ls-files-format.sh     | 130 +++++++++++++++++++++++++++++++++
> >   3 files changed, 307 insertions(+), 4 deletions(-)
> >   create mode 100755 t/t3013-ls-files-format.sh
> >
> > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> > index 0dabf3f0ddc..9a88c92f1ad 100644
> > --- a/Documentation/git-ls-files.txt
> > +++ b/Documentation/git-ls-files.txt
> > @@ -20,7 +20,7 @@ SYNOPSIS
> >               [--exclude-standard]
> >               [--error-unmatch] [--with-tree=<tree-ish>]
> >               [--full-name] [--recurse-submodules]
> > -             [--abbrev[=<n>]] [--] [<file>...]
> > +             [--abbrev[=<n>]] [--format=<format>] [--] [<file>...]
> >
> >   DESCRIPTION
> >   -----------
> > @@ -192,6 +192,13 @@ followed by the  ("attr/<eolattr>").
> >       to the contained files. Sparse directories will be shown with a
> >       trailing slash, such as "x/" for a sparse directory "x".
> >
> > +--format=<format>::
> > +     A string that interpolates `%(fieldname)` from the result being shown.
> > +     It also interpolates `%%` to `%`, and `%xx` where `xx` are hex digits
> > +     interpolates to character with hex code `xx`; for example `%00`
> > +     interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
> > +     --format cannot be combined with `-s`, `-o`, `-k`, `--resolve-undo` and
> > +     `--debug`.
> >   \--::
> >       Do not interpret any more arguments as options.
> >
> > @@ -223,6 +230,48 @@ quoted as explained for the configuration variable `core.quotePath`
> >   (see linkgit:git-config[1]).  Using `-z` the filename is output
> >   verbatim and the line is terminated by a NUL byte.
> >
> > +It is possible to print in a custom format by using the `--format`
> > +option, which is able to interpolate different fields using
> > +a `%(fieldname)` notation. For example, if you only care about the
> > +"objectname" and "path" fields, you can execute with a specific
> > +"--format" like
> > +
> > +     git ls-files --format='%(objectname) %(path)'
> > +
> > +FIELD NAMES
> > +-----------
> > +Various values from structured fields can be used to interpolate
> > +into the resulting output. For each outputting line, the following
> > +names can be used:
> > +
> > +tag::
> > +     The tag of file status.
>
> The documentation for -t strong discourages its use, so I wonder if we
> really want to expose it here.
>

> > +objectmode::
> > +     The mode of the object.
> > +objectname::
> > +     The name of the object.
> > +stage::
> > +     The stage of the file.
> > +eol::
> > +     The line endings of files.
>
> Every other option refers to either a "file" or "object" but here we
> have "files". Looking at the implementation below this will print the
> line ending from both the index and the worktree, it would be useful to
> clarify that here.
>

> > +path::
> > +     The pathname of the object.
> > +ctime::
> > +     The create time of file.
>
> It is not clear from this whether this (and all the file attributes
> below) are coming from the worktree or the index or both like eol?
>

> > +mtime::
> > +     The modify time of file.
> > +dev::
> > +     The ID of device containing file.
> > +ino::
> > +     The inode number of file.
> > +uid::
> > +     The user id of file owner.
> > +gid::
> > +     The group id of file owner.
> > +size::
> > +     The size of the file.
> > +flags::
> > +     The flags of the file.
>
> What are the flags?
>

> > [...]
> > +static size_t expand_show_index(struct strbuf *sb, const char *start,
> > +                            void *context)
> > +{
> > +     struct show_index_data *data = context;
> > +     const char *end;
> > +     const char *p;
> > +     unsigned int errlen;
>  > [...]
> > +     else if (skip_prefix(start, "(flags)", &p))
> > +             strbuf_addf(sb, "flags: %x", data->ce->ce_flags);
> > +     else {
> > +             errlen = (unsigned long)len;
> > +             die(_("bad ls-files format: %%%.*s"), errlen, start);
>
> errlen is declared as an unsigned int, but you cast len which is a
> size_t to unsigned long when assigning to errlen. Then errlen is used
> where a signed int is required by die. There is also a style violation
> as if any branch of an if needs braces then they should all be braced. I
> think that the best solution would be to drop errlen and just write
>
>         else
>                 die(_("bad ls-files format: %%%.*s"), (int)len, start);
>

> It would be interesting to check the performance of this implementation
> on a large repository as it is doing a lot of branching inside a loop. I
> don't think we should change it unless it turns out to be a problem.
> Then we could try switching on the first character of the format
> specifier or some other optimization.
>