Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff

Jeff King [off-list ref] writes:

...how expensive is the check for convert_to_working_tree? It should
just be a gitattributes lookup, shouldn't it? Which:

  a. we are doing anyway and caching

  b. which takes a fraction of a second (try "time git ls-files | git
     check-attr --stdin diff >/dev/null", which should give a
     worst-case).

Ok.  Although I already queued the removal to 'pu' for tonight's pushout
and it is way too late to revert that, I think I didn't have to remove the
function.  The codepath that lets you cheat by borrowing from the checkout
runs convert_to_git() when it borrows, and if you are seeing a meaningful
optimization even with that overhead, perhaps it would be worth keeping.

There is another check that should be there but is missing from the
current implementation of reuse_worktree_file(), other than the "is
convert_to_working_tree() a no-op for this path?" check.  The last check
in the function would say "Yeah, we can reuse it" if the ce is marked
"assume unchanged"; we do not want to blindly reuse the file from the work
tree in that case.

Anyway, I was planning to make a patch to always feed textconv the
_clean_ version of each file. My thinking was:

  1. Then tools get a consistent view of the data across platforms.
     I.e., my textconv munger or external diff script will work no
     matter what you think the working tree should look like.

  2. The tool may want the clean version, or it may want the smudged
     version. Or it may be able to operate on either. If we give it a
     format it doesn't like, it will have to undo whatever we did.

     For most cases, we start with the clean file (i.e., from a tree or
     from the index).  If we hand out the clean file and the script
     doesn't like it, it pays the cost to smudge once. If we hand it the
     smudged file and the script doesn't like it, we pay the cost to
     smudge _and_ the script pays the cost to clean.

While the purist in me says #1 above is the right argument to make for
feeding "clean" version, I suspect that the textconv or extdiff tools more
often are not made from scratch and ported across platforms than are
cobbled up together out of tools the script writer finds on his platform.
I suspect that Dscho's "a tempfile should look like a checkout" would be
much friendlier to them in practice for this reason.

For some reason, with your patch the tempfiles are created with mode
0005 for me (whereas they are usually 0505), which makes open() in the
called script unhappy.  Looking over the patch text, though, I have no
idea what change could be causing that.

Neither 0005 nor 0505 sounds correct to me; shouldn't they be 0600 or
something like that?

On Sun, Mar 22, 2009 at 12:18:33AM -0700, Junio C Hamano wrote:

Ok.  Although I already queued the removal to 'pu' for tonight's pushout
and it is way too late to revert that, I think I didn't have to remove the
function.  The codepath that lets you cheat by borrowing from the checkout
runs convert_to_git() when it borrows, and if you are seeing a meaningful
optimization even with that overhead, perhaps it would be worth keeping.

I certainly haven't done exhaustive tests. Obviously the one I did was a
bit contrived. I just think it makes sense to have numbers rather than
saying "this probably doesn't do anything anymore".

While the purist in me says #1 above is the right argument to make for
feeding "clean" version, I suspect that the textconv or extdiff tools more
often are not made from scratch and ported across platforms than are
cobbled up together out of tools the script writer finds on his platform.
I suspect that Dscho's "a tempfile should look like a checkout" would be
much friendlier to them in practice for this reason.

I think you and I have about the same feeling on this, then. As somebody
who does not actually use smudge/clean filters at all, I am willing to
defer to Dscho's opinion, which is based on practical experience.

quoted

For some reason, with your patch the tempfiles are created with mode
0005 for me (whereas they are usually 0505), which makes open() in the
called script unhappy.  Looking over the patch text, though, I have no
idea what change could be causing that.

Neither 0005 nor 0505 sounds correct to me; shouldn't they be 0600 or
something like that?

Argh, sorry, of course I should have realized something was wrong with
my baseline when I saw 0505. I was building on a half-finished WIP
related to textconv, which obviously is broken. Sorry for the noise.

-Peff

quoted

While the purist in me says #1 above is the right argument to make for
feeding "clean" version, I suspect that the textconv or extdiff tools more
often are not made from scratch and ported across platforms than are
cobbled up together out of tools the script writer finds on his platform.
I suspect that Dscho's "a tempfile should look like a checkout" would be
much friendlier to them in practice for this reason.

I think you and I have about the same feeling on this, then. As somebody
who does not actually use smudge/clean filters at all, I am willing to
defer to Dscho's opinion, which is based on practical experience.

Me being the reporter of the original msysGit issue #177, I'd like to
clarify that my intention not necessarily was to make
"core.autocrlf=true" affect temporary files (i.e. to "smudge" them),
but to ensure that the files fed into "git diff" are always generated
/ acquired in a consistent way, so that they are in fact comparable.
I'd also be happy with a solution that always feeds clean files into
"git diff", although that would probably mean that we could not reuse
working tree files if "core.autocrlf=true" is set.

