git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Victoria Dye" <vdye@github.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"ZheNing Hu" <adlternative@gmail.com>
Subject: Re: [PATCH] ls-files: add %(skipworktree) atom to format option
Date: Thu, 12 Jan 2023 02:00:03 -0800	[thread overview]
Message-ID: <CABPp-BGsD=6PiJtnsuYPsiZJ1rm2X8yTeu-YeP4q5uu5UDw2og@mail.gmail.com> (raw)
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:
>
> 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>
[...]
> 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)?

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

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

  reply	other threads:[~2023-01-12 10:04 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-11 15:42 [PATCH] ls-files: add %(skipworktree) atom to format option ZheNing Hu via GitGitGadget
2023-01-12 10:00 ` Elijah Newren [this message]
2023-01-12 19:58   ` Junio C Hamano
2023-01-13  4:43     ` Elijah Newren
2023-01-13 17:07       ` ZheNing Hu
2023-01-13 16:50   ` ZheNing Hu
2023-01-14  3:00     ` Elijah Newren
2023-01-12 10:07 ` Ævar Arnfjörð Bjarmason
2023-01-13 16:59   ` ZheNing Hu
2023-01-19 17:34 ` [PATCH v2 0/2] " ZheNing Hu via GitGitGadget
2023-01-19 17:34   ` [PATCH v2 1/2] docs: fix sparse-checkout docs link ZheNing Hu via GitGitGadget
2023-01-20  5:12     ` Elijah Newren
2023-01-20  9:35       ` Martin Ågren
2023-01-23 15:16         ` ZheNing Hu
2023-01-23 15:15       ` ZheNing Hu
2023-01-19 17:34   ` [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option ZheNing Hu via GitGitGadget
2023-01-20  5:30     ` Elijah Newren
2023-01-20 16:34       ` Junio C Hamano
2023-01-23 15:35         ` ZheNing Hu
2023-01-23 22:39           ` Junio C Hamano
2023-01-23 15:33       ` ZheNing Hu
2023-02-04 16:16   ` [PATCH v3 0/2] " ZheNing Hu via GitGitGadget
2023-02-04 16:16     ` [PATCH v3 1/2] docs: fix sparse-checkout docs link ZheNing Hu via GitGitGadget
2023-02-04 16:16     ` [PATCH v3 2/2] ls-files: add %(skipworktree) atom to format option ZheNing Hu via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BGsD=6PiJtnsuYPsiZJ1rm2X8yTeu-YeP4q5uu5UDw2og@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=adlternative@gmail.com \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).