threads / bug / 61005

Bug: diff --no-index with cachetextconv crashes

Subject: Bug: diff --no-index with cachetextconv crashes

## tl;dr

5 messages between Feb 26, 2024 and Feb 26, 2024.

replies: 4people: 3as markdown or json

Paweł Dominiak· Feb 26, 2024, 07:03 UTC · lore
Hey!

That's my first bug report for git and my first email to a mailing list in general, I hope for understanding :)

[Steps to reproduce your issue]
Global .gitattributes:
*.txt diff=test
Global .gitconfig:
[diff "test"]
    textconv = cat
    cachetextconv = true
Called command:
git --no-pager diff --no-index foo.txt bar.txt
[Expected behavior]
diff --git a/foo.txt b/bar.txt
index f6a4b70..2b24d27 100644
--- a/foo.txt
+++ b/bar.txt
@@ -1 +1 @@
-Foo bar baz
+Foo bar qux

[Actual behavior]

BUG: refs.c:2095: attempting to get main_ref_store outside of repository

The command works as expected if cachetextconv is disabled:

git --no-pager -c diff.test.cachetextconv=false diff --no-index foo.txt bar.txt

[System Info]
git version 2.44.0.windows.1
cpu: x86_64
built from commit: ad0bbfffa543db6979717be96df630d3e5741331
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19045
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe

[Enabled Hooks]
not run from a git repository - no hooks to show

Pawel
Jeff King· Feb 26, 2024, 07:22 UTC · re: Paweł Dominiak · lore

Re: Bug: diff --no-index with cachetextconv crashes

On Mon, Feb 26, 2024 at 08:03:04AM +0100, Paweł Dominiak wrote:
> That's my first bug report for git and my first email to a mailing
> list in general, I hope for understanding :)
Hi, welcome, and thanks for a clear bug report. :)
Show 15 quoted lines
> [Steps to reproduce your issue]
> 
> Global .gitattributes:
> 
> *.txt diff=test
> 
> Global .gitconfig:
> 
> [diff "test"]
>     textconv = cat
>     cachetextconv = true
> 
> Called command:
> 
> git --no-pager diff --no-index foo.txt bar.txt

OK, I would say that this failing is semi-expected. :) The caching system works using "git notes", which are stored in refs in the repository. And since you are running "diff --no-index" outside of a repository, there is nowhere to put them.

And so this BUG call makes sense:
> BUG: refs.c:2095: attempting to get main_ref_store outside of repository

We tried to load notes but there's no ref store at all, and the low-level ref code caught this.

Of course any time we see a BUG something has gone wrong. What I think _should_ happen is that we should quietly disable the caching (which, after all, is just an optimization) and otherwise complete the command.

So we'd probably want something like this:
diff --git a/userdiff.c b/userdiff.c
index e399543823..fce3a31efa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -3,6 +3,7 @@
 #include "userdiff.h"
 #include "attr.h"
 #include "strbuf.h"
+#include "environment.h"
 
 static struct userdiff_driver *drivers;
 static int ndrivers;
@@ -460,7 +461,8 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,
 	if (!driver->textconv)
 		return NULL;
 
