{"thread":{"id":"59797","subject":"git 2.40.1 tree-diff crashes with (glob) magic on pathspecs","startedAt":"2023-05-27T14:01:09Z","lastAt":"2023-06-15T19:00:08Z","messageCount":12,"participants":["Jim Pryor","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"477756","messageId":"c2b73d05-1214-4e30-bab6-14e0b8d69773@app.fastmail.com","threadId":"59797","inReplyTo":null,"subject":"git 2.40.1 tree-diff crashes with (glob) magic on pathspecs","fromName":"Jim Pryor","fromEmail":"dubiousjim@gmail.com","sentAt":"2023-05-27T13:53:31Z","receivedAt":"2023-05-27T14:01:09Z","isPatch":false,"sender":{"key":"dubiousjim@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n$ mkdir test2\n$ cd test2\n$ git init\nInitialized empty Git repository in /Users/jim/repo/test2/.git/\n$ mkdir -p foo/bar\n$ touch foo/bar/test\n$ git add foo/bar/test\n$ git commit -m \"first commit\" -u\n[master (root-commit) c6eb1c1] first commit\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 foo/bar/test\n$ git mv foo/bar/test foo/bar/test2\n$ git commit -m \"second commit\" -u\n[master 2bacc0c] second commit\n 1 file changed, 0 insertions(+), 0 deletions(-)\n rename foo/bar/{test => test2} (100%)\n$ echo \"contents\" > foo/bar/test2\n$ git commit -m \"third commit\" -a\n[master 13a1b11] third commit\n 1 file changed, 1 insertion(+)\n\n$ git --no-pager log --oneline -- ':(glob)**/test2'\n\nWhat did you expect to happen? (Expected behavior)\n\n13a1b11 (HEAD -> master) third commit\n2bacc0c second commit\nc6eb1c1 first commit\n\nWhat happened instead? (Actual behavior)\n\n13a1b11 (HEAD -> master) third commit\nBUG: tree-diff.c:596: unsupported magic 8\nerror: git died of signal 6\n\nWhat's different between what you expected and what actually happened?\n\ngit tree-diff crashed\n\nAnything else you want to add:\n\n* Also get the crashes with ':(glob)*/bar/test2' and ':(glob)foo/*/test2'.\n* The `git mv` does not seem to be necessary to reproduce.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.40.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Tue Jun 21 21:18:39 PDT 2022; root:xnu-6153.141.66~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 12.0.0 (clang-1200.0.32.29)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n-- \ndubiousjim@gmail.com\n"},{"id":"477760","messageId":"20230527173903.GB1460206@coredump.intra.peff.net","threadId":"59797","inReplyTo":"c2b73d05-1214-4e30-bab6-14e0b8d69773@app.fastmail.com","subject":"Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-27T17:39:03Z","receivedAt":"2023-05-27T17:39:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 27, 2023 at 03:53:31PM +0200, Jim Pryor wrote:\n\n> $ git --no-pager log --oneline -- ':(glob)**/test2'\n> \n> What did you expect to happen? (Expected behavior)\n> \n> 13a1b11 (HEAD -> master) third commit\n> 2bacc0c second commit\n> c6eb1c1 first commit\n> \n> What happened instead? (Actual behavior)\n> \n> 13a1b11 (HEAD -> master) third commit\n> BUG: tree-diff.c:596: unsupported magic 8\n> error: git died of signal 6\n\nThanks for your report. I had trouble reproducing at first, but there is\none extra twist. By default, the command you gave produces:\n\n  $ git --no-pager log --oneline -- ':(glob)**/test2'\n  48eab09 (HEAD -> main) third commit\n  89a47e5 second commit\n\nwhich makes sense; we did not ask about \"test\", which is the path the\noriginal commit touched. So I guessed you might have log.follow set, and\nindeed:\n\n  $ git --no-pager log --oneline --follow -- ':(glob)**/test2'\n  48eab09 (HEAD -> main) third commit\n  BUG: tree-diff.c:596: unsupported magic 8\n  Aborted\n\nI _think_ in this case that \":(glob)\" is not actually doing much. It is\nalready the default that we will glob pathspecs; it is only needed to\noverride \"git --literal\" (which changes the default to not glob).\n\nBut this also seems to be wading into a spot that is not actually\nsupported by --follow. The code near where the BUG() is triggered looks\nlike this:\n\nstatic void try_to_follow_renames([...])\n{\n[...]\n        /*\n         * follow-rename code is very specific, we need exactly one\n         * path. Magic that matches more than one path is not\n         * supported.\n         */\n        GUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);\n#if 0\n        /*\n         * We should reject wildcards as well. Unfortunately we\n         * haven't got a reliable way to detect that 'foo\\*bar' in\n         * fact has no wildcards. nowildcard_len is merely a hint for\n         * optimization. Let it slip for now until wildmatch is taught\n         * about dry-run mode and returns wildcard info.\n         */\n        if (opt->pathspec.has_wildcard)\n                BUG(\"wildcards are not supported\");\n#endif\n\nSo follow-mode does not actually work with wildcards, but we err on the\nside of not rejecting them outright. In that sense, the simplest \"fix\"\nhere would be to allow :(glob) to match the '#if 0' section, like:\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 69031d7cba..d287079253 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -593,7 +593,8 @@ static void try_to_follow_renames(const struct object_id *old_oid,\n \t * path. Magic that matches more than one path is not\n \t * supported.\n \t */\n-\tGUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);\n+\tGUARD_PATHSPEC(&opt->pathspec,\n+\t\t       PATHSPEC_FROMTOP | PATHSPEC_LITERAL | PATHSPEC_GLOB);\n #if 0\n \t/*\n \t * We should reject wildcards as well. Unfortunately we\n\nThat would avoid the BUG() and make things work consistently. But it\nstill would not produce the output you expect, because --follow simply\ndoes not work with wildcard pathspecs. And fixing that would presumably\nbe a much bigger change (I didn't dig into it).\n\nComplicating this slightly is the fact that you didn't explicitly ask\nfor --follow, but just had log.follow set. I think we automatically\ndisable log.follow when there is more than one pathspec, and in theory\nwe ought to do the same for actual wildcards. But maybe it would be\nenough for follow-mode to kick in, but to end up mostly ignored (which\nis what the patch above would do).\n\n(I say \"mostly\" because I suspect you could construct a history where\nthe glob matched a literal filename, too, and the follow code gets\nconfused. But outside of intentionally-constructed pathological cases,\nthat seems unlikely).\n\n-Peff\n"},{"id":"477762","messageId":"51012e79-c6ce-41dd-9755-960e065214ef@app.fastmail.com","threadId":"59797","inReplyTo":"20230527173903.GB1460206@coredump.intra.peff.net","subject":"Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs","fromName":"Jim Pryor","fromEmail":"dubiousjim@gmail.com","sentAt":"2023-05-27T19:00:27Z","receivedAt":"2023-05-27T19:01:02Z","isPatch":false,"sender":{"key":"dubiousjim@gmail.com","avatar":null},"body":"On Sat, May 27, 2023, at 7:39 PM, Jeff King wrote:\n> On Sat, May 27, 2023 at 03:53:31PM +0200, Jim Pryor wrote:\n>\n>> $ git --no-pager log --oneline -- ':(glob)**/test2'\n>> \n>> What did you expect to happen? (Expected behavior)\n>> \n>> 13a1b11 (HEAD -> master) third commit\n>> 2bacc0c second commit\n>> c6eb1c1 first commit\n>> \n>> What happened instead? (Actual behavior)\n>> \n>> 13a1b11 (HEAD -> master) third commit\n>> BUG: tree-diff.c:596: unsupported magic 8\n>> error: git died of signal 6\n>\n> Thanks for your report. I had trouble reproducing at first, but there is\n> one extra twist. By default, the command you gave produces:\n>\n>   $ git --no-pager log --oneline -- ':(glob)**/test2'\n>   48eab09 (HEAD -> main) third commit\n>   89a47e5 second commit\n>\n> which makes sense; we did not ask about \"test\", which is the path the\n> original commit touched. So I guessed you might have log.follow set, and\n> indeed:\n>\n>   $ git --no-pager log --oneline --follow -- ':(glob)**/test2'\n>   48eab09 (HEAD -> main) third commit\n>   BUG: tree-diff.c:596: unsupported magic 8\n>   Aborted\n>\n> I _think_ in this case that \":(glob)\" is not actually doing much. It is\n> already the default that we will glob pathspecs; it is only needed to\n> override \"git --literal\" (which changes the default to not glob).\n>\n> But this also seems to be wading into a spot that is not actually\n> supported by --follow. The code near where the BUG() is triggered looks\n> like this:\n>\n> static void try_to_follow_renames([...])\n> {\n> [...]\n>         /*\n>          * follow-rename code is very specific, we need exactly one\n>          * path. Magic that matches more than one path is not\n>          * supported.\n>          */\n>         GUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);\n> #if 0\n>         /*\n>          * We should reject wildcards as well. Unfortunately we\n>          * haven't got a reliable way to detect that 'foo\\*bar' in\n>          * fact has no wildcards. nowildcard_len is merely a hint for\n>          * optimization. Let it slip for now until wildmatch is taught\n>          * about dry-run mode and returns wildcard info.\n>          */\n>         if (opt->pathspec.has_wildcard)\n>                 BUG(\"wildcards are not supported\");\n> #endif\n>\n> So follow-mode does not actually work with wildcards, but we err on the\n> side of not rejecting them outright. In that sense, the simplest \"fix\"\n> here would be to allow :(glob) to match the '#if 0' section, like:\n>\n> diff --git a/tree-diff.c b/tree-diff.c\n> index 69031d7cba..d287079253 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -593,7 +593,8 @@ static void try_to_follow_renames(const struct \n> object_id *old_oid,\n>  \t * path. Magic that matches more than one path is not\n>  \t * supported.\n>  \t */\n> -\tGUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);\n> +\tGUARD_PATHSPEC(&opt->pathspec,\n> +\t\t       PATHSPEC_FROMTOP | PATHSPEC_LITERAL | PATHSPEC_GLOB);\n>  #if 0\n>  \t/*\n>  \t * We should reject wildcards as well. Unfortunately we\n>\n> That would avoid the BUG() and make things work consistently. But it\n> still would not produce the output you expect, because --follow simply\n> does not work with wildcard pathspecs. And fixing that would presumably\n> be a much bigger change (I didn't dig into it).\n>\n> Complicating this slightly is the fact that you didn't explicitly ask\n> for --follow, but just had log.follow set. I think we automatically\n> disable log.follow when there is more than one pathspec, and in theory\n> we ought to do the same for actual wildcards. But maybe it would be\n> enough for follow-mode to kick in, but to end up mostly ignored (which\n> is what the patch above would do).\n>\n> (I say \"mostly\" because I suspect you could construct a history where\n> the glob matched a literal filename, too, and the follow code gets\n> confused. But outside of intentionally-constructed pathological cases,\n> that seems unlikely).\n>\n> -Peff\n\nConfirmed I have log.follow = true, and also GIT_NOGLOB_PATHSPECS set, thus the explicit `(glob)` magic.\n\nI'd be happy with --follow not working with the wildcard pathspecs, but with the crash avoided.\n\n-- \ndubiousjim@gmail.com\n"},{"id":"477852","messageId":"xmqqsfbbd9oi.fsf@gitster.g","threadId":"59797","inReplyTo":"20230527173903.GB1460206@coredump.intra.peff.net","subject":"Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-01T05:38:05Z","receivedAt":"2023-06-01T05:39:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> 13a1b11 (HEAD -> master) third commit\n>> BUG: tree-diff.c:596: unsupported magic 8\n>> error: git died of signal 6\n> ...\n> Thanks for your report. I had trouble reproducing at first, but there is\n> one extra twist. By default, the command you gave produces:\n> But this also seems to be wading into a spot that is not actually\n> supported by --follow. The code near where the BUG() is triggered looks\n> like this:\n>\n> static void try_to_follow_renames([...])\n> {\n> [...]\n>         /*\n>          * follow-rename code is very specific, we need exactly one\n>          * path. Magic that matches more than one path is not\n>          * supported.\n>          */\n>         GUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);\n> #if 0\n>         /*\n>          * We should reject wildcards as well. Unfortunately we\n>          * haven't got a reliable way to detect that 'foo\\*bar' in\n>          * fact has no wildcards. nowildcard_len is merely a hint for\n>          * optimization. Let it slip for now until wildmatch is taught\n>          * about dry-run mode and returns wildcard info.\n>          */\n>         if (opt->pathspec.has_wildcard)\n>                 BUG(\"wildcards are not supported\");\n> #endif\n>\n> So follow-mode does not actually work with wildcards, but we err on the\n> side of not rejecting them outright. In that sense, the simplest \"fix\"\n> here would be to allow :(glob) to match the '#if 0' section, like:\n\nIs it \"fix\" or widening the wound, though?  The runtime BUG is very\nunpleasant to see, but silently giving a possibly wrong result would\nbe even worse, I am afraid.\n\n> That would avoid the BUG() and make things work consistently. But it\n> still would not produce the output you expect, because --follow simply\n> does not work with wildcard pathspecs. And fixing that would presumably\n> be a much bigger change (I didn't dig into it).\n\nTrue.  And wildcard is not the only thing it is broken for; the\nbiggest one is that it only follows one path at the time across all\nthe traversal paths, which has interesting implications like it can\ngive inconsistent result depending on the traversal order in a mergy\nhistory.\n\nIf somebody cares about the \"--follow\" mode very deeply, the \"upon\nfinding that the path disappears, run the rename detection with the\nparent and switch the (single) path to follow to the old path in the\nparent\" logic must be updated to keep track of these pathspecs per\ntraversal paths.  If false positives are allowed, an approximation\nthat may be easier to implement is to add paths to the set of paths\nto be followed every time such a rename is found.\n\n\n"},{"id":"477876","messageId":"20230601133057.GA2458601@coredump.intra.peff.net","threadId":"59797","inReplyTo":"xmqqsfbbd9oi.fsf@gitster.g","subject":"Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T13:30:57Z","receivedAt":"2023-06-01T13:31:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 01, 2023 at 02:38:05PM +0900, Junio C Hamano wrote:\n\n> > So follow-mode does not actually work with wildcards, but we err on the\n> > side of not rejecting them outright. In that sense, the simplest \"fix\"\n> > here would be to allow :(glob) to match the '#if 0' section, like:\n> \n> Is it \"fix\" or widening the wound, though?  The runtime BUG is very\n> unpleasant to see, but silently giving a possibly wrong result would\n> be even worse, I am afraid.\n\nI think it's the same wrong result we'd give in other circumstances,\nwhich in some ways makes it orthogonal. I.e., this is already wrong:\n\n  git log --follow 'foo*'\n\nDoing:\n\n  git --noglob-pathspecs --follow 'foo*'\n\nmakes it right (assuming you really have a literal '*' in your\npathname). And then doing:\n\n  git --noglob-pathspecs --follow ':(glob)foo*'\n\nmakes it wrong again, but in exactly the same way as the first case.\nWhich is why I said \"orthogonal\" above, because it is really just\ntwiddling options to cancel each other out and reach the same broken\nstate. ;)\n\nAll of that said, I do think the patch I showed earlier is pretty ugly,\nand it is not even fixing all of the problems (you can trigger the same\nBUG() for \":(icase)\", etc). I have a series which moves us in a better\ndirection, but I need to put a few finishing touches on it. I'll\nhopefully send it later today.\n\n> If somebody cares about the \"--follow\" mode very deeply, the \"upon\n> finding that the path disappears, run the rename detection with the\n> parent and switch the (single) path to follow to the old path in the\n> parent\" logic must be updated to keep track of these pathspecs per\n> traversal paths.  If false positives are allowed, an approximation\n> that may be easier to implement is to add paths to the set of paths\n> to be followed every time such a rename is found.\n\nYes, it would be nice to fix all of these --follow bugs once and for\nall. But that's a pretty big task. I think in the meantime, it is not\ntoo hard to make Jim's case behave more sensibly.\n\n-Peff\n"},{"id":"477887","messageId":"20230601173724.GA4158369@coredump.intra.peff.net","threadId":"59797","inReplyTo":"51012e79-c6ce-41dd-9755-960e065214ef@app.fastmail.com","subject":"[PATCH 0/3] handling pathspec magic with --follow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T17:37:24Z","receivedAt":"2023-06-01T17:37:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 27, 2023 at 09:00:27PM +0200, Jim Pryor wrote:\n\n> Confirmed I have log.follow = true, and also GIT_NOGLOB_PATHSPECS set,\n> thus the explicit `(glob)` magic.\n\nOK, all of that makes sense. The same problem hits \":(icase)\" and other\npathspec magic, too. So talking about :(glob) too much is kind of a red\nherring, anyway.\n\n> I'd be happy with --follow not working with the wildcard pathspecs,\n> but with the crash avoided.\n\nHere's a series that I think should improve things for you. For an\nexplicit --follow, it finds the forbidden magic sooner (which gives us a\nnice error message instead of the BUG()), and for log.follow, it avoids\nfollow-mode when forbidden pathspecs are used rather than enabling it\nand then complaining.\n\nThe interesting bits are in the third patch. The other two are\npreparatory.\n\n  [1/3]: pathspec: factor out magic-to-name function\n  [2/3]: diff: factor out --follow pathspec check\n  [3/3]: diff: detect pathspec magic not supported by --follow\n\n builtin/log.c  |  2 +-\n diff.c         | 29 +++++++++++++++++++++++++++--\n diff.h         |  7 +++++++\n pathspec.c     | 19 ++++++++++++-------\n pathspec.h     |  8 ++++++++\n t/t4202-log.sh | 15 +++++++++++++++\n 6 files changed, 70 insertions(+), 10 deletions(-)\n\n"},{"id":"477888","messageId":"20230601173814.GA4165297@coredump.intra.peff.net","threadId":"59797","inReplyTo":"20230601173724.GA4158369@coredump.intra.peff.net","subject":"[PATCH 1/3] pathspec: factor out magic-to-name function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T17:38:14Z","receivedAt":"2023-06-01T17:38:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have unsupported magic in a pathspec (because a command or code\npath does not support particular items), we list the unsupported ones in\nan error message.\n\nLet's factor out the code here that converts the bits back into their\nhuman-readable names, so that it can be used from other callers, which\nmay want to provide more flexible error messages.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pathspec.c | 19 ++++++++++++-------\n pathspec.h |  8 ++++++++\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 6966b265d3..5049dbb528 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -531,24 +531,29 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n \treturn strcmp(a->match, b->match);\n }\n \n-static void NORETURN unsupported_magic(const char *pattern,\n-\t\t\t\t       unsigned magic)\n+void pathspec_magic_names(unsigned magic, struct strbuf *out)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n \t\tif (!(magic & m->bit))\n \t\t\tcontinue;\n-\t\tif (sb.len)\n-\t\t\tstrbuf_addstr(&sb, \", \");\n+\t\tif (out->len)\n+\t\t\tstrbuf_addstr(out, \", \");\n \n \t\tif (m->mnemonic)\n-\t\t\tstrbuf_addf(&sb, _(\"'%s' (mnemonic: '%c')\"),\n+\t\t\tstrbuf_addf(out, _(\"'%s' (mnemonic: '%c')\"),\n \t\t\t\t    m->name, m->mnemonic);\n \t\telse\n-\t\t\tstrbuf_addf(&sb, \"'%s'\", m->name);\n+\t\t\tstrbuf_addf(out, \"'%s'\", m->name);\n \t}\n+}\n+\n+static void NORETURN unsupported_magic(const char *pattern,\n+\t\t\t\t       unsigned magic)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tpathspec_magic_names(magic, &sb);\n \t/*\n \t * We may want to substitute \"this command\" with a command\n \t * name. E.g. when \"git add -p\" or \"git add -i\" dies when running\ndiff --git a/pathspec.h b/pathspec.h\nindex a5b38e0907..fec4399bbc 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -130,6 +130,14 @@ void parse_pathspec_file(struct pathspec *pathspec,\n void copy_pathspec(struct pathspec *dst, const struct pathspec *src);\n void clear_pathspec(struct pathspec *);\n \n+/*\n+ * Add a human-readable string to \"out\" representing the PATHSPEC_* flags set\n+ * in \"magic\". The result is suitable for error messages, but not for\n+ * parsing as pathspec magic itself (you get 'icase' with quotes, not\n+ * :(icase)).\n+ */\n+void pathspec_magic_names(unsigned magic, struct strbuf *out);\n+\n static inline int ps_strncmp(const struct pathspec_item *item,\n \t\t\t     const char *s1, const char *s2, size_t n)\n {\n-- \n2.41.0.346.g8d12207a4f\n\n"},{"id":"477889","messageId":"20230601174106.GB4165297@coredump.intra.peff.net","threadId":"59797","inReplyTo":"20230601173724.GA4158369@coredump.intra.peff.net","subject":"[PATCH 2/3] diff: factor out --follow pathspec check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T17:41:06Z","receivedAt":"2023-06-01T17:41:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In --follow mode, we require exactly one pathspec. We check this\ncondition in two places:\n\n  - in diff_setup_done(), we complain if --follow is used with an\n    inapropriate pathspec\n\n  - in git-log's revision \"tweak\" function, we enable log.follow only if\n    the pathspec allows it\n\nThe duplication isn't a big deal right now, since the logic is so\nsimple. But in preparation for it becoming more complex, let's pull it\ninto a shared function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't love the die_on_error pattern here, but the obvious alternative\nof having the caller die when the boolean is false means it won't be\nable to give as descriptive a message. And I didn't think it was worth\ngetting into passing out an err strbuf, given that there are only these\ntwo callers.\n\n builtin/log.c |  2 +-\n diff.c        | 14 ++++++++++++--\n diff.h        |  7 +++++++\n 3 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 676de107d6..83d9b69dba 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -866,7 +866,7 @@ static void log_setup_revisions_tweak(struct rev_info *rev,\n \t\t\t\t      struct setup_revision_opt *opt)\n {\n \tif (rev->diffopt.flags.default_follow_renames &&\n-\t    rev->prune_data.nr == 1)\n+\t    diff_check_follow_pathspec(&rev->prune_data, 0))\n \t\trev->diffopt.flags.follow_renames = 1;\n \n \tif (rev->first_parent_only)\ndiff --git a/diff.c b/diff.c\nindex 3c88c37908..9f548f3471 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4751,6 +4751,16 @@ unsigned diff_filter_bit(char status)\n \treturn filter_bit[(int) status];\n }\n \n+int diff_check_follow_pathspec(struct pathspec *ps, int die_on_error)\n+{\n+\tif (ps->nr != 1) {\n+\t\tif (die_on_error)\n+\t\t\tdie(_(\"--follow requires exactly one pathspec\"));\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n void diff_setup_done(struct diff_options *options)\n {\n \tunsigned check_mask = DIFF_FORMAT_NAME |\n@@ -4858,8 +4868,8 @@ void diff_setup_done(struct diff_options *options)\n \n \toptions->diff_path_counter = 0;\n \n-\tif (options->flags.follow_renames && options->pathspec.nr != 1)\n-\t\tdie(_(\"--follow requires exactly one pathspec\"));\n+\tif (options->flags.follow_renames)\n+\t\tdiff_check_follow_pathspec(&options->pathspec, 1);\n \n \tif (!options->use_color || external_diff())\n \t\toptions->color_moved = 0;\ndiff --git a/diff.h b/diff.h\nindex 3a7a9e8b88..6c10ce289d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -539,6 +539,13 @@ void repo_diff_setup(struct repository *, struct diff_options *);\n struct option *add_diff_options(const struct option *, struct diff_options *);\n int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n void diff_setup_done(struct diff_options *);\n+\n+/*\n+ * Returns true if the pathspec can work with --follow mode. If die_on_error is\n+ * set, die() with a specific error message rather than returning false.\n+ */\n+int diff_check_follow_pathspec(struct pathspec *ps, int die_on_error);\n+\n int git_config_rename(const char *var, const char *value);\n \n #define DIFF_DETECT_RENAME\t1\n-- \n2.41.0.346.g8d12207a4f\n\n"},{"id":"477890","messageId":"20230601174351.GC4165297@coredump.intra.peff.net","threadId":"59797","inReplyTo":"20230601173724.GA4158369@coredump.intra.peff.net","subject":"[PATCH 3/3] diff: detect pathspec magic not supported by --follow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T17:43:51Z","receivedAt":"2023-06-01T17:43:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The --follow code doesn't handle most forms of pathspec magic. We check\nthat no unexpected ones have made it to try_to_follow_renames() with a\nruntime GUARD_PATHSPEC() check, which gives behavior like this:\n\n  $ git log --follow ':(icase)makefile' >/dev/null\n  BUG: tree-diff.c:596: unsupported magic 10\n  Aborted\n\nThe same is true of \":(glob)\", \":(attr)\", and so on. It's good that we\nnotice the problem rather than continuing and producing a wrong answer.\nBut there are two non-ideal things:\n\n  1. The idea of GUARD_PATHSPEC() is to catch programming errors where\n     low-level code gets unexpected pathspecs. We'd usually try to catch\n     unsupported pathspecs by passing a magic_mask to parse_pathspec(),\n     which would give the user a much better message like:\n\n       pathspec magic not supported by this command: 'icase'\n\n     That doesn't happen here because git-log usually _does_ support\n     all types of pathspec magic, and so it passes \"0\" for the mask\n     (this call actually happens in setup_revisions()). It needs to\n     distinguish the normal case from the \"--follow\" one but currently\n     doesn't.\n\n  2. In addition to --follow, we have the log.follow config option. When\n     that is set, we try to turn on --follow mode only when there is a\n     single pathspec (since --follow doesn't handle anything else). But\n     really, that ought to be expanded to \"use --follow when the\n     pathspec supports it\". Otherwise, we'd complain any time you use an\n     exotic pathspec:\n\n       $ git config log.follow true\n       $ git log ':(icase)makefile' >/dev/null\n       BUG: tree-diff.c:596: unsupported magic 10\n       Aborted\n\n     We should instead just avoid enabling follow mode if it's not\n     supported by this particular invocation.\n\nThis patch expands our diff_check_follow_pathspec() function to cover\npathspec magic, solving both problems.\n\nA few final notes:\n\n  - we could also solve (1) by passing the appropriate mask to\n    parse_pathspec(). But that's not great for two reasons. One is that\n    the error message is less precise. It says \"magic not supported by\n    this command\", but really it is not the command, but rather the\n    --follow option which is the problem. The second is that it always\n    calls die(). But for our log.follow code, we want to speculatively\n    ask \"is this pathspec OK?\" and just get a boolean result.\n\n  - This is obviously the right thing to do for ':(icase)' and most\n    other magic options. But ':(glob)' is a bit odd here. The --follow\n    code doesn't support wildcards, but we allow them anyway. From\n    try_to_follow_renames():\n\n\t#if 0\n\t        /*\n\t         * We should reject wildcards as well. Unfortunately we\n\t         * haven't got a reliable way to detect that 'foo\\*bar' in\n\t         * fact has no wildcards. nowildcard_len is merely a hint for\n\t         * optimization. Let it slip for now until wildmatch is taught\n\t         * about dry-run mode and returns wildcard info.\n\t         */\n\t        if (opt->pathspec.has_wildcard)\n\t                BUG(\"wildcards are not supported\");\n\t#endif\n\n    So something like \"git log --follow 'Make*'\" is already doing the\n    wrong thing, since \":(glob)\" behavior is already the default (it is\n    used only to countermand an earlier --noglob-pathspecs).\n\n    So we _could_ loosen the guard to allow :(glob), since it just\n    behaves the same as pathspecs do by default. But it seems like a\n    backwards step to do so. It already doesn't work (it hits the BUG()\n    case currently), and given that the user took an explicit step to\n    say \"this pathspec should glob\", it is reasonable for us to say \"no,\n    --follow does not support globbing\" (or in the case of log.follow,\n    avoid turning on follow mode). Which is what happens after this\n    patch.\n\n  - The set of allowed pathspec magic is obviously the same as in\n    GUARD_PATHSPEC(). We could perhaps factor these out to avoid\n    repetition. The point of having separate masks and GUARD calls is\n    that we don't necessarily know which parsed pathspecs will be used\n    where. But in this case, the two are heavily correlated. Still,\n    there may be some value in keeping them separate; it would make\n    anyone think twice about adding new magic to the list in\n    diff_check_follow_pathspec(). They'd need to touch\n    try_to_follow_renames() as well, which is the code that would\n    actually need to be updated to handle more exotic pathspecs.\n\n  - The documentation for log.follow says that it enables --follow\n    \"...when a single <path> is given\". We could possibly expand that to\n    say \"with no unsupported pathspec magic\", but that raises the\n    question of documenting which magic is supported. I think the\n    existing wording of \"single <path>\" sufficiently encompasses the\n    idea (the forbidden magic is stuff that might match multiple\n    entries), and the spirit remains the same.\n\nReported-by: Jim Pryor <dubiousjim@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c         | 15 +++++++++++++++\n t/t4202-log.sh | 15 +++++++++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 9f548f3471..8ca16fefe7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4753,11 +4753,26 @@ unsigned diff_filter_bit(char status)\n \n int diff_check_follow_pathspec(struct pathspec *ps, int die_on_error)\n {\n+\tunsigned forbidden_magic;\n+\n \tif (ps->nr != 1) {\n \t\tif (die_on_error)\n \t\t\tdie(_(\"--follow requires exactly one pathspec\"));\n \t\treturn 0;\n \t}\n+\n+\tforbidden_magic = ps->items[0].magic & ~(PATHSPEC_FROMTOP |\n+\t\t\t\t\t\t PATHSPEC_LITERAL);\n+\tif (forbidden_magic) {\n+\t\tif (die_on_error) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tpathspec_magic_names(forbidden_magic, &sb);\n+\t\t\tdie(_(\"pathspec magic not supported by --follow: %s\"),\n+\t\t\t    sb.buf);\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n \treturn 1;\n }\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex ae73aef922..5b54d7928e 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -187,6 +187,21 @@ test_expect_success 'git config log.follow does not die with no paths' '\n \tgit log --\n '\n \n+test_expect_success 'git log --follow rejects unsupported pathspec magic' '\n+\ttest_must_fail git log --follow \":(top,glob,icase)ichi\" 2>stderr &&\n+\t# check full error message; we want to be sure we mention both\n+\t# of the rejected types (glob,icase), but not the allowed one (top)\n+\techo \"fatal: pathspec magic not supported by --follow: ${SQ}glob${SQ}, ${SQ}icase${SQ}\" >expect &&\n+\ttest_cmp expect stderr\n+'\n+\n+test_expect_success 'log.follow disabled with unsupported pathspec magic' '\n+\ttest_config log.follow true &&\n+\tgit log --format=%s \":(glob,icase)ichi\" >actual &&\n+\techo third >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git config log.follow is overridden by --no-follow' '\n \ttest_config log.follow true &&\n \tgit log --no-follow --pretty=\"format:%s\" ichi >actual &&\n-- \n2.41.0.346.g8d12207a4f\n"},{"id":"477921","messageId":"xmqq4jnq9vee.fsf@gitster.g","threadId":"59797","inReplyTo":"20230601174351.GC4165297@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] diff: detect pathspec magic not supported by --follow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-02T07:27:05Z","receivedAt":"2023-06-02T07:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The --follow code doesn't handle most forms of pathspec magic. We check\n> that no unexpected ones have made it to try_to_follow_renames() with a\n> runtime GUARD_PATHSPEC() check, which gives behavior like this:\n>\n>   $ git log --follow ':(icase)makefile' >/dev/null\n>   BUG: tree-diff.c:596: unsupported magic 10\n>   Aborted\n\nStepping back a bit, isn't the real reason why follow mode is\nincompatible with the pathspec magic because it does not want to\ndeal with anything but a single literal pathname?\n\nIn other words, in \"git log --follow ':(icase)makefile'\", if the\ngiven pathspec matches a single path in the starting commit, we\nshould be able to start from the commit inspecting that uniquely\nfound path, compare the commit with its parent(s) at the path, and\nwhen the parent does not have the path, detect rename and continue\nfollowing that the name before the rename as the single path\nstarting from that parent.  Perhaps the commit we start at, HEAD,\nhas \"Makefile\" and not \"makefile\" (if we have two, that is an\nerror), so we start traversal, following the pathname \"Makefile\".\nAt some point, we may discover that the parent did not have\n\"Makefile\", but has \"GNUMakefile\" with a similar contents, at which\npoint we swap the path we follow from \"Makefile\" to \"GNUMakefile\".\n\nAt each step, tree-diff does not need to and does not want to do\nanything fancy with pathspec---the operation is always \"this path in\nthe parent, does it exist in the parent? If not, did it come from a\ndifferent path?\", and use of \"literal pathspec\" mode makes sense\nthere.\n\nSo why does try_to_follow_renames() even *need* to say anything\nother than \"We are in follow mode; what we have is a single path;\nuse it as a literal pathspec that matches the path literally without\nany funky magic\"?  GUARD_PATHSPEC() is \"please inspect the pathspec\nand see what kind of special pattern matching it requests---we\ncannot allow it to ask for certain things but do allow it to ask for\nother things\", that sounds terribly misguided here.  The string is\nliteral, so we do not even want to look at it to see what it\nasks---it is taken as nothing but a literal string and we do not let\nit ask for anything special.\n\nFor example, if we started at HEAD to follow Makefile and in an\nancestor commit we find that the Makefile came from a funny path\n\":(icase)foo\", instead of allowing the pathspec match code to\ninterprete it as a magic pathspec that matches any case permutations\nof 'f/o/o', we want to force the pathspec match code to match\nnothing but the funny path \"colon parens icase close parens foo\" by\nusing literal pathspec magic.\n\nSo perhaps having GUARD_PATHSPEC() there is the mistake, no?\n\nFor the above idea to work, I think we need to resolve the pathspec\nearly; \"log --follow\" should find its starting point, apply the\ngiven pathspec to its treeish to grab the literal pathname, and use\nthat single literal pathname as the literal pathspec and continue,\nwhich I do not think it does right now.  But with it done upfront,\nthe pathspec limiting done during the history traversal, including\ntry_to_follow_renames() bit, should be able to limit itself to \"is\nthe path literally the string given as the pathspec\"?\n\nI dunno.\n"},{"id":"478432","messageId":"20230615072625.GD1460739@coredump.intra.peff.net","threadId":"59797","inReplyTo":"xmqq4jnq9vee.fsf@gitster.g","subject":"Re: [PATCH 3/3] diff: detect pathspec magic not supported by --follow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-15T07:26:25Z","receivedAt":"2023-06-15T07:26:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2023 at 04:27:05PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The --follow code doesn't handle most forms of pathspec magic. We check\n> > that no unexpected ones have made it to try_to_follow_renames() with a\n> > runtime GUARD_PATHSPEC() check, which gives behavior like this:\n> >\n> >   $ git log --follow ':(icase)makefile' >/dev/null\n> >   BUG: tree-diff.c:596: unsupported magic 10\n> >   Aborted\n> \n> Stepping back a bit, isn't the real reason why follow mode is\n> incompatible with the pathspec magic because it does not want to\n> deal with anything but a single literal pathname?\n\nSorry, I'm a little behind on list reading; I know this topic has\nadvanced almost to 'master' in the meantime, but I had a few thoughts\nworth getting out.\n\nBasically, yes, I agree with everything in your email. My series is more\nor less papering over deficiencies in the --follow code, and that code\nwould be much better off thinking of single literal paths that it is\nfollowing.\n\nI was trying with my series not to go down the rabbit hole of --follow\ndeficiencies. So in that sense, I think it is still a good idea for the\nshort-term. And though we might perhaps revert some of it if we actually\nimprove how --follow works, I don't think it would be too hard to do so\nlater. But of course if we are going to improve --follow _now_, then it\ncould replace my series.\n\nAnd what you outlined here:\n\n> For the above idea to work, I think we need to resolve the pathspec\n> early; \"log --follow\" should find its starting point, apply the\n> given pathspec to its treeish to grab the literal pathname, and use\n> that single literal pathname as the literal pathspec and continue,\n> which I do not think it does right now.  But with it done upfront,\n> the pathspec limiting done during the history traversal, including\n> try_to_follow_renames() bit, should be able to limit itself to \"is\n> the path literally the string given as the pathspec\"?\n\nseems mostly sensible to me, except that the notion of \"starting point\"\nis ambiguous. If I do:\n\n  git log --follow branch1 branch2 -- foo*\n\nthen we have two treeishes. Do we look at one? Both? What happens if the\npathspec resolves differently?\n\nOff the top of my head, I'd say that we should look at all starting\ntips, resolve the pathspec in all of them, and complain if it doesn't\nresolve to the same single path in each one. Because it otherwise would\nproduce garbled results.\n\nTo be fair, this same garbled case already happens when traversing a\nmerge sends us down two paths of history, as a followed rename might be\npresent on one but not the other, and we keep only a single path. But as\nyou know, that is a long-time deficiency of --follow. :) At least\ndetecting it from the starting tips is easy and something the user can\ndeal with by modifying the command invocation.\n\nSo I dunno. That doesn't sound _too_ hard to do, though it probably also\nneeds a lot of the same infrastructure I introduced in my series. Namely\nthat log.follow needs to ask \"hey, is this pathspec usable for\nfollowing?\" so that it can quietly turn the feature off, rather than\ntriggering an error.\n\nSo I'd suggest to let my series continue graduating, and then if anybody\nwants to work on more sensible pathspec handling, to do so on top.\n\n-Peff\n"},{"id":"478446","messageId":"xmqqr0qcv9e6.fsf@gitster.g","threadId":"59797","inReplyTo":"20230615072625.GD1460739@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] diff: detect pathspec magic not supported by --follow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-15T19:00:01Z","receivedAt":"2023-06-15T19:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... except that the notion of \"starting point\"\n> is ambiguous. If I do:\n>\n>   git log --follow branch1 branch2 -- foo*\n>\n> then we have two treeishes. Do we look at one? Both? What happens if the\n> pathspec resolves differently?\n\nIf we consider a middle-ground where we do not keep a separate\npathname that each history traversal is following but only keep a\nsingle pathname, then the answer is one of \"ignore others and pick\none\" (which is essentially what needs to happen in the current\nimplementation when a path gets renamed between a child commit and\nits parent while the other parents do not have renames), \"ignore\nothers and pick one, but give a warning that the result may be\nincomplete\", or \"barf\".  The latter two would need us to look at all\ntreeishes.\n\nA false-positive-ridden \"solution\" that would slightly be better\nthan that would be to still keep a global, single, pathspec that has\nmore than one elements (all of which are literal pathspec elements).\nEvery time we discover a rename, we add the newly discovered old\nname as an additional element, instead of replacing the single\nelement.  If we were to go that route, then starting from multiple\npoints, each following a pathname, would be a simple matter of\nlooking at all trees and finding which pathname each history\ntraversal wants to follow and creating a pathspec with all these\n(literal) pathnames.\n\nBut once we fix the underlying problem, and start tracking which\npath each history traversal trajectory is currently following, then\nit is not a problem for each starting point to have a pathname that\nis being followed.  Each commit would _know_ which pathname it is\ninterested in, and when a traversal from a commit to its parent sees\na rename, the pathname being followed for the parent commit will be\ndifferent from the child.\n\nWhen this happens, the parent may have a pathname it is already\ninterested in, set by traversing from another child (i.e. one side\nof a fork renamed the path one way, while the other side of a fork\nkept the same name or renamed it to another way, and we traversed\nboth forks down and are now at the fork point).  It most likely\nshould be the same name we discovered from the other child, but in\nweird cases it may be different (e.g. a child thinks its path A came\nfrom parent's path X while another child thinks its path A came from\nparent's path Y), in which case we may have to follow two pathnames\nin the parent commit, as if the pathname we have been following in\nthe child had combined contents from multiple paths.\n\n> So I'd suggest to let my series continue graduating, and then if anybody\n> wants to work on more sensible pathspec handling, to do so on top.\n\nI think we are in agreement on this---otherwise I wouldn't have\nmerged the patch as-is down to 'next' ;-)\n\nAs I said multiple times in the past, the current \"log --follow\" is\nmore of a checkbox item than a feature, and in that context, I think\nyour \"fix\", which may be papering over just one obvious breakage, is\nwhat the codepath should be happy with, before it gets reworked more\nseriously.\n\nThanks.\n"}]}