Re: [PATCH 2/2] ls-files: introduce "--object-only" option

[ZheNing Hu <adlternative@gmail.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年6月16日周四 04:25写道：
>
>
> On Wed, Jun 15 2022, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > --object-only is an alias for --format=%(objectname),
> > which output objectname of index entries, taking
> > inspiration from the option with the same name in
> > the `git ls-tree` command.
> >
> > --object-only cannot be used with --format, and -s, -o,
> > -k, --resolve-undo, --deduplicate, --debug.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >  Documentation/git-ls-files.txt |  8 +++++++-
> >  builtin/ls-files.c             | 36 +++++++++++++++++++++++++++++++++-
> >  t/t3013-ls-files-format.sh     | 34 ++++++++++++++++++++++++++++++++
> >  3 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> > index b22860ec8c0..c3f46bb821b 100644
> > --- a/Documentation/git-ls-files.txt
> > +++ b/Documentation/git-ls-files.txt
> > @@ -13,7 +13,7 @@ SYNOPSIS
> >               [-c|--cached] [-d|--deleted] [-o|--others] [-i|--|ignored]
> >               [-s|--stage] [-u|--unmerged] [-k|--|killed] [-m|--modified]
> >               [--directory [--no-empty-directory]] [--eol]
> > -             [--deduplicate]
> > +             [--deduplicate] [--object-only]
> >               [-x <pattern>|--exclude=<pattern>]
> >               [-X <file>|--exclude-from=<file>]
> >               [--exclude-per-directory=<file>]
> > @@ -199,6 +199,12 @@ followed by the  ("attr/<eolattr>").
> >       interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
> >       --format cannot be combined with `-s`, `-o`, `-k`, `--resolve-undo`,
> >       `--debug`.
> > +
> > +--object-only::
> > +     List only names of the objects, one per line. This is equivalent
> > +     to specifying `--format='%(objectname)'`. Cannot be combined with
> > +     `--format=<format>`.
> > +
> >  \--::
> >       Do not interpret any more arguments as options.
> >
> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> > index 9dd6c55eeb9..4ac8f34baac 100644
> > --- a/builtin/ls-files.c
> > +++ b/builtin/ls-files.c
> > @@ -60,6 +60,27 @@ static const char *tag_modified = "";
> >  static const char *tag_skip_worktree = "";
> >  static const char *tag_resolve_undo = "";
> >
> > +static enum ls_files_cmdmode {
> > +     MODE_DEFAULT = 0,
> > +     MODE_OBJECT_ONLY,
> > +} ls_files_cmdmode;
> > +
> > +struct ls_files_cmdmodee_to_fmt {
> > +     enum ls_files_cmdmode mode;
> > +     const char *const fmt;
> > +};
> > +
> > +static struct ls_files_cmdmodee_to_fmt ls_files_cmdmode_format[] = {
> > +     {
> > +             .mode = MODE_DEFAULT,
> > +             .fmt = NULL,
> > +     },
> > +     {
> > +             .mode = MODE_OBJECT_ONLY,
> > +             .fmt = "%(objectname)",
> > +     },
> > +};
> [...snip...]
>
> This code all looks OK from skimming it, and is substantially copied
> from builtin/ls-tree.c (which is good).
>
> But I wonder as in that case whether having such an alias is worth it at
> all, especially since in the case of ls-files (unlike ls-tree) we don't
> start out with various --just-the-X-field type options, this is the
> first one.
>
> So I *really* like that you took my suggestion of "why not a --format"
> from a previous round, but given the above for ls-files in particular is
> it really worth it to have this extra code just to type:
>
>     --object-only
>
> Instead of:
>
>     --format="%(objectname)"
>
> So, maybe, and I'm not set against it, but I think it's worth
> re-evaluating in this case.
>
> In particular because the part of ls-tree's code is missing here where
> we "format optimize", i.e. we take a form like:
>
>     --format="%(objectname)"
>
> And dispatch it to the more optimized special function, instead of the
> generic strbuf_expand(), whereas in this case it's the other way around,
> the option is just an alias for --format.

Thanks for clarifying that --object-only uses the fast path instead of a simple
alias of --format=%(objectname). Maybe I should do this too, or just
drop this patch
because git ls-file --format has included such a function :-)

ZheNing Hu