git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] ls-files: add %(skipworktree) atom to format option
@ 2023-01-11 15:42 ZheNing Hu via GitGitGadget
  2023-01-12 10:00 ` Elijah Newren
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: ZheNing Hu via GitGitGadget @ 2023-01-11 15:42 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye, Elijah Newren,
	Nguyễn Thái Ngọc Duy, ZheNing Hu, ZheNing Hu

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,

So introduce "%(skipworktree)" atom to git ls-files
`--format` option. When we use this option, if the file
match the sparse specification and removed from working
tree, it will output "yes", othewise, output "no".

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    ls-files: add %(skipworktree) atom to format option
    
    git ls-files -t is semi-deprecated, but in face we need to use -t option
    to check if a file in the index match the sparse specification.
    
    So I think this feature can be migrated to git ls-files --format, add a
    %(skipworktree) atom to indicate whether the file in the index match the
    sparse specification and is removed from the working tree.
    
    v1: add %(skipworktree) atom to git ls-files format option.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1458%2Fadlternative%2Fzh%2Fls-file-format-skipworktree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1458/adlternative/zh/ls-file-format-skipworktree-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1458

 Documentation/git-ls-files.txt |  6 ++++++
 builtin/ls-files.c             |  3 +++
 t/t3013-ls-files-format.sh     | 22 ++++++++++++++++++++++
 3 files changed, 31 insertions(+)

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

base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  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
  2023-01-12 19:58   ` Junio C Hamano
  2023-01-13 16:50   ` ZheNing Hu
  2023-01-12 10:07 ` Ævar Arnfjörð Bjarmason
  2023-01-19 17:34 ` [PATCH v2 0/2] " ZheNing Hu via GitGitGadget
  2 siblings, 2 replies; 21+ messages in thread
From: Elijah Newren @ 2023-01-12 10:00 UTC (permalink / raw)
  To: ZheNing Hu via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy, ZheNing Hu

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  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
@ 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
  2 siblings, 1 reply; 21+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 10:07 UTC (permalink / raw)
  To: ZheNing Hu via GitGitGadget
  Cc: git, Junio C Hamano, Derrick Stolee, Victoria Dye, Elijah Newren,
	Nguyễn Thái Ngọc Duy, ZheNing Hu


On Wed, Jan 11 2023, ZheNing Hu via GitGitGadget 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,
>
> So introduce "%(skipworktree)" atom to git ls-files
> `--format` option. When we use this option, if the file
> match the sparse specification and removed from working
> tree, it will output "yes", othewise, output "no".

Your code says "true" and "false", not "yes" or "no". Which is it ?:)

More generally it's unfortanute that we don't support the ref-filter.c
formats more generally, then we could just make this 1 or the empty
string, and you'd do:

	%(if)%(skipworktree)%(then)true%(else)false%(end)

Now, I don't think your change needs to be blocked by generalizing that
if/else stuff in ref-filter, that would probably be some thousand-line
series if we're lucky, even though it's a good eventual goal.

But I feel strongly that you should not pick "true", or "false", or
"yes" or "no", but rather "1" or "", here, as doing so will be
future-proof when it comes to this format being compatible with using
the ref-filter.c conditional support.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     ls-files: add %(skipworktree) atom to format option
>     
>     git ls-files -t is semi-deprecated, but in face we need to use -t option
>     to check if a file in the index match the sparse specification.
>     
>     So I think this feature can be migrated to git ls-files --format, add a
>     %(skipworktree) atom to indicate whether the file in the index match the
>     sparse specification and is removed from the working tree.
>     
>     v1: add %(skipworktree) atom to git ls-files format option.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1458%2Fadlternative%2Fzh%2Fls-file-format-skipworktree-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1458/adlternative/zh/ls-file-format-skipworktree-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1458
>
>  Documentation/git-ls-files.txt |  6 ++++++
>  builtin/ls-files.c             |  3 +++
>  t/t3013-ls-files-format.sh     | 22 ++++++++++++++++++++++
>  3 files changed, 31 insertions(+)
>
> 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.

I likewise think this won't need to be blocked, but this is a sign that
we should probably move these to the main doc namespace. See 1e2320161d2
(docs: move http-protocol docs to man section 5, 2022-08-04) (and
commits before that) for prior art.

But what we should fix here is that this ling is wrong, as you can see
in that commit we need to link to the HTML docs for these (confusing as
that is).

Or (and I didn't check) if we never generate the *.html either for this
particular one this link will always be broken. I.e. we won't install
this, nor the HTML docs.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  2023-01-12 10:00 ` Elijah Newren
@ 2023-01-12 19:58   ` Junio C Hamano
  2023-01-13  4:43     ` Elijah Newren
  2023-01-13 16:50   ` ZheNing Hu
  1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2023-01-12 19:58 UTC (permalink / raw)
  To: Elijah Newren
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Derrick Stolee,
	Victoria Dye, Nguyễn Thái Ngọc Duy, ZheNing Hu

Elijah Newren <newren@gmail.com> writes:

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

Thanks.  I think that would be the better way forward between the
two (i.e. adding this one-shot new feature vs resurrecting -t).

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  2023-01-12 19:58   ` Junio C Hamano
@ 2023-01-13  4:43     ` Elijah Newren
  2023-01-13 17:07       ` ZheNing Hu
  0 siblings, 1 reply; 21+ messages in thread
