On Sat, Dec 18, 2010 at 07:23:29PM -0800, Junio C Hamano wrote:
> Kirill Smelkov <kirr@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
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:
>
> In short:
>
> Acked-by: Jeff King <peff@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
Cc: ClÑâment Poulain <clement.poulain@xxxxxxxxxxxxxxx>
Cc: Diane Gasselin <diane.gasselin@xxxxxxxxxxxxxxx>
Cc: Jeff King <peff@xxxxxxxx>
Signed-off-by: Kirill Smelkov <kirr@xxxxxxxxxxxxxxxxxxx>
Acked-by: Jeff King <peff@xxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html