Re: Using clean/smudge filters with difftool

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 19.06.2015 19:03:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Now, since external diff runs on smudged blobs, it appears as if we
>> mixed cleaned and smudged blobs when feeding external diffs; whereas
>> really, we mix "worktree blobs" and "smudged repo blobs", which is okay
>> as per our definition of clean/smudge: the difference is irrelevant by
>> definition.
> 
> It does not appear to "mix cleaned and smudged" to me (even though
> before Dscho's commit that John pointed out, we did mix by mistake)
> to me,

... neither to me. I appears as if you missed the past subjunctive ;)

> but you arrived at the correct conclusion in the rest of your
> sentence.

> We treat "worktree files" and "smudged repo blobs" as "comparable"
> because by definition the latter is what you get if you did a
> "checkout" of the blob.  Indeed, when we know a worktree file is an
> unmodified checkout from a blob and we want to have a read-only
> temporary file for a "smudged repo blob", we allow that worktree
> file to be used as such.
> 
> So in that sense, the commit by Dscho that John pointed out earlier
> was not something that changed the semantics; it merely made things
> consistent (before that commit, we used to use clean version if we
> do not have a usable worktree file).
> 
> It is a separate question which of clean or smudged an external diff
> tool should be given to work on.
> 
>> I still think that feeding cleaned blobs to external diff would be less
>> surprising (and should be the default, but maybe can't be changed any
>> more) and feeding smudged blobs should be the special case requiring a
>> special config.
> 
> Go back six years and make a review comment before 4e218f54 (Smudge
> the files fed to external diff and textconv, 2009-03-21) was taken
> ;-).  The argument against that commit may have gone like this:
> 
>  * The current (that is, current as of 4e218f54^) code is
>    inconsistent, and your patch has a side effect of making it
>    consistent by always feeding smudged version.
> 
>  * We however could make it consistent by always feeding clean
>    version (i.e. disable borrow-from-working-tree codepath when
>    driving external diff).  And that gives us cleaner semantics; the
>    internal diff and external diff will both work on clean, not
>    smudged data.
> 
>  * Of course, going the "clean" way would not help your cause of
>    allowing external diff to work on smudged version, so you would
>    need a separate patch on top of that "consistently feed 'clean'
>    version" fix to optionally allow "consistently feed 'smudge'
>    version" mode to help msysGit issue 177.
> 
> And I would have bought such an argument with 97% chance [*1*].
> 
> I do not think 6 years have changed things very much with respect to
> the above three-bullet point argument, except that it would be too
> late to set the default to 'clean' all of a sudden.  So a plausible
> way forward would be to
> 
>  * introduce an option to feed 'clean' versions to external diff
>    drivers, perhaps with --ext-diff-clean=<driver> command line
>    option and GIT_EXTERNAL_DIFF_CLEAN environment variable, both of
>    which take precedence over existing --ext-diff/GIT_EXTERNAL_DIFF
> 
>  * optionally add a configuration variable diff.feedCleanToExternal
>    that makes --ext-diff/GIT_EXTERNAL_DIFF behave as if their
>    'clean' siblings were given.  Default it to false.
> 
> My gut feeling is that textconv should need a similar treatment for
> consistency (after all, it goes through the same prepare_temp_file()
> infrastructure).
> 
> 
> [Footnote]
> 
> *1* The 3% reservation is that I am not entirely convinced that
> "both internal and external get to work on the same 'clean'
> representation gives us cleaner semantics" is always true.

With consistency stepping back behind compatibility, I don't expect any
defaults to change.

But a knob to change defaults would be nice, yes, and in that case for
external diff as well as textconv. A config variable should suffice
given that we have "git -c" these days.

Michael