From: Elijah Newren @ 2023-01-13  4:43 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Derrick Stolee,
	Victoria Dye, Nguyễn Thái Ngọc Duy, ZheNing Hu

On Thu, Jan 12, 2023 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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".
> > ...
> > 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.
>
> Thanks.  I think that would be the better way forward between the
> two (i.e. adding this one-shot new feature vs resurrecting -t).

Submitted over here:
https://lore.kernel.org/git/pull.1463.git.1673584914.gitgitgadget@gmail.com/

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  2023-01-12 10:00 ` Elijah Newren
  2023-01-12 19:58   ` Junio C Hamano
@ 2023-01-13 16:50   ` ZheNing Hu
  2023-01-14  3:00     ` Elijah Newren
  1 sibling, 1 reply; 21+ messages in thread
From: ZheNing Hu @ 2023-01-13 16:50 UTC (permalink / raw)
  To: Elijah Newren
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy

Elijah Newren <newren@gmail.com> 于2023年1月12日周四 18:00写道:
>
> 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.)
>

OK, I probably understand that the '-t' "semi-deprecated" was caused
by the confusion of the 'H' semantics.

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

Ah, It seems we can't go back so far :)

> [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)
>

It was good to discover that I had misunderstood the point:
"semi-obsolete" is not git want to remove `-t`.

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

Agree now.

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

I just quoted the definition of SKIP_WORKTREE from
Documentation/technical/sparse-checkout.txt:

    SKIP_WORKTREE: When tracked files do not match the sparse specification and
      are removed from the working tree, the file in the index is marked
      with a SKIP_WORKTREE bit.

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

Yeah, I would have said "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)?
>

Yes, it will be better to mention its output.

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

Agree.

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

To be honest, right now I think %(skipworktree) just refines the
--format option's ability to read the index entry SKIP_WORKTREE
flag bits. It is probably still worth keeping.

Thanks,
ZheNing Hu

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  2023-01-12 10:07 ` Ævar Arnfjörð Bjarmason
@ 2023-01-13 16:59   ` ZheNing Hu
  0 siblings, 0 replies; 21+ messages in thread
From: ZheNing Hu @ 2023-01-13 16:59 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: ZheNing Hu via GitGitGadget, git, Junio C Hamano, Derrick Stolee,
	Victoria Dye, Elijah Newren,
	Nguyễn Thái Ngọc Duy

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2023年1月12日周四 18:15写道:
>
>
> On Wed, Jan 11 2023, ZheNing Hu via GitGitGadget 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,
> >
> > So introduce "%(skipworktree)" atom to git ls-files
> > `--format` option. When we use this option, if the file
> > match the sparse specification and removed from working
> > tree, it will output "yes", othewise, output "no".
>
> Your code says "true" and "false", not "yes" or "no". Which is it ?:)
>

true/false. This is a typo.


> More generally it's unfortanute that we don't support the ref-filter.c
> formats more generally, then we could just make this 1 or the empty
> string, and you'd do:
>
>         %(if)%(skipworktree)%(then)true%(else)false%(end)
>
> Now, I don't think your change needs to be blocked by generalizing that
> if/else stuff in ref-filter, that would probably be some thousand-line
> series if we're lucky, even though it's a good eventual goal.
>

Aha, if we were to introduce %(if) %(else) in the ref-filter I'm sure that
would be a very complicated situation.

> But I feel strongly that you should not pick "true", or "false", or
> "yes" or "no", but rather "1" or "", here, as doing so will be
> future-proof when it comes to this format being compatible with using
> the ref-filter.c conditional support.
> >

Good point.

> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     ls-files: add %(skipworktree) atom to format option
> >
> >     git ls-files -t is semi-deprecated, but in face we need to use -t option
> >     to check if a file in the index match the sparse specification.
> >
> >     So I think this feature can be migrated to git ls-files --format, add a
> >     %(skipworktree) atom to indicate whether the file in the index match the
> >     sparse specification and is removed from the working tree.
> >
> >     v1: add %(skipworktree) atom to git ls-files format option.
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1458%2Fadlternative%2Fzh%2Fls-file-format-skipworktree-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1458/adlternative/zh/ls-file-format-skipworktree-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1458
> >
> >  Documentation/git-ls-files.txt |  6 ++++++
> >  builtin/ls-files.c             |  3 +++
> >  t/t3013-ls-files-format.sh     | 22 ++++++++++++++++++++++
> >  3 files changed, 31 insertions(+)
> >
> > 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.
>
> I likewise think this won't need to be blocked, but this is a sign that
> we should probably move these to the main doc namespace. See 1e2320161d2
> (docs: move http-protocol docs to man section 5, 2022-08-04) (and
> commits before that) for prior art.
>
> But what we should fix here is that this ling is wrong, as you can see
> in that commit we need to link to the HTML docs for these (confusing as
> that is).
>
> Or (and I didn't check) if we never generate the *.html either for this
> particular one this link will always be broken. I.e. we won't install
> this, nor the HTML docs.

Yeah, you should be right: there may be a problem with the link here,
maybe this needs a patch to migrate technical/sparse-checkout.txt.

Thanks,
ZheNing Hu

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  2023-01-13  4:43     ` Elijah Newren
@ 2023-01-13 17:07       ` ZheNing Hu
  0 siblings, 0 replies; 21+ messages in thread
From: ZheNing Hu @ 2023-01-13 17:07 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Junio C Hamano, ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Derrick Stolee,
	Victoria Dye, Nguyễn Thái Ngọc Duy

Elijah Newren <newren@gmail.com> 于2023年1月13日周五 12:44写道:
>
> On Thu, Jan 12, 2023 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Elijah Newren <newren@gmail.com> writes:
> >
> > > 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".
> > > ...
> > > 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.
> >
> > Thanks.  I think that would be the better way forward between the
> > two (i.e. adding this one-shot new feature vs resurrecting -t).
>
> Submitted over here:
> https://lore.kernel.org/git/pull.1463.git.1673584914.gitgitgadget@gmail.com/

Thanks, I will check them later.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ls-files: add %(skipworktree) atom to format option
  2023-01-13 16:50   ` ZheNing Hu
@ 2023-01-14  3:00     ` Elijah Newren
  0 siblings, 0 replies; 21+ messages in thread
From: Elijah Newren @ 2023-01-14  3:00 UTC (permalink / raw)
  To: ZheNing Hu
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy

On Fri, Jan 13, 2023 at 8:50 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> 于2023年1月12日周四 18:00写道:
> >
> > On Wed, Jan 11, 2023 at 7:42 AM ZheNing Hu via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
[...]
> > 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.)
> >
>
> I just quoted the definition of SKIP_WORKTREE from
> Documentation/technical/sparse-checkout.txt:
>
>     SKIP_WORKTREE: When tracked files do not match the sparse specification and
>       are removed from the working tree, the file in the index is marked
>       with a SKIP_WORKTREE bit.

Ah, thanks for pointing out the error.  This should probably be reworded to:

SKIP_WORKTREE: When a tracked file which is unmodified does not match
        the sparsity patterns, it is removed from the working tree and
        the file in the index is marked with a SKIP_WORKTREE bit (to
        distinguish the missing file from an unstaged deletion).

[...]
> To be honest, right now I think %(skipworktree) just refines the
> --format option's ability to read the index entry SKIP_WORKTREE
> flag bits. It is probably still worth keeping.

I'm not opposed to the idea of a special skipworktree formatting in
conjunction with ls-files' --format option, so long as it has
alternate rationale and isn't worded to suggest it supersedes `git
ls-files -t` for all sparse-checkout uses.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 0/2] ls-files: add %(skipworktree) atom to format option
  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
  2023-01-12 10:07 ` Ævar Arnfjörð Bjarmason
