From: Jeff King <peff@peff.net>
To: larsxschneider@gmail.com
Cc: git@vger.kernel.org
Subject: [PATCH] diff: do not reuse worktree files that need "clean" conversion
Date: Fri, 22 Jul 2016 11:27:53 -0400 [thread overview]
Message-ID: <20160722152753.GA6859@sigill.intra.peff.net> (raw)
In-Reply-To: <20160721213740.GB4604@sigill.intra.peff.net>
On Thu, Jul 21, 2016 at 05:37:40PM -0400, Jeff King wrote:
> The next three are to show the final diffstat after the commit
> completes. It looks like the "should we reuse the worktree file?"
> optimization in diff_populate_filespec() does not take into account
> whether we will have to convert the contents, and it probably should.
> The point is that sometimes mmap-ing the file contents is more efficient
> than zlib inflating the object from disk. But if we have to exec an
> extra process and read the whole object contents into a strbuf, that is
> probably not a win.
>
> Adding a "return 0" at the top of reuse_worktree_file() drops our 6 to
> 3 (but obviously it should actually check the attributes).
Here's a patch for that. I put it in t0021, as well, since I couldn't
find a more appropriate place. This means it conflicts with your earlier
patch, but I think it does a better job of addressing the specific area
it fixes.
We may want further tests on top as we fix more areas (though as I said
earlier, I think by avoiding the racy timestamps, your "commit" call
drops to just a single invocation per file. That still seems like one
more than we should need, but it at least matches your original
expectation :) ).
-- >8 --
Subject: diff: do not reuse worktree files that need "clean" conversion
When accessing a blob for a diff, we may try to reuse file
contents in the working tree, under the theory that it is
faster to mmap those file contents than it would be to
extract the content from the object database.
When we have to filter those contents, though, that
assumption does not hold. Even for our internal conversions
like CRLF, we have to allocate and fill a new buffer anyway.
But much worse, for external clean filters we have to exec
an arbitrary script, and we have no idea how expensive it
may be to run.
So let's skip this optimization when conversion into git's
"clean" form is required. This applies whenever the
"want_file" flag is false. When it's true, the caller
actually wants the smudged worktree contents, which the
reused file by definition already has (in fact, this is a
key optimization going the other direction, since reusing
the worktree file there lets us skip smudge filters).
Signed-off-by: Jeff King <peff@peff.net>
---
diff.c | 7 +++++++
t/t0021-conversion.sh | 11 +++++++++++
2 files changed, 18 insertions(+)
diff --git a/diff.c b/diff.c
index 7d03419..b43d3dd 100644
--- a/diff.c
+++ b/diff.c
@@ -2683,6 +2683,13 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
return 0;
+ /*
+ * Similarly, if we'd have to convert the file contents anyway, that
+ * makes the optimization not worthwhile.
+ */
+ if (!want_file && would_convert_to_git(name))
+ return 0;
+
len = strlen(name);
pos = cache_name_pos(name, len);
if (pos < 0)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 7bac2bc..e799e59 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -268,4 +268,15 @@ test_expect_success 'disable filter with empty override' '
test_must_be_empty err
'
+test_expect_success 'diff does not reuse worktree files that need cleaning' '
+ test_config filter.counter.clean "echo . >>count; sed s/^/clean:/" &&
+ echo "file filter=counter" >.gitattributes &&
+ test_commit one file &&
+ test_commit two file &&
+
+ >count &&
+ git diff-tree -p HEAD &&
+ test_line_count = 0 count
+'
+
test_done
--
2.9.2.506.g8452fe7
next prev parent reply other threads:[~2016-07-22 15:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-21 20:59 [PATCH v1] convert: add test to demonstrate clean invocations larsxschneider
2016-07-21 21:37 ` Jeff King
2016-07-22 15:27 ` Jeff King [this message]
2016-07-22 17:44 ` [PATCH] diff: do not reuse worktree files that need "clean" conversion Junio C Hamano
2016-07-22 18:10 ` Jeff King
2016-07-22 19:30 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160722152753.GA6859@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=larsxschneider@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).