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

[Elijah Newren <newren@gmail.com>]

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.

> 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.

>  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.