@ 2023-01-19 17:34 ` 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-19 17:34   ` [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option ZheNing Hu via GitGitGadget
  2 siblings, 2 replies; 21+ messages in thread
From: ZheNing Hu via GitGitGadget @ 2023-01-19 17:34 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye, Elijah Newren,
	Nguyễn Thái Ngọc Duy, ZheNing Hu

Add a %(skipworktree) atom git ls-files --format to indicate whether the
file in the index match the sparse specification.

v1: add %(skipworktree) atom to git ls-files format option. v2:

 1. no longer mentioned git ls-files -t.
 2. change %(skipworktree) output from "true"/"false" to "1"/"".
 3. fix the sparse-checkout docs link.

ZheNing Hu (2):
  docs: fix sparse-checkout docs link
  ls-files: add %(skipworktree) atom to format option

 Documentation/Makefile                      |  1 +
 Documentation/git-ls-files.txt              |  5 +++
 Documentation/technical/sparse-checkout.txt | 43 ++++++++++++++-------
 builtin/ls-files.c                          |  3 ++
 t/t3013-ls-files-format.sh                  | 23 +++++++++++
 5 files changed, 61 insertions(+), 14 deletions(-)


base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1458%2Fadlternative%2Fzh%2Fls-file-format-skipworktree-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1458/adlternative/zh/ls-file-format-skipworktree-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1458

Range-diff vs v1:

 -:  ----------- > 1:  cde4827da13 docs: fix sparse-checkout docs link
 1:  c4cd5b3a32f ! 2:  9ebd6b77a69 ls-files: add %(skipworktree) atom to format option
     @@ Commit message
          ls-files: add %(skipworktree) atom to format option
      
          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,
     -
     -    So introduce "%(skipworktree)" atom to git ls-files
     -    `--format` option. When we use this option, if the file
     -    match the sparse specification and removed from working
     -    tree, it will output "yes", othewise, output "no".
     +    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 "".
      
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
     @@ Documentation/git-ls-files.txt: eolattr::
       	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.
     ++	It means the file do not match the sparse specification.
      +	See link:technical/sparse-checkout.txt[sparse-checkout]
      +	for more information.
       
     @@ builtin/ls-files.c: static size_t expand_show_index(struct strbuf *sb, const cha
       		write_name_to_buf(sb, data->pathname);
      +	else if (skip_prefix(start, "(skipworktree)", &p))
      +		strbuf_addstr(sb, ce_skip_worktree(data->ce) ?
     -+			      "true" : "false");
     ++			      "1" : "");
       	else
       		die(_("bad ls-files format: %%%.*s"), (int)len, start);
       
     @@ t/t3013-ls-files-format.sh: test_expect_success 'git ls-files --format with --de
       '
       
      +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 &&
     ++	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
     ++	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
      +'

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/2] docs: fix sparse-checkout docs link
  2023-01-19 17:34 ` [PATCH v2 0/2] " ZheNing Hu via GitGitGadget
@ 2023-01-19 17:34   ` ZheNing Hu via GitGitGadget
  2023-01-20  5:12     ` Elijah Newren
  2023-01-19 17:34   ` [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option ZheNing Hu via GitGitGadget
  1 sibling, 1 reply; 21+ messages in thread
From: ZheNing Hu via GitGitGadget @ 2023-01-19 17:34 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye, Elijah Newren,
	Nguyễn Thái Ngọc Duy, ZheNing Hu, ZheNing Hu

From: ZheNing Hu <adlternative@gmail.com>

There is a linking error when other documents want to refer to
technical/sparse-checkout.txt, Also, there is something wrong
with the format of the paragraphs in sparse-checkout.txt, which
makes acsiidoc compile errors.

So fix the format of sparse-checkout.txt, and link it in the
Makefile to correct that.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 Documentation/Makefile                      |  1 +
 Documentation/technical/sparse-checkout.txt | 43 ++++++++++++++-------
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 9c67c3a1c50..7540da27b8c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -121,6 +121,7 @@ TECH_DOCS += technical/reftable
 TECH_DOCS += technical/scalar
 TECH_DOCS += technical/send-pack-pipeline
 TECH_DOCS += technical/shallow
+TECH_DOCS += technical/sparse-checkout
 TECH_DOCS += technical/trivial-merge
 SP_ARTICLES += $(TECH_DOCS)
 SP_ARTICLES += technical/api-index
diff --git a/Documentation/technical/sparse-checkout.txt b/Documentation/technical/sparse-checkout.txt
index fa0d01cbda4..52e86764a6c 100644
--- a/Documentation/technical/sparse-checkout.txt
+++ b/Documentation/technical/sparse-checkout.txt
@@ -1,3 +1,6 @@
+Sparse Checkout Design Notes
+============================
+
 Table of contents:
 
   * Terminology
@@ -14,7 +17,8 @@ Table of contents:
   * Reference Emails
 
 
-=== Terminology ===
+Terminology
+-----------
 
 cone mode: one of two modes for specifying the desired subset of files
 	in a sparse-checkout.  In cone-mode, the user specifies
@@ -92,7 +96,8 @@ vivifying: When a command restores a tracked file to the working tree (and
 	file), this is referred to as "vivifying" the file.
 
 
-=== Purpose of sparse-checkouts ===
+Purpose of sparse-checkouts
+---------------------------
 
 sparse-checkouts exist to allow users to work with a subset of their
 files.
@@ -255,7 +260,8 @@ will perceive the checkout as dense, and commands should thus behave as if
 all files are present.
 
 
-=== Usecases of primary concern ===
+Usecases of primary concern
+---------------------------
 
 Most of the rest of this document will focus on Behavior A and Behavior
 B.  Some notes about the other two cases and why we are not focusing on
@@ -300,7 +306,8 @@ Behavior C do not assume they are part of the Behavior B camp and propose
 patches that break things for the real Behavior B folks.
 
 
-=== Oversimplified mental models ===
+Oversimplified mental models
+----------------------------
 
 An oversimplification of the differences in the above behaviors is:
 
@@ -313,7 +320,8 @@ An oversimplification of the differences in the above behaviors is:
 	      they can later lazily be populated instead.
 
 
-=== Desired behavior ===
+Desired behavior
+----------------
 
 As noted previously, despite the simple idea of just working with a subset
 of files, there are a range of different behavioral changes that need to be
@@ -544,7 +552,8 @@ understanding these differences can be beneficial.
   * gitk?
 
 
-=== Behavior classes ===
+Behavior classes
+----------------
 
 From the above there are a few classes of behavior:
 
@@ -611,7 +620,8 @@ From the above there are a few classes of behavior:
     specification.
 
 
-=== Subcommand-dependent defaults ===
+Subcommand-dependent defaults
+-----------------------------
 
 Note that we have different defaults depending on the command for the
 desired behavior :
@@ -751,7 +761,8 @@ desired behavior :
     implemented.
 
 
-=== Sparse specification vs. sparsity patterns ===
+Sparse specification vs. sparsity patterns
+------------------------------------------
 
 In a well-behaved situation, the sparse specification is given directly
 by the $GIT_DIR/info/sparse-checkout file.  However, it can transiently
@@ -823,7 +834,8 @@ under behavior B index operations are lumped with history and tend to
 operate full-tree.
 
 
-=== Implementation Questions ===
+Implementation Questions
+------------------------
 
   * Do the options --scope={sparse,all} sound good to others?  Are there better
     options?
@@ -894,7 +906,8 @@ operate full-tree.
     is seamless for them.
 
 
-=== Implementation Goals/Plans ===
+Implementation Goals/Plans
+--------------------------
 
  * Get buy-in on this document in general.
 
@@ -922,13 +935,14 @@ operate full-tree.
      commands.  IMPORTANT: make sure diff machinery changes don't mess with
      format-patch, fast-export, etc.
 
-=== Known bugs ===
+Known bugs
+----------
 
 This list used to be a lot longer (see e.g. [1,2,3,4,5,6,7,8,9]), but we've
 been working on it.
 
-0. Behavior A is not well supported in Git.  (Behavior B didn't used to
-   be either, but was the easier of the two to implement.)
+Behavior A is not well supported in Git.  (Behavior B didn't used to
+be either, but was the easier of the two to implement.)
 
 1. am and apply:
 
@@ -1052,7 +1066,8 @@ been working on it.
     https://lore.kernel.org/git/CABPp-BEkJQoKZsQGCYioyga_uoDQ6iBeW+FKr8JhyuuTMK1RDw@mail.gmail.com/
 
 
-=== Reference Emails ===
+Reference Emails
+----------------
 
 Emails that detail various bugs we've had in sparse-checkout:
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
  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-19 17:34   ` ZheNing Hu via GitGitGadget
  2023-01-20  5:30     ` Elijah Newren
  1 sibling, 1 reply; 21+ messages in thread
From: ZheNing Hu via GitGitGadget @ 2023-01-19 17:34 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye, Elijah Newren,
	Nguyễn Thái Ngọc Duy, ZheNing Hu, ZheNing Hu

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

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.
 
 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" : "");
 	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
+'
+
 test_done
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/2] docs: fix sparse-checkout docs link
  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:15       ` ZheNing Hu
  0 siblings, 2 replies; 21+ messages in thread
From: Elijah Newren @ 2023-01-20  5:12 UTC (permalink / raw)
  To: ZheNing Hu via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy, ZheNing Hu

On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: ZheNing Hu <adlternative@gmail.com>
>
> There is a linking error when other documents want to refer to
> technical/sparse-checkout.txt, Also, there is something wrong
> with the format of the paragraphs in sparse-checkout.txt, which
> makes acsiidoc compile errors.
>
> So fix the format of sparse-checkout.txt, and link it in the
> Makefile to correct that.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>  Documentation/Makefile                      |  1 +
>  Documentation/technical/sparse-checkout.txt | 43 ++++++++++++++-------
>  2 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 9c67c3a1c50..7540da27b8c 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -121,6 +121,7 @@ TECH_DOCS += technical/reftable
>  TECH_DOCS += technical/scalar
>  TECH_DOCS += technical/send-pack-pipeline
>  TECH_DOCS += technical/shallow
> +TECH_DOCS += technical/sparse-checkout
>  TECH_DOCS += technical/trivial-merge
>  SP_ARTICLES += $(TECH_DOCS)
>  SP_ARTICLES += technical/api-index
> diff --git a/Documentation/technical/sparse-checkout.txt b/Documentation/technical/sparse-checkout.txt
> index fa0d01cbda4..52e86764a6c 100644
> --- a/Documentation/technical/sparse-checkout.txt
> +++ b/Documentation/technical/sparse-checkout.txt
> @@ -1,3 +1,6 @@
> +Sparse Checkout Design Notes
> +============================
> +
>  Table of contents:
>
>    * Terminology
> @@ -14,7 +17,8 @@ Table of contents:
>    * Reference Emails
>
>
> -=== Terminology ===
> +Terminology
> +-----------
>
>  cone mode: one of two modes for specifying the desired subset of files
>         in a sparse-checkout.  In cone-mode, the user specifies
> @@ -92,7 +96,8 @@ vivifying: When a command restores a tracked file to the working tree (and
>         file), this is referred to as "vivifying" the file.
>
>
> -=== Purpose of sparse-checkouts ===
> +Purpose of sparse-checkouts
> +---------------------------
>
>  sparse-checkouts exist to allow users to work with a subset of their
>  files.
> @@ -255,7 +260,8 @@ will perceive the checkout as dense, and commands should thus behave as if
>  all files are present.
>
>
> -=== Usecases of primary concern ===
> +Usecases of primary concern
> +---------------------------
>
>  Most of the rest of this document will focus on Behavior A and Behavior
>  B.  Some notes about the other two cases and why we are not focusing on
> @@ -300,7 +306,8 @@ Behavior C do not assume they are part of the Behavior B camp and propose
>  patches that break things for the real Behavior B folks.
>
>
> -=== Oversimplified mental models ===
> +Oversimplified mental models
> +----------------------------
>
>  An oversimplification of the differences in the above behaviors is:
>
> @@ -313,7 +320,8 @@ An oversimplification of the differences in the above behaviors is:
>               they can later lazily be populated instead.
>
>
> -=== Desired behavior ===
> +Desired behavior
> +----------------
>
>  As noted previously, despite the simple idea of just working with a subset
>  of files, there are a range of different behavioral changes that need to be
> @@ -544,7 +552,8 @@ understanding these differences can be beneficial.
>    * gitk?
>
>
> -=== Behavior classes ===
> +Behavior classes
> +----------------
>
>  From the above there are a few classes of behavior:
>
> @@ -611,7 +620,8 @@ From the above there are a few classes of behavior:
>      specification.
>
>
> -=== Subcommand-dependent defaults ===
> +Subcommand-dependent defaults
> +-----------------------------
>
>  Note that we have different defaults depending on the command for the
>  desired behavior :
> @@ -751,7 +761,8 @@ desired behavior :
>      implemented.
>
>
> -=== Sparse specification vs. sparsity patterns ===
> +Sparse specification vs. sparsity patterns
> +------------------------------------------
>
>  In a well-behaved situation, the sparse specification is given directly
>  by the $GIT_DIR/info/sparse-checkout file.  However, it can transiently
> @@ -823,7 +834,8 @@ under behavior B index operations are lumped with history and tend to
>  operate full-tree.
>
>
> -=== Implementation Questions ===
> +Implementation Questions
> +------------------------
>
>    * Do the options --scope={sparse,all} sound good to others?  Are there better
>      options?
> @@ -894,7 +906,8 @@ operate full-tree.
>      is seamless for them.
>
>
> -=== Implementation Goals/Plans ===
> +Implementation Goals/Plans
> +--------------------------
>
>   * Get buy-in on this document in general.
>
> @@ -922,13 +935,14 @@ operate full-tree.
>       commands.  IMPORTANT: make sure diff machinery changes don't mess with
>       format-patch, fast-export, etc.
>
> -=== Known bugs ===
> +Known bugs
> +----------

Everything so far makes sense.

>  This list used to be a lot longer (see e.g. [1,2,3,4,5,6,7,8,9]), but we've
>  been working on it.
>
> -0. Behavior A is not well supported in Git.  (Behavior B didn't used to
> -   be either, but was the easier of the two to implement.)
> +Behavior A is not well supported in Git.  (Behavior B didn't used to
> +be either, but was the easier of the two to implement.)

Why did you remove the numbering on this, though?  If asciidoc doesn't
like starting with 0 (the only guess I can think of for why you'd
change this), shouldn't the series be renumbered starting at 1 rather
than removing this from the list?

>  1. am and apply:
>
> @@ -1052,7 +1066,8 @@ been working on it.
>      https://lore.kernel.org/git/CABPp-BEkJQoKZsQGCYioyga_uoDQ6iBeW+FKr8JhyuuTMK1RDw@mail.gmail.com/
>
>
> -=== Reference Emails ===
> +Reference Emails
> +----------------
>
>  Emails that detail various bugs we've had in sparse-checkout:
>
> --
> gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
  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:33       ` ZheNing Hu
  0 siblings, 2 replies; 21+ messages in thread
