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

From: Elijah Newren <newren@gmail.com>
Date: Jan 20, 2023, 05:30 UTC
Message-ID: <CABPp-BGLmhoHAcuLoz_yQ4TmNBvDU6Ehymy_3rh0wguSw0hjGw@mail.gmail.com>
In-Reply-To: <9ebd6b77a69be414388a52a482912173f2a4e7d8.1674149666.git.gitgitgadget@gmail.com>

On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 9 quoted lines

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

Show 20 quoted lines

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

Show 13 quoted lines

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

Show 33 quoted lines

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