Maybe it's a good idea to look at how gitk displays the diff, for an
orientation. If the diff gitk shows is based on smudged files, git
diff should probably also always be fed with smudged files, and if the
diff gitk shows is based on clean files, git diff should probably also
always be fed with clean files.

-- 
Sebastian Schuberth

Sebastian Schuberth [off-list ref] writes:

Me being the reporter of the original msysGit issue #177, I'd like to
clarify that my intention not necessarily was to make
"core.autocrlf=true" affect temporary files (i.e. to "smudge" them),
but to ensure that the files fed into "git diff" are always generated
/ acquired in a consistent way, so that they are in fact comparable.

Thanks.  I think everybody involved in the thread is in agreement with
that.

I'd also be happy with a solution that always feeds clean files into
"git diff", although that would probably mean that we could not reuse
working tree files if "core.autocrlf=true" is set.

When we generate diff internally, even when we borrow from the work tree,
we clean it before using.  See diff_populate_filespec(), ll.1900-1915.

Borrowing done by diff_tempfile(), which currently does not run clean, and
the call to prep_temp_blob() in ll.2030-2035 that gives a temporary file
without convert_to_working_tree() are inconsistent, as pointed out by you
and Dscho.

If you run "git diff <filename>" after cloning, I expect that no temporary
files are involved, _unless_ you have some settings that force "git diff"
not to use the internal diff.  Do you use GIT_EXTERNAL_DIFF?  Do you use
"textconv" attribute?  What external program do you invoke from these
mechanisms, and what does it expect to see as its input?

The discussion in the last few messages in this thread speculates that the
external programs are more likely to expect representations suitable in
the work tree, aka "smudged", than "clean" one.  It would be nice to get a
datapoint from you as the original reporter to confirm or refute that
speculation.

If you run "git diff <filename>" after cloning, I expect that no temporary
files are involved, _unless_ you have some settings that force "git diff"
not to use the internal diff.  Do you use GIT_EXTERNAL_DIFF?  Do you use
"textconv" attribute?  What external program do you invoke from these
mechanisms, and what does it expect to see as its input?

As I prefer graphical diff tools, I do not use internal diff, but
Beyond Compare [1]. In order to do that I have configured
diff.external to point to a wrapper script that contains the following
lines:

---8<---

#!/bin/bash

# diff is called with 7 parameters:
# path old-file old-hex old-mode new-file new-hex new-mode

"C:/Program Files/Beyond Compare 3/BCompare.exe" "$2" "$5" | cat

---8<---

As Beyond Compare is a stand-alone diff / merge tool, it expects to be
working on regular files in the file system. And to be hostest, I did
not know about the "textconv" attribute until now.

The discussion in the last few messages in this thread speculates that the
external programs are more likely to expect representations suitable in
the work tree, aka "smudged", than "clean" one.  It would be nice to get a
datapoint from you as the original reporter to confirm or refute that
speculation.

I agree to the speculations. IMHO calling an external diff tool with
two revisions of a file should result in the same as e.g. checking out
the two revisions in two different working trees and then launching
the user's external diff tool on the two working tree files.

[1] http://www.scootersoftware.com/

-- 
Sebastian Schuberth

Jeff King [off-list ref] writes:

On Sun, Mar 22, 2009 at 12:18:33AM -0700, Junio C Hamano wrote:

quoted

Ok.  Although I already queued the removal to 'pu' for tonight's pushout
and it is way too late to revert that, I think I didn't have to remove the
function.  The codepath that lets you cheat by borrowing from the checkout
runs convert_to_git() when it borrows, and if you are seeing a meaningful
optimization even with that overhead, perhaps it would be worth keeping.

I certainly haven't done exhaustive tests. Obviously the one I did was a
bit contrived. I just think it makes sense to have numbers rather than
saying "this probably doesn't do anything anymore".

quoted

While the purist in me says #1 above is the right argument to make for
feeding "clean" version, I suspect that the textconv or extdiff tools more
often are not made from scratch and ported across platforms than are
cobbled up together out of tools the script writer finds on his platform.
I suspect that Dscho's "a tempfile should look like a checkout" would be
much friendlier to them in practice for this reason.

I think you and I have about the same feeling on this, then. As somebody
who does not actually use smudge/clean filters at all, I am willing to
defer to Dscho's opinion, which is based on practical experience.

Thanks for a sanity check.

I've split these as two unrelated issues, and have queued

 (1) Dscho's patch, that always feeds smudged representation to the
     external diff and textconv filter;

 (2) A change to reuse_worktree_file() that says "don't reuse" for paths
     with CE_VALID set.

in 'pu'.