-	if (driver->textconv_want_cache && !driver->textconv_cache) {
+	if (driver->textconv_want_cache && !driver->textconv_cache &&
+	    have_git_dir()) {
 		struct notes_cache *c = xmalloc(sizeof(*c));
 		struct strbuf name = STRBUF_INIT;
 

-Peff
Paweł Dominiak· Feb 26, 2024, 07:56 UTC · re: Jeff King · lore

Re: Bug: diff --no-index with cachetextconv crashes

> OK, I would say that this failing is semi-expected. :) The caching
> system works using "git notes", which are stored in refs in the
> repository. And since you are running "diff --no-index" outside of a
> repository, there is nowhere to put them.

I have not mentioned this specifically, but my goal is a general diff command, which internally uses text conversions, pager etc. as configured for git. It makes sense to cache the textconv results when used in a repository, but I don't think it should fail when not in one. I think the default behavior should be to silently skip caching in such situations but produce a diff otherwise.

> Of course any time we see a BUG something has gone wrong. What I think
> _should_ happen is that we should quietly disable the caching (which,
> after all, is just an optimization) and otherwise complete the command.
In my script I currently disable caching explicitly for all drivers:

keys=$(git config --name-only --get-regexp '^diff\.\w+\.cachetextconv$') config=(); for key in $keys; do config+=(-c "$key=false"); done

git "${config[@]}" diff --no-index --no-prefix "$@"

But it seems like something git should handle on its own, so that diff would accommodate use in different circumstances with the same config.

-Pawel
Jeff King· Feb 26, 2024, 10:27 UTC · re: Paweł Dominiak · lore

[PATCH] userdiff: skip textconv caching when not in a repository

On Mon, Feb 26, 2024 at 08:56:09AM +0100, Paweł Dominiak wrote:
> But it seems like something git should handle on its own, so that diff
> would accommodate use in different circumstances with the same config.
Yes, definitely. Here's my patch with a commit message and a test.
-- >8 --
Subject: userdiff: skip textconv caching when not in a repository

The textconv caching system uses git-notes to store its cache entries. But if you're using "diff --no-index" outside of a repository, then obviously that isn't going to work.

Since caching is just an optimization, it's OK for us to skip it. However, the current behavior is much worse: we call notes_cache_init() which tries to look up the ref, and the low-level ref code hits a BUG(), killing the program. Instead, we should notice before setting up the cache that it there's no repository, and just silently skip it.

Reported-by: Paweł Dominiak <dominiak.pawel@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4042-diff-textconv-caching.sh | 22 ++++++++++++++++++++++
 userdiff.c                       |  4 +++-
 2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index bf33aedf4b..8ebfa3c1be 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -118,4 +118,26 @@ test_expect_success 'log notes cache and still use cache for -p' '
 	git log --no-walk -p refs/notes/textconv/magic HEAD
 '
 
+test_expect_success 'caching is silently ignored outside repo' '
+	mkdir -p non-repo &&
+	echo one >non-repo/one &&
+	echo two >non-repo/two &&
+	echo "* diff=test" >attr &&
+	test_expect_code 1 \
+	nongit git -c core.attributesFile="$PWD/attr" \
+		   -c diff.test.textconv="tr a-z A-Z <" \
+		   -c diff.test.cachetextconv=true \
+		   diff --no-index one two >actual &&
+	cat >expect <<-\EOF &&
+	diff --git a/one b/two
+	index 5626abf..f719efd 100644
+	--- a/one
+	+++ b/two
+	@@ -1 +1 @@
+	-ONE
+	+TWO
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/userdiff.c b/userdiff.c
index e399543823..fce3a31efa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -3,6 +3,7 @@
 #include "userdiff.h"
 #include "attr.h"
 #include "strbuf.h"
+#include "environment.h"
 
 static struct userdiff_driver *drivers;
 static int ndrivers;
@@ -460,7 +461,8 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,
 	if (!driver->textconv)
 		return NULL;
 
-	if (driver->textconv_want_cache && !driver->textconv_cache) {
+	if (driver->textconv_want_cache && !driver->textconv_cache &&
+	    have_git_dir()) {
 		struct notes_cache *c = xmalloc(sizeof(*c));
 		struct strbuf name = STRBUF_INIT;
 
-- 
2.44.0.rc2.424.gbdbf4d014b
Junio C Hamano· Feb 26, 2024, 16:35 UTC · re: Jeff King · lore

Re: [PATCH] userdiff: skip textconv caching when not in a repository

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> Subject: userdiff: skip textconv caching when not in a repository
>
> The textconv caching system uses git-notes to store its cache entries.
> But if you're using "diff --no-index" outside of a repository, then
> obviously that isn't going to work.
>
> Since caching is just an optimization, it's OK for us to skip it.
> However, the current behavior is much worse: we call notes_cache_init()
> which tries to look up the ref, and the low-level ref code hits a BUG(),
> killing the program. Instead, we should notice before setting up the
> cache that it there's no repository, and just silently skip it.
Makes sense.
Will queue.  Thanks.

← back to recent threads