Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid
- From
Kirill Smelkov <kirr@landau.phys.spbu.ru>
- Date
- Dec 19, 2010, 12:10 UTC
- Message-ID
- <20101219121059.GA10985@landau.phys.spbu.ru>
- In-Reply-To
- <7vk4j6fnta.fsf@alter.siamese.dyndns.org>
On Sat, Dec 18, 2010 at 07:23:29PM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes: > > > Thanks for your ACK and for the explanation. > > > > My last patches to git were blame related so semi-intuitively I knew > > that invalid sha1 are coming from files in worktree. Your description > > makes things much more clear and I'd put it into patch log as well. > > What is the best practice for this? For me to re-roll, or for Junio to > > merge texts? > > Re-rolling to explain changes in your own words is preferred; thanks.
I see, thanks.
I'm not that familiar with git internals involved, so here is updated patch with added paragraph about "df->sha1_valid=0 means files from worktree with unknown sha1", and appropriate excerpt from Jeff's post. That's the most reasonable I could come up with.
Thanks, Kirill
P.S. please don't forget to pick patch 1 which is unchanged.
---- 8< ----
From: Kirill Smelkov <kirr@landau.phys.spbu.ru> Date: Sat, 18 Dec 2010 16:27:28 +0300 Subject: [PATCH v2 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
It turned out, under blame there are requests to fill_textconv() with sha1=0000000000000000000000000000000000000000 and sha1_valid=0.
As the code did not analyzed sha1 validity, we ended up putting 000000 into textconv cache which was fooling later blames to discover lots of lines in 'Not Yet Committed' state.
In practice df->sha1_valid=0 means blame requests to run textconv on a file in worktree whose sha1 is not know yet.
Fix it.
On Sat, Dec 18, 2010 at 11:13:37AM -0500, Jeff King wrote:
Show 23 quoted lines
> > In short: > > Acked-by: Jeff King <peff@peff.net> > > But it took some thinking to convince myself, so the long answer is > below if anyone cares. > > I was dubious at first that this could be the right solution. We still > end up putting the filespec through run_textconv, which didn't seem > right if it is not valid. > > But reading into it more, there are two levels of invalidity: > > 1. !DIFF_FILE_VALID(df) - we are not a valid file at all. I.e., we are > /dev/null. > > 2. !df->sha1_valid - we are pointing to a working tree file whose sha1 > we don't know > > I think level (2) never happens at all in the regular diff code, which > is why this case was completely unhandled. But it is OK in that case > (required, even) to put the contents through run_textconv.
Cc: Axel Bonnet <axel.bonnet@ensimag.imag.fr> Cc: Clц╘ment Poulain <clement.poulain@ensimag.imag.fr> Cc: Diane Gasselin <diane.gasselin@ensimag.imag.fr> Cc: Jeff King <peff@peff.net> Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru> Acked-by: Jeff King <peff@peff.net> --- diff.c | 4 ++-- t/t8006-blame-textconv.sh | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c index 0a43869..5422c43 100644 --- a/diff.c +++ b/diff.c @@ -4412,7 +4412,7 @@ size_t fill_textconv(struct userdiff_driver *driver, return df->size; } - if (driver->textconv_cache) { + if (driver->textconv_cache && df->sha1_valid) { *outbuf = notes_cache_get(driver->textconv_cache, df->sha1, &size); if (*outbuf) @@ -4423,7 +4423,7 @@ size_t fill_textconv(struct userdiff_driver *driver, if (!*outbuf) die("unable to read files to diff"); - if (driver->textconv_cache) { + if (driver->textconv_cache && df->sha1_valid) { /* ignore errors, as we might be in a readonly repository */ notes_cache_put(driver->textconv_cache, df->sha1, *outbuf, size); diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh index fe90541..ea64cd8 100755 --- a/t/t8006-blame-textconv.sh +++ b/t/t8006-blame-textconv.sh @@ -81,8 +81,7 @@ cat >expected_one <<EOF (Number2 2010-01-01 20:00:00 +0000 1) converted: test 1 version 2 EOF -# one.bin is blamed as 'Not Committed yet' -test_expect_failure 'blame --textconv works with textconvcache' ' +test_expect_success 'blame --textconv works with textconvcache' ' git blame --textconv two.bin >blame && find_blame <blame >result && test_cmp expected result &&
-- 1.7.3.4.570.g14308