{"thread":{"id":"53575","subject":"[PATCH] doc: ls-tree paths do not support wildcards","startedAt":"2020-05-28T21:23:30Z","lastAt":"2020-05-28T23:17:34Z","messageCount":7,"participants":["Steven Willis via GitGitGadget","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"398805","messageId":"pull.796.git.git.1590700996483.gitgitgadget@gmail.com","threadId":"53575","inReplyTo":null,"subject":"[PATCH] doc: ls-tree paths do not support wildcards","fromName":"Steven Willis via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-28T21:23:16Z","receivedAt":"2020-05-28T21:23:30Z","isPatch":true,"sender":{"key":"name:Steven Willis","avatar":null},"body":"From: Steven Willis \n\nSigned-off-by: Steven Willis \n---\n doc: ls-tree paths do not support wildcards\n \n The documentation for ls-tree says that paths can be wildcards, but this\n appears to be incorrect, only raw paths seem to work.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-796%2Fonlynone%2Fls-tree-paths-do-not-support-wildcards-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-796/onlynone/ls-tree-paths-do-not-support-wildcards-v1\nPull-Request: https://github.com/git/git/pull/796\n\n Documentation/git-ls-tree.txt | 6 +++---\n t/t3102-ls-tree-wildcards.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\nindex a7515714da1..8a8ce20cf51 100644\n--- a/Documentation/git-ls-tree.txt\n+++ b/Documentation/git-ls-tree.txt\n@@ -19,7 +19,7 @@ Lists the contents of a given tree object, like what \"/bin/ls -a\" does\n in the current working directory. Note that:\n \n - the behaviour is slightly different from that of \"/bin/ls\" in that the\n- '' denotes just a list of patterns to match, e.g. so specifying\n+ '' denotes just a list of files to match, e.g. so specifying\n directory name (without `-r`) will behave differently, and order of the\n arguments does not matter.\n \n@@ -74,8 +74,8 @@ OPTIONS\n \tImplies --full-name.\n \n [...]::\n-\tWhen paths are given, show them (note that this isn't really raw\n-\tpathnames, but rather a list of patterns to match). Otherwise\n+\tWhen paths are given, show them (note that this is really raw\n+\tpathnames, not a list of patterns to match). Otherwise\n \timplicitly uses the root level of the tree as the sole path argument.\n \n \ndiff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh\nindex 1e16c6b8ea6..6c0f2af1d04 100755\n--- a/t/t3102-ls-tree-wildcards.sh\n+++ b/t/t3102-ls-tree-wildcards.sh\n@@ -33,4 +33,10 @@ test_expect_failure 'ls-tree does not yet support negated pathspec' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'ls-tree does not yet support wildcard pathspec' '\n+\tgit ls-files \"a*\" >expect &&\n+\tgit ls-tree --name-only -r HEAD \"a*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3\n-- \ngitgitgadget\n"},{"id":"398806","messageId":"20200528215107.GA1265681@coredump.intra.peff.net","threadId":"53575","inReplyTo":"pull.796.git.git.1590700996483.gitgitgadget@gmail.com","subject":"Re: [PATCH] doc: ls-tree paths do not support wildcards","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-28T21:51:07Z","receivedAt":"2020-05-28T21:51:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2020 at 09:23:16PM +0000, Steven Willis via GitGitGadget wrote:\n\n> From: Steven Willis \n> \n> Signed-off-by: Steven Willis \n> ---\n> doc: ls-tree paths do not support wildcards\n> \n> The documentation for ls-tree says that paths can be wildcards, but this\n> appears to be incorrect, only raw paths seem to work.\n\nThis part probably should be in the commit message. :)\n\n> [...]::\n> -\tWhen paths are given, show them (note that this isn't really raw\n> -\tpathnames, but rather a list of patterns to match). Otherwise\n> +\tWhen paths are given, show them (note that this is really raw\n> +\tpathnames, not a list of patterns to match). Otherwise\n> \timplicitly uses the root level of the tree as the sole path argument.\n\nYou're right that we don't match globs, but I don't think it's accurate\nto say that these aren't patterns, or that they are raw pathnames. We\n_do_ parse them as pathspecs, include magic prefixes. E.g.:\n\n $ git -C Documentation ls-tree HEAD -- :/Makefile\n 100644 blob 90aa329eb7836824a7a45383e4b5b157124d815c\t../Makefile\n\nAnd we complain about pathspec magic that isn't supported in the\nmatching code:\n\n $ git ls-tree HEAD -- :^Makefile\n fatal: :^Makefile: pathspec magic not supported by this command: 'exclude' (mnemonic: '!')\n\nBut we don't complain about an attempt to use glob characters (which\n_could_ really be an attempt to specify a funny-named file, though I\nguess you could argue the same for \":\" magic).\n\nSo I think for now we ought to explain the situation a bit more clearly:\nleave this language as-is, but add a new section describing what\npatterns we do support.\n\nIn the long run it would be nice to actually match regular pathspecs.\nThat would be a backwards-incompatibility, which I think is why nobody\nhas pursued it further (and ls-tree is meant to be plumbing that should\nstay consistent, so we need to be extra careful). So we'd need a\ntransition plan. Perhaps:\n\n 1. Deprecate the current behavior in the documentation and release\n notes, encouraging people who want literal matching to use\n --literal-pathspecs or the \":(literal)\" magic. AFAICT we've\n supported these since at least 2013 for this command, so it should\n be safe to use unconditionally.\n\n 2. Add a new option, \"--use-pathspecs\" or similar, that switches the\n matching code to use match_pathspec(). That lets people use the new\n feature immediately if they want to.\n\n 3. When --use-pathspecs is not in use, warn to stderr about any\n wildcard characters in the input. That reinforces the deprecation\n notice in (1) and is likely to get more people's attention.\n\n 4. After several releases, flip the default to --use-pathspecs,\n leaving --no-use-pathspecs as an escape hatch for people who still\n haven't switched their scripts.\n\n 5. After several more releases, eventually remove the old-style\n matching (perhaps leaving --use-pathspecs as a noop).\n\nTo be honest, that may be more careful than we absolutely need to be.\nWe'd only do the wrong thing if you really do have files with glob\nmetacharacters in their names. And if you're expecting to match names\nliterally and not using \":(literal)\" or similar, your script is\n_already_ wrong, since it would be barfing on names starting with a\ncolon. I have a feeling we made that backwards-incompatible change when\nthis was converted to pathspecs years ago, and nobody noticed either\nway.\n\n-Peff\n"},{"id":"398807","messageId":"xmqq4krz3edq.fsf@gitster.c.googlers.com","threadId":"53575","inReplyTo":"pull.796.git.git.1590700996483.gitgitgadget@gmail.com","subject":"Re: [PATCH] doc: ls-tree paths do not support wildcards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T22:04:33Z","receivedAt":"2020-05-28T22:04:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Steven Willis via GitGitGadget\" writes:\n\n> From: Steven Willis \n>\n> Signed-off-by: Steven Willis \n> ---\n> doc: ls-tree paths do not support wildcards\n> \n> The documentation for ls-tree says that paths can be wildcards, but this\n> appears to be incorrect, only raw paths seem to work.\n\nThis is not something you would write after the three-dash line.\n\nThe documentation does not say paths can be wildcards. It allows a\nlist of \"patterns to match\" and never says they are wildcards.\n\nI think they take the traditional \"leading paths\" (i.e. by saying\n\"git ls-tree HEAD t/\", you can show all paths that are under t/,\nand is different from the \"raw path\" i.e. \"git ls-tree HEAD t\").\n\n"},{"id":"398808","messageId":"xmqqy2pb1z0x.fsf@gitster.c.googlers.com","threadId":"53575","inReplyTo":"20200528215107.GA1265681@coredump.intra.peff.net","subject":"Re: [PATCH] doc: ls-tree paths do not support wildcards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T22:21:34Z","receivedAt":"2020-05-28T22:21:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think for now we ought to explain the situation a bit more clearly:\n> leave this language as-is, but add a new section describing what\n> patterns we do support.\n\nThanks; you said a lot better than I could ;-)\n\n> In the long run it would be nice to actually match regular pathspecs.\n> That would be a backwards-incompatibility, which I think is why nobody\n> has pursued it further (and ls-tree is meant to be plumbing that should\n> stay consistent, so we need to be extra careful). So we'd need a\n> transition plan. Perhaps:\n>\n> 1. Deprecate the current behavior in the documentation and release\n> notes, encouraging people who want literal matching to use\n> --literal-pathspecs or the \":(literal)\" magic. AFAICT we've\n> supported these since at least 2013 for this command, so it should\n> be safe to use unconditionally.\n>\n> 2. Add a new option, \"--use-pathspecs\" or similar, that switches the\n> matching code to use match_pathspec(). That lets people use the new\n> feature immediately if they want to.\n>\n> 3. When --use-pathspecs is not in use, warn to stderr about any\n> wildcard characters in the input. That reinforces the deprecation\n> notice in (1) and is likely to get more people's attention.\n\nHmph, if we are serious about deprecation and migration, I would\nimage that in stage #1, we should do this check already. When\n\"--literal-pathspecs\" is NOT in use, if a pathspec would change its\nmeaning if not taken literally (e.g. has glob letters, begins with\n:-magic introducer, etc.), we warn and do so from the very beginning\nof the migration process.\n\n> 4. After several releases, flip the default to --use-pathspecs,\n> leaving --no-use-pathspecs as an escape hatch for people who still\n> haven't switched their scripts.\n\nWouldn't --literal-pathspecs be the accepted escape hatch that will\nalways be accepted, even after --use-pathspecs becomes a no-op?\n\n> 5. After several more releases, eventually remove the old-style\n> matching (perhaps leaving --use-pathspecs as a noop).\n\n> To be honest, that may be more careful than we absolutely need to be.\n\nYeah, there seem to be some room for optimization, but I think the\nkey steps are about right if we wanted to do this migration.\n\nThanks.\n"},{"id":"398811","messageId":"20200528230454.GA1267915@coredump.intra.peff.net","threadId":"53575","inReplyTo":"xmqqy2pb1z0x.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] doc: ls-tree paths do not support wildcards","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-28T23:04:54Z","receivedAt":"2020-05-28T23:04:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2020 at 03:21:34PM -0700, Junio C Hamano wrote:\n\n> > 1. Deprecate the current behavior in the documentation and release\n> > notes, encouraging people who want literal matching to use\n> > --literal-pathspecs or the \":(literal)\" magic. AFAICT we've\n> > supported these since at least 2013 for this command, so it should\n> > be safe to use unconditionally.\n> >\n> > 2. Add a new option, \"--use-pathspecs\" or similar, that switches the\n> > matching code to use match_pathspec(). That lets people use the new\n> > feature immediately if they want to.\n> >\n> > 3. When --use-pathspecs is not in use, warn to stderr about any\n> > wildcard characters in the input. That reinforces the deprecation\n> > notice in (1) and is likely to get more people's attention.\n> \n> Hmph, if we are serious about deprecation and migration, I would\n> image that in stage #1, we should do this check already. When\n> \"--literal-pathspecs\" is NOT in use, if a pathspec would change its\n> meaning if not taken literally (e.g. has glob letters, begins with\n> :-magic introducer, etc.), we warn and do so from the very beginning\n> of the migration process.\n\nYeah, sorry, I meant these three steps to all happen at once.\n\nTechnically we don't need step (2) in there for the deprecation, but I\nthink it lets people adjust to the new world order as their solution to\navoid the warning (though I guess literal-pathspecs would also prevent\nthe warning; we wouldn't be looking for \"*\" in the input so much as\nchecking whether the parsed pathspec contains a wildcard).\n\n> > 4. After several releases, flip the default to --use-pathspecs,\n> > leaving --no-use-pathspecs as an escape hatch for people who still\n> > haven't switched their scripts.\n> \n> Wouldn't --literal-pathspecs be the accepted escape hatch that will\n> always be accepted, even after --use-pathspecs becomes a no-op?\n\nHmm, I guess so. That wouldn't restore the behavior as it is _now_, but\nthe current behavior is sort of pointless. It treats pathspec magic as\nspecial, but not globs. I doubt anybody actually wants that; they'd\neither want pathspecs or literal paths.\n\n-Peff\n"},{"id":"398814","messageId":"20200528231622.GB1267915@coredump.intra.peff.net","threadId":"53575","inReplyTo":"xmqq4krz3edq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] doc: ls-tree paths do not support wildcards","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-28T23:16:22Z","receivedAt":"2020-05-28T23:16:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2020 at 03:04:33PM -0700, Junio C Hamano wrote:\n\n> The documentation does not say paths can be wildcards. It allows a\n> list of \"patterns to match\" and never says they are wildcards.\n> \n> I think they take the traditional \"leading paths\" (i.e. by saying\n> \"git ls-tree HEAD t/\", you can show all paths that are under t/,\n> and is different from the \"raw path\" i.e. \"git ls-tree HEAD t\").\n\nHmm, that might be weird if we switch to the pathspec matching code,\nbecause we're sort-of recursive and sort-of not. E.g.:\n\n $ mkdir -p one/two/three\n $ echo content >one/two/three/file\n $ git init && git add . && git commit -m foo\n\n $ git ls-tree HEAD\n 040000 tree 8f303030860565e9c51101f993e1db4feb2792a7\tone\n\n $ git ls-tree -r HEAD\n 100644 blob d95f3ad14dee633a758d2e331151e950dd13e4ed\tone/two/three/file\n\n git ls-tree HEAD one\n 040000 tree 8f303030860565e9c51101f993e1db4feb2792a7\tone\n\n $ git ls-tree HEAD one/\n 040000 tree c89b49675f4ccf1d03f7c95b5f072566ce0d7f83\tone/two\n\nSo the pathspec match triggers one level of recursion, which is unlike a\npathspec used with non-recursive diff:\n\n $ git diff-tree --root --abbrev HEAD\n 6f4ed388a766b500bcf3bbd9877cc7213c76a959\n :000000 040000 0000000 8f30303 A\tone\n \n $ git diff-tree --root --abbrev -r HEAD\n 6f4ed388a766b500bcf3bbd9877cc7213c76a959\n :000000 100644 0000000 d95f3ad A\tone/two/three/file\n \n $ git diff-tree --root --abbrev HEAD one\n 6f4ed388a766b500bcf3bbd9877cc7213c76a959\n :000000 040000 0000000 8f30303 A\tone\n \n $ git diff-tree --root --abbrev HEAD one/\n 6f4ed388a766b500bcf3bbd9877cc7213c76a959\n :000000 040000 0000000 8f30303 A\tone\n\nI think we should be able to reuse the pathspec matching code, but we\nmay not be able to reuse other tree-walking bits. Or maybe we can. It\nlooks like we already use read_tree_recursive(), so I guess our custom\nshow_tree() callback is able to handle that. But definitely something to\nwatch out for during any transition.\n\n-Peff\n"},{"id":"398815","messageId":"xmqqk10v1wfs.fsf@gitster.c.googlers.com","threadId":"53575","inReplyTo":"20200528230454.GA1267915@coredump.intra.peff.net","subject":"Re: [PATCH] doc: ls-tree paths do not support wildcards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T23:17:27Z","receivedAt":"2020-05-28T23:17:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 28, 2020 at 03:21:34PM -0700, Junio C Hamano wrote:\n>\n>> > 1. Deprecate the current behavior in the documentation and release\n>> > notes, encouraging people who want literal matching to use\n>> > --literal-pathspecs or the \":(literal)\" magic. AFAICT we've\n>> > supported these since at least 2013 for this command, so it should\n>> > be safe to use unconditionally.\n>> >\n>> > 2. Add a new option, \"--use-pathspecs\" or similar, that switches the\n>> > matching code to use match_pathspec(). That lets people use the new\n>> > feature immediately if they want to.\n>> >\n>> > 3. When --use-pathspecs is not in use, warn to stderr about any\n>> > wildcard characters in the input. That reinforces the deprecation\n>> > notice in (1) and is likely to get more people's attention.\n>> \n>> Hmph, if we are serious about deprecation and migration, I would\n>> image that in stage #1, we should do this check already. When\n>> \"--literal-pathspecs\" is NOT in use, if a pathspec would change its\n>> meaning if not taken literally (e.g. has glob letters, begins with\n>> :-magic introducer, etc.), we warn and do so from the very beginning\n>> of the migration process.\n>\n> Yeah, sorry, I meant these three steps to all happen at once.\n>\n> Technically we don't need step (2) in there for the deprecation, but I\n> think it lets people adjust to the new world order as their solution to\n> avoid the warning (though I guess literal-pathspecs would also prevent\n> the warning; we wouldn't be looking for \"*\" in the input so much as\n> checking whether the parsed pathspec contains a wildcard).\n\nI do agree that letting early adopters experiemtn is a great and\nnecessary step. And I think I misread the condition you wrote in\nyour 3.; I do agree that we should warn when --use-pathspecs is not\nin use, but if --literal-pathspecs is in use, then the user wants\nto match literally even when the pattern has globs and :-magic\nintroducer sequence, so we shouldn't warn.\n\nThanks.\n"}]}