From: Elijah Newren @ 2023-01-20  5:30 UTC (permalink / raw)
  To: ZheNing Hu via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy, ZheNing Hu

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/2] docs: fix sparse-checkout docs link
  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
  1 sibling, 1 reply; 21+ messages in thread
From: Martin Ågren @ 2023-01-20  9:35 UTC (permalink / raw)
  To: Elijah Newren
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy, ZheNing Hu

On Fri, 20 Jan 2023 at 06:29, Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> > So fix the format of sparse-checkout.txt, and link it in the
> > Makefile to correct that.

> > -0. Behavior A is not well supported in Git.  (Behavior B didn't used to
> > -   be either, but was the easier of the two to implement.)
> > +Behavior A is not well supported in Git.  (Behavior B didn't used to
> > +be either, but was the easier of the two to implement.)
>
> Why did you remove the numbering on this, though?  If asciidoc doesn't
> like starting with 0 (the only guess I can think of for why you'd
> change this), shouldn't the series be renumbered starting at 1 rather
> than removing this from the list?

It looks like the zero causes both asciidoc and Asciidoctor to emit
warnings (one per item, since each item's number is off by one). They
also helpfully relabel everything starting at 1.

I agree that there's a better fix here than dropping the 0. Either
renumber everything or, probably better, just use "." for each item
rather than "1.", "2." and so on. The right numbers will be inserted
automatically. This also means that if an item is ever added earlier in
the list, we won't need to update all the numbers below that point.

(The numbers being generated automatically means we can't refer to them
("see item 2") without potentially getting out of sync, but that is true
regardless if the numbers are corrected for us, as now, or if we just
use ".".)

The contents of these list items could be prettified in various ways,
but I'm not sure what the status and goal is for these technical/
documents. Avoiding warnings in the build process, as ZheNing aimed for,
seems like a good idea regardless.

Martin

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
  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 15:33       ` ZheNing Hu
  1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2023-01-20 16:34 UTC (permalink / raw)
  To: Elijah Newren
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Derrick Stolee,
	Victoria Dye, Nguyễn Thái Ngọc Duy, ZheNing Hu

Elijah Newren <newren@gmail.com> writes:

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

Perhaps it becomes useful in conjunction with %(if) and friends,
when they become avaiable?

Until then, I agree that the output format looks pretty klunky.
The calling scripts still can do

	--format='%(path) A=%(A) B=%(B) C=%(C)'

and take an empty value as false, though.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/2] docs: fix sparse-checkout docs link
  2023-01-20  5:12     ` Elijah Newren
  2023-01-20  9:35       ` Martin Ågren
@ 2023-01-23 15:15       ` ZheNing Hu
  1 sibling, 0 replies; 21+ messages in thread
From: ZheNing Hu @ 2023-01-23 15:15 UTC (permalink / raw)
  To: Elijah Newren
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy

Elijah Newren <newren@gmail.com> 于2023年1月20日周五 13:12写道:
>
> >  This list used to be a lot longer (see e.g. [1,2,3,4,5,6,7,8,9]), but we've
> >  been working on it.
> >
> > -0. Behavior A is not well supported in Git.  (Behavior B didn't used to
> > -   be either, but was the easier of the two to implement.)
> > +Behavior A is not well supported in Git.  (Behavior B didn't used to
> > +be either, but was the easier of the two to implement.)
>
> Why did you remove the numbering on this, though?  If asciidoc doesn't
> like starting with 0 (the only guess I can think of for why you'd
> change this), shouldn't the series be renumbered starting at 1 rather
> than removing this from the list?
>

I see, I admitted that I treated "0." as some overview information and it
should also be considered as one item of the "Known bugs".

> >  1. am and apply:
> >
> > @@ -1052,7 +1066,8 @@ been working on it.
> >      https://lore.kernel.org/git/CABPp-BEkJQoKZsQGCYioyga_uoDQ6iBeW+FKr8JhyuuTMK1RDw@mail.gmail.com/
> >
> >
> > -=== Reference Emails ===
> > +Reference Emails
> > +----------------
> >
> >  Emails that detail various bugs we've had in sparse-checkout:
> >
> > --
> > gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/2] docs: fix sparse-checkout docs link
  2023-01-20  9:35       ` Martin Ågren
@ 2023-01-23 15:16         ` ZheNing Hu
  0 siblings, 0 replies; 21+ messages in thread
From: ZheNing Hu @ 2023-01-23 15:16 UTC (permalink / raw)
  To: Martin Ågren
  Cc: Elijah Newren, ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy

Martin Ågren <martin.agren@gmail.com> 于2023年1月20日周五 17:35写道:
>
> On Fri, 20 Jan 2023 at 06:29, Elijah Newren <newren@gmail.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > From: ZheNing Hu <adlternative@gmail.com>
> > > So fix the format of sparse-checkout.txt, and link it in the
> > > Makefile to correct that.
>
> > > -0. Behavior A is not well supported in Git.  (Behavior B didn't used to
> > > -   be either, but was the easier of the two to implement.)
> > > +Behavior A is not well supported in Git.  (Behavior B didn't used to
> > > +be either, but was the easier of the two to implement.)
> >
> > Why did you remove the numbering on this, though?  If asciidoc doesn't
> > like starting with 0 (the only guess I can think of for why you'd
> > change this), shouldn't the series be renumbered starting at 1 rather
> > than removing this from the list?
>
> It looks like the zero causes both asciidoc and Asciidoctor to emit
> warnings (one per item, since each item's number is off by one). They
> also helpfully relabel everything starting at 1.
>
> I agree that there's a better fix here than dropping the 0. Either
> renumber everything or, probably better, just use "." for each item
> rather than "1.", "2." and so on. The right numbers will be inserted
> automatically. This also means that if an item is ever added earlier in
> the list, we won't need to update all the numbers below that point.
>

Good idea.

> (The numbers being generated automatically means we can't refer to them
> ("see item 2") without potentially getting out of sync, but that is true
> regardless if the numbers are corrected for us, as now, or if we just
> use ".".)
>

That shouldn't matter, there are no references to any of these items.

> The contents of these list items could be prettified in various ways,
> but I'm not sure what the status and goal is for these technical/
> documents. Avoiding warnings in the build process, as ZheNing aimed for,
> seems like a good idea regardless.
>
> Martin

Thanks,
--
ZheNing Hu

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
  2023-01-20  5:30     ` Elijah Newren
  2023-01-20 16:34       ` Junio C Hamano
@ 2023-01-23 15:33       ` ZheNing Hu
  1 sibling, 0 replies; 21+ messages in thread
From: ZheNing Hu @ 2023-01-23 15:33 UTC (permalink / raw)
  To: Elijah Newren
  Cc: ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Junio C Hamano,
	Derrick Stolee, Victoria Dye,
	Nguyễn Thái Ngọc Duy

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
  2023-01-20 16:34       ` Junio C Hamano
@ 2023-01-23 15:35         ` ZheNing Hu
  2023-01-23 22:39           ` Junio C Hamano
  0 siblings, 1 reply; 21+ messages in thread
From: ZheNing Hu @ 2023-01-23 15:35 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Elijah Newren, ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Derrick Stolee,
	Victoria Dye, Nguyễn Thái Ngọc Duy

Junio C Hamano <gitster@pobox.com> 于2023年1月21日周六 00:34写道:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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.
>
> Perhaps it becomes useful in conjunction with %(if) and friends,
> when they become avaiable?
>
> Until then, I agree that the output format looks pretty klunky.
> The calling scripts still can do
>
>         --format='%(path) A=%(A) B=%(B) C=%(C)'
>
> and take an empty value as false, though.

Can this strange design be considered as a bad design of %(if) and
%(else) in ref-filter?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
  2023-01-23 15:35         ` ZheNing Hu
@ 2023-01-23 22:39           ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-01-23 22:39 UTC (permalink / raw)
  To: ZheNing Hu
  Cc: Elijah Newren, ZheNing Hu via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Derrick Stolee,
	Victoria Dye, Nguyễn Thái Ngọc Duy

ZheNing Hu <adlternative@gmail.com> writes:

>> Perhaps it becomes useful in conjunction with %(if) and friends,
>> when they become avaiable?
>> ...
> Can this strange design be considered as a bad design of %(if) and
> %(else) in ref-filter?

Sorry, but I am not sure what "strange design" you are referring to.

On the ref-filter side, thanks to these conditional formatting
primitives, a boolean %(token) that gives an empty string vs a
non-empty string does make sense for a good design.  If another
--format="" wants to imitate it, it would be a good idea to first
see what part of the implementation can be shared and reused from
there.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2023-01-23 22:39 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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