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

From: Elijah Newren <newren@gmail.com>
Date: Jan 12, 2023, 10:00 UTC
Message-ID: <CABPp-BGsD=6PiJtnsuYPsiZJ1rm2X8yTeu-YeP4q5uu5UDw2og@mail.gmail.com>
In-Reply-To: <pull.1458.git.1673451741587.gitgitgadget@gmail.com>

On Wed, Jan 11, 2023 at 7:42 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 6 quoted lines

>
> From: ZheNing Hu <adlternative@gmail.com>
>
> Because sometimes we want to check if the files in the
> index match the sparse specification by using
> `git ls-files -t`, but `-t` option have semi-deprecated,

Where `semi-deprecated` was explicitly "suggest other functionality in preference, but do not ever remove"[1]. The "suggest other functionality in preference" for "ls-files -t" came about because people got confused about staged files which have (a) no unstaged changes, vs. (b) unstaged content changes, vs. (c) unstaged deletion of the file. Such users accidentally presumed that "H" (defined simply as "cached") should only refer to class (a) when it refers to the fact that the file is tracked (and not conflicted) and thus refers to any of (a), (b), and (c)[2]. (I wonder if changing the definition of "H" in the manual from "cached" to "tracked-and-not-conflicted" would fix this confusion.) In contrast, comparing tracked vs. not-tracked-because-skip-worktree files, the distinction between "H" and "S" makes lots of sense and naturally you want "H" to represent all 3 of (a), (b), and (c). So, for the skip-worktree bit usecase, "ls-files -t" doesn't cause the same confusion. (Perhaps the fact that we have a tri-state of "M" (unmerged) vs "S" (skip-worktree) vs. "H" (all other tracked files) could cause minor confusion, but in practice the possibility of "M" just hasn't seemed to have caused issues for sparse-checkout users or scripts.)

Further, since sparse-checkouts and monorepos really started taking off 4-5 years ago, "git ls-files -t" has been used *heavily* (but mostly by low-level script things rather than user-facing UI). If we wanted to come up with a better place to report on the skip-worktree bit and have scripts rely on that, we probably should have made such a change back then...if not another 8-9 years earlier. At this point, "ls-files -t" simply cannot be removed, even if we wanted to.

[1] As per this quote from 5bc0e247c4 ("Document ls-files -t as
semi-obsolete.", 2010-07-28):
    "git ls-files -t" is [...] badly documented, hence we point the
    users to superior alternatives.
    The feature is marked as "semi-obsolete" but not "scheduled for removal"
    since it's a plumbing command, scripts might use it, and Git testsuite
    already uses it to test the state of the index.
[2] https://lore.kernel.org/git/fcaeb9bf0908190204h31bc839ai39972a251040d449@mail.gmail.com/
(a.k.a. gmane:126516 from the commit message referenced above)

> So introduce "%(skipworktree)" atom to git ls-files
> `--format` option.

Given my above comments, I personally don't buy this as justification for adding a new way of reporting on the skip-worktree bit. It may still make sense to add this feature or something like it, but I personally think it deserves separate justification from "`ls-files -t` is semi-deprecated".

(Others, of course, may disagree with me, but if this is the only justification for this change, then I'm more likely to want to fix the ls-files manual to remove the "semi-deprecated" notice and fix the definition of "H" to be less misleading than to make a change like this.)

> When we use this option, if the file
> match the sparse specification and removed from working
> tree...

The "and removed from working tree" portion of this sentence is superfluous. (And actually makes it harder to understand, I had to try to think through a bunch of cases to try to figure out why you might be trying to add some extra qualifier.)

> ...it will output "yes", othewise, output "no".

typo in "otherwise".

Also, your commit message claims output different from what your code below implements and what your testcase shows. ("yes"/"no" vs. "true"/"false")

> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

[...]

Show 14 quoted lines

> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 440043cdb8e..0e50307121d 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -260,6 +260,12 @@ 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
> +       and removed from working tree.
> +       See link:technical/sparse-checkout.txt[sparse-checkout]
> +       for more information.

Should the actual wording be included here? (i.e. "yes"/"no", "true"/"false", or whatever you end up using)?

Show 25 quoted lines

>  EXCLUDE PATTERNS
>  ----------------
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index a03b559ecaa..d1a27f28f01 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) ?
> +                             "true" : "false");
>         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..ac8b865c275 100755
> --- a/t/t3013-ls-files-format.sh
> +++ b/t/t3013-ls-files-format.sh
> @@ -92,4 +92,26 @@ test_expect_success 'git ls-files --format with --debug' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'git ls-files --format with skipworktree' '

Should probably add a
    test_when_finished "git sparse-checkout disable" &&
at the beginning of this test, otherwise you are potentially causing
confusion to future developers who try to add additional testcases to
this file.

Show 20 quoted lines

> +       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 false
> +       dir2/file2.txt true
> +       o1.txt false
> +       o2.txt false
> +       o3.txt false
> +       o4.txt false
> +       o5.txt false
> +       o6.txt false
> +       o7.txt false
> +       EOF
> +       test_cmp expect actual
> +'

To be honest, I don't yet see any compelling reason to use this new option. Even if this patch is accepted, I'd just continue using "git ls-files -t" (both directly and in scripts) in preference to this. However, you have inspired me to try to fix up the ls-files documentation and remove the "semi-deprecated" label for the -t option. Patches over here: https://github.com/gitgitgadget/git/pull/1463 ; I'll submit them in the next few days.