{"thread":{"id":"61005","subject":"Bug: diff --no-index with cachetextconv crashes","startedAt":"2024-02-26T07:03:12Z","lastAt":"2024-02-26T16:35:33Z","messageCount":5,"participants":["Paweł Dominiak","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"489351","messageId":"CACeVQwQ4MELjB8nZyeu9QDTtgwhhw0oOsL8BHdm_rxTj1vMy+A@mail.gmail.com","threadId":"61005","inReplyTo":null,"subject":"Bug: diff --no-index with cachetextconv crashes","fromName":"Paweł Dominiak","fromEmail":"dominiak.pawel@gmail.com","sentAt":"2024-02-26T07:03:04Z","receivedAt":"2024-02-26T07:03:12Z","isPatch":false,"sender":{"key":"dominiak.pawel@gmail.com","avatar":null},"body":"Hey!\n\nThat's my first bug report for git and my first email to a mailing\nlist in general, I hope for understanding :)\n\n[Steps to reproduce your issue]\n\nGlobal .gitattributes:\n\n*.txt diff=test\n\nGlobal .gitconfig:\n\n[diff \"test\"]\n    textconv = cat\n    cachetextconv = true\n\nCalled command:\n\ngit --no-pager diff --no-index foo.txt bar.txt\n\n[Expected behavior]\n\ndiff --git a/foo.txt b/bar.txt\nindex f6a4b70..2b24d27 100644\n--- a/foo.txt\n+++ b/bar.txt\n@@ -1 +1 @@\n-Foo bar baz\n+Foo bar qux\n\n[Actual behavior]\n\nBUG: refs.c:2095: attempting to get main_ref_store outside of repository\n\nThe command works as expected if cachetextconv is disabled:\n\ngit --no-pager -c diff.test.cachetextconv=false diff --no-index foo.txt bar.txt\n\n[System Info]\ngit version 2.44.0.windows.1\ncpu: x86_64\nbuilt from commit: ad0bbfffa543db6979717be96df630d3e5741331\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Windows 10.0 19045\ncompiler info: gnuc: 13.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n\nPawel\n"},{"id":"489353","messageId":"20240226072248.GC780982@coredump.intra.peff.net","threadId":"61005","inReplyTo":"CACeVQwQ4MELjB8nZyeu9QDTtgwhhw0oOsL8BHdm_rxTj1vMy+A@mail.gmail.com","subject":"Re: Bug: diff --no-index with cachetextconv crashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T07:22:48Z","receivedAt":"2024-02-26T07:22:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2024 at 08:03:04AM +0100, Paweł Dominiak wrote:\n\n> That's my first bug report for git and my first email to a mailing\n> list in general, I hope for understanding :)\n\nHi, welcome, and thanks for a clear bug report. :)\n\n> [Steps to reproduce your issue]\n> \n> Global .gitattributes:\n> \n> *.txt diff=test\n> \n> Global .gitconfig:\n> \n> [diff \"test\"]\n>     textconv = cat\n>     cachetextconv = true\n> \n> Called command:\n> \n> git --no-pager diff --no-index foo.txt bar.txt\n\nOK, I would say that this failing is semi-expected. :) The caching\nsystem works using \"git notes\", which are stored in refs in the\nrepository. And since you are running \"diff --no-index\" outside of a\nrepository, there is nowhere to put them.\n\nAnd so this BUG call makes sense:\n\n> BUG: refs.c:2095: attempting to get main_ref_store outside of repository\n\nWe tried to load notes but there's no ref store at all, and the\nlow-level ref code caught this.\n\nOf course any time we see a BUG something has gone wrong. What I think\n_should_ happen is that we should quietly disable the caching (which,\nafter all, is just an optimization) and otherwise complete the command.\n\nSo we'd probably want something like this:\n\ndiff --git a/userdiff.c b/userdiff.c\nindex e399543823..fce3a31efa 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -3,6 +3,7 @@\n #include \"userdiff.h\"\n #include \"attr.h\"\n #include \"strbuf.h\"\n+#include \"environment.h\"\n \n static struct userdiff_driver *drivers;\n static int ndrivers;\n@@ -460,7 +461,8 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,\n \tif (!driver->textconv)\n \t\treturn NULL;\n \n-\tif (driver->textconv_want_cache && !driver->textconv_cache) {\n+\tif (driver->textconv_want_cache && !driver->textconv_cache &&\n+\t    have_git_dir()) {\n \t\tstruct notes_cache *c = xmalloc(sizeof(*c));\n \t\tstruct strbuf name = STRBUF_INIT;\n \n\n-Peff\n"},{"id":"489355","messageId":"CACeVQwRkoJejrWvym_BAZE-OWi+q3RZfyEpwAwA+0hGUBCeD=Q@mail.gmail.com","threadId":"61005","inReplyTo":"20240226072248.GC780982@coredump.intra.peff.net","subject":"Re: Bug: diff --no-index with cachetextconv crashes","fromName":"Paweł Dominiak","fromEmail":"dominiak.pawel@gmail.com","sentAt":"2024-02-26T07:56:09Z","receivedAt":"2024-02-26T07:56:17Z","isPatch":false,"sender":{"key":"dominiak.pawel@gmail.com","avatar":null},"body":"> OK, I would say that this failing is semi-expected. :) The caching\n> system works using \"git notes\", which are stored in refs in the\n> repository. And since you are running \"diff --no-index\" outside of a\n> repository, there is nowhere to put them.\n\nI have not mentioned this specifically, but my goal is a general diff\ncommand, which internally uses text conversions, pager etc. as\nconfigured for git.\nIt makes sense to cache the textconv results when used in a\nrepository, but I don't think it should fail when not in one.\nI think the default behavior should be to silently skip caching in\nsuch situations but produce a diff otherwise.\n\n> Of course any time we see a BUG something has gone wrong. What I think\n> _should_ happen is that we should quietly disable the caching (which,\n> after all, is just an optimization) and otherwise complete the command.\n\nIn my script I currently disable caching explicitly for all drivers:\n\nkeys=$(git config --name-only --get-regexp '^diff\\.\\w+\\.cachetextconv$')\nconfig=(); for key in $keys; do config+=(-c \"$key=false\"); done\n\ngit \"${config[@]}\" diff --no-index --no-prefix \"$@\"\n\nBut it seems like something git should handle on its own, so that diff\nwould accommodate use in different circumstances with the same config.\n\n-Pawel\n"},{"id":"489367","messageId":"20240226102729.GB2685773@coredump.intra.peff.net","threadId":"61005","inReplyTo":"CACeVQwRkoJejrWvym_BAZE-OWi+q3RZfyEpwAwA+0hGUBCeD=Q@mail.gmail.com","subject":"[PATCH] userdiff: skip textconv caching when not in a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T10:27:29Z","receivedAt":"2024-02-26T10:27:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2024 at 08:56:09AM +0100, Paweł Dominiak wrote:\n\n> But it seems like something git should handle on its own, so that diff\n> would accommodate use in different circumstances with the same config.\n\nYes, definitely. Here's my patch with a commit message and a test.\n\n-- >8 --\nSubject: userdiff: skip textconv caching when not in a repository\n\nThe textconv caching system uses git-notes to store its cache entries.\nBut if you're using \"diff --no-index\" outside of a repository, then\nobviously that isn't going to work.\n\nSince caching is just an optimization, it's OK for us to skip it.\nHowever, the current behavior is much worse: we call notes_cache_init()\nwhich tries to look up the ref, and the low-level ref code hits a BUG(),\nkilling the program. Instead, we should notice before setting up the\ncache that it there's no repository, and just silently skip it.\n\nReported-by: Paweł Dominiak <dominiak.pawel@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t4042-diff-textconv-caching.sh | 22 ++++++++++++++++++++++\n userdiff.c                       |  4 +++-\n 2 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex bf33aedf4b..8ebfa3c1be 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -118,4 +118,26 @@ test_expect_success 'log notes cache and still use cache for -p' '\n \tgit log --no-walk -p refs/notes/textconv/magic HEAD\n '\n \n+test_expect_success 'caching is silently ignored outside repo' '\n+\tmkdir -p non-repo &&\n+\techo one >non-repo/one &&\n+\techo two >non-repo/two &&\n+\techo \"* diff=test\" >attr &&\n+\ttest_expect_code 1 \\\n+\tnongit git -c core.attributesFile=\"$PWD/attr\" \\\n+\t\t   -c diff.test.textconv=\"tr a-z A-Z <\" \\\n+\t\t   -c diff.test.cachetextconv=true \\\n+\t\t   diff --no-index one two >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/one b/two\n+\tindex 5626abf..f719efd 100644\n+\t--- a/one\n+\t+++ b/two\n+\t@@ -1 +1 @@\n+\t-ONE\n+\t+TWO\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/userdiff.c b/userdiff.c\nindex e399543823..fce3a31efa 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -3,6 +3,7 @@\n #include \"userdiff.h\"\n #include \"attr.h\"\n #include \"strbuf.h\"\n+#include \"environment.h\"\n \n static struct userdiff_driver *drivers;\n static int ndrivers;\n@@ -460,7 +461,8 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,\n \tif (!driver->textconv)\n \t\treturn NULL;\n \n-\tif (driver->textconv_want_cache && !driver->textconv_cache) {\n+\tif (driver->textconv_want_cache && !driver->textconv_cache &&\n+\t    have_git_dir()) {\n \t\tstruct notes_cache *c = xmalloc(sizeof(*c));\n \t\tstruct strbuf name = STRBUF_INIT;\n \n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489384","messageId":"xmqqle77dvzj.fsf@gitster.g","threadId":"61005","inReplyTo":"20240226102729.GB2685773@coredump.intra.peff.net","subject":"Re: [PATCH] userdiff: skip textconv caching when not in a repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T16:35:28Z","receivedAt":"2024-02-26T16:35:33Z","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> Subject: userdiff: skip textconv caching when not in a repository\n>\n> The textconv caching system uses git-notes to store its cache entries.\n> But if you're using \"diff --no-index\" outside of a repository, then\n> obviously that isn't going to work.\n>\n> Since caching is just an optimization, it's OK for us to skip it.\n> However, the current behavior is much worse: we call notes_cache_init()\n> which tries to look up the ref, and the low-level ref code hits a BUG(),\n> killing the program. Instead, we should notice before setting up the\n> cache that it there's no repository, and just silently skip it.\n\nMakes sense.\n\nWill queue.  Thanks.\n"}]}