Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option

[ZheNing Hu <adlternative@gmail.com>]

Elijah Newren <newren@gmail.com> 于2023年1月20日周五 13:30写道：
>
> On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Because sometimes we want to check if the files in the
> > index match the sparse specification, so introduce
> > "%(skipworktree)" atom to git ls-files `--format` option.
> > When we use this option, if the file match the sparse
> > specification, it will output "1", otherwise, output
> > empty string "".
>
> Why is that output format useful?  It seems like it'll just lead to
> bugs, or someone re-implementing the same field with a different name
> to make it useful in the future.  In particular, if there are multiple
> boolean fields and someone specifies e.g.
>    git ls-files --format="%(path) %(skipworktree) %(intentToAdd)"
> and both boolean fields are displayed the same way (either a "1" or a
> blank string), and we see something like:
>    foo.c 1
>    bar.c 1
> Then how do we know if foo.c and bar.c are SKIP_WORKTREE or
> INTENT_TO_ADD?  The "1" could have come from either field.
>

I understand your confusion here. If we need to combine these
boolean values in --format with %(if) %(else) of ref-filter in the future,
we can only do this strange design. Output like "true"/"false" or "1"/"0"
would be better without considering %(if), %(else).

> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >  Documentation/git-ls-files.txt |  5 +++++
> >  builtin/ls-files.c             |  3 +++
> >  t/t3013-ls-files-format.sh     | 23 +++++++++++++++++++++++
> >  3 files changed, 31 insertions(+)
> >
> > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> > index 440043cdb8e..2540b404808 100644
> > --- a/Documentation/git-ls-files.txt
> > +++ b/Documentation/git-ls-files.txt
> > @@ -260,6 +260,11 @@ eolattr::
> >         that applies to the path.
> >  path::
> >         The pathname of the file which is recorded in the index.
> > +skipworktree::
> > +       If the file in the index marked with SKIP_WORKTREE bit.
> > +       It means the file do not match the sparse specification.
> > +       See link:technical/sparse-checkout.txt[sparse-checkout]
> > +       for more information.
>
> minor nits: Missing an "is", and "do" should be "does".
>
> I'm curious whether the second sentence is even necessary; we've
> already got the link to the more technical docs.  Perhaps just:
>
> skipworktree::
>     Whether the file in the index has the SKIP_WORKTREE bit set.
>     See link:technical/sparse-checkout.txt[sparse-checkout]
>     for more information.
>

Agree.

> >  EXCLUDE PATTERNS
> >  ----------------
> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> > index a03b559ecaa..bbff868ae6b 100644
> > --- a/builtin/ls-files.c
> > +++ b/builtin/ls-files.c
> > @@ -280,6 +280,9 @@ static size_t expand_show_index(struct strbuf *sb, const char *start,
> >                               data->pathname));
> >         else if (skip_prefix(start, "(path)", &p))
> >                 write_name_to_buf(sb, data->pathname);
> > +       else if (skip_prefix(start, "(skipworktree)", &p))
> > +               strbuf_addstr(sb, ce_skip_worktree(data->ce) ?
> > +                             "1" : "");
>
> As I mentioned in response to the commit message, I don't understand
> why having an empty string would be desirable.
>
> >         else
> >                 die(_("bad ls-files format: %%%.*s"), (int)len, start);
> >
> > diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
> > index efb7450bf1e..cd35dba5930 100755
> > --- a/t/t3013-ls-files-format.sh
> > +++ b/t/t3013-ls-files-format.sh
> > @@ -92,4 +92,27 @@ test_expect_success 'git ls-files --format with --debug' '
> >         test_cmp expect actual
> >  '
> >
> > +test_expect_success 'git ls-files --format with skipworktree' '
> > +       test_when_finished "git sparse-checkout disable" &&
> > +       mkdir dir1 dir2 &&
> > +       echo "file1" >dir1/file1.txt &&
> > +       echo "file2" >dir2/file2.txt &&
> > +       git add dir1 dir2 &&
> > +       git commit -m skipworktree &&
> > +       git sparse-checkout set dir1 &&
> > +       git ls-files --format="%(path)%(skipworktree)" >actual &&
> > +       cat >expect <<-\EOF &&
> > +       dir1/file1.txt
> > +       dir2/file2.txt1
> > +       o1.txt
> > +       o2.txt
> > +       o3.txt
> > +       o4.txt
> > +       o5.txt
> > +       o6.txt
> > +       o7.txt
> > +       EOF
> > +       test_cmp expect actual
> > +'
>
> I find this test hard to read; it's just too easy to miss
> "dir2/file2.txt1" vs "dir2/file2.txt".  I'd suggest at least adding a
> space, and likely having the skipworktree attribute come first in the
> format string.  It might also be useful to add "dir*" on the ls-files
> command to limit which paths are shown, just because there's an awful
> lot of files in the root directory and no variance between them, and
> it's easier to notice the binary difference between two items than
> having a full 9 and figuring out which are relevant.

Good idea.

I deliberately removed the space between %(path) and
%(skipworktree) before, because according to the current design,
the "" output by %(skipworktree) is empty, which leads to an extra
space at the end of the output line, which will break github's
 "whitespace" ci tests. Maybe swapping the location of %(path) and
%(skipworktree) will fix this.