{"thread":{"id":"57442","subject":"[PATCH] ls-files: support --recurse-submodules --stage","startedAt":"2022-02-18T22:32:19Z","lastAt":"2022-02-24T00:23:13Z","messageCount":10,"participants":["Jonathan Tan","Junio C Hamano","Ævar Arnfjörð Bjarmason","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"448813","messageId":"20220218223212.1139366-1-jonathantanmy@google.com","threadId":"57442","inReplyTo":null,"subject":"[PATCH] ls-files: support --recurse-submodules --stage","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-18T22:32:11Z","receivedAt":"2022-02-18T22:32:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"e77aa336f1 (\"ls-files: optionally recurse into submodules\", 2016-10-10)\ntaught ls-files the --recurse-submodules argument, but only in a limited\nset of circumstances. In particular, --stage was unsupported, perhaps\nbecause there was no repo_find_unique_abbrev(), which was only\nintroduced in 8bb95572b0 (\"sha1-name.c: add\nrepo_find_unique_abbrev_r()\", 2019-04-16). This function is needed for\nusing --recurse-submodules with --stage.\n\nNow that we have repo_find_unique_abbrev(), teach support for this\ncombination of arguments.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nI got the similar-hashing object contents from Ævar's work in [1].\n\n[1] https://lore.kernel.org/git/patch-v7-1.6-28c01b7f8a5-20220111T130811Z-avarab@gmail.com/\n---\n Documentation/git-ls-files.txt         |  2 +-\n builtin/ls-files.c                     |  4 ++--\n t/t3007-ls-files-recurse-submodules.sh | 20 +++++++++++++++++++-\n 3 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 48cc7c0b6f..0dabf3f0dd 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -156,7 +156,7 @@ a space) at the start of each line:\n \n --recurse-submodules::\n \tRecursively calls ls-files on each active submodule in the repository.\n-\tCurrently there is only support for the --cached mode.\n+\tCurrently there is only support for the --cached and --stage modes.\n \n --abbrev[=<n>]::\n \tInstead of showing the full 40-byte hexadecimal object\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex f7ea56cc63..e791b65e7e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -244,7 +244,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,\n \t\t\tprintf(\"%s%06o %s %d\\t\",\n \t\t\t       tag,\n \t\t\t       ce->ce_mode,\n-\t\t\t       find_unique_abbrev(&ce->oid, abbrev),\n+\t\t\t       repo_find_unique_abbrev(repo, &ce->oid, abbrev),\n \t\t\t       ce_stage(ce));\n \t\t}\n \t\twrite_eolinfo(repo->index, ce, fullname);\n@@ -726,7 +726,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tsetup_work_tree();\n \n \tif (recurse_submodules &&\n-\t    (show_stage || show_deleted || show_others || show_unmerged ||\n+\t    (show_deleted || show_others || show_unmerged ||\n \t     show_killed || show_modified || show_resolve_undo || with_tree))\n \t\tdie(\"ls-files --recurse-submodules unsupported mode\");\n \ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex 4a08000713..3d2da360d1 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -34,6 +34,25 @@ test_expect_success 'ls-files correctly outputs files in submodule' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--stage' '\n+\t# In order to test hash abbreviation, write two objects that have the\n+\t# same first 4 hexadecimal characters in their (SHA-1) hashes.\n+\techo brocdnra >submodule/c &&\n+\tgit -C submodule commit -am \"update c\" &&\n+\techo brigddsv >submodule/c &&\n+\tgit -C submodule commit -am \"update c again\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t100644 6da7 0\t.gitmodules\n+\t100644 7898 0\ta\n+\t100644 6178 0\tb/b\n+\t100644 dead9 0\tsubmodule/c\n+\tEOF\n+\n+\tgit ls-files --stage --recurse-submodules --abbrev=4 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'ls-files correctly outputs files in submodule with -z' '\n \tlf_to_nul >expect <<-\\EOF &&\n \t.gitmodules\n@@ -292,7 +311,6 @@ test_incompatible_with_recurse_submodules () {\n test_incompatible_with_recurse_submodules --deleted\n test_incompatible_with_recurse_submodules --modified\n test_incompatible_with_recurse_submodules --others\n-test_incompatible_with_recurse_submodules --stage\n test_incompatible_with_recurse_submodules --killed\n test_incompatible_with_recurse_submodules --unmerged\n \n-- \n2.35.1.473.g83b2b277ed-goog\n\n"},{"id":"448820","messageId":"xmqqzgmnwv9e.fsf@gitster.g","threadId":"57442","inReplyTo":"20220218223212.1139366-1-jonathantanmy@google.com","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-19T00:33:01Z","receivedAt":"2022-02-19T00:33:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index f7ea56cc63..e791b65e7e 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -244,7 +244,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,\n>  \t\t\tprintf(\"%s%06o %s %d\\t\",\n>  \t\t\t       tag,\n>  \t\t\t       ce->ce_mode,\n> -\t\t\t       find_unique_abbrev(&ce->oid, abbrev),\n> +\t\t\t       repo_find_unique_abbrev(repo, &ce->oid, abbrev),\n>  \t\t\t       ce_stage(ce));\n>  \t\t}\n\nQuite straight-forward.  At this point, repo is the repository we\nare currently working in (which will be updated to the submodule\nrepository by show_submodule() and passed down to show_files()), so\nthe only thing we need to do is to make sure we use that repo\nconsistently.  Makes sense.\n\n>  \t\twrite_eolinfo(repo->index, ce, fullname);\n> @@ -726,7 +726,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n>  \t\tsetup_work_tree();\n>  \n>  \tif (recurse_submodules &&\n> -\t    (show_stage || show_deleted || show_others || show_unmerged ||\n> +\t    (show_deleted || show_others || show_unmerged ||\n>  \t     show_killed || show_modified || show_resolve_undo || with_tree))\n>  \t\tdie(\"ls-files --recurse-submodules unsupported mode\");\n>  \n> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n> index 4a08000713..3d2da360d1 100755\n> --- a/t/t3007-ls-files-recurse-submodules.sh\n> +++ b/t/t3007-ls-files-recurse-submodules.sh\n> @@ -34,6 +34,25 @@ test_expect_success 'ls-files correctly outputs files in submodule' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success '--stage' '\n> +\t# In order to test hash abbreviation, write two objects that have the\n> +\t# same first 4 hexadecimal characters in their (SHA-1) hashes.\n> +\techo brocdnra >submodule/c &&\n> +\tgit -C submodule commit -am \"update c\" &&\n> +\techo brigddsv >submodule/c &&\n> +\tgit -C submodule commit -am \"update c again\" &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t100644 6da7 0\t.gitmodules\n> +\t100644 7898 0\ta\n> +\t100644 6178 0\tb/b\n> +\t100644 dead9 0\tsubmodule/c\n> +\tEOF\n> +\n> +\tgit ls-files --stage --recurse-submodules --abbrev=4 >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'ls-files correctly outputs files in submodule with -z' '\n>  \tlf_to_nul >expect <<-\\EOF &&\n>  \t.gitmodules\n> @@ -292,7 +311,6 @@ test_incompatible_with_recurse_submodules () {\n>  test_incompatible_with_recurse_submodules --deleted\n>  test_incompatible_with_recurse_submodules --modified\n>  test_incompatible_with_recurse_submodules --others\n> -test_incompatible_with_recurse_submodules --stage\n>  test_incompatible_with_recurse_submodules --killed\n>  test_incompatible_with_recurse_submodules --unmerged\n"},{"id":"448830","messageId":"220219.868ru7fsad.gmgdl@evledraar.gmail.com","threadId":"57442","inReplyTo":"20220218223212.1139366-1-jonathantanmy@google.com","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T03:11:30Z","receivedAt":"2022-02-19T03:29:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Jonathan Tan wrote:\n\n> e77aa336f1 (\"ls-files: optionally recurse into submodules\", 2016-10-10)\n> taught ls-files the --recurse-submodules argument, but only in a limited\n> set of circumstances. In particular, --stage was unsupported, perhaps\n> because there was no repo_find_unique_abbrev(), which was only\n> introduced in 8bb95572b0 (\"sha1-name.c: add\n> repo_find_unique_abbrev_r()\", 2019-04-16). This function is needed for\n> using --recurse-submodules with --stage.\n>\n> Now that we have repo_find_unique_abbrev(), teach support for this\n> combination of arguments.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> I got the similar-hashing object contents from Ævar's work in [1].\n\nHah! FWIW that was made by this script I hacked up at the time:\n\t\n\t#!/usr/bin/env perl\n\tuse v5.32.0;\n\tuse strict;\n\tuse warnings;\n\tuse Digest::SHA qw(sha1_hex sha256_hex);\n\t\n\t# Usage:\n\t## prefix= type=bad git find-colliding-hashes | tee garbage-coll-bad.txt\n\t## prefix= type=bad want=bad git find-colliding-hashes | tee garbage-coll-bad.txt\n\t\n\t$| = 1;\n\tmy $s = $ENV{s} // \"s\";\n\tmy %seen;\n\tmy $type = $ENV{type} // \"blob\";\n\tmy $prefix = $ENV{prefix} // \"\";\n\tmy $want = $ENV{want} // \"\";\n\twhile ($s++) {\n\t\tmy $str = $prefix . $s;\n\t\tmy $l = length($str) + 1;\n\t\tmy $p = \"$type $l\\0$str\\n\";\n\t\tmy $o = sha1_hex($p);\n\t\tnext if length $want && index($o, $want) != 0;\n\t\tmy $n = sha256_hex($p);\n\t\tmy $os = substr($o, 0, 4);\n\t\tmy $ns = substr($n, 0, 4);\n\t\tif ($os eq $ns) {\n\t\t\tsay \"hash($str) = [$os, $ns]\" . ($seen{$os} ? \" SEEN\" : \"\");\n\t\t\t$seen{$os} = 1;\n\t\t}\n\t}\n\nhttps://gist.github.com/avar/9e4c2bde7fbdc888b031713065a9eaf6 has some\nmore colliding blob prefixes, which I generated until I got bored with\nit...\n\n> +test_expect_success '--stage' '\n> +\t# In order to test hash abbreviation, write two objects that have the\n> +\t# same first 4 hexadecimal characters in their (SHA-1) hashes.\n> +\techo brocdnra >submodule/c &&\n> +\tgit -C submodule commit -am \"update c\" &&\n> +\techo brigddsv >submodule/c &&\n> +\tgit -C submodule commit -am \"update c again\" &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t100644 6da7 0\t.gitmodules\n> +\t100644 7898 0\ta\n> +\t100644 6178 0\tb/b\n> +\t100644 dead9 0\tsubmodule/c\n> +\tEOF\n\nThis test though will break, as you can see with:\n\n    GIT_TEST_DEFAULT_HASH=sha256 ./t3007-ls-files-recurse-submodules.sh\n\nSo you'll need at least something like:\n\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex 3d2da360d17..0fe69da8dcf 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -42,10 +42,10 @@ test_expect_success '--stage' '\n \techo brigddsv >submodule/c &&\n \tgit -C submodule commit -am \"update c again\" &&\n \n-\tcat >expect <<-\\EOF &&\n-\t100644 6da7 0\t.gitmodules\n-\t100644 7898 0\ta\n-\t100644 6178 0\tb/b\n+\tcat >expect <<-EOF &&\n+\t100644 $(git rev-parse --short=4 HEAD:.gitmodules) 0\t.gitmodules\n+\t100644 $(git rev-parse --short=4 HEAD:a) 0\ta\n+\t100644 $(git rev-parse --short=4 HEAD:b/b) 0\tb/b\n \t100644 dead9 0\tsubmodule/c\n \tEOF\n \nBut then the problem is that one is dead9 and the other dead6, I was\njust trying to find 4-char prefixes.\n\nBut having indulged in all that, I'm now entirely confused about why any\nof this needs to be tested here.\n\nYou're adding --stage, which will give us --stage-y output, and it was\npreviously incompatible with --recurse-submodules. Having the two\ncombine is good!\n\nBut why do we need to test the OID abbreviation at all, isn't that a bit\ntoo much paranoia? Isn't it sufficient just do:\n\n    opts=\"--stage --abbrev=4\" &&\n    git -C submodule ls-files $opts >expect &&\n    git ls-files --recurse-submodules $opts --stage >raw &&\n    grep submodule raw >actual &&\n    test_cmp expect actual\n\nOr well, then the path won't be the same, but I think you get the\nidea.\n\nI.e. don't we just want to test that the submodule is indeed included\nhere, not that some particular feature works in combination with it.\n\nSupposing that repo_find_unique_abbrev() won't work might be a bit too\nmuch paranoia, and I'm more test-happy than most :)\n\nI'd think that if we should test anything it would be more meaningful to\ne.g. test the sort order of the returned entries.\n\nYour test case won't disambiguate between index entries being returned\nin sort order v.s. just \"submodules at the end\". Since \"s\" sorts after\n0, a and b.\n\nPresumably it does the former, but I'd think distinguishing those would\nbe one meaningful test of actual --recurse-submodules --stage\nfunctionality.\n"},{"id":"448831","messageId":"YhBo+hg8yxIOGKNS@nand.local","threadId":"57442","inReplyTo":"220219.868ru7fsad.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-19T03:50:18Z","receivedAt":"2022-02-19T03:50:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 19, 2022 at 04:11:30AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> But why do we need to test the OID abbreviation at all, isn't that a bit\n> too much paranoia? Isn't it sufficient just do:\n>\n>     opts=\"--stage --abbrev=4\" &&\n>     git -C submodule ls-files $opts >expect &&\n>     git ls-files --recurse-submodules $opts --stage >raw &&\n>     grep submodule raw >actual &&\n>     test_cmp expect actual\n\nYeah; I like this direction. I try to err on the side of reconstructing\nthe whole output and then calling test_cmp on it. I usually go that way\nbecause it's nice for somebody reading the test script to see what the\noutput is supposed to look like.\n\nBut because you have to have a bunch of $(git rev-parse --short=4 ...)\nin subshells everywhere in order to produce the right output, I don't\nthink the result resembles the actual output in this case.\n\nI'd probably go a bit further than what you propose, maybe replacing\nthat grep with:\n\n    grep \"0.submodule\" actual\n\nto make sure that we got the correct stage number, too. But I agree with\nyou on the general direction.\n\nThanks,\nTaylor\n"},{"id":"448937","messageId":"xmqq35kdugzt.fsf@gitster.g","threadId":"57442","inReplyTo":"20220218223212.1139366-1-jonathantanmy@google.com","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-21T01:48:38Z","receivedAt":"2022-02-21T01:48:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> +test_expect_success '--stage' '\n> +\t# In order to test hash abbreviation, write two objects that have the\n> +\t# same first 4 hexadecimal characters in their (SHA-1) hashes.\n\nWhat about linux-sha256 jobs?\ne.g. https://github.com/git/git/runs/5267681569?check_suite_focus=true\n\n> +\techo brocdnra >submodule/c &&\n> +\tgit -C submodule commit -am \"update c\" &&\n> +\techo brigddsv >submodule/c &&\n> +\tgit -C submodule commit -am \"update c again\" &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t100644 6da7 0\t.gitmodules\n> +\t100644 7898 0\ta\n> +\t100644 6178 0\tb/b\n> +\t100644 dead9 0\tsubmodule/c\n> +\tEOF\n> +\n> +\tgit ls-files --stage --recurse-submodules --abbrev=4 >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'ls-files correctly outputs files in submodule with -z' '\n>  \tlf_to_nul >expect <<-\\EOF &&\n>  \t.gitmodules\n> @@ -292,7 +311,6 @@ test_incompatible_with_recurse_submodules () {\n>  test_incompatible_with_recurse_submodules --deleted\n>  test_incompatible_with_recurse_submodules --modified\n>  test_incompatible_with_recurse_submodules --others\n> -test_incompatible_with_recurse_submodules --stage\n>  test_incompatible_with_recurse_submodules --killed\n>  test_incompatible_with_recurse_submodules --unmerged\n"},{"id":"448945","messageId":"YhL8rmsnnLZhMTJL@nand.local","threadId":"57442","inReplyTo":"xmqq35kdugzt.fsf@gitster.g","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T02:45:02Z","receivedAt":"2022-02-21T02:45:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Feb 20, 2022 at 05:48:38PM -0800, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n> > +test_expect_success '--stage' '\n> > +\t# In order to test hash abbreviation, write two objects that have the\n> > +\t# same first 4 hexadecimal characters in their (SHA-1) hashes.\n>\n> What about linux-sha256 jobs?\n> e.g. https://github.com/git/git/runs/5267681569?check_suite_focus=true\n\nYeah; this is indeed broken. See the discussion beginning at [1] for\nsome options.\n\n[1]: https://lore.kernel.org/git/220219.868ru7fsad.gmgdl@evledraar.gmail.com/\n\nThanks,\nTaylor\n"},{"id":"449027","messageId":"xmqqk0dot74j.fsf@gitster.g","threadId":"57442","inReplyTo":"220219.868ru7fsad.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-21T18:19:24Z","receivedAt":"2022-02-21T18:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> This test though will break, as you can see with:\n>\n>     GIT_TEST_DEFAULT_HASH=sha256 ./t3007-ls-files-recurse-submodules.sh\n>\n> So you'll need at least something like:\n>\n> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n> index 3d2da360d17..0fe69da8dcf 100755\n> --- a/t/t3007-ls-files-recurse-submodules.sh\n> +++ b/t/t3007-ls-files-recurse-submodules.sh\n> @@ -42,10 +42,10 @@ test_expect_success '--stage' '\n>  \techo brigddsv >submodule/c &&\n>  \tgit -C submodule commit -am \"update c again\" &&\n>  \n> -\tcat >expect <<-\\EOF &&\n> -\t100644 6da7 0\t.gitmodules\n> -\t100644 7898 0\ta\n> -\t100644 6178 0\tb/b\n> +\tcat >expect <<-EOF &&\n> +\t100644 $(git rev-parse --short=4 HEAD:.gitmodules) 0\t.gitmodules\n> +\t100644 $(git rev-parse --short=4 HEAD:a) 0\ta\n> +\t100644 $(git rev-parse --short=4 HEAD:b/b) 0\tb/b\n>  \t100644 dead9 0\tsubmodule/c\n>  \tEOF\n>  \n> But then the problem is that one is dead9 and the other dead6, I was\n> just trying to find 4-char prefixes.\n>\n> But having indulged in all that, I'm now entirely confused about why any\n> of this needs to be tested here.\n>\n> You're adding --stage, which will give us --stage-y output, and it was\n> previously incompatible with --recurse-submodules. Having the two\n> combine is good!\n\nI think what this is trying to make sure is that it (1) enabled the\ncombination and (2) uses the object store of the submodule when\ndisambiguating names of the objects from the submodule, because the\nauthor suspects that the reason why these two options were made\nincompatible in the first place was because long time ago there\nwasn't a way to ask \"here is an object name---please uniquify in the\ncontext of _that_ repository\".  So it is understandable to prepare\nan object X in a submodule and another object Y in the superproject,\nsuch that the abbreviated name of X in the context of the submodule\nis different from the abbreviated name of X in the context of the\nsuperproject (i.e. if X were in the superproject's object store,\nbecause the object names of X and Y share the prefix, it may require\nlonger prefix to disambiguate from Y), and make sure that the uniquify\nis indeed happening in the context of the submodule.\n\nSo, you are only concentrating on (1) but forgetting why the author\nwants (2).\n"},{"id":"449036","messageId":"220221.86ley4aw88.gmgdl@evledraar.gmail.com","threadId":"57442","inReplyTo":"xmqqk0dot74j.fsf@gitster.g","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T18:51:47Z","receivedAt":"2022-02-21T18:54:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> This test though will break, as you can see with:\n>>\n>>     GIT_TEST_DEFAULT_HASH=sha256 ./t3007-ls-files-recurse-submodules.sh\n>>\n>> So you'll need at least something like:\n>>\n>> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n>> index 3d2da360d17..0fe69da8dcf 100755\n>> --- a/t/t3007-ls-files-recurse-submodules.sh\n>> +++ b/t/t3007-ls-files-recurse-submodules.sh\n>> @@ -42,10 +42,10 @@ test_expect_success '--stage' '\n>>  \techo brigddsv >submodule/c &&\n>>  \tgit -C submodule commit -am \"update c again\" &&\n>>  \n>> -\tcat >expect <<-\\EOF &&\n>> -\t100644 6da7 0\t.gitmodules\n>> -\t100644 7898 0\ta\n>> -\t100644 6178 0\tb/b\n>> +\tcat >expect <<-EOF &&\n>> +\t100644 $(git rev-parse --short=4 HEAD:.gitmodules) 0\t.gitmodules\n>> +\t100644 $(git rev-parse --short=4 HEAD:a) 0\ta\n>> +\t100644 $(git rev-parse --short=4 HEAD:b/b) 0\tb/b\n>>  \t100644 dead9 0\tsubmodule/c\n>>  \tEOF\n>>  \n>> But then the problem is that one is dead9 and the other dead6, I was\n>> just trying to find 4-char prefixes.\n>>\n>> But having indulged in all that, I'm now entirely confused about why any\n>> of this needs to be tested here.\n>>\n>> You're adding --stage, which will give us --stage-y output, and it was\n>> previously incompatible with --recurse-submodules. Having the two\n>> combine is good!\n>\n> I think what this is trying to make sure is that it (1) enabled the\n> combination and (2) uses the object store of the submodule when\n> disambiguating names of the objects from the submodule, because the\n> author suspects that the reason why these two options were made\n> incompatible in the first place was because long time ago there\n> wasn't a way to ask \"here is an object name---please uniquify in the\n> context of _that_ repository\".  So it is understandable to prepare\n> an object X in a submodule and another object Y in the superproject,\n> such that the abbreviated name of X in the context of the submodule\n> is different from the abbreviated name of X in the context of the\n> superproject (i.e. if X were in the superproject's object store,\n> because the object names of X and Y share the prefix, it may require\n> longer prefix to disambiguate from Y), and make sure that the uniquify\n> is indeed happening in the context of the submodule.\n>\n> So, you are only concentrating on (1) but forgetting why the author\n> wants (2).\n\nIndeed. That makes sense, but it would really help to e.g. have the test\ndescription make that goal explicit.\n"},{"id":"449379","messageId":"20220224001143.2150137-1-jonathantanmy@google.com","threadId":"57442","inReplyTo":"220221.86ley4aw88.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ls-files: support --recurse-submodules --stage","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-24T00:11:42Z","receivedAt":"2022-02-24T00:11:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n> On Mon, Feb 21 2022, Junio C Hamano wrote:\n> > I think what this is trying to make sure is that it (1) enabled the\n> > combination and (2) uses the object store of the submodule when\n> > disambiguating names of the objects from the submodule, because the\n> > author suspects that the reason why these two options were made\n> > incompatible in the first place was because long time ago there\n> > wasn't a way to ask \"here is an object name---please uniquify in the\n> > context of _that_ repository\".  So it is understandable to prepare\n> > an object X in a submodule and another object Y in the superproject,\n> > such that the abbreviated name of X in the context of the submodule\n> > is different from the abbreviated name of X in the context of the\n> > superproject (i.e. if X were in the superproject's object store,\n> > because the object names of X and Y share the prefix, it may require\n> > longer prefix to disambiguate from Y), and make sure that the uniquify\n> > is indeed happening in the context of the submodule.\n> >\n> > So, you are only concentrating on (1) but forgetting why the author\n> > wants (2).\n> \n> Indeed. That makes sense, but it would really help to e.g. have the test\n> description make that goal explicit.\n\nThat indeed was why I did that (thanks, Junio). It looks like it doesn't\nwork with other hash algorithms, and maybe it's too much paranoia on my\npart. I'll send an updated version following Ævar's suggestion.\n"},{"id":"449381","messageId":"20220224002303.2363189-1-jonathantanmy@google.com","threadId":"57442","inReplyTo":"20220218223212.1139366-1-jonathantanmy@google.com","subject":"[PATCH v2] ls-files: support --recurse-submodules --stage","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-24T00:23:03Z","receivedAt":"2022-02-24T00:23:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"e77aa336f1 (\"ls-files: optionally recurse into submodules\", 2016-10-10)\ntaught ls-files the --recurse-submodules argument, but only in a limited\nset of circumstances. In particular, --stage was unsupported, perhaps\nbecause there was no repo_find_unique_abbrev(), which was only\nintroduced in 8bb95572b0 (\"sha1-name.c: add\nrepo_find_unique_abbrev_r()\", 2019-04-16). This function is needed for\nusing --recurse-submodules with --stage.\n\nNow that we have repo_find_unique_abbrev(), teach support for this\ncombination of arguments.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nHere's version 2 with an updated test.\n---\nRange-diff against v1:\n1:  d61268bcc8 ! 1:  17f3f77b86 ls-files: support --recurse-submodules --stage\n    @@ Commit message\n     \n         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n         ---\n    -    I got the similar-hashing object contents from Ævar's work in [1].\n    -\n    -    [1] https://lore.kernel.org/git/patch-v7-1.6-28c01b7f8a5-20220111T130811Z-avarab@gmail.com/\n    +    Here's version 2 with an updated test.\n     \n      ## Documentation/git-ls-files.txt ##\n     @@ Documentation/git-ls-files.txt: a space) at the start of each line:\n    @@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success 'ls-files correctly\n      '\n      \n     +test_expect_success '--stage' '\n    -+\t# In order to test hash abbreviation, write two objects that have the\n    -+\t# same first 4 hexadecimal characters in their (SHA-1) hashes.\n    -+\techo brocdnra >submodule/c &&\n    -+\tgit -C submodule commit -am \"update c\" &&\n    -+\techo brigddsv >submodule/c &&\n    -+\tgit -C submodule commit -am \"update c again\" &&\n    ++\tGITMODULES_HASH=$(git rev-parse HEAD:.gitmodules) &&\n    ++\tA_HASH=$(git rev-parse HEAD:a) &&\n    ++\tB_HASH=$(git rev-parse HEAD:b/b) &&\n    ++\tC_HASH=$(git -C submodule rev-parse HEAD:c) &&\n     +\n    -+\tcat >expect <<-\\EOF &&\n    -+\t100644 6da7 0\t.gitmodules\n    -+\t100644 7898 0\ta\n    -+\t100644 6178 0\tb/b\n    -+\t100644 dead9 0\tsubmodule/c\n    ++\tcat >expect <<-EOF &&\n    ++\t100644 $GITMODULES_HASH 0\t.gitmodules\n    ++\t100644 $A_HASH 0\ta\n    ++\t100644 $B_HASH 0\tb/b\n    ++\t100644 $C_HASH 0\tsubmodule/c\n     +\tEOF\n     +\n    -+\tgit ls-files --stage --recurse-submodules --abbrev=4 >actual &&\n    ++\tgit ls-files --stage --recurse-submodules >actual &&\n     +\ttest_cmp expect actual\n     +'\n     +\n\n Documentation/git-ls-files.txt         |  2 +-\n builtin/ls-files.c                     |  4 ++--\n t/t3007-ls-files-recurse-submodules.sh | 18 +++++++++++++++++-\n 3 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 48cc7c0b6f..0dabf3f0dd 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -156,7 +156,7 @@ a space) at the start of each line:\n \n --recurse-submodules::\n \tRecursively calls ls-files on each active submodule in the repository.\n-\tCurrently there is only support for the --cached mode.\n+\tCurrently there is only support for the --cached and --stage modes.\n \n --abbrev[=<n>]::\n \tInstead of showing the full 40-byte hexadecimal object\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex f7ea56cc63..e791b65e7e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -244,7 +244,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,\n \t\t\tprintf(\"%s%06o %s %d\\t\",\n \t\t\t       tag,\n \t\t\t       ce->ce_mode,\n-\t\t\t       find_unique_abbrev(&ce->oid, abbrev),\n+\t\t\t       repo_find_unique_abbrev(repo, &ce->oid, abbrev),\n \t\t\t       ce_stage(ce));\n \t\t}\n \t\twrite_eolinfo(repo->index, ce, fullname);\n@@ -726,7 +726,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tsetup_work_tree();\n \n \tif (recurse_submodules &&\n-\t    (show_stage || show_deleted || show_others || show_unmerged ||\n+\t    (show_deleted || show_others || show_unmerged ||\n \t     show_killed || show_modified || show_resolve_undo || with_tree))\n \t\tdie(\"ls-files --recurse-submodules unsupported mode\");\n \ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex 4a08000713..dd7770e85d 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -34,6 +34,23 @@ test_expect_success 'ls-files correctly outputs files in submodule' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--stage' '\n+\tGITMODULES_HASH=$(git rev-parse HEAD:.gitmodules) &&\n+\tA_HASH=$(git rev-parse HEAD:a) &&\n+\tB_HASH=$(git rev-parse HEAD:b/b) &&\n+\tC_HASH=$(git -C submodule rev-parse HEAD:c) &&\n+\n+\tcat >expect <<-EOF &&\n+\t100644 $GITMODULES_HASH 0\t.gitmodules\n+\t100644 $A_HASH 0\ta\n+\t100644 $B_HASH 0\tb/b\n+\t100644 $C_HASH 0\tsubmodule/c\n+\tEOF\n+\n+\tgit ls-files --stage --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'ls-files correctly outputs files in submodule with -z' '\n \tlf_to_nul >expect <<-\\EOF &&\n \t.gitmodules\n@@ -292,7 +309,6 @@ test_incompatible_with_recurse_submodules () {\n test_incompatible_with_recurse_submodules --deleted\n test_incompatible_with_recurse_submodules --modified\n test_incompatible_with_recurse_submodules --others\n-test_incompatible_with_recurse_submodules --stage\n test_incompatible_with_recurse_submodules --killed\n test_incompatible_with_recurse_submodules --unmerged\n \n-- \n2.35.1.574.g5d30c73bfb-goog\n\n"}]}