{"thread":{"id":"18446","subject":"[PATCH] Respect core.autocrlf when preparing temporary files for external diff","startedAt":"2009-03-21T11:42:52Z","lastAt":"2009-03-23T00:39:47Z","messageCount":14,"participants":["Johannes Schindelin","Michael J Gruber","Junio C Hamano","Jeff King","Sebastian Schuberth"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108792","messageId":"8cb424b16f21164ddc26d0be3f6f7727254b3506.1237635609u.git.johannes.schindelin@gmx.de","threadId":"18446","inReplyTo":"cover.1237635609u.git.johannes.schindelin@gmx.de","subject":"[PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-21T11:42:52Z","receivedAt":"2009-03-21T11:42:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When preparing temporary files for an external diff, the files should be\nhandled as if they were worktree files.\n\nThis makes things consistent for the case when one side of the diff was\nfound in the current working directory (and therefore creating a temporary\nfile could be avoided altogether).\n\nThis fixes msysGit issue 177.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 13 ++++++++++---\n t/t4020-diff-external.sh | 16 ++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 75d9fab..699ae6a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1946,17 +1946,23 @@ void diff_free_filespec_data(struct diff_filespec *s)\n \ts->cnt_data = NULL;\n }\n \n-static void prep_temp_blob(struct diff_tempfile *temp,\n+static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \t\t\t void *blob,\n \t\t\t unsigned long size,\n \t\t\t const unsigned char *sha1,\n \t\t\t int mode)\n {\n \tint fd;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n \tif (fd < 0)\n \t\tdie(\"unable to create temp-file: %s\", strerror(errno));\n+\tif (convert_to_working_tree(path,\n+\t\t\t(const char *)blob, (size_t)size, &buf)) {\n+\t\tblob = buf.buf;\n+\t\tsize = buf.len;\n+\t}\n \tif (write_in_full(fd, blob, size) != size)\n \t\tdie(\"unable to write temp-file\");\n \tclose(fd);\n@@ -1964,6 +1970,7 @@ static void prep_temp_blob(struct diff_tempfile *temp,\n \tstrcpy(temp->hex, sha1_to_hex(sha1));\n \ttemp->hex[40] = 0;\n \tsprintf(temp->mode, \"%06o\", mode);\n+\tstrbuf_release(&buf);\n }\n \n static struct diff_tempfile *prepare_temp_file(const char *name,\n@@ -2004,7 +2011,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t\t\tdie(\"readlink(%s)\", name);\n \t\t\tif (ret == sizeof(buf))\n \t\t\t\tdie(\"symlink too long: %s\", name);\n-\t\t\tprep_temp_blob(temp, buf, ret,\n+\t\t\tprep_temp_blob(name, temp, buf, ret,\n \t\t\t\t (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n \t\t\t\t (one->sha1_valid ?\n@@ -2030,7 +2037,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \telse {\n \t\tif (diff_populate_filespec(one, 0))\n \t\t\tdie(\"cannot read data blob for %s\", one->path);\n-\t\tprep_temp_blob(temp, one->data, one->size,\n+\t\tprep_temp_blob(name, temp, one->data, one->size,\n \t\t\t one->sha1, one->mode);\n \t}\n \treturn temp;\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 281680d..f8c99f1 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -136,4 +136,20 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '\n \tGIT_EXTERNAL_DIFF=echo git diff\n '\n \n+echo \"#!$SHELL_PATH\" >fake-diff.sh\n+cat >> fake-diff.sh <<\\EOF\n+cat $2 >> crlfed.txt\n+EOF\n+chmod a+x fake-diff.sh\n+\n+keep_only_cr () {\n+\ttr -dc '\\015'\n+}\n+\n+test_expect_success 'external diff with autocrlf = true' '\n+\tgit config core.autocrlf true &&\n+\tGIT_EXTERNAL_DIFF=./fake-diff.sh git diff &&\n+\ttest $(wc -l < crlfed.txt) = $(cat crlfed.txt | keep_only_cr | wc -c)\n+'\n+\n test_done\n-- \n1.6.2.1.493.g67cf3\n"},{"id":"108799","messageId":"49C51DB4.4010103@drmicha.warpmail.net","threadId":"18446","inReplyTo":"8cb424b16f21164ddc26d0be3f6f7727254b3506.1237635609u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-03-21T17:02:44Z","receivedAt":"2009-03-21T17:02:44Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Schindelin venit, vidit, dixit 21.03.2009 12:42:\n> When preparing temporary files for an external diff, the files should be\n> handled as if they were worktree files.\n> \n> This makes things consistent for the case when one side of the diff was\n> found in the current working directory (and therefore creating a temporary\n> file could be avoided altogether).\n> \n> This fixes msysGit issue 177.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> diff.c | 13 ++++++++++---\n> t/t4020-diff-external.sh | 16 ++++++++++++++++\n> 2 files changed, 26 insertions(+), 3 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 75d9fab..699ae6a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1946,17 +1946,23 @@ void diff_free_filespec_data(struct diff_filespec *s)\n> \ts->cnt_data = NULL;\n> }\n> \n> -static void prep_temp_blob(struct diff_tempfile *temp,\n> +static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n> \t\t\t void *blob,\n> \t\t\t unsigned long size,\n> \t\t\t const unsigned char *sha1,\n> \t\t\t int mode)\n> {\n> \tint fd;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \n> \tfd = git_mkstemp(temp->tmp_path, PATH_MAX, \".diff_XXXXXX\");\n> \tif (fd < 0)\n> \t\tdie(\"unable to create temp-file: %s\", strerror(errno));\n> +\tif (convert_to_working_tree(path,\n> +\t\t\t(const char *)blob, (size_t)size, &buf)) {\n> +\t\tblob = buf.buf;\n> +\t\tsize = buf.len;\n> +\t}\n> \tif (write_in_full(fd, blob, size) != size)\n> \t\tdie(\"unable to write temp-file\");\n> \tclose(fd);\n> @@ -1964,6 +1970,7 @@ static void prep_temp_blob(struct diff_tempfile *temp,\n> \tstrcpy(temp->hex, sha1_to_hex(sha1));\n> \ttemp->hex[40] = 0;\n> \tsprintf(temp->mode, \"%06o\", mode);\n> +\tstrbuf_release(&buf);\n> }\n> \n> static struct diff_tempfile *prepare_temp_file(const char *name,\n> @@ -2004,7 +2011,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n> \t\t\t\tdie(\"readlink(%s)\", name);\n> \t\t\tif (ret == sizeof(buf))\n> \t\t\t\tdie(\"symlink too long: %s\", name);\n> -\t\t\tprep_temp_blob(temp, buf, ret,\n> +\t\t\tprep_temp_blob(name, temp, buf, ret,\n> \t\t\t\t (one->sha1_valid ?\n> \t\t\t\t\tone->sha1 : null_sha1),\n> \t\t\t\t (one->sha1_valid ?\n> @@ -2030,7 +2037,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n> \telse {\n> \t\tif (diff_populate_filespec(one, 0))\n> \t\t\tdie(\"cannot read data blob for %s\", one->path);\n> -\t\tprep_temp_blob(temp, one->data, one->size,\n> +\t\tprep_temp_blob(name, temp, one->data, one->size,\n> \t\t\t one->sha1, one->mode);\n> \t}\n> \treturn temp;\n> diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\n> index 281680d..f8c99f1 100755\n> --- a/t/t4020-diff-external.sh\n> +++ b/t/t4020-diff-external.sh\n> @@ -136,4 +136,20 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '\n> \tGIT_EXTERNAL_DIFF=echo git diff\n> '\n> \n> +echo \"#!$SHELL_PATH\" >fake-diff.sh\n> +cat >> fake-diff.sh <<\\EOF\n> +cat $2 >> crlfed.txt\n> +EOF\n> +chmod a+x fake-diff.sh\n> +\n> +keep_only_cr () {\n> +\ttr -dc '\\015'\n> +}\n> +\n> +test_expect_success 'external diff with autocrlf = true' '\n> +\tgit config core.autocrlf true &&\n> +\tGIT_EXTERNAL_DIFF=./fake-diff.sh git diff &&\n> +\ttest $(wc -l < crlfed.txt) = $(cat crlfed.txt | keep_only_cr | wc -c)\n> +'\n> +\n> test_done\n\nNice! This fixes also an issue I reported earlier: When\ndiff.foo.textconv is set, the textconv filter is fed with cleaned files\nsome times, with smudged files other times. Now it's always fed with\nsmudged files. Back then I even suspected crlf problems.\n\nMichael\n"},{"id":"108809","messageId":"7vocvuekjb.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"8cb424b16f21164ddc26d0be3f6f7727254b3506.1237635609u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-21T19:35:36Z","receivedAt":"2009-03-21T19:35:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> When preparing temporary files for an external diff, the files should be\n> handled as if they were worktree files.\n\nI do not think so. convert_to_working_tree() aka \"smudge\" means you would\nbe feeding crap like $Id$ expansion to the external diff, which we chose\nnot to do quite on purpose.\n\nI think the right solution is to filter inside GIT_EXTERNAL_DIFF *if* the\ndiff tool you are dispatching into from the external diff script wants to\ntake smudged representation. I know we don't have such a command right\nnow, but something like\n\n\tgit filter --take-attr-for-path=$path --direction=out smudged\n\tgit filter --take-attr-for-path=$path --direction=in >clean > Johannes Schindelin writes:\n> \n> > When preparing temporary files for an external diff, the files should \n> > be handled as if they were worktree files.\n> \n> I do not think so. convert_to_working_tree() aka \"smudge\" means you \n> would be feeding crap like $Id$ expansion to the external diff, which we \n> chose not to do quite on purpose.\n\nYou might have missed me mentioning that we often can do without temporary \nfiles, taking the working directory copies right away?\n\nAnd if you think about it, it makes complete sense.\n\nCiao,\nDscho\n"},{"id":"108859","messageId":"7v8wmybf06.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"alpine.DEB.1.00.0903220053260.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T00:03:21Z","receivedAt":"2009-03-22T00:03:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sat, 21 Mar 2009, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > When preparing temporary files for an external diff, the files should \n>> > be handled as if they were worktree files.\n>> \n>> I do not think so. convert_to_working_tree() aka \"smudge\" means you \n>> would be feeding crap like $Id$ expansion to the external diff, which we \n>> chose not to do quite on purpose.\n>\n> You might have missed me mentioning that we often can do without temporary \n> files, taking the working directory copies right away?\n>\n> And if you think about it, it makes complete sense.\n\nNot \"complete\".\n\nWhat is at issue is how consistent the codepath that calls out to an\nexternal diff should be with the rest of git that solely work with the\n\"clean\" version of blob contents. If you value consistency, I would say\nthat it is valid to argue that letting it borrow from a work tree is a\nbug. It should be always feeding a clean version.\n\nBut if you think that we do not care really about the correctness of the\nexternal diff codepath, because it is a mechanism used mostly for giving\noutput to humans (as opposed to feeding the output of the external diff\nprogram back to programs to be processed further) , I can understand why\nyou think it is easier to the external programs if \"smudged\" version is\nfed to them.\n\nNot just I can _understand_, but I think I could be pursuaded to agree\nwith that position, iff you try harder.\n\nBut the assumption/rationale behind this change needs to be spelled out.\nIn essence, we are sacrificing purity and correctness because we consider\nease of building external diff filter is more important.\n\nOr something like that.\n"},{"id":"108862","messageId":"7vzlfe9ynj.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"7v8wmybf06.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T00:41:52Z","receivedAt":"2009-03-22T00:41:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> On Sat, 21 Mar 2009, Junio C Hamano wrote:\n>>\n>>> Johannes Schindelin writes:\n>>> \n>>> > When preparing temporary files for an external diff, the files should \n>>> > be handled as if they were worktree files.\n>>> \n>>> I do not think so. convert_to_working_tree() aka \"smudge\" means you \n>>> would be feeding crap like $Id$ expansion to the external diff, which we \n>>> chose not to do quite on purpose.\n>>\n>> You might have missed me mentioning that we often can do without temporary \n>> files, taking the working directory copies right away?\n>>\n>> And if you think about it, it makes complete sense.\n>\n> Not \"complete\".\n>\n> What is at issue is how consistent the codepath that calls out to an\n> external diff should be with the rest of git that solely work with the\n> \"clean\" version of blob contents. If you value consistency, I would say\n> that it is valid to argue that letting it borrow from a work tree is a\n> bug. It should be always feeding a clean version.\n>\n> But if you think that we do not care really about the correctness of the\n> external diff codepath, because it is a mechanism used mostly for giving\n> output to humans (as opposed to feeding the output of the external diff\n> program back to programs to be processed further) , I can understand why\n> you think it is easier to the external programs if \"smudged\" version is\n> fed to them.\n>\n> Not just I can _understand_, but I think I could be pursuaded to agree\n> with that position, iff you try harder.\n>\n> But the assumption/rationale behind this change needs to be spelled out.\n> In essence, we are sacrificing purity and correctness because we consider\n> ease of building external diff filter is more important.\n>\n> Or something like that.\n\nThe situation is worse than I thought, and I am surely glad that this was\nbrought into my attention. I think diff.c::reuse_worktree_file() should\nalways say \"Don't\" when we know convert_to_working_tree() is not a no-op.\n\nI have a suspicion that:\n\n (1) borrowing from the work tree may not be buying us very much these\n days; the introduction of the logic predates not just \"clean/smudge\"\n feature but packfiles. Back then, the tradeoff was between opening a\n loose object file, deflating and writing out a temporary and reusing\n a file in the work tree. These days, most of the time the former\n would be to reconstruct a blob from the pack data that is already\n mapped, and performance characteristics would be different.\n\n (2) having to check if convert_to_working_tree() is a no-op or not may be\n a lot more costly than any performance gain we get from borrowing\n from the work tree.\n\nHere is a patch to get rid of the borrowing.\n\nYou can work on top of this patch to add the convert_to_working_tree()\ncall to prep_temp_blob(). Such a change would also affect the \"textconv\"\ncodepath and the result will start feeding smudged contents to the\n\"textconv\" filter, and I think the argument \"external tools prefer to be\nfed smudged contents, not clean ones, because that is the representation\ntailored for the platform\" would hold even more stronger in that context.\n\n diff.c | 69 ++-------------------------------------------------------------\n 1 files changed, 3 insertions(+), 66 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 75d9fab..4d71ebc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1746,67 +1746,6 @@ void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,\n \t}\n }\n \n-/*\n- * Given a name and sha1 pair, if the index tells us the file in\n- * the work tree has that object contents, return true, so that\n- * prepare_temp_file() does not have to inflate and extract.\n- */\n-static int reuse_worktree_file(const char *name, const unsigned char *sha1, int want_file)\n-{\n-\tstruct cache_entry *ce;\n-\tstruct stat st;\n-\tint pos, len;\n-\n-\t/* We do not read the cache ourselves here, because the\n-\t * benchmark with my previous version that always reads cache\n-\t * shows that it makes things worse for diff-tree comparing\n-\t * two linux-2.6 kernel trees in an already checked out work\n-\t * tree. This is because most diff-tree comparisons deal with\n-\t * only a small number of files, while reading the cache is\n-\t * expensive for a large project, and its cost outweighs the\n-\t * savings we get by not inflating the object to a temporary\n-\t * file. Practically, this code only helps when we are used\n-\t * by diff-cache --cached, which does read the cache before\n-\t * calling us.\n-\t */\n-\tif (!active_cache)\n-\t\treturn 0;\n-\n-\t/* We want to avoid the working directory if our caller\n-\t * doesn't need the data in a normal file, this system\n-\t * is rather slow with its stat/open/mmap/close syscalls,\n-\t * and the object is contained in a pack file. The pack\n-\t * is probably already open and will be faster to obtain\n-\t * the data through than the working directory. Loose\n-\t * objects however would tend to be slower as they need\n-\t * to be individually opened and inflated.\n-\t */\n-\tif (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))\n-\t\treturn 0;\n-\n-\tlen = strlen(name);\n-\tpos = cache_name_pos(name, len);\n-\tif (pos < 0)\n-\t\treturn 0;\n-\tce = active_cache[pos];\n-\n-\t/*\n-\t * This is not the sha1 we are looking for, or\n-\t * unreusable because it is not a regular file.\n-\t */\n-\tif (hashcmp(sha1, ce->sha1) || !S_ISREG(ce->ce_mode))\n-\t\treturn 0;\n-\n-\t/*\n-\t * If ce matches the file in the work tree, we can reuse it.\n-\t */\n-\tif (ce_uptodate(ce) ||\n-\t (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n-\t\treturn 1;\n-\n-\treturn 0;\n-}\n-\n static int populate_from_stdin(struct diff_filespec *s)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1861,8 +1800,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \tif (S_ISGITLINK(s->mode))\n \t\treturn diff_populate_gitlink(s, size_only);\n \n-\tif (!s->sha1_valid ||\n-\t reuse_worktree_file(s->path, s->sha1, 0)) {\n+\tif (!s->sha1_valid) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct stat st;\n \t\tint fd;\n@@ -1988,8 +1926,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\tremove_tempfile_installed = 1;\n \t}\n \n-\tif (!one->sha1_valid ||\n-\t reuse_worktree_file(name, one->sha1, 1)) {\n+\tif (!one->sha1_valid) {\n \t\tstruct stat st;\n \t\tif (lstat(name, &st) < 0) {\n \t\t\tif (errno == ENOENT)\n@@ -2011,7 +1948,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t\t\t\tone->mode : S_IFLNK));\n \t\t}\n \t\telse {\n-\t\t\t/* we can borrow from the file in the work tree */\n+\t\t\t/* we use the file in the work tree */\n \t\t\ttemp->name = name;\n \t\t\tif (!one->sha1_valid)\n \t\t\t\tstrcpy(temp->hex, sha1_to_hex(null_sha1));\n"},{"id":"108878","messageId":"20090322061046.GA14765@coredump.intra.peff.net","threadId":"18446","inReplyTo":"7vzlfe9ynj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-22T06:10:46Z","receivedAt":"2009-03-22T06:10:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2009 at 05:41:52PM -0700, Junio C Hamano wrote:\n\n> I have a suspicion that:\n> \n> (1) borrowing from the work tree may not be buying us very much these\n> days; the introduction of the logic predates not just \"clean/smudge\"\n> feature but packfiles. Back then, the tradeoff was between opening a\n> loose object file, deflating and writing out a temporary and reusing\n> a file in the work tree. These days, most of the time the former\n> would be to reconstruct a blob from the pack data that is already\n> mapped, and performance characteristics would be different.\n\nI don't think this is the case. We only reuse the worktree file when we\nhave already read the index, which means the main user is \"git diff\n--cached\". If a file's SHA1 is the same in the HEAD and the index, then\nwe don't need to run the diff at all. But if it is, then that implies\ncontent newly added for a commit, which is probably not in a packfile,\nbut rather a loose object.\n\nTry something like this with and without your patch:\n\n cd linux-2.6 &&\n git ls-files |\n while read f; do\n echo trivial change >>$f\n done\n git add -u\n git diff --cached\n\nI get about 10.5s with stock git, and 13.5s with your patch (actually,\nthe first run with without reuse_worktree_file is doubly painful -- it\ntouches the disk for all of the newly written loose objects).\n\nSo I think it does make a difference now. The question, though, is:\n\n> (2) having to check if convert_to_working_tree() is a no-op or not may be\n> a lot more costly than any performance gain we get from borrowing\n> from the work tree.\n\n...how expensive is the check for convert_to_working_tree? It should\njust be a gitattributes lookup, shouldn't it? Which:\n\n a. we are doing anyway and caching\n\n b. which takes a fraction of a second (try \"time git ls-files | git\n check-attr --stdin diff >/dev/null\", which should give a\n worst-case).\n\nTo be fair, though, the diff I outlined above is pretty ridiculous.\nWorktree reuse saved me three seconds, but out of 26683 files. Even if\nyou had 1000 files in your commit, that's still only .1s.\n\nWhere I think it _would_ make an impact is in crazy large-file\nrepositories. Where the I/O cost of dumping a 200MB avi into a tempfile\nis actually meaningful. But for that, even the current code is not very\ngood; it is optimized for many small files. For a few large files, you\nare probably better off reading the index from disk in order to make\nthat optimization for other cases (like tree to tree diff, where one of\nthe trees happens to have the same contents for the file as HEAD).\n\n> You can work on top of this patch to add the convert_to_working_tree()\n> call to prep_temp_blob(). Such a change would also affect the \"textconv\"\n> codepath and the result will start feeding smudged contents to the\n> \"textconv\" filter, and I think the argument \"external tools prefer to be\n> fed smudged contents, not clean ones, because that is the representation\n> tailored for the platform\" would hold even more stronger in that context.\n\nIn practice, I don't think it will affect textconv users much. The point\nof textconv is to munge ugly binary files into something humans can\nread. I'm not sure if people are actually smudging those (though I guess\npeople have talked about smuding openoffice files, so anything is\npossible...).\n\nAnyway, I was planning to make a patch to always feed textconv the\n_clean_ version of each file. My thinking was:\n\n 1. Then tools get a consistent view of the data across platforms.\n I.e., my textconv munger or external diff script will work no\n matter what you think the working tree should look like.\n\n 2. The tool may want the clean version, or it may want the smudged\n version. Or it may be able to operate on either. If we give it a\n format it doesn't like, it will have to undo whatever we did.\n\n For most cases, we start with the clean file (i.e., from a tree or\n from the index). If we hand out the clean file and the script\n doesn't like it, it pays the cost to smudge once. If we hand it the\n smudged file and the script doesn't like it, we pay the cost to\n smudge _and_ the script pays the cost to clean.\n\nBut I have to admit that I have never once used a clean/smudge filter in\nan actual project. So I am coming at this purely from a hypothetical\nposition. I do think the format that any scripts get should be\nconsistent (i.e., always clean or always smudged), and textconv and\nextdiff should use the same format.\n\nOne thing that I have considered that may make it moot is adding a\n\"fast\" extdiff/textconv interface: instead of writing temp files, call\nthe script with the quick information (like sha-1 and filename), and let\nit ask git for the data as appropriate.\n\nIn my case, I'm motivated by making textconv faster for my media repo;\nthe textconv script just displays the exif (and similar) metadata for\nthe files. So right now for each diffed file it writes out the entire\nlarge file to a tempfile, reads the exif data, and prints it. If it got\njust the sha-1 it could:\n\n - use its own caching layer (which my metadata reading tools already\n have) to avoid looking at the file at all, since it knows the\n textconv for a particular sha-1 is immutable\n\n - when it _does_ have to read, it can stream only as much data as it\n needs to get the metadata and never touch the disk at all. I.e., by\n piping from \"git cat-file\".\n\nGiven such an interface, it would also be trivial to ask cat-file for\nyour data with or without smudging, as appropriate. Which is not that\ndifferent from what you suggested earlier in the thread with \"git\nfilter\", except that instead of smudging the existing tempfile, we skip\nthe part where we write out the uninteresting bit. :)\n\nSo it's a little more work on the part of the script-writer (who now has\nto know about getting the data from git instead of just opening some\nfiles), but it can potentially be a lot faster. And of course I would\nretain the original interface both for historical reasons and because it\nis simpler to use; this would be diff.*.textconv-quick or similar.\n\n> diff.c | 69 ++-------------------------------------------------------------\n> 1 files changed, 3 insertions(+), 66 deletions(-)\n\nFor some reason, with your patch the tempfiles are created with mode\n0005 for me (whereas they are usually 0505), which makes open() in the\ncalled script unhappy. Looking over the patch text, though, I have no\nidea what change could be causing that.\n\n-Peff\n"},{"id":"108883","messageId":"7v63i281py.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"20090322061046.GA14765@coredump.intra.peff.net","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T07:18:33Z","receivedAt":"2009-03-22T07:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...how expensive is the check for convert_to_working_tree? It should\n> just be a gitattributes lookup, shouldn't it? Which:\n>\n> a. we are doing anyway and caching\n>\n> b. which takes a fraction of a second (try \"time git ls-files | git\n> check-attr --stdin diff >/dev/null\", which should give a\n> worst-case).\n\nOk. Although I already queued the removal to 'pu' for tonight's pushout\nand it is way too late to revert that, I think I didn't have to remove the\nfunction. The codepath that lets you cheat by borrowing from the checkout\nruns convert_to_git() when it borrows, and if you are seeing a meaningful\noptimization even with that overhead, perhaps it would be worth keeping.\n\nThere is another check that should be there but is missing from the\ncurrent implementation of reuse_worktree_file(), other than the \"is\nconvert_to_working_tree() a no-op for this path?\" check. The last check\nin the function would say \"Yeah, we can reuse it\" if the ce is marked\n\"assume unchanged\"; we do not want to blindly reuse the file from the work\ntree in that case.\n\n> Anyway, I was planning to make a patch to always feed textconv the\n> _clean_ version of each file. My thinking was:\n>\n> 1. Then tools get a consistent view of the data across platforms.\n> I.e., my textconv munger or external diff script will work no\n> matter what you think the working tree should look like.\n>\n> 2. The tool may want the clean version, or it may want the smudged\n> version. Or it may be able to operate on either. If we give it a\n> format it doesn't like, it will have to undo whatever we did.\n>\n> For most cases, we start with the clean file (i.e., from a tree or\n> from the index). If we hand out the clean file and the script\n> doesn't like it, it pays the cost to smudge once. If we hand it the\n> smudged file and the script doesn't like it, we pay the cost to\n> smudge _and_ the script pays the cost to clean.\n\nWhile the purist in me says #1 above is the right argument to make for\nfeeding \"clean\" version, I suspect that the textconv or extdiff tools more\noften are not made from scratch and ported across platforms than are\ncobbled up together out of tools the script writer finds on his platform.\nI suspect that Dscho's \"a tempfile should look like a checkout\" would be\nmuch friendlier to them in practice for this reason.\n\n> For some reason, with your patch the tempfiles are created with mode\n> 0005 for me (whereas they are usually 0505), which makes open() in the\n> called script unhappy. Looking over the patch text, though, I have no\n> idea what change could be causing that.\n\nNeither 0005 nor 0505 sounds correct to me; shouldn't they be 0600 or\nsomething like that?\n"},{"id":"108884","messageId":"20090322074643.GA4826@coredump.intra.peff.net","threadId":"18446","inReplyTo":"7v63i281py.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-22T07:46:43Z","receivedAt":"2009-03-22T07:46:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2009 at 12:18:33AM -0700, Junio C Hamano wrote:\n\n> Ok. Although I already queued the removal to 'pu' for tonight's pushout\n> and it is way too late to revert that, I think I didn't have to remove the\n> function. The codepath that lets you cheat by borrowing from the checkout\n> runs convert_to_git() when it borrows, and if you are seeing a meaningful\n> optimization even with that overhead, perhaps it would be worth keeping.\n\nI certainly haven't done exhaustive tests. Obviously the one I did was a\nbit contrived. I just think it makes sense to have numbers rather than\nsaying \"this probably doesn't do anything anymore\".\n\n> While the purist in me says #1 above is the right argument to make for\n> feeding \"clean\" version, I suspect that the textconv or extdiff tools more\n> often are not made from scratch and ported across platforms than are\n> cobbled up together out of tools the script writer finds on his platform.\n> I suspect that Dscho's \"a tempfile should look like a checkout\" would be\n> much friendlier to them in practice for this reason.\n\nI think you and I have about the same feeling on this, then. As somebody\nwho does not actually use smudge/clean filters at all, I am willing to\ndefer to Dscho's opinion, which is based on practical experience.\n\n> > For some reason, with your patch the tempfiles are created with mode\n> > 0005 for me (whereas they are usually 0505), which makes open() in the\n> > called script unhappy. Looking over the patch text, though, I have no\n> > idea what change could be causing that.\n> \n> Neither 0005 nor 0505 sounds correct to me; shouldn't they be 0600 or\n> something like that?\n\nArgh, sorry, of course I should have realized something was wrong with\nmy baseline when I saw 0505. I was building on a half-finished WIP\nrelated to textconv, which obviously is broken. Sorry for the noise.\n\n-Peff\n"},{"id":"108903","messageId":"bdca99240903220830u50999dfcnee0f0091b4dec835@mail.gmail.com","threadId":"18446","inReplyTo":"20090322074643.GA4826@coredump.intra.peff.net","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-03-22T15:30:58Z","receivedAt":"2009-03-22T15:30:58Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":">> While the purist in me says #1 above is the right argument to make for\n>> feeding \"clean\" version, I suspect that the textconv or extdiff tools more\n>> often are not made from scratch and ported across platforms than are\n>> cobbled up together out of tools the script writer finds on his platform.\n>> I suspect that Dscho's \"a tempfile should look like a checkout\" would be\n>> much friendlier to them in practice for this reason.\n>\n> I think you and I have about the same feeling on this, then. As somebody\n> who does not actually use smudge/clean filters at all, I am willing to\n> defer to Dscho's opinion, which is based on practical experience.\n\nMe being the reporter of the original msysGit issue #177, I'd like to\nclarify that my intention not necessarily was to make\n\"core.autocrlf=true\" affect temporary files (i.e. to \"smudge\" them),\nbut to ensure that the files fed into \"git diff\" are always generated\n/ acquired in a consistent way, so that they are in fact comparable.\nI'd also be happy with a solution that always feeds clean files into\n\"git diff\", although that would probably mean that we could not reuse\nworking tree files if \"core.autocrlf=true\" is set.\n\nMaybe it's a good idea to look at how gitk displays the diff, for an\norientation. If the diff gitk shows is based on smudged files, git\ndiff should probably also always be fed with smudged files, and if the\ndiff gitk shows is based on clean files, git diff should probably also\nalways be fed with clean files.\n\n-- \nSebastian Schuberth\n"},{"id":"108939","messageId":"7vljqxcj84.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"bdca99240903220830u50999dfcnee0f0091b4dec835@mail.gmail.com","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T21:59:07Z","receivedAt":"2009-03-22T21:59:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Me being the reporter of the original msysGit issue #177, I'd like to\n> clarify that my intention not necessarily was to make\n> \"core.autocrlf=true\" affect temporary files (i.e. to \"smudge\" them),\n> but to ensure that the files fed into \"git diff\" are always generated\n> / acquired in a consistent way, so that they are in fact comparable.\n\nThanks. I think everybody involved in the thread is in agreement with\nthat.\n\n> I'd also be happy with a solution that always feeds clean files into\n> \"git diff\", although that would probably mean that we could not reuse\n> working tree files if \"core.autocrlf=true\" is set.\n\nWhen we generate diff internally, even when we borrow from the work tree,\nwe clean it before using. See diff_populate_filespec(), ll.1900-1915.\n\nBorrowing done by diff_tempfile(), which currently does not run clean, and\nthe call to prep_temp_blob() in ll.2030-2035 that gives a temporary file\nwithout convert_to_working_tree() are inconsistent, as pointed out by you\nand Dscho.\n\nIf you run \"git diff \" after cloning, I expect that no temporary\nfiles are involved, _unless_ you have some settings that force \"git diff\"\nnot to use the internal diff. Do you use GIT_EXTERNAL_DIFF? Do you use\n\"textconv\" attribute? What external program do you invoke from these\nmechanisms, and what does it expect to see as its input?\n\nThe discussion in the last few messages in this thread speculates that the\nexternal programs are more likely to expect representations suitable in\nthe work tree, aka \"smudged\", than \"clean\" one. It would be nice to get a\ndatapoint from you as the original reporter to confirm or refute that\nspeculation.\n"},{"id":"108948","messageId":"bdca99240903221548y38f31177q6abb88c2f7f87a73@mail.gmail.com","threadId":"18446","inReplyTo":"7vljqxcj84.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-03-22T22:48:20Z","receivedAt":"2009-03-22T22:48:20Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"> If you run \"git diff \" after cloning, I expect that no temporary\n> files are involved, _unless_ you have some settings that force \"git diff\"\n> not to use the internal diff.  Do you use GIT_EXTERNAL_DIFF?  Do you use\n> \"textconv\" attribute?  What external program do you invoke from these\n> mechanisms, and what does it expect to see as its input?\n\nAs I prefer graphical diff tools, I do not use internal diff, but\nBeyond Compare [1]. In order to do that I have configured\ndiff.external to point to a wrapper script that contains the following\nlines:\n\n---8<---\n\n#!/bin/bash\n\n# diff is called with 7 parameters:\n# path old-file old-hex old-mode new-file new-hex new-mode\n\n\"C:/Program Files/Beyond Compare 3/BCompare.exe\" \"$2\" \"$5\" | cat\n\n---8<---\n\nAs Beyond Compare is a stand-alone diff / merge tool, it expects to be\nworking on regular files in the file system. And to be hostest, I did\nnot know about the \"textconv\" attribute until now.\n\n> The discussion in the last few messages in this thread speculates that the\n> external programs are more likely to expect representations suitable in\n> the work tree, aka \"smudged\", than \"clean\" one.  It would be nice to get a\n> datapoint from you as the original reporter to confirm or refute that\n> speculation.\n\nI agree to the speculations. IMHO calling an external diff tool with\ntwo revisions of a file should result in the same as e.g. checking out\nthe two revisions in two different working trees and then launching\nthe user's external diff tool on the two working tree files.\n\n[1] http://www.scootersoftware.com/\n\n-- \nSebastian Schuberth\n"},{"id":"108955","messageId":"7vwsahb19z.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"bdca99240903221548y38f31177q6abb88c2f7f87a73@mail.gmail.com","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T23:12:08Z","receivedAt":"2009-03-22T23:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n>> The discussion in the last few messages in this thread speculates that the\n>> external programs are more likely to expect representations suitable in\n>> the work tree, aka \"smudged\", than \"clean\" one.  It would be nice to get a\n>> datapoint from you as the original reporter to confirm or refute that\n>> speculation.\n>\n> I agree to the speculations. IMHO calling an external diff tool with\n> two revisions of a file should result in the same as e.g. checking out\n> the two revisions in two different working trees and then launching\n> the user's external diff tool on the two working tree files.\n\nOk.\n"},{"id":"108970","messageId":"7vljqx9ing.fsf@gitster.siamese.dyndns.org","threadId":"18446","inReplyTo":"20090322074643.GA4826@coredump.intra.peff.net","subject":"Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-23T00:39:47Z","receivedAt":"2009-03-23T00:39:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Mar 22, 2009 at 12:18:33AM -0700, Junio C Hamano wrote:\n>\n>> Ok. Although I already queued the removal to 'pu' for tonight's pushout\n>> and it is way too late to revert that, I think I didn't have to remove the\n>> function. The codepath that lets you cheat by borrowing from the checkout\n>> runs convert_to_git() when it borrows, and if you are seeing a meaningful\n>> optimization even with that overhead, perhaps it would be worth keeping.\n>\n> I certainly haven't done exhaustive tests. Obviously the one I did was a\n> bit contrived. I just think it makes sense to have numbers rather than\n> saying \"this probably doesn't do anything anymore\".\n>\n>> While the purist in me says #1 above is the right argument to make for\n>> feeding \"clean\" version, I suspect that the textconv or extdiff tools more\n>> often are not made from scratch and ported across platforms than are\n>> cobbled up together out of tools the script writer finds on his platform.\n>> I suspect that Dscho's \"a tempfile should look like a checkout\" would be\n>> much friendlier to them in practice for this reason.\n>\n> I think you and I have about the same feeling on this, then. As somebody\n> who does not actually use smudge/clean filters at all, I am willing to\n> defer to Dscho's opinion, which is based on practical experience.\n\nThanks for a sanity check.\n\nI've split these as two unrelated issues, and have queued\n\n (1) Dscho's patch, that always feeds smudged representation to the\n external diff and textconv filter;\n\n (2) A change to reuse_worktree_file() that says \"don't reuse\" for paths\n with CE_VALID set.\n\nin 'pu'.\